[rush-daemon][WS2.6][5/9] Match Rush exit semantics - #5
Open
mojaza wants to merge 1 commit into
Open
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Three moderate issues remain in warning-policy lookup, retained operation outcomes, and missing global executor results.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds Rush-compatible command outcomes and exit-code handling for opt-in phased and global daemon routing.
Changes:
- Adds typed command results and shared warning policy.
- Delivers ordered final results for phased and global requests.
- Updates clients, protocol contracts, tests, documentation, and API metadata.
File summaries
| File | Summary |
|---|---|
libraries/rush-daemon/src/test/PhasedRequestRouterTestUtilities.ts |
Supports phased result assertions. |
libraries/rush-daemon/src/test/PhasedRequestRouter.test.ts |
Tests phased outcomes and delivery ordering. |
libraries/rush-daemon/src/test/GlobalCommandRequestRouter.test.ts |
Tests global outcomes, cancellation, and cleanup. |
libraries/rush-daemon/src/test/CommandResultPolicy.test.ts |
Tests outcome and warning-policy parity. |
libraries/rush-daemon/src/PhasedRequestRouter.ts |
Produces phased command results. |
libraries/rush-daemon/src/PhasedRequestClient.ts |
Adds phased result delivery. |
libraries/rush-daemon/src/index.ts |
Exports updated daemon APIs. |
libraries/rush-daemon/src/GlobalCommandRequestRouter.ts |
Applies global exit semantics. |
libraries/rush-daemon/src/GlobalCommandRequestClient.ts |
Adds global result delivery. |
libraries/rush-daemon/src/CommandResultPolicy.ts |
Implements command outcome and warning policy. |
libraries/rush-daemon/README.md |
Documents result semantics. |
libraries/rush-daemon-protocol/src/index.ts |
Exports protocol result types. |
libraries/rush-daemon-protocol/src/DaemonPhasedRequest.ts |
Extends phased request and result types. |
libraries/rush-daemon-protocol/src/DaemonCommandResult.ts |
Defines command result contracts. |
libraries/rush-daemon-protocol/README.md |
Documents the result contract. |
common/reviews/api/rush-daemon.api.md |
Updates the daemon API report. |
common/reviews/api/rush-daemon-protocol.api.md |
Updates the protocol API report. |
common/changes/@rushstack/rush-daemon/mojazayeri-exit-semantics_2026-08-21-19-36.json |
Records daemon package changes. |
common/changes/@rushstack/rush-daemon-protocol/mojazayeri-command-results_2026-08-21-19-36.json |
Records protocol package changes. |
Review details
Suppressed comments (3)
libraries/rush-daemon/src/CommandResultPolicy.ts:112
options.abortedis only considered for unscheduled requests. If the client disconnects after the scheduled graph has finished but whileflushAsync()or result construction is in progress, the router passesaborted: truewhilegraphStatuscan remainSuccess; this branch then reports success with exit code 0 despite the observed cancellation. Check the request abort flag after failure precedence and before warning/success classification.
if (
options.graphStatus === OperationStatus.SuccessWithWarning ||
options.operationOutcomes.some(
({ result }) => result.status === OperationStatus.SuccessWithWarning
)
libraries/rush-daemon/src/GlobalCommandRequestRouter.ts:109
abortedonly reflects the executor race, butGlobalCommandExecutionContextaborts its internal signal when a terminal write fails and that failure is surfaced here ascleanupError. If the executor has already resolved, a client write can therefore produce a delivered failure result withaborted: false, despite the result contract promising that disconnects are reflected in this flag. Propagate a writer/client-disconnect flag from the context through cleanup instead of relying only on the race.
const combinedError: unknown = combineExecutionAndCleanupErrors(executionError, cleanupError);
let result: IDaemonCommandResult;
try {
result = createGlobalCommandResult({
aborted,
libraries/rush-daemon/src/GlobalCommandRequestRouter.ts:145
- If the executor rejects after observing cancellation, this await propagates before
waitForExecutionAsyncreturns, so line 92 never records the'aborted'outcome and the final result reportsaborted: falseeven though the client cancellation was observed. Swallow the rejection here and let the subsequent await capture it after the abort flag is set.
await executorPromise;
- Files reviewed: 19/19 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| export function parseWarningsAllowedByEnvironment( | ||
| environment: Readonly<Record<string, string>> | ||
| ): boolean { | ||
| const value: string | undefined = environment[RUSH_ALLOW_WARNINGS_ENVIRONMENT_VARIABLE]; |
Comment on lines
+110
to
+112
| options.operationOutcomes.some( | ||
| ({ result }) => result.status === OperationStatus.SuccessWithWarning | ||
| ) |
| result = createGlobalCommandResult({ | ||
| aborted, | ||
| error: combinedError, | ||
| exitCode: executionResult?.exitCode ?? RUSH_SUCCESS_EXIT_CODE, |
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
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
Adds the authoritative Rush-compatible command outcome and exit-code translation for opt-in rushd phased and global routing.
Depends on #3. This stack follows merged microsoft#5949 and advances microsoft#5897. PBI: https://onedrive.visualstudio.com/EFun/_workitems/edit/3216022
Details
allowWarningsInSuccessfulBuildas captured by operation runners and request-localRUSH_ALLOW_WARNINGS_IN_SUCCESSFUL_BUILD=1without mutatingprocess.env;0preserves Rush's existing OR precedence.Limitations
This remains opt-in and does not integrate the standalone daemon into
apps/rushor the normal Rush launcher. It does not add stdin/raw-mode/PTY fallback, scheduler command classification or wait/no-wait admission, concurrent shared-build merging, daemon lifecycle/restart, or CLI routing/configuration. Existing action construction and full command parsing remain integration-owned pending microsoft#5895.How it was tested
rush build --only @rushstack/rush-daemon-protocol --only @rushstack/rush-daemon --verbose