fix: awaited release on connect-inventory failures; kill grace outlasts teardown - #239
Open
MingruiZhang wants to merge 2 commits into
Open
fix: awaited release on connect-inventory failures; kill grace outlasts teardown#239MingruiZhang wants to merge 2 commits into
MingruiZhang wants to merge 2 commits into
Conversation
…ts teardown Post-merge review follow-ups to #238: - A mint whose browser websocket then failed inventory (error or 20s timeout) exited through ?/bail!, leaving the release to the spawned Drop path — and run_connect freeing the connect lock could let a shutdown waiting in disconnect() reach process exit and abort that release. Both inventory error exits now release the owner awaited before surfacing the error. - The daemon's SIGTERM kill grace (6s) was shorter than disconnect()'s own worst-case chain (tab close ≤5s + release ≤5s + connect-settle ≤8s ≈ 18s), so a SIGTERM during a mid-flight connect could SIGKILL the daemon while it was legitimately finishing teardown. Grace is now 20s, derived from those bounds; healthy daemons still exit in under a second via the early-exit poll. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
There was a problem hiding this comment.
Pull request overview
Ensures remote browser sessions are released during connection failures and gives daemon teardown sufficient time to complete.
Changes:
- Awaits session release after inventory errors/timeouts.
- Extends daemon kill grace from 6 to 20 seconds.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
core/src/cdp/lifecycle.rs |
Adds awaited cleanup for failed connection inventory. |
cli/src/daemon.rs |
Extends graceful daemon termination time. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
release_owner_now took the session id before awaiting the HTTP call, so a connect-budget expiry cancelling it mid-release dropped a disarmed RemoteSession — losing even the Drop-spawned backstop. The id is now cloned for the request and taken only after the await completes, so cancellation falls back to Drop (and process-exit paths degrade to the documented server-timeout backstop instead of a silently lost release). Also documents the two accepted settle-window residuals at the disconnect() connect-lock wait: closing them would need release ownership to outlive the connect task, machinery disproportionate to a seconds-wide window backstopped by the server-side session timeout. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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.
Follow-up to #238, addressing the two findings Copilot raised in its post-merge review pass (review 4823774609 — no threads existed to resolve since the PR was already merged):
?/bail!, leaving the session release to the fire-and-forgetDrop. Sincerun_connectthen frees the connect lock, a shutdown blocked indisconnect()'s connect-settle wait could proceed to process exit and abort that spawned release. Both exits nowrelease_owner_now(owner).awaitbefore surfacing the error, preserving the invariant that a free connect lock means no release from that attempt is still in flight. (The one remaining spawn-only path is budget-exhaustion cancellation, where the attempt future is dropped mid-await — a cancelled future cannot await; the server-side timeout remains that path's backstop.)KILL_GRACE(6s) was shorter than the teardown it protects:disconnect()'s worst-case chain is tab close (≤5s, local only) + awaited release (≤5s) + connect-settle (≤8s) ≈ 18s. A SIGTERM landing during a mid-flight connect could SIGKILL the daemon while it was legitimately finishing the release. Grace is now 20s, derived from those bounds; the early-exit poll means healthy daemons still terminate in well under a second.Verified: core+cli compile, all test suites pass. Not in v0.4.18 (built before this); rides the next release.
🤖 Generated with Claude Code