[rush] Add per-iteration runner persistence control - #5888
Open
Bharat Middha (bmiddha) wants to merge 10 commits into
Open
[rush] Add per-iteration runner persistence control#5888Bharat Middha (bmiddha) wants to merge 10 commits into
Bharat Middha (bmiddha) wants to merge 10 commits into
Conversation
Bharat Middha (bmiddha)
changed the title
David Michon (dmichon-msft)
requested changes
Jul 20, 2026
Mo Jazayeri (mojaza)
approved these changes
Jul 21, 2026
Bharat Middha (bmiddha)
changed the title
Mo Jazayeri (mojaza)
approved these changes
Jul 28, 2026
Mo Jazayeri (mojaza)
left a comment
Mo Jazayeri (mojaza)
left a comment
Contributor
There was a problem hiding this comment.
LGTM Overall!
David Michon (dmichon-msft)
requested changes
Aug 11, 2026
Move IPC runner teardown into the operation graph's per-iteration options while preserving resident runners when no policy is supplied. Cover persistent, one-shot, changing, and default policies across successive iterations. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8f21c8c8-452a-4922-aca3-fb40b007f948
Close nonpersistent IPC runners before downstream execution, retain fallback cleanup for bypassed active runners, and rename the policy to shouldRunnerPersist. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8f21c8c8-452a-4922-aca3-fb40b007f948
Replace the per-iteration predicate with a mutable record boolean configured alongside enabled. Keep runner teardown in OperationGraph so cold runners close before dependents execute, with a pre-after-iteration fallback for bypassed records. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8f21c8c8-452a-4922-aca3-fb40b007f948
Convert fallback close failures into operation and iteration failures so final status and post-iteration hooks still run. Cover the bypassed-runner failure path with an ordering regression test. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8f21c8c8-452a-4922-aca3-fb40b007f948
Close terminal records' output resources before iteration summary hooks. This lets runner cleanup failures on bypassed records be summarized without an open StdioSummarizer exception. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8f21c8c8-452a-4922-aca3-fb40b007f948 Assistant-model: GPT-5.6 Sol
Mark the new public persistence control as a minor change and document why cleanup does not race execution or run twice. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8ebd5bf2-c44b-42d5-be25-e7936d4b0a14
Pass persistence through the runner context so IPC processes close during execution. Keep batched end-of-iteration cleanup as an idempotent fallback for bypassed runners. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8ebd5bf2-c44b-42d5-be25-e7936d4b0a14
Bharat Middha (bmiddha)
force-pushed
the
bmiddha/ws0-t4-persist-policy-hook
branch
from
August 12, 2026 23:40
7f9c3a4 to
8d9b896
Compare
David Michon (dmichon-msft)
approved these changes
Aug 13, 2026
David Michon (dmichon-msft)
left a comment
David Michon (dmichon-msft)
left a comment
Contributor
There was a problem hiding this comment.
Better, but I wonder about the state of the cleanup records.
Contributor
There was a problem hiding this comment.
Should this be using Promise.allSettled so that we can report the individual results?
Member
Author
There was a problem hiding this comment.
updated to use Promise.allSettled
Preserve successful runner state updates when another cleanup fails, and report failures only for affected operations. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8ebd5bf2-c44b-42d5-be25-e7936d4b0a14
| super(innerError.message); | ||
| this.name = OperationRunnerCloseError.name; | ||
| this.operation = operation; | ||
| this.innerError = innerError; |
Contributor
There was a problem hiding this comment.
This is probably now error.cause
| } | ||
| } | ||
|
|
||
| class OperationRunnersCloseError extends Error { |
Contributor
There was a problem hiding this comment.
There's a concept called AggregateError: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/AggregateError
| record.error = e; | ||
| record.status = OperationStatus.Failure; | ||
| } | ||
| _onOperationComplete(record, state); |
Contributor
There was a problem hiding this comment.
You could do the close runners for the operation here instead of waiting for the end of the iteration if you want to force it to be closed.
Expose modern built-in error types through the Node rig so runner cleanup can use AggregateError and Error.cause without file-level lib overrides. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8ebd5bf2-c44b-42d5-be25-e7936d4b0a14
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
4 participants
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 opt-in, per-iteration runner persistence control to the operation graph. Hosts can keep selected runners warm between iterations and tear down cold runners, while the default behavior remains unchanged.
Details
IConfigurableOperationnow exposes a mutableshouldRunnerPersist: boolean, defaulting totrue. Hosts set it duringconfigureIterationalongsideenabled, giving both controls the same per-operation, per-iteration semantics.IOperationExecutionResultexposes the configured value as readonly.After
afterExecuteOperationAsync, the graph closes a completed operation's runner when the value isfalse, before queue completion can unblock downstream operations. Cold operations that bypass normal completion are closed beforeafterExecuteIterationAsync. If one of those fallback closes fails, the current operation record and iteration become failures, the error is reported, and iteration finalization still runs instead of leaving the graph in theExecutingstate. Terminal output resources are finalized before iteration hooks so summary consumers can safely inspect failures for records that never executed.This moves persistence ownership out of
IPCOperationRunnerPluginand keepsIPCOperationRunnerpolicy-agnostic. The plugin no longer hard-codespersist: true; runners remain resident by default, so existing watch behavior is preserved. The new control has no production caller yet and is available for a future daemon warm-selection policy.The record-based control was chosen over an iteration callback because
configureIterationalready owns per-iteration operation decisions and mutable record state avoids threading policy through runner contexts.The public additions are
@alpha, so there is no stable API compatibility break. The internalIIPCOperationRunnerOptions.persistfield was removed; unsupported deep-import callers must remove that argument. Each iteration performs a linear record sweep, and cold-runner cleanup is bounded by the graph's configured parallelism.How it was tested
cd libraries/rush-lib && node_modules/.bin/heft test --test-path-pattern OperationGraph- 42/42 tests passednode common/scripts/install-run-rush.js test --to @microsoft/rush-lib- 718/718 tests passed across 75 suitesnode common/scripts/install-run-rush.js build -t rush- passednode common/scripts/install-run-rush.js change --verifyapps/rush/lib-commonjs/start-dev.jsin an isolated Git-backed Rush repo - default reuse, cold teardown, bypass fallback teardown, and injected cleanup-failure scenarios all passedThe tests cover two successive iterations with default, persistent, one-shot, and changing policies. They also verify teardown before downstream execution, fallback teardown before
afterExecuteIterationAsync, and cleanup-failure reporting/finalization for bypassed runners. The local CLI harness confirmed that the default policy reused one IPC child PID, the cold policy created a fresh PID per iteration and exited each child before its dependent ran, a bypassed cold runner exited before the iteration post-hook, and an injected fallback close failure reached the normal Rush failure summary without an uncaught exception.