fix: close remaining audit lifecycle gaps - #705
Conversation
|
Warning Review limit reached
Next review available in: 59 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughChangesCancellation-aware cleanup
WebTransport lifecycle
Multi-context WebSocket lifecycle
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ContextSend
participant MultiContextWebSocket
participant WebSocket
participant CloseTransaction
ContextSend->>MultiContextWebSocket: request send
MultiContextWebSocket->>MultiContextWebSocket: acquire send lock and validate admission
MultiContextWebSocket->>WebSocket: write frames
CloseTransaction->>MultiContextWebSocket: acquire close locks
CloseTransaction->>WebSocket: send bounded close frame
CloseTransaction->>WebSocket: close captured socket
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4355d3779a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
# Conflicts: # src/easycat/transports/webtransport.py # src/easycat/tts/_multi_context_ws.py # tests/transports/test_webtransport_server_protocol.py
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@src/easycat/tts/_multi_context_ws.py`:
- Around line 635-644: In _close_socket_only, keep incrementing
_fallback_close_waiters before awaiting _connect_lock, but move the _closing =
True assignment inside that lock and after the self._closed early return.
Preserve the existing lock ordering and permanent-close guard so unexpected
socket failures during the lock wait still reach _finalize_reader as errors.
- Around line 411-414: Move the snapshot-related comment to the `ws` capture
around the existing snapshot assignment, documenting that it occurs before
cancelling the reader. Replace the current comment above
`_cancel_background_tasks()` with a concise note that the reader’s `finally`
block nulls `self._ws`, preserving the existing cancellation and cleanup
behavior.
In `@tests/tts/test_multi_context_ws.py`:
- Around line 699-702: Bound the teardown awaits in this test with short
asyncio.wait_for timeouts, including the await sending and await closing joins
shown here and the corresponding teardown join around line 734. Preserve the
existing RuntimeError assertion while ensuring a wedged send or close fails fast
instead of hanging indefinitely.
🪄 Autofix (Beta)
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: d7b2fea8-d9e6-4305-ac62-888b814e5d29
📒 Files selected for processing (11)
src/easycat/runtime/scope.pysrc/easycat/server/transports.pysrc/easycat/session_manager.pysrc/easycat/transports/webtransport.pysrc/easycat/tts/_multi_context_ws.pytests/runtime/test_scope.pytests/server/test_capacity_gate_drain.pytests/session/test_session_manager.pytests/transports/test_webtransport_connection_transport.pytests/transports/test_webtransport_server_protocol.pytests/tts/test_multi_context_ws.py
Stack
This is a follow-up to #704 and intentionally targets
agent/fix-subagent-audit-bugs. Merge #704 first, then retarget this PR tomainif GitHub does not do so automatically.Problem
The final post-#704 audit found a small set of remaining lifecycle gaps:
Changes
RuntimeScope.drain, server_safe_await, and session-manager owned-stop joinsdebug/bundle.pyRuff complexity exemptionImpact
These fixes prevent false caller cancellation, replay state being armed after a context has closed, leaked WebTransport sessions after teardown failure, and invalid raw-audio configurations reaching runtime.
Validation
7341 passed, 306 skipped340 passed, 4 skipped97 passed, 15 skipped90 passed278 source filescleangit diff --check: cleanSummary by CodeRabbit