fix(approval): RAII guard so a parked waiter is cleaned up on external turn teardown#4819
Conversation
…l turn teardown ApprovalGate::intercept_audited_inner registers an in-memory waiter and persists a pending_approvals row, then parks on tokio::time::timeout. All cleanup (evict_waiter / store::decide(Deny) / thread+meeting routing-map removal) lived only inside the timeout match arms, so it ran only on a normal park resolution. Since tinyhumansai#4751 (the tinyhumansai#4746 wall-clock backstop), a turn future can be torn down externally while a tool call is parked. Dropping the future skips the match arms, leaking the waiter, the routing mappings, and the pending row until the store TTL sweeps them — a later yes/no then routes to a dead request and returns without starting a fresh turn. Add a WaiterGuard RAII guard created just before the park await: on Drop (external cancellation) it evicts the waiter, clears routing, and denies the still-open row (store::decide is WHERE decided_at IS NULL, so a same-instant real decision is honored). Disarmed on every normal exit so the match arms stay authoritative. Regression test tears the parked future down mid-park and asserts waiter eviction + routing clear + row denied. Closes tinyhumansai#4774
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 32 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: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8ca1d875e5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…tore vendor pins On external turn teardown the WaiterGuard drop cleared the thread/meeting routing maps unconditionally. If a replacement turn had already parked a new approval on the same thread and overwritten the entry, this deleted the *new* request's routing, so the next typed yes/no fell through as a fresh chat turn instead of resolving the live gate. Only remove the mapping when it still points at this guard's request_id (both thread and meeting maps). Also restores vendor/tinyagents and vendor/tinycortex to the main pins; the branch had picked up stray submodule bumps that broke the build (tinycortex 33dda94 lacks the git-diff feature openhuman requires).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
W4 update — pushed
All checks green; review thread resolved. |
oxoxDev
left a comment
oxoxDev
left a comment
There was a problem hiding this comment.
Review ✅ — approve
Correct and well-reasoned. WaiterGuard is armed just before the timeout(wait, rx).await park and disarm()ed right after the match block, so on external future-drop (the #4751 backstop tearing down a turn mid-park) its sync Drop runs the exact teardown the match arms own: evict_waiter, request-id-scoped routing clear, store::decide(Deny).
Verified
- No double / missed cleanup: normal exit → match arm cleans up →
disarm()→Dropearly-returns; external drop → guard runs full cleanup.evict_waiter/decideare keyed on the guard's ownrequest_id, so even the unconditionaldecide(Deny)in Drop only touches this request's row. - Sync-safe Drop:
store::decideis synchronous rusqlite, routing usesparking_lot::Mutex— no.await/block_on/spawnin Drop, can't panic in async context. - Idempotent:
decideisUPDATE … WHERE decided_at IS NULL, so a same-instant Approve wins and the guard's Deny is a no-op; the late approve RPC short-circuits onOk(None). - No armed-window gap: only
.awaitbetween waiter/row registration and the park is the timeout itself, so a drop can't land while registered-but-unarmed. Real external-drop test (Box::pin→ poll to park →drop()→ assert evicted + routing cleared + row Denied).
Non-blocking
- [low] Drop test only exercises the chat/thread path; the
meeting_keybranch is helper-covered but not through an actual mid-park future drop — add anin_call_ctxvariant. - [low] Pre-existing (not this diff):
insert_pendingfailure path clears thread route but not meeting route →meeting_to_requestleak on persist failure with an active call. Optional follow-up.
Codex's routing-scoping P2 fixed in PATCH 3; CodeRabbit APPROVED, no open threads.
3 participants
Problem
ApprovalGate::intercept_audited_inner(src/openhuman/approval/gate.rs) registers an in-memory waiter and persists apending_approvalsrow, then parks ontokio::time::timeout(wait, rx). All cleanup —evict_waiter/store::decide(Deny)/ thread+meeting routing-map removal — lived only inside the timeout match arms, so it ran only when the park resolved normally.Since #4751 (the #4746 harness
max_wall_clock_msbackstop, and the outer 900s web backstop), a turn future can now be torn down externally while a tool call is parked in the gate. Dropping the future skips the match arms entirely, leaking:thread_to_request/meeting_to_requestrouting mappings,pending_approvalsrow (nostore::decide(Deny)),until the store TTL sweeps them. A later "yes"/"no" arriving before that expiry is then routed to a dead approval request and returns without starting a fresh chat turn.
Carved out of #4751 per the Codex reviewer (P2), confirmed by @YellowSnnowmann — deferred because it needs a ≥600s user approval wait and the turn already ends
turn_timeout(retryable).Fix
Add a
WaiterGuardRAII guard, created just before the parkawait. OnDrop(i.e. external cancellation, when the match arms never run) it:store::decideisWHERE decided_at IS NULL, so a decision that committed in the same instant is honored rather than overwritten.The guard is
disarm()ed on every normal exit — the timeout match arm already ran the exact teardown for its outcome, so the guard'sDropis reserved solely for external teardown.Test
waiter_future_dropped_mid_park_evicts_waiter_clears_routing_and_denies_rowboxes the parked intercept future, polls it just long enough to park (asserts the waiter is registered and the row is open), then drops it mid-park and asserts the waiter is evicted, routing cleared, and the pending row denied.Checks
cargo fmt— clean.Closes #4774