diff --git a/CHANGELOG.md b/CHANGELOG.md index 53340922..f8765a4d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,10 @@ product overview. ## Unreleased +- Keep cancelled Slack turns from restoring the loading indicator while their engine shuts down. + Stop and cold-engine steering close progress immediately and ignore late output; cancellation + during preflight cannot start a new indicator. + - Fix open SSH sessions retaining stale channel secrets after additions, rotations or removals. Refreshes now recheck the developer’s access and close sessions when access is revoked or their mounted workspace/runtime changes. diff --git a/FEATURES.md b/FEATURES.md index 589bbd03..acdf62ab 100644 --- a/FEATURES.md +++ b/FEATURES.md @@ -1620,8 +1620,14 @@ A categorized catalog of what's shipped. Cross-linked to `TEST-PLAN.md` checks. in DMs and in channel threads alike. Pressing it stops exactly that thread through the same `stopRunsInChannel` path and authorization gate as a `stop` message or 🛑 reaction (redelivered envelopes are deduplicated, the bot's own events ignored); whether or not a run was found the - session is moved off `processing`, because Slack never does that on its own. Per-thread titles - use `agents.sessions.rename` (200 chars). A workspace or app without the native surface + session is moved off `processing`, because Slack never does that on its own. + Stop also closes that turn's progress producer immediately when its abort signal fires. Late + engine output, warmup notices and status heartbeats cannot restart processing while the runner + unwinds; cold-engine steering uses the same cancellation boundary. A stop during directory or + thread-context preflight does not start a replacement indicator or runner. + Repeated cancellation cleanup calls share one bounded completion, keeping a steered successor + behind its predecessor's pending status clear for the existing cleanup grace. + Per-thread titles use `agents.sessions.rename` (200 chars). A workspace or app without the native surface (`feature_disabled`, `missing_scope`, `unknown_method`, `missing_agent_session_stopped_event_subscription`, …) falls back to the legacy methods for the rest of the process after one warning; a per-thread refusal (`not_authorized`) only skips that diff --git a/TEST-PLAN.md b/TEST-PLAN.md index d0812948..edda8f6e 100644 --- a/TEST-PLAN.md +++ b/TEST-PLAN.md @@ -1,5 +1,27 @@ # ChannelGate — Test Plan +## Slack cancellation progress ordering (2026-10-16) + +- [x] Regression: `test/slack-stop-progress.test.js` holds actual pipeline runtime preparation + open after Stop/cold-engine Steer for Claude and Codex. Late warmup notices cannot reassert + `processing`; native status clears before unwind. Real stub children emit late answer deltas + after SIGTERM: neither processing nor the late answer reaches Slack. Directory-preflight Stop + creates no later indicator or runner. Existing queue, thread-context preflight and Stop-card + suites retain their durable cancellation, successor ownership and single-notice checks. + A held native status write plus a fast-exiting cold engine keeps the actual steered successor + queued until the predecessor's shared bounded cleanup completes; its late clear cannot erase + the successor's processing status within that existing grace window. +- [ ] Live **Claude and Codex**, separately, in owned DM and channel fixtures: select each engine + through the native picker, ask “Run sleep 120, then answer STOP_NOT_HONORED”, and press the real + native Stop before completion. Record exact root, loaded revision, resolved engine/model, + click time, native control/loading disappearance interval and process-tree termination; wait + beyond the original sleep deadline to exclude a late answer. Rename the thread and retain its + native rename event. Capture only safe session/thread/status fields from browser WebSocket + events to distinguish status receipt from DOM repaint. **Pass:** one Stopped notice, no surviving + owned process or late answer, no post-clear processing reassertion, and loading clears within + the existing one-second gate. An interval crossing one second is inconclusive; passing source + tests does not establish Slack client timing. Restore test pins and fixture settings. + ## Slack Settings picker rendering (2026-10-15) - [x] Regression: the existing all-page/all-role/inline-form Block Kit validation includes a diff --git a/src/slack/message-pipeline.js b/src/slack/message-pipeline.js index 2da8cecd..a160f6b7 100644 --- a/src/slack/message-pipeline.js +++ b/src/slack/message-pipeline.js @@ -87,7 +87,8 @@ async function slackPermalink(client, channel, ts) { return `https://slack.com/archives/${channel}/p${String(ts).replace(".", "")}`; } -// Terminalize every matching in-flight run synchronously — no Slack calls, no awaits. The shared +// Terminalize every matching in-flight run synchronously — no awaits, including progress cleanup +// started by abort listeners. The shared // core of the `stop` command (stopRunsInChannel below, which adds the user-facing messaging) and // `/clear` (which must kill the thread's live run through this exact path BEFORE dropping its // session: the abort marks handle.aborted, which suppresses the late answer post, the @@ -1542,6 +1543,7 @@ export async function processMessageEvent(event, client, { botUserId = "", teamI // From here EVERYTHING runs inside the try so the finally always releases this turn's queue // slot — a throw anywhere (even starting the progress indicator) must not jam the thread. let status = null; + let stopProgressOnAbort = null; let terminal = false; const markTerminal = () => { terminal = true; @@ -1551,6 +1553,12 @@ export async function processMessageEvent(event, client, { botUserId = "", teamI // spent the moment the run resolves, on every branch below, and exactly once. const bankUsage = createUsageBank(); try { + // Directory lookup above may have yielded after queue ownership. A stopped/steered owner + // must not create a new processing session while its successor is waiting for this slot. + if (handle.controller.signal.aborted || isForceStopping()) { + if (handle.controller.signal.aborted) markTerminal(); + return; + } // Progress indicator per the configured mode (shimmer / activity log / both / native stream). status = startProgress(getProgressView(), client, event.channel, threadKey, { isDM: meta.isDM, @@ -1558,6 +1566,12 @@ export async function processMessageEvent(event, client, { botUserId = "", teamI teamId, dir, }); + // Close the producer at cancellation, not after the runner finally settles. Its stop() + // seals callbacks/timers synchronously before awaiting Slack cleanup, so late engine output + // or runtime notices cannot reassert processing after the Stop handler clears the session. + stopProgressOnAbort = () => { void status.stop().catch(() => {}); }; + handle.controller.signal.addEventListener("abort", stopProgressOnAbort, { once: true }); + if (handle.controller.signal.aborted) stopProgressOnAbort(); // First time the bot is EVER pulled into an existing thread → replay its earlier messages // for context. Gated on the persistent has-ever-had-a-session marker (not "no current // session"), so /clear doesn't re-inject the whole history on the next reply; checked @@ -1587,8 +1601,8 @@ export async function processMessageEvent(event, client, { botUserId = "", teamI // preflight. Recheck at the last asynchronous boundary before enriching the durable row; // otherwise an explicit stop can clear the row and this owner can resurrect it immediately // afterwards. No await exists between this gate and recordActiveRun below. - if (handle.aborted || isForceStopping()) { - if (handle.aborted) markTerminal(); + if (handle.controller.signal.aborted || isForceStopping()) { + if (handle.controller.signal.aborted) markTerminal(); await status.stop(); return; } @@ -1643,12 +1657,16 @@ export async function processMessageEvent(event, client, { botUserId = "", teamI attachments: attachmentPaths, signal: handle.controller.signal, progressReport: true, - onDelta: status.onDelta, + onDelta: (delta) => { + if (!handle.controller.signal.aborted) return status.onDelta(delta); + }, onEvent: (ev) => { + if (handle.controller.signal.aborted) return; if (ev?.kind === "tool_use" && isMemorySaveTool(ev.name)) memorySavesInTurn += 1; return status.onEvent(ev); }, onRuntimeResolved: (runtime) => { + if (handle.controller.signal.aborted) return; updateActiveRunRuntime(runId, runtime); status.onRuntimeResolved?.(runtime); }, @@ -1853,6 +1871,7 @@ export async function processMessageEvent(event, client, { botUserId = "", teamI } } } finally { + if (stopProgressOnAbort) handle.controller.signal.removeEventListener("abort", stopProgressOnAbort); // Per-turn terminal state wins over the global shutdown phase: a delivered answer, reported // error, steering completion, or explicit stop must never replay merely because force-stop // flipped before this finally ran. Only genuinely interrupted work remains recoverable. diff --git a/src/slack/progress.js b/src/slack/progress.js index c383fe09..9702b058 100644 --- a/src/slack/progress.js +++ b/src/slack/progress.js @@ -697,6 +697,7 @@ function startStreamingProgress(client, { channel, threadTs, isDM, authorId, tea let failed = false; let stopped = false; let terminal = null; // "finalize" or "stop" once the run starts closing + let stopCompletion = null; // all cancellation callers share the original cleanup and grace // Streaming failures used to be swallowed whole, which made field problems (e.g. Slack's // undocumented limit on how long a message may stay in the streaming state) undiagnosable from // the logs. One warn per failure site per turn — never the payload, only the error code. @@ -1365,7 +1366,8 @@ function startStreamingProgress(client, { channel, threadTs, isDM, authorId, tea }, // Abort/error path: close the stream so the partial message finalizes cleanly (or clear the // shimmer if it never started). The caller posts its own error/stop notice. - stop: async () => { + stop: () => { + if (stopCompletion) return stopCompletion; const bounded = async (work) => { let timer; try { @@ -1373,8 +1375,7 @@ function startStreamingProgress(client, { channel, threadTs, isDM, authorId, tea } finally { clearTimeout(timer); } }; if (terminal) { - await bounded(chain); - return; + return bounded(chain); } terminal = "stop"; stopped = true; @@ -1398,7 +1399,12 @@ function startStreamingProgress(client, { channel, threadTs, isDM, authorId, tea } } })().catch((error) => reportStreamFailure("abort cleanup", error)); - await bounded(cleanup); + // An abort listener may start this before the runner's catch/finally calls stop again. + // Every caller must await the same clear/stream cleanup, not only the stream chain: an + // early queue release could let this turn's pending clear erase its successor's status. + // Sharing the bounded promise also keeps retries from starting a fresh grace period. + stopCompletion = bounded(cleanup); + return stopCompletion; }, }; } diff --git a/test/slack-stop-progress.test.js b/test/slack-stop-progress.test.js new file mode 100644 index 00000000..ed51b638 --- /dev/null +++ b/test/slack-stop-progress.test.js @@ -0,0 +1,237 @@ +import test from "node:test"; +import assert from "node:assert/strict"; +import { setTimeout as delay } from "node:timers/promises"; +import { ensureTestEnv } from "./helpers.js"; + +ensureTestEnv(); +process.env.SESSION_KEEPALIVE = "0"; +const { createFakeRuntimeBackend, useFakeRuntime } = await import("./runtime-fake.js"); +const { setUser } = await import("../src/config/store.js"); +const { saveSettings } = await import("../src/config/settings.js"); +const { processMessageEvent, runQueue, stopRunsInChannel } = await import("../src/slack/message-pipeline.js"); +const { steerActiveRun } = await import("../src/slack/busy-thread-choice.js"); + +function deferred() { + let resolve; + const promise = new Promise((done) => { resolve = done; }); + return { promise, resolve }; +} + +function fakeSlack(user) { + const calls = [], posted = [], streamed = []; + let seq = 0; + const ok = async () => ({ ok: true }); + return { + calls, posted, streamed, + apiCall: async (method, args) => { calls.push({ method, args }); return { ok: true }; }, + chat: { + postMessage: async (message) => { posted.push(message); return { ok: true, ts: `bot.${++seq}` }; }, + update: ok, delete: ok, postEphemeral: ok, + }, + chatStream: () => ({ ts: `stream.${++seq}`, + append: async (args) => { streamed.push(args); }, stop: ok }), + users: { + info: async () => ({ user: { id: user, real_name: "Stop fixture" } }), + list: async () => ({ members: [{ id: user, real_name: "Stop fixture" }], response_metadata: {} }), + }, + conversations: { + history: async () => ({ messages: [] }), replies: async () => ({ messages: [] }), + info: async ({ channel }) => ({ channel: { id: channel } }), + members: async () => ({ members: [user], response_metadata: {} }), + }, + }; +} + +test("Stop during directory preflight never starts progress or the runner afterwards", { timeout: 10_000 }, async () => { + const user = "U_PROGRESS_DIRECTORY", channel = "D_PROGRESS_DIRECTORY", thread = "9100.301"; + const slug = `dm-${user.toLowerCase()}`; + await setUser(user, { approved: true }); + saveSettings({ engine: "claude", memoryReviewEvery: 0, composioMode: "personal" }); + const backend = await useFakeRuntime(); + const client = fakeSlack(user), entered = deferred(), release = deferred(); + client.users.list = async () => { + entered.resolve(); + await release.promise; + return { members: [], response_metadata: {} }; + }; + const processing = processMessageEvent({ type: "message", channel, channel_type: "im", user, + text: "stop during directory lookup", ts: thread }, client, { botUserId: "U_BOT", teamId: "T_PROGRESS_DIRECTORY" }); + try { + await entered.promise; + assert.ok(runQueue.activeHandle(`${slug}::${thread}`)); + await stopRunsInChannel(client, channel, slug, user, thread); + const afterStop = client.calls.length; + release.resolve(); + await processing; + assert.equal(client.calls.slice(afterStop).some(({ args }) => args.status === "processing"), false); + assert.equal(backend.calls.spawn.length, 0); + assert.equal(runQueue.isActive(`${slug}::${thread}`), false); + } finally { + release.resolve(); + await processing; + await useFakeRuntime(null); + } +}); + +for (const engine of ["claude", "codex"]) { + test(`${engine} late stdout after Stop cannot restart status or stream an answer`, { timeout: 10_000 }, async () => { + const user = `U_LATE_PROGRESS_${engine}`, channel = `D_LATE_PROGRESS_${engine}`; + const slug = `dm-${user.toLowerCase()}`, thread = "9100.201"; + await setUser(user, { approved: true }); + saveSettings({ engine, memoryReviewEvery: 0, composioMode: "personal", engineFallback: false }); + const ready = deferred(), late = deferred(); + const backend = createFakeRuntimeBackend(), spawn = backend.spawn; + const fixture = ` + const engine = process.argv[1]; + const write = (text) => console.log(JSON.stringify(engine === "claude" + ? { type: "stream_event", event: { type: "content_block_delta", index: 0, delta: { type: "text_delta", text } } } + : { type: "item.completed", item: { id: text, type: "agent_message", text } })); + process.on("SIGTERM", () => { + setTimeout(() => write("LATE_ABORTED_ANSWER"), 100); + setTimeout(() => process.exit(0), 300); + }); + write("FIXTURE_READY"); + setInterval(() => {}, 1000); + `; + backend.spawn = (target, spec) => { + const child = spawn(target, { ...spec, cmd: process.execPath, args: ["-e", fixture, engine] }); + child.stdout.on("data", (bytes) => { + if (String(bytes).includes("FIXTURE_READY")) ready.resolve(); + if (String(bytes).includes("LATE_ABORTED_ANSWER")) late.resolve(); + }); + return child; + }; + await useFakeRuntime(backend); + const client = fakeSlack(user); + const processing = processMessageEvent({ type: "message", channel, channel_type: "im", user, + text: "run the delayed abort fixture", ts: thread }, client, { botUserId: "U_BOT", teamId: `T_LATE_${engine}` }); + try { + await ready.promise; + await stopRunsInChannel(client, channel, slug, user, thread); + const afterStop = client.calls.length; + await late.promise; + await processing; + assert.equal(client.calls.slice(afterStop).some(({ args }) => args.status === "processing"), false); + assert.doesNotMatch(JSON.stringify([...client.posted, ...client.streamed]), /LATE_ABORTED_ANSWER/); + assert.equal(client.posted.filter((m) => m.text === "🛑 Stopped.").length, 1); + } finally { + await stopRunsInChannel(client, channel, slug, user, thread); + await processing; + await useFakeRuntime(null); + } + }); +} + +for (const engine of ["claude", "codex"]) for (const action of ["stop", "steer"]) { + test(`${engine} ${action} closes progress before delayed runtime unwind`, { timeout: 10_000 }, async () => { + const user = `U_PROGRESS_${engine}_${action}`; + const channel = `D_PROGRESS_${engine}_${action}`; + const slug = `dm-${user.toLowerCase()}`; + const thread = "9100.101"; + await setUser(user, { approved: true }); + saveSettings({ engine, memoryReviewEvery: 0, composioMode: "personal", engineFallback: false }); + const entered = deferred(), release = deferred(); + const backend = createFakeRuntimeBackend(); + backend.ensureUp = async () => { + entered.resolve(); + await release.promise; + throw new Error("owned runtime fixture unwound after cancellation"); + }; + await useFakeRuntime(backend); + const client = fakeSlack(user); + const processing = processMessageEvent({ type: "message", channel, channel_type: "im", user, + text: "wait for the owned runtime fixture", ts: thread }, client, { botUserId: "U_BOT", teamId: `T_${engine}_${action}` }); + try { + await entered.promise; + const active = runQueue.activeHandle(`${slug}::${thread}`); + assert.ok(active, "runtime remains owned until unwind completes"); + if (action === "stop") await stopRunsInChannel(client, channel, slug, user, thread); + else assert.equal(steerActiveRun(runQueue, `${slug}::${thread}`, { requesterAuthorId: user, + interruptWarm: () => false }), "aborted"); + assert.equal(active.controller.signal.aborted, true); + // The runtime warmup notice fires after two seconds, while the cancelled runner is still + // held. This real late onEvent used to reassert processing after the Stop handler's clear. + const afterAbort = client.calls.length; + await delay(2_100); + assert.equal(client.calls.slice(afterAbort).some(({ args }) => args.status === "processing"), false, + "cancelled progress must not restart while the runner is still unwinding"); + assert.ok(client.calls.some(({ args }) => args.status === "active"), "cancellation clears native status before unwind"); + } finally { + release.resolve(); + await processing; + await useFakeRuntime(null); + } + assert.equal(client.posted.filter((m) => m.text === "🛑 Stopped.").length, action === "stop" ? 1 : 0); + assert.equal(client.posted.some((m) => /fixture unwound|wait for the owned runtime/.test(m.text || "")), false, + "neither the late failure nor an answer is delivered after cancellation"); + }); +} + +for (const engine of ["claude", "codex"]) { + test(`${engine} steered successor waits for the predecessor's deferred status cleanup`, { timeout: 10_000 }, async () => { + const user = `U_STEER_CLEANUP_${engine}`, channel = `D_STEER_CLEANUP_${engine}`; + const slug = `dm-${user.toLowerCase()}`, thread = "9100.401"; + await setUser(user, { approved: true }); + saveSettings({ engine, memoryReviewEvery: 0, composioMode: "personal", engineFallback: false }); + const ready = deferred(), exited = deferred(), oldWrite = deferred(), successorReady = deferred(); + const backend = createFakeRuntimeBackend(), spawn = backend.spawn; + let spawns = 0; + const fixture = ` + process.on("SIGTERM", () => process.exit(0)); + console.log(JSON.stringify(process.argv[1] === "claude" + ? { type: "stream_event", event: { type: "content_block_delta", index: 0, delta: { type: "text_delta", text: "READY" } } } + : { type: "item.completed", item: { id: "ready", type: "agent_message", text: "READY" } })); + setInterval(() => {}, 1000); + `; + backend.spawn = (target, spec) => { + const predecessor = ++spawns === 1; + const child = spawn(target, { ...spec, cmd: process.execPath, args: ["-e", fixture, engine] }); + child.stdout.once("data", () => (predecessor ? ready : successorReady).resolve()); + if (predecessor) child.once("exit", () => exited.resolve()); + return child; + }; + await useFakeRuntime(backend); + const client = fakeSlack(user), accepted = []; + let held = false; + client.apiCall = async (method, args) => { + client.calls.push({ method, args }); + if (method === "agents.sessions.setStatus") { + if (!held && args.status === "processing") { held = true; await oldWrite.promise; } + accepted.push(args.status); + } + return { ok: true }; + }; + const options = { botUserId: "U_BOT", teamId: `T_STEER_CLEANUP_${engine}` }; + const first = processMessageEvent({ type: "message", channel, channel_type: "im", user, + text: "first cleanup fixture", ts: thread }, client, options); + let second; + try { + await ready.promise; + assert.equal(held, true); + second = processMessageEvent({ type: "message", channel, channel_type: "im", user, + text: "steered cleanup successor", thread_ts: thread, ts: "9100.402" }, client, { ...options, busyChoice: "steer" }); + await exited.promise; + // The child has already exited; only the old Slack write remains. This delay is shorter + // than the existing one-second stop grace, so it must not promote the queued successor. + const settledEarly = await Promise.race([first.then(() => true), delay(100).then(() => false)]); + assert.equal(settledEarly, false, "the second stop() must await the original cleanup, not just its stream chain"); + assert.equal(spawns, 1); + assert.equal(runQueue.count(`${slug}::${thread}`), 2); + oldWrite.resolve(); + await first; + await successorReady.promise; + assert.equal(spawns, 2); + assert.equal(accepted.at(-1), "processing", "the old clear lands before the successor starts processing"); + assert.ok(accepted.indexOf("active") < accepted.lastIndexOf("processing")); + } finally { + oldWrite.resolve(); + await first; + if (second) { + await successorReady.promise; + await stopRunsInChannel(client, channel, slug, user, thread); + await second; + } + await useFakeRuntime(null); + } + }); +}