Repository navigation
fix(rs-dapi): keep the Core RPC permit until the call returns - #5507
Conversation
guarded_blocking_call held the concurrency permit in the caller's future. When a gRPC request was cancelled, or after the 10 s permit timeout, the permit was freed while the spawn_blocking Core call kept running, so the two-permit limit did not bound the number of Core calls in flight. The permit now moves into the blocking task and is dropped when the call returns. One REQUEST_TIMEOUT deadline covers both the permit wait and the call, so a queued caller gets a timeout instead of waiting without limit. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 20 minutes. View limit details
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Description Check |  |
Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | |
The title clearly summarizes the main change: Core RPC permits remain held until the underlying call returns. |
| Docstring Coverage | |
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 10 functions across 1 files. |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
✨ Finishing Touches
-
📝 Generate docstrings
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- 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.
|
Waiting for bot review — coderabbitai not yet · thepastaclaw not yet. Wait for the missing reviews, or a writer can post |
github-actions
Bot
added
the
waiting-bots
|
✅ Final review complete — no blockers (commit abf2344) · triage: normal |
…dapi-core-rpc-6711dc
|
Waiting for bot review — coderabbitai not yet · thepastaclaw not yet. Wait for the missing reviews, or a writer can post |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
The Core RPC permit is now correctly owned by the blocking task, so caller cancellation cannot release capacity while the underlying operation is still running. The focused cancellation test covers the primary regression, but the newly introduced shared 30-second deadline and timeout error paths remain untested. The Rust workspace test check was still in progress at review time.
🟡 1 suggestion(s)
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
normalbygpt-6.1-sol(effort low) — This is a contained but behavior-changing asynchronous concurrency fix whose permit ownership, cancellation, timeout, and background task lifetimes require more than a straightforward low-effort check, without affecting a critical surface. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(lane failed),glm-5.3-flash(not used above high effort; tier asks max) - Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort high); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-dapi/src/clients/core_client.rs`:
- [SUGGESTION] packages/rs-dapi/src/clients/core_client.rs:53-73: Cover the shared deadline timeout paths
The new cancellation test verifies that permits remain held until detached blocking calls return, but it does not exercise the other behavior introduced here: one deadline covers both semaphore acquisition and result waiting, and either expiry maps to `DapiError::Timeout`. A regression that applies separate 30-second waits, returns a different error while waiting for a permit, or mishandles the permit after the caller timeout would pass the current test suite. Add a private deadline-injectable helper or otherwise make the timeout duration controllable in tests, then cover both a request queued behind occupied permits and a caller timing out while its blocking operation remains active.
|
Bots are done — your move: post |
github-actions
Bot
added
waiting-self-review
guarded_blocking_call now delegates to guarded_blocking_call_until, which takes the deadline, so tests can use a short one. New tests check that a call queued behind busy permits times out at its deadline without running, and that a running call times out at the same deadline while its operation keeps the permit until it returns. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Waiting for bot review — coderabbitai ✓ · thepastaclaw not yet. Wait for the missing reviews, or a writer can post |
github-actions
Bot
added
waiting-bots
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
At c212ae4, the permit correctly remains owned by the blocking operation after caller cancellation or timeout, and both waiting stages use one deadline. The added tests address the prior timeout-coverage finding; static verification found no actionable in-scope issues. The current Rust workspace tests check failed in the unchanged drive-abci contract-fee-claim activation test, which expects an internal error instead of the returned StateTransitionNotActive consensus error; this is unrelated to the Core RPC change, and no local builds or tests were run.
🔴 0 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(s)
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
normalbygpt-6.1-sol(effort low) — This is a contained asynchronous Rust concurrency change in rs-dapi that alters request timeout and semaphore-lifetime behavior, but it does not modify a critical surface such as consensus, funds, cryptography, or storage. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(lane failed),glm-5.3-flash(not used above high effort; tier asks max) - Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort high); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify the current code and confirm that no unresolved issues remain.
No unresolved findings remain from the prior review on this head.
|
Bots are done — your move: post |
github-actions
Bot
added
waiting-self-review
…dapi-core-rpc-6711dc
|
Bots are done — your move: post |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Independently reviewed the complete diff at abf2344 and found no actionable defects. The blocking closure retains the semaphore permit after caller cancellation or timeout, both waiting stages use the same deadline, and the added tests address the prior coverage finding. Validation was static only; current CI reports successful Rust workspace tests and no failed code checks, with PR Hygiene still pending.
🔴 0 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(s)
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
normalbygpt-6.1-sol(effort low) — The contained change modifies cancellation-sensitive semaphore ownership and shared timeout behavior across asynchronous and blocking execution, requiring concurrency reasoning but not changing a critical surface. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(lane failed),glm-5.3-flash(not used above high effort; tier asks max) - Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort high); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify the current code and confirm that no unresolved issues remain.
No unresolved findings remain from the prior review on this head.
Basic explanation
What this does: DAPI lets at most two calls to Dash Core run at once and makes every other call wait its turn. Until now a call gave up its turn as soon as the request that started it went away (the client hung up or the request timed out), or after 10 seconds, even though the call was still running on Core. With this change a call keeps its turn until Core answers.
Value: the "two at a time" limit now really holds. Core-backed endpoints (
getBlock,getTransaction,broadcastTransaction,getBlockchainStatusand others) can no longer build up a backlog of leftover Core calls that delays requests from everyone else.Risks: Low. The change is limited to rs-dapi's Core client and has no consensus impact. One behaviour changes: a request waiting for its turn now times out after 30 seconds in total (waiting plus running), where before the wait had no limit. This only shows up when Core is already slow. Calls were already serialized on a single Core connection, so throughput when Core is healthy does not change.
Issue being fixed or feature implemented
CoreClient::guarded_blocking_callheld the concurrency permit in the caller's future and ran the Core call inspawn_blocking. A blocking call cannot be stopped, so dropping the caller (gRPC cancellation, the outerREQUEST_TIMEOUT, or the 10 sPERMIT_TIMEOUTrelease) freed the permit while the call kept running. Each new request could then take a permit and start another blocking call. These calls queued on the transport's socket mutex and used blocking-pool threads, with no limit tied toCORE_RPC_GUARD_PERMITS.Before, with two permits:
After:
What was done?
OwnedSemaphorePermitmoves into thespawn_blockingclosure, so it is dropped exactly when the Core call returns.REQUEST_TIMEOUTdeadline (timeout_at, passed toguarded_blocking_call_until) covers both the permit wait and the call. A caller that hits it gets the sameDapiError::Timeoutas before. The call itself finishes in the background and is bounded by the jsonrpc transport's 15 s socket timeouts.PERMIT_TIMEOUTand thespawn+mpsc+select!loop. Releasing the permit early did not raise throughput, becausejsonrpc'sSimpleHttpTransportalready sends one request at a time over a single socket behind a mutex.How Has This Been Tested?
clients::core_client::tests::should_keep_permits_until_cancelled_calls_return: starts two calls that block, cancels both callers, checks that no permit is free while the calls still run, releases them, and checks that the permits come back and a new call succeeds. Against the old implementation it fails withleft: 2, right: 0(both permits free while both calls still run).should_time_out_a_call_still_waiting_for_a_permit_at_its_deadlineandshould_time_out_a_running_call_at_the_same_deadline_and_keep_its_permit(through the privateguarded_blocking_call_until, which takes the deadline so tests can use a short one): a call queued behind busy permits times out at its deadline without running, and a running call times out at the same deadline while its operation keeps the permit until it returns. Each fails if its half of the shared deadline is removed (freshREQUEST_TIMEOUTfor the call stage, or no limit on the permit wait).cargo test -p rs-dapi --lib clients(16 passed) andcargo test -p rs-dapi --lib core_service(3 passed).cargo clippy -p rs-dapi --all-targets: clean for rs-dapi.cargo fmt --all.Breaking Changes
None.
Checklist:
structure.rs, regeneratedgrovedb-structure.json, and checked the structure viewer link posted on this pull requestFor repository code-owners and collaborators only
🤖 Generated with Claude Code
PR Hygiene ·
abf2344/self-reviewedrust-dapi(packages/rs-dapi/src/clients/core_client.rs) — lklimek or shumkovWhen every merge requirement is met, the
PR Hygienecheck passes. Reviewer limits do not block merging; other required GitHub checks and protections still apply.Summary by CodeRabbit