From a9dadab88e688222ee2d903a4dab02a4afd0892e Mon Sep 17 00:00:00 2001 From: Cho Young-Hwi Date: Sat, 25 Jul 2026 00:34:04 +0000 Subject: [PATCH 1/2] [#1010] Bound Claude deferred-wake delivery under continuous TUI repaint MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Claude Code TUI repaints its status line ~5x/sec while idle, so the deferred-wake drain's idle timer — reset on every onData — never saw the 5s gap and a queued wake was starved indefinitely. Claude agents in file-chat mode were therefore never woken, while codex agents (whose TUI goes quiet between turns) worked normally. Adds a bounded deadline that output cannot postpone, gated to Claude via explicit session backend identity so codex/gemini keep their current genuine-quiescence-only behavior — a wall-clock deadline would otherwise barge into their legitimately long streaming turns. Safety contract: - The cap never preempts the #736 active-send suppression window: armed from the recentlySent path it re-arms once for the remainder. - Idle-gap delivery remains the preferred path; the cap is only the starvation escape hatch. - The onData listener and both timers are one per-key lifecycle, disposed on idle drain, cap drain, cleanup and session exit; a timer that outlives its cycle cannot consume or dispose a later one. - Delivery re-resolves the session by key and requires a running session with a live term, re-checking backend eligibility against the live session in case of a respawn onto another backend. - A per-key cooldown holds (never drops) further cap deliveries, so repeated mentions during one long turn cannot become a wake storm — which would trip the 30-hop loop guard and stop wake dispatch entirely. - The delayed submit ("\r") timer is now owned per key, so stopping a session inside its delay cannot write to a dead term. Tests drive the captured onData callbacks — the pre-existing stub never did, which is why repaint starvation shipped green. Cap timings are overridable so the regressions run at millisecond scale. Verified the core case fails with the cap disabled (11 assertions fail). Co-Authored-By: Claude Opus 5 --- server/index.js | 8 +- server/pty-dispatcher.js | 243 +++++++++++++++++++++++++++++----- server/pty-dispatcher.test.js | 228 ++++++++++++++++++++++++++++++- 3 files changed, 441 insertions(+), 38 deletions(-) diff --git a/server/index.js b/server/index.js index 25bd6ac..a6c7540 100644 --- a/server/index.js +++ b/server/index.js @@ -13,7 +13,7 @@ const { dispatchToAgentPTY, cleanupSession: cleanupPtyDispatcher } = require("./ const { runAcMigration } = require("./migrate-ac"); const selfHeal = require("./self-heal"); const tempCleanup = require("./temp-cleanup"); // #957: stale backend-temp sweep -const { injectModeForCommand } = require("../src/lib/injectMode.js"); +const { injectModeForCommand, cliBaseFromCommand } = require("../src/lib/injectMode.js"); const telegramBridge = require("./bridges/telegram"); // #972: stop on shutdown const discordBridge = require("./bridges/discord"); // #972: stop on shutdown @@ -654,6 +654,12 @@ async function spawnAgentPty(project, agent, opts = {}) { lastDims: null, state: "running", error: null, + // #1010: explicit backend identity for the session. The PTY dispatcher + // gates its bounded deferred-wake cap on this — Claude only, because only + // the Claude TUI repaints continuously while idle. Derived with the same + // helper the spawn/arg paths use, so "claude", "/usr/bin/claude" and + // "claude --foo" all resolve to "claude". + backend: cliBaseFromCommand(command), lastOutputAt: Date.now(), // #418: ring buffer of recent PTY output so reconnecting WS // clients see the terminal state instead of a blank panel. diff --git a/server/pty-dispatcher.js b/server/pty-dispatcher.js index 8776137..8a60427 100644 --- a/server/pty-dispatcher.js +++ b/server/pty-dispatcher.js @@ -8,6 +8,37 @@ const IDLE_THRESHOLD_MS = 5000; const COALESCE_WINDOW_MS = 1000; const ACTIVE_SUPPRESSION_MS = 30000; +// #1010: bounded deferred-wake deadline — a Claude-only escape hatch for the +// starvation below. The Claude Code TUI repaints its status line ~5x/sec even +// when idle, so the drain's idle timer (reset on every onData) never sees the +// IDLE_THRESHOLD_MS gap and a deferred wake is starved indefinitely. This +// deadline is NOT postponed by output. It is deliberately Claude-only: codex +// and gemini TUIs go quiet between turns, so their defers already drain on +// genuine quiescence, and a wall-clock deadline would instead barge into a +// legitimately long streaming turn (build/test output). Idle-gap delivery stays +// the preferred path for every backend; the cap only breaks the starvation. +const MAX_DEFER_MS = 10000; + +// #1010: after a CAP-driven injection we have no evidence the agent's turn +// ended — unlike an idle-gap drain, which proves quiescence. So a further cap +// delivery for that key is held for the same window we already treat an agent's +// own chat post as "probably still mid-turn" (#736). Held wakes are DEFERRED, +// never dropped, so repeated mentions during one long turn cannot become a wake +// storm — which matters because the loop guard pauses the whole project after 30 +// agent-to-agent messages and a paused guard stops wake dispatch entirely. +const CAP_COOLDOWN_MS = ACTIVE_SUPPRESSION_MS; + +// #1010: the timings above are read through this indirection so the dispatcher +// tests can run the repaint/cap/cooldown regressions at millisecond scale +// instead of real seconds. Production never calls the setter. +const DEFAULT_TIMINGS = { + idleThresholdMs: IDLE_THRESHOLD_MS, + maxDeferMs: MAX_DEFER_MS, + capCooldownMs: CAP_COOLDOWN_MS, + activeSuppressionMs: ACTIVE_SUPPRESSION_MS, +}; +let _timings = { ...DEFAULT_TIMINGS }; + // Per-agent coalescing timers: key = "project/agent" → timeout handle const _coalesceTimers = new Map(); @@ -17,9 +48,22 @@ const _lastChatSentAt = new Map(); // Per-agent pending wake flag: key = "project/agent" → true const _pendingWake = new Map(); -// Per-agent drain listeners: key = "project/agent" → { disposable, timeoutHandle } +// Per-agent deferred-wake cycle state: key = "project/agent" → +// { disposable, idleHandle, capHandle, capRearmed }. #1010: one object owns the +// whole cycle (listener + both timers) so every terminal path disposes all of +// it, and a timer that outlives its own cycle can be recognized as stale. const _drainListeners = new Map(); +// #1010: last CAP-driven injection per key = "project/agent" → epoch ms. +// Idle-gap deliveries are NOT recorded — they prove the turn ended, so they are +// not rate-limited. +const _lastCapInjectedAt = new Map(); + +// #1010: the delayed submit ("\r") timer per key = "project/agent" → handle. +// Previously unowned, so stopping a session inside the submit delay left a +// pending write against a dead term. +const _submitTimers = new Map(); + /** * Dispatch a chat message to mentioned agents' PTYs. * @@ -59,9 +103,9 @@ function dispatchToAgentPTY(projectId, msg, agentSessions, deps) { // new turn within seconds. The scheduled trigger is now only a periodic // backstop, not the primary wake mechanism for direct assignments. const lastSent = _lastChatSentAt.get(key); - const recentlySent = lastSent && (Date.now() - lastSent < ACTIVE_SUPPRESSION_MS); + const recentlySent = lastSent && (Date.now() - lastSent < _timings.activeSuppressionMs); if (isAgentBusy(session) || recentlySent) { - queuePendingWake(key, session, deps); + queuePendingWake(key, session, agentSessions, deps); continue; } @@ -70,60 +114,163 @@ function dispatchToAgentPTY(projectId, msg, agentSessions, deps) { } function isAgentBusy(session) { - return session.lastOutputAt && (Date.now() - session.lastOutputAt < IDLE_THRESHOLD_MS); + return session.lastOutputAt && (Date.now() - session.lastOutputAt < _timings.idleThresholdMs); +} + +/** + * #1010: may this session's deferred wake use the bounded cap? + * + * Claude only. `session.backend` is the CLI base name stamped on the session at + * spawn (server/index.js). A session without it — an older object, an error + * placeholder — is treated as NOT eligible, so the fail-safe direction is the + * pre-#1010 behavior (genuine-quiescence-only) rather than a barge-in. + */ +function capEligible(session) { + return !!session && session.backend === "claude"; } /** - * Queue a pending wake using high-water mark (since_id). - * Hooks term.onData to drain after idle period, with a fallback - * timer so the wake fires even if no further PTY output arrives. + * Queue a pending wake for a busy/recently-active agent (#923: defer, never + * drop). One cycle per key: `term.onData` resets an idle timer, so the wake + * drains once the agent has been quiet for the idle window. For Claude a second + * timer — the #1010 cap — bounds the wait, because a continuously repainting + * TUI means the idle gap never arrives. + * + * @param {string} key - "project/agent" + * @param {object} session - the session that armed the cycle (for backend + term) + * @param {Map} agentSessions - re-resolved at fire time; the armed session may be dead by then + * @param {object} deps - { isLoopGuardPaused, safeWrite } */ -function queuePendingWake(key, session, deps) { +function queuePendingWake(key, session, agentSessions, deps) { _pendingWake.set(key, true); - const drainFn = () => { + // A cycle is already armed for this key — the pending flag above is all a + // repeat mention needs. Do NOT arm a second listener or a second cap. + if (_drainListeners.has(key)) return; + + const state = { disposable: null, idleHandle: null, capHandle: null, capRearmed: false }; + + // #1010: only the cycle currently registered under this key may drain. A + // timer that outlived its own cycle (replaced, cleaned up, or already + // delivered) holds a stale `state`, so it can never consume or dispose a + // later cycle's pending wake. + const isCurrentCycle = () => _drainListeners.get(key) === state; + + // #1010: the session captured at arm time may have exited or been respawned + // before a timer fires. Re-resolve by key and require a live, running term — + // the same check the coalesce path already makes at fire time. + const resolveLive = () => { + const live = agentSessions.get(key); + return live && live.state === "running" && live.term ? live : null; + }; + + const deliver = (live) => { + _pendingWake.delete(key); + cleanupDrainListener(key); + injectIntoTerm(key, live.term, buildInjectionPrompt(live.agentId), deps); + }; + + // Shared preamble for both timers: returns the live session, or null if this + // cycle should simply stand down. + const claimCycle = () => { + if (!isCurrentCycle()) return null; if (!_pendingWake.get(key)) { cleanupDrainListener(key); - return; + return null; } - _pendingWake.delete(key); - cleanupDrainListener(key); + const live = resolveLive(); + if (!live) { + _pendingWake.delete(key); + cleanupDrainListener(key); + return null; + } + return live; + }; + + const idleDrain = () => { + const live = claimCycle(); + if (!live) return; - // #923: fire the deferred wake unconditionally once the agent has been - // quiet for the idle window. Do NOT re-suppress on a recent send here — a - // standing-by agent that just posted (then ended its turn) would otherwise - // be stranded again, which is the exact stall this fix removes. - injectIntoTerm(session.term, buildInjectionPrompt(session.agentId), deps); + // #923: fire unconditionally once the agent has been quiet for the idle + // window. Do NOT re-suppress on a recent send here — a standing-by agent + // that just posted (then ended its turn) would otherwise be stranded again, + // which is the exact stall this path removes. The quiet gap IS the evidence + // the turn ended, so this delivery is not cooldown-limited either. + deliver(live); }; - if (_drainListeners.has(key)) return; + const capDrain = () => { + const live = claimCycle(); + if (!live) return; + + // #1010: re-check eligibility against the LIVE session, not just the one + // that armed the cycle. If the agent was respawned onto another backend in + // the meantime, stand down and leave the wake to the idle-gap path — a cap + // must never inject into a codex/gemini turn. The cycle stays armed, so the + // pending wake is still delivered on genuine quiescence. + if (!capEligible(live)) return; + + // #736: the cap must not preempt the deliberate active-send suppression + // window. If the agent posted recently it is probably still mid-turn, so + // re-arm ONCE for whatever remains of that window instead of barging in. + const lastSent = _lastChatSentAt.get(key); + const remainingSuppression = lastSent + ? _timings.activeSuppressionMs - (Date.now() - lastSent) + : 0; + if (!state.capRearmed && remainingSuppression > 0) { + state.capRearmed = true; + state.capHandle = setTimeout(capDrain, remainingSuppression); + return; + } - const state = { timeoutHandle: null }; + // #1010: hold — not drop — a cap delivery inside the cooldown window left + // by the previous cap injection. The pending flag stays set, so the wake is + // delivered when the window closes (or earlier, if the agent goes genuinely + // quiet and the idle timer wins). + const lastCap = _lastCapInjectedAt.get(key); + const remainingCooldown = lastCap ? _timings.capCooldownMs - (Date.now() - lastCap) : 0; + if (remainingCooldown > 0) { + state.capHandle = setTimeout(capDrain, remainingCooldown); + return; + } + + _lastCapInjectedAt.set(key, Date.now()); + deliver(live); + }; function resetIdleTimer() { - if (state.timeoutHandle) clearTimeout(state.timeoutHandle); - state.timeoutHandle = setTimeout(drainFn, IDLE_THRESHOLD_MS); + if (state.idleHandle) clearTimeout(state.idleHandle); + state.idleHandle = setTimeout(idleDrain, _timings.idleThresholdMs); } - // Start fallback timer immediately so drain fires even without further output + // Start the idle timer immediately so the drain fires even if no further PTY + // output arrives at all. resetIdleTimer(); - const disposable = session.term.onData(() => { + // #1010: the cap deadline. Armed once per cycle, never reset by onData. + if (capEligible(session)) { + state.capHandle = setTimeout(capDrain, _timings.maxDeferMs); + } + + state.disposable = session.term.onData(() => { resetIdleTimer(); }); - _drainListeners.set(key, { disposable, state }); + _drainListeners.set(key, state); } +// #1010: one lifecycle for the whole deferred-wake cycle — the onData listener +// and BOTH timers. Leaving the cap timer armed is not a benign leak: the drain +// only checks the pending flag, so a survivor would consume a later cycle's +// wake and dispose its listener. function cleanupDrainListener(key) { - const listener = _drainListeners.get(key); - if (!listener) return; - if (listener.disposable && typeof listener.disposable.dispose === "function") { - listener.disposable.dispose(); - } - if (listener.state && listener.state.timeoutHandle) { - clearTimeout(listener.state.timeoutHandle); + const state = _drainListeners.get(key); + if (!state) return; + if (state.disposable && typeof state.disposable.dispose === "function") { + state.disposable.dispose(); } + if (state.idleHandle) clearTimeout(state.idleHandle); + if (state.capHandle) clearTimeout(state.capHandle); _drainListeners.delete(key); } @@ -159,26 +306,33 @@ function scheduleCoalescedInjection(key, projectId, agentId, msg, agentSessions, // goes quiet) rather than injecting mid-activity (#736). Crucially this // defers, it does NOT drop (#923) — the drain still delivers the wake. const lastSent = _lastChatSentAt.get(key); - const recentlySent = lastSent && (Date.now() - lastSent < ACTIVE_SUPPRESSION_MS); + const recentlySent = lastSent && (Date.now() - lastSent < _timings.activeSuppressionMs); if (isAgentBusy(session) || recentlySent) { - queuePendingWake(key, session, deps); + queuePendingWake(key, session, agentSessions, deps); return; } - injectIntoTerm(session.term, buildInjectionPrompt(agentId), deps); + injectIntoTerm(key, session.term, buildInjectionPrompt(agentId), deps); }, COALESCE_WINDOW_MS); state.timer = timer; _coalesceTimers.set(key, state); } -function injectIntoTerm(term, text, deps) { +// #1010: the delayed submit is owned per key so `cleanupSession` can cancel it. +// Without an owner, stopping a session inside the submit delay left a pending +// "\r" write against a term that was already killed. +function injectIntoTerm(key, term, text, deps) { const flat = text.replace(/\n/g, " "); deps.safeWrite(term, flat); const submitDelayMs = Math.max(300, flat.length); - setTimeout(() => { + const previous = _submitTimers.get(key); + if (previous) clearTimeout(previous); + const handle = setTimeout(() => { + _submitTimers.delete(key); try { deps.safeWrite(term, "\r"); } catch {} }, submitDelayMs); + _submitTimers.set(key, handle); } /** @@ -193,6 +347,14 @@ function cleanupSession(key) { _pendingWake.delete(key); _lastChatSentAt.delete(key); cleanupDrainListener(key); + // #1010: the delayed submit timer and the cap cooldown are part of this + // session's state — a stopped/respawned session must not inherit either. + const submit = _submitTimers.get(key); + if (submit) { + clearTimeout(submit); + _submitTimers.delete(key); + } + _lastCapInjectedAt.delete(key); } module.exports = { @@ -203,7 +365,16 @@ module.exports = { _pendingWake, _drainListeners, _lastChatSentAt, + _lastCapInjectedAt, + _submitTimers, IDLE_THRESHOLD_MS, COALESCE_WINDOW_MS, ACTIVE_SUPPRESSION_MS, + MAX_DEFER_MS, + CAP_COOLDOWN_MS, + // #1010: lets the dispatcher tests run the repaint/cap/cooldown regressions at + // millisecond scale. Call with no argument to restore the shipped timings. + _setTimingsForTest: (overrides) => { + _timings = { ...DEFAULT_TIMINGS, ...(overrides || {}) }; + }, }; diff --git a/server/pty-dispatcher.test.js b/server/pty-dispatcher.test.js index 32f30f8..0ad730b 100644 --- a/server/pty-dispatcher.test.js +++ b/server/pty-dispatcher.test.js @@ -7,9 +7,14 @@ const { _pendingWake, _drainListeners, _lastChatSentAt, + _lastCapInjectedAt, + _submitTimers, + _setTimingsForTest, IDLE_THRESHOLD_MS, COALESCE_WINDOW_MS, ACTIVE_SUPPRESSION_MS, + MAX_DEFER_MS, + CAP_COOLDOWN_MS, } = require("./pty-dispatcher"); async function runTests() { @@ -33,7 +38,15 @@ async function runTests() { write: (data) => written.push(data), onData: (cb) => { onDataCallbacks.push(cb); - return { dispose: () => {} }; + // #1010: a real node-pty disposable detaches the listener. Modelling that + // lets the tests assert the drain listener was actually disposed (and + // stops a disposed cycle from being driven by a repaint loop). + return { + dispose: () => { + const i = onDataCallbacks.indexOf(cb); + if (i >= 0) onDataCallbacks.splice(i, 1); + }, + }; }, }; const session = { @@ -240,6 +253,219 @@ async function runTests() { assert(!_pendingWake.has("proj/dev"), "cleanupSession clears pending wake"); } + // ══════════════════════════════════════════════════════════════════════════ + // #1010: bounded deferred-wake cap under continuous TUI repaint. + // + // The bug these cover was invisible to the suite above because NO test ever + // invoked the captured onData callbacks — the `resetIdleTimer()` path was + // unreachable, so a repainting TUI could starve wake delivery forever and the + // suite stayed green. Every case below drives onData on a sub-idle-threshold + // interval, which is what makes it a real regression test. + // + // Timings are overridden to millisecond scale via _setTimingsForTest so these + // run in ~6 s rather than ~2 min at the shipped 10 s cap / 30 s cooldown. + // ══════════════════════════════════════════════════════════════════════════ + const KEY = "proj/dev"; + const sleep = (ms) => new Promise((r) => setTimeout(r, ms)); + const prompts = (written) => written.filter((w) => w.includes("You are @dev")).length; + + // Drive the captured onData callbacks like the Claude TUI's ~200 ms status-line + // repaint: fast enough that the idle timer is reset before it can ever fire. + function startRepaint(onDataCallbacks, intervalMs) { + const handle = setInterval(() => { + for (const cb of [...onDataCallbacks]) cb("\x1b[2K\x1b[1G· idle spinner"); + }, intervalMs); + return () => clearInterval(handle); + } + + // --- Test 14: #1010 core regression — a repainting Claude TUI still gets its + // wake, exactly once, at the cap. FAILS before the fix (0 injections: + // onData resets the only drain timer every 40 ms, so the idle gap never + // arrives and the wake is starved indefinitely). --- + { + _setTimingsForTest({ idleThresholdMs: 250, maxDeferMs: 300, capCooldownMs: 1000, activeSuppressionMs: 800 }); + const { sessions, written, onDataCallbacks } = makeSessions({ backend: "claude", lastOutputAt: Date.now() }); + const deps = makeDeps(); + dispatchToAgentPTY("proj", makeMsg({ sender: "head" }), sessions, deps); + const stopRepaint = startRepaint(onDataCallbacks, 40); + await sleep(150); + assert(prompts(written) === 0, "#1010: repainting Claude agent is not injected before the cap"); + await sleep(400); + assert(prompts(written) === 1, "#1010 REGRESSION: a continuously repainting Claude TUI receives its wake at the cap (pre-fix: never)"); + await sleep(300); + assert(prompts(written) === 1, "#1010: the cap delivers exactly once — the same cycle's idle timer cannot double-inject"); + stopRepaint(); + cleanupSession(KEY); + } + + // --- Test 15: #1010 — codex/gemini keep genuine-idleness-only delivery. No + // cap-driven text may land in an actively streaming turn; the wake must + // still be DEFERRED (not dropped) and land once the stream stops. --- + for (const backend of ["codex", "gemini"]) { + _setTimingsForTest({ idleThresholdMs: 250, maxDeferMs: 300, capCooldownMs: 1000, activeSuppressionMs: 800 }); + const { sessions, written, onDataCallbacks } = makeSessions({ backend, lastOutputAt: Date.now() }); + const deps = makeDeps(); + dispatchToAgentPTY("proj", makeMsg({ sender: "head" }), sessions, deps); + const stopRepaint = startRepaint(onDataCallbacks, 40); + await sleep(700); // well past the cap + assert(prompts(written) === 0, `#1010: ${backend} is NOT cap-injected during a continuously streaming turn`); + assert(_pendingWake.has(KEY), `#1010: ${backend}'s wake is still DEFERRED, not dropped`); + stopRepaint(); // the turn ends → genuine quiescence + await sleep(350); + assert(prompts(written) === 1, `#1010: ${backend} still wakes on a genuine idle gap (pre-#1010 behavior preserved)`); + cleanupSession(KEY); + } + + // --- Test 16: #1010 — a cap armed from the recent-send path must not preempt + // the active-send suppression window (#736). It re-arms once for the + // remainder instead of barging in. --- + { + _setTimingsForTest({ idleThresholdMs: 250, maxDeferMs: 200, capCooldownMs: 3000, activeSuppressionMs: 800 }); + const { sessions, written, onDataCallbacks } = makeSessions({ backend: "claude", lastOutputAt: 0 }); + const deps = makeDeps(); + _lastChatSentAt.set(KEY, Date.now()); // agent posted just now → presumed mid-turn + dispatchToAgentPTY("proj", makeMsg({ sender: "head" }), sessions, deps); + // The recent-send branch defers at dispatch time, so the cycle (and its cap) + // is already armed here. Start the repaint immediately: without it the idle + // timer would win and deliver on genuine quiescence (#923), which is correct + // behavior but not the path under test. + const stopRepaint = startRepaint(onDataCallbacks, 40); + assert(_pendingWake.has(KEY), "#1010: recent-send mention is deferred into a pending wake"); + await sleep(400); // past maxDeferMs, still inside activeSuppressionMs + assert(prompts(written) === 0, "#1010: the cap does NOT fire inside the active-send suppression window (#736 preserved)"); + await sleep(700); // suppression window has now closed + assert(prompts(written) === 1, "#1010: the re-armed cap delivers once the suppression window closes"); + stopRepaint(); + cleanupSession(KEY); + } + + // --- Test 17: #1010 — repeated deferred mentions during one long repainting + // turn yield one injection per cooldown window, and the next pending wake + // is deferred rather than discarded (no wake storm → no loop-guard trip). --- + { + _setTimingsForTest({ idleThresholdMs: 400, maxDeferMs: 150, capCooldownMs: 700, activeSuppressionMs: 0 }); + const { sessions, written, onDataCallbacks } = makeSessions({ backend: "claude", lastOutputAt: Date.now() }); + const deps = makeDeps(); + dispatchToAgentPTY("proj", makeMsg({ id: 1, sender: "head" }), sessions, deps); + const stopRepaint = startRepaint(onDataCallbacks, 40); + await sleep(250); + assert(prompts(written) === 1, "#1010 cooldown: the first cap delivery lands"); + // Second mention arrives while the same long turn is still repainting. + dispatchToAgentPTY("proj", makeMsg({ id: 2, sender: "re1" }), sessions, deps); + await sleep(300); // past a second cap interval, still inside the cooldown + assert(prompts(written) === 1, "#1010 cooldown: a second cap injection is withheld inside the cooldown window"); + assert(_pendingWake.has(KEY), "#1010 cooldown: the withheld wake is DEFERRED, not dropped"); + await sleep(500); // cooldown closes + assert(prompts(written) === 2, "#1010 cooldown: the deferred wake is delivered when the cooldown expires"); + stopRepaint(); + cleanupSession(KEY); + } + + // --- Test 18: #1010 — one lifecycle. After an idle drain, a cap drain, and + // cleanupSession, no pending wake, listener, timer handle or delayed-submit + // handle remains, and a delivered cycle's stale callbacks cannot affect the + // next cycle. --- + { + // 18a: idle drain disposes listener + handles, and the submit timer is + // released once it fires. + _setTimingsForTest({ idleThresholdMs: 150, maxDeferMs: 5000, capCooldownMs: 1000, activeSuppressionMs: 0 }); + const { sessions, written, onDataCallbacks } = makeSessions({ backend: "claude", lastOutputAt: Date.now() }); + const deps = makeDeps(); + dispatchToAgentPTY("proj", makeMsg({ sender: "head" }), sessions, deps); + assert(onDataCallbacks.length === 1, "#1010 lifecycle: the deferred-wake cycle hooks exactly one onData listener"); + await sleep(250); + assert(prompts(written) === 1, "#1010 lifecycle: idle drain delivers"); + assert(!_pendingWake.has(KEY), "#1010 lifecycle: idle drain clears the pending wake"); + assert(!_drainListeners.has(KEY), "#1010 lifecycle: idle drain clears the cycle state (both timer handles)"); + assert(onDataCallbacks.length === 0, "#1010 lifecycle: idle drain disposes the onData listener"); + assert(_submitTimers.has(KEY), "#1010 lifecycle: the delayed submit timer is owned while pending"); + await sleep(400); + assert(!_submitTimers.has(KEY), "#1010 lifecycle: the delayed submit timer releases itself after firing"); + assert(deps._writes.includes("\r"), "#1010 lifecycle: the submit CR is still written on the normal path"); + cleanupSession(KEY); + + // 18b: cap drain leaves nothing behind, and the delivered cycle's surviving + // idle timer cannot inject into the NEXT cycle (cross-cycle staleness). + _setTimingsForTest({ idleThresholdMs: 400, maxDeferMs: 150, capCooldownMs: 0, activeSuppressionMs: 0 }); + const b = makeSessions({ backend: "claude", lastOutputAt: Date.now() }); + const depsB = makeDeps(); + dispatchToAgentPTY("proj", makeMsg({ id: 1, sender: "head" }), b.sessions, depsB); + const stopRepaint = startRepaint(b.onDataCallbacks, 40); + await sleep(250); + assert(prompts(b.written) === 1, "#1010 lifecycle: cap drain delivers"); + assert(!_pendingWake.has(KEY) && !_drainListeners.has(KEY), "#1010 lifecycle: cap drain clears pending wake + cycle state"); + assert(b.onDataCallbacks.length === 0, "#1010 lifecycle: cap drain disposes the onData listener"); + dispatchToAgentPTY("proj", makeMsg({ id: 2, sender: "re1" }), b.sessions, depsB); + const stopRepaint2 = startRepaint(b.onDataCallbacks, 40); + await sleep(600); // past the new cap AND past the previous cycle's idle deadline + assert(prompts(b.written) === 2, "#1010 lifecycle: the next cycle delivers exactly once — no stale timer from the delivered cycle fires into it"); + stopRepaint(); + stopRepaint2(); + cleanupSession(KEY); + + // 18c: cleanupSession during the submit delay cancels the CR write, so a + // stopped session's dead term is never written to. + _setTimingsForTest({ idleThresholdMs: 150, maxDeferMs: 5000, capCooldownMs: 0, activeSuppressionMs: 0 }); + const c = makeSessions({ backend: "claude", lastOutputAt: Date.now() }); + const depsC = makeDeps(); + dispatchToAgentPTY("proj", makeMsg({ sender: "head" }), c.sessions, depsC); + await sleep(250); + assert(prompts(c.written) === 1 && _submitTimers.has(KEY), "#1010 lifecycle: injection wrote the prompt and armed the submit timer"); + cleanupSession(KEY); // operator stops the agent inside the submit delay + assert(!_submitTimers.has(KEY), "#1010 lifecycle: cleanupSession cancels the pending delayed-submit timer"); + await sleep(400); + assert(!depsC._writes.includes("\r"), "#1010 lifecycle: no CR is written to a term whose session was stopped mid-delay"); + } + + // --- Test 19: #1010 — a cap that fires after the session is gone performs no + // write. The armed session object is stale by then; delivery must + // re-resolve by key and require a running session with a live term. --- + { + _setTimingsForTest({ idleThresholdMs: 400, maxDeferMs: 150, capCooldownMs: 0, activeSuppressionMs: 0 }); + const { sessions, session, written, onDataCallbacks } = makeSessions({ backend: "claude", lastOutputAt: Date.now() }); + const deps = makeDeps(); + dispatchToAgentPTY("proj", makeMsg({ sender: "head" }), sessions, deps); + const stopRepaint = startRepaint(onDataCallbacks, 40); + session.state = "stopped"; // agent exits before the cap deadline + await sleep(300); + assert(prompts(written) === 0, "#1010: a cap firing on a no-longer-running session performs no write"); + assert(!_pendingWake.has(KEY) && !_drainListeners.has(KEY), "#1010: that cycle stands down cleanly instead of leaking"); + stopRepaint(); + cleanupSession(KEY); + + // #1010: eligibility is re-checked against the LIVE session. If the agent was + // respawned onto another backend after the cycle was armed, the cap must + // stand down — but the wake stays deferred and the idle path still delivers. + const r = makeSessions({ backend: "claude", lastOutputAt: Date.now() }); + const depsR = makeDeps(); + dispatchToAgentPTY("proj", makeMsg({ sender: "head" }), r.sessions, depsR); + const stopRepaintR = startRepaint(r.onDataCallbacks, 40); + r.sessions.set(KEY, { ...r.session, backend: "codex" }); // respawned onto codex + await sleep(300); + assert(prompts(r.written) === 0, "#1010: a cap armed on Claude does not inject after the session is respawned onto codex"); + assert(_pendingWake.has(KEY), "#1010: that wake stays deferred for the idle-gap path rather than being dropped"); + stopRepaintR(); + await sleep(500); // stream stops → genuine quiescence + assert(prompts(r.written) === 1, "#1010: the respawned codex session still wakes on a genuine idle gap"); + cleanupSession(KEY); + + // Same guard for a running session whose term was torn down. + const d = makeSessions({ backend: "claude", lastOutputAt: Date.now() }); + const depsD = makeDeps(); + dispatchToAgentPTY("proj", makeMsg({ sender: "head" }), d.sessions, depsD); + const stopRepaintD = startRepaint(d.onDataCallbacks, 40); + d.session.term = null; + await sleep(300); + assert(depsD._writes.length === 0, "#1010: a cap firing on a session with no live term performs no write"); + stopRepaintD(); + cleanupSession(KEY); + } + + // Restore the shipped timings so nothing after this block runs on test values. + _setTimingsForTest(); + assert(MAX_DEFER_MS === 10000 && CAP_COOLDOWN_MS === ACTIVE_SUPPRESSION_MS, "#1010: shipped cap is 10s and the cap cooldown matches the active-suppression window"); + assert(!_lastCapInjectedAt.has(KEY), "#1010: cleanupSession clears the per-key cap cooldown"); + console.log(`\n${passed} passed, ${failed} failed\n`); process.exit(failed > 0 ? 1 : 0); } From 01c2c12a1f5367cce4c957c376067052322b7e97 Mon Sep 17 00:00:00 2001 From: Cho Young-Hwi Date: Sat, 25 Jul 2026 00:49:31 +0000 Subject: [PATCH 2/2] [#1010] Pin the stale-cycle guard with a discriminating test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit re2 on PR #1020: test 18b asserted the "a stale callback cannot affect a subsequent cycle" contract but could not discriminate it — by the time the cap drain returns, cleanupDrainListener has already cleared both handles, so no stale timer ever exists. Replacing the guard with `() => true` left the suite fully green. Adds a case that constructs the stale timer explicitly: arm a Claude cycle under repaint, supersede it in _drainListeners while its cap is still pending, then let that cap fire. Asserts it performs no injection, does not dispose the superseding cycle's listener, and does not consume its pending wake. Verified discriminating — with the guard replaced by `() => true` all three assertions fail (injection lands, sentinel disposed, pending wake consumed). Also mutation-tested the two guards not covered in re2's sweep: dropping the fire-time running/live-term check and dropping the submit-timer cancellation both fail their cases. No production behavior change. The one source edit documents the `capRearmed` bound re2 flagged: the MAX_DEFER_MS + ACTIVE_SUPPRESSION_MS worst case holds for a single post, and re-arming per post is deliberately not done since a chatty agent could then extend deferral indefinitely — recreating the starvation this cap bounds. Co-Authored-By: Claude Opus 5 --- server/pty-dispatcher.js | 8 ++++++++ server/pty-dispatcher.test.js | 37 +++++++++++++++++++++++++++++++++++ 2 files changed, 45 insertions(+) diff --git a/server/pty-dispatcher.js b/server/pty-dispatcher.js index 8a60427..45d1c08 100644 --- a/server/pty-dispatcher.js +++ b/server/pty-dispatcher.js @@ -213,6 +213,14 @@ function queuePendingWake(key, session, agentSessions, deps) { // #736: the cap must not preempt the deliberate active-send suppression // window. If the agent posted recently it is probably still mid-turn, so // re-arm ONCE for whatever remains of that window instead of barging in. + // + // `capRearmed` is per-cycle and spent on first use, so total deferral is + // bounded by MAX_DEFER_MS + ACTIVE_SUPPRESSION_MS for a single post. An + // agent that posts AGAIN during the re-arm window is injected inside a fresh + // suppression window — deliberate: the alternative is re-arming per post, + // which a chatty agent could extend indefinitely, recreating the starvation + // this cap exists to bound. Safe because the cap is Claude-only and Claude + // queues input received mid-turn rather than interrupting. const lastSent = _lastChatSentAt.get(key); const remainingSuppression = lastSent ? _timings.activeSuppressionMs - (Date.now() - lastSent) diff --git a/server/pty-dispatcher.test.js b/server/pty-dispatcher.test.js index 0ad730b..5fb0571 100644 --- a/server/pty-dispatcher.test.js +++ b/server/pty-dispatcher.test.js @@ -461,6 +461,43 @@ async function runTests() { cleanupSession(KEY); } + // --- Test 20: #1010 — the per-cycle identity guard is load-bearing. A timer + // that outlived its own cycle must not inject, must not dispose the cycle + // that superseded it, and must not consume that cycle's pending wake. + // + // @re2 on PR #1020: test 18b asserted this property but could not + // discriminate it — by the time the cap drain returns, cleanupDrainListener + // has already cleared both handles, so no stale timer ever exists and + // replacing the guard with `() => true` left the suite green. This case + // constructs the stale timer explicitly: arm a cycle, supersede it in + // `_drainListeners` while its cap is still pending, then let that cap fire. + { + _setTimingsForTest({ idleThresholdMs: 800, maxDeferMs: 200, capCooldownMs: 0, activeSuppressionMs: 0 }); + const { sessions, written, onDataCallbacks } = makeSessions({ backend: "claude", lastOutputAt: Date.now() }); + const deps = makeDeps(); + dispatchToAgentPTY("proj", makeMsg({ sender: "head" }), sessions, deps); + const stopRepaint = startRepaint(onDataCallbacks, 40); + + // Supersede the armed cycle. Its cap timer is still pending and WILL fire. + let sentinelDisposed = false; + const sentinel = { + disposable: { dispose: () => { sentinelDisposed = true; } }, + idleHandle: null, + capHandle: null, + capRearmed: false, + }; + _drainListeners.set(KEY, sentinel); + + await sleep(400); // past the superseded cycle's cap deadline + assert(prompts(written) === 0, "#1010 stale-cycle: a cap timer that outlived its cycle performs no injection"); + assert(_drainListeners.get(KEY) === sentinel && !sentinelDisposed, "#1010 stale-cycle: it does not dispose the cycle that superseded it"); + assert(_pendingWake.has(KEY), "#1010 stale-cycle: it does not consume the superseding cycle's pending wake"); + + stopRepaint(); + _drainListeners.delete(KEY); + cleanupSession(KEY); + } + // Restore the shipped timings so nothing after this block runs on test values. _setTimingsForTest(); assert(MAX_DEFER_MS === 10000 && CAP_COOLDOWN_MS === ACTIVE_SUPPRESSION_MS, "#1010: shipped cap is 10s and the cap cooldown matches the active-suppression window");