fix(boxel-cli): register realm-watch signal handlers before the initial poll#5525
Conversation
…al poll The realm-watch loop wired up its SIGINT/SIGTERM (and abort-signal) stop triggers only after the initial poll. An interrupt during a slow first poll therefore left the lock file and process-registry entry dangling. Wire up the stop triggers before the initial poll and skip the poll when a stop has already arrived, so Ctrl+C cleans up at any point. The integration tests observed this asynchronously-populated state (the lock file, the registered handlers) with a fixed 150ms sleep, which loses under CI load once lock acquisition, watcher init, and the initial network poll exceed that window. Wait on the state with a bounded poll-until-condition helper that reports the observed state on timeout. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6e5f51cf42
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
This PR hardens boxel realm watch startup/shutdown behavior by ensuring stop triggers (AbortSignal or SIGINT/SIGTERM) are registered before the initial poll, eliminating a race that intermittently flaked CI and also closing a real-world gap where Ctrl+C during a slow first poll could leave locks/registry entries behind.
Changes:
- Register abort/signal stop handlers before the initial
tickAll()and skip the initial poll if a stop already arrived. - Replace fixed
sleep(150)waits in therealm-watchintegration tests with a boundedwaitFor(predicate, describeFailure)polling helper to avoid CI timing races. - Improve failure diagnostics in the integration tests by reporting observed listener counts / lock-file contents on timeout.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| packages/boxel-cli/src/commands/realm/watch/start.ts | Installs stop triggers before the initial poll and gates the initial poll on !stopped to avoid dangling locks/registry entries during slow startup. |
| packages/boxel-cli/tests/integration/realm-watch.test.ts | Replaces fixed sleeps with a timeout-bounded poll-until helper and enhances failure messages to make future flakes diagnosable. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Registering a signal handler suppresses Node's default Ctrl+C termination, so once the stop triggers are wired up before the initial poll, a stop that fires while that poll is still in flight would leave watchRealms parked on the poll - cleanup runs but the function can't return until the (possibly wedged) poll resolves, making the watch look hung. Race the initial poll against the stop so an interrupt returns promptly. When the stop wins, the still-pending poll is inert: the watcher is already shut down and the next-tick scheduler is guarded by the stopped flag. Add an integration test that stops the watch while the initial poll is blocked and asserts watchRealms returns and releases the lock without the poll ever completing. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Background
boxel realm watchpolls a realm's_mtimeson an interval and pulls remote changes into a local directory. Its programmatic entry point,watchRealms(), acquires a lock, initializes each watcher, does an initial poll, and then blocks until it's told to stop — either via a caller-suppliedAbortSignalor, when none is supplied (the real CLI path), viaSIGINT/SIGTERMhandlers it registers itself. On stop it clears the timer, shuts the watchers down, releases the lock file, and unregisters from the process registry.The problem
The
realm-watchintegration suite is intermittently red on CI:watchRealms()registered its stop triggers after the initial poll, which sits behind lock acquisition →watcher.initialize()→ an_mtimesnetwork round-trip. The test asserted the handler was registered after a fixedsleep(150). Under CI load that async chain overruns 150ms, so the handler isn't registered yet when the assertion runs → empty array.Two other tests in the file used the same
sleep(150)-then-assert shape against the lock file; they're the same latent race and are hardened here too.The fix
start.ts— wire up the stop triggers (abort signal, orSIGINT/SIGTERM) before the initial poll, and skip the poll if a stop already arrived. This also closes a real product gap: with the pre-existing ordering, pressing Ctrl+C during a slow first poll left the lock file and process-registry entry dangling because no handler was installed yet.realm-watch.test.ts— replace the fixedsleep(150)waits with a boundedwaitFor(predicate, describeFailure)poll-until-condition helper (mirrors the existingwaitForRemoteVisibilityidiom). Its timeout message reports the observed state — actualSIGINT/SIGTERMlistener counts and lock-file contents — so a future failure is diagnosable rather than opaque.Verifying the root cause
A temporary repro that blocks the initial poll pinned it down deterministically:
0→expected +0 to be 1(reproduces the CI failure exactly).1— the handler is present even while the initial poll is still blocked.Verification
tsc --noEmitand eslint clean.realm-watchintegration suite: 23/23 passing, run twice, no unhandled rejections.🤖 Generated with Claude Code