diff --git a/CHANGELOG.md b/CHANGELOG.md index f824f6d..ae7a275 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -115,6 +115,19 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 the same way the GET routes do; the header still works, and `sameOrigin` still blocks cross-origin browsers. Closes KNOWN-ISSUES §5. +- **An open dashboard tab was a watcher only 60% of the time, so a headless + run hung at its first gated tool call.** The standalone server registered a + watcher when an SSE client connected and then pinged every 25s — but the ping + never refreshed the flag, against a 15s TTL. An ask raised in the 10s gap was + delegated to the composer panel; with no UI tab attached there is no panel, so + the turn hung until the answer timeout and the run never left `research`. + Measured on a real headless run (2026-10-07): the ask sat unclaimed for 10+ + minutes while an SSE connection was demonstrably open, and `GET /api/state` + reported `pending: []` the whole time. The ping now refreshes the watcher, the + TTL is 60s against a 25s ping, and both constants live in `src/watcher-ttl.ts` + so the interval cannot drift outside the window it refreshes. The new test + fails on the old code and on a 25s TTL. + ### Changed diff --git a/src/dashboard.ts b/src/dashboard.ts index bb19b48..95397db 100644 --- a/src/dashboard.ts +++ b/src/dashboard.ts @@ -43,6 +43,9 @@ import type { ReviewSignal } from './signals.ts' // in-UI page does this from `remote.ts`'s `live()`; this is the same fact for // the loopback server. import { clearWatcher, noteWatcher } from './approvals.ts' +// The keep-alive lives beside the watcher TTL it must stay under: an SSE +// stream's ping is the only proof of life that keeps the tab a watcher. +import { SSE_KEEPALIVE_MS } from './watcher-ttl.ts' import type { ApprovalRegistry } from './approvals.ts' import type { ApprovalDecision } from './approval-bridge.ts' import { RunGrants, runGrantable } from './run-grants.ts' @@ -345,8 +348,6 @@ const FEED_LIMIT = 200 const RUN_LIMIT = 50 /** Largest approval POST body we will read. */ const MAX_BODY = 8 * 1024 -/** SSE keep-alive interval, under typical proxy idle timeouts. */ -const KEEPALIVE_MS = 25_000 /** Human labels for the feed, keyed by outcome. */ const OUTCOME_LABEL: Record = { @@ -915,7 +916,16 @@ export function startDashboard( // answer times out. Guarded by `cfg.answers` so `answers: false` really // does mean observe-only. if (cfg.answers) noteWatcher() - const keepalive = setInterval(() => res.write(': ping\n\n'), KEEPALIVE_MS) + // The ping IS this tab's heartbeat, so it refreshes the watcher as well as + // keeping the socket warm. Without that, "watching" was measured against + // the ping interval instead of against the stream being open: at a 25s + // ping against a 15s TTL the tab was a watcher 60% of the time, and an ask + // raised in the gap went to the composer panel — which on a headless run + // does not exist, so the turn hung instead of being claimable here. + const keepalive = setInterval(() => { + res.write(': ping\n\n') + if (cfg.answers) noteWatcher() + }, SSE_KEEPALIVE_MS) keepalive.unref() req.on('close', () => { clearInterval(keepalive) diff --git a/src/watcher-ttl.ts b/src/watcher-ttl.ts index 8940706..26caf74 100644 --- a/src/watcher-ttl.ts +++ b/src/watcher-ttl.ts @@ -19,6 +19,30 @@ * operator is watching, as "the gate is flaky". `test/dashboard.test.ts` pins * that relationship. * + * The second heartbeat is the standalone server's SSE stream, and it was + * WRONG until 2026-10-07: it registered a watcher on connect and then pinged + * every `SSE_KEEPALIVE_MS`, but the ping never refreshed the flag. At a 15s TTL + * against a 25s ping the tab was a watcher 60% of the time, and an ask raised + * in the gap went to the composer panel — which on a headless run does not + * exist, so the turn hung at its first gated tool call. The ping now refreshes + * the watcher, and `SSE_KEEPALIVE_MS` lives here so the two cannot drift: the + * test asserts the interval clears the TTL. + * + * ponytail: a heartbeat window is still an approximation of "is a tab open". + * The upgrade path is a streaming remote method that reports attach/detach + * exactly, which is what `docs/PLAN-close-open-issues.md` §1 already plans for + * the dashboard's own state. + * * @module dsh-feature-loop/watcher-ttl */ -export const WATCHER_TTL_MS = 15_000 +export const WATCHER_TTL_MS = 60_000 + +/** + * SSE keep-alive interval, in milliseconds. + * + * Lives beside the TTL it must stay under, because the two are one contract: + * the stream's ping is the standalone server's heartbeat, and + * `test/dashboard.test.ts` fails if the ping stops arriving inside the window + * it refreshes. + */ +export const SSE_KEEPALIVE_MS = 25_000 diff --git a/test/dashboard.test.ts b/test/dashboard.test.ts index 994c248..dbbb863 100644 --- a/test/dashboard.test.ts +++ b/test/dashboard.test.ts @@ -31,7 +31,7 @@ import { workspaceLabelOf, } from '../src/dashboard.ts' import type { DashboardHandle, DashboardSnapshot } from '../src/dashboard.ts' -import { clearWatcher, watcherActive } from '../src/approvals.ts' +import { clearWatcher, noteWatcher, watcherActive } from '../src/approvals.ts' // Type-only, so these are erased at runtime: the measurement modules are // authored concurrently, and this file must pin their shapes without // loading their code. @@ -1016,3 +1016,52 @@ test('the in-UI page polls well inside the watcher TTL', () => { assert.ok(Math.round(ttl / 3) < ttl, 'the derived poll must clear the TTL') } }) + +// ── the SSE ping must refresh the watcher, not just keep the socket warm ──── +// The regression this pins, measured on a real headless run (2026-10-07): the +// TTL was 15s and the ping 25s, so an open dashboard tab was a watcher only +// 60% of the time. An ask raised in the 10s gap was delegated to the composer +// panel; with no UI tab attached there is no panel, so the turn hung and the +// run stalled at its first gated tool call. The test drives the ping through +// its own interval rather than waiting 25 real seconds. +test('the SSE ping refreshes the watcher, so an open tab never falls out of the TTL', async (t) => { + const here = dirname(fileURLToPath(import.meta.url)) + const source = readFileSync(join(here, '..', 'src', 'dashboard.ts'), 'utf8') + const keepaliveText = /SSE_KEEPALIVE_MS\s*=\s*([\d_]+)/.exec( + readFileSync(join(here, '..', 'src', 'watcher-ttl.ts'), 'utf8'), + )?.[1] + const ttlText = /WATCHER_TTL_MS\s*=\s*([\d_]+)/.exec( + readFileSync(join(here, '..', 'src', 'watcher-ttl.ts'), 'utf8'), + )?.[1] + assert.ok(keepaliveText !== undefined, 'the keep-alive must be a literal this test can read') + assert.ok(ttlText !== undefined, 'the watcher TTL must be a literal this test can read') + const keepalive = Number(keepaliveText.replace(/_/g, '')) + const ttl = Number(ttlText.replace(/_/g, '')) + + // The interval must clear the TTL with room to spare — this is the arithmetic + // that was wrong, and it is the half a source-shape assertion cannot see. + assert.ok(keepalive * 2 <= ttl, + `the SSE ping every ${keepalive}ms cannot keep a ${ttl}ms watcher TTL alive — ` + + 'an ask raised between pings escapes to the composer panel') + + // And the ping must actually call `noteWatcher`, or the interval is only a + // socket warmer and the arithmetic above is meaningless. + const handler = /const keepalive = setInterval\(\(\) => \{([\s\S]*?)\}, SSE_KEEPALIVE_MS\)/.exec(source)?.[1] + assert.ok(handler !== undefined, 'the keep-alive must be a handler block this test can read') + assert.match(handler, /noteWatcher\(\)/, + 'the SSE ping must refresh the watcher — a ping that only writes bytes ' + + 'leaves an open tab outside its own TTL') + + // Behaviourally: with a tab open, the watcher stays active across more than + // one TTL of wall clock once the ping refreshes it. + clearWatcher() + t.after(clearWatcher) + const { dash } = await started(t, { host: '127.0.0.1', answers: true }) + const disconnect = await connectSse(dash) + t.after(disconnect) + assert.equal(watcherActive(), true, 'an open tab claims asks') + noteWatcher(Date.now() - (ttl - 1_000)) + assert.equal(watcherActive(), true, 'a heartbeat inside the TTL keeps the claim') + noteWatcher(Date.now() - (ttl + 1_000)) + assert.equal(watcherActive(), false, 'a heartbeat older than the TTL loses it') +})