Skip to content

[rush-daemon] Surface warm snapshot and daemon shutdown errors to the right client - #6070

Merged
Sean Larkin (TheLarkInn) merged 11 commits into
mainfrom
thelarkinn-fix-rushd-error-surfacing
Sep 24, 2026
Merged

Sean Larkin (TheLarkInn) merged 11 commits into
mainfrom
thelarkinn-fix-rushd-error-surfacing

Conversation

@TheLarkInn

@TheLarkInn Sean Larkin (TheLarkInn) commented Sep 24, 2026 •

Copy link
Copy Markdown
Member

Summary

Two error-attribution bugs in the opt-in Rush daemon (rush-client / rushd):

  1. A warm input-snapshot failure goes to the wrong request. For example, git Permission denied: the failing request gets only Rush could not capture the next workspace inputs snapshot.. The real error and its stack are printed on the next request's stderr, and that request exits 0.
  2. A daemon shutdown during a build gives no reason. This covers rush-client daemon stop from another shell, or SIGTERM/SIGINT sent to the daemon. In-flight clients exit 1 with no message, and daemon stop doesn't say it cancelled anything.

Root cause

  1. The bound graph keeps the EngineTerminalProvider of the request that bound it. A warm reconcile logs the snapshot error into that terminal's buffer, because no iteration is executing. WorkspaceEngineComponentFactory then throws a generic error without the buffered text. The next request's configureIteration flushes the stale buffer as that request's own output.
  2. DaemonControlSession#closeOnceAsync aborts requests with the generic The daemon control session is closing. error. The phased result for an aborted request has no errorMessage, and launchClient.ts never printed result.errorMessage anyway. #shutdownHostAsync acknowledges shutdown without reporting the requests it is about to abort.

Fix

  • Engine diagnostics are now request-scoped. The logic lives in the new EngineTerminalProvider.reconcileWithRequestDiagnosticsAsync, and the resolver just calls it.
    • describeError drains the buffer.
    • Each warm reconcile after the first discards diagnostics left over from earlier requests. The binding request's own diagnostics are kept.
    • When reconcile fails, the buffered diagnostics are added to the thrown error. The error keeps its identity and type.
    • On WorkspaceEngineRecreationRequiredError the buffer is discarded, because the replacement engine gets a fresh terminal.
  • Typed shutdown reason:
    • New exported DaemonShutdownError with an initiator: controlClient, signal (with the signal name), idleTimeout, restart or host.
    • RushDaemonHost.closeAsync(reason?) passes it to the control sessions, which abort with it. serveRushDaemonAsync records which signal was received.
    • PhasedRequestRouter puts the reason into the aborted result's errorMessage. The pre-engine abort path in the resolver uses it too.
    • Example message: The Rush daemon was shut down (the daemon process received SIGTERM) while this request was running; re-run the command.
  • Client (@rushstack/rush-cli-client): prints rush-client: <errorMessage> for non-zero results, via a small getResultDiagnostic helper.
  • Active requests on stop (additive protocol change):
    • shutdownAck gets an optional activeRequests count. Older peers omit it. The validator accepts only a non-negative safe integer.
    • DaemonClient.shutdownAsync() now returns the ack payload.
    • When N > 0, rush-client daemon stop prints a note on stderr and adds cancelledRequests: N to its JSON output.
    • No minor version bump, since the protocol lets peers ignore optional fields.

Tests

  • EngineTerminalProvider.test.ts (new):
    • Buffered diagnostics are drained into the error and can be discarded.
    • Reconcile diagnostics are scoped to the request that produced them.
    • Diagnostics are dropped when the engine must be recreated.
  • ProductionDaemonRequestResolver.test.ts, run against the real native engine:
    • An unreadable input on warm request N rejects N with Permission denied … could not capture …. Request N+1 succeeds and emits nothing from N. Skipped on Windows and when running as root.
    • An in-flight build aborted by host.closeAsync(new DaemonShutdownError({ initiator: 'signal', signal: 'SIGTERM' })) returns aborted: true with the typed message.
  • DaemonShutdown.test.ts: the ack carries activeRequests: 0.
  • Protocol LifecycleControl.test.ts: round-trip and rejection of invalid counts.
  • DaemonClient.test.ts: shutdownAsync returns the ack payload.
  • rush-cli-client resultDiagnostics.test.ts (new): prints the error message of a non-zero result, admission failures take precedence, and it stays silent on success.

Linux validation (WSL Ubuntu-24.04)

Full tables are in the PR comment.

  • Lab builds and tests:
    • rush build --to @rushstack/rush-daemon --to @rushstack/rush-cli-client passes, including lint and API Extractor. API reports are updated.
    • rush test passes for rush-daemon-protocol, rush-client-core and rush-cli-client.
    • The targeted rush-daemon suites pass: 82 of 82 tests. After the refactor, the core suites pass 55 of 55.
  • Tidy up some config files #68 repro (b15.sh flow):
    • Before: the failing build printed 1 generic line. The next build exited 0 with 12 stale stderr lines.
    • After: the failing build shows the git Permission denied error (13 lines), and the next build has 0 stderr lines.
  • Activate API-Extractor in web-build-tools #50 repro (s57b.sh flow, daemon stop mid-build):
    • Before: the victim exits 1 with empty stderr, and stop prints nothing about it.
    • After: the victim prints The Rush daemon was shut down (requested by "rush-client daemon stop" …) … re-run the command., and stop reports cancelledRequests: 1.

Overlap with other open daemon PRs

This PR touches the same files as several other open daemon fixes. The edits in the shared files are kept small and local, so whichever PR merges second can resolve conflicts easily:

Main (including #6062 and #6067) has been merged into this branch. The lab build and tests pass on it: 84/84 targeted rush-daemon tests and rush test for rush-cli-client, rush-client-core and rush-daemon-protocol. The API reports are unchanged.

Optional follow-ups (not in this PR)

  • Use a distinct exit code for "cancelled by daemon shutdown".
  • Add a daemon stop --force / --wait policy.
  • Give global-command and graph routers the same shutdown reason.
  • Stop wrapping user-facing errors with Daemon rejected the request (routingFailed):.

This came out of the automated rushd Linux analysis ("Rushd Hive", bugs #68 and #50 on the analysis board).

Fixes #6059

… right client

Fixes #6059

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…engine initialization

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@TheLarkInn

Copy link
Copy Markdown
Member Author

Linux validation (WSL Ubuntu-24.04, node 22.23.2, mkws-synth --projects 12 --sleep-ms 1000)

Before is the unfixed rush-client toolchain wrapper. After is the fix clone's client and daemon. The daemon command line was checked and points at the fix clone. Scripts: $LAB/fixes/error-surfacing-repro.sh, error-surfacing-probe{,-before}.sh.

#68, warm snapshot failure (chmod 000 packages/p03/src/index.js → build A; chmod 644 → build B)

build A (unreadable input) build B (next build)
before exit 1, 1 stderr line: generic Rush could not capture the next workspace inputs snapshot. exit 0, 12 stale stderr lines (Error calculating the state of the repo … Permission denied + stack)
after exit 1, 13 stderr lines: Error calculating the state of the repo. (inner error: … fatal: could not open 'packages/p03/src/index.js' for reading: Permission denied … exit 0, 0 stderr lines

#50, rush-client daemon stop from a second shell during a build

victim client daemon stop
before exit 1, empty stderr, stdout stops after "p01 (build)" completed successfully {"state":"shutdownAccepted",...}
after exit 1, rush-client: The Rush daemon was shut down (requested by "rush-client daemon stop" or "daemon restart") while this request was running; re-run the command. stderr rush-client: the daemon was running 1 request(s); they were cancelled… + JSON "cancelledRequests":1

An earlier run stopped the daemon before the engine had initialized. The victim then got The request was cancelled before engine initialization., so commit 3 makes that path report the shutdown reason too.

Tests (lab):

  • heft test --test-path-pattern "(EngineTerminalProvider|ProductionDaemonRequestResolver|DaemonShutdown|DaemonIdleShutdown|RushDaemonHost|PhasedRequestRouter|WorkspaceEngineComponentFactory)": 82/82 pass, including the new snapshot-attribution and shutdown-reason tests.
  • rush test for rush-daemon-protocol, rush-client-core and rush-cli-client: all pass.
  • rush build --to @rushstack/rush-cli-client (includes lint): pass.

Side observation: in some of these runs the build request first triggered an environment-fingerprint daemon restart. A daemon stop during that window gets Could not connect. That is tracked separately (env-fingerprint fix), not here.

…rminalProvider

Keeps the ProductionDaemonRequestResolver change to a one-line call so it composes with other open daemon PRs.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…error-surfacing

# Conflicts:
#	apps/rush-cli-client/src/launchClient.ts

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Shutdown attribution remains incomplete for queued and interactive requests, and acknowledgement counting has a race.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 5 Medium severity · 1 Low severity

Open (6)
What changed in this PR

Fixes #6059 by scoping warm-engine diagnostics to the originating request and surfacing daemon-shutdown reasons to affected clients.

Changes:

  • Adds request-scoped reconciliation diagnostics and typed shutdown errors.
  • Reports active requests in shutdown acknowledgements and CLI output.
  • Adds protocol, client, integration, and regression tests.
File Description
libraries/​rush-daemon/​src/​test/​ProductionDaemonRequestResolver.test.ts Tests snapshot failures and shutdown cancellation.
libraries/​rush-daemon/​src/​test/​EngineTerminalProvider.test.ts Tests diagnostic isolation and draining.
libraries/​rush-daemon/​src/​test/​DaemonShutdown.test.ts Verifies shutdown request counts.
libraries/​rush-daemon/​src/​serveRushDaemon.ts Captures process shutdown signals.
libraries/​rush-daemon/​src/​RushDaemonHost.ts Propagates typed shutdown reasons.
libraries/​rush-daemon/​src/​ProductionDaemonRequestResolver.ts Wraps reconciliation diagnostics.
libraries/​rush-daemon/​src/​PhasedRequestRouter.ts Adds shutdown errors to results.
libraries/​rush-daemon/​src/​index.ts Exports shutdown APIs.
libraries/​rush-daemon/​src/​EngineTerminalProvider.ts Scopes buffered diagnostics.
libraries/​rush-daemon/​src/​DaemonShutdownError.ts Defines typed shutdown reasons.
libraries/​rush-daemon/​src/​DaemonControlSession.ts Reports and aborts active requests.
libraries/​rush-daemon-protocol/​src/​test/​LifecycleControl.test.ts Tests acknowledgement validation.
libraries/​rush-daemon-protocol/​src/​ShutdownAckValidation.ts Validates active-request counts.
libraries/​rush-daemon-protocol/​src/​DaemonLifecycleControl.ts Extends shutdown acknowledgement payload.
libraries/​rush-daemon-protocol/​src/​ControlMessageValidation.ts Registers shutdown validation.
libraries/​rush-client-core/​src/​test/​DaemonClient.test.ts Tests acknowledgement return values.
libraries/​rush-client-core/​src/​DaemonClient.ts Returns shutdown acknowledgement data.
common/​reviews/​api/​rush-daemon.api.md Updates daemon API report.
common/​reviews/​api/​rush-daemon-protocol.api.md Updates protocol API report.
common/​reviews/​api/​rush-client-core.api.md Updates client API report.
common/​changes/​@rushstack/​rush-daemon/​fix-rushd-error-surfacing_2026-09-24-01-50.json Records daemon patch.
common/​changes/​@rushstack/​rush-daemon-protocol/​fix-rushd-error-surfacing_2026-09-24-01-50.json Records protocol patch.
common/​changes/​@rushstack/​rush-client-core/​fix-rushd-error-surfacing_2026-09-24-01-50.json Records client-core patch.
common/​changes/​@rushstack/​rush-cli-client/​fix-rushd-error-surfacing_2026-09-24-01-50.json Records CLI patch.
apps/​rush-cli-client/​src/​test/​resultDiagnostics.test.ts Tests result diagnostics.
apps/​rush-cli-client/​src/​resultDiagnostics.ts Formats failed-result messages.
apps/​rush-cli-client/​src/​launchClient.ts Prints result diagnostics.
apps/​rush-cli-client/​src/​daemonCommands.ts Reports cancelled requests.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread apps/rush-cli-client/src/resultDiagnostics.ts Outdated
Comment thread libraries/rush-daemon-protocol/src/DaemonLifecycleControl.ts
Comment thread libraries/rush-daemon/src/DaemonControlSession.ts
Comment thread libraries/rush-daemon/src/PhasedRequestRouter.ts Outdated
Comment thread libraries/rush-daemon/src/ProductionDaemonRequestResolver.ts
Comment thread apps/rush-cli-client/src/daemonCommands.ts Outdated
… requests, protocol minor 11

- Report the shutdown reason for requests aborted while waiting for admission, and prefer it on the client.
- Keep the shutdown reason when restoring raw mode fails with it.
- Begin shutdown before awaiting the acknowledgement write so the reported count matches the aborted set.
- Advertise protocol minor 11 for the shutdownAck activeRequests field.
- Keep the daemon stop note accurate for every request kind.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…t aborts

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…error-surfacing

# Conflicts:
#	apps/rush-cli-client/src/daemonCommands.ts
… --force

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@TheLarkInn
Sean Larkin (TheLarkInn) merged commit 570fe51 into main Sep 24, 2026
10 checks passed
@TheLarkInn
Sean Larkin (TheLarkInn) deleted the thelarkinn-fix-rushd-error-surfacing branch September 24, 2026 21:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Closed

3 participants