[1/3] fix: bound Windows ACP shutdown and retain failed cleanup - #2
[1/3] fix: bound Windows ACP shutdown and retain failed cleanup#2slashdevcorpse wants to merge 6 commits into
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
slashdevcorpse
changed the title
|
Corrected the stack trunk: fork main was 231 upstream commits behind the original patch baseline, which caused unrelated history to appear in this PR. Fast-forwarded fork main to the original baseline 12f919b without changing any implementation branch. The intended bottom-layer range contains four shutdown commits across 24 files (+1892/-150). Parts 2 and 3 retain their existing incremental bases. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 809 files
Note: This PR contains a large number of files. cubic selects up to 200 of the highest-priority eligible files for this review, so some files may not have been reviewed.
Re-trigger cubic
|
Verified corrected GitHub PR diff: 24 files, +1892/-150, exactly four shutdown commits. GitHub retained the stale base in native stack metadata after main advanced, so the same PRs were unstacked, the bottom base refreshed, and relinked as native stack LodyAI#6. PR numbers and all implementation commit SHAs are unchanged. |
There was a problem hiding this comment.
All reported issues were addressed across 23 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
2 issues found across 17 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/cli/tests/session-terminate-cleanup.test.ts">
<violation number="1" location="apps/cli/tests/session-terminate-cleanup.test.ts:443">
P3: The new serialization test relies on the in-flight createAgent rejecting with any error, but the only reason `first` rejects quickly is that `createProcessHandle` builds a plain EventEmitter child with no `stdout`/`stderr` properties, so `if (!agentProcess.stdout) throw` fires inside `createAgentOnce`. `first` is also left floating (never awaited directly). If the mock is ever made to expose `stdout`/`stdin` streams, `createAcpClient` would drive a real ACP handshake against a silent child and `await rejected` would stall on the ACP startup timeout instead of failing fast. Make the rejection deterministic by supplying `createProcessHandle` streams that timeout/no-op or by aborting via `abortSignal`, and assert the specific error.</violation>
</file>
<file name="apps/cli/src/lib/lody-fleet.ts">
<violation number="1" location="apps/cli/src/lib/lody-fleet.ts:542">
P3: In `forceTerminateSessions`, `terminal-pty-service.closeAll()` throws `AggregateError('Terminal PTY cleanup failed', [e1, e2, ...])` when one or more PTY kills fail. That nested `AggregateError` is pushed as a single element into the outer `AggregateError('Forced fleet process cleanup failed')`, so the resulting `errors` array is `[AggregateError([...]), ...workspaceRejections]` rather than a flat list of individual failure reasons. Any consumer that iterates `.errors` (or the `toMatchObject({ errors: [...] })` assertion in the new test) won't see the underlying PTY kill failures — the test only passes because its mock throws a plain `Error` instead of the real `AggregateError`. Flatten the terminal failure into the outer array so the individual PTY failures surface consistently with the workspace failures.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| args: ['acp'], | ||
| } as Parameters<Session['createAgent']>[0]; | ||
| const first = session.createAgent(callbacks); | ||
| const rejected = expect(first).rejects.toThrow(); |
There was a problem hiding this comment.
P3: The new serialization test relies on the in-flight createAgent rejecting with any error, but the only reason first rejects quickly is that createProcessHandle builds a plain EventEmitter child with no stdout/stderr properties, so if (!agentProcess.stdout) throw fires inside createAgentOnce. first is also left floating (never awaited directly). If the mock is ever made to expose stdout/stdin streams, createAcpClient would drive a real ACP handshake against a silent child and await rejected would stall on the ACP startup timeout instead of failing fast. Make the rejection deterministic by supplying createProcessHandle streams that timeout/no-op or by aborting via abortSignal, and assert the specific error.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/cli/tests/session-terminate-cleanup.test.ts, line 443:
<comment>The new serialization test relies on the in-flight createAgent rejecting with any error, but the only reason `first` rejects quickly is that `createProcessHandle` builds a plain EventEmitter child with no `stdout`/`stderr` properties, so `if (!agentProcess.stdout) throw` fires inside `createAgentOnce`. `first` is also left floating (never awaited directly). If the mock is ever made to expose `stdout`/`stdin` streams, `createAcpClient` would drive a real ACP handshake against a silent child and `await rejected` would stall on the ACP startup timeout instead of failing fast. Make the rejection deterministic by supplying `createProcessHandle` streams that timeout/no-op or by aborting via `abortSignal`, and assert the specific error.</comment>
<file context>
@@ -401,9 +401,50 @@ it('propagates failed startup cleanup and retains ownership for shutdown', async
+ args: ['acp'],
+ } as Parameters<Session['createAgent']>[0];
+ const first = session.createAgent(callbacks);
+ const rejected = expect(first).rejects.toThrow();
+ await spawning;
+ await expect(session.createAgent(callbacks)).rejects.toThrow('Previous agent ownership');
</file context>
| try { | ||
| this.terminalPtyService.closeAll(); | ||
| } catch (error) { | ||
| terminalFailures.push(error); |
There was a problem hiding this comment.
P3: In forceTerminateSessions, terminal-pty-service.closeAll() throws AggregateError('Terminal PTY cleanup failed', [e1, e2, ...]) when one or more PTY kills fail. That nested AggregateError is pushed as a single element into the outer AggregateError('Forced fleet process cleanup failed'), so the resulting errors array is [AggregateError([...]), ...workspaceRejections] rather than a flat list of individual failure reasons. Any consumer that iterates .errors (or the toMatchObject({ errors: [...] }) assertion in the new test) won't see the underlying PTY kill failures — the test only passes because its mock throws a plain Error instead of the real AggregateError. Flatten the terminal failure into the outer array so the individual PTY failures surface consistently with the workspace failures.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/cli/src/lib/lody-fleet.ts, line 542:
<comment>In `forceTerminateSessions`, `terminal-pty-service.closeAll()` throws `AggregateError('Terminal PTY cleanup failed', [e1, e2, ...])` when one or more PTY kills fail. That nested `AggregateError` is pushed as a single element into the outer `AggregateError('Forced fleet process cleanup failed')`, so the resulting `errors` array is `[AggregateError([...]), ...workspaceRejections]` rather than a flat list of individual failure reasons. Any consumer that iterates `.errors` (or the `toMatchObject({ errors: [...] })` assertion in the new test) won't see the underlying PTY kill failures — the test only passes because its mock throws a plain `Error` instead of the real `AggregateError`. Flatten the terminal failure into the outer array so the individual PTY failures surface consistently with the workspace failures.</comment>
<file context>
@@ -532,14 +532,24 @@ export class LodyFleet {
+ try {
+ this.terminalPtyService.closeAll();
+ } catch (error) {
+ terminalFailures.push(error);
+ }
const results = await Promise.allSettled(
</file context>
|
Superseded by upstream PR LodyAI#456. The series now lives in LodyAI/Lody as LodyAI#456 -> LodyAI#457 -> LodyAI#458. Source branches and review history are preserved. |
Related issue
Refs LodyAI#429
Problem / pressure
Cleanup can discard owned child handles or hide shutdown failures. PID-based taskkill also creates a reuse race between the liveness check and invocation. Pending terminal starts and queued machine requests can be admitted while shutdown tears down their owners, and stalled disposal can consume the forced-cleanup deadline.
Summary
Stack order
Native GitHub stack LodyAI#6 in slashdevcorpse/Lody, created with gh stack link:
Replaces the closed cross-fork LodyAI#430 and LodyAI#435. Each PR has an incremental diff against its predecessor. GitHub native stacks require all branches in the same repository, so this stack is in the contributor fork. It has not merged or landed upstream; LodyAI#429 remains open.
Landing dependency: Ship the complete three-PR stack for the Windows process-tree fix. This first slice supplies safe retained-root termination; PR #4 supplies required descendant and crash ownership. Do not ship PR #2 alone as complete Windows tree cleanup.
Before / after
Test plan
Context handoff
Instructions for reviewing agents
Authoring context
Latest review verification
At 720e84c, the stack includes the eight latest PR #2 review resolutions from 720e84c: PTY admission/draining and retry retention, authentication cleanup ownership, startup owner admission, spawn-error handling, explicit Windows termination semantics and non-retryable cleanup failures. PR #4 supplies the requested descendant Job Object ownership. Local composed verification and exact-head GitHub Static checks, Tests and Desktop E2E all passed. The top PR also passed Windows x64, Windows ARM64 and the same-run dual-architecture npm archive verification.