-
Notifications
You must be signed in to change notification settings - Fork 702
[rush] Add per-iteration runner persistence control #5888
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
55e1bf4
94af331
4854b5b
9eb5f0c
e20c69f
bd9cd75
7f9c3a4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| { | ||
| "changes": [ | ||
| { | ||
| "packageName": "@microsoft/rush", | ||
| "comment": "Add a per-iteration, host-driven runner persist policy so IPC runners can be kept hot or torn down per operation per iteration, instead of fixing persistence at plugin construction.", | ||
| "type": "minor" | ||
| } | ||
| ] | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,7 +28,6 @@ export interface IIPCOperationRunnerOptions { | |
| initialCommand: string; | ||
| incrementalCommand: string | undefined; | ||
| commandForHash: string; | ||
| persist: boolean; | ||
| ignoredParameterValues: ReadonlyArray<string>; | ||
| } | ||
|
|
||
|
|
@@ -58,7 +57,6 @@ export class IPCOperationRunner implements IOperationRunner { | |
| private readonly _initialCommand: string; | ||
| private readonly _incrementalCommand: string | undefined; | ||
| private readonly _commandForHash: string; | ||
| private readonly _persist: boolean; | ||
| private readonly _ignoredParameterValues: ReadonlyArray<string>; | ||
|
|
||
| private _ipcProcess: ChildProcess | undefined; | ||
|
|
@@ -72,7 +70,6 @@ export class IPCOperationRunner implements IOperationRunner { | |
| initialCommand, | ||
| incrementalCommand, | ||
| commandForHash, | ||
| persist, | ||
| ignoredParameterValues | ||
| } = options; | ||
| this.name = name; | ||
|
|
@@ -83,7 +80,6 @@ export class IPCOperationRunner implements IOperationRunner { | |
| this._incrementalCommand = incrementalCommand; | ||
| this._commandForHash = commandForHash; | ||
|
|
||
| this._persist = persist; | ||
| this._ignoredParameterValues = ignoredParameterValues; | ||
| } | ||
|
|
||
|
|
@@ -100,7 +96,6 @@ export class IPCOperationRunner implements IOperationRunner { | |
| const invalidate: (reason: string) => void = context.getInvalidateCallback(); | ||
| return await context.runWithTerminalAsync( | ||
| async (terminal: ITerminal, terminalProvider: ITerminalProvider): Promise<OperationStatus> => { | ||
| let isConnected: boolean = false; | ||
| if (!this._ipcProcess || typeof this._ipcProcess.exitCode === 'number') { | ||
| // Log any ignored parameters | ||
| if (this._ignoredParameterValues.length > 0) { | ||
|
|
@@ -202,9 +197,7 @@ export class IPCOperationRunner implements IOperationRunner { | |
| subProcess.on('message', finishHandler); | ||
| subProcess.on('error', reject); | ||
| subProcess.on('exit', onExit); | ||
|
|
||
| this._processReadyPromise!.then(() => { | ||
| isConnected = true; | ||
| terminal.writeLine('Child supports IPC protocol. Sending "run" command...'); | ||
| const runCommand: IRunCommandMessage = { | ||
| command: 'run' | ||
|
|
@@ -213,10 +206,6 @@ export class IPCOperationRunner implements IOperationRunner { | |
| }, reject); | ||
| }); | ||
|
|
||
| if (isConnected && !this._persist) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should just become |
||
| await this.closeAsync(); | ||
| } | ||
|
bmiddha marked this conversation as resolved.
|
||
|
|
||
| // @rushstack/operation-graph does not currently have a concept of "Success with Warning" | ||
| // To match existing ShellOperationRunner behavior we treat any stderr as a warning. | ||
| return status === OperationStatus.Success && hasWarningOrError | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -84,6 +84,11 @@ export class OperationExecutionRecord implements IOperationRunnerContext, IOpera | |
| */ | ||
| public enabled: boolean; | ||
|
|
||
| /** | ||
| * If true, this operation's runner should remain active after this iteration. | ||
| */ | ||
| public shouldRunnerPersist: boolean = true; | ||
|
|
||
| /** | ||
| * This number represents how far away this Operation is from the furthest "root" operation (i.e. | ||
| * an operation with no consumers). This helps us to calculate the critical path (i.e. the | ||
|
|
@@ -448,11 +453,18 @@ export class OperationExecutionRecord implements IOperationRunnerContext, IOpera | |
| // Delegate global state reporting | ||
| await executeContext.onResultAsync(this); | ||
| } finally { | ||
| if (this.isTerminal) { | ||
| this._collatedWriter?.close(); | ||
| this.stdioSummarizer.close(); | ||
| this.problemCollector.close(); | ||
| } | ||
| this.finalize(); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Closes per-record output resources after the record reaches a terminal state. | ||
| */ | ||
| public finalize(): void { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this a public API? It should only ever need to get invoked in the finally above, because the terminal may not be written after that |
||
| if (this.isTerminal) { | ||
| this._collatedWriter?.close(); | ||
| this.stdioSummarizer.close(); | ||
| this.problemCollector.close(); | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -799,6 +799,10 @@ export class OperationGraph implements IOperationGraph { | |
| onStartAsync: onOperationStartAsync, | ||
| onResultAsync: onOperationCompleteAsync | ||
| }; | ||
| // Track runners closed by normal operation completion so the end-of-iteration fallback does not | ||
| // issue a concurrent or duplicate closeAsync() call for the same runner. | ||
| const recordsWithRunnerCleanup: Set<OperationExecutionRecord> = new Set(); | ||
| const graph: OperationGraph = this; | ||
|
|
||
| if (!this.quietMode) { | ||
| const plural: string = totalOperations === 1 ? '' : 's'; | ||
|
|
@@ -873,9 +877,36 @@ export class OperationGraph implements IOperationGraph { | |
| }); | ||
| } | ||
|
|
||
| const recordsToClose: OperationExecutionRecord[] = []; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This regressed again. Runner closing must happen during the operation's own execution phase, or else we have RAM exhaustion concerns. |
||
| for (const record of executionRecords.values()) { | ||
| if (!recordsWithRunnerCleanup.has(record) && !record.shouldRunnerPersist) { | ||
| recordsToClose.push(record); | ||
| } | ||
| } | ||
| function reportRunnerCleanupFailure(record: OperationExecutionRecord, error: Error): void { | ||
|
bmiddha marked this conversation as resolved.
|
||
| record.error = error; | ||
| record.status = OperationStatus.Failure; | ||
| _reportOperationErrorIfAny(record); | ||
| state.hasAnyFailures = true; | ||
| } | ||
| await Async.forEachAsync( | ||
| recordsToClose, | ||
| async (record: OperationExecutionRecord) => { | ||
| try { | ||
| await this.closeRunnersAsync([record.operation]); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we please call this API with all of them at once, and if you want cleaner error handling, update the API's error reporting? |
||
| } catch (e) { | ||
| reportRunnerCleanupFailure(record, e); | ||
| } | ||
| }, | ||
| { concurrency: this.parallelism } | ||
| ); | ||
|
bmiddha marked this conversation as resolved.
|
||
| for (const record of executionRecords.values()) { | ||
| record.finalize(); | ||
| } | ||
|
|
||
| const status: OperationStatus = (() => { | ||
| if (bailStatus) return bailStatus; | ||
| if (state.hasAnyFailures) return OperationStatus.Failure; | ||
| if (bailStatus) return bailStatus; | ||
| if (state.hasAnyAborted) return OperationStatus.Aborted; | ||
| if (state.hasAnyNonAllowedWarnings) return OperationStatus.SuccessWithWarning; | ||
| if (iterationContext.totalOperations === 0) return OperationStatus.NoOp; | ||
|
|
@@ -1027,6 +1058,18 @@ export class OperationGraph implements IOperationGraph { | |
| record.error = e; | ||
| record.status = OperationStatus.Failure; | ||
| } | ||
| if (!record.shouldRunnerPersist) { | ||
| // The runner's executeAsync() and the post-operation hook have both settled before cleanup, | ||
| // so persistence cleanup cannot race this operation's execution. | ||
| recordsWithRunnerCleanup.add(record); | ||
| try { | ||
| await graph.closeRunnersAsync([record.operation]); | ||
| } catch (e) { | ||
| _reportOperationErrorIfAny(record); | ||
| record.error = e; | ||
| record.status = OperationStatus.Failure; | ||
| } | ||
| } | ||
| _onOperationComplete(record, state); | ||
| } | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we flip this so it defaults to false?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before this change,
IPCOperationRunnerPluginhard-codedpersist: true, so runners persisted by default. Flipping this tofalsechanges existing behavior.