HOMELAB-1558: feat(subagents): persisted child sessions (flag-gated, single-writer enforced) #25
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "plane/HOMELAB-1558-phase4"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
What
Phase 4 of HOMELAB-1555 (ticket HOMELAB-1558): persisted child sessions — the option-B "actual sessions" ask. Every subagent gets a real pi v3 session file, flag-gated, without breaking Workflow Mode or the single-writer invariant.
persistChildSessionsinsubagents.json(+ settings page), default OFF. OFF = byte-identical spawn (SessionManager.inMemory(cwd)— arity-asserted in tests).<agentDir>/subagent-sessions/<encodeCwd>/<treeId>/— proven invisible to pi's picker (SessionManager.listAll/listscan<agentDir>/sessions/only) and to rpiv-workflow'slocateSessionFile(reads onlydirname(ref.file); rungs are existsSync /*_<id>.jsonl/ header-id — never up/sideways/recursive; child ids aresub-<agentId>so id-equality can't match a workflow id; replica-tested incl. child file copied into root dir). Workflow Mode can never adopt a child as stage driver.agent-runner.ts→createChildSessionManager({persist, cwd, agentId, treeId, parentSession})→SessionManager.create(cwd, dir, {id, parentSession}); flushes on first assistant turn; replayable viaSessionManager.open(tested) → post-mortem resume/fork + per-session cost attribution.rootSessionId ?? getSessionId()split hazard is real — fixed withresolveTreeAnchor()at all three sites +pinRootSessionAnchor()at rootsession_start(re-pins on/new//resume/--fork); nested lead+worker land in one dir (tested).<sessionFile>.live.json(0600) per live child, cleared on terminal/close/remove/dispose. A process opening a live foreign marker → loud warning +ctx.shutdown()+ spawn-refusal latch; same-pid self and stale (dead-pid) exempt. Resume is post-mortem only, enforced not just documented.Deviations
Session-file link in TUI detail row (not grid card); no retention sweep yet (documented);
shutdownguard gated off under vitest.Verify
tsc --noEmitcleanvitest run: 65 files / 621 tests pass (42 new) — flag-off identity, dir isolation (real listAll/list + locateSessionFile replica), replay via SessionManager.open, tree-anchor pinning, guard lifecycle (create/clear/stale/foreign/self)subagents.json)Reviewer focus: guard semantics (shutdown hammer, fail-closed unreadable marker), the locateSessionFile isolation claim, tree-anchor pin across session switches, flag-off byte-identity.
Refs HOMELAB-1558, epic HOMELAB-1555.
Flag-gated `persistChildSessions` (subagents.json + suite settings page, default OFF): every subagent gets a real pi v3 session file instead of an in-memory session, so `pi --session <child>` replays it post-mortem, `--fork` branches it, and per-session cost attribution becomes queryable. Off is the exact pre-feature call — `SessionManager.inMemory(cwd)`, arity-asserted in tests, nothing on disk. - New `core/child-sessions.ts`: the whole feature seam. Dir namespace `<agentDir>/subagent-sessions/<encodedCwd>/<treeId>/` — a sibling of pi's `sessions/`, so `SessionManager.listAll/list` (real pi) never see a child, and rpiv-workflow's `locateSessionFile` (which reads only `dirname(ref.file)`, rungs 1–3, never recursing) can never adopt one as a stage driver. Second layer: child ids are `sub-<agentId>`, so the rungs' id equality cannot match a workflow session id either. - The one spawn site (`agent-runner.ts`) now calls `createChildSessionManager({persist, cwd, agentId, treeId, parentSession})`; explicit `{id, parentSession}` linkage, file written on the first assistant turn like any pi session (verified replayable via `SessionManager.open`). - Tree anchor: `resolveTreeAnchor()` + `pinRootSessionAnchor()` replace the three `rootSessionId ?? getSessionId()` sites in agent-manager. The root pins once at `session_start` (and re-pins on `/new`, `/resume`, `--fork`); children inherit the stamped id, so a lead's worker lands in the ROOT's dir, never in a dir named after the lead's own session id. - Single-writer rule (pi has no locking — verified `SessionManager.open` appends with no owner check): the parent writes `<sessionFile>.live.json` for every live child and clears it on every terminal/dispose path. A pi whose current session file carries a live foreign marker warns loudly, refuses to spawn, and gracefully shuts down; the in-process child (same pid + same session id) and stale markers (dead holder → cleaned) are exempt, so post-mortem resume works. `pi --fork` documented as the safe live alternative. - Linkage surfaces: `AgentRecord.sessionFile`, task-store `session.json`, `.output` entries, `<session-file>` in the task notification, `subagents:*` event payloads, and the swarm TUI detail row. Tests: 621 passed (65 files); 42 new — dir/id/tree/guard units, spawn-path wiring (marker lifecycle, task link, transcript stamp, flag-off no-op), settings round-trip, TUI diagnosis. `npm run typecheck` clean. No pi-core changes.Landing review — BLOCKERS → NOT merged
Independent review in a fresh one-shot context (
pi -p --no-session, reviewer ≠ implementer), built from primary sources only: PR diff13ba30d..9e2bfb9, the ticket's acceptance criteria, and the real installed implementations (pidist/,@juicesharp/rpiv-workflow/sessions/locate.ts). No implementer summary was used as the brief.Local verification reproduced by me on the branch head:
npx tsc --noEmitclean,npx vitest run→ 65 files / 621 tests passed.BLOCKER 1 — the single-writer stop is a silent no-op in print/JSON mode, so a second writer really does append to a live child's file
Acceptance criterion 4 ("enforced: never resume a LIVE child's file") and the docstring/README claim "any pi process whose current session file carries a live marker refuses to run" are false for
-pand--mode json.Verified chain (installed pi
~/.npm-global/lib/node_modules/@earendil-works/pi-coding-agent):src/core/child-sessions.ts:351-355— the violation branch's only stop ishooks.shutdown?.().src/core/index.ts:599— that hook isctx.shutdown?.();:601gates it only onVITEST.dist/core/agent-session.js:2073—shutdown: () => { this._extensionShutdownHandler?.(); };:1919-1920assigns_extensionShutdownHandleronly if the mode passed one.dist/modes/print-mode.js:53-84—bindExtensions({ mode, commandContextActions, onError }): noshutdownHandler(the file contains zero occurrences of "shutdown"). Print mode serves both-ptext and--mode json.dist/modes/interactive/interactive-mode.js:1458anddist/modes/rpc/rpc-mode.js:256do pass one — so the guard works there and silently doesn't everywhere else.dist/modes/print-mode.js:105then runsawait session.prompt(initialMessage), anddist/core/session-manager.js:739-747(_persist) short-circuits onhasAssistant— a resumed child file does already contain assistant entries — so itappendFileSyncs straight into the live child's file.Net effect:
pi -p --session <live child>.jsonl "…"prints the SINGLE-WRITER VIOLATION banner, latches the spawn refusal, does not exit, and writes a second writer into one append-only tree. That is exactly the corruption this criterion exists to prevent.Repro (deterministic, no cluster): with
persistChildSessionson, spawn a long-lived background child, then runpi -p --session ~/.pi/agent/subagent-sessions/<encCwd>/<treeId>/<ts>_sub-<id>.jsonl "say hi"— the warning prints, the process continues, and the file gains the second process's turns.Fix direction (owner's call, but the stop must not be optional): have the guard hard-stop unconditionally — e.g.
process.exit(1)on violation (extensions are free to do this; no pi-core change needed) — or, if the mode-dependent stop is kept, narrow the claim insrc/core/child-sessions.ts:33andREADME.md:376to interactive/RPC and stop advertising enforcement. Docs and enforcement must agree either way.Concerns (non-blocking)
src/core/child-sessions.ts:311-327— pid-recycling false positive is availability-destructive. A crashed parent's marker whose pid is later recycled →isPidAlivetrue → unescapableviolation: a legitimate post-mortem resume of a finished child is warned, spawn-latched, and shut down, and the violating branch never clears the marker (onlystaledoes). Nothing bounds marker age. Cheap mitigation: record/compare the holder's process start time (/proc/<pid>/statfield 22) or a boot id.src/core/agent-manager.ts:744-757,src/core/output-file.ts:87— the replay handle can point at a nonexistent file.SessionManager.createwrites the file lazily (first assistant message — asserted by this PR's own test attests/core/child-sessions.test.ts:197-202), yet the marker,record.sessionFile,session.jsonand every.outputentry are stamped at spawn. A child that dies before its first assistant turn leaves links to a missing path;pi --session <that path>then creates a new empty session at that path with a different id instead of replaying (dist/core/session-manager.jsopen:existsSyncfalse → freshnewSession()).src/core/child-sessions.ts:295-308— theselfexemption keys on pid+id equality only, so it cannot distinguish "the child's own spawn-site instance" from any other manager in this process that opens the same file. No reachable trigger found today, hence a concern.Suggestions
tests/core/child-sessions.test.ts:104-131— thelocateSessionFilereplica is faithful in rungs/inputs (I diffed it against the realsessions/locate.ts; the isolation claim holds: dirname-only, no recursion, no parent walk) but it guarantees nothing about the real module long-term. Pin it (fixture/hash of the upstream file) or assert the structural property against the upstream source.src/view/tui.ts:383— thesession:line renders for running agents, i.e. precisely the file that must not be opened; label the ownership state (live — replay post-mortem/replayable).writeChildSessionLink— non-atomicwriteFileSync; a reader landing mid-write reads a truncated marker → unreadable → fail-closed → could destroy a legitimate session. Write temp +renameSync.MissingSessionCwdError.Claim audit highlights (verified TRUE)
locateSessionFileisolation (dir +sub-*id equality, two layers) · picker/listAllsees onlygetSessionsDir()subdirs (session-manager.js:1319-1336,config.js:457) · pi has no session locking · child bus carries the root anchor (agent-runner.ts:628stamps before session creation;index.ts:570pins root-only) · flag OFF =SessionManager.inMemory(cwd)with exact arity.FALSE: the "refuses to run" claim (above).
CI + base state
actions/taskslists runs for#22,#23,#24andmainpushes only — nothing for#25, ~10 min after it was opened, while main-push runs were dispatched in that window. Working hypothesis:pull_requestdispatch needs a merge ref, and this PR is unmergeable (mergeable: false).13ba30d;origin/mainnow has PR #24 merged atf883bb6.git merge-tree→ one conflict,pi-swarm-suite/README.md(this branch adds the Persisted child sessions table row; main added the Attach socket + CLI row). Everything else, incl.src/core/index.ts, auto-merges.Decision: not merged. Blocker stands; no push made from this review (the fix worker should own the single follow-up commit so the branch isn't moved twice).
Landing review round 2 — no blockers → merging
Independent fresh-context review (
pi -p --no-session --no-context-files --no-skills, reviewer ≠ implementer, foreground one-shot), brief built from primary sources only: deltafaf441f..79b590e, ticket acceptance criteria, invariants (flag-OFF identity / no pi-core change / single-writer), and the real installed implementations. No implementer summary used as the brief. CI at this head:pr-title✅pi-swarm-suite✅pi-otel-telemetry✅.Verdict: APPROVE-WITH-CONCERNS.
Round-1 blocker: CONFIRMED FIXED
The print/json no-op is closed. Re-derived from installed
dist/at 0.85.1:modes/print-mode.jsbinds{mode, commandContextActions, onError}— zero occurrences ofshutdown;modes/interactive/interactive-mode.js:1458+modes/rpc/rpc-mode.js:256are the only handlers. The stop is now unconditional and synchronous on the real path:agent-session.js:1926is the last statement ofbindExtensions,print-mode.js:103awaitsrebindSession()beforesession.promptat:105/108, andindex.ts:617stop: (code) => process.exit(code)runs inside the handler → no append on the prompt path. Same-pid scoping is correct (child-sessions.ts:545foreignHolder = link===undefined || link.pid!==pid; unreadable marker has no link → foreign → hard stop). Warning lands on fd 2 viawriteSyncbefore the exit.--forkdoes not misfire (forkFromwrites a NEW file → no marker).Round-1 concerns: CONFIRMED FIXED
/proc/self/stat(index 1 == PPid → field 4; index 19 == field 22): correct,lastIndexOf(")")is the right split. Direction errs toward refuse ("unknown"only from a failed read, never a mismatch; thenkill(0)). Tokens are written for the marker's own pid (child-sessions.ts:227), and the sole production call site passesprocess.pid— holder ≡ writer.releaseChildSessionis reached from all four terminal paths (agent-manager.ts:241terminal choke point,:276closeTranscript,:1068removeRecord,:1204dispose); never-created case drops marker + record handle + task-store link;TaskStore.clearSessionLinkidempotent.locate.tsrungs never walk up/sideways/recursive (rung 1existsSync(ref.file), rungs 2–3 readdir/header-scan ofdirname(ref.file)only),sub-<agentId>ids can't satisfy an id-equality rung;getSessionsDir()=<agentDir>/sessions(config.js:457) vs child dir<agentDir>/subagent-sessions/…— siblings, picker invisible. Tree anchor at all spawn sites; children never re-pin and never even emitsession_start(sdk.js:253constructsAgentSessionwithoutbindExtensions). Flag OFF =SessionManager.inMemory(cwd), byte-identical, no fs writes; default OFF atsettings.ts:352andsuite-settings.ts:61. Merge integrity: no conflict markers,f883bb6..faf441ftouches exactly the 16 feature files, attach-socket content intact on both sides.Concerns posted (non-blocking) — follow-up work recommended
pi --name+ live child appends one entry before the guard can fire.dist/main.ts:696sessionManager.appendSessionInfo(name)runs right aftercreateSessionManager, before runtime creation → beforesession_start. For a resumed child file_persisttakes theappendFileSyncbranch → exactly onesession_infoline lands in the live tree, then the guard exits. Bounded (metadata-only, requires the unrelated--name), unfixable from the suite without a pi-core change (I2) → the actionable part is a docs correction:child-sessions.ts:567"no append will be made" and the README "unconditional" wording should name this exception.allowStop: !process.env.VITEST(index.ts:619) is an env-var bypass of the only real guarantee. WithVITESTset in a pi process's environment, print-mode resume returns to the exact round-1 shape. Test-survivability trade-off, but it should be narrowed (injectedstophook / explicit test opt-in) or documented next to the mode caveat.child-sessions.ts:366-368):/proc/sys/kernel/random/boot_idis per-machine, so a marker read from another box on a shared agent dir judges a genuinely live holderdead, deletes the marker, and allows a second writer — and the rung short-circuits before the valid pid token. Low likelihood in a single-box lab, high blast radius. Fix: only treat a boot mismatch asdeadwhen a recorded machine identity (machine-id/hostname) also matches.dispose()nilsthis.taskStorebeforereleaseChildSession(agent-manager.ts:1201vs:1204), so the never-created-casesession.jsonrepair can be skipped whenrecord.taskStoreRefis unset. Reorder or pass the store explicitly.ctx.shutdown()(documented intent says "only warns"), andresolveChildSessionFiletrusts existence, not content/header id.Local verification run by the reviewer:
npx tsc --noEmitexit 0;npx vitest run tests/core/child-sessions.test.ts tests/core/child-session-wiring.test.ts→ 2 files / 45 tests passed. Merging now (plain merge, flag default OFF).