fix(filesystem): recover reliably after dropped filesystem events#489
fix(filesystem): recover reliably after dropped filesystem events#489morluto wants to merge 12 commits into
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| if let stream = fseventStreamRef { | ||
| nextFSEventStreamStartEventID = max( | ||
| nextFSEventStreamStartEventID, | ||
| FSEventStreamGetLatestEventId(stream) | ||
| ) | ||
| FSEventStreamStop(stream) | ||
| FSEventStreamFlushSync(stream) | ||
| FSEventStreamInvalidate(stream) | ||
| // Drain callbacks that were already submitted before releasing their | ||
| // unretained context pointer. | ||
| fseventCallbackQueue.sync {} | ||
| deactivateFSEventCallbackContext() | ||
| FSEventStreamRelease(stream) | ||
| fseventStreamRef = nil | ||
|
|
There was a problem hiding this comment.
Bug: Stopping and restarting the FSEvent stream can cause event replay. In-flight events may be discarded without updating nextFSEventStreamStartEventID, leading to double-delivery on restart.
Severity: HIGH
Suggested Fix
Before stopping the stream in stopFSEventStream(), reintroduce the call to capture the latest event ID from the stream itself using FSEventStreamGetLatestEventId(stream). Use this value to update nextFSEventStreamStartEventID, ensuring that even if in-flight events are dropped, the stream restarts from a point after those events, preventing replay.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location:
Sources/RepoPrompt/Infrastructure/FileSystem/FileSystemService+FSEvents.swift#L431-L443
Potential issue: The removal of `FSEventStreamGetLatestEventId()` during stream shutdown
creates a race condition. If FSEvent callbacks are in-flight when `stopFSEventStream()`
is called, they can be discarded before their event IDs are used to update the
`nextFSEventStreamStartEventID` watermark. Upon restarting, the stream uses this stale
watermark, causing it to replay events that were already seen but not fully processed.
This results in the double-delivery of events, which could lead to data corruption or
redundant processing.
Did we get this right? 👍 / 👎 to inform future reviews.
| } | ||
| watcherRecoveryDiagnostics.recordRecoveryEpisode( | ||
| evidence: recoveryEvidence, | ||
| outcome: .fullResyncFailed, | ||
| acceptedHighWatermark: batch.watcherAcceptedHighWatermark, | ||
| servicePublicationSequence: nil | ||
| ) | ||
| return testMode ? [] : nil | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Bug: The actor is blocked during the recovery path in reconcileUncertainWatcherBatch because rebuildPerFolderIgnoreCache is awaited directly, unlike in the non-recovery path where it runs in a background Task.
Severity: MEDIUM
Suggested Fix
In reconcileUncertainWatcherBatch, wrap the await rebuildPerFolderIgnoreCache(...) call in a Task { ... } block. This will make its behavior consistent with the non-recovery path in handleBatchedEvents and prevent the actor from being blocked during ignore cache rebuilds in the recovery flow.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location:
Sources/RepoPrompt/Infrastructure/FileSystem/FileSystemService+FSEvents.swift#L1921-L2025
Potential issue: In the recovery path function `reconcileUncertainWatcherBatch`, the
call to `rebuildPerFolderIgnoreCache` is awaited directly, which blocks the actor. This
is inconsistent with the normal event handling path in `handleBatchedEvents`, where the
same call is wrapped in a detached `Task` to prevent blocking. Blocking the actor during
recovery, which is already a performance-sensitive operation, can delay the processing
of subsequent file system events and degrade responsiveness.
Did we get this right? 👍 / 👎 to inform future reviews.
baron
left a comment
baron
left a comment
There was a problem hiding this comment.
Intent Analysis
Problem: FSEvents recovery flags and callback bursts can leave workspace inventory uncertain while freshness watermarks advance, and callback filtering runs on the main queue — the confirmed vulnerable recovery path behind issue #465 (the initiating FSEvent itself was never captured, per the issue).
Before → After: Main-queue, unbounded callback processing → serial utility queue with a 4,096-entry cap plus recovery sentinel. Recovery flags routed through ordinary scan machinery with unbounded retries → classification before path mapping, full-tree reconciliation before freshness publication, single-flight coalesced recovery episodes, bounded retry then quiescence. Weak teardown isolation → watcherIngressGeneration fencing around scans and publication. Full resyncs now trigger .watcherGap codemap invalidation with bounded in-memory diagnostics.
Evidence: Two independent code-first reviews of FileSystemService+FSEvents.swift (~497–625, 782–834, 1022–1046, 1946–2023), FileSystemService+DirectoryEnumeration.swift (~213–285, 487–559, 1379–1403), and the 12 new regression tests (FileSystemServiceRecoveryTests.swift, FileSystemAcceptedIngressBarrierTests.swift). Both converged on the same mechanism map.
Confidence: High that the behavioral change is implemented as described; Medium that it resolves the original CPU incident, since #465 records the trigger as unconfirmed (scoping caveat, not a defect).
Red Flags: Large concurrency-sensitive state-machine change (14 files, +1,630/−107) spanning callback lifecycle, ignore authority, inventory, freshness, and codemap invalidation. Seven of twelve new tests appear absent from the curated contract ledger. Quiescence is not observable in production. CI is now effectively green (shard 3 pending, Seer neutral), but the new tests do not exercise the failing interleavings below, so green CI does not offset the findings.
Findings
1. (P1) Truncated oversized callback can lose an ignore-control-file deletion and publish false freshness. Callbacks over 4,096 entries retain only the prefix; the tail collapses into a pathless recovery sentinel (FileSystemService+FSEvents.swift:497–550). Recovery invalidates ignore caches only for control paths preserved in the retained payload (:1954–1961). A .gitignore/.repo_ignore/.cursorignore deletion after entry 4,096 loses its identity; re-enumeration can reuse stale cached rules, omit newly unignored content, and still publish requiresFullResync with the accepted watermark (:1964–1994). The large-callback test (FileSystemAcceptedIngressBarrierTests.swift:39–53) covers only ordinary created files and sentinel shape. Required: on truncation, conservatively invalidate all relevant ignore-rule caches (or scan the discarded tail for control/recovery metadata only) before reconciliation; add a >4,096-entry regression with a trailing ignore-file deletion. Both independent reviews found this with identical references.
2. (P1) Old-generation work can leak into the successor generation's canonical state — one incomplete fence, two mechanisms:
- (a)
scanFoldersInParallelcommitsvisitedPaths/visitedItemsincrementally per child result (FileSystemService+DirectoryEnumeration.swift:213–276); when reset increments the generation mid-scan, the abort path deliberately skips restoration (:282–285), leaving pre-reset partial results resident in the new generation — phantom workspace/search/codemap inventory. - (b) Enumeration merges directory-level ignore decisions into actor-owned
ignoreCacheStorewithout generation fencing (:1379–1403); the generation check occurs only aftergatherPathsUsingEnumeratorreturns (:487–506, 552–559), andresetWatcherIngressState()cancels without awaiting the batch (FileSystemService+FSEvents.swift:1022–1046), so a stale task can install old ignore decisions even though its final inventory installation is rejected.
The restart test (FileSystemServiceRecoveryTests.swift:500–536) blocks all enumerations before any result is applied, so it misses both interleavings. Required: generation-local staging with an atomic post-generation-check commit for inventory results and ignore-cache merges (or generation-fence each mutation); add a restart test that applies at least one child result before reset, with an ignore-policy change in between.
3. (P2) Quiescence can indefinitely suspend production freshness barriers. After five failures recovery goes quiescent (FileSystemService+FSEvents.swift:782–805); flushPendingEventsNow parks on a continuation until a new filesystem event or explicit requestWatcherRecovery() (:287–313, 820–834), which has no production caller. A scoped ingress barrier can remain suspended indefinitely after the error clears with no user-facing signal. Required: propagate a terminal/retryable state to freshness callers or add an owned timeout/reactivation path.
4. (P2, process) Contract-ledger registration gap. Seven of twelve new tests appear unregistered in the curated contract ledger; repo guidance (docs/testing.md) makes ledger maintenance mandatory for test adds. Run verify-ledger plus the authoritative test list and register before merge.
Non-blocking: both Sentry comments are correctly non-blocking — restart replay is intentional given the mailbox-discard design, and await rebuildPerFolderIgnoreCache yields the actor rather than monopolizing it.
Maintainer-guidance check
- Invariant: A published-fresh watermark must represent one coherent, current-generation inventory + ignore-policy snapshot; obsolete generations must never mutate canonical state; freshness barriers must terminate or surface actionable state. Findings 1–3 each violate one of these.
- Root-cause confidence: Findings 1–2 confirmed by control-flow reading in two independent sessions (Finding 1 with identical references). No runtime reproduction; the #465 trigger remains unconfirmed, bounding incident claims but not these defects.
- Authority: Current enumeration + current ignore configuration, jointly fenced by
watcherIngressGenerationand the accepted watermark; the fence is incomplete at exactly the two seams identified. - Scale/observability: Queue offloading, bounding, coalescing, and retry caps are directionally right and should be retained. Oversized-callback source counts and internal-only diagnostics are follow-up quality items.
- Recommended scope: Fix Findings 1–2 and the ledger gap now, Finding 3 preferably in the same PR; keep broader performance tuning as measured follow-up.
- Validation boundary: (1) >4,096-entry callback with tail ignore-file deletion; (2) mid-scan restart with one result applied pre-reset and an ignore-policy change; (3)
verify-ledger+ authoritative test list; (4) live CE watcher/build-churn stress validation.
Verdict
REQUEST_CHANGES. Two independent reviews converged, with matching exact code references, on defects that let the system publish freshness over inventory derived from stale ignore authority (Finding 1) and let obsolete watcher generations leave partial state in the successor generation (Finding 2) — both in the exact recovery boundary this PR rewrites, and both demonstrably uncovered by the new tests. Green CI does not mitigate them. The overall design (bounded off-main callbacks, single-flight reconciliation, generation fencing, .watcherGap attribution) is sound and should be preserved; the requested changes are narrow: atomic generation-fenced commits, conservative ignore invalidation on truncation, the two named regression tests, ledger registration, and a production quiescence exit.
…e-rule rebuild A callback that exceeds the maximum entry count is truncated by dropping the tail events and appending a MustScanSubDirs sentinel. Because ignore-control deletions (e.g. a removed .gitignore) could be in the discarded tail, the previous path ignored that truncation when deciding whether to rebuild ignore rules. Add an isTruncated flag to the FSEvent callback payload and propagate it through the early filter, mailbox, and accepted-payload path so that any truncated payload conservatively invalidates all ignore rules before recovery or reconciliation runs. Also fence the directory enumeration state against watcher-ingress generation changes: - pass the expected generation into gatherPathsUsingEnumerator and check it before merging per-directory ignore decisions into the canonical cache; - capture/restore visitedInventory and pathCompsCache in scanOneLevelAndDiff; - move the originalVisitedInventory snapshot in scanFoldersInParallel until after missing-folder removals and restore on any error. Add a test that a 5000-event truncated payload with a trailing .gitignore deletion rebuilds ignore rules and exposes the previously ignored file. Relates to PR repoprompt#489 P1.
|
@sentry[bot] (HIGH): The removal of |
|
@sentry[bot] (MEDIUM): |
|
Addressed Baron findings across pushed commits |
|
Fresh exact-head shard 3 exposed an unrelated transient codemap setup failure in |
2 participants
Problem
macOS can report dropped FSEvents with a provider-level path such as
/. The watcher previously attempted workspace-relative path mapping before examining reliability flags, so it could reject the path before noticingMustScanSubDirs,UserDropped,KernelDropped, orRootChanged.Recovery could also stop after a one-level scan, leaving nested changes absent from the workspace inventory while freshness advanced.
Addresses #465.
Approach
Classify stream reliability before path mapping. Once the stream reports uncertainty, only a successful authoritative recursive reconciliation can close the gap and advance freshness.
Recovery results are fenced by watcher generation because cancellation alone cannot prevent an older asynchronous task from finishing after restart or teardown. Repeated failure becomes quiescent rather than retrying indefinitely, while new evidence or an explicit request can reactivate recovery.
Changes
Performance effect
Repeated recovery signals become one active recovery with at most one follow-up, and persistent failures stop autonomous retry after five consecutive attempts.
This prevents event bursts or persistent filesystem failures from creating unbounded recovery work. Recovery is now recursive, so a successful pass establishes a complete inventory instead of repeatedly operating from incomplete state.
Testing
Coverage includes provider path
/, combined recovery flags, recursive resync, retained gaps, follow-up coalescing, quiescence and reactivation, restart and teardown races, ignored bursts, mailbox overflow, and codemap preload rescheduling.FileSystemServiceRecoveryTests: 21 passedFileSystemAcceptedIngressBarrierTests: 20 passed, 1 skippedCodemapPreloadTests: 15 passedRepoPromptproduct build: passedDeferred
Moving the FSEvents callback off the main queue remains separate. This PR adds the recovery attribution needed to determine whether callback execution contributes materially to the observed stalls.