Repository navigation
fix(sdk): bound each DAPI request attempt, including the response body - #4973
Conversation
The request timeout was only sent as the `grpc-timeout` header, and tonic enforces that header on the client only until the response headers arrive. Reading the body had no limit, and channels had no HTTP/2 keepalive, so an attempt over a half-open connection (a network path that died mid-response, e.g. a mobile handover) never returned: no error, no retry, no ban. A mobile wallet observed requests pending for 8+ minutes while other traffic had recovered, and one stalled stream took 123 s to fail. - Bound the whole attempt, from dispatch to the last byte of the response, by `timeout + connect_timeout` — the bound `RequestSettings::timeout` already documents. A miss fails with the retryable `DeadlineExceeded`, so the existing loop bans the node and retries elsewhere. Zero still means "no limit". Native targets only; wasm is unchanged. - Evict the pooled connection after a miss so the next request to that node dials a fresh one instead of reusing the dead channel. - Ping with HTTP/2 keepalive while a request is in flight (15 s interval, 10 s timeout; idle connections are not pinged). - Give the megabyte-sized responses (shielded encrypted notes chunks, compacted address balance changes) a 5 minute attempt timeout, so slow links do not fail them. Callers that pass their own short timeout (e.g. the rs-sdk-ffi mobile address-sync settings, 5 s + 3 s connect) now bound the response body too. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: dashpay/platform/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe DAPI client adds native attempt deadlines that include response-body reading for unary calls. On timeout, it returns ChangesDAPI client timing
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant DapiClient
participant TransportClient
participant ConnectionPool
participant RetryLogic
DapiClient->>TransportClient: Build client and return its connection generation
DapiClient->>DapiClient: Bound transport attempt by deadline
DapiClient->>ConnectionPool: Remove matching URI entries up to that generation
DapiClient->>RetryLogic: Return DeadlineExceeded
RetryLogic->>DapiClient: Select next address for retry
Suggested reviewers: Merge Risk: 🔵 Low · up to Built-in DAPI requests use the corrected generation handling. Custom pool-backed transports may still evict a newer connection after a timeout, causing an unnecessary reconnect; this is bounded follow-up risk rather than a merge blocker. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The built-in transports preserve the identity of the connection used by each attempt, and eviction is limited to the attempted node. A narrower availability risk remains for custom transports that use the new default behavior: a timed-out request could discard a connection created by another request. Use of that extension path outside this repository is not established. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
|
✅ Final review complete — no blockers (commit 8442d58) · triage: normal |
|
Bots are done — your move: post |
github-actions
Bot
added
waiting-self-review
|
/self-reviewed |
github-actions
Bot
removed
the
waiting-self-review
|
Bots are done — your move: post |
github-actions
Bot
added
the
waiting-self-review
|
Bots are done — your move: post |
romchornyi
left a comment
romchornyi
left a comment
There was a problem hiding this comment.
Approving — no blockers found. The per-attempt deadline, pool eviction and keepalive are the right shape, and the new request_deadline.rs tests pin the executor behavior well. Everything below is a non-blocking recommendation — none of it needs to happen before merge.
-
LARGE_RESPONSE_TIMEOUTis silently defeated by execute-level settings.execute()-level settings overrideR::SETTINGS_OVERRIDES(dapi_client.rs:730-733), so the 300 s carve-out forGetRecentCompactedAddressBalanceChangesnever applies where a caller passes an explicit short timeout — concretely the rs-sdk-ffi mobile address-sync settings (5 s + 3 s → an 8 s cap on a ~9 MB response, plus a healthy-node ban per miss). The PR body already flags the 8 s cap, and I verifiedAddressSyncService/dash_sdk_sync_addresses_batch_with_resultcurrently has no callers in dashwallet-ios or anywhere org-wide, so this is latent — but the first caller of that path gets a ban machine instead of the protection this diff adds for exactly that request. A comment at the ffimobile_settings(or a larger timeout there for the catch-up phase) would keep the carve-out honest. -
Broadcast + deadline widens the ambiguous-broadcast class. A
broadcast_state_transitionattempt cut by the new deadline is retried on another node although the first node may have already relayed the ST — a hang becomes a possible double-submit / false-failure. This is the class already tracked in #4762 / dashwallet-ios#1132; worth a cross-reference there rather than a change here. -
Keepalive tuning on saturated links. On a slow bufferbloated link the server's PING ack can queue behind megabytes of in-flight response body; an ack delayed past the 10 s
HTTP2_KEEP_ALIVE_TIMEOUTkills a healthy connection mid-transfer and fails every in-flight chunk fetch on it — precisely during the parallel shielded-notes fetches the 300 s carve-out targets. Since the attempt deadline now bounds true hangs anyway, a more generous ack timeout would reduce false kills. -
Wedged-but-alive node now stalls large requests 300 s. Pre-PR, a node that never sent response headers was cut by tonic at the 10 s grpc-timeout; for the two large-response requests it is now 300 s per attempt (keepalive won't fire — the h2 layer is alive). A separate header-arrival bound, or a progress-based deadline, would keep the 300 s for the body only.
-
Smaller notes: (a) with
DEFAULT_CONNECT_TIMEOUT = Nonethe documentedtimeout + connect_timeoutbound reserves nothing for a fresh dial, so TCP+TLS on a high-RTT link is charged against the 10 s response budget; (b) deadline misses go through the ban ladder, so a ~15 s local outage bans every node with an in-flight request (the PR's own test log shows 5 nodes banned) and a quick reconnect can still hitNoAvailableAddresses; (c)remove_urievicts by URI, so a second straggler miss also evicts a freshly dialed healthy replacement channel; (d) the executor→pool eviction wiring (remove_urion a miss indapi_client.rs) has no test through the executor —FakeClientnever touches the pool, so a mis-keyed or deleted call would pass the suite.
🤖 Reviewed with Claude Code
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
Verified the supplied findings against head b23cdb1, the surrounding transport code, and the PR description. No blocking issue was confirmed; three non-blocking suggestions remain concerning streaming timeout documentation and regression coverage of stalled response bodies and connection eviction. Source inspection and git diff --check completed successfully; reviewer-reported test runs were not independently repeated.
🟡 3 suggestion(s)
Review provenance
Source: reviewer 1: gemini-3.8-flash-high (agent: phase1-reviewer, role: general); reviewer 2: gemini-3.8-flash-high (agent: phase1-reviewer, role: architecture-layering); reviewer 3: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 4: muse-spark-1.3-contributor (agent: phase1-reviewer, role: security-auditor); reviewer 5: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 6: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 7: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 8: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)
- Triage:
normalbygpt-6-astra(effort low) — This is a substantial but well-tested change to DAPI request deadlines, connection pooling, transport keepalive, and retry behavior, but it does not modify consensus, funds movement, cryptography, key handling, peer-facing deserialization, or storage migrations. - Phase 1 reviewers:
gemini-3.8-flash-high— general (completed, effort high); agentphase1-reviewer,gemini-3.8-flash-high— architecture-layering (completed, effort high); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— security-auditor (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-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (completed, effort high); agentphase2-reviewer,gpt-6-astra— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6-astra— rust-quality (completed, effort high); agentphase2-reviewer,gpt-6-astra— 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-client/src/request_settings.rs`:
- [SUGGESTION] packages/rs-dapi-client/src/request_settings.rs:25-30: Scope the response-body timeout guarantee to unary RPCs
The new documentation promises a client-side bound through the last response byte without distinguishing unary and streaming responses. However, the streaming requests in transport/grpc.rs return tonic::Streaming when headers arrive, so execute_transport completes and the executor's timeout ends before subsequent stream consumption. The PR description correctly identifies this limitation, but callers reading the public settings documentation will not see it. Document that unary responses are bounded through body and trailers, while streaming responses are bounded only until headers arrive; apply the same qualification to AppliedRequestSettings::attempt_deadline.
In `packages/rs-dapi-client/tests/request_deadline.rs`:
- [SUGGESTION] packages/rs-dapi-client/tests/request_deadline.rs:121-133: Cover connection-pool eviction through the deadline executor
The deadline tests exercise retries and banning, but FakeClient::with_uri_and_settings ignores the supplied ConnectionPool, leaving the executor's pool empty. The remove_uri unit test verifies removal directly, so deleting self.pool.remove_uri(address.uri()) from the timeout branch would leave both sets of tests passing. Since eviction is an explicit part of this fix, add an executor-level test with a pool-aware fake transport or an internal test that seeds lazy channels, triggers the deadline, and verifies that the timed-out URI is removed while another URI remains cached. This can run without network access.
- [SUGGESTION] packages/rs-dapi-client/tests/request_deadline.rs:158-161: Exercise a headers-complete, body-stalled response through tonic
DelayedRequest::execute_transport only sleeps before returning FakeResponse; it never produces response headers or exercises tonic's body/trailer reader. This verifies the executor's deadline around a delayed future, but does not demonstrate the comment's distinction between a headers-only timeout and a complete-response timeout—the specific failure this PR addresses. Add a deterministic in-memory transport regression test through tonic that supplies headers promptly and then stalls the unary body or trailers, asserting DeadlineExceeded and failover. Keep the existing fake-transport tests for focused retry-policy coverage.
|
Your move: thepastaclaw left review threads unresolved; resolve them. |
github-actions
Bot
removed
the
bot-review-skipped
Review follow-ups for the per-attempt DAPI deadline: - Document that the attempt bound covers the response body and trailers for unary RPCs only; for streaming RPCs it ends when the response headers arrive (`RequestSettings::timeout`, `AppliedRequestSettings::attempt_deadline`). - Executor-level test that the pooled connection of a node whose attempt missed its deadline is evicted while another node's stays pooled, using a fake client that takes its channel from the executor's pool. - Regression through tonic itself: an in-memory HTTP/2 server answers with response headers and never sends the body. The tests show tonic alone is still pending 2 s after a 200 ms `grpc-timeout`; that the executor cuts the same call with `DeadlineExceeded`; and that it fails over to another node. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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
The PR correctly bounds native unary attempts through response-body consumption, evicts timed-out pooled connections, configures HTTP/2 keepalive, and adds regression coverage for stalled bodies and pool eviction. The prior three findings are fixed at this head. One in-scope suggestion remains: the new URI eviction matcher can remove a different pooled URI when one URI is a textual prefix of another at a colon boundary.
🟡 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: architecture-layering); reviewer 3: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 4: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 5: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 6: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 7: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 8: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 9: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)
- Triage:
normalbygpt-6-astra(effort low) — The change coordinates request deadlines, retry-triggered connection eviction, keepalive, and large-response timeout settings across the SDK transport, but does not itself alter consensus, funds movement, cryptography, peer-facing deserialization, or storage migrations. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— architecture-layering (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(antigravity below 15% reserve: weekly 35% left, 5h 0% left),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-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (completed, effort high); agentphase2-reviewer,gpt-6-astra— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6-astra— rust-quality (completed, effort high); agentphase2-reviewer,gpt-6-astra— general (completed, effort high); agentphase2-reviewer,gpt-6-astra— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6-astra— rust-quality (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-client/src/connection_pool.rs`:
- [SUGGESTION] packages/rs-dapi-client/src/connection_pool.rs:102-113: Avoid prefix collisions when evicting pooled URIs
`remove_uri` identifies entries with `starts_with(format!("{}:{}:", prefix, uri))`, but pool keys are also built as `prefix:uri:settings`. This is not an exact URI match when the target URI ends before a colon: removing `http://node` constructs the prefix `Platform:http://node:`, which also matches the distinct pooled key for `http://node:443`. A timeout for the shorter URI can therefore evict the healthy longer URI's connection, defeating targeted eviction and causing an unnecessary reconnect. Use a structured pool key or otherwise compare the URI component exactly, and add a regression test for `http://node` versus `http://node:443`; the existing `3000` versus `30001` test does not expose this collision because the next character is not the delimiter.
|
Your move: thepastaclaw left review threads unresolved; resolve them. |
github-actions
Bot
added
waiting-self-review
`remove_uri` matched pooled entries by a `prefix:uri:` string prefix. A URI whose path contains a colon extends a shorter one past that delimiter, so evicting `http://node/grpc` also dropped the healthy connection to `http://node/grpc:8080`. Pool keys are now a struct (client type, URI, connection settings) and eviction compares the URI exactly. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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
Independently reviewed the complete diff at 1ac23c6 and confirmed that all four prior findings are fixed. One non-blocking concurrency issue remains: a late timeout can evict a newer healthy connection cached for the same URI. Validation passed for 152 unit/integration tests, 5 doctests, the WASM compilation check, and the review-range diff check; the working tree remains unchanged.
🟡 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: architecture-layering); reviewer 3: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 4: muse-spark-1.3-contributor (agent: phase1-reviewer, role: security-auditor); reviewer 5: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 6: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 7: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 8: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); reviewer 9: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 10: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 11: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 12: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)
- Triage:
normalbygpt-6-astra(effort low) — This is a substantial but well-tested SDK networking change spanning request deadlines, connection pooling, transport keepalive, retries, and response-body handling, without modifying consensus, funds, cryptography, storage, or other critical surfaces. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— architecture-layering (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— security-auditor (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 35% left, 5h 0% left),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-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (completed, effort high); agentphase2-reviewer,gpt-6-astra— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6-astra— rust-quality (completed, effort high); agentphase2-reviewer,gpt-6-astra— security-auditor (completed, effort high); agentphase2-reviewer,gpt-6-astra— general (completed, effort high); agentphase2-reviewer,gpt-6-astra— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6-astra— rust-quality (completed, effort high); agentphase2-reviewer,gpt-6-astra— 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-client/src/connection_pool.rs`:
- [SUGGESTION] packages/rs-dapi-client/src/connection_pool.rs:111-122: Avoid evicting a newer pooled connection after a concurrent timeout
The executor calls this method after an attempt expires, but the pool entry at that point need not be the connection used by that attempt. For example, with `ban_failed_address: false`, two concurrent requests can share a stalled channel; the first times out and evicts it, then its retry creates a fresh channel and succeeds. When the second original request reaches its later deadline, this URI-wide removal also evicts the fresh channel. Different request timeouts deliberately share a pool key, and the shared pool has no generation check to prevent this sequence. Existing users of the fresh channel retain their clones, but subsequent requests unnecessarily reconnect, and staggered timeouts can repeat that churn during recovery. Capture the connection generation used by each attempt and make deadline eviction conditional on that generation still being cached; add a regression test where a replacement is inserted before an older attempt expires.
Deadline eviction dropped every pooled connection to the node. Two requests can share a stalled channel: the first times out, evicts it and reconnects on retry, and when the second reaches its later deadline it also evicted that fresh connection, so the next request dialed again. The pool now numbers each connection it stores. The executor notes the pool's generation once the attempt has its connection, and a deadline eviction drops only connections to that node pooled at or before it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
github-actions
Bot
added
waiting-bots
|
Your move: thepastaclaw left review threads unresolved; resolve them. |
github-actions
Bot
added
waiting-self-review
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Reviewed the complete diff at 939431e. The DAPI client tests, native clippy checks, and WASM compilation check pass; four prior findings are fixed, but replacement-safe eviction still has a confirmed acquisition/snapshot race. A temporary deterministic regression reproduced the unnecessary eviction, and the worktree was restored clean.
🟡 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: architecture-layering); reviewer 3: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 4: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 5: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 6: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 7: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 8: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 9: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)
- Triage:
normalbygpt-6-astra(effort low) — The changes introduce nontrivial SDK request deadlines, connection-pool eviction, keepalive behavior, and regression tests, but do not alter consensus, funds movement, cryptography, peer-facing network deserialization, or storage migrations. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— architecture-layering (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(antigravity below 15% reserve: weekly 20% left, 5h 14% left),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-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (completed, effort high); agentphase2-reviewer,gpt-6-astra— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6-astra— rust-quality (completed, effort high); agentphase2-reviewer,gpt-6-astra— general (completed, effort high); agentphase2-reviewer,gpt-6-astra— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6-astra— rust-quality (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-client/src/dapi_client.rs`:
- [SUGGESTION] packages/rs-dapi-client/src/dapi_client.rs:1045-1050: Avoid evicting a newer pooled connection after a concurrent timeout
The generation guard protects replacements inserted after this snapshot, but connection acquisition and generation capture still take separate pool locks. Attempt A can clone connection N, another worker can replace it with N+1, and A can then sample N+1 here while retaining N. If A times out, `remove_uri(..., N+1)` removes the replacement even though A never used it. Existing channel clones remain usable, but subsequent requests unnecessarily reconnect during recovery. Your new executor regression inserts its replacement inside `execute_transport`, after the snapshot; I independently reproduced the uncovered ordering by retaining the acquired client and inserting a replacement on another thread before the client constructor returned, which fails the replacement-preservation assertion. Capture the acquired entry's generation together with its client under the same pool lock, propagate that token through transport-client construction, and add coverage for replacement between acquisition and constructor return.
|
Your move: thepastaclaw left review threads unresolved; resolve them. |
The executor read the pool generation after the transport client was built, under a separate lock. If another worker replaced the node's connection in between, the attempt kept the old connection but recorded the replacement's generation, and a deadline eviction then dropped the replacement. `ConnectionPool::get_or_create_with_generation` returns the connection and its generation from the same lock. `TransportClient` gains the provided method `with_uri_and_settings_and_generation`; the gRPC clients override it with that generation, and the executor evicts by it. The default still reads the pool generation after building the client. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
github-actions
Bot
added
waiting-bots
|
Your move: thepastaclaw left review threads unresolved; resolve them. |
github-actions
Bot
added
waiting-self-review
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 8442d58 and verified that all six prior findings are fixed, with no remaining actionable in-scope findings. The native deadline covers unary response consumption, and both production gRPC clients propagate the acquired connection's generation atomically so timeout eviction preserves newer replacements. Default-feature and all-feature rs-dapi-client tests, the WebAssembly package check, and diff whitespace checks passed; the worktree remains unchanged.
🔴 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: architecture-layering); reviewer 3: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 4: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 5: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 6: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 7: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 8: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 9: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)
- Triage:
normalbygpt-6-astra(effort low) — The change involves nontrivial asynchronous deadline enforcement, retry behavior, and generation-aware connection-pool eviction, but does not itself alter consensus, funds movement, cryptography, peer-facing network deserialization, or storage migrations. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— architecture-layering (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(antigravity below 15% reserve: weekly 13% left, 5h 52% left),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-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (completed, effort high); agentphase2-reviewer,gpt-6-astra— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6-astra— rust-quality (completed, effort high); agentphase2-reviewer,gpt-6-astra— general (completed, effort high); agentphase2-reviewer,gpt-6-astra— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6-astra— rust-quality (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
|
/self-reviewed |
|
Ready for review — needs QuantumExplorer or shumkov. |
github-actions
Bot
added
ready-for-human
4 participants
Issue being fixed or feature implemented
RequestSettings::timeoutis only sent as thegrpc-timeoutheader. tonic enforces that header on the client only until the response headers arrive (GrpcTimeoutwraps the service call, not the body read), and the tonic channels have no HTTP/2 keepalive. An attempt whose response body stalls — typically a half-open connection after a mobile network blip — therefore never returns: no error, no retry, no ban.Seen in a Dash Wallet iOS diagnostic export (build on
fe90c2f674):h2 protocol error: error reading a body from connectionwhen the node finally reset the streams;One of the hung requests was a DPNS name refresh that the wallet's network switch awaited, so the switch never finished and the app had to be killed. A fresh process worked immediately.
What was done?
dapi_client.rs, native targets only): the whole attempt — connect, headers, body, trailers — is bounded bytimeout + connect_timeout, the boundRequestSettings::timeoutalready documents. A miss becomesStatus::deadline_exceeded, which is already retryable, so the existing loop bans the node and retries on another one. A zero timeout still means "no limit". New helper:AppliedRequestSettings::attempt_deadline().ConnectionPool::remove_uri). Otherwise the dead channel stays pooled and the next request to that node stalls again once its ban expires.tonic_channel.rs): 15 s interval, 10 s timeout,keep_alive_while_idle(false), so idle pooled connections are never pinged.LARGE_RESPONSE_TIMEOUT):GetShieldedEncryptedNotes(chunks of thousands of notes, fetched in parallel) andGetRecentCompactedAddressBalanceChanges(~9 MB per the existing comment). Without it the new deadline would fail these on slow links and ban healthy nodes.RequestSettings::timeout/AppliedRequestSettings::timeoutdescribe the new bound.Unchanged: wasm (it only sends the header, as before), streaming methods (their future resolves at the response headers, so their streams are not bounded by this), the retry/ban policy itself.
Behaviour change worth a look: callers that pass a short explicit timeout now bound the response body too. For example, the rs-sdk-ffi mobile address-sync settings (
timeout5 s +connect_timeout3 s) now cap the whole attempt at 8 s, and the Dash Wallet iOS app doesn't call that path. I did not change those call sites.How Has This Been Tested?
cargo test -p rs-dapi-client: all green, including the new tests:tests/request_deadline.rs(fake transport, tokio paused clock):NoAvailableAddressesToRetry(DeadlineExceeded);timeout + connect_timeoutis cut;timeout == 0does not cut a slow response.attempt_deadline(sum, zero, saturation) andremove_uri(all prefixes and settings for the URI; a longer URI sharing the prefix is kept).attempt_deadline()forced toNone, the body-deadline test fails.cargo clippy -p rs-dapi-client --all-targets -- -D warnings,cargo fmt --check,cargo check -p rs-dapi-client --target wasm32-unknown-unknown,cargo check -p dash-sdk -p platform-wallet-ffi: all clean.Breaking Changes
None in the API:
AppliedRequestSettings::attempt_deadlineandConnectionPool::remove_uriare additions. The behaviour change for callers with short explicit timeouts is described above.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 ·
8442d58Cargo.lock,packages/rs-dapi-client/Cargo.toml,packages/rs-dapi-client/src/connection_pool.rsand 7 more) — QuantumExplorer or shumkovWhen every box is checked the
PR Hygienecheck passes and this can merge.Summary by CodeRabbit