fix(approval): clean up waiters dropped by turn timeouts#4786
Conversation
📝 WalkthroughWalkthrough
ChangesParked approval cleanup
Estimated code review effort: 4 (Complex) | ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
![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: dbcc54bc9a
ℹ️ 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".
| self.gate.clear_thread(&self.thread_id); | ||
| self.gate.clear_meeting(&self.in_call_ctx); |
There was a problem hiding this comment.
When an old parked approval is cancelled because a newer turn starts, the old task is only signalled and can be dropped after the new turn has already parked its own approval on the same thread/meeting. These key-only clears then remove whatever request is currently stored for that thread/meeting, so a fresh approval's yes/no reply no longer routes through pending_for_thread/pending_for_meeting and the new request can hang until TTL. Please remove the mapping only if it still points at this guard's request_id.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/openhuman/approval/gate.rs (2)
1400-1479: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSolid regression coverage for the external-abort cleanup path.
Both tests correctly validate waiter eviction, routing-map cleanup, and durable Deny persistence for chat- and meeting-routed parks. One gap worth considering: there's no test exercising the race this design is built to survive —
decide()committing anApprove*decision concurrently with the abort, verifying the guard'sstore::decide(Deny)becomes a no-op (per theWHERE decided_at IS NULLconditional) rather than clobbering the already-persisted decision. That's the core correctness guarantee cited in the PR objectives ("conditional updates preserve concurrently committed decisions") and isn't directly covered here.Want me to draft a test that calls
gate.decide(&request_id, ApprovalDecision::ApproveOnce)immediately beforehandle.abort()and asserts the persisted decision staysApproveOnce?🤖 Prompt for 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. In `@src/openhuman/approval/gate.rs` around lines 1400 - 1479, Add regression coverage for the abort/decision race in the existing external-abort waiter tests: commit an approval via gate.decide using the captured request_id immediately before aborting the spawned waiter, then assert cleanup still occurs and store::get_decision remains ApprovalDecision::ApproveOnce. Ensure the test exercises the conditional Deny cleanup path so it cannot overwrite an already-persisted approval.
206-271: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winBlocking SQLite write inside
Drop::dropcan stall the runtime thread performing the cancellation.
Drop::drop(lines 237-270) callsstore::decide(&self.gate.config, &self.request_id, ApprovalDecision::Deny)synchronously (line 254).Dropcannot yield, so whichever thread runs this destructor — plausibly a Tokio worker thread reclaiming the task duringabort()or an outer-deadline drop — is blocked for the duration of the SQLite write/lock acquisition. Tokio's own docs call this out explicitly: "if you call a non-async method from async code, that non-async method is still inside the asynchronous context, so you should also avoid blocking operations there. This includes destructors of objects destroyed in async code."This mirrors the existing synchronous
store::decidecalls at lines 842/862, but doing it inDropis strictly worse: there's no way to bound it with a timeout or defer it, and it fires precisely during the failure scenario this PR targets (turn-deadline cancellation), which is exactly when many tasks might be torn down in a burst.Consider offloading via
tokio::task::spawn_blocking(fire-and-forget, logging errors from the spawned task) so the drop path never blocks its calling thread — or explicitly document that local SQLite writes are treated as acceptable "fast enough" blocking work here.♻️ Possible direction (fire-and-forget via spawn_blocking)
match store::decide(&self.gate.config, &self.request_id, ApprovalDecision::Deny) { - Ok(Some(_)) => tracing::debug!(...), - Ok(None) => tracing::debug!(...), - Err(err) => tracing::error!(...), - } + } + let config = self.gate.config.clone(); + let request_id = self.request_id.clone(); + tokio::task::spawn_blocking(move || { + match store::decide(&config, &request_id, ApprovalDecision::Deny) { + Ok(Some(_)) => tracing::debug!(request_id = %request_id, "..."), + Ok(None) => tracing::debug!(request_id = %request_id, "..."), + Err(err) => tracing::error!(request_id = %request_id, error = %err, "..."), + } + });Note
Configmust be cheaply cloneable (or wrapped inArc) for this to work without extra cost — worth confirming before adopting.🤖 Prompt for 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. In `@src/openhuman/approval/gate.rs` around lines 206 - 271, Remove the synchronous store::decide call from ParkedApprovalCleanupGuard::drop so destructor execution never performs SQLite I/O. Preserve the existing in-memory cleanup and denial behavior by cloning or otherwise safely capturing the required config and request_id, then dispatching the denial persistence through tokio::task::spawn_blocking with equivalent success and error logging inside the spawned task; handle any task-spawn failure without blocking Drop.
🤖 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.
Nitpick comments:
In `@src/openhuman/approval/gate.rs`:
- Around line 1400-1479: Add regression coverage for the abort/decision race in
the existing external-abort waiter tests: commit an approval via gate.decide
using the captured request_id immediately before aborting the spawned waiter,
then assert cleanup still occurs and store::get_decision remains
ApprovalDecision::ApproveOnce. Ensure the test exercises the conditional Deny
cleanup path so it cannot overwrite an already-persisted approval.
- Around line 206-271: Remove the synchronous store::decide call from
ParkedApprovalCleanupGuard::drop so destructor execution never performs SQLite
I/O. Preserve the existing in-memory cleanup and denial behavior by cloning or
otherwise safely capturing the required config and request_id, then dispatching
the denial persistence through tokio::task::spawn_blocking with equivalent
success and error logging inside the spawned task; handle any task-spawn failure
without blocking Drop.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 16d5574d-ba95-469e-bc01-db894687d36f
📒 Files selected for processing (1)
src/openhuman/approval/gate.rs
1 participant
Summary
Problem
ApprovalGate::intercept_auditedpreviously cleaned up its waiter, routing entries, and pending approval row only aftertokio::time::timeout(...).awaitreturned. When the surrounding turn was torn down by a wall-clock backstop, the future was dropped before those match arms ran. That left stale in-memory routes and a durable pending row until TTL expiry, so a later yes/no reply could be routed to a request that no longer had a live turn.Solution
An armed
Dropguard now owns cleanup responsibility while the approval is parked. Normal decision and timeout paths disarm it after their existing cleanup. If the future is dropped externally, the guard:The in-memory cleanup happens before the SQLite write so a persistence error cannot leave a reply route pointing at a dead waiter. The existing conditional
store::decideupdate preserves any decision that won a concurrent race.Submission Checklist
cargo-llvm-covis not installed locally.## Related- N/A: no matrix feature row changed.Closes #NNNin the## RelatedsectionImpact
Related
AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
codex/4774-approval-waiter-drop-cleanupdbcc54bc9a03b6a0b67654117254ac4477795351Validation Run
pnpm --filter openhuman-app format:check- passed in the pre-push hook.pnpm typecheck- passed in the pre-push hook.env GGML_NATIVE=OFF cargo test --manifest-path Cargo.toml --lib openhuman::approval::gate::tests::(31 passed, 0 failed).cargo fmt --manifest-path Cargo.toml --all --checkandenv GGML_NATIVE=OFF cargo clippy -p openhumanpassed; clippy reported the existing warning baseline.pnpm --filter openhuman-app rust:checkcompleted successfully.Validation Blocked
command:cargo llvm-cov --versionerror:no such command: llvm-covimpact:Local diff coverage was not generated; CI must verify the required 80% changed-line threshold before review-ready status.Behavior Changes
Parity Contract
Duplicate / Superseded PR Handling
Summary by CodeRabbit