From b9d8aa2a405065f1a16fb55647da7991a2a53e93 Mon Sep 17 00:00:00 2001 From: Hessabi Date: Sat, 24 Oct 2026 23:33:03 -0500 Subject: [PATCH] fix: detach polled cloud handoffs at session end instead of cancelling them --- README.md | 2 +- plugins/devin/scripts/devin-companion.mjs | 2 +- .../devin/scripts/session-lifecycle-hook.mjs | 88 +++++++- tests/session-end-hook.test.mjs | 201 ++++++++++++++++++ 4 files changed, 282 insertions(+), 11 deletions(-) create mode 100644 tests/session-end-hook.test.mjs diff --git a/README.md b/README.md index c4f3aad..b1514f2 100644 --- a/README.md +++ b/README.md @@ -129,7 +129,7 @@ In Codex, install the fusion skill (via `codex plugin add fusion@devin-plugin-cc ## Hooks - **SessionStart** — exports session metadata for job attribution. -- **SessionEnd** — kills local jobs still running from this session. Detached cloud handoffs are left alone and reconciled on the next `status`/`result` call. +- **SessionEnd**: kills local jobs still running from this session. Cloud handoffs are never marked cancelled by the hook: a `--wait`/`--background` handoff only loses its local poller and becomes `detached`, like a plain handoff, because the remote session keeps running. Both are reconciled on the next `status`/`result` call, and `/devin:cancel` still stops the remote session. - **Stop (opt-in)**: when enabled via `/devin:setup --enable-review-gate`, reviews the previous turn's diff and blocks session stop only on a `BLOCK:` verdict. Infrastructure failures, including an unavailable sandbox, fail open with a warning; the gate never reruns the review without the sandbox. ## State diff --git a/plugins/devin/scripts/devin-companion.mjs b/plugins/devin/scripts/devin-companion.mjs index 5fa0c4d..7c6be06 100755 --- a/plugins/devin/scripts/devin-companion.mjs +++ b/plugins/devin/scripts/devin-companion.mjs @@ -444,7 +444,7 @@ async function cmdHandoff(cwd, argv) { const { sessionId, url } = created; // --wait keeps a pid so cancel can kill the poller; detached has none so it - // outlives the session without being swept by cleanup. + // outlives the session. SessionEnd turns a polled handoff into a detached one. const base = { ...job, cloudSessionId: sessionId, diff --git a/plugins/devin/scripts/session-lifecycle-hook.mjs b/plugins/devin/scripts/session-lifecycle-hook.mjs index e7fc12c..5b33bc2 100644 --- a/plugins/devin/scripts/session-lifecycle-hook.mjs +++ b/plugins/devin/scripts/session-lifecycle-hook.mjs @@ -4,9 +4,16 @@ import fs from "node:fs"; import process from "node:process"; -import { isJobActive } from "./lib/jobs.mjs"; +import { appendLogLine, isJobActive, nowIso } from "./lib/jobs.mjs"; import { terminateProcessTree } from "./lib/process.mjs"; -import { loadState, resolveStateFile, saveState } from "./lib/state.mjs"; +import { + loadState, + readJobFile, + resolveJobFile, + resolveStateFile, + updateState, + writeJobFile +} from "./lib/state.mjs"; import { resolveWorkspaceRoot } from "./lib/workspace.mjs"; export const SESSION_ID_ENV = "DEVIN_COMPANION_SESSION_ID"; @@ -32,6 +39,39 @@ function appendEnvVar(name, value) { fs.appendFileSync(process.env.CLAUDE_ENV_FILE, `export ${name}=${shellEscape(value)}\n`, "utf8"); } +// A `handoff --wait/--background` job keeps a pid (the local poller) and a cloudSessionId. +// Killing the poller does not stop the remote session, so such a job is never "cancelled". +function isPolledCloudHandoff(job) { + return job.kind === "handoff" && Boolean(job.cloudSessionId); +} + +// Block (briefly) until the process is gone so a poller mid-write cannot land a +// stale state.json rename after the hook's update. Bounded: SessionEnd is time-limited. +function waitForExit(pid, timeoutMs = 1000) { + const sleeper = new Int32Array(new SharedArrayBuffer(4)); + const deadline = Date.now() + timeoutMs; + while (Date.now() < deadline) { + try { + process.kill(pid, 0); + } catch (error) { + return error?.code === "ESRCH"; + } + Atomics.wait(sleeper, 0, 0, 20); + } + return false; +} + +function detachHandoffJobFile(workspaceRoot, job, patch) { + const jobFile = resolveJobFile(workspaceRoot, job.id); + if (!fs.existsSync(jobFile)) { + return; + } + const stored = readJobFile(jobFile); + if (stored.status === "running") { + writeJobFile(workspaceRoot, job.id, { ...stored, ...patch }); + } +} + function cleanupSessionJobs(cwd, sessionId) { if (!cwd || !sessionId) { return; @@ -44,8 +84,9 @@ function cleanupSessionJobs(cwd, sessionId) { } const state = loadState(workspaceRoot); - // Only local, killable jobs: a pid-bearing active job. Detached cloud handoffs - // deliberately outlive the session and must not be marked cancelled. + // Only jobs with a local process to kill: a pid-bearing active job. Handoffs + // started with neither --wait nor --background have no pid and deliberately + // outlive the session, so they are never touched here. const killable = state.jobs.filter( (job) => job.sessionId === sessionId && isJobActive(job) && Number.isFinite(job.pid) ); @@ -59,15 +100,44 @@ function cleanupSessionJobs(cwd, sessionId) { } catch { // Ignore teardown failures during session shutdown. } + if (isPolledCloudHandoff(job)) { + waitForExit(job.pid); + } } const killedIds = new Set(killable.map((job) => job.id)); - saveState(workspaceRoot, { - ...state, - jobs: state.jobs.map((job) => - killedIds.has(job.id) ? { ...job, status: "cancelled", phase: "cancelled", pid: null, completedAt: new Date().toISOString() } : job - ) + const detachedIds = new Set(); + // updateState re-reads state.json after the kills, so nothing a poller wrote in the + // meantime is lost. No network here: the remote session is neither stopped nor probed. + updateState(workspaceRoot, (fresh) => { + const completedAt = nowIso(); + fresh.jobs = fresh.jobs.map((job) => { + if (!killedIds.has(job.id)) { + return job; + } + if (!isPolledCloudHandoff(job)) { + return { ...job, status: "cancelled", phase: "cancelled", pid: null, completedAt }; + } + // Skip a poller that finished (or a cancel that landed) before the kill took effect. + if (job.status !== "running") { + return job; + } + detachedIds.add(job.id); + return { ...job, status: "detached", phase: "poller stopped at session end", pid: null, updatedAt: completedAt }; + }); }); + + for (const job of killable) { + if (!detachedIds.has(job.id)) { + continue; + } + try { + detachHandoffJobFile(workspaceRoot, job, { status: "detached", phase: "poller stopped at session end", pid: null }); + appendLogLine(job.logFile, "Local poller stopped at session end; the cloud session keeps running. Run /devin:status to refresh it or /devin:cancel to stop it."); + } catch { + // The state.json entry is authoritative; a missing job file or log is not fatal. + } + } } function handleSessionStart(input) { diff --git a/tests/session-end-hook.test.mjs b/tests/session-end-hook.test.mjs new file mode 100644 index 0000000..aec0346 --- /dev/null +++ b/tests/session-end-hook.test.mjs @@ -0,0 +1,201 @@ +import assert from "node:assert/strict"; +import { spawn } from "node:child_process"; +import fs from "node:fs"; +import http from "node:http"; +import os from "node:os"; +import path from "node:path"; +import test from "node:test"; +import { fileURLToPath } from "node:url"; + +import { isJobActive } from "../plugins/devin/scripts/lib/jobs.mjs"; +import { listJobs, readJobFile, resolveJobFile, upsertJob, writeJobFile } from "../plugins/devin/scripts/lib/state.mjs"; + +const HOOK = fileURLToPath(new URL("../plugins/devin/scripts/session-lifecycle-hook.mjs", import.meta.url)); +const SESSION = "claude-session-example"; +const OTHER_SESSION = "claude-session-other-example"; + +// Hook and fake API run in this process's event loop, so the hook is spawned +// asynchronously: that also lets Node reap the stand-in poller the hook kills. +function runSessionEnd({ cwd, env, sessionId = SESSION }) { + return new Promise((resolve, reject) => { + const child = spawn(process.execPath, [HOOK, "SessionEnd"], { cwd, env: { ...process.env, ...env } }); + let stderr = ""; + child.stderr.on("data", (chunk) => (stderr += chunk)); + child.on("error", reject); + child.on("close", (status) => resolve({ status, stderr })); + child.stdin.end(JSON.stringify({ hook_event_name: "SessionEnd", session_id: sessionId, cwd })); + }); +} + +function spawnStandInProcess() { + const child = spawn(process.execPath, ["-e", "setInterval(() => {}, 1000)"], { detached: true, stdio: "ignore" }); + const exited = new Promise((resolve) => child.on("exit", (code, signal) => resolve({ code, signal }))); + return { child, exited }; +} + +async function createHarness() { + const requests = []; + const server = http.createServer((req, res) => { + requests.push(`${req.method} ${req.url}`); + res.writeHead(500); + res.end("{}"); + }); + await new Promise((resolve) => server.listen(0, "127.0.0.1", resolve)); + const dir = fs.mkdtempSync(path.join(os.tmpdir(), "devin-session-end-test-")); + const dataDir = fs.mkdtempSync(path.join(os.tmpdir(), "devin-session-end-data-")); + const env = { + CLAUDE_PLUGIN_DATA: dataDir, + DEVIN_API_KEY: "apk_example", + DEVIN_API_URL: `http://127.0.0.1:${server.address().port}`, + DEVIN_COMPANION_ALLOW_LOOPBACK_API: "1" + }; + process.env.CLAUDE_PLUGIN_DATA = dataDir; + const spawned = []; + const seed = (job) => { + const record = { workspaceRoot: dir, status: "running", sessionId: SESSION, startedAt: new Date().toISOString(), ...job }; + writeJobFile(dir, record.id, record); + upsertJob(dir, record); + }; + const standIn = () => { + const entry = spawnStandInProcess(); + spawned.push(entry.child); + return entry; + }; + const stored = (id) => listJobs(dir).find((job) => job.id === id); + return { + dir, + env, + requests, + seed, + standIn, + stored, + storedFile: (id) => readJobFile(resolveJobFile(dir, id)), + run: (options = {}) => runSessionEnd({ cwd: dir, env, ...options }), + async cleanup() { + for (const child of spawned) { + try { + process.kill(-child.pid, "SIGKILL"); + } catch { + // already gone + } + } + await new Promise((resolve) => server.close(resolve)); + fs.rmSync(dir, { recursive: true, force: true }); + fs.rmSync(dataDir, { recursive: true, force: true }); + } + }; +} + +test("SessionEnd stops a --wait handoff poller and detaches the job instead of cancelling it", async () => { + const h = await createHarness(); + try { + const poller = h.standIn(); + h.seed({ id: "devin-handoff-wait", kind: "handoff", phase: "running-cloud", pid: poller.child.pid, cloudSessionId: "devin-example1" }); + + const result = await h.run(); + assert.equal(result.status, 0, result.stderr); + + const exit = await poller.exited; + assert.equal(exit.signal, "SIGTERM"); + + for (const job of [h.stored("devin-handoff-wait"), h.storedFile("devin-handoff-wait")]) { + assert.equal(job.status, "detached"); + assert.equal(job.pid, null); + assert.equal(job.cloudSessionId, "devin-example1"); + assert.equal(job.completedAt, undefined); + assert.match(job.phase, /poller stopped at session end/); + assert.ok(isJobActive(job)); + } + assert.deepEqual(h.requests, []); + } finally { + await h.cleanup(); + } +}); + +test("SessionEnd leaves other sessions and non-polled jobs alone and still cancels local jobs", async () => { + const h = await createHarness(); + try { + const local = h.standIn(); + const foreign = h.standIn(); + h.seed({ id: "devin-task-local", kind: "task", phase: "running", pid: local.child.pid }); + h.seed({ + id: "devin-handoff-foreign", + kind: "handoff", + phase: "running-cloud", + pid: foreign.child.pid, + cloudSessionId: "devin-example2", + sessionId: OTHER_SESSION + }); + h.seed({ id: "devin-handoff-detached", kind: "handoff", status: "detached", phase: "detached", pid: null, cloudSessionId: "devin-example3" }); + + const result = await h.run(); + assert.equal(result.status, 0, result.stderr); + + assert.equal((await local.exited).signal, "SIGTERM"); + const task = h.stored("devin-task-local"); + assert.equal(task.status, "cancelled"); + assert.equal(task.phase, "cancelled"); + assert.equal(task.pid, null); + assert.ok(task.completedAt); + + const other = h.stored("devin-handoff-foreign"); + assert.equal(other.status, "running"); + assert.equal(other.pid, foreign.child.pid); + process.kill(foreign.child.pid, 0); + + const detached = h.stored("devin-handoff-detached"); + assert.equal(detached.status, "detached"); + assert.equal(detached.phase, "detached"); + assert.deepEqual(h.requests, []); + } finally { + await h.cleanup(); + } +}); + +test("SessionEnd still cancels a handoff that has no cloud session yet", async () => { + const h = await createHarness(); + try { + const creating = h.standIn(); + h.seed({ id: "devin-handoff-creating", kind: "handoff", phase: "creating", pid: creating.child.pid }); + + const result = await h.run(); + assert.equal(result.status, 0, result.stderr); + assert.equal((await creating.exited).signal, "SIGTERM"); + assert.equal(h.stored("devin-handoff-creating").status, "cancelled"); + assert.deepEqual(h.requests, []); + } finally { + await h.cleanup(); + } +}); + +test("SessionEnd waits for the poller to exit and keeps a terminal state it wrote while dying", async () => { + const h = await createHarness(); + try { + // Stand-in poller that records "completed" on SIGTERM, then exits: the hook must not + // overwrite that with a stale snapshot taken before the kill. + const stateModule = fileURLToPath(new URL("../plugins/devin/scripts/lib/state.mjs", import.meta.url)); + const script = ` + import(${JSON.stringify(stateModule)}).then(({ upsertJob }) => { + process.on("SIGTERM", () => { + upsertJob(${JSON.stringify(h.dir)}, { id: "devin-handoff-done", status: "completed", phase: "exit", pid: null, completedAt: new Date().toISOString() }); + process.exit(0); + }); + console.log("ready"); + setInterval(() => {}, 1000); + });`; + const child = spawn(process.execPath, ["-e", script], { detached: true, stdio: ["ignore", "pipe", "ignore"] }); + const exited = new Promise((resolve) => child.on("exit", resolve)); + await new Promise((resolve) => child.stdout.once("data", resolve)); + h.seed({ id: "devin-handoff-done", kind: "handoff", phase: "running-cloud", pid: child.pid, cloudSessionId: "devin-example4" }); + + const result = await h.run(); + assert.equal(result.status, 0, result.stderr); + await exited; + const job = h.stored("devin-handoff-done"); + assert.equal(job.status, "completed"); + assert.equal(job.phase, "exit"); + assert.deepEqual(h.requests, []); + } finally { + await h.cleanup(); + } +});