[rush-daemon] Fix summary writer racing early coalesced results (main is red) - #6095
Open
Sean Larkin (TheLarkInn) wants to merge 1 commit into
Open
Sean Larkin (TheLarkInn) wants to merge 1 commit into
Sean Larkin (TheLarkInn) wants to merge 1 commit into
Conversation
The early per-client result path (#6092) runs from onOperationCompleted, which the record's finalizeOperation() invokes synchronously before closing its StdioSummarizer. The summary writer (#6068) reads the failure tail from that summarizer, so it threw. Yield once before producing the early result. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
This branch has not been deployed
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
mainfails 3@rushstack/rush-daemontests withStdioSummarizer: The summary cannot be prepared until after close() is called. Two PRs that merged close together conflict at runtime, though not textually. This PR fixes that interaction, and both features keep working.Fixed tests:
PhasedRequestBatching: "publishes an early failure result while the batch continues for other clients"PhasedRequestBatching: "derives shared and disjoint failure results from each client subset"DaemonRequestWirePhased: "merges two connections into one shared iteration with subset-specific results"Root cause (#6068 × #6092)
writePhasedRequestSummary. For a failed operation, it prints the native summary, which reads the failure tail fromrecord.stdioSummarizer.getReport(). That call throws if the summarizer is still open.PhasedRequestEventSink.onOperationCompletedcalls#finishSettledEntry, which calls#produceResultAsync. That function writes the summary synchronously before its firstawait.onOperationCompletedis triggered fromOperationExecutionRecord.finalizeOperation(). At every call site, rush-lib closesstdioSummarizerandproblemCollectorsynchronously right afterfinalizeOperation()returns. So the early path wrote the summary while the failed operation's summarizer was still open.Fix
#finishSettledEntrynow publishes the early result through#produceEarlyResultAsync. That function yields one microtask (await Promise.resolve()) before calling#produceResultAsync. By then the notifying record has closed its summarizer, so the summary contains the full failure tail.Ordering and per-client correctness don't change:
entry.finishPromiseand the unsubscribe are still set synchronously. Other participants'#needsIterationchecks and the batch's#finishEntryAsync(??=) still see the early result as claimed.#produceResultAsyncstill flushes the sink before writing the result.rush-lib is not changed.
Tests
PhasedRequestSummary.test.ts: "writes the failure summary before an early result while the coalesced batch continues". Client A's selection fails early while client C's operation is held. The test asserts:FAILUREbanner, the--[ FAILURE: … ]--section containing the failed operation's output, the duration line, andOperations failed.Linux validation (WSL Ubuntu-24.04, fresh clone of current
main@ 7a0348d)heft testin rush-daemon gave 422 passed, 3 failed (the 3 above).rush build --to @rushstack/rush-daemon --to @rushstack/rush-cli-client(includes lint) passed, thenrush test --only @rushstack/rush-daemon --only @rushstack/rush-cli-client:Optional follow-ups
stdioSummarizer/problemCollectorbeforefinalizeOperation()notifies observers. AnyonOperationCompletedobserver could then read a finished report synchronously.This came out of the automated rushd Linux analysis (Rushd Hive, bugs 15 and 38). It unblocks #6065, #6066 and #6069.