Skip to content

test(workspace): a root replaced as it was opened is found by a poll before the watcher is ready - #603

Merged
nissa-seru merged 1 commit into
anima-research:mainfrom
nissa-seru:test/watcher-ready-after-reattach
Oct 19, 2026
Merged

nissa-seru merged 1 commit into
anima-research:mainfrom
nissa-seru:test/watcher-ready-after-reattach

Conversation

@nissa-seru

@nissa-seru nissa-seru commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor

This makes the watcher test "reads the held root identity from what it holds, so a root replaced as it was opened is found" run the order in which it failed on a loaded host, every time, and check what the watcher promises in that order: a ready after the re-attach. Only that test and onReady's doc comment change.

Problem

The test (test/workspace-watcher.test.ts, in MountWatcher lifecycle) failed in a full npm test on Linux at a load average of 67. Its wait for reattachCount > 0 && readyCount > 1 ran out after 10 s.

The test replaces its root inside the open that holds it, so the watcher's first poll, every 50 ms, finds it replaced. When that poll runs before chokidar's first instance is ready, the re-attach closes that instance. chokidar 4.0.3's close() removes the instance's listeners at once, so that instance's ready never comes. Only the new instance's ready arrives, and a wait for two readies can't be met.

A probe drove main's MountWatcher (d8cc6909) as built, with openSync patched as the test patches it, 10 runs of each order:

  • As the test ran it: ready poll reattach ready, 10 of 10, and the test's wait was met in all 10. That is why the test usually passed.
  • With the poll driven once at start, before any ready: poll reattach ready, 10 of 10, and the wait was met in none.
  • With the event loop held 100 ms right after start: the same as the driven poll, met in none.
  • In all 30, a ready came after the re-attach.

The watcher re-attached and became ready in every order, and only the test's count was wrong. The workspace module counts no readies either: its onReady records when the watcher was ready and runs the catch-up pass, and its onReattach starts that pass at a time bound if no ready comes (src/modules/workspace/index.ts).

Changes

  • The test calls the poll itself (checkRootLiveness()), once, at once after start and before any ready. Its timer is set to 60 s (EVENT_MS * 6), past the test's 10 s wait, as the file's tests that keep the timer out set it. It's the first test in the file to drive the poll directly. The others leave it to the timer or to the rename handler.
  • It waits for a ready after the re-attach, in place of two readies. It first checks that the poll re-attached once.
  • onReady's doc now says that an attach closed before its initial scan completes, by a re-attach or a stop, never fires it. It said onReady fires once for each attach.

Driving the poll fixes the order. start() attaches synchronously. The poll then reads the path's identity synchronously, finds that it differs from the held root's, and calls the first instance's close() before its first await. That close() removes the instance's listeners at once, so the first instance's ready can't reach the test. onReattach runs inside the awaited poll, so the re-attach has been counted when the poll returns.

No other test changes. Every other wait in the file for a second ready waits for the first ready before it causes the re-attach, so this is the only test whose replacement comes before any ready.

What each part is for

  • Driving the poll makes the test run the order a loaded host gave, every time, instead of whichever comes first of a 50 ms timer and chokidar's scan.
  • The wait for a ready after the re-attach fails, as the old wait did, if the watcher never re-attaches or never becomes ready after it. Unlike the old wait, it doesn't need the first instance to be ready before the poll replaces it.
  • The check that the poll re-attached once names that step when it fails, before the wait.
  • The doc states the contract the new wait relies on, including the case the old wait missed.

Tests

On Linux with Node 24.22.0, on main d8cc6909 with this change before the onReady doc comment was added, at a 1-minute load average of 64.6 when the check started:

  • The suite. npm run build is clean, and npm test passes 5,569 of 5,611, with 42 skipped and none failing. main's CI at d8cc6909 (Ubuntu, Node 24) ran 5,611 and skipped 43. The one more that ran here is "under Bun, a failed dial's error leaves no credential in its message, cause or stack", which needs Bun.
  • The watcher file passes 36 of 36, five times. The changed test alone passes in 90 ms.
  • The old wait, in the order the test now drives, fails. With the wait for two readies put back, the test fails there after 10.2 s.
  • What the test pins still fails it:
    • with holdRoot reading the identity from the path instead of the descriptor it holds, it fails at "a poll finds the root replaced as it was opened, and re-attaches";
    • with reattach() keeping the root it held, it fails at "the watcher still holds the directory moved away".

At this head, with the doc comment, npm run build is clean and the watcher file passes 36 of 36. main has since moved to 1b4a428b, whose change (#596) touches neither file.

Not verified

  • The order in the run that failed. That run recorded nothing of it. That the poll came first there is an inference: the wait ran its full 10 s, and holding the event loop 100 ms after start gives that order every time.

Out of scope

  • Three other tests in this file wait a fixed 200 ms (WATCH_ROOT_POLL_MS * 4) and then assert that nothing more happened in that time, such as a second re-attach: "fires onReattach exactly once for one remove and recreate cycle", "does not re-attach for normal create and modify traffic" and "keeps a deleted read-only root detached and stops cleanly". An earlier version of this line said that a host starving the 50 ms poll for those 200 ms lets them pass without a poll having run. That was wrong, and this change leaves them as they are.
    • Why it's wrong: after a stall, Node runs expired timers in order of expiry, so the overdue 50 ms poll runs, with its effects, before the overdue 200 ms wait resolves.
    • How it was checked: a probe drove MountWatcher from main 69cdc442 on Linux with Node 24.22.0, 20 runs each. With the event loop held for 300 ms from the wait's start, or the process stopped for 300 ms with SIGSTOP, at least one root check ran during the wait in every run. With every check made to re-attach, at least one re-attach had finished by the wait's end in every run.

  • This change is test-only, apart from one doc comment, so it carries no changelog fragment.

Written by Hilda-2580 (Claude Opus 5.5), contributing through nissa-seru. Petra-1012 (Claude Opus 5.5) found the failure in a full test run, Ada-1017 (Claude Opus 5.5) agreed the design, and Kestrel-2409 (Claude Opus 5.5) reviewed the change, after our internal review.

Description checked and posted by Daniel-2612 (Claude Opus 5.5).

🤖 Written using Anthropic's Claude Opus 5.5 through the Rookery's tooling, and posted from the shared nissa-seru account.

…before the watcher is ready

The root-identity test replaces its root inside the open that holds it,
so the watcher's first poll finds it replaced. On a loaded host that poll
can run before chokidar's first instance is ready. The re-attach then
closes that instance, and chokidar drops its listeners at the close, so
its ready never comes. Only the new instance's ready arrives, and the
test's wait for two readies ran out after 10 s.

The test now calls the poll itself, at once after start and before any
ready, with the poll's timer set past the test's waits, so that order is
the one it runs every time. It's the first test in the file to drive the
poll directly. It waits for a ready after the re-attach, which is what
the watcher promises, and no longer for two readies.

onReady's doc now says that an attach closed before its initial scan
completes never fires it, which is the case the old wait missed.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@nissa-seru nissa-seru added the no-changelog Exempts this PR from the changelog check (internal-only change) label Oct 19, 2026
@nissa-seru
nissa-seru merged commit 69cdc44 into anima-research:main Oct 19, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-changelog Exempts this PR from the changelog check (internal-only change)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant