fix: fail ao send when killed session delivery is not confirmed (#1236)

* fix: fail send when killed session delivery is not confirmed

* fix(core): drop requireConfirmation throw that re-introduced duplicate-message bug

The PR's sendWithConfirmation added a throw when confirmation heuristics
did not flip within SEND_CONFIRMATION_ATTEMPTS on a restored session.
But runtimePlugin.sendMessage had already fired, so the throw bubbled up
to the lifecycle manager's catch-all, leaving lastCIFailureDispatchHash
(and its merge-conflict twin) unset. Next poll re-dispatched the same
message — exactly the duplicate-message bug that commit 77685a5 removed.

Real fix for #1074 is preserved: restoreForDelivery still throws when
waitForRestoredSession returns false, which happens before sendMessage
fires. Killed sessions that cannot be revived are still reported as
failures, with no duplicate-send risk.

- sendWithConfirmation no longer takes requireConfirmation; unconfirmed
  delivery always returns (soft success).
- prepareSession returns Session (the tuple only existed to drive the
  removed throw).
- send's retry predicate reverts to prepared.restoredAt === undefined
  && isRestorable(prepared).
- Adds regression test: restored session + sendMessage fires +
  confirmation never flips → send() resolves.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
This commit is contained in:
Dhruv Sharma 2026-04-20 13:56:31 +05:30 committed by GitHub
parent f330a1ea69
commit f0d4faf2a4
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
4 changed files with 123 additions and 8 deletions

View File

@ -401,6 +401,39 @@ describe("send command", () => {
expect(mockTmux).not.toHaveBeenCalledWith("has-session", "-t", expect.any(String));
});
it("fails loudly when lifecycle delivery fails for an AO session", async () => {
mockConfigRef.current = makeConfig();
mockSessionManager.get.mockResolvedValue({
id: "app-1",
projectId: "my-app",
status: "killed",
activity: "exited",
branch: null,
issueId: null,
pr: null,
workspacePath: null,
runtimeHandle: { id: "tmux-target-1", runtimeName: "tmux", data: {} },
agentInfo: null,
createdAt: new Date(),
lastActivityAt: new Date(),
metadata: { agent: "opencode" },
});
mockSessionManager.send.mockRejectedValue(
new Error("Cannot send to session app-1: session is not running (restore timed out)"),
);
await expect(
program.parseAsync(["node", "test", "send", "app-1", "hello"]),
).rejects.toThrow("process.exit(1)");
expect(consoleErrorSpy).toHaveBeenCalledWith(
expect.stringContaining("Cannot send to session app-1: session is not running"),
);
expect(consoleSpy).not.toHaveBeenCalledWith(
expect.stringContaining("Message sent and processing"),
);
});
it("passes file contents through SessionManager.send for AO sessions", async () => {
mockConfigRef.current = makeConfig();
mockSessionManager.get.mockResolvedValue({

View File

@ -184,8 +184,13 @@ export function registerSend(program: Command): void {
}
if (existingSession && sessionManager) {
await sessionManager.send(session, message);
console.log(chalk.green("Message sent and processing"));
try {
await sessionManager.send(session, message);
console.log(chalk.green("Message sent and processing"));
} catch (err) {
console.error(chalk.red(err instanceof Error ? err.message : String(err)));
process.exit(1);
}
return;
}

View File

@ -86,6 +86,43 @@ describe("send", () => {
);
});
it("throws when a killed session cannot be restored to a ready state for delivery", async () => {
vi.useFakeTimers();
try {
const wsPath = join(tmpDir, "ws-app-1");
mkdirSync(wsPath, { recursive: true });
writeMetadata(sessionsDir, "app-1", {
worktree: wsPath,
branch: "feat/TEST-1",
status: "killed",
project: "my-app",
issue: "TEST-1",
runtimeHandle: JSON.stringify(makeHandle("rt-old")),
});
vi.mocked(mockRuntime.isAlive).mockImplementation(async (handle) => handle.id !== "rt-restored");
vi.mocked(mockAgent.isProcessRunning).mockImplementation(
async (handle) => handle.id !== "rt-restored",
);
vi.mocked(mockRuntime.create).mockResolvedValue(makeHandle("rt-restored"));
vi.mocked(mockRuntime.getOutput).mockResolvedValue("");
vi.mocked(mockAgent.detectActivity).mockReturnValue("idle");
const sm = createSessionManager({ config, registry: mockRegistry });
const sendPromise = sm.send("app-1", "hello");
const rejection = expect(sendPromise).rejects.toThrow(
"Cannot send to session app-1: session is not running",
);
await vi.runAllTimersAsync();
await rejection;
expect(mockRuntime.sendMessage).not.toHaveBeenCalled();
} finally {
vi.useRealTimers();
}
});
it("waits for spawning sessions to become interactive before considering restore", async () => {
writeMetadata(sessionsDir, "app-1", {
worktree: "/tmp",
@ -137,6 +174,42 @@ describe("send", () => {
expect(mockRuntime.sendMessage).toHaveBeenCalled();
});
it("resolves on restored session when confirmation never flips (soft success)", async () => {
// Regression test: on a restored session (post-#1074 fix), if
// sendMessage fires but the confirmation heuristics never flip,
// send() must still resolve — otherwise the lifecycle-manager's
// dispatch-hash never updates and the message re-sends next poll,
// reintroducing the duplicate-message bug that 77685a5 removed.
const wsPath = join(tmpDir, "ws-app-1");
mkdirSync(wsPath, { recursive: true });
writeMetadata(sessionsDir, "app-1", {
worktree: wsPath,
branch: "feat/TEST-1",
status: "working",
project: "my-app",
issue: "TEST-1",
runtimeHandle: JSON.stringify(makeHandle("rt-old")),
});
// rt-old is dead → restore kicks in → rt-restored is ready
vi.mocked(mockRuntime.isAlive).mockImplementation(async (handle) => handle.id !== "rt-old");
vi.mocked(mockAgent.isProcessRunning).mockImplementation(
async (handle) => handle.id !== "rt-old",
);
vi.mocked(mockRuntime.create).mockResolvedValue(makeHandle("rt-restored"));
// Steady output — confirmation heuristics will never flip
vi.mocked(mockRuntime.getOutput).mockResolvedValue("steady output");
vi.mocked(mockAgent.detectActivity).mockReturnValue("idle");
const sm = createSessionManager({ config, registry: mockRegistry });
await expect(sm.send("app-1", "retry after restore")).resolves.toBeUndefined();
expect(mockRuntime.sendMessage).toHaveBeenCalledWith(
makeHandle("rt-restored"),
"retry after restore",
);
});
it("throws for nonexistent session", async () => {
const sm = createSessionManager({ config, registry: mockRegistry });
await expect(sm.send("nope", "hello")).rejects.toThrow("not found");

View File

@ -2271,10 +2271,10 @@ export function createSessionManager(deps: SessionManagerDeps): OpenCodeSessionM
}
};
const waitForRestoredSession = async (restoredSession: Session): Promise<void> => {
const waitForRestoredSession = async (restoredSession: Session): Promise<boolean> => {
const handle = restoredSession.runtimeHandle;
if (!handle) {
return;
return false;
}
const deadline = Date.now() + SEND_RESTORE_READY_TIMEOUT_MS;
@ -2292,11 +2292,11 @@ export function createSessionManager(deps: SessionManagerDeps): OpenCodeSessionM
foregroundCommand === null || foregroundCommand === agentPlugin.processName;
if (runtimeAlive && foregroundReady && (processRunning || output.trim().length > 0)) {
return;
return true;
}
if (Date.now() >= deadline) {
return;
return false;
}
await sleep(SEND_RESTORE_READY_POLL_MS);
@ -2310,7 +2310,10 @@ export function createSessionManager(deps: SessionManagerDeps): OpenCodeSessionM
try {
const restored = await restore(sessionId);
await waitForRestoredSession(restored);
const ready = await waitForRestoredSession(restored);
if (!ready) {
throw new Error("restored session did not become ready for delivery");
}
return restored;
} catch (err) {
const detail = err instanceof Error ? err.message : String(err);
@ -2414,7 +2417,8 @@ export function createSessionManager(deps: SessionManagerDeps): OpenCodeSessionM
try {
await sendWithConfirmation(prepared);
} catch (err) {
const shouldRetryWithRestore = prepared.restoredAt === undefined && isRestorable(prepared);
const shouldRetryWithRestore =
prepared.restoredAt === undefined && isRestorable(prepared);
if (!shouldRetryWithRestore) {
if (err instanceof Error) {