feat(core): eagerly shutdown plugins that don't provide later hooks#34253
feat(core): eagerly shutdown plugins that don't provide later hooks#34253FrozenPandaz merged 6 commits intomasterfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
✅ Deploy Preview for nx-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
bfdd743 to
d811c6c
Compare
|
View your CI Pipeline Execution ↗ for commit 1a38243
☁️ Nx Cloud last updated this comment at |
🐳 We have a release for that!This PR has a release associated with it. You can try it out using this command: npx create-nx-workspace@0.0.0-pr-34253-d811c6c my-workspaceOr just copy this version and use it in your own command: 0.0.0-pr-34253-d811c6c
To request a new release for this pull request, mention someone from the Nx team or the |
d811c6c to
f7c29c2
Compare
f9e1d83 to
b96340d
Compare
b96340d to
d49bd3f
Compare
d49bd3f to
75bedf2
Compare
75bedf2 to
f6ce841
Compare
f6ce841 to
1d2335a
Compare
|
Failed to publish a PR release of this pull request, triggered by @AgentEnder. |
🐳 We have a release for that!This PR has a release associated with it. You can try it out using this command: npx create-nx-workspace@0.0.0-pr-34253-1d2335a my-workspaceOr just copy this version and use it in your own command: 0.0.0-pr-34253-1d2335a
To request a new release for this pull request, mention someone from the Nx team or the |
182887f to
5ad7412
Compare
5ad7412 to
802dca0
Compare
✅ Deploy Preview for nx-dev ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
🐳 We have a release for that!This PR has a release associated with it. You can try it out using this command: npx create-nx-workspace@22.5.0-pr.34253.802dca0 my-workspaceOr just copy this version and use it in your own command: 22.5.0-pr.34253.802dca0
To request a new release for this pull request, mention someone from the Nx team or the |
| } | ||
|
|
||
| destroy(): void { | ||
| this.shutdown(); |
| export type PluginWorkerResult = AllResults<PluginMessageDefs>; | ||
|
|
||
| /** Any message (request or result) */ | ||
| export type AnyMessage = PluginWorkerMessage | PluginWorkerResult; |
There was a problem hiding this comment.
Remove the unused types
| }; | ||
|
|
||
| /** Extract the full result type for a given key */ | ||
| type ResultOf< |
There was a problem hiding this comment.
Move these library types to a separate file.
| const registered = new Set(registeredHooks); | ||
|
|
||
| // Determine which phases are active and find first/last hooks per phase | ||
| this.activePhases = {}; |
There was a problem hiding this comment.
Rather than active phases can we go with registered phases
| private readonly phaseSessionCount: Partial<Record<Phase, number>> = {}; | ||
|
|
||
| /** Ordered list of phases (derived from HOOKS_BY_PHASE key order, narrowed to active phases) */ | ||
| private readonly phaseOrder: Phase[] = []; |
There was a problem hiding this comment.
make this registered phase order
| 'preTasksExecution', | ||
| ]); | ||
|
|
||
| expect(lifecycle.hasLaterActivePhases('graph')).toBe(true); |
There was a problem hiding this comment.
add a check for preTask
| expect(lifecycle.hasLaterActivePhases('post-task')).toBe(false); | ||
| }); | ||
|
|
||
| it('should return false after post-task phase', () => { |
| * This happens when the plugin has no hooks in the first phase. | ||
| */ | ||
| shouldShutdownImmediately(): boolean { | ||
| return this.phaseOrder[0] !== PHASE_ORDER[0]; |
There was a problem hiding this comment.
Make this check only if the first phase is POST TASK
| it('should identify active phases for single-hook plugin', () => { | ||
| const lifecycle = new PluginLifecycleManager(['createNodes']); | ||
|
|
||
| expect(lifecycle.hasHooksInPhase('graph')).toBe(true); |
There was a problem hiding this comment.
getRegisteredPhases().matchesSnapshot() or .registeredPhases.matchesSnapshot()
| private shutdownIfInactive(): void { | ||
| if (this.pendingCount > 0) { | ||
| logger.verbose( | ||
| `[plugin-pool] worker for "${this.name}" has ${this.pendingCount} pending request(s), not shutting down yet` |
There was a problem hiding this comment.
Change these logs.. because there's no more plugin-pool
| private async ensureAlive(): Promise<void> { | ||
| if (this._alive) { | ||
| return; | ||
| } | ||
|
|
||
| logger.verbose(`[plugin] restarting worker for "${this.name}"`); | ||
| await this.spawnAndConnect(); | ||
| } |
There was a problem hiding this comment.
Race condition when restarting worker
When _alive is false and multiple hooks are called concurrently, they will all pass the if (this._alive) check and call spawnAndConnect() multiple times, spawning duplicate workers.
Example scenario:
- Plugin shuts down after graph phase (
_alive = false) - Two concurrent calls to
postTasksExecutionoccur - Both check
_alive(still false), both callspawnAndConnect() - Two workers get spawned for the same plugin
Fix: Add a flag to track restart-in-progress:
private _restarting = false;
private async ensureAlive(): Promise<void> {
if (this._alive) {
return;
}
if (this._restarting) {
// Wait for in-progress restart
while (this._restarting) {
await new Promise(resolve => setTimeout(resolve, 10));
}
return;
}
this._restarting = true;
try {
logger.verbose(`[plugin] restarting worker for "${this.name}"`);
await this.spawnAndConnect();
} finally {
this._restarting = false;
}
}| private async ensureAlive(): Promise<void> { | |
| if (this._alive) { | |
| return; | |
| } | |
| logger.verbose(`[plugin] restarting worker for "${this.name}"`); | |
| await this.spawnAndConnect(); | |
| } | |
| private _restarting = false; | |
| private async ensureAlive(): Promise<void> { | |
| if (this._alive) { | |
| return; | |
| } | |
| if (this._restarting) { | |
| // Wait for in-progress restart | |
| while (this._restarting) { | |
| await new Promise(resolve => setTimeout(resolve, 10)); | |
| } | |
| return; | |
| } | |
| this._restarting = true; | |
| try { | |
| logger.verbose(`[plugin] restarting worker for "${this.name}"`); | |
| await this.spawnAndConnect(); | |
| } finally { | |
| this._restarting = false; | |
| } | |
| } |
Spotted by Graphite Agent
Is this helpful? React 👍 or 👎 to let us know.
a804993 to
bb26e7a
Compare
Introduce a unified type system for plugin worker messaging that reduces boilerplate. Handlers now return just the payload and infrastructure wraps responses automatically. Update ARCHITECTURE.md with new workflow.
…hitecture Add "why" documentation to help developers understand the intent behind design decisions, not just the technical flows. New sections cover: - Why plugin isolation exists (memory, crashes, global state) - Design principles (Unix sockets, eager shutdown, session counting) - Phase system rationale and shutdown rules - Common pitfalls when modifying the system
ad458aa to
9c278b5
Compare
There was a problem hiding this comment.
✅ The fix from Nx Cloud was applied
These changes fix the snapshot test failures by updating the regex pattern to filter out the new [isolated-plugin] debug logs introduced in this PR. The independent-projects.test.ts file was missing the same regex update that was already applied to independent-projects.workspaces.test.ts, causing snapshot mismatches when the new verbose logging appeared in test output.
Suggested Fix changes
diff --git a/e2e/release/src/independent-projects.test.ts b/e2e/release/src/independent-projects.test.ts
index e531dc9205..b4af7ecc60 100644
--- a/e2e/release/src/independent-projects.test.ts
+++ b/e2e/release/src/independent-projects.test.ts
@@ -238,7 +238,7 @@ describe('nx release - independent projects', () => {
`release version 999.9.9-version-git-operations-test.2 -p ${pkg1} --git-commit --git-tag --verbose` // add verbose so we get richer output
);
const filteredOutput = versionWithGitActionsCLIOutput.replace(
- /\[plugin-(pool|worker)\].*\n/g,
+ /\[(isolated-plugin|plugin-worker)\].*\n/g,
''
);
expect(filteredOutput).toMatchInlineSnapshot(`
@@ -325,7 +325,7 @@ describe('nx release - independent projects', () => {
`release version 999.9.9-version-git-operations-test.3 --verbose --gitTag` // add verbose so we get richer output
);
const filteredConfigOutput = versionWithGitActionsConfigOutput.replace(
- /\[plugin-(pool|worker)\].*\n/g,
+ /\[(isolated-plugin|plugin-worker)\].*\n/g,
''
);
expect(filteredConfigOutput).toMatchInlineSnapshot(`
@@ -525,7 +525,7 @@ describe('nx release - independent projects', () => {
`release changelog 999.9.9-changelog-git-operations-test.1 -p ${pkg1} --verbose`
);
const filteredChangelogOutput = versionWithGitActionsCLIOutput.replace(
- /\[plugin-(pool|worker)\].*\n/g,
+ /\[(isolated-plugin|plugin-worker)\].*\n/g,
''
);
expect(filteredChangelogOutput).toMatchInlineSnapshot(`
This fix was applied by Craigory Coppola
🎓 Learn more about Self-Healing CI on nx.dev
Co-authored-by: AgentEnder <AgentEnder@users.noreply.github.com>
Plugin Isolation Architecture
1. Plugin Loading Flow
1a. Entry Point - Isolation Decision
flowchart TD Start([getPlugins called]) --> CheckIsolation{Isolation<br/>enabled?} CheckIsolation -->|Yes| LoadIsolated[loadIsolatedNxPlugin] CheckIsolation -->|No| LoadInProcess[loadNxPluginInProcess] LoadIsolated --> IsolatedPath([See: Isolated Loading]) LoadInProcess --> InProcessPath([See: In-Process Loading])1b. Isolated Plugin Loading
flowchart TD subgraph Main["Main Process"] Start([loadIsolatedNxPlugin]) --> CheckCache{In cache?} CheckCache -->|Yes| ReturnCached([Return cached promise]) CheckCache -->|No| StaticLoad[IsolatedPlugin.load] StaticLoad --> Resolve[resolveNxPlugin<br/>find plugin path] Resolve --> SpawnWorker[spawn child process] end SpawnWorker -.->|"start process"| WorkerStart subgraph Worker["Worker Process (plugin-worker.ts)"] WorkerStart([process starts]) --> CreateServer[create Unix socket server] CreateServer --> Listen[listen for connections] Listen --> WaitForConnect[wait for main process] WaitForConnect --> HandleLoad[receive 'load' message] subgraph InProcess["In-Process Loading (same as 1c)"] HandleLoad --> RequirePlugin[require plugin module] RequirePlugin --> NormalizePlugin[normalizeNxPlugin] end NormalizePlugin --> SendLoadResult[send 'loadResult'<br/>with hook capabilities] SendLoadResult --> WaitForMessages[wait for hook messages<br/>or socket close] WaitForMessages --> HandleHook{message<br/>received?} HandleHook -->|hook message| ExecuteHook[call plugin.hook] ExecuteHook --> SendResult[send result] SendResult --> WaitForMessages HandleHook -->|socket closed| Cleanup[cleanup & exit] end subgraph Main2["Main Process (continued)"] ConnectSocket[connect via<br/>Unix socket] --> SendLoad[send 'load' message] SendLoad --> WaitLoad[wait for 'loadResult'] WaitLoad --> SetupHooks[setupHooks<br/>create lifecycle manager] SetupHooks --> CheckGraphHooks{Has graph<br/>phase hooks?} CheckGraphHooks -->|No| EarlyShutdown[socket.end<br/>shutdown worker] CheckGraphHooks -->|Yes| KeepAlive[keep worker alive] EarlyShutdown --> Done([Plugin ready]) KeepAlive --> Done end SpawnWorker --> ConnectSocket SendLoadResult -.->|"loadResult"| WaitLoad EarlyShutdown -.->|"socket close"| Cleanup1c. In-Process Plugin Loading
flowchart TD Start([loadNxPluginInProcess]) --> Resolve[resolveNxPlugin] Resolve --> Require[require plugin module] Require --> Normalize[normalizeNxPlugin<br/>wrap hooks] Normalize --> Done([Plugin ready])2. Hook Execution Flow
2a. Isolated Hook Execution
flowchart TD Start([hook called<br/>e.g. createNodes]) --> EnsureAlive{_alive?} EnsureAlive -->|No| Restart[spawnAndConnect<br/>restart worker] Restart --> SetAlive[_alive = true] SetAlive --> EnsureAlive EnsureAlive -->|Yes| EnterHook[lifecycle.enterHook<br/>increment session count] EnterHook --> SendRequest[sendRequest<br/>over socket] SendRequest --> WaitResponse[wait for response<br/>with timeout] WaitResponse --> CheckSuccess{success?} CheckSuccess -->|No| ExitHookError[lifecycle.exitHook] ExitHookError --> ThrowError[throw error] CheckSuccess -->|Yes| ExitHook[lifecycle.exitHook] ExitHook --> CheckShutdown{should<br/>shutdown?} CheckShutdown -->|Yes| Shutdown[shutdown worker] CheckShutdown -->|No| Return([return result]) Shutdown --> Return2b. Shutdown Decision Logic
flowchart TD Start([exitHook called]) --> IsLastHook{Last hook<br/>in phase?} IsLastHook -->|No| NoShutdown1([return false]) IsLastHook -->|Yes| CheckSessions{sessionCount<br/>== 0?} CheckSessions -->|No| NoShutdown2([return false<br/>other callers active]) CheckSessions -->|Yes| CheckLaterPhases{Has later<br/>active phases?} CheckLaterPhases -->|Yes| NoShutdown3([return false<br/>needed later]) CheckLaterPhases -->|No| YesShutdown([return true<br/>safe to shutdown])3. Developer Workflow: Adding/Modifying Plugin Hooks
Step 1: Design Public API
flowchart TD A1[public-api.ts] --> A2[Define context type<br/>e.g. MyHookContext] A2 --> A3[Export new types] A3 --> A4[loaded-nx-plugin.ts] A4 --> A5[Add hook to<br/>LoadedNxPlugin interface]Step 2: Define Message Types
flowchart TD B1[messaging.ts] --> B2[Add entry to PluginMessageDefs] B2 --> B3[Define payload and result types] B3 --> B4[Add to MESSAGE_TYPES array] B4 --> B5[Add to RESULT_TYPES array]The messaging system uses a unified
DefineMessagespattern. To add a new message:The individual message/result types (
PluginWorkerMyHookMessage,PluginWorkerMyHookResult)are automatically derived. Export them if needed for external use:
Step 3: Handle in Worker Process
flowchart TD C1[plugin-worker.ts] --> C2[Add handler in<br/>consumeMessage] C2 --> C3["Call plugin.myHook()"] C3 --> C4[Return result payload]Handlers return just the result payload - the infrastructure wraps it automatically:
Step 4: Update Load Result
flowchart TD D1[messaging.ts] --> D2[Add hasMyHook to<br/>load.result in PluginMessageDefs] D2 --> D3[plugin-worker.ts] D3 --> D4[Populate hasMyHook<br/>in load handler]Step 5: Wire Up IsolatedPlugin
flowchart TD E1[isolated-plugin.ts] --> E2[Add hook property<br/>to class] E2 --> E3[Update LoadResultPayload<br/>type export] E3 --> E4[Add to registeredHooks<br/>array in setupHooks] E4 --> E5[Add wrapped hook<br/>implementation] E5 --> E6["wrap('myHook', async (ctx) => {<br/> sendRequest('myHook', { context: ctx })<br/>})"]Step 6: Update Lifecycle Phases (if needed)
flowchart TD F1{New phase<br/>needed?} -->|Yes| F2[plugin-lifecycle-manager.ts] F2 --> F3[Add phase to<br/>HOOKS_BY_PHASE] F1 -->|No| F4[Add hook to existing<br/>phase array in HOOKS_BY_PHASE]Step 7: Add Tests
flowchart TD G1[isolated-plugin.spec.ts] --> G2[Test hook registration] G2 --> G3[Test hook execution] G3 --> G4[Test restart behavior] G4 --> G5[plugin-lifecycle-manager.spec.ts] G5 --> G6[Test phase transitions<br/>with new hook] G6 --> G7[Test shutdown decisions]File Reference
../public-api.ts../loaded-nx-plugin.tsmessaging.tsplugin-worker.tsisolated-plugin.tsplugin-lifecycle-manager.tsload-isolated-plugin.ts../get-plugins.tsLifecycle Phases
Shutdown rules:
postTasksExecution: shutdown immediately after load, restart when needed