From 8061fb2e0af9f1df66b622b16bd83506c871377f Mon Sep 17 00:00:00 2001 From: roboomp Date: Sun, 9 Aug 2026 23:46:40 +0000 Subject: [PATCH 1/2] fix(agent): reject cold revive that finishes after lifecycle dispose A cold persisted-agent revival could complete after AgentLifecycleManager.dispose() and still attach its live session. When teardown lands during the reviver-factory await, the id is not yet tracked in #adopted, so dispose() skips it and leaves the parked registry ref intact; the late factory then cold-adopts and #revive attaches a session plus a TTL timer onto the torn-down manager, leaking a live session graph and timers past teardown. Set a #disposed flag at the top of dispose() and recheck it after the reviver-factory await and after the reviver await. A late revive now rejects deterministically and disposes any session it already built, preserving singleflight for concurrent callers. Fixes #8114 --- packages/coding-agent/CHANGELOG.md | 4 ++ .../src/registry/agent-lifecycle.ts | 19 +++++ .../test/registry/agent-lifecycle.test.ts | 72 +++++++++++++++++++ 3 files changed, 95 insertions(+) diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index a4d66e8d2..786744193 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### Fixed + +- Fixed a cold persisted-agent revival racing with lifecycle teardown: a revive whose reviver factory or session resolved after `AgentLifecycleManager.dispose()` could attach a live session and arm a TTL on the disposed manager, leaking a session graph and timers past teardown. Late revivals now reject deterministically and dispose any session they built ([#8114](https://github.com/can1357/oh-my-pi/issues/8114)). + ## [17.2.12] - 2026-08-08 ### Fixed diff --git a/packages/coding-agent/src/registry/agent-lifecycle.ts b/packages/coding-agent/src/registry/agent-lifecycle.ts index ae8dd13e6..95556924b 100644 --- a/packages/coding-agent/src/registry/agent-lifecycle.ts +++ b/packages/coding-agent/src/registry/agent-lifecycle.ts @@ -127,6 +127,8 @@ export class AgentLifecycleManager { #persistedReviverFactory: PersistedSubagentReviverFactory | undefined; /** TTL applied when a cold-revived ref is adopted on demand. */ #persistedReviveTtlMs = 0; + /** Set once {@link dispose} runs; blocks late revivals from adopting into a torn-down manager. */ + #disposed = false; constructor(registry: AgentRegistry = AgentRegistry.global()) { this.#registry = registry; @@ -332,6 +334,14 @@ export class AgentLifecycleManager { let coldAdopted = false; if (!revive && ref.status === "parked" && ref.sessionFile && this.#persistedReviverFactory) { revive = await this.#persistedReviverFactory(ref); + // Teardown can complete during the factory await. A late cold revive must + // not cold-adopt (and later attach a live session + TTL) into a disposed + // manager — reject deterministically before creating any session. + if (this.#disposed) { + throw new Error( + `Agent "${id}" revival aborted: its lifecycle was disposed while its persisted session was being prepared.`, + ); + } if (revive) { adoption = { ref, idleTtlMs: this.#persistedReviveTtlMs, revive }; this.#adopted.set(id, adoption); @@ -414,6 +424,7 @@ export class AgentLifecycleManager { /** Teardown everything (process exit / main session dispose). */ async dispose(deadlineAt: number = Date.now() + AGENT_RELEASE_GRACE_MS): Promise { this.#unsubscribe?.(); + this.#disposed = true; this.#unsubscribe = undefined; const ids = [...new Set([...this.#adopted.keys(), ...this.#parks.keys()])]; await Promise.all( @@ -439,6 +450,14 @@ export class AgentLifecycleManager { async #revive(id: string, revive: AgentReviver, ref: AgentRef, adopted: AdoptedAgent): Promise { const session = await revive(ref); + if (this.#disposed) { + // The owning lifecycle tore down while the reviver was in flight; dispose + // the freshly built session instead of attaching it, and fail the waiter. + await session.dispose(); + throw new Error( + `Agent "${id}" revival aborted: its lifecycle was disposed while its persisted session was reviving.`, + ); + } let liveRef = this.#registry.get(id); if (liveRef === ref && ref.status === "parked" && !ref.session) { // A simple reviver returned a session without claiming the parked ref; diff --git a/packages/coding-agent/test/registry/agent-lifecycle.test.ts b/packages/coding-agent/test/registry/agent-lifecycle.test.ts index 9f61c6de4..e01c11c63 100644 --- a/packages/coding-agent/test/registry/agent-lifecycle.test.ts +++ b/packages/coding-agent/test/registry/agent-lifecycle.test.ts @@ -621,4 +621,76 @@ describe("AgentLifecycleManager", () => { await registerPersistedSubagents(restoredRegistry, rootSessionFile); expect(restoredRegistry.get(workerId)?.status).toBe("aborted"); }); + + it("a cold revive whose factory resolves after dispose rejects without adopting or arming a TTL", async () => { + vi.useFakeTimers(); + const gate = deferred(); + const revived = makeSessionStub(); + let reviverRuns = 0; + // Restored-from-disk parked ref: never adopted, so dispose() does not track it. + registry.register({ + id: "Cold-DisposeRace", + displayName: "task", + kind: "sub", + session: null, + sessionFile: "/tmp/Cold-DisposeRace.jsonl", + status: "parked", + }); + lifecycle.setPersistedSubagentReviverFactory(async () => { + await gate.promise; + return async () => { + reviverRuns++; + return revived.session; + }; + }, TTL); + + const revival = lifecycle.ensureLive("Cold-DisposeRace"); + await flushAsync(); // reach the factory await + await lifecycle.dispose(Date.now()); // teardown while the factory is in flight + gate.resolve(); // factory completes for a superseded owner + + await expect(revival).rejects.toThrow(/disposed/); + // Rejected before the reviver ran: no session was ever created. + expect(reviverRuns).toBe(0); + expect(revived.disposeCalls()).toBe(0); + // No adoption, no live session, no armed TTL that could fire a late park. + expect(lifecycle.has("Cold-DisposeRace")).toBe(false); + expect(registry.get("Cold-DisposeRace")?.session ?? null).toBeNull(); + expect(registry.get("Cold-DisposeRace")?.status).not.toBe("idle"); + vi.advanceTimersByTime(TTL * 10); + await flushAsync(); + expect(revived.disposeCalls()).toBe(0); + }); + + it("a cold revive whose session resolves after dispose disposes that session and rejects", async () => { + const gate = deferred(); + const revived = makeSessionStub(); + registry.register({ + id: "Cold-SessionRace", + displayName: "task", + kind: "sub", + session: null, + sessionFile: "/tmp/Cold-SessionRace.jsonl", + status: "parked", + }); + // Factory resolves immediately (cold-adopts), but the reviver — which builds + // the live session — is held open across dispose(). + lifecycle.setPersistedSubagentReviverFactory( + async () => async () => { + await gate.promise; + return revived.session; + }, + TTL, + ); + + const revival = lifecycle.ensureLive("Cold-SessionRace"); + await flushAsync(); // reach the reviver await + await lifecycle.dispose(Date.now()); // teardown while the reviver is in flight + gate.resolve(); // reviver hands back a live session for a disposed owner + + await expect(revival).rejects.toThrow(/disposed/); + expect(revived.disposeCalls()).toBe(1); + expect(lifecycle.has("Cold-SessionRace")).toBe(false); + expect(registry.get("Cold-SessionRace")?.session ?? null).toBeNull(); + }); }); From dc47d73e9c28d545e44cedc34563a9b11c508ec4 Mon Sep 17 00:00:00 2001 From: roboomp Date: Sun, 9 Aug 2026 23:51:52 +0000 Subject: [PATCH 2/2] fix(agent): recreated global lifecycle after disposal Clear the disposed singleton after teardown so a later top-level session receives a fresh manager with an active registry subscription. Keep the retired manager marked disposed so its in-flight revivals still reject and clean up late sessions. Add regression coverage for sequential top-level lifecycle owners cold-reviving in the same process. Fixes #8114 --- .../src/registry/agent-lifecycle.ts | 3 ++- .../test/registry/agent-lifecycle.test.ts | 20 +++++++++++++++++++ 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/packages/coding-agent/src/registry/agent-lifecycle.ts b/packages/coding-agent/src/registry/agent-lifecycle.ts index 95556924b..cdc0116a5 100644 --- a/packages/coding-agent/src/registry/agent-lifecycle.ts +++ b/packages/coding-agent/src/registry/agent-lifecycle.ts @@ -421,7 +421,7 @@ export class AgentLifecycleManager { return true; } - /** Teardown everything (process exit / main session dispose). */ + /** Teardown everything; disposing the global manager makes its next owner a fresh instance. */ async dispose(deadlineAt: number = Date.now() + AGENT_RELEASE_GRACE_MS): Promise { this.#unsubscribe?.(); this.#disposed = true; @@ -446,6 +446,7 @@ export class AgentLifecycleManager { this.#revivals.clear(); this.#parks.clear(); this.#persistedReviverFactory = undefined; + if (AgentLifecycleManager.#global === this) AgentLifecycleManager.#global = undefined; } async #revive(id: string, revive: AgentReviver, ref: AgentRef, adopted: AdoptedAgent): Promise { diff --git a/packages/coding-agent/test/registry/agent-lifecycle.test.ts b/packages/coding-agent/test/registry/agent-lifecycle.test.ts index e01c11c63..611e79de4 100644 --- a/packages/coding-agent/test/registry/agent-lifecycle.test.ts +++ b/packages/coding-agent/test/registry/agent-lifecycle.test.ts @@ -693,4 +693,24 @@ describe("AgentLifecycleManager", () => { expect(lifecycle.has("Cold-SessionRace")).toBe(false); expect(registry.get("Cold-SessionRace")?.session ?? null).toBeNull(); }); + + it("a new top-level owner can cold-revive after the previous global lifecycle was disposed", async () => { + await lifecycle.dispose(Date.now()); + const revived = makeSessionStub(); + registry.register({ + id: "Next-Owner", + displayName: "task", + kind: "sub", + session: null, + sessionFile: "/tmp/Next-Owner.jsonl", + status: "parked", + }); + + const nextLifecycle = AgentLifecycleManager.global(); + nextLifecycle.setPersistedSubagentReviverFactory(async () => async () => revived.session, 0); + + await expect(nextLifecycle.ensureLive("Next-Owner")).resolves.toBe(revived.session); + expect(registry.get("Next-Owner")).toMatchObject({ status: "idle", session: revived.session }); + expect(revived.disposeCalls()).toBe(0); + }); });