fix(sdk): watch-mode chat subscriptions survive quiet windows - #4548
Conversation
🦋 Changeset detectedLatest commit: 550b717 The changes in this PR will be included in the next version bump. This PR includes changesets to release 27 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughWatch mode now reconnects after completed turns and idle-window EOFs. It continues beyond the normal EOF resubscribe limit. The subscription stops when the session is settled or the operation is aborted. Tests cover reconnect behavior, settled-session termination, aborts during backoff, and updated SSE fixtures. A patch changeset documents the SDK change. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
… side effect (TRI-13070)
@trigger.dev/build
trigger.dev
@trigger.dev/core
@trigger.dev/python
@trigger.dev/react-hooks
@trigger.dev/redis-worker
@trigger.dev/rsc
@trigger.dev/schema-to-json
@trigger.dev/sdk
commit: |
…to fix/watch-mode-keepalive-tri-13065
…to fix/watch-mode-keepalive-tri-13065
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…ate on give-up - reconnect no longer peek-settles in watch mode, so a settled peek between turns can't close the standing subscription before the next turn. - the returned stream now aborts its resubscribe loop when the reader is cancelled, instead of leaking it. - clear and persist isStreaming before the budget-exhaustion throw so a reload doesn't reopen a doomed subscription.
…stream-error A consumer cancelling the watch stream aborts the resubscribe loop, which reaches controller.close() on an already-closed controller. The resulting 'Invalid state' throw was surfaced as a bogus stream-error on every clean watch-viewer unmount. Wrap the remaining bare close sites to match the existing pattern.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…at/agent-storybook-gallery
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| // Watch mode is a standing subscription: it outlives turn-complete | ||
| // (which clears `isStreaming`) and idle windows EOF by design, so the | ||
| // give-up budget doesn't apply. Only abort or a settled session ends it. | ||
| while ( | ||
| state.isStreaming && | ||
| (this.watchMode || (state.isStreaming && eofResubscribes < MAX_EOF_RESUBSCRIBES)) && | ||
| !currentSubscription?.sessionSettled && | ||
| !combinedSignal.aborted && | ||
| eofResubscribes < MAX_EOF_RESUBSCRIBES | ||
| !combinedSignal.aborted |
There was a problem hiding this comment.
🟡 A cancelled older chat connection can wrongly mark the new, in-progress reply as finished
When an older subscription that was cancelled mid-backoff finally tears down, it clears the shared "a reply is in progress" flag (state.isStreaming = false at packages/trigger-sdk/src/v3/chat.ts:1856) even though it was cancelled and a newer reply already set that flag, so the new reply is recorded as finished while it is still arriving.
Impact: If the page is reloaded during that new reply, it will not resume and the rest of the answer is lost.
Race between supersede and the EOF-backoff tail of the old stream
sendMessages (packages/trigger-sdk/src/v3/chat.ts:866-880) aborts the previous stream, then sets state.isStreaming = true and subscribes again — both streams share the same ChatSessionState object from this.sessions. If the aborted stream was sleeping inside the backoff in resumeAfterEof, the abort resolves the sleep, the loop breaks, the new throw block is correctly skipped (!combinedSignal.aborted), but the following tail block is not guarded by combinedSignal.aborted and unconditionally writes state.isStreaming = false plus notifySessionChange, clobbering the successor turn's state and persisting isStreaming: false to the customer's storage. On reload, reconnectToStream returns null because state.isStreaming === false, so the in-flight turn is never resumed. Adding !combinedSignal.aborted to the tail condition (mirroring the guard used in the new budget-exhausted block at packages/trigger-sdk/src/v3/chat.ts:1838-1842) avoids the clobber.
Was this helpful? React with 👍 or 👎 to provide feedback.
| while ( | ||
| state.isStreaming && | ||
| (this.watchMode || (state.isStreaming && eofResubscribes < MAX_EOF_RESUBSCRIBES)) && | ||
| !currentSubscription?.sessionSettled && | ||
| !combinedSignal.aborted && | ||
| eofResubscribes < MAX_EOF_RESUBSCRIBES | ||
| !combinedSignal.aborted |
There was a problem hiding this comment.
🔴 With watch mode on, sending a message leaves the chat stuck in a sending state forever
The message stream is kept open indefinitely after the reply has finished (this.watchMode || at packages/trigger-sdk/src/v3/chat.ts:1809) instead of ending, so the chat never returns to a ready state and keeps re-contacting the server forever.
Impact: On a chat configured for watch mode, every send leaves the UI spinning with the input disabled after the answer is complete, and the browser keeps opening new requests for that chat indefinitely.
Why the standing-subscription condition also captures owning send streams, and why the settled exit can never fire
subscribeToSessionStream is shared by sendMessages (packages/trigger-sdk/src/v3/chat.ts:877-880), sendAction (packages/trigger-sdk/src/v3/chat.ts:1288-1291) and reconnectToStream. The new loop condition drops the state.isStreaming guard whenever this.watchMode is set, regardless of which caller opened the stream. Previously, in watch mode a send stream read past trigger:turn-complete (packages/trigger-sdk/src/v3/chat.ts:2014), which sets state.isStreaming = false, and then closed at the next body EOF because resumeAfterEof's state.isStreaming guard was false. Now the loop keeps resubscribing.
The only remaining exits are combinedSignal.aborted and currentSubscription.sessionSettled. sessionSettled is derived from the X-Session-Settled response header (packages/core/src/v3/apiClient/runStream.ts:424), and the server only ever emits that header when the request carried X-Peek-Settled: 1 (apps/webapp/app/services/realtime/s2realtimeStreams.server.ts:417-432, apps/webapp/app/routes/realtime.v1.sessions.$session.$io.ts:186). sendMessages/sendAction never set peekSettled, and reconnectToStream now explicitly disables it in watch mode (packages/trigger-sdk/src/v3/chat.ts:1184). So for a watch-mode send there is no terminating condition other than an explicit abort — the returned ReadableStream never closes, and the AI SDK consumer never leaves the streaming state.
The PR's own tests reflect this: the watch-mode sendMessages streams in the "superseded stream teardown" suite only finish after transport.dispose() or an explicit supersede/abort.
Was this helpful? React with 👍 or 👎 to provide feedback.
kathiekiwi
changed the title
kathiekiwi
changed the title
kathiekiwi
changed the title
…ry (#4549) ## What & why The agent's query tool is read-only, but that was true by three separate facts and only one of them had a test. This proves the boundary holds by contract rather than by prompt, and stops a broken query from burning a whole agent turn. Two small guards, the rest is tests. [TRI-11165](https://linear.app/triggerdotdev/issue/TRI-11165). ## Stack Stacked on **#4548** (watch-mode keepalive). Merge that first. ## What's inside - **A route-level read-only test** (`apps/webapp/test/queryRouteReadOnly.test.ts`) that drives `api.v1.query` with a real signed environment JWT: a multi-statement write and a mutating statement are both refused before anything reaches ClickHouse, and a plain read passes so the seam stays live. - **`readonly=1` made non-overridable** in `queryService.server.ts` — caller `clickhouseSettings` were spread after the defaults and could clear it. - **A per-turn query-retry cap** in the agent's `run_query` tool (`internal-packages/dashboard-agent/src/tool-api.ts`): three consecutive failures returns a terminal "stop and answer with what you have". ## Key decisions - **The read-only guarantee is grammar-level, not filtered.** TRQL has no write statements — they don't parse — ClickHouse runs with `readonly=1`, and the org/project/env scoping is injected server-side from the credential. The request body can't widen scope or turn a read into a write. - **The deny test runs through the route, not the parser.** A parser-only test would stay green if a refactor routed agent SQL around the compiler; driving the real route with a signed JWT pins the boundary end-to-end, and the deliberate positive read keeps the assertion honest. - **The retry cap lives per turn, not in the prompt.** A failed query hands the model the database error to fix, and usually it does — but the only other limit was the turn's 10 steps, so one query the model couldn't fix could eat the whole turn and leave the user with no answer. The tool set is built per turn, so the counter caps consecutive failures; a success resets it. The retry instruction rides the error text, so the prompt prefix is unchanged. ## Testing - `queryRouteReadOnly.test.ts` — write statements → 400, ClickHouse never called; a read passes. - `tool-query-retry-cap.test.ts` — terminal at the third consecutive failure, counter resets on a success. - The load-bearing guards were control-broken first (readonly override re-enabled; cap removed) and the tests went red.
297d6b6
into
feat/agent-storybook-gallery
| --- | ||
| "@trigger.dev/sdk": patch | ||
| --- | ||
|
|
||
| Watch-mode chat streams now survive quiet windows and keep delivering later turns, and a reply cut off by a lost connection now shows an error instead of appearing finished. |
There was a problem hiding this comment.
🟡 Pull request bundles several unrelated features and fixes instead of one change
The change set mixes many unrelated pieces of work (the chat stream fix in packages/trigger-sdk/src/v3/chat.ts:1804-1852) with a new monthly message allowance, watch plan limits, query read-only enforcement, a query retry cap and an investigation sweep backstop, so the repository's one-change-per-request rule is broken.
Impact: Reviewers cannot evaluate or revert the stated fix independently of several unrelated behaviour changes shipped alongside it.
Which unrelated changes are bundled together
CONTRIBUTING.md states: "We only accept PRs that address a single issue. Please do not submit PRs containing multiple unrelated fixes or features."
The stated intent covers only TRI-13065/TRI-13070 in packages/trigger-sdk/src/v3/chat.ts. The diff additionally contains:
- Dashboard agent monthly message quota:
apps/webapp/app/services/dashboardAgentQuota.server.ts,internal-packages/dashboard-agent-db/drizzle/0004_stale_corsair.sql,apps/webapp/app/components/dashboard-agent/message-quota.ts. - Watch plan limits:
apps/webapp/app/services/dashboardAgentWatchLimits.server.ts,apps/webapp/app/services/dashboardAgentWatches.server.ts:353-374. - Query API read-only pinning and 429 mapping:
apps/webapp/app/services/queryService.server.ts:400,apps/webapp/app/routes/api.v1.query.ts:84-89. - Agent query retry cap:
internal-packages/dashboard-agent/src/tool-api.ts:189-205. - Investigation sweep poison-row handling:
apps/webapp/app/services/dashboardAgentInvestigationSweep.server.ts:116-153,internal-packages/dashboard-agent-db/drizzle/0005_ambitious_mordo.sql.
Prompt for agents
CONTRIBUTING.md requires each pull request to address a single issue. This branch bundles the SDK chat-transport fixes (packages/trigger-sdk/src/v3/chat.ts) with several independent webapp/internal-package features: the dashboard agent monthly message quota (dashboardAgentQuota.server.ts, new agent_message_usage migration, message-quota.ts and the UI wiring), watch plan limits (dashboardAgentWatchLimits.server.ts plus enforcement inside createDashboardAgentWatch), the query API read-only pinning and 429 mapping, the agent run_query consecutive-failure cap, and the investigation sweep poison-row backstop. Split these into separate pull requests so each can be reviewed, released and reverted on its own.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
|
||
| return resolveMessageQuota({ | ||
| isFreePlan, | ||
| used: usedElsewhere === undefined ? undefined : usedElsewhere + countUserMessages(messages), | ||
| }); | ||
| return resolveMessageQuota({ isFreePlan, used }); |
There was a problem hiding this comment.
🔍 Client cap of 20 can disagree with the server's plan limit once billing ships
useAgentMessageQuota calls resolveMessageQuota without a limit, so the browser always compares the server's org-wide used against the hard-coded FREE_PLAN_MESSAGE_LIMIT of 20 (apps/webapp/app/components/dashboard-agent/message-quota.ts:5). The server, meanwhile, refuses against the platform limit agentMessages (apps/webapp/app/services/dashboardAgentQuota.server.ts:15). Today the platform key is absent so the server is effectively unlimited and the 20 is purely a client-side nudge, but as soon as cloud billing sets agentMessages to anything other than 20, free-plan users will see the upgrade block (and be blocked from sending, via the atMessageCap guard in submit) at a count the server would still accept — or vice versa. Worth wiring the limit through the ?quota=1 response alongside used.
Was this helpful? React with 👍 or 👎 to provide feedback.
| // Only a real user message consumes quota; action turns were refused above. | ||
| countsAgainstQuota = agentTurnCountsAgainstQuota(parsed); | ||
| if (countsAgainstQuota) { | ||
| const quota = await resolveAgentMessageQuota(dashboardAgentDb, { | ||
| organizationId: project.organizationId, | ||
| }); | ||
| if (quota?.reached) { | ||
| return json({ error: MESSAGE_QUOTA_REACHED_ERROR, limit: quota.limit }, { status: 403 }); | ||
| } | ||
| } |
There was a problem hiding this comment.
🔍 Quota check and increment are not atomic, so concurrent sends can overshoot the cap
resolveAgentMessageQuota reads the counter and recordAgentMessageSent bumps it in a separate write after the upstream append succeeds, with the mint/forward round trip in between. Several tabs (or the create path racing an in append) can all read used = limit - 1 and all be admitted. Given the code comments describe the cap as a nudge rather than a security boundary this is likely acceptable, but it is worth confirming the product expectation before billing hooks the real limit up.
Was this helpful? React with 👍 or 👎 to provide feedback.
2 participants
Two related fixes to the chat transport's watch/read-only subscription lifecycle.
TRI-13065 — In watch mode the chat stream died at the first long-poll window boundary after a turn completed: the EOF-reconnect path was gated on
isStreaming, which turn-complete clears, so a watcher stopped hearing later turns. Watch mode now keeps reconnecting across quiet windows and only stops on abort or a settled session. The bounded give-up budget still applies to normal (mid-turn) streams, but not to watch mode, where empty windows are expected.TRI-13070 —
reconnectToStreamderived mutation rights from mere signal presence (sendStopOnAbort: !!options.abortSignal), so a passive/read-only subscriber that passed an abortSignal would append a{kind:"stop"}to.inon unmount and could stop a turn it didn't own. Subscription lifecycle is not session ownership:reconnectToStreamnow takes an explicitstopOnAbortoption that defaults tofalse, and the owning turn paths (sendMessages,sendAction) passsendStopOnAbort: trueexplicitly. A read-only subscription ending never mutates the session; it still cancels its own request.