feat(gql): add configurable soft deadline to GraphQL fan-out#813
Open
arielmelendez wants to merge 2 commits into
Open
feat(gql): add configurable soft deadline to GraphQL fan-out#813arielmelendez wants to merge 2 commits into
arielmelendez wants to merge 2 commits into
Conversation
List queries (transactions/blocks) fan out to every upstream and wait for all of them via Promise.allSettled, so one degraded upstream imposes its full request timeout (default 9.5s) on the whole query even when a healthy peer already answered in milliseconds. Add GATEWAYS_GQL_SOFT_DEADLINE_ENABLED (default false, preserving current behavior) and GATEWAYS_GQL_SOFT_DEADLINE_MS (default 2000). When enabled, the merge returns once the fastest upstreams have answered and the deadline elapses, surfacing each unfinished source as an UPSTREAM_SOFT_DEADLINE warning. The deadline is armed only after the first usable result, so an all-slow moment never turns a would-succeed query into a failure. Abandoned requests keep running so the per-upstream circuit breaker still observes their true outcome. Adds a gateways_gql_soft_deadline_source_cut_total counter, unit tests, and env docs (envs.md + docker-compose passthrough). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughIntroduces a configurable soft deadline for GraphQL fan-out queries in ChangesGraphQL Fan-out Soft Deadline
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/database/gateways-gql-queryable.ts (1)
1272-1282: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider adding a short TSDoc for
warningCodeForReason.Newly added helper; a one-line TSDoc documenting the
code → warning-codemapping keeps it consistent with the documented methods around it.As per coding guidelines: "Add or improve TSDoc comments on code you touch".
🤖 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/database/gateways-gql-queryable.ts` around lines 1272 - 1282, Add a short TSDoc comment for warningCodeForReason describing that it maps an upstream error reason/code to a warning code, with the EOPENBREAKER and ESOFTDEADLINE cases documented and the default fallback noted. Keep it consistent with nearby documented helpers in gateways-gql-queryable.ts and place it directly above warningCodeForReason so future readers understand the mapping at a glance.Source: Coding guidelines
src/database/gateways-gql-queryable.test.ts (2)
1006-1035: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy liftTight real-timer margin risks flaky test under CI load.
This test relies on both sources resolving in ~30ms and the deadline (armed after the first fulfillment) firing 15ms later (~45ms total). Under a loaded CI runner, a few extra milliseconds of scheduling jitter on the second
DelayedTxQueryablecould push it past the 45ms window, causing the test to intermittently produce a warning/partial result instead of asserting['a', 'b']with no warnings.Consider widening the margin (e.g., delay=20ms / deadline=60ms) or using fake timers to make the arming semantics deterministic instead of relying on real wall-clock races.
🤖 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/database/gateways-gql-queryable.test.ts` around lines 1006 - 1035, The soft-deadline test in GatewaysGqlQueryable.forTesting is too timing-sensitive because it depends on real 30ms delays and a 15ms post-fulfillment deadline, which can flake under CI load. Update the test to avoid wall-clock races by either widening the timing gap (for example, slower source delays with a much larger softDeadlineMs) or switching the DelayedTxQueryable setup to fake timers so the arming behavior in getGqlTransactions is deterministic. Keep the assertions on the merged transaction IDs and warnings unchanged.
1072-1077: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTighten block-query warning assertion for parity with the transaction test.
The transaction test (Line 977) asserts
warnings.length === 1and the source (Line 979), but this block test only checks the first warning's code via optional chaining, without asserting length orsource. Consider mirroring the stricter assertions here for consistency and to catch regressions (e.g., duplicate warnings).Proposed tightened assertions
- assert.equal((result.warnings ?? [])[0]?.code, 'UPSTREAM_SOFT_DEADLINE'); + const warnings = result.warnings ?? []; + assert.equal(warnings.length, 1); + assert.equal(warnings[0].code, 'UPSTREAM_SOFT_DEADLINE'); + assert.equal(warnings[0].source, 'http://slow.example');🤖 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/database/gateways-gql-queryable.test.ts` around lines 1072 - 1077, The block-query test currently only checks the first warning code, which is looser than the transaction test; update the assertions in gateways-gql-queryable.test.ts around the block-query result to mirror the stricter pattern used in the transaction test by verifying warnings.length is exactly 1 and asserting the warning source as well as the UPSTREAM_SOFT_DEADLINE code. Use the existing result.warnings access in this test and the transaction test’s warning checks as the reference point so the two cases stay consistent and catch duplicate warnings.
🤖 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.
Inline comments:
In `@src/database/gateways-gql-queryable.test.ts`:
- Around line 925-940: The soft-deadline test helper DelayedTxQueryable is
leaving ref’ed setTimeout handles behind, which can keep node --test open after
the fast path finishes. Update the delayed wait inside getGqlTransactions to use
an unref’ed timer (or otherwise avoid keeping the event loop alive) while
preserving the existing delay behavior in the DelayedTxQueryable test class.
---
Nitpick comments:
In `@src/database/gateways-gql-queryable.test.ts`:
- Around line 1006-1035: The soft-deadline test in
GatewaysGqlQueryable.forTesting is too timing-sensitive because it depends on
real 30ms delays and a 15ms post-fulfillment deadline, which can flake under CI
load. Update the test to avoid wall-clock races by either widening the timing
gap (for example, slower source delays with a much larger softDeadlineMs) or
switching the DelayedTxQueryable setup to fake timers so the arming behavior in
getGqlTransactions is deterministic. Keep the assertions on the merged
transaction IDs and warnings unchanged.
- Around line 1072-1077: The block-query test currently only checks the first
warning code, which is looser than the transaction test; update the assertions
in gateways-gql-queryable.test.ts around the block-query result to mirror the
stricter pattern used in the transaction test by verifying warnings.length is
exactly 1 and asserting the warning source as well as the UPSTREAM_SOFT_DEADLINE
code. Use the existing result.warnings access in this test and the transaction
test’s warning checks as the reference point so the two cases stay consistent
and catch duplicate warnings.
In `@src/database/gateways-gql-queryable.ts`:
- Around line 1272-1282: Add a short TSDoc comment for warningCodeForReason
describing that it maps an upstream error reason/code to a warning code, with
the EOPENBREAKER and ESOFTDEADLINE cases documented and the default fallback
noted. Keep it consistent with nearby documented helpers in
gateways-gql-queryable.ts and place it directly above warningCodeForReason so
future readers understand the mapping at a glance.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b9a6b289-0ab8-481d-bcd3-8c2f1eb60934
📒 Files selected for processing (6)
docker-compose.yamldocs/envs.mdsrc/config.tssrc/database/gateways-gql-queryable.test.tssrc/database/gateways-gql-queryable.tssrc/metrics.ts
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #813 +/- ##
===========================================
+ Coverage 78.82% 78.85% +0.03%
===========================================
Files 132 132
Lines 50172 50321 +149
Branches 3787 3800 +13
===========================================
+ Hits 39548 39683 +135
- Misses 10575 10589 +14
Partials 49 49 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
- Add TSDoc for warningCodeForReason (code -> warning-code mapping). - Bound the abandoned-laggard delays in the soft-deadline tests to 500ms (was 10s) so their still-ref'd timers can't hang `node --test` at exit; tighten test 1's assertion (<300ms) to prove the deadline path. (Note: unref'ing the shared helper's timers instead, as suggested, cancels the tests that legitimately await the laggard once the timer is the sole live handle, so the delay is bounded rather than unref'd.) - Widen the "arm after first result" test to 200ms delay / 100ms deadline to avoid CI timer-jitter flakiness, keeping deadline < delay so it still distinguishes call-time vs first-fulfillment arming. - Tighten the block-query test's warning assertions (length + source) for parity with the transaction test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Author
|
@coderabbitai — addressed the nitpicks in b6794d5:
The one actionable inline comment (unref timers) is answered in its thread. |
Contributor
|
Tip For best results, initiate chat on the files or code changes.
I'll take a look at the unref timers thread separately. This all looks good to me. ⋆｟•ω•｠⋆ ✏️ Learnings added
|
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
1 participant
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.
Problem
GatewaysGqlQueryablefans list queries (transactions/blocks) out to every configured upstream and waits for all of them viaPromise.allSettled. A single degraded upstream therefore imposes its full request timeout (default 9.5s) on the whole query — even when a healthy peer already returned in milliseconds. The result is still served (partial, with warnings), but only after the slow upstream's timeout elapses.Change
Add an opt-in soft deadline for list-query fan-out, off by default:
GATEWAYS_GQL_SOFT_DEADLINE_ENABLED(defaultfalse— preserves the currentPromise.allSettledbehavior exactly)GATEWAYS_GQL_SOFT_DEADLINE_MS(default2000)When enabled, once the first upstream returns a usable result the merge waits at most the deadline for the remaining upstreams, then returns a partial merge and surfaces each unfinished source as an
UPSTREAM_SOFT_DEADLINEwarning.Design notes
GATEWAYS_GQL_REQUEST_TIMEOUT_MS, which would make the breaker treat "slower than the deadline" as a failure and open on sources that would have succeeded.getGqlTransaction/getGqlBlockare unchanged — they already resolve on the first responding source.Observability
gateways_gql_soft_deadline_source_cut_total{source}counter.UPSTREAM_SOFT_DEADLINEGraphQL warning per cut source; logged at debug (like circuit-open) so it doesn't drown real upstream errors.Testing
yarn lint:checkclean; prod build typecheck (tsconfig.prod.json) clean.Rollout
Ships disabled; enable per deployment via env once validated.
🤖 Generated with Claude Code