Repository navigation
fix(python): drain requests after repeated cancellation - #25411
BlueRaddish wants to merge 8 commits into
Conversation
AI-assisted with OpenAI Codex.
AI-assisted with OpenAI Codex.
Bugbot needs on-demand usage enabledBugbot uses usage-based billing for this team and requires on-demand usage to be enabled. A team admin can enable on-demand usage in the Cursor dashboard. |
|
No actionable comments were generated in the recent review.
|
| Layer / File(s) | Summary |
|---|---|
Drain cancelled control requests cmux-tui/bindings/python/cmux/aio.py, cmux-tui/bindings/python/tests/test_resource_api.py |
Client._run_control uses _await_cleanup to drain the worker future after cancellation. The regression test checks repeated cancellation, cancel acknowledgment, and a subsequent successful ping. |
Priority:
Estimated code review effort: 2 (Simple) | ~10 minutes
Change: Bug fix
Suggested reviewers: lawrencecchen
Merge Risk: 
Minimal · up to 2b1fa
-
The repeated-cancellation path has a regression test, and no remaining issue was identified that should block merging.
Pre-merge checks: 
24
-
Full details: Title check
-
Explanation
The title "fix(python): drain requests after repeated cancellation" names the main change: the Python SDK async client now waits for its request worker to drain when the caller cancels repeatedly. It matches the raw summary (_run_control drains its executor future via _await_cleanup) and the objectives. It is specific and uses a conventional-commit prefix. Minor note: "drain requests" is slightly less precise than "drain request worker", but the title is still clearly related to the change.
Full details: Cmux Cloud Persistent Session And Early Input
-
Explanation
PASS: The diff changes only Python request cancellation cleanup and adds a local Unix-socket regression test. It does not change Cloud terminal creation, persistent transport, renderer admission, input routing, auth, idempotency, revision fences, or leases. No failure condition in the rule is introduced.
Full details: Cmux Swift Actor Isolation
-
Explanation
The pull request changes only two Python files:
cmux-tui/bindings/python/cmux/aio.pyandcmux-tui/bindings/python/tests/test_resource_api.py. The reviewed diff contains no Swift files, so it does not introduce or worsen Swift actor isolation issues.
Full details: Cmux Swift Blocking Runtime
-
Explanation
PASS: The pull request changes only Python files:
cmux-tui/bindings/python/cmux/aio.pyandcmux-tui/bindings/python/tests/test_resource_api.py. It introduces no production Swift changes. The test-onlyasyncio.sleep(0)is deterministic test scaffolding and is explicitly allowed by the check.
Full details: Cmux Browser Automation Off-Main
-
Explanation
PASS: The pull request changes only Python async request cleanup and its regression test. The diff introduces no browser socket commands, WebKit/AppKit access, main-actor routing, worker-lane routing, or browser policy coverage concerns. The browser automation rule is therefore not applicable.
Full details: Cmux Cache Substitution Correctness
-
Explanation
PASS: The reviewed diff changes only Python files (
cmux-tui/bindings/python/cmux/aio.pyand its Python test). The cache-substitution condition applies only to production Swift, TypeScript, and JavaScript changes, so it is not applicable.
Full details: Cmux No Hacky Sleeps
-
Explanation
PASS. The pull request changes Python runtime code and Python tests only. The production change replaces
asyncio.shield(future)with the existing_await_cleanup(future)and introduces no sleep, timer, polling loop, or wall-clock wait. The only newasyncio.sleep(0)is deterministic test-only scaffolding, which the rule allows.
Full details: Cmux Algorithmic Complexity
-
Explanation
PASS. The PR changes only Python files. The production change replaces one await with
_await_cleanupand does not add collection scans, sorting, filtering, joins, or a slower algorithm. The added collection of test code is test-only scaffolding, which the rule explicitly excludes.
Full details: Cmux Swift Concurrency
-
Explanation
The pull request changes only Python files:
cmux-tui/bindings/python/cmux/aio.pyandcmux-tui/bindings/python/tests/test_resource_api.py. The diff contains no Swift code, so it does not introduce or expand any listed Swift concurrency pattern.
Full details: Cmux Swift `@Concurrent`
-
Explanation
PASS: The pull request changes only Python files (
cmux-tui/bindings/python/cmux/aio.pyandcmux-tui/bindings/python/tests/test_resource_api.py). The authoritative diff contains no Swift changes, so the@concurrentrule does not apply.
Full details: Cmux Swift Package Boundaries
-
Explanation
PASS: The reviewed diff changes only
cmux-tui/bindings/python/cmux/aio.pyandcmux-tui/bindings/python/tests/test_resource_api.py. It contains no Swift, SwiftPM, or app-target changes, so the Swift package boundary rule is not applicable.
Full details: Cmux Swiftpm Lockfiles
-
Explanation
PASS. The reviewed diff changes only
cmux-tui/bindings/python/cmux/aio.pyandcmux-tui/bindings/python/tests/test_resource_api.py. It contains no SwiftPM package, Xcode project,.gitignore, workflow, dependency, orPackage.resolvedchanges, so the SwiftPM lockfile conditions do not apply.
Full details: Cmux Swift Logging
-
Explanation
The pull request changes only Python SDK code and a Python regression test:
cmux-tui/bindings/python/cmux/aio.pyandcmux-tui/bindings/python/tests/test_resource_api.py. The reviewed diff contains no Swift files and adds or changes no logging.
Full details: Cmux User-Facing Error Privacy
-
Explanation
PASS. The production diff only replaces a shielded await with the existing
_await_cleanuphelper incmux-tui/bindings/python/cmux/aio.py; it adds no user-facing error, alert, command output, API error body, or recovery text. The added assertion text is test-only, which the rule allows. No changed text exposes implementation details or sensitive data.
Full details: Cmux Full Internationalization
-
Explanation
The pull request changes only Python async cleanup logic and a regression test. It introduces no Swift UI text, app catalog or Info.plist entries, web UI/API copy, metadata, markdown, changelog, or locale-dependent user-facing data. The added strings are test assertion/fixture text, which the rule allows.
Full details: Cmux Swiftui State Layout
-
Explanation
PASS. The pull request changes only Python SDK code and Python tests. It contains no SwiftUI, ObservableObject,
@Published, GeometryReader, list-row store, or render-time state changes. The SwiftUI state-layout check is not applicable.
Full details: Cmux Swift Auxiliary Window Close Shortcuts
-
Explanation
PASS: The pull request changes only Python files:
cmux-tui/bindings/python/cmux/aio.pyandcmux-tui/bindings/python/tests/test_resource_api.py. It adds no Swift code and does not add or change any cmux-owned window, window identifier, or close-shortcut ownership.
Full details: Cmux Source Artifacts
-
Explanation
The PR changes only
cmux-tui/bindings/python/cmux/aio.pyandcmux-tui/bindings/python/tests/test_resource_api.py. These are hand-written source and regression-test files. The diff adds no logs, screenshots, recordings, temporary directories, caches, build output, dependency checkouts, or other source-control artifacts.
Full details: Cmux No Test Or Debug Seam In Production Source
-
Explanation
The pull request changes only two Python files:
cmux-tui/bindings/python/cmux/aio.pyandcmux-tui/bindings/python/tests/test_resource_api.py. It changes no Swift file under a productionSources/path, so this check is not applicable.
Full details: Description check
-
Explanation
The description clearly explains the problem, the fix, the regression coverage, test results, limitations, and changelog entry. It omits the required Summary heading, Proof section, and Checklist, although the opening paragraphs provide the summary and the testing section documents the non-user-visible scope and verification.
-
Finishing Touches: 💡 1
-
🛠️ Fix failing CI checks: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 help to get the list of available commands.
|
All contributors have signed the CLA ✍️ ✅ |
AI-assisted with OpenAI Codex.
AI-assisted with OpenAI Codex.
|
Update from the quality audit: #25411 is the active PR. Maintainer edits to the contributor fork succeeded with a plain push; my earlier claim that the fork could not be updated was incorrect. Duplicate #25747 is closed. See the audit update for the rebase and passing 119-test Python suite. |
AI-assisted with OpenAI Codex.
AI-assisted with OpenAI Codex.
…thon-cancel-drain
|
The quality audit corrected my earlier claim about fork access: maintainer edits are enabled, and a plain push to The branch is rebased onto The full Python suite now passes all 119 tests with zero skips, including the packaging test, in an isolated Python 3.13.3 environment with setuptools 84.2.1. The earlier local packaging error was an unmet build prerequisite; no test, assertion, timeout, or retry policy was changed. Fresh CI is running on Regarding CodeRabbit's docstring-coverage advisory: this change adds no public API. The reused |
|
@coderabbitai full review The incremental review reported that it could not recover its checkpoint after the rebase. Please review current head The Cursor Bugbot comment reports disabled on-demand billing, rather than a code finding. That account setting is outside this PR; I am preserving the existing checks and obtaining an independent structured code review alongside CodeRabbit. The docstring-coverage advisory is addressed in the audit update above. |
|
|
@coderabbitai full review The previous full-review refusal came from GitHub retaining base SHA |
|
Ready for review: root cause a second caller cancellation interrupts the single shielded cleanup wait, letting the async request return while its executor worker is still draining; fix reuse Final audit at
The final pass found nothing left to repair. The original contributor's fix and authorship are preserved. |
Calling
cancel()again while an async Python SDK request is still acknowledging its first cancellation can let the caller finish before its request worker has drained. Reuse the existing_await_cleanuphelper so repeated cancellation still waits for that worker, then raisesCancelledError. The regression usesclient.session(...).terminal(...).wait()over a real local Unix socket, holds the cancellation acknowledgment, and confirms the same client can still ping afterward.Testing
The focused regression fails on the rebased test-only commit
dcfb31e186c700effcef0d73872adaccbc255d6awithrequest worker was not drained, and passes on head01c92e9330400f97f7b168ce07798ae10a3b80b5. Fromcmux-tui/bindings/python/tests, the same command executes one test with no skips:Observed / Not run
setuptools>=77). Command:PYTHONPATH=cmux-tui/bindings/python python3 -m unittest discover -v -s cmux-tui/bindings/python/tests.mainat36540f6194b746d48d47aa8963adbd0f1cd35cdbwithout conflicts. The published ancestry is retained for a plain push.git diff --checkpasses; the diff remains the two Python files.Changelog
Fixed: Python async request cancellation waits for cleanup even when the caller is cancelled repeatedly.
AI-assisted with OpenAI Codex.
Need help on this PR? Tag
@codesmith-botwith what you need.Summary by CodeRabbit