agent-memory-sync: document macOS fs.watch arming race, drop retry machinery (f876dff6) - #110
Merged
Merged
Conversation
added 2 commits
August 16, 2026 23:06
…n test triggers (partial) Root-caused the pre-existing watch-trigger stall (agent-tasks f876dff6): a bare fs.watch() on macOS can silently and permanently miss a write issued immediately after the watch reports armed (nodejs/node#52601), independent of chokidar and independent of CPU load - proven via an isolated repro bypassing both. Fix lives on the test side since no deadline size can recover a lost event: applyTriggerWithRetry re-applies a trigger edit until a progress signal confirms it landed, budgeted well under the unmodified 90s inactivity deadline. deleteRetrySafe makes the two delete-based triggers safe to retry. This measurably fixes the proven mechanism, but an interleaved matched-control run under the full documented load scenario still failed repeatedly on two specific tests on both the fixed and unmodified arms. One failure's raw output showed evidence of a second, different mechanism this change does not fix: the parent test-runner's own delayed stderr-pipe read under heavy contention, which can make an already-completed tick look like a stall. Left open with instrumentation notes in tests/helpers/watch-process.ts for a follow-up.
…e docs only (f876dff6) Review found the retry helpers added in 2cb9173 (applyTriggerWithRetry, deleteRetrySafe) introduce a deterministic regression for no measurable benefit, and that the stall symptom they targeted does not currently reproduce against the merge base. Revert tests/helpers/watch-process.ts, tests/helpers/cli.ts, tests/integration/watch-mirror-delete.test.ts and tests/integration/watch-restore.test.ts to base (47e2d60); drop tests/unit/watch-process-trigger-retry.test.ts. Keep only: the corrected ready-event comment in src/commands/watch.ts, tightened to the reviewer-verified measurement (macOS fs.watch() can permanently lose a write <1ms after arming; 0/10 at 0ms vs 10/10 at >=1ms, idle and under load; nodejs/node#52601); a compact root-cause header in tests/helpers/watch-process.ts covering that mechanism, the 2026-08-16/17 non-repro measurement, the ruled-out stderr-read-delay theory, and the exit-listener ordering constraint; and a one-line hardening (spawnWatch's child stdout is now "ignore" instead of an unread "pipe", which could otherwise back up past 64KB and look like an unexplained stall). CHANGELOG entry rewritten to match: no "fixed" claim, no behavior change beyond the stdout hardening.
LanNguyenSi
added
review:tests-pass
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes task f876dff6 (qw10-fu-01, watch trigger FS-event stall) as a knowledge deliverable, per operator decision: root cause documented, no behavior change (one test-helper stdio hardening), no retry machinery.
fs.watch()(which chokidar 4.x uses directly: no fsevents dependency, no polling default) permanently loses a write issued within ~1ms of the watch arming: 0/10 events at 0ms delay, 10/10 at >=1ms, idle and under 12-worker load. Matches open Node issue Not possible to know when fs.watch has started on macOS nodejs/node#52601. The other suspects were falsified: no atomic-rename writes in this suite, no fsevents in play, and parent-side stderr-pipe read delay measured at p99 9ms / max 11ms over 600 samples under load, which cannot blow a 90s budget.src/commands/watch.ts(comment-only, verified zero non-comment lines changed), a compact root-cause header intests/helpers/watch-process.ts(mechanism + threshold + non-repro measurement + refuted stderr-pipe theory + the exit-listener ordering constraint as a warning for future edits),spawnWatchchild stdout switched from an unread pipe to 'ignore' (an over-64KB stdout child would otherwise present as exactly this phantom-stall class), and a factual CHANGELOG note.Verification
applyTriggerWithRetry/deleteRetrySafegrep 0 repo-wide.Task: f876dff6 (agent-memory). Orchestrator lane, batch 11; investigation history preserved in the branch commits and .ai run log.