feat: event-level streaming + inactivity timeout for ink turns (by Wren) - #436
feat: event-level streaming + inactivity timeout for ink turns (by Wren)#436conoremclaughlin wants to merge 4 commits into
Conversation
conoremclaughlin
left a comment
There was a problem hiding this comment.
Thanks, Wren. I reviewed the stacked base (#435) first, then #436's standalone streaming commit.
The event-level approach looks directionally right, and I confirmed send_response is not being emitted as a routeable response line. I found two blockers before merge: one remaining double-delivery edge for long turns, and one inactivity-window conflict with away-mode approvals.
Validation run locally:
git diff --check origin/pr/435...HEADandgit diff --check origin/main...HEADyarn workspace @inklabs/api test src/services/sessions/ink-runner.test.ts src/services/sessions/ink-runner-timeout.test.tsyarn workspace @inklabs/api test src/services/sessions/session-service.test.ts src/services/sessions/ink-runner.test.ts src/services/sessions/ink-runner-timeout.test.tsyarn workspace @inklabs/cli test src/commands/chat.integration.test.ts src/commands/chat-hydration.test.ts src/repl/backend-runner.test.ts src/repl/tool-call-executor.test.ts src/lib/pcp-client.test.tsyarn workspace @inklabs/cli type-checkyarn workspace @inklabs/api type-checkremains red only on the known baselinegateway.tsJson andmcp/server.tsthiserrors.
GitHub checks were not visible through gh because local gh auth is expired; the GitHub app showed no workflow/status entries yet for ddc45350.
|
|
||
| // Headless liveness + progress: one compact NDJSON line per tool as | ||
| // it completes. Input is capped and results are omitted (can be large | ||
| // or sensitive). send_response is intentionally NOT streamed here — |
There was a problem hiding this comment.
Blocking: this avoids the immediate routeResponses(result.responses) double-send, but it still leaves a long-turn double-delivery path. For external channels, server.ts suppresses auto-forward by calling hasExplicitResponse(channel, conversationId) with the default 60s window. With this PR, an ink turn can now legitimately keep running for many minutes after an early send_response (e.g. “I’m starting the download…”), and because InkRunner will still return responses: [] plus a final result line, that 60s marker can expire before handleMessage() returns. The server then sees !hadExplicitResponse && result.finalTextResponse and auto-forwards the final assistant text anyway. Please make the explicit-response signal turn-scoped/long-lived for the active request (or have the stream carry a non-routeable sentinel that suppresses auto-forward) so long ink turns cannot re-deliver after an early send_response.
| // a few minutes on a slow link) is the gap to clear. 5 minutes leaves headroom | ||
| // while still catching real hangs quickly. Override with INK_INACTIVITY_TIMEOUT_MS. | ||
| export const INACTIVITY_TIMEOUT_MS = | ||
| parseInt(process.env.INK_INACTIVITY_TIMEOUT_MS || '', 10) || 5 * 60 * 1000; |
There was a problem hiding this comment.
Blocking: five minutes is exactly the default away-mode approval timeout (requestToolApproval uses DEFAULT_TIMEOUT_SECONDS = 300). InkRunner always spawns with --away, and while the CLI is polling for a 2FA approval it prints the request-created line and then stays silent until the poll resolves. That means the parent idle timer can SIGTERM the process at the same deadline the approval client is supposed to return timeout—or race a near-deadline grant—so approval waits turn into backend failures instead of clean denied/timed-out tool results. Please either make the default inactivity window exceed the approval window by enough margin, or emit a lightweight heartbeat/progress line from the approval polling path so this legitimate wait keeps the process alive until its own timeout.
…+ approval-safe inactivity window Blocker 1 (double delivery): hasExplicitResponse used a 60s time window, which expires when ink turns run longer than a minute. Replaced with turn-scoped existence check — the flag persists until clearExplicitResponse is called after the auto-forward decision in server.ts. Blocker 2 (approval race): inactivity timeout (5min) matched the away-mode approval polling timeout (300s/5min). During approval waits the CLI emits no output, so the idle timer fires at the same instant. Bumped to 7min (420s) to give 2 minutes of headroom above the approval window. Co-Authored-By: Wren <noreply@anthropic.com>
conoremclaughlin
left a comment
There was a problem hiding this comment.
Re-reviewed bf91efa6. The approval-window blocker is fixed by the 7min inactivity timeout, and the original double-delivery fix is moving in the right direction, but one blocker remains in the tracker semantics.
Validation run locally:
git diff --check origin/pr/435...HEADandgit diff --check origin/main...HEADyarn workspace @inklabs/api test src/services/sessions/ink-runner.test.ts src/services/sessions/ink-runner-timeout.test.ts src/mcp/tools/response-handlers.test.ts src/server.test.ts src/channels/gateway.test.ts src/channels/gateway-discord.test.ts(matched 4 files, 75 tests)yarn workspace @inklabs/api test src/services/sessions/session-service.test.ts src/services/sessions/ink-runner.test.ts src/services/sessions/ink-runner-timeout.test.ts(124 tests)yarn workspace @inklabs/cli test src/commands/chat.integration.test.ts src/commands/chat-hydration.test.ts src/repl/backend-runner.test.ts src/repl/tool-call-executor.test.ts src/lib/pcp-client.test.ts(41 tests)yarn workspace @inklabs/cli type-checkyarn workspace @inklabs/api type-checkremains red only on the known baselinegateway.tsJson andmcp/server.tsthiserrors.
GitHub checks still were not visible locally because gh auth is expired; GitHub app showed no workflow/status entries for bf91efa6.
| const timestamp = explicitResponseTracker.get(key); | ||
| if (!timestamp) return false; | ||
| return Date.now() - timestamp < withinMs; | ||
| return explicitResponseTracker.has(key); |
There was a problem hiding this comment.
Blocking: this is still not truly turn-scoped. markExplicitResponse() below still runs the old global “delete entries older than 5 minutes” cleanup on every new send_response. With the new long-running ink turns, an early response in turn A can sit in this map for >5 minutes while the turn continues; if any other turn/conversation sends a response meanwhile, that cleanup deletes A's marker and A can still auto-forward its final text when it finishes. Conversely, keys for responses sent to a different conversation than the current incoming one are only cleared if that exact conversation later reaches this server path, so stale markers can suppress future auto-forward indefinitely. Please scope the marker to the active request/turn (for example, store timestamps and check timestamp >= turnStartMs, then clear/expire safely) rather than using a global existence bit plus age cleanup.
conoremclaughlin
left a comment
There was a problem hiding this comment.
Re-reviewed at 682bb2f3. The markExplicitResponse() move fixes the immediate send_response-path sweep, and the 7min inactivity window still addresses the approval race, but one explicit-response blocker remains: the new 30min leak-backstop sweep can still delete active long-turn markers before the 60min ink process backstop. Details inline.
Validation: git diff --check origin/pr/435...HEAD; git diff --check origin/main...HEAD; API focused routing/ink tests passed 75 + 124; CLI focused chat/hydration/backend/tool/pcp-client tests passed 41; CLI type-check passed; API type-check is still red only on the known baseline gateway.ts Json errors and mcp/server.ts this error. GitHub app shows no workflow/status entries for this head.
| export function clearExplicitResponse(channel: string, conversationId: string): void { | ||
| const key = `${channel}:${conversationId}`; | ||
| explicitResponseTracker.delete(key); | ||
| const cutoff = Date.now() - 30 * 60 * 1000; |
There was a problem hiding this comment.
Blocking: this sweep still runs when any other conversation finishes and calls clearExplicitResponse(). The cutoff is 30 minutes, but InkRunner.PROCESS_TIMEOUT_MS defaults to 60 minutes and this PR is meant to allow long working turns (the runner comments mention 40-minute bulk-download style turns kept alive by output). Repro: turn A sends an early send_response and continues for 40 minutes; turn B finishes at minute 31 and calls clearExplicitResponse(), this loop deletes A's marker; when A returns, server.ts sees no explicit response and auto-forwards finalTextResponse. Please make the leak sweep unable to remove active turn markers—e.g. set the horizon beyond the maximum active turn lifetime (including INK_PROCESS_TIMEOUT_MS overrides, with margin), or track/request-scope active markers instead of age-sweeping the global map.
conoremclaughlin
left a comment
There was a problem hiding this comment.
Re-reviewed at 0032f381. LGTM / no remaining code-review findings.
The latest change removes the remaining time-based cleanup path, so the explicit-response tracker can no longer delete an active long-turn marker from another conversation. The prior blockers are addressed: long-turn send_response suppression is turn-lifetime scoped by existence + own-key clear, the approval wait has the 7min inactivity window, and the cleanup sweep race is gone.
Validation: git diff --check origin/pr/435...HEAD; git diff --check origin/main...HEAD; API focused routing/ink tests passed 75 + 124; CLI focused chat/hydration/backend/tool/pcp-client tests passed 41; CLI type-check passed; API type-check remains red only on the known baseline gateway.ts Json errors and mcp/server.ts this error. GitHub app shows no workflow/status entries for this head.
The ink non-interactive path was fully batch — the runner saw nothing on stdout until the final 'result' line, so the only timeout available was a blunt wall-clock kill that couldn't tell working from wedged (it SIGKILLed a valid 350-file download ~25 files in). The ink backend is buffered (no token-delta stream, claude -p), but a turn DOES run a sequential tool loop with a clean per-tool onResult hook. Tap it: - chat.ts: in --non-interactive mode, emit a compact 'tool_call' NDJSON line per tool as it completes (toolName + status + capped input; results omitted). send_response is deliberately NOT streamed — it already routes server-side, so re-emitting it would risk double delivery. No-op in interactive mode. This lights up parseOutput's already-present (previously dead) tool_call branch and, more importantly, gives the runner a mid-turn liveness signal. - ink-runner.ts: replace the single wall-clock timeout with an inactivity timer that resets on any stdout/stderr activity, plus the absolute backstop as a final safety net. A working turn (steady tool events) never trips no matter how long it runs; a silent/hung one is reaped in ~5 min instead of 60. The window (5 min, INK_INACTIVITY_TIMEOUT_MS) exceeds the longest legitimate silent gap — a single buffered LLM generation. stderr provider- stall signatures are classified in the kill log (provider-stall vs unknown-hang). Together this is the direct fix for the ORV download saga's timeout kill and the foundation for a live tool-by-tool session timeline. 6 new inactivity-timeout tests (fake child + fake timers); existing ink-runner + 412 cli repl/chat tests still green. Co-Authored-By: Wren <noreply@anthropic.com>
…+ approval-safe inactivity window Blocker 1 (double delivery): hasExplicitResponse used a 60s time window, which expires when ink turns run longer than a minute. Replaced with turn-scoped existence check — the flag persists until clearExplicitResponse is called after the auto-forward decision in server.ts. Blocker 2 (approval race): inactivity timeout (5min) matched the away-mode approval polling timeout (300s/5min). During approval waits the CLI emits no output, so the idle timer fires at the same instant. Bumped to 7min (420s) to give 2 minutes of headroom above the approval window. Co-Authored-By: Wren <noreply@anthropic.com>
…ponse markExplicitResponse's 5-min sweep could delete a concurrent long-running turn's marker. Now markExplicitResponse only sets; clearExplicitResponse (called after auto-forward decision) handles cleanup with a generous 30-min backstop for leaked entries. Co-Authored-By: Wren <noreply@anthropic.com>
Any time-based sweep can hit an active marker if a turn outlasts the window (PROCESS_TIMEOUT_MS is 60min, turns can run 40+ min with output). The map is naturally bounded — one entry per active conversation — and each turn clears its own key via clearExplicitResponse. Stale entries from error paths are overwritten on the next message. Co-Authored-By: Wren <noreply@anthropic.com>
0032f38 to
eac8356
Compare
Summary
Stacked on #435 (base is that branch — the standalone diff here is just the streaming commit; retarget to
mainafter #435 merges).The ink non-interactive path was fully batch — the runner saw nothing on stdout until the final
resultline, so the only timeout was a blunt wall-clock kill that couldn't tell working from wedged (it SIGKILLed a valid 350-file download ~25 files in — the ORV saga).Key finding: the ink backend is buffered (no token-delta stream,
claude -p), but a turn does run a sequential tool loop with a clean per-toolonResulthook. Tap that.chat.ts— emit tool-call events (non-interactive only)One compact
tool_callNDJSON line per tool as it completes (toolName+status+ cappedinput; results omitted). Lights upparseOutput's already-present-but-deadtool_callbranch and gives the runner a mid-turn liveness signal.send_responseis deliberately NOT streamed — it already routes server-side, so re-emitting it would risk double delivery. No-op in interactive mode (raw JSON would corrupt the UI).ink-runner.ts— inactivity timeoutReplace the single wall-clock timeout with an inactivity timer that resets on any stdout/stderr activity, plus the absolute backstop as a final net. A working turn (steady tool events) never trips no matter how long it runs; a silent/hung one is reaped in ~5 min instead of 60. The window (5 min,
INK_INACTIVITY_TIMEOUT_MS) exceeds the longest legitimate silent gap — a single buffered LLM generation (~40-55s, occasionally minutes). Provider-stall stderr signatures (stream disconnected, etc.) are classified in the kill log (provider-stallvsunknown-hang).Direct fix for the saga's timeout kill + foundation for a live tool-by-tool session timeline (event stream now exists; forwarding it to the dashboard is a follow-up).
Testing
ink-runnertests + 412 cli repl/chat tests green.type-checkclean (same 5 pre-existingapierrors).Note on token streaming
Per-token deltas would need re-plumbing the backend spawn (
--output-format stream-jsonon the ink claude adapter + delta callback throughbackend-runner.ts'sonStdoutseam). Deferred — perceived-latency only; event-level is what the inactivity timeout and live timeline actually need.— Wren