[rush] Add per-iteration IPCOperationRunner persistence policy#5888
Open
bmiddha wants to merge 3 commits into
Open
[rush] Add per-iteration IPCOperationRunner persistence policy#5888bmiddha wants to merge 3 commits into
bmiddha wants to merge 3 commits into
Conversation
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
bmiddha
requested review from
apostolisms,
dmichon-msft,
iclanton,
jxanthony and
octogonz
as code owners
July 20, 2026 23:21
bmiddha
changed the title
bmiddha
commented
Jul 20, 2026
dmichon-msft
requested changes
Jul 20, 2026
Comment on lines
-216
to
-218
| if (isConnected && !this._persist) { | ||
| await this.closeAsync(); | ||
| } |
Contributor
There was a problem hiding this comment.
If persistence is not desired, we absolutely must invoke the close at this step in the process or we risk RAM exhaustion.
mojaza
approved these changes
Jul 21, 2026
| * When omitted, all runners remain active. Returning `false` causes the operation's runner to be closed | ||
| * after the iteration, including when the operation was disabled for that iteration. | ||
| */ | ||
| getRunnerPersistence?: (operation: Operation) => boolean; |
There was a problem hiding this comment.
nit: shouldRunnerPersist seems closer to the comment IMO!
| subProcess.on('exit', onExit); | ||
|
|
||
| this._processReadyPromise!.then(() => { | ||
| isConnected = true; |
There was a problem hiding this comment.
Why is the variable isConnected is not needed anymore? Is getRunnerPersistence gonna be in place in the parent callers?
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
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
3 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 an opt-in, per-iteration runner persistence policy to the alpha operation graph API. Hosts can keep selected IPC runners warm between iterations and tear down cold runners, while the default behavior remains unchanged when no policy is supplied.
Details
IOperationGraphIterationOptionsnow acceptsgetRunnerPersistence?: (operation: Operation) => boolean. The operation graph preserves this callback with the scheduled iteration and, after execution hooks and telemetry, closes runners for operations that returnfalsethrough the existing selectivecloseRunnersAsync()path.This moves persistence ownership out of
IPCOperationRunnerPluginand removes constructor-time persistence state fromIPCOperationRunner. IPC runners remain resident by default, so existing watch behavior is unchanged. The new policy has no production caller yet and is available for a future daemon warm-selection policy.An iteration option was chosen instead of a dedicated hook or runner-local dynamic callback because the host already supplies iteration state and the graph already owns runner teardown. The public change is additive and
@alpha; unsupported deep-import callers of the internal IPC runner options must remove the oldpersistargument. When no callback is supplied, the graph performs no policy scan or teardown.How it was tested
node common/scripts/install-run-rush.js test --to @microsoft/rush-libcd libraries/rush-lib && node_modules/.bin/heft test --test-path-pattern OperationGraph— 39/39 tests passedcd libraries/rush-lib && node_modules/.bin/heft test --clean— 715/715 tests passed across 75 suitesnode common/scripts/install-run-rush.js change --verifyThe new tests exercise two successive iterations and verify the no-policy default, always-persistent, always-one-shot, and changing per-iteration policies.