Abort superseded watch starts before they queue any uploads

[cohogan]

Problem

Follow-up to the resume-watching PR. A start_watching that got superseded mid-scan (by stop_watching or a newer start) had already enqueued the entire folder into the upload queue — the generation check only prevented the watcher from installing, not the side effects. Worst case: a user picks a huge folder by mistake, hits stop during the initial scan, and the app uploads everything anyway. There was also a brief window where the new watcher was live before the commit check, so events for the abandoned folder could slip into the queue, and a theoretical race where a stale start could tear down a watcher a newer request had just installed.

Fix

• Collect-then-commit: the initial scan now only collects file paths. Enqueueing happens after the commit point (watcher installed under the generation check), so a superseded start queues nothing.
• Per-entry supersede check in the scan, so a stop during a large scan takes effect almost immediately instead of after the full walk.
• Generation-checked drain task: watcher events that race in around a stop/newer start are dropped instead of queued.
• Generation check before stopping the existing watcher (under the watcher lock), closing the race where a stale start could destroy the winner's watcher.
• Enqueueing after commit also aborts mid-way if superseded.

Tests

New tests/watch_supersede.rs (10 tests, all passing) driving the real start_watching_impl with Tauri's MockRuntime: normal start commits and queues (ignore patterns respected), stale starts abort with zero side effects and don't disturb the current watcher, stop-superseded starts leave the queue empty, plus a threaded race test. The touched functions are now generic over tauri::Runtime and exposed #[doc(hidden)] pub to make this possible.

Note: tests are an integration-test target rather than #[cfg(test)] unit tests because on Windows only integration test targets get the Common Controls v6 manifest from build.rs — unit test binaries crash at load with STATUS_ENTRYPOINT_NOT_FOUND (rustc-link-arg-tests doesn't apply to them).

cargo test, cargo check, and cargo clippy --all-targets are all clean.

🤖 Generated with Claude Code

[cursor]

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

[github-actions]

Claude finished @cohogan's task in 2m 30s —— View job

---

Code Review: Abort superseded watch starts before they queue any uploads

• Read changed files and understand the diff
• Review lib.rs changes
• Review upload.rs changes
• Review watch_supersede.rs tests
• Post review findings

---

This is a well-designed fix for a real and potentially painful bug (user picks wrong folder, hits stop, app uploads everything anyway). The "collect-then-commit" pattern is clean, and the per-entry supersede checks make cancellation responsive. Here's my detailed review:

Architecture

The three-phase approach — scan, commit, enqueue — is the right design. Separating data collection from side effects (queueing) means a superseded start leaves no trace. The generation counter with SeqCst ordering is a simple, correct concurrency primitive for this use case.

Particularly well done:

• Generation check under the watcher lock before teardown (lib.rs:146) closes the race where a stale start could destroy a newer watcher
• The drain task's generation check (lib.rs:186) catches events that race in around stop/restart
• The is_superseded closure keeps check-site code readable and consistent

Potential Concerns

1. Partial enqueue after commit is invisible to the caller (lib.rs:260-268)

After the commit point, enqueue_scanned_files can be superseded mid-way, but start_watching_impl still returns WatchStartOutcome::Started. The caller (and the user) won't know that only a subset of files were queued. This seems intentional — the watch IS started and the next start will re-scan — but it's worth calling out. If you wanted, you could return a variant like StartedPartial, but I'd agree it's not worth the complexity.

2. One-event race in the drain task (lib.rs:180-190)

The generation check is at the top of the loop, so if a supersede happens while add_to_upload_queue_sync is executing, one extra file slips through before the next iteration checks and breaks. Completely negligible in practice — just noting it for completeness.

3. stop_watching doesn't clear the queue (lib.rs:349-371)

If the commit succeeds and some files are enqueued before the enqueue is superseded by a stop, those files remain in the upload queue and will be uploaded. This was also true before this PR (so no regression), and likely intentional, but worth documenting if it isn't already.

upload.rs Changes

Minimal and correct — just widening three function signatures from &AppHandle to &AppHandle<R> where R: Runtime to support MockRuntime in tests. No behavioral changes.

Tests

The test suite is thorough and well-structured. Good coverage of:

• Scan-only (no queue side effects)
• Immediate and mid-walk supersede during scan
• Enqueue filtering (ignore patterns) and mid-enqueue supersede
• Full start_watching_impl happy path
• Stale start with non-existent folder (would error if it reached the scan — clever)
• Stale start not disturbing an existing watcher
• Stop-superseded start leaving clean state
• Non-deterministic race test with correct dual-outcome assertion (lib.rs test at line 333)

The superseded_after(n) helper is a nice pattern for deterministic supersede timing.

One gap: there's no test for the drain task's generation check (watcher events being dropped after supersede). This would require async test infrastructure and is harder to test with MockRuntime, so it's understandable to skip.

Minor Notes

• #[doc(hidden)] pub is the right call for exposing internals to integration tests in an application crate. Since this isn't a published library, the widened API surface has no downstream impact.
• The TempDir wrapper avoids adding a tempfile dependency — reasonable given the small scope.
• The comment explaining why tests are integration-test targets (Windows Common Controls v6 manifest) is a great example of a comment that earns its keep.

Verdict

Looks good. The concurrency model is sound, the separation of scan/commit/enqueue cleanly addresses the root cause, and the tests cover the important race scenarios. No blocking issues.

[github-actions]

Claude finished @cohogan's task in 1m 16s —— View job

---

Code Review: Abort superseded watch starts before they queue any uploads

• Read changed files and understand the diff
• Review lib.rs changes (core logic)
• Review upload.rs changes
• Review watch_supersede.rs tests
• Post review findings

---

Architecture

The three-phase approach — scan → commit → enqueue — is a clean design that eliminates the root cause: a superseded start can no longer leave orphaned uploads in the queue. The is_superseded closure keeps the generation checks readable and consistent across all three phases.

Key design wins:

• Generation check under the watcher lock before teardown (lib.rs:144-148) closes the race where a stale start could destroy a newer watcher
• Per-entry supersede check in scan_folder_contents (lib.rs:291-293) makes cancellation responsive during large folder walks
• The drain task's generation check (lib.rs:186) drops events that race in around stop/restart

Findings

1. start_watching_impl returns Started even when enqueue was partially superseded — lib.rs:269

After the commit point at line 244, enqueue_scanned_files can be superseded mid-way, but start_watching_impl still returns Ok(WatchStartOutcome::Started). The watcher IS installed and the watched folder IS recorded, so Started is technically accurate, but the caller has no visibility into whether 10% or 100% of the scanned files were queued. This is likely fine since the next start will re-scan, but worth noting.

2. One-event slip in the drain task — lib.rs:180-189

The generation check is at the top of the while loop. If a supersede happens while add_to_upload_queue_sync is executing on line 189, one extra file gets queued before the next iteration detects the change and breaks. Negligible in practice — just noting for completeness.

3. stop_watching doesn't drain already-queued files — lib.rs:349-371

If the commit succeeds and some files are enqueued before enqueue_scanned_files is superseded by a stop, those files remain in the upload queue. This was also true before this PR (not a regression), and is likely intentional behavior, but documenting this explicitly could prevent future confusion.

upload.rs Changes

Minimal and mechanical — three function signatures widened from &AppHandle to &AppHandle<R> where R: Runtime to support MockRuntime in tests. No behavioral changes.

Test Suite

The 10 tests are thorough and well-structured:

• scan_collects_files_recursively_without_queueing — confirms scan-only has no queue side effects
• scan_aborts_when_superseded_immediately / _mid_walk — per-entry supersede check
• enqueue_queues_files_and_respects_ignore_patterns — filtering at enqueue time (not scan time)
• enqueue_aborts_when_superseded_midway — partial enqueue on supersede
• start_watching_impl_starts_queues_and_records_state — full happy path
• stale_start_is_superseded_before_any_side_effects — stale start with non-existent folder (would error if it reached the scan — clever)
• stale_start_does_not_disturb_current_watcher — stale start leaves existing watcher intact
• start_superseded_by_stop_leaves_queue_empty_and_no_watcher — stop wins the race
• stop_arriving_mid_scan_aborts_start_without_queueing — threaded race with correct dual-outcome assertion

The superseded_after(n) helper is a nice pattern for deterministic supersede timing. The TempDir wrapper avoids adding a tempfile dev-dependency — reasonable for this scope.

Test gap: No test for the drain task's generation check (watcher events dropped after supersede). This would require async test infrastructure and is understandably hard with MockRuntime, so fine to skip.

Minor Notes

• #[doc(hidden)] pub is the right approach for exposing internals to integration tests in an application crate — since this isn't a published library, no downstream API surface concern.
• The comment explaining why tests use an integration test target (Windows Common Controls v6 manifest) earns its keep.

Verdict

Looks good. The concurrency model is sound, the collect-then-commit pattern cleanly addresses the root cause, and the test suite covers the important race scenarios. No blocking issues — the notes above are informational.