From 0e1846e857b42dc92ea4d6d1dc2f9e121c3b7e31 Mon Sep 17 00:00:00 2001 From: Yash Date: Mon, 14 Sep 2026 22:19:54 +0000 Subject: [PATCH] fix(harness): order Terminal binding writes through End --- .changeset/terminal-binding-write-order.md | 5 ++ packages/harness/docs/assistant-history.md | 5 ++ .../src/core/session-manager-close.test.ts | 13 +++- .../session-manager-retirement-review.test.ts | 78 +++++++++++++++++-- packages/harness/src/core/session-manager.ts | 21 ++++- packages/harness/src/server/rest.test.ts | 11 ++- packages/harness/src/server/rest.ts | 1 + 7 files changed, 122 insertions(+), 12 deletions(-) create mode 100644 .changeset/terminal-binding-write-order.md diff --git a/.changeset/terminal-binding-write-order.md b/.changeset/terminal-binding-write-order.md new file mode 100644 index 000000000..5915b1952 --- /dev/null +++ b/.changeset/terminal-binding-write-order.md @@ -0,0 +1,5 @@ +--- +"@sapiom/harness": patch +--- + +Order Terminal binding writes with End and surface failed restart repairs. diff --git a/packages/harness/docs/assistant-history.md b/packages/harness/docs/assistant-history.md index e60c4d750..f3644fb1c 100644 --- a/packages/harness/docs/assistant-history.md +++ b/packages/harness/docs/assistant-history.md @@ -128,3 +128,8 @@ Missing native history offers a separate action to review the retained record. Brief reduction preserves the newest useful Assistant state and the user's task context, with explicit omission markers. Sibling legacy associations that claim the same native scope with contradictory workspaces fail closed before creation. + +Terminal binding persistence serializes actual sidecar writes with End, taking +each snapshot when its write runs. A failed compensating repair is surfaced so +cleanup can be retried. Resume and Start Terminal during shutdown return a +conflict instead of an internal-server error. diff --git a/packages/harness/src/core/session-manager-close.test.ts b/packages/harness/src/core/session-manager-close.test.ts index 477b85128..ebca402fd 100644 --- a/packages/harness/src/core/session-manager-close.test.ts +++ b/packages/harness/src/core/session-manager-close.test.ts @@ -237,12 +237,17 @@ describe("Terminal close admission and evidence", () => { }, {}, ); - await held.entered; - await manager.close(id); - held.release(); - await expect(creating).rejects.toMatchObject({ + const cancelled = expect(creating).rejects.toMatchObject({ code: "SESSION_PREPARATION_CANCELLED", }); + await held.entered; + const closing = manager.close(id); + try { + expect(writes).toBe(1); + } finally { + held.release(); + await Promise.all([closing, cancelled]); + } expect(spawnPty).not.toHaveBeenCalled(); }); diff --git a/packages/harness/src/core/session-manager-retirement-review.test.ts b/packages/harness/src/core/session-manager-retirement-review.test.ts index 4518e9d49..570be5651 100644 --- a/packages/harness/src/core/session-manager-retirement-review.test.ts +++ b/packages/harness/src/core/session-manager-retirement-review.test.ts @@ -230,13 +230,24 @@ describe("retained Terminal cleanup and bound restart cancellation", () => { trusted, recorded, ); - await held.entered; - await manager.close(id); - expect(manager.getSubsessionBinding(id)).toBeNull(); - held.release(); - await expect(restarting).rejects.toMatchObject({ + const cancelled = expect(restarting).rejects.toMatchObject({ code: "SESSION_PREPARATION_CANCELLED", }); + await held.entered; + const closing = manager.close(id); + try { + if (stage === "binding-write") { + // The initial reservation and held restart are the only writes. + // End must wait for the older write before persisting its tombstone. + expect(writeBinding).toHaveBeenCalledTimes(2); + } else { + await closing; + expect(manager.getSubsessionBinding(id)).toBeNull(); + } + } finally { + held.release(); + await Promise.all([closing, cancelled]); + } expect(manager.getSubsessionBinding(id)).toBeNull(); expect( JSON.parse(await readFile(`${path}.subsession-bindings.json`, "utf8")), @@ -245,6 +256,63 @@ describe("retained Terminal cleanup and bound restart cancellation", () => { expect(adapter.launch).toHaveBeenCalledOnce(); expect(manager.get(id)?.status).toBe("exited"); if (stage === "vendor-probe") expect(recorded).not.toHaveBeenCalled(); + fixture(); + await manager.init(); + expect(manager.getSubsessionBinding(id)).toBeNull(); + }, + ); + + it.each([false, true])( + "surfaces failed marker repair after commit=%s and permits durable close retry", + async (committed) => { + const { request, path, writeBinding, spawnPty } = fixture(); + const id = "00000000-0000-4000-8000-000000000334"; + const marker = { + sessionId: id, + projectId: "project-1", + parentSessionId: "parent-1", + bindingId: "binding-1", + incarnation: 1, + spawnEpoch: 1, + }; + const trusted = { + agentMapIdentity: () => ({ + sessionId: id, + projectId: "project-1", + userId: "user-1", + }), + }; + await manager.createReserved(id, request, marker, trusted); + spawns[0]!.exit(); + await manager.flush(); + const repairFailure = Object.assign(new Error("marker repair failed"), { + code: "ENOSPC", + }); + writeBinding + .mockImplementationOnce(async (file, text) => { + if (committed) await writeFile(file, text); + throw new Error("marker write outcome uncertain"); + }) + .mockRejectedValueOnce(repairFailure); + await expect( + manager.restartFreshBound( + id, + marker, + { ...marker, incarnation: 2, spawnEpoch: 2 }, + trusted, + async () => false, + ), + ).rejects.toBe(repairFailure); + expect(spawnPty).toHaveBeenCalledOnce(); + // A failed write must not poison the queue or conceal the repair error. + // A later successful End must remove even a possibly committed marker. + await manager.close(id); + expect( + JSON.parse(await readFile(`${path}.subsession-bindings.json`, "utf8")), + ).toEqual({ version: 1, markers: {}, closedSessionIds: [] }); + fixture(); + await manager.init(); + expect(manager.getSubsessionBinding(id)).toBeNull(); }, ); }); diff --git a/packages/harness/src/core/session-manager.ts b/packages/harness/src/core/session-manager.ts index d6b845a8e..acbc4f9d3 100644 --- a/packages/harness/src/core/session-manager.ts +++ b/packages/harness/src/core/session-manager.ts @@ -1195,6 +1195,7 @@ export class SessionManager { private agentSessionIdentityQueue: Promise = Promise.resolve(); private agentSessionOwnerWriteSeq = 0; private subsessionBindingWriteSeq = 0; + private subsessionBindingWriteQueue: Promise = Promise.resolve(); private subsessionBindingQueue: Promise = Promise.resolve(); private initialized = false; @@ -3262,6 +3263,7 @@ export class SessionManager { } await this.agentSessionIdentityQueue; await this.subsessionBindingQueue; + await this.subsessionBindingWriteQueue; await this.writeQueue; } @@ -3836,7 +3838,19 @@ export class SessionManager { } } - private async persistSubsessionBindings(): Promise { + private persistSubsessionBindings(): Promise { + // End may overtake asynchronous restart preparation. Serialize the actual + // sidecar writes and snapshot at execution so an older write cannot land + // after End has durably removed the binding. Keep this separate from the + // operation queue: End must not wait on held launch preparation. + const next = this.subsessionBindingWriteQueue.then(() => + this.writeSubsessionBindings(), + ); + this.subsessionBindingWriteQueue = next.catch(() => {}); + return next; + } + + private async writeSubsessionBindings(): Promise { const markers = Object.fromEntries( [...this.subsessionBindings.entries()] .sort(([left], [right]) => left.localeCompare(right)) @@ -4358,7 +4372,10 @@ export class SessionManager { this.subsessionBindings.get(id) === next ) this.subsessionBindings.set(id, current); - await this.persistSubsessionBindings().catch(() => {}); + // Report cancellation only after its durable repair succeeds. A failed + // repair is actionable storage uncertainty, including a post-rename + // failure that may have committed the advanced marker. + await this.persistSubsessionBindings(); throw error; } } diff --git a/packages/harness/src/server/rest.test.ts b/packages/harness/src/server/rest.test.ts index 4a2f30860..5b22d9a0b 100644 --- a/packages/harness/src/server/rest.test.ts +++ b/packages/harness/src/server/rest.test.ts @@ -352,9 +352,10 @@ describe("createRestRouter", () => { }); it.each([ + new SessionManagerClosingError(), new SessionPreparationCancelledError(), new SessionCleanupUnconfirmedError(), - ])("returns 409 for cancelled or unconfirmed Terminal resume: $code", async (error) => { + ])("returns 409 for Terminal resume lifecycle conflicts: $code", async (error) => { const sessionManager = fakeSessionManager([exitedSession()]); vi.mocked(sessionManager.resume).mockRejectedValue(error); vi.mocked(sessionManager.restartForMcpCredentials).mockRejectedValue(error); @@ -390,6 +391,14 @@ describe("createRestRouter", () => { vi.mocked(sessionManager.activateDormant).mockRejectedValueOnce(error); expect((await request()).status).toBe(409); } + const shutdown = new SessionManagerClosingError(); + vi.mocked(sessionManager.activateDormant).mockRejectedValueOnce(shutdown); + const denied = await request(); + expect(denied.status).toBe(409); + expect(await denied.json()).toEqual({ + error: shutdown.message, + code: shutdown.code, + }); vi.mocked(sessionManager.activateDormant).mockRejectedValueOnce(new UnknownSessionError("sess-1")); expect((await request()).status).toBe(404); }); diff --git a/packages/harness/src/server/rest.ts b/packages/harness/src/server/rest.ts index b4502ac01..3b9cbea3c 100644 --- a/packages/harness/src/server/rest.ts +++ b/packages/harness/src/server/rest.ts @@ -732,6 +732,7 @@ export function createRestRouter(options: RestRouterOptions): Router { err instanceof McpSessionRestartUnavailableError || err instanceof ProjectSessionScopeUnavailableError || err instanceof SessionAlreadyLiveError || + err instanceof SessionManagerClosingError || err instanceof SessionPreparationCancelledError || err instanceof SessionCleanupUnconfirmedError || err instanceof SessionNotDormantError ||