Repository navigation
Conversation
…s children The extension's session_shutdown handler disposed every live in-process child through the process-wide factory. In a single-session CLI process that is exactly right, but a multi-session host such as pi-web runs many sessions in one process and shuts their wrappers down one at a time (its idle reaper closes any wrapper whose page stopped renewing its lease). One such shutdown aborted every session's running subagents mid-flight: the child died with `Request was aborted` while its parent session kept running, so the loss looked like a provider failure. Track each live child by the session file of the parent that launched it and add disposeForParent(), which aborts and disposes only that session's attached children (children of file-less parents match undefined). session_shutdown now calls it with the extension's own session file; dispose() keeps its whole-process semantics for the detached runner's exit path, and detached children stay out of both scopes.
Contributor
[Medium impact] The change affects child cleanup in processes shared by multiple parent sessions. Do not merge yet: shutdown still stops other sessions' children, and required contributor credit is missing. Findings
Fix with agent prompt### Issue 1
CHANGELOG.md:7
Merge gate: the new `CHANGELOG.md` entry omits credit for @Jarcis-cy and PR #3204. The repository requires the contributor's login or name and a PR or issue number for material external work. Add that credit before merging.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
### Issue 2
src/extension/index.ts:1269
`runtimeEntry.cleanup()` sets `state.parentSessionFile` to `null` before this call reads it. Every normal shutdown therefore calls `disposeChildSessions(undefined)`, which disposes all attached children across the process. Closing one session still aborts other sessions' running children.
Capture the parent's file before cleanup. Add a focused test through the actual `session_shutdown` handler; the new factory-only tests miss this ordering.
### Issue 3
src/runs/shared/child-session.ts:826-827
`disposeChildSessions(undefined)` means whole-process disposal, but the extension also passes `undefined` for a parent without a session file. Even after fixing the cleanup ordering, shutting down a file-less parent will still abort file-backed parents' children.
Distinguish an omitted argument from an explicitly supplied `undefined`, or expose separate scoped and whole-process helpers, so file-less shutdown reaches `disposeForParent(undefined)`.
### Issue 4
src/runs/shared/child-session.ts:514-516
`disposeForParent()` now increments the shared `disposals` counter. If parent B is opening an MCP child while parent A is disposed, B sees the changed counter at line 715 or 719 and shuts its session down, even though its parent did not match.
Track scoped disposal separately for each parent while retaining a whole-process counter. Cover disposal of A while B waits for its MCP tools.
### Issue 5
src/runs/shared/herdr-placed-run.ts:175
The new placement `disposeForParent()` disposes the entire `remote` set and clears it without checking `parentSessionFile`. The process-wide placement factory can hold children from several parents, so scoped cleanup for A also closes B's bridge and connection. B loses control of its running remote work.
Record each remote child's launching parent and dispose only matching attached children, leaving the others tracked.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
|
…ent disposal epochs Review follow-ups on nicobailon#3204: - Capture the parent session file before runtimeEntry.cleanup() clears it: the handler read state.parentSessionFile after cleanup had already nulled it, so every session_shutdown disposed the whole process and the scoping never took effect. A new extension-level test drives the real handlers and asserts the scoped call, which fails against that ordering. - Split disposeChildSessionsForParent(parent) from the whole-process disposeChildSessions() so a parent without a session file no longer rides the whole-process path; undefined is now an explicit file-less scope. - Track disposal epochs per parent (plus a global counter): a scoped disposal of one parent no longer trips the launch-race guard of another parent's child waiting for its MCP tools. - Key the placement wrapper's remote children by their launching parent so disposeForParent closes only that parent's remote bridges. - Credit the CHANGELOG entry per the repository's contributor rule.
Contributor
|
Your organization has run out of review credits. Add credits to resume code reviews. |
Author
|
Thanks for the review — all five findings were valid, fixed in 8a58b9f (typecheck clean, full unit suite 4359 pass / 0 fail):
|
This branch has not been deployed
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
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.
Problem
In a multi-session host such as pi-web, one session's
session_shutdownaborted every session's running in-process subagents.The extension's shutdown handler called
disposeChildSessions(), which disposes the process-wide factory's wholeliveset — not just the children of the session that is shutting down. In a single-session CLI process the two sets are identical, so nothing was visible there. In pi-web, which runs many sessions per process, the difference matters:session_shutdownto its own extension instance.Request was aborted(its provider fetch is aborted) while its parent session keeps running, so the loss masquerades as a provider failure.Evidence (reproduced on a live pi-web host)
Two subagent runs in session
01a1ae62-26fcdied at 09:47:18 (446 s of work) and 10:00:01 (952 s) withRequest was aborted. Stack probes on the kill path showed both entering throughdisposeChildSessions()←session_shutdown, while the wrapper-shutdown diagnostic log recorded the shutdowns on different, idle sessions:The victim session's own wrapper was correctly spared (
running=true) the whole time; only its child was killed by the sibling's cleanup. A kill of this shape also explains clusters where children of several sessions die within milliseconds of each other: one reaped wrapper, one global dispose.Fix
ChildSessionLaunch.parentSessionFilealready carried it; children of file-less parents map toundefined).disposeForParent(parentSessionFile)aborts and disposes only that session's attached children, including MCP waits still starting for it.session_shutdownnow callsdisposeChildSessions(state.parentSessionFile); without an argument the whole-process behavior is kept for the detached runner's exit path.Tests
test/unit/child-session-scoped-dispose.test.tscovers: cross-session disposal leaves the other parent's child running, file-less-parent scoping, whole-processdispose()semantics, and detached children surviving both scopes. Full unit suite: 4358 pass / 0 fail;tsc --noEmitclean.