fix(core): tell nx watch when the daemon stops watching - #36758
Draft
AgentEnder wants to merge 9 commits into
Draft
fix(core): tell nx watch when the daemon stops watching#36758AgentEnder wants to merge 9 commits into
AgentEnder wants to merge 9 commits into
Conversation
When the native workspace watcher dies, `ingest_event` returns an error and the watch loop breaks, so no further file events are ever produced. The daemon recorded this in `workspaceWatcherError` -- write-once and never cleared -- and from then on `handleWorkspaceChanges` returned early for the rest of the process's life. The error only reached clients that sent a new message, and `nx watch` never sends one: it registers a socket and waits. The command therefore kept running and silently never fired again. - notify already-registered file-watcher sockets when the watcher fails, so passive clients learn the daemon has gone deaf - surface it client-side as WatcherFailedError, and have `nx watch` report it and exit rather than wait forever - return from the generic watch error branch; `data` is null there, so falling through to `data.changedProjects` threw The inotify watch limit is the common trigger on Linux, so the message points at it.
Same source tree produced one failure in one CI run and four in the next, every failure shaped as `Received: []` or a TypeError on an empty output array. Two harness defects, both measured against a fixture workspace driving real `nx watch`: - `runWatch` never re-added NX_PROJECT_GRAPH_CACHE_DIRECTORY, which getStrippedEnvironmentVariables drops, while beforeAll, runWatchWithReconnect and the afterEach log dump all did. Seven of the eight tests ran against a different daemon than the one whose log CI printed, and a second daemon watched the same workspace for the whole suite. - `getOutput()` slept a flat 1000ms and then looked once. Happy-path latency measures 100-116ms (the native watcher's IDLE_WINDOW), so the window had ~10x headroom, but any excursion past it became an unreadable empty array. Waiting for the expected output instead of a fixed window keeps a silent watcher failing while letting a slow one pass. The 500ms settle after the match preserves the suite's negative assertions -- several tests also assert that unwatched projects did NOT run, which an early return would have silently stopped checking.
✅ Deploy Preview for nx-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for nx-dev ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
|
View your CI Pipeline Execution ↗ for commit 6f5f27a
☁️ Nx Cloud last updated this comment at |
![nx-cloud[bot]](https://avatars.githubusercontent.com/in/80458?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Follow-up to the previous commit. With `nx watch` now exiting on a fatal
daemon watcher error, the suite as written would have hidden exactly the
signal we added:
- `await new Promise((res) => p.on('close', res))` was attached after the
kill, so a process that exited on its own never fired it and the test hung
to the 50s jest timeout. The close promise is now created up front, and the
poll loop stops as soon as the process is gone.
- an exit before the expected output now throws with the raw stream attached,
instead of letting the caller assert on `[]`
- `nx watch` reports errors via `output.error`, which is NX-prefixed and so
dropped by the line filter. afterEach now dumps the raw stream, and
runWatch captures stderr into it. stdout stays the only thing parsed, so
stderr noise can't be mistaken for a project name.
The client tells a change event from a failure by reading `watcherError` off the parsed message. parseMessage auto-detects JSON vs v8, so assert the payload the daemon actually sends survives that round trip with the key intact.
The old code used `on('close')`, not `once`; the hazard is adding any
listener after the event already fired.
Contributor
There was a problem hiding this comment.
Important
Nx Cloud is proposing a fix for your failed CI:
We fix the flaky "should watch for global workspace file changes" test by replacing batchedAll with a new receivedAll helper that accepts files spread across multiple NX_FILE_CHANGES batches. The previous batchedAll approach required all 4 expected files to appear in a single batch, but the native watcher's 100ms idle window can flush the first write before the remaining files arrive, so the condition was never satisfied and the test timed out. The assertion now aggregates all received file paths across batches and compares the sorted set, preserving the 500ms settle window so negative assertions (unwatched projects did not fire) remain protected.
Warning
❌ We could not verify this fix.
Suggested Fix changes
diff --git a/e2e/nx/src/watch.test.ts b/e2e/nx/src/watch.test.ts
index bbf7bb60..1410d599 100644
--- a/e2e/nx/src/watch.test.ts
+++ b/e2e/nx/src/watch.test.ts
@@ -48,6 +48,14 @@ const batchedAll =
(lines: string[]): boolean =>
lines.some((line) => expected.every((f) => line.split(' ').includes(f)));
+/** All files in `expected` have been received across any number of batches. */
+const receivedAll =
+ (expected: string[]) =>
+ (lines: string[]): boolean => {
+ const received = new Set(lines.flatMap((l) => l.split(' ')));
+ return expected.every((e) => received.has(e));
+ };
+
// '' when the watcher emitted nothing, so the assertion diffs readably instead of
// throwing on undefined.
function matchingBatch(lines: string[], expected: string[]): string {
@@ -178,10 +186,14 @@ describe('Nx Watch', () => {
`libs/${proj2}/newfile.txt`,
'newfile2.txt',
];
- const lines = await getOutput(batchedAll(expected));
- let results = matchingBatch(lines, expected).split(' ').sort();
-
- expect(results).toEqual(expected);
+ const lines = await getOutput(receivedAll(expected));
+ const allReceived = [
+ ...new Set(
+ lines.flatMap((l) => l.split(' ')).filter((s) => s.length > 0)
+ ),
+ ].sort();
+
+ expect(allReceived).toEqual([...expected].sort());
}, 50000);
it('should watch selected projects only', async () => {
🔔 Heads up, your workspace has pending recommendations ↗ to auto-apply fixes for similar failures.
Or Apply changes locally with:
npx nx-cloud apply-locally g7ak-ypD4
Apply fix locally with your editor ↗ View interactive diff ↗
🎓 Learn more about Self-Healing CI on nx.dev
CI failure on 4a8bdbb (e2e-nx watch.test.ts, "should watch for global workspace file changes"). The watcher reported all four files, split across three batches: libs/<p1>/newfile.txt libs/<p2>/newfile.txt libs/<p1>/newfile2.txt newfile2.txt The writes are 10ms apart and the watcher coalesces on a 100ms idle window, so a single batch is a property of an idle machine, not an invariant. That run's per-test times were 10-26s against 6-7s on the green run before it. The pre-existing assertion had the same assumption -- it read output[0] -- so this is a latent flake the rewrite carried forward rather than a new one. Assert the union across batches instead. The negative assertions survive: a global file leaking into a project-scoped run still breaks equality. Also stop reporting a timeout as an early exit. `exited` was read after our own treeKill, so every timeout claimed "nx watch exited before producing the expected output". Sample it before the kill. Contrary to the hypothesis this branch started from, the daemon watcher was alive and reporting throughout -- no inotify exhaustion. The watcher-failure plumbing in bc2cffe stands on its own as a real bug, but it is not what makes this suite flaky.
No code change. Sampling the suite after the batch-union fix to see whether the flake rate actually moved. Also checks that an empty commit produces a real e2e run rather than a cache replay.
Sample 4's pipeline died on maven-batch-runner:_package before the e2e stage, so the watch task never ran and that CIPE carries no signal.
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.
Current Behavior
Product. When the daemon's native workspace watcher dies,
ingest_eventreturns an error and the watch loopbreaks, so no file event is ever produced again.handleWorkspaceChangesrecords this inworkspaceWatcherError, which is write-once and never cleared, and from then on returns early for the rest of the process's life.That error only reaches clients that send a new message (
handleMessage, server.ts:229).nx watchnever sends one — it registers a file-watcher socket and waits. So the command keeps running and silently never fires again, with no output. For a user that's "nx watchjust stopped working"; in CI it'sReceived: [].There is also a latent crash next to it: the generic watch error branch reports the error and then falls through to
data.changedProjects, butdataisnullon every error path.Tests.
e2e/nx/src/watch.test.tshas been failing nondeterministically. The same source tree produced one failure in one CI run and four in the next, every failure shaped asReceived: []or aTypeErroron an empty output array. Two defects in the harness:runWatchnever re-addedNX_PROJECT_GRAPH_CACHE_DIRECTORY, whichgetStrippedEnvironmentVariablesdrops — whilebeforeAll,runWatchWithReconnectand theafterEachlog dump all did. Seven of the eight tests therefore ran against a different daemon than the one whose log CI printed, so the diagnostic added in chore(misc): surface trimmed daemon logs in flaky watcher e2e suites #36421 has been aimed at a daemon that never saw the events. It also left a second daemon watching the same workspace for the whole suite.getOutput()slept a flat 1000ms and then looked exactly once. Measured happy-path latency is 100–116ms — dominated by the native watcher's 100msIDLE_WINDOW— so the window had ~10x headroom, but any excursion past it turned into an unreadable empty array.Expected Behavior
Product. The daemon pushes the failure to already-registered file-watcher sockets, so passive clients learn it has gone deaf. The client surfaces it as
WatcherFailedError;nx watchreports it — pointing at the inotify watch limit, the common trigger on Linux — and exits instead of waiting forever. The generic error branch now returns rather than dereferencingnull.Tests. Every nx invocation in the suite shares one cache directory, so there is a single daemon and
afterEachdumps its log.getOutput()waits for the expected output (15s ceiling) instead of a fixed window, then keeps reading for 500ms.The settle window is load-bearing: several of these tests also assert that unwatched projects did not run, and returning the instant the positive expectation is met would have silently stopped checking that. Waiting for a condition also keeps the change honest — it converts slow-but-correct into a pass while leaving a silent watcher still failing, so it is not masking the bug above.
Notes for reviewers
The root cause of the CI flake is not proven. Measurements (fixture workspace driving real
nx watch): idle p50 111ms, and 2x CPU oversubscription moved p50 only to 116ms while inflating process boot 880→1350ms. So contention alone does not explain a 10x excursion on a small workspace. The leading hypothesis is the watcher-death path this PR now surfaces — inotify watch exhaustion across parallel CI jobs, which has bitten this repo before (#34329). If the flake survives, the daemon log will now say so, which it could not before.No linked issue — this came out of investigating repeated CI failures rather than a filed report.
Related Issue(s)
N/A — no existing issue. Happy to file one if the team prefers a tracking issue for the e2e flake.
View Polygraph session ↗