fix(web): read a done pull future on every WS pump exit - #40
Merged
Merged
Conversation
Reproduces the live line from 2026-09-23 startup: "Task exception was never retrieved ... StopAsyncIteration". The WS pump pulls each transport event as its own future; when a newer socket takes the consumer slot at the moment the stream drains, the pump raises _SupersededError before reading the drained future and its finally leaves a done future neither cancelled nor read, so asyncio logs the exception from the finalizer. The test drives the shared-portal TestClient, sets the takeover flag and drains the gated stub in one loop turn, pins the supersede poll long so the pump wakes only because the pull completed, then forces gc and asserts no "never retrieved" record on the asyncio logger.
The WS pump pulls each transport event as its own future. Two exits left a done future unread: a takeover (_SupersededError raised before the read) landing beside the drain, and a client close cancelling the pump after the pull had completed. The finally only cancelled a pending future, so a done one kept its StopAsyncIteration and asyncio logged "Task exception was never retrieved" from the finalizer -- the line seen at web startup on 2026-09-23 when a PWA tab reattached to a dead session. The finally now reads the outcome of a done, non-cancelled future and cancels only a pending one. A drained stream ending there is expected, not an error, so nothing is logged. RED 8bcf7d9 flips; WS, grace, slot-reconcile and live-session suites 67/67.
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The fix is covered by regression tests and no unresolved issues were identified.
Review effort: Lite
Findings: None
What changed in this PR
Fixes unretrieved StopAsyncIteration warnings when the WebSocket pump exits with a completed pull future.
Changes:
- Consume completed pull outcomes and cancel only pending futures.
- Add regression coverage for takeover during stream drain.
| File | Description |
|---|---|
packages/studyloop/tests/test_web_session_ws.py |
Adds regression coverage for the logged exception. |
packages/studyloop/src/studyloop/web/routes/session/_ws.py |
Safely handles pull futures during teardown. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Fixes the line seen at
studyloop web --lanstartup on 2026-09-23:The
/api/session/wspump (web/routes/session/_ws.py) pulls each transport event as its own future. Two exits left a done future unread: a takeover (_SupersededErroris raised before the read) landing beside the stream's drain, and a client close cancelling the pump after the pull had already completed. Thefinallyonly cancelled a pending future, so a done one kept itsStopAsyncIterationand asyncio logged it from the finalizer. That is exactly a PWA tab reattaching to a session whose transport had already ended — which theStale Kiro backup detectedline on the same startup says had happened.Change
One guard in the pump's
finally: read the outcome of a done, non-cancelled future; cancel only a pending one. A drained stream ending there is expected, not an error, so nothing is logged.Evidence
8bcf7d91—TestDrainedPullFuturedrives the shared-portalTestClient, sets the takeover flag and drains a gated stub in one loop turn (the supersede poll pinned long so the pump wakes only because the pull completed), forcesgc.collect(), and asserts nonever retrievedrecord on theasynciologger. Onmainit captures the live line byte-for-byte.b33ac049— the RED flips;test_web_session_ws,test_session_ws_grace,test_session_slot_reconcile,test_web_live_session67/67; ruff, format, pyright clean on the touched files.Not a Kiro-specific fix and not related to the stale-backup restore, which is the launcher's correct recovery from an unclean previous exit.