fix(dashboard): stop a stale usage stream from reopening itself - #651
fix(dashboard): stop a stale usage stream from reopening itself#651SantiagoDePolonia wants to merge 2 commits into
Conversation
#readStream scheduled a reconnect whenever its reader finished, without checking whether its controller was still the active one. Leaving the Overview page and returning replaces the controller, but the old reader can still complete normally afterwards and schedule a reconnect against the current state, opening a second usage stream alongside the live one. Guard the completion path with the controller identity/abort check the audit-log stream already uses (liveLogs.svelte.js), so only the active stream can reconnect. Reported by Greptile on #650; the flaw predates that PR. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe live usage SSE stream now skips reconnect scheduling when its controller was aborted or replaced. The error path also ignores aborted reads reported as ChangesLive usage SSE lifecycle
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web/dashboard/src/pages/overview/liveTokensState.svelte.js`:
- Around line 168-174: Update `#scheduleReconnect`() and every call site in the
stream reader to accept and propagate the reader controller, including
invalid-response and catch paths. Before scheduling, require the stream to
remain active, un-aborted, and identical to the current `#sseController`; preserve
the existing final-branch guard behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d6debd08-82d3-4cd3-8c01-62f166927046
⛔ Files ignored due to path filters (2)
internal/admin/dashboard/static/dist/assets/index-DTrQJRSk.jsis excluded by!**/dist/**internal/admin/dashboard/static/dist/index.htmlis excluded by!**/dist/**
📒 Files selected for processing (1)
web/dashboard/src/pages/overview/liveTokensState.svelte.js
| // A reader that ends normally after this stream was replaced (navigate | ||
| // away and back) must not resurrect it — reconnecting here would open a | ||
| // second stream alongside the live one. Same guard as the audit-log | ||
| // stream in liveLogs.svelte.js. | ||
| if (controller.signal.aborted || this.#sseController !== controller) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 8 \
'`#scheduleReconnect`|isAbortError|signal\.aborted|`#sseController`' \
web/dashboard/src/pages/overview/liveTokensState.svelte.js \
web/dashboard/src/lib/api/client.js \
web/dashboard/src/pages/audit-logs/liveLogs.svelte.jsRepository: ENTERPILOT/GoModel
Length of output: 17927
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== liveLogs.svelte.js reconnect implementation =="
sed -n '198,230p' web/dashboard/src/pages/audit-logs/liveLogs.svelte.js | cat -n
echo
echo "== eventStream.js =="
sed -n '1,220p' web/dashboard/src/lib/api/eventStream.js | cat -n
echo
echo "== all scheduleReconnect/reconnect call sites in overview page =="
rg -n -C 4 '\bschedule(UsageSignal|LiveLogs|Reconnect)|`#scheduleReconnect`|liveLogsController|`#sseController`|consumeEventStream|start\(' web/dashboard/src/pages/overview/liveTokensState.svelte.js web/dashboard/src/pages/audit-logs/liveLogs.svelte.jsRepository: ENTERPILOT/GoModel
Length of output: 22276
Guard #scheduleReconnect() against stale controllers.
Replace only the final reconnect branch, but the invalid-response branch and catch block also call #scheduleReconnect(). Those calls do not pass the reader controller, so #scheduleReconnect() only checks active and can restart an obsolete stream when start() has replaced #sseController. Propagate controller to every reconnect path, then keep #scheduleReconnect(controller) from scheduling a reconnect while the current sseController is different or aborted.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/dashboard/src/pages/overview/liveTokensState.svelte.js` around lines 168
- 174, Update `#scheduleReconnect`() and every call site in the stream reader to
accept and propagate the reader controller, including invalid-response and catch
paths. Before scheduling, require the stream to remain active, un-aborted, and
identical to the current `#sseController`; preserve the existing final-branch
guard behavior.
Confidence Score: 4/5
 T-Rex LogsWhat T-Rex did
Comments Outside Diff (1)
Reviews (1): Last reviewed commit: "fix(dashboard): stop a stale usage strea..." | Re-trigger Greptile |
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
The first pass only guarded the normal-completion path. The invalid-response branch and the catch block also schedule reconnects, and both are reachable by a reader whose stream was already replaced — the fetch can resolve before the abort lands. Route all three through one controller-identity check. Firefox also rejects a deliberately-aborted read with a plain TypeError instead of an AbortError, so the catch now trusts the controller state as well as the error name, matching liveLogs.svelte.js. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Follow-up to #650, where Greptile flagged this (P1). The flaw predates that PR — the refactor moved the line, it did not introduce the bug.
The bug
liveTokensState.#readStreamscheduled a reconnect whenever its reader finished, without checking whether its controller was still the active one:#startUsageSignalStreamaborts and replaces#sseControlleron every start, so leaving the Overview page and coming back creates a new stream while an old reader may still be pending. If that old reader then completes normally (rather than rejecting with an abort), it schedules a reconnect against the current state and opens a second usage stream alongside the live one. An abort-rejected read is already handled by theisAbortErrorcatch; this is the completion path it doesn't cover.The fix
Guard the completion path with the same controller identity/abort check the audit-log stream has carried all along (
liveLogs.svelte.js:178-183), so only the active stream can reconnect. The two streams now agree.Verification
svelte-check0 errors, 444/444 dashboard tests, clean build.Honest limitation: no regression test.
liveTokensStateis a rune-based singleton importing through the$libalias, so the existingnode --testsuite cannot drive it, and the pure-logic split for this page (liveTokensLogic.js) does not cover stream control. The change was verified by reading against the twin implementation it mirrors, plus the reproduction Greptile reported on #650. Making the stream layer testable would mean extracting its control flow the waylive-logs-logic.jswas — worth doing, but larger than this fix.🤖 Generated with Claude Code
Summary by CodeRabbit