Skip to content

[rush-daemon] Publish coalesced phased results as soon as each client's own selection completes - #6092

Merged
Sean Larkin (TheLarkInn) merged 2 commits into
mainfrom
thelarkinn-fix-rushd-coalesced-batch-hol-blocking
Sep 24, 2026
Merged

Sean Larkin (TheLarkInn) merged 2 commits into
mainfrom
thelarkinn-fix-rushd-coalesced-batch-hol-blocking

Conversation

@TheLarkInn

Copy link
Copy Markdown
Member

Summary

When the daemon coalesces compatible SHARED-BUILD requests into one graph iteration, each client now gets its final result as soon as every operation in its own selection has completed and its output has drained. Before this change, every client waited for the whole union iteration. The iteration, graph lease, and native execution lease keep running for the other participants.

Fixes #6084

Root cause

PhasedRequestBatchCoordinator.#executeBatchAsync awaited executeScheduledIterationAsync() for the whole union before it called #finishEntryAsync for any entry. Then createBatchReleaseBarrier made every entry's result wait for the native execution lease release, and that release only happened after all entries arrived. So a client that asked for --to p04 sat idle until an unrelated coalesced --to p16 finished.

Fix

  • PhasedRequestEventSink: new optional onActiveOperationsSettled callback.
    • onIterationScheduled records which of the client's active operations are scheduled and not yet terminal.
    • onOperationCompleted removes each one. rush-lib emits that event from finalizeOperation() after the collated writer closes, so the operation's header, chunks, status and stream-closed events are already queued on the client's ordered writer.
    • When the set is empty, the callback fires once per iteration.
    • The callback is suppressed if any of the client's operations completed as ABORTED. That covers iteration abort, a failed start, and discarded work, and those results stay on the batch path.
  • PhasedRequestRouter (batch coordinator):
    • #finishSettledEntry unsubscribes the entry and produces its result right away. The result drains the sink first. Outcomes come from the statuses the client observed in the current iteration, because retained results may be from the previous iteration while it runs.
    • This only happens while another live participant still needs the iteration. The last participant keeps today's contract: its result follows iteration end and execution-lease release. Single-client behaviour, warm-state retention and "native lock released before result" are unchanged for the last result.
    • #finishEntryAsync is memoized through entry.finishPromise, so each entry gets exactly one result. The batch's final Promise.all also waits for early results.
    • createBatchReleaseBarrier counts only entries that haven't started finishing.
    • #rejectEntryAsync waits for an in-flight finish instead of racing it.
    • A new #needsIteration (live and not yet finishing) replaces #isEntryLive in the "does anyone still need this iteration?" checks. If every remaining client cancels after others got early results, the iteration is still aborted.
    • A failure in an early result rejects only that client, not the batch.
  • README: the lock/result guarantee now reads "released before the batch's last participant's result". The batching section describes early results.

Compared with the analysis prototype (A02-fix-38-early-result.diff):

  • No setImmediate deferral. The completion event already follows all of the operation's output.
  • Aborted operations suppress early results, so iteration-start failures and aborts keep their error on the result.
  • An early-finish failure can't reject the rest of the batch.
  • The rejection path no longer races.

Compatibility with #6067 and #6065

  • [rush-daemon] Don't let the default queue timeout fail late compatible builds #6067 (queue timeout) only changes admissionController.acquireAsync to acquireGraphExecutionAsync in enqueueAsync. It doesn't overlap these hunks, so the two compose without changes.
  • [rush-daemon] Terminate running operations when a daemon client cancels #6065 (cancellation) adds a very similar mechanism for cancelled participants: finishStarted, #finishDetachedEntry, an in-progress flag on collectOperationOutcomes, and a barrier that skips entries whose finish already started. The two PRs will conflict textually in those spots, but the semantics compose. Whichever lands second should merge finishStarted into finishPromise (or the reverse) and make #needsIteration exclude detached entries. The barrier and outcome changes are the same idea in both PRs.

Tests

  • PhasedRequestBatching.test.ts:
    • publishes a coalesced client result as soon as its own closure settles: B→A with B blocked. A slow reader for client A still gets all of A's output, stream-closed and header before its result. The result arrives while the graph is EXECUTING and before the execution lease is released. B's result comes after released. One schedule, A ran once.
    • publishes an early failure result while the batch continues for other clients: the failing client gets exit 1 early and the other client succeeds after lease release.
    • aborts the iteration when the only client still needing it cancels after an early result.
    • The existing unsubscribes one mid-run cancellation… test now keeps the continuing client's operation outstanding across the cancellation (parallelism 2). Otherwise the continuing client could legitimately finish early, and then aborting the leftover work is correct.
  • PhasedRequestEventSink.test.ts: settlement fires exactly once after all scheduled active operations complete (other operations and duplicate completions are ignored). It does not fire when an active operation was aborted.

Linux validation (WSL Ubuntu-24.04, node 22)

  • rush build --to @rushstack/rush-daemon --to @rushstack/rush-cli-client: passes, including lint.
  • heft test --test-path-pattern "PhasedRequest(Batching|EventSink|Router)": 37/37 passing, 3 runs in a row. The full rush test --only @rushstack/rush-daemon result will be posted as a comment.
  • Before/after repro in one lab run: private mkws-synth workspace, 16-project chain, 1.5 s per op, p16 = 20 s, all sources dirtied, clients started at the same time. "Before" is the unfixed toolchain rush-client; "after" is this branch's rush-client. Load average was about 13–20.
scenario before: exit times after: exit times
--to p04 / --to p08 / --to p12 / --to p16 44.9 / 44.8 / 44.8 / 44.9 s 7.8 / 14.1 / 20.4 / 45.2 s
--only p03 / --only p09 / --to p16 46.4 / 46.4 / 46.4 s 5.9 / 15.3 / 47.7 s

All clients exited 0. Per-client output was identical before and after (4/8/12/16 and 1/1/16 "completed successfully" blocks). Each scenario ran 16 executions over 16 unique operations, so there was no duplicate work.

Follow-ups (not in this PR)

This came out of the automated rushd Linux analysis ("Rushd Hive", bug #38, prototype by analysis agent A02).

… selection completes

A coalesced SHARED-BUILD participant now receives its result as soon as every operation in its own selection has completed and its output has drained, while the iteration and leases continue for the remaining participants. The last participant keeps the result-after-lease-release contract.

Fixes #6084

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

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

Copy link
Copy Markdown
Member Author

Full @rushstack/rush-daemon suite on Linux (WSL Ubuntu-24.04, node 22), with this branch applied to a private lab clone:

  • heft test (full suite, load average about 5–12): 368 passed, 0 failed (369 total, 1 skipped).

  • An earlier rush test --only @rushstack/rush-daemon run at load average about 13–20 hit two 5 s Jest timeouts:

    • NativeIpcFixture.test.ts: a beforeEach hook that runs a real native build.
    • ProductionDaemonRequestResolver.test.ts: drains the real install/update worker result….

    Both files passed 2/2 when re-run in isolation, and the full suite then passed.

Neither test goes through a coalesced batch. They are single-request paths, and this PR leaves the single-request contract unchanged (the last participant still gets its result after the lease is released). They look like pre-existing load-sensitive timeouts in real-native-build tests, similar to the DaemonGraphRequestRouter beforeEach timeout the analysis prototype hit. I did not raise any timeouts. Making those fixtures cheaper under parallel Jest load is a separate follow-up.

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

🟢 Approval recommended

The lifecycle, ordering, cancellation, failure isolation, and lease-release behavior are consistently implemented and covered by focused tests.

Review effort: Balanced
Findings: None

What changed in this PR

Enables coalesced Rush daemon clients to receive results when their own operations settle, avoiding head-of-line blocking.

Changes:

  • Tracks per-client operation settlement and safely publishes early results.
  • Preserves execution-lease behavior for the final participant.
  • Adds coverage for early success, failure, cancellation, and ordered output.
File Description
libraries/​rush-daemon/​src/​PhasedRequestRouter.ts Coordinates early result production and lease release.
libraries/​rush-daemon/​src/​PhasedRequestEventSink.ts Detects when client-selected operations settle.
libraries/​rush-daemon/​src/​test/​PhasedRequestBatching.test.ts Tests early-result batching behavior.
libraries/​rush-daemon/​src/​test/​PhasedRequestEventSink.test.ts Tests settlement tracking and abort suppression.
libraries/​rush-daemon/​src/​test/​PhasedRequestRouterTestUtilities.ts Adds execution-lease test instrumentation.
libraries/​rush-daemon/​README.md Documents revised batching and lease guarantees.
common/​changes/​@rushstack/​rush-daemon/​rushd-coalesced-early-result_2026-09-24.json Records the patch release change.

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

@TheLarkInn
Sean Larkin (TheLarkInn) merged commit caf15b4 into main Sep 24, 2026
11 checks passed
@TheLarkInn
Sean Larkin (TheLarkInn) deleted the thelarkinn-fix-rushd-coalesced-batch-hol-blocking branch September 24, 2026 18:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Closed

Development

Successfully merging this pull request may close these issues.

[rush] rushd: coalesced builds return each client's result only when the largest coalesced request finishes (head-of-line blocking)

3 participants