fix: stop retrying silent chat streams indefinitely - #4949
Conversation
🦋 Changeset detectedLatest commit: 7825d1b 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 |
WalkthroughCore SSE streams now track stall retries separately from connection retries. Retry state resets only after decoded records. Internal abort exhaustion reports Priority: ⬇️ Low Merge Risk: 🔵 Low · up to A replacement stream can remain healthy while callers receive a spurious stream-error event from the old stream. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 4 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches🧪 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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
gtremper
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟡 Minor · Suppress stale stream-error events. · chat.ts:2442-2465
packages/trigger-sdk/src/v3/chat.ts:2442-2465
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSuppress stale
stream-errorevents. The late token-refresh path can reject aftersendMessagesreplaces the old stream. The old stream then enters this non-abort handler whileactiveStreamspoints to the replacement. The current guard protects session state only; unconditionalemitEventnotifiesonEventof a false error for the active chat.Move the
stream-erroremission inside the existing ownership guard. Keepcontroller.error(error)outside the guard so the superseded reader still settles with its own error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: 29592fdc-bd49-47b9-85dc-50638f858213
📒 Files selected for processing (3)
.changeset/quiet-chat-stream-retries.mdpackages/core/src/v3/apiClient/runStream-retries.test.tspackages/core/src/v3/apiClient/runStream.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- .changeset/quiet-chat-stream-retries.md
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.
📜 Review details
⏰ Context from checks skipped due to timeout. (17)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (17, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (19, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (18, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (11, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (21, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (20, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (16, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (15, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (12, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (13, 24)
- GitHub Check: e2e / 🧪 CLI v3 tests (warp-windows-latest-x64-8x - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (warp-windows-latest-x64-8x - pnpm)
- GitHub Check: e2e-webapp / 🧪 E2E Tests: Webapp (1, 2)
- GitHub Check: internal / 🧪 Unit Tests: Internal (2)
- GitHub Check: internal / 🧪 Unit Tests: Internal (1)
- GitHub Check: packages / 🧪 Unit Tests: Packages (3, 3)
- GitHub Check: Analyze (javascript-typescript)
🧰 Additional context used
📓 Path-based instructions (7)
**Public packages** (`packages/*`): Use `build`.
📄 CodeRabbit inference engine (AGENTS.md)
Files:
packages/core/src/v3/apiClient/runStream-retries.test.tspackages/core/src/v3/apiClient/runStream.ts
Use zod for validation in packages/core and apps/webapp
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
packages/core/src/v3/apiClient/runStream-retries.test.tspackages/core/src/v3/apiClient/runStream.ts
Never import the root package (`@trigger.dev/core`).
📄 CodeRabbit inference engine (packages/core/CLAUDE.md)
Files:
packages/core/src/v3/apiClient/runStream-retries.test.tspackages/core/src/v3/apiClient/runStream.ts
Use vitest for all tests in the Trigger.dev repository
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
packages/core/src/v3/apiClient/runStream-retries.test.ts
Use function declarations instead of default exports
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
packages/core/src/v3/apiClient/runStream-retries.test.tspackages/core/src/v3/apiClient/runStream.ts
Use types over interfaces for TypeScript Avoid using enums; prefer string unions or const objects instead
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
packages/core/src/v3/apiClient/runStream-retries.test.tspackages/core/src/v3/apiClient/runStream.ts
When creating or editing OTEL metrics (counters, histograms, gauges), ensure metric attributes have low cardinality by using only enums, booleans, bounded error codes, or bounded shard IDs Do not use high-cardinality attributes in OTEL metr...
📄 CodeRabbit inference engine (.cursor/rules/otel-metrics.mdc)
Files:
packages/core/src/v3/apiClient/runStream-retries.test.tspackages/core/src/v3/apiClient/runStream.ts
🔇 Additional comments (2)
packages/core/src/v3/apiClient/runStream.ts (1)
224-224: LGTM!Also applies to: 300-300, 658-663
packages/core/src/v3/apiClient/runStream-retries.test.ts (1)
86-86: LGTM!Also applies to: 121-121, 134-134, 185-185
2 participants
When the server returns
200without records, chat subscriptions can retry indefinitely. The client stall timer fires after 60 seconds, before S2 closes the response at its 120-second timeout, so the existing EOF budget never applies. Terminal failures also leave persistedisStreamingstate active, so a reload starts another subscription.Normal chat subscriptions now permit five reconnects after stall timeouts. A separate stall counter preserves unlimited retries for retryable connection failures, fetch timeouts, and browser wakeups. Decoded records restore the stall budget. Terminal failures clear state only for the owning subscription, and watch subscriptions remain unlimited.
The existing stall timer already ignores keepalives because the parser drops them before the timer reset. The comment correction does not change that behavior.
Checklist
Testing
8149b96: 695 SDK tests and 1,122 core tests passed.apps/webapp/app/services/previewAutoArchive.server.ts; the changed files contain none.The tests use local HTTP servers. They cover separate stall limits, fetch timeouts, body failures, keepalives, progress resets, mixed failures, wakeups, cancellation, watch recovery, and replacement ownership. Core tests exercise stall exhaustion with short timers. The SDK's six-minute silence window needs reference-project validation.
Changelog
Silent chat subscriptions now report
Stream stalled: no records receivedafter five stall retries. Network failures retain automatic recovery, and watch subscriptions remain unlimited.Risk
onlineeventWith immediate response headers, six silent attempts take 368.5–377 seconds, about 6.1–6.3 minutes. Network delays extend this window. A healthy tool call with no records can also reach this limit: silence does not prove that the run is dead. The terminal error stops automatic client resumption but does not cancel the server-side run.
Shared core consumers retain their configured retry limits. Repeated successful responses without records now increase backoff until a decoded record arrives. Caller cancellation and token-refresh limits remain unchanged.