From fde87d6d63b0614b4f1f6ef268a92f9d58ca4701 Mon Sep 17 00:00:00 2001 From: hack-cli-tests Date: Tue, 6 Oct 2026 10:54:11 -0400 Subject: [PATCH] fix(lifecycle): accept confirmed concurrent session cleanup --- docs/lifecycle.md | 4 + src/lib/project-lifecycle-sessions.ts | 15 ++- tests/project-lifecycle-sessions.test.ts | 129 +++++++++++++++++++++++ 3 files changed, 146 insertions(+), 2 deletions(-) diff --git a/docs/lifecycle.md b/docs/lifecycle.md index aa348e50f..e8b9a3bbf 100644 --- a/docs/lifecycle.md +++ b/docs/lifecycle.md @@ -216,6 +216,10 @@ signals received while the mux session is still being initialized. If `lifecycle.down.before` fails, shutdown is aborted before Compose or lifecycle processes are stopped. `hack restart` preserves the same guard semantics during its down phase. +If a concurrent foreground finalizer removes the same owned lifecycle session during shutdown, +Hack accepts a fresh, explicit absence check. Missing ownership metadata by itself, a changed token, +or an unavailable session check still refuses cleanup. + ### `hack restart` `hack restart` runs the down lifecycle hooks and stops owned host processes, but preserves the diff --git a/src/lib/project-lifecycle-sessions.ts b/src/lib/project-lifecycle-sessions.ts index 27fa8aa44..ec2d84322 100644 --- a/src/lib/project-lifecycle-sessions.ts +++ b/src/lib/project-lifecycle-sessions.ts @@ -191,15 +191,26 @@ export async function inspectLifecycleSession(opts: { opts.backend.listSessionWindowNames?.({ name: opts.expectedSessionName }) ?? Promise.resolve(null), ]); + // A concurrent owned finalizer can remove the session after listSessions. + // Missing metadata is not absence: require a fresh exact-session query, and + // never excuse a changed token or state belonging to another session/backend. + const removedOwnedSession = + opts.entry?.backend === opts.backend.name && + opts.entry.sessionName === opts.expectedSessionName && + Boolean(opts.entry.ownershipToken) && + observedOwnershipToken === null && + (await opts.backend.readSessionPresence?.({ + name: opts.expectedSessionName, + })) === "absent"; return classifyLifecycleSession({ - session, + session: removedOwnedSession ? null : session, entry: opts.entry, observedOwnershipToken, expectedBackend: opts.backend.name, expectedSessionName: opts.expectedSessionName, expectedProjectRoot: await normalizePath(opts.expectedProjectRoot), expectedDefinitionHash: opts.expectedDefinitionHash, - liveWindowNames, + liveWindowNames: removedOwnedSession ? null : liveWindowNames, }); } diff --git a/tests/project-lifecycle-sessions.test.ts b/tests/project-lifecycle-sessions.test.ts index 48bd54da7..30fb85f19 100644 --- a/tests/project-lifecycle-sessions.test.ts +++ b/tests/project-lifecycle-sessions.test.ts @@ -3,6 +3,8 @@ import { expect, test } from "bun:test"; import type { LifecycleStateEntry } from "../src/lib/lifecycle-runtime.ts"; import { classifyLifecycleSession, + inspectLifecycleSession, + killInspectedLifecycleSession, killLifecycleSessionWithOwnership, resolveLifecycleDefinitionHash, resolveLifecycleEnvironmentFingerprint, @@ -155,6 +157,133 @@ test("classifyLifecycleSession blocks same-name sessions without ownership proof expect(inspection.decision).toMatchObject({ kind: "block" }); }); +function inspectionBackend(overrides: Partial = {}): MuxBackend { + return { + name: "tmux", + available: true, + listSessions: async () => [session], + createSession: async () => ({ ok: true, session }), + killSession: async () => { + throw new Error("Inspection must not kill a session"); + }, + readLifecycleOwnerToken: async () => null, + listSessionWindowNames: async () => null, + execInSession: async () => ({ exitCode: 0, stdout: "", stderr: "" }), + sendInput: async () => ({ exitCode: 0, stdout: "", stderr: "" }), + ...overrides, + }; +} + +function inspectFixture(backend: MuxBackend) { + return inspectLifecycleSession({ + backend, + entry, + expectedSessionName: session.name, + expectedProjectRoot: "/tmp/event-agent", + expectedDefinitionHash: definitionHash, + }); +} + +test("inspection accepts exact-owned cleanup between session listing and owner read", async () => { + let live = true; + let kills = 0; + const presenceQueries: string[] = []; + const backend = inspectionBackend({ + listSessions: async () => { + const snapshot = live ? [session] : []; + // The foreground finalizer retires its session after down took a snapshot. + expect( + await killLifecycleSessionWithOwnership({ + backend, + sessionName: session.name, + ownershipToken, + }) + ).toBe(true); + return snapshot; + }, + readLifecycleOwnerToken: async () => (live ? ownershipToken : null), + readSessionPresence: async ({ name }) => { + presenceQueries.push(name); + return live ? "present" : "absent"; + }, + killSession: async () => { + kills += 1; + live = false; + return { exitCode: 0, stdout: "", stderr: "" }; + }, + }); + + const inspection = await inspectFixture(backend); + expect(inspection.classification).toBe("absent"); + expect(inspection.decision).toEqual({ kind: "create" }); + expect(inspection.session).toBeNull(); + expect(presenceQueries).toEqual([session.name]); + expect(await killInspectedLifecycleSession({ backend, inspection })).toBe( + false + ); + expect(kills).toBe(1); +}); + +test.each([ + "present", + "unknown", +] as const)("inspection refuses missing ownership when fresh session presence is %s", async (presence) => { + const backend = inspectionBackend({ + readSessionPresence: async () => presence, + }); + const inspection = await inspectFixture(backend); + expect(inspection.classification).toBe("foreign"); + expect(inspection.decision.kind).toBe("block"); +}); + +test("inspection refuses missing ownership without an explicit presence query", async () => { + expect((await inspectFixture(inspectionBackend())).decision.kind).toBe( + "block" + ); +}); + +test("inspection propagates a presence query failure", async () => { + const backend = inspectionBackend({ + readSessionPresence: async () => { + throw new Error("Presence query unavailable"); + }, + }); + await expect(inspectFixture(backend)).rejects.toThrow( + "Presence query unavailable" + ); +}); + +test("inspection never excuses a changed token with subsequent absence", async () => { + const backend = inspectionBackend({ + readLifecycleOwnerToken: async () => "foreign-token", + readSessionPresence: async () => { + throw new Error( + "A changed token must refuse without an absence fallback" + ); + }, + }); + const inspection = await inspectFixture(backend); + expect(inspection.classification).toBe("foreign"); + expect(inspection.decision.kind).toBe("block"); +}); + +test("inspection does not reinterpret unrelated persisted ownership", async () => { + const backend = inspectionBackend({ + readSessionPresence: async () => { + throw new Error("Mismatched state must not use an absence fallback"); + }, + }); + const inspection = await inspectLifecycleSession({ + backend, + entry: { ...entry, sessionName: "another-session" }, + expectedSessionName: session.name, + expectedProjectRoot: "/tmp/event-agent", + expectedDefinitionHash: definitionHash, + }); + expect(inspection.classification).toBe("foreign"); + expect(inspection.decision.kind).toBe("block"); +}); + test("killLifecycleSessionWithOwnership cleans up only an exact token match", async () => { const killed: string[] = []; let observedToken: string | null = ownershipToken;