enhance: retry the request if the schema is reloaded during the request #818
Conversation
🚀 Snapshot Release (
|
| Package | Version | Info |
|---|---|---|
@graphql-mesh/fusion-runtime |
0.11.4-alpha-7db7e2ccc8b399acae16e9269769a3b328e4827f |
npm ↗︎ unpkg ↗︎ |
@graphql-hive/gateway |
1.12.1-alpha-7db7e2ccc8b399acae16e9269769a3b328e4827f |
npm ↗︎ unpkg ↗︎ |
@graphql-hive/nestjs |
1.0.2-alpha-7db7e2ccc8b399acae16e9269769a3b328e4827f |
npm ↗︎ unpkg ↗︎ |
@graphql-hive/plugin-aws-sigv4 |
1.0.1-alpha-7db7e2ccc8b399acae16e9269769a3b328e4827f |
npm ↗︎ unpkg ↗︎ |
@graphql-mesh/plugin-opentelemetry |
1.3.47-alpha-7db7e2ccc8b399acae16e9269769a3b328e4827f |
npm ↗︎ unpkg ↗︎ |
@graphql-mesh/plugin-prometheus |
1.3.35-alpha-7db7e2ccc8b399acae16e9269769a3b328e4827f |
npm ↗︎ unpkg ↗︎ |
@graphql-hive/gateway-runtime |
1.6.1-alpha-7db7e2ccc8b399acae16e9269769a3b328e4827f |
npm ↗︎ unpkg ↗︎ |
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis pull request implements several changes across the codebase. It introduces a new retry mechanism for schema reloads during query execution and refactors the unified graph management by adding an optional callback and consolidating cleanup methods. Test files have been updated to streamline asynchronous handling and to validate schema reload and subscription scenarios. Additionally, new files are added for gateway configuration, end-to-end tests, and for simulating upstream service behaviors. Minor modifications to error messaging and GitHub Actions workflows are also included. Changes
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (1)`**`: For all PRs, we would like to verify that a Linear iss...
⏰ Context from checks skipped due to timeout of 90000ms (43)
🔇 Additional comments (11)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
🚀 Snapshot Release (Binary for
|
🚀 Snapshot Release (Binary for
|
🚀 Snapshot Release (Node Docker Image)The latest changes of this PR are available as image on GitHub Container Registry (based on the declared |
🚀 Snapshot Release (Binary for
|
🚀 Snapshot Release (Bun Docker Image)The latest changes of this PR are available as image on GitHub Container Registry (based on the declared |
🚀 Snapshot Release (Binary for
|
🚀 Snapshot Release (Binary for
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
packages/runtime/src/plugins/useRetryOnSchemaReload.ts (1)
7-14: Avoid using{}as a type.The WeakMap uses
{}as a key type, which is discouraged in TypeScript. Empty object types don't provide type safety and can lead to unexpected behavior.Consider using a more specific type or the
objecttype instead:- const execHandlerByContext = new WeakMap< - {}, - () => MaybePromise<MaybeAsyncIterable<ExecutionResult>> - >(); + const execHandlerByContext = new WeakMap< + object, + () => MaybePromise<MaybeAsyncIterable<ExecutionResult>> + >();🧰 Tools
🪛 Biome (1.9.4)
[error] 11-11: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
packages/runtime/tests/schema-reload.test.ts (2)
52-75: Consider adding more test cases for error scenarios.The current test only covers the happy path where the schema reload and retry succeed. Consider adding test cases for situations where retries fail or where multiple reloads occur.
// You could add an additional test case like: + it('handles multiple schema reloads correctly', async () => { + // Similar setup but with three schema versions and multiple reloads + }); + + it('handles retry failures appropriately', async () => { + // Setup where the second schema also has issues + });
76-110: Consider asserting the number of fetch attempts.The test ensures the final result is correct, but it would be beneficial to verify that the retry mechanism worked as expected by checking that
fetchCountequals 2 by the end of the test.+ expect(fetchCount).toBe(2); expect(result).toMatchObject({ data: { foo: 'barFromSecond', }, });packages/fusion-runtime/src/unifiedGraphManager.ts (2)
107-108: Consider adding documentation foronUnifiedGraphChange.Introducing
onUnifiedGraphChangeis a great enhancement. Documenting its usage and any potential side effects in the JSDoc or the related README would help clarify its expected behavior and integration points.
169-175: Consider adding a retry or fallback mechanism here.While logging and clearing
this.polling$is helpful when polling fails, you might want to add some logic to retry or wait before polling again. Otherwise, errors like network hiccups could permanently halt polling if not handled upstream.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.changeset/many-numbers-sell.md(1 hunks)packages/fusion-runtime/src/unifiedGraphManager.ts(4 hunks)packages/fusion-runtime/tests/polling.test.ts(3 hunks)packages/runtime/src/createGatewayRuntime.ts(4 hunks)packages/runtime/src/plugins/useRetryOnSchemaReload.ts(1 hunks)packages/runtime/tests/schema-reload.test.ts(1 hunks)packages/runtime/tests/subscriptions.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`packages/**`: In this directory we keep all packages releva...
packages/**: In this directory we keep all packages relevant to the gateway.In most cases, when changes are made to the source code / dependencies in these directories, we also require to have a corresponding changeset file, created using
yarn changesetwith a full description on the changes with an example.
Also, when new features are added the packages, the changeset file should mentionminorsemver change. When a bug is fixed,patchneeds to be used. Themajorbump is used for a PR that has breaking changes.
packages/runtime/src/plugins/useRetryOnSchemaReload.tspackages/runtime/tests/subscriptions.test.tspackages/fusion-runtime/tests/polling.test.tspackages/runtime/tests/schema-reload.test.tspackages/fusion-runtime/src/unifiedGraphManager.tspackages/runtime/src/createGatewayRuntime.ts
`**`: For all PRs, we would like to verify that a Linear iss...
**: For all PRs, we would like to verify that a Linear issue is linked. Check if there's a mention for "GW-*" in the PR description, and the us know when it's missing.In most cases, when a PR adds a new functionality, we would like to make sure it has documentation in the corresponding
consoleproject.
Use the global knowledge feature to search for PRs ingraphql-hive/consolerepository that adds documentation. Suggest to create a follow up issue to add the missing documentation.
packages/runtime/src/plugins/useRetryOnSchemaReload.tspackages/runtime/tests/subscriptions.test.tspackages/fusion-runtime/tests/polling.test.tspackages/runtime/tests/schema-reload.test.tspackages/fusion-runtime/src/unifiedGraphManager.tspackages/runtime/src/createGatewayRuntime.ts
🪛 Biome (1.9.4)
packages/runtime/src/plugins/useRetryOnSchemaReload.ts
[error] 11-11: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
⏰ Context from checks skipped due to timeout of 90000ms (21)
- GitHub Check: Examples / Convert openapi-arg-rename
- GitHub Check: Examples / Convert federation-subscriptions-passthrough
- GitHub Check: Binary built on ubuntu-24.04-arm
- GitHub Check: Binary built on windows-latest
- GitHub Check: Bun Docker image
- GitHub Check: Binary built on macos-14
- GitHub Check: Node Docker image
- GitHub Check: Binary built on macos-13
- GitHub Check: Binary built on ubuntu-latest
- GitHub Check: E2E / Bun Docker on Ubuntu
- GitHub Check: E2E / Node Binary on Windows
- GitHub Check: E2E / Node Binary on Ubuntu
- GitHub Check: E2E / Node Docker on Ubuntu
- GitHub Check: E2E / Node 23 on Ubuntu
- GitHub Check: Leaks / Node v23
- GitHub Check: E2E / Node 22 on Ubuntu
- GitHub Check: Leaks / Node v22
- GitHub Check: E2E / Node 20 on Ubuntu
- GitHub Check: Leaks / Node v20
- GitHub Check: Leaks / Node v18
- GitHub Check: Benchmark / bun / 10 items
🔇 Additional comments (18)
.changeset/many-numbers-sell.md (1)
1-7: LGTM! Changeset accurately describes the PR's purpose.The changeset correctly specifies patch version updates for both affected packages and provides a clear, concise description of the change: retrying requests instead of throwing errors when schema reloads occur during query execution.
packages/runtime/tests/subscriptions.test.ts (1)
170-174: Error code and message updates appropriately distinguish subscription-specific behavior.The changes make the error handling more precise by using a subscription-specific error code and message, which aligns with the PR objective of improving schema reload handling.
packages/runtime/src/plugins/useRetryOnSchemaReload.ts (2)
1-6: LGTM! Imports are appropriate for the plugin's functionality.The imports cover all necessary utilities for handling async operations, execution results, and integration with the gateway plugin system.
15-23: LGTM! Handler registration is appropriately implemented.The
onParamshook correctly stores the execution handler in the WeakMap for later retrieval, using the context as the key. This pattern enables the retry mechanism to access the original handler when needed.packages/runtime/tests/schema-reload.test.ts (2)
11-17: LGTM! Proper test cleanup.The test correctly sets up and cleans up the interval using the afterAll hook to prevent resource leaks.
18-51: Well-structured test setup for schema reload scenario.The test effectively simulates a schema reload scenario by creating two different upstream schemas - one that never resolves (simulating a long-running operation) and another with an immediate response.
packages/fusion-runtime/src/unifiedGraphManager.ts (4)
289-324: Mechanics for updating the unified graph look solid.This block properly ensures the schema, updates the transport entry map, and composes subgraph logic. The approach appears both clear and robust.
325-353: Validate the forced closure of old subscriptions.Setting
this.disposeReasontoSUBSCRIPTION_SCHEMA_RELOADforcibly closes existing subscriptions, which is appropriate for preventing stale data. However, confirm that downstream consumers can gracefully handle the 503 status code, if relevant.
434-458: Async disposal logic is well-structured.Centralizing the disposal of transportExecutorStack and executor in
[DisposableSymbols.asyncDispose]helps maintain clarity. The approach of setting them toundefinedreduces potential memory leaks.
461-473: Helper function for parallel disposal is well-organized.
disposeAllnicely handles synchronous and async disposables, ensuring each disposal is initiated. This improves code reuse and clarity in the disposal phase.packages/fusion-runtime/tests/polling.test.ts (4)
17-20: New imports align with the refactoring approach.Bringing in
DeferredPromiseandhandleMaybePromiseis suitable for coordinating asynchronous logic in a more controlled fashion.
81-95: Method conversion to handleMaybePromise is consistent.Refactoring
getFetchedTimeOnComment()to usehandleMaybePromisepreserves asynchronous handling and keeps the test code succinct.
97-119: Synchronous form with handleMaybePromise adds clarity.Switching from async functions to returning
handleMaybePromiseappears consistent with the new pattern. Just ensure the final result is properly awaited in all calling code.
121-133: Helper to compare times is well-structured.
compareTimes()now sequentially flows throughhandleMaybePromise, improving consistency across the test suite.packages/runtime/src/createGatewayRuntime.ts (4)
101-101: IncludinguseRetryOnSchemaReloadimproves reliability.Importing and using the new plugin helps ensure requests are retried if the schema reloads mid-request. This aligns with the PR’s main objective.
195-197: Ensure concurrency-safety when replacing schemas.
replaceSchema(newSchema)sets the new schema immediately, which is typically fine. In high-traffic scenarios, consider confirming that multiple near-simultaneous reloads don’t cause race conditions or partial updates.
690-693: Callback approach foronUnifiedGraphChangeis well integrated.Invoking both
unifiedGraph = newUnifiedGraphandreplaceSchema(newUnifiedGraph)ensures consistent updates in the gateway runtime and external references.
973-973: AddinguseRetryOnSchemaReloadto the plugin chain.This final integration fully activates the retry logic in the gateway. The placement in
basePluginsseems appropriate for capturing schema reload events early.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
e2e/graphos-polling/services/graphos.ts (2)
1-79: 💡 Verification agent🧩 Analysis chain
Linear issue reference missing in PR description
The PR appears to be missing a reference to a Linear issue (GW-*) in the description. This is required according to your coding guidelines.
🏁 Script executed:
#!/bin/bash # Check for Linear issue reference in the PR description # Get PR description PR_DESC=$(gh pr view 818 --json body -q .body) # Check if it contains GW-* pattern if echo "$PR_DESC" | grep -q "GW-[0-9]\+"; then echo "✅ Linear issue reference found in PR description" else echo "❌ Linear issue reference missing in PR description" echo "PR description should contain a reference to a Linear issue (GW-*)" fiLength of output: 768
Action Required: Add Linear Issue Reference in PR Description
It appears that the PR description is missing the required Linear issue reference (GW-*). Please update the PR description with a valid Linear issue identifier to comply with the coding guidelines.
1-79: 💡 Verification agent🧩 Analysis chain
Documentation might be needed for this new functionality
This PR adds new functionality for retrying requests when schema is reloaded. According to the coding guidelines, new functionality should have corresponding documentation in the
consoleproject.
🌐 Web query:
Are there any pull requests in graphql-hive/console repository that add documentation for schema reload retry functionality?💡 Result:
Based on the provided search results, there are no specific pull requests mentioned in the graphql-hive/console repository that document schema reload retry functionality. However, the documentation does cover related schema management features:
Schema Reload Mechanics:
The Hive CLI supports schema reloads through commands likehive dev --remote --watch, which automatically updates the supergraph when local schemas change[1][3]. This enables hot-reloading during local development without explicit retry logic.GitHub Integration:
The CLI supports--githuband--contextIdflags to retain schema check approvals within PR/branch lifecycles[1][6]. Failed checks can be manually approved through the Hive interface[4], but automatic retry behavior isn't documented.Error Handling:
While error codes 110-112 are documented for GitHub integration failures[1], and schema changes automatically close active subscriptions with an error message[14], there's no mention of built-in retry mechanisms for schema reloads in the provided documentation.For detailed implementation history, you'd need to examine the repository's pull requests directly, as the search results don't include specific PR discussions about retry functionality.
Citations:
- 1: https://the-guild.dev/graphql/hive/docs/api-reference/cli
- 2: https://the-guild.dev/graphql/stitching/handbook/architecture/hot-schema-reloading
- 3: https://the-guild.dev/graphql/hive/product-updates
- 4: https://the-guild.dev/graphql/hive/docs/schema-registry
- 5: https://www.youtube.com/watch?v=75F8hHAEiXU
- 6: https://github.com/graphql-hive/console/blob/main/deployment/CHANGELOG.md
- 7: https://github.com/graphql-hive/console
- 8: https://www.apollographql.com/docs/react/data/error-handling
- 9: https://community.developer.atlassian.com/t/rest-graphql-api-for-issue-pull-requests/56965
- 10: https://docs.stardog.com/release-notes/stardog-platform
- 11: https://github.com/graphql-hive/console/blob/main/packages/libraries/cli/README.md
- 12: https://bot.amplify.aws/questions
- 13: https://raw.githubusercontent.com/aws/aws-cli/v2/CHANGELOG.rst
- 14: https://the-guild.dev/graphql/hive/docs/gateway/subscriptions
- 15: https://docs.datadoghq.com/integrations/
- 16: Project Roadmap console#3858
- 17: https://npmjs.com/package/@graphql-hive/apollo
- 18: https://www.http4k.org/api/http4k-core/org.http4k.filter/-client-filters/-set-content-type/index.html
Action Required: Update Documentation for Retry Functionality
The implementation of retrying requests on schema reload looks correct, but our investigation confirms there is currently no explicit documentation in the console project covering this new retry mechanism. Since our coding guidelines require new functionality to be documented, please add or update the relevant documentation in the console project to detail:
- How the retry behavior works during schema reloads.
- Any new error codes, configurations, or CLI flags (if applicable) related to this functionality.
- Guidelines on expected behavior and troubleshooting steps for end users.
🧹 Nitpick comments (1)
e2e/graphos-polling/services/upstreamStuck.ts (1)
19-24: The resolver intentionally never resolves which could lead to memory leaks in tests.This Promise is created but never resolved or rejected, which could potentially lead to memory leaks in long-running test scenarios. While this is intentional for testing purposes, it would be good to add a comment explaining why this approach was chosen rather than using a very long timeout or other techniques.
foo: () => new Promise(() => { console.log('foo on upstreamStuck'); - // never resolves + // Intentionally never resolves to simulate a stuck upstream service + // This is used to test the retry mechanism during schema reload }),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (8)
e2e/graphos-polling/graphos-polling.e2e.ts(1 hunks)e2e/graphos-polling/package.json(1 hunks)e2e/graphos-polling/services/graphos.ts(1 hunks)e2e/graphos-polling/services/upstreamGood.ts(1 hunks)e2e/graphos-polling/services/upstreamStuck.ts(1 hunks)internal/e2e/src/example-setup.ts(1 hunks)internal/e2e/src/tenv.ts(4 hunks)packages/gateway/src/commands/supergraph.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- e2e/graphos-polling/package.json
🧰 Additional context used
📓 Path-based instructions (4)
`e2e/**`: This directory includes end-to-end tests for the g...
e2e/**: This directory includes end-to-end tests for the gateway.
Theexamplesdirectory is generated based on the code in this directory.
e2e/graphos-polling/services/upstreamGood.tse2e/graphos-polling/services/upstreamStuck.tse2e/graphos-polling/graphos-polling.e2e.tse2e/graphos-polling/services/graphos.ts
`**`: For all PRs, we would like to verify that a Linear iss...
**: For all PRs, we would like to verify that a Linear issue is linked. Check if there's a mention for "GW-*" in the PR description, and the us know when it's missing.In most cases, when a PR adds a new functionality, we would like to make sure it has documentation in the corresponding
consoleproject.
Use the global knowledge feature to search for PRs ingraphql-hive/consolerepository that adds documentation. Suggest to create a follow up issue to add the missing documentation.
e2e/graphos-polling/services/upstreamGood.tse2e/graphos-polling/services/upstreamStuck.tsinternal/e2e/src/example-setup.tspackages/gateway/src/commands/supergraph.tse2e/graphos-polling/graphos-polling.e2e.tse2e/graphos-polling/services/graphos.tsinternal/e2e/src/tenv.ts
`packages/**`: In this directory we keep all packages releva...
packages/**: In this directory we keep all packages relevant to the gateway.In most cases, when changes are made to the source code / dependencies in these directories, we also require to have a corresponding changeset file, created using
yarn changesetwith a full description on the changes with an example.
Also, when new features are added the packages, the changeset file should mentionminorsemver change. When a bug is fixed,patchneeds to be used. Themajorbump is used for a PR that has breaking changes.
packages/gateway/src/commands/supergraph.ts
`packages/gateway/**`: The main runtime for the Hive gateway...
packages/gateway/**: The main runtime for the Hive gateway.
This package is CLI that runs the gateway and configures the internals of the gateway function.
packages/gateway/src/commands/supergraph.ts
⏰ Context from checks skipped due to timeout of 90000ms (42)
- GitHub Check: Examples / Convert operation-field-permissions
- GitHub Check: Examples / Convert type-merging-batching
- GitHub Check: Examples / Convert subscriptions-with-transforms
- GitHub Check: Examples / Convert programmatic-batching
- GitHub Check: Examples / Convert openapi-javascript-wiki
- GitHub Check: Examples / Convert openapi-arg-rename
- GitHub Check: Examples / Convert openapi-additional-resolvers
- GitHub Check: Examples / Convert json-schema-subscriptions
- GitHub Check: Examples / Convert interface-additional-resolvers
- GitHub Check: Examples / Convert hmac-auth-https
- GitHub Check: Examples / Convert federation-subscriptions-passthrough
- GitHub Check: Examples / Convert apq-subgraphs
- GitHub Check: Examples / Convert federation-mixed
- GitHub Check: Examples / Convert federation-example
- GitHub Check: Examples / Convert extra-fields
- GitHub Check: Binary built on ubuntu-24.04-arm
- GitHub Check: Binary built on windows-latest
- GitHub Check: Binary built on macos-14
- GitHub Check: Binary built on macos-13
- GitHub Check: Bun Docker image
- GitHub Check: Binary built on ubuntu-latest
- GitHub Check: Node Docker image
- GitHub Check: E2E / Bun Docker on Ubuntu
- GitHub Check: E2E / Bun on Ubuntu
- GitHub Check: E2E / Node Binary on Windows
- GitHub Check: E2E / Node Binary on Ubuntu
- GitHub Check: E2E / Node Docker on Ubuntu
- GitHub Check: E2E / Node 23 on Ubuntu
- GitHub Check: E2E / Node 22 on Ubuntu
- GitHub Check: Unit / Node v18
- GitHub Check: Leaks / Node v23
- GitHub Check: E2E / Node 20 on Ubuntu
- GitHub Check: Leaks / Node v22
- GitHub Check: E2E / Node 18 on Ubuntu
- GitHub Check: Leaks / Node v20
- GitHub Check: Leaks / Node v18
- GitHub Check: Benchmark / bun / 1000 items
- GitHub Check: Benchmark / bun / 100 items
- GitHub Check: Benchmark / bun / 10 items
- GitHub Check: Benchmark / node / 1000 items
- GitHub Check: Benchmark / node / 100 items
- GitHub Check: Benchmark / node / 10 items
🔇 Additional comments (13)
packages/gateway/src/commands/supergraph.ts (2)
32-267: Verify that a Linear issue is linked to this PR.According to the coding guidelines, all PRs should reference a Linear issue with "GW-*" in the PR description. Please ensure this requirement is met.
1-406:❓ Verification inconclusive
Ensure a changeset file was created for this change.
The PR implements a retry mechanism for schema reloads during query execution, which is a new feature. According to the coding guidelines, changes to packages under
packages/gateway/**should be documented with a changeset file created usingyarn changesetwith a full description of the changes and an example. This would be aminorsemver change since it's adding a new feature.
🏁 Script executed:
#!/bin/bash # Check if a changeset file exists in the PR git diff --name-only origin/main... | grep -E "\.changeset/.*\.md"Length of output: 109
Action Required: Add a Changeset File Using
yarn changeset
It appears that our automated check for changeset files encountered an error (no merge base found), so we couldn’t confirm whether a changeset file was added or not. Please manually verify that a changeset file exists in your PR (under the.changesetfolder) to document the new retry mechanism for schema reloads. According to our guidelines, this new feature should be accompanied by a detailed changeset (a minor semver change).🧰 Tools
🪛 Biome (1.9.4)
[error] 298-298: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
e2e/graphos-polling/services/upstreamGood.ts (1)
1-31: LGTM! Good implementation of the test service.The service is correctly implemented to complement the upstreamStuck service in the test scenario. It properly returns "bar" when the foo query is executed, enabling verification of the retry mechanism.
internal/e2e/src/example-setup.ts (1)
28-30: Updated to handle new composition result structure.The code now correctly extracts the
outputproperty from the composition result, which indicates thatcomposeWithApollonow returns a more complex object rather than just a string. This change properly aligns with modifications in the API structure.e2e/graphos-polling/graphos-polling.e2e.ts (2)
6-12: Good cleanup practice for interval resources.Properly cleaning up the interval in the
afterAllhook ensures that resources are released after the test completes, preventing potential memory leaks.
14-73: Well-structured test for schema reload retry behavior.This test effectively verifies the retry mechanism by:
- Setting up a service that never resolves (upstreamStuck)
- Executing a query against it
- Switching the schema to a service that does resolve (upstreamGood)
- Verifying that the original query completes successfully
The test demonstrates that the system now retries requests when schemas are reloaded during execution, which aligns with the PR objective.
Can you verify if a Linear issue for "GW-*" is mentioned in the PR description? According to the coding guidelines, all PRs should have a linked Linear issue.
internal/e2e/src/tenv.ts (4)
270-270: Updated function signature to return Compose objectThe interface for
composeWithApollohas been updated to returnPromise<Compose>instead of a simple string. This change aligns with the implementation and provides a more structured return value with access to the output file path, result content, and cleanup methods.
383-384: Updated destructuring to match new return typeThe code now correctly destructures the
outputproperty from theComposeobject returned bycomposeWithApollo. This matches the updated function implementation.
972-982: Store IntrospectAndCompose instance for later cleanupThe implementation now stores the
IntrospectAndComposeinstance in a variable instead of directly accessing itssupergraphSdlproperty. This allows for proper resource cleanup through thecleanup()method later.
983-994: Enhanced return object with proper cleanup supportThe function now returns a comprehensive
Composeobject that includes the output file path, result content, placeholder methods for stats and logs, and most importantly, anasyncDisposemethod that properly cleans up resources by callingintrospectAndCompose.cleanup().This change supports the PR's objective of retrying requests during schema reloads by ensuring proper resource management.
e2e/graphos-polling/services/graphos.ts (3)
1-8: Good implementation of a mock GraphOS serverThe import statements and setup are clean and well-organized. The server uses
graphql-yogafor the GraphQL implementation and maintains state through thesupergraphSDLvariable, which is appropriate for this testing scenario.
9-68: Well-structured GraphQL schema for testing schema reloadsThe schema definition is comprehensive and includes all necessary types for simulating GraphOS behavior:
RouterConfigResultsunion supports different response typesRouterConfigResultincludes the supergraph SDL and other metadata- The mutation
setSupergraphSDLallows for dynamic schema updatesThis design properly supports the PR objective of testing retry behavior during schema reloads.
69-78: Simple and effective server setupThe server setup is minimal but sufficient for testing purposes. It creates a GraphQL server using Yoga and listens on the port provided by the testing options.
One small improvement could be to add error handling for the server startup, but this is acceptable for a test service.
ardatan
force-pushed
the
retry
branch
2 times, most recently
from
00900a0 to
3f027c2
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
e2e/graphos-polling/services/graphos.ts (5)
5-5: Add or confirm a reference to GW- (Linear) in the PR description.*According to the coding guidelines, every PR should reference a Linear issue (e.g., "GW-*"). Please confirm whether this is tracked elsewhere or add the reference for proper traceability.
7-8: Consider concurrency or multi-threaded scenarios.
supergraphSDLis stored in a global variable, which might lead to data inconsistencies under high concurrency if multiple requests mutate it simultaneously.
53-59: Validate or utilize query parameters.The
routerConfigresolver ignoresref,apiKey, andifAfterId. If these are intended for dynamic logic (e.g., conditional responses), consider implementing proper authorization, parameter validation, or usage in subsequent logic.
62-66: Validate SDL input before updating.The mutation blindly updates
supergraphSDLwithout checks. Consider validating or sanitizing the provided SDL for correctness or security concerns before storing it.
74-78: Handle server startup failures and document usage.Currently, no error handling or logging is done if the server fails to start (port in use, etc.). Also, consider creating or referencing documentation in the
consoleproject for this new service to help developers understand configuration and usage.packages/runtime/src/plugins/useRetryOnSchemaReload.ts (1)
11-11: Avoid using empty object type.The WeakMap key is defined as an empty object type
{}, which is generally discouraged.- const execHandlerByContext = new WeakMap< - {}, - () => MaybePromise<MaybeAsyncIterable<ExecutionResult>> - >(); + const execHandlerByContext = new WeakMap< + Record<string, unknown>, + () => MaybePromise<MaybeAsyncIterable<ExecutionResult>> + >();🧰 Tools
🪛 Biome (1.9.4)
[error] 11-11: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (18)
examples/apq-subgraphs/example.tar.gzis excluded by!**/*.gzexamples/extra-fields/example.tar.gzis excluded by!**/*.gzexamples/federation-example/example.tar.gzis excluded by!**/*.gzexamples/federation-mixed/example.tar.gzis excluded by!**/*.gzexamples/federation-subscriptions-passthrough/example.tar.gzis excluded by!**/*.gzexamples/file-upload/example.tar.gzis excluded by!**/*.gzexamples/hmac-auth-https/example.tar.gzis excluded by!**/*.gzexamples/interface-additional-resolvers/example.tar.gzis excluded by!**/*.gzexamples/json-schema-subscriptions/example.tar.gzis excluded by!**/*.gzexamples/openapi-additional-resolvers/example.tar.gzis excluded by!**/*.gzexamples/openapi-arg-rename/example.tar.gzis excluded by!**/*.gzexamples/openapi-javascript-wiki/example.tar.gzis excluded by!**/*.gzexamples/openapi-subscriptions/example.tar.gzis excluded by!**/*.gzexamples/operation-field-permissions/example.tar.gzis excluded by!**/*.gzexamples/programmatic-batching/example.tar.gzis excluded by!**/*.gzexamples/subscriptions-with-transforms/example.tar.gzis excluded by!**/*.gzexamples/type-merging-batching/example.tar.gzis excluded by!**/*.gzyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (15)
.changeset/many-numbers-sell.md(1 hunks)e2e/graphos-polling/graphos-polling.e2e.ts(1 hunks)e2e/graphos-polling/package.json(1 hunks)e2e/graphos-polling/services/graphos.ts(1 hunks)e2e/graphos-polling/services/upstreamGood.ts(1 hunks)e2e/graphos-polling/services/upstreamStuck.ts(1 hunks)internal/e2e/src/example-setup.ts(1 hunks)internal/e2e/src/tenv.ts(4 hunks)packages/fusion-runtime/src/unifiedGraphManager.ts(4 hunks)packages/fusion-runtime/tests/polling.test.ts(3 hunks)packages/gateway/src/commands/supergraph.ts(1 hunks)packages/runtime/src/createGatewayRuntime.ts(4 hunks)packages/runtime/src/plugins/useRetryOnSchemaReload.ts(1 hunks)packages/runtime/tests/schema-reload.test.ts(1 hunks)packages/runtime/tests/subscriptions.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- e2e/graphos-polling/package.json
- .changeset/many-numbers-sell.md
- e2e/graphos-polling/services/upstreamStuck.ts
- packages/runtime/tests/subscriptions.test.ts
- internal/e2e/src/example-setup.ts
- packages/gateway/src/commands/supergraph.ts
- packages/runtime/tests/schema-reload.test.ts
- internal/e2e/src/tenv.ts
- e2e/graphos-polling/graphos-polling.e2e.ts
- e2e/graphos-polling/services/upstreamGood.ts
🧰 Additional context used
📓 Path-based instructions (3)
`packages/**`: In this directory we keep all packages releva...
packages/**: In this directory we keep all packages relevant to the gateway.In most cases, when changes are made to the source code / dependencies in these directories, we also require to have a corresponding changeset file, created using
yarn changesetwith a full description on the changes with an example.
Also, when new features are added the packages, the changeset file should mentionminorsemver change. When a bug is fixed,patchneeds to be used. Themajorbump is used for a PR that has breaking changes.
packages/fusion-runtime/src/unifiedGraphManager.tspackages/fusion-runtime/tests/polling.test.tspackages/runtime/src/createGatewayRuntime.tspackages/runtime/src/plugins/useRetryOnSchemaReload.ts
`**`: For all PRs, we would like to verify that a Linear iss...
**: For all PRs, we would like to verify that a Linear issue is linked. Check if there's a mention for "GW-*" in the PR description, and the us know when it's missing.In most cases, when a PR adds a new functionality, we would like to make sure it has documentation in the corresponding
consoleproject.
Use the global knowledge feature to search for PRs ingraphql-hive/consolerepository that adds documentation. Suggest to create a follow up issue to add the missing documentation.
packages/fusion-runtime/src/unifiedGraphManager.tspackages/fusion-runtime/tests/polling.test.tspackages/runtime/src/createGatewayRuntime.tse2e/graphos-polling/services/graphos.tspackages/runtime/src/plugins/useRetryOnSchemaReload.ts
`e2e/**`: This directory includes end-to-end tests for the g...
e2e/**: This directory includes end-to-end tests for the gateway.
Theexamplesdirectory is generated based on the code in this directory.
e2e/graphos-polling/services/graphos.ts
🪛 Biome (1.9.4)
packages/runtime/src/plugins/useRetryOnSchemaReload.ts
[error] 11-11: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
⏰ Context from checks skipped due to timeout of 90000ms (29)
- GitHub Check: Binary built on ubuntu-24.04-arm
- GitHub Check: Binary built on windows-latest
- GitHub Check: Binary built on macos-14
- GitHub Check: Bun Docker image
- GitHub Check: Binary built on macos-13
- GitHub Check: Node Docker image
- GitHub Check: Binary built on ubuntu-latest
- GitHub Check: Bundle
- GitHub Check: Benchmark / bun / 1000 items
- GitHub Check: Build
- GitHub Check: Lint
- GitHub Check: Benchmark / bun / 100 items
- GitHub Check: Benchmark / bun / 10 items
- GitHub Check: Benchmark / node / 1000 items
- GitHub Check: Benchmark / node / 100 items
- GitHub Check: Benchmark / node / 10 items
- GitHub Check: E2E / Bun Docker on Ubuntu
- GitHub Check: E2E / Bun on Ubuntu
- GitHub Check: E2E / Node Binary on Windows
- GitHub Check: E2E / Node Binary on Ubuntu
- GitHub Check: E2E / Node Docker on Ubuntu
- GitHub Check: Leaks / Node v23
- GitHub Check: E2E / Node 23 on Ubuntu
- GitHub Check: Leaks / Node v22
- GitHub Check: E2E / Node 22 on Ubuntu
- GitHub Check: Leaks / Node v20
- GitHub Check: E2E / Node 20 on Ubuntu
- GitHub Check: Leaks / Node v18
- GitHub Check: E2E / Node 18 on Ubuntu
🔇 Additional comments (20)
packages/runtime/src/plugins/useRetryOnSchemaReload.ts (4)
1-6: Clean imports with relevant dependencies.The imports are logically organized and include all necessary dependencies for handling async iterable, maybe promises, and GraphQL execution results. The
YogaInitialContextwill be used for context casting during request handling.
7-13: Good plugin structure with type-safe context handling.The plugin is defined as a generic function that accepts a type parameter for context, allowing it to integrate with various context types in the gateway. The WeakMap is an excellent choice for associating execution handlers with contexts as it avoids memory leaks.
🧰 Tools
🪛 Biome (1.9.4)
[error] 11-11: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
14-23: Effective context and execution handler caching.The
onParamshook correctly stores the execution handler function in the WeakMap using the context as a key. This is a solid approach for preserving the execution context for potential retries.
24-38:✅ Verification successful
Extend error handling for more schema reload scenarios.
Currently, the plugin only handles errors with the code
'SUBSCRIPTION_SCHEMA_RELOAD'. Based on the PR objectives which aim to enhance the retry mechanism for schema reloads during query execution, this should be extended to handle other schema reload error codes as well.
🏁 Script executed:
#!/bin/bash # Search for all schema reload related error codes in the codebase echo "Searching for schema reload related error codes..." rg -i "SCHEMA_RELOAD" --type=ts echo "Checking for error handlers related to schema reloads..." rg -i "schema reload|reload.+schema" --type=ts -A 3 -B 3Length of output: 5069
Schema Reload Error Handling – Current Implementation is Sufficient
After verifying the repository, it appears that the only schema reload error code in use is
'SUBSCRIPTION_SCHEMA_RELOAD'. The tests and other parts of the codebase (e.g., inpackages/runtime/tests/subscriptions.test.tsandpackages/fusion-runtime/src/unifiedGraphManager.ts) all rely on that single code. As such, the current logic inpackages/runtime/src/plugins/useRetryOnSchemaReload.tsis consistent with the rest of the system.
- If future PRs or requirements introduce additional schema reload error codes, we should update this handler accordingly.
- For now, no changes are needed.
packages/runtime/src/createGatewayRuntime.ts (4)
101-101: Clean integration of the new retry plugin.The import statement for
useRetryOnSchemaReloadis correctly added, ensuring the new functionality is available for use in the gateway runtime.
195-197: Well-defined schema replacement function.The new
replaceSchemafunction provides a clear mechanism for updating the unified graph schema. This lambda function is defined at the appropriate scope, ensuring it has access to theunifiedGraphvariable.
690-693: Effective schema update handling in UnifiedGraphManager.The implementation of
onUnifiedGraphChangecorrectly updates the local schema and calls thereplaceSchemafunction, ensuring consistency when the schema changes. This is crucial for the retry mechanism to work properly.
973-973: Strategic placement of the retry plugin in basePlugins.The
useRetryOnSchemaReload()plugin is correctly added to thebasePluginsarray, ensuring it's included in every gateway instance. Its position in the array is appropriate as it needs to be active for all requests.packages/fusion-runtime/src/unifiedGraphManager.ts (7)
108-108: Clear interface extension for schema change notifications.The
onUnifiedGraphChangeoptional method is a well-designed addition to theUnifiedGraphManagerOptionsinterface. It provides a clean way to handle schema changes externally.
169-175: Improved error handling for polling failures.The enhanced error handling now properly logs errors when polling the supergraph fails. This provides better visibility into issues that may occur during schema polling.
289-351: Streamlined schema update logic with proper notification.The refactored
handleLoadedUnifiedGraphmethod now correctly updates all relevant properties when a new schema is loaded and calls theonUnifiedGraphChangecallback if provided. This ensures that all components are aware of schema changes.
336-336: Essential notification for schema changes.This line is crucial as it notifies external components about schema changes by calling the
onUnifiedGraphChangecallback. This is the key connection point that enables the retry mechanism in the gateway runtime.
339-350: Clear error message for subscription schema reloads.The error message for subscription schema reloads is clear and includes the appropriate code and HTTP status. This helps clients understand why their subscription was closed and allows the retry mechanism to identify these errors.
443-458: Consolidated resource cleanup in asyncDispose.The
asyncDisposemethod now handles all cleanup operations, including setting various properties toundefined. This is a cleaner approach to resource management compared to having separate cleanup methods.
461-473: Efficient utility for handling multiple disposables.The new
disposeAllfunction provides a clean way to dispose of multiple resources, making the code more maintainable. It correctly filters promises and handles both single and multiple promise cases.packages/fusion-runtime/tests/polling.test.ts (5)
17-20: Enhanced imports for better async handling.The import of
handleMaybePromisefrom@whatwg-node/promise-helpersenables more streamlined handling of asynchronous operations in the tests.
62-62: Simplified mock function setup.The
disposeFnmock function is now defined more simply without the explicitmockResolvedValue(undefined)call, which is sufficient for testing purposes.
81-95: Refactored test helper for better error handling.The
getFetchedTimeOnCommentfunction has been refactored to usehandleMaybePromisefor managing asynchronous operations. This approach provides better error handling and makes the code more readable.
97-119: Improved async handling in test helper.The
getFetchedTimeFromResolversfunction now uses nestedhandleMaybePromisecalls to handle the asynchronous operations, including the execution of GraphQL queries. This approach provides more robust error handling.
121-133: Streamlined comparison logic with better async flow.The
compareTimesfunction has been refactored to usehandleMaybePromisefor a cleaner async flow. This makes the test more robust and easier to understand.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/runtime/src/plugins/useRetryOnSchemaReload.ts (1)
27-29:⚠️ Potential issueExtend error handling to cover all schema reload scenarios, not just subscriptions
The current implementation only checks for errors with the code
'SUBSCRIPTION_SCHEMA_RELOAD', but based on the PR description, this plugin should handle schema reloads for all types of operations, not just subscriptions.Consider updating the error checking logic to handle all schema reload error codes. For example:
- (e) => e.extensions?.['code'] === 'SUBSCRIPTION_SCHEMA_RELOAD', + (e) => e.extensions?.['code'] === 'SCHEMA_RELOAD' || e.extensions?.['code'] === 'SUBSCRIPTION_SCHEMA_RELOAD',This would ensure that both regular query schema reloads and subscription schema reloads are handled properly.
🧹 Nitpick comments (5)
e2e/graphos-polling/services/graphos.ts (4)
5-7: Consider potential concurrency issues with the global variable.Storing
supergraphSDLin a global variable may introduce concurrency risks when multiple requests are served simultaneously. If this is just for tests, it might be acceptable. Otherwise, consider scoping the variable more narrowly or storing it in a concurrent-safe data store.
9-47: Unused union types might be unnecessary.Although the union
RouterConfigResultssupportsFetchErrorandUnchanged, the resolver always returnsRouterConfigResult. If these extra possibilities are no longer needed, consider removing them for clarity and maintainability.
53-59: Use string IDs for GraphQL consistency.
Date.now()returns a numeric timestamp, but GraphQL IDs are often treated as strings. Converting the timestamp to string ensures consistent type usage if your schema or clients expect string identifiers.- id: Date.now(), + id: String(Date.now()),
74-78: Add error handling and ensure proper PR references & documentation.
- Consider adding basic error handling or logging for the server startup to help diagnose potential port binding errors.
- We don’t see a mention of a “GW-*” Linear issue in this PR. If relevant, please ensure proper linking.
- This new functionality may need documentation in the
graphql-hive/consolerepository. Consider creating a follow-up issue if documentation is missing.packages/runtime/src/plugins/useRetryOnSchemaReload.ts (1)
10-13: Avoid using '{}' as a typeThe static analysis tool flagged this line because using
{}as a type is generally discouraged. It means "any non-nullable value" rather than an empty object.Consider using a more specific type:
const execHandlerByContext = new WeakMap< - {}, + Record<string, any>, () => MaybePromise<MaybeAsyncIterable<ExecutionResult>> >();🧰 Tools
🪛 Biome (1.9.4)
[error] 11-11: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (15)
.changeset/many-numbers-sell.md(1 hunks)e2e/graphos-polling/graphos-polling.e2e.ts(1 hunks)e2e/graphos-polling/package.json(1 hunks)e2e/graphos-polling/services/graphos.ts(1 hunks)e2e/graphos-polling/services/upstreamGood.ts(1 hunks)e2e/graphos-polling/services/upstreamStuck.ts(1 hunks)internal/e2e/src/example-setup.ts(1 hunks)internal/e2e/src/tenv.ts(4 hunks)packages/fusion-runtime/src/unifiedGraphManager.ts(4 hunks)packages/fusion-runtime/tests/polling.test.ts(3 hunks)packages/gateway/src/commands/supergraph.ts(1 hunks)packages/runtime/src/createGatewayRuntime.ts(4 hunks)packages/runtime/src/plugins/useRetryOnSchemaReload.ts(1 hunks)packages/runtime/tests/schema-reload.test.ts(1 hunks)packages/runtime/tests/subscriptions.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- .changeset/many-numbers-sell.md
- e2e/graphos-polling/package.json
- e2e/graphos-polling/services/upstreamStuck.ts
- packages/runtime/tests/subscriptions.test.ts
- packages/gateway/src/commands/supergraph.ts
- e2e/graphos-polling/services/upstreamGood.ts
- internal/e2e/src/example-setup.ts
- packages/runtime/tests/schema-reload.test.ts
- packages/fusion-runtime/tests/polling.test.ts
- e2e/graphos-polling/graphos-polling.e2e.ts
- packages/fusion-runtime/src/unifiedGraphManager.ts
🧰 Additional context used
📓 Path-based instructions (3)
`**`: For all PRs, we would like to verify that a Linear iss...
**: For all PRs, we would like to verify that a Linear issue is linked. Check if there's a mention for "GW-*" in the PR description, and the us know when it's missing.In most cases, when a PR adds a new functionality, we would like to make sure it has documentation in the corresponding
consoleproject.
Use the global knowledge feature to search for PRs ingraphql-hive/consolerepository that adds documentation. Suggest to create a follow up issue to add the missing documentation.
internal/e2e/src/tenv.tspackages/runtime/src/createGatewayRuntime.tse2e/graphos-polling/services/graphos.tspackages/runtime/src/plugins/useRetryOnSchemaReload.ts
`packages/**`: In this directory we keep all packages releva...
packages/**: In this directory we keep all packages relevant to the gateway.In most cases, when changes are made to the source code / dependencies in these directories, we also require to have a corresponding changeset file, created using
yarn changesetwith a full description on the changes with an example.
Also, when new features are added the packages, the changeset file should mentionminorsemver change. When a bug is fixed,patchneeds to be used. Themajorbump is used for a PR that has breaking changes.
packages/runtime/src/createGatewayRuntime.tspackages/runtime/src/plugins/useRetryOnSchemaReload.ts
`e2e/**`: This directory includes end-to-end tests for the g...
e2e/**: This directory includes end-to-end tests for the gateway.
Theexamplesdirectory is generated based on the code in this directory.
e2e/graphos-polling/services/graphos.ts
🪛 Biome (1.9.4)
packages/runtime/src/plugins/useRetryOnSchemaReload.ts
[error] 11-11: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
⏰ Context from checks skipped due to timeout of 90000ms (37)
- GitHub Check: Examples / Convert operation-field-permissions
- GitHub Check: Examples / Convert subscriptions-with-transforms
- GitHub Check: Examples / Convert programmatic-batching
- GitHub Check: Examples / Convert openapi-subscriptions
- GitHub Check: Examples / Convert openapi-arg-rename
- GitHub Check: Examples / Convert json-schema-subscriptions
- GitHub Check: Examples / Convert interface-additional-resolvers
- GitHub Check: Examples / Convert hmac-auth-https
- GitHub Check: Examples / Convert apq-subgraphs
- GitHub Check: Examples / Convert file-upload
- GitHub Check: Examples / Convert federation-mixed
- GitHub Check: Examples / Convert federation-example
- GitHub Check: Examples / Convert extra-fields
- GitHub Check: Binary built on ubuntu-24.04-arm
- GitHub Check: Binary built on windows-latest
- GitHub Check: Binary built on macos-14
- GitHub Check: Bun Docker image
- GitHub Check: Binary built on macos-13
- GitHub Check: Node Docker image
- GitHub Check: Binary built on ubuntu-latest
- GitHub Check: E2E / Bun Docker on Ubuntu
- GitHub Check: E2E / Node Binary on Windows
- GitHub Check: E2E / Node Binary on Ubuntu
- GitHub Check: E2E / Node Docker on Ubuntu
- GitHub Check: E2E / Node 23 on Ubuntu
- GitHub Check: Leaks / Node v23
- GitHub Check: E2E / Node 22 on Ubuntu
- GitHub Check: Leaks / Node v22
- GitHub Check: E2E / Node 20 on Ubuntu
- GitHub Check: Leaks / Node v20
- GitHub Check: E2E / Node 18 on Ubuntu
- GitHub Check: Leaks / Node v18
- GitHub Check: Unit / Bun
- GitHub Check: Benchmark / bun / 10 items
- GitHub Check: Benchmark / node / 1000 items
- GitHub Check: Benchmark / node / 100 items
- GitHub Check: Benchmark / node / 10 items
🔇 Additional comments (9)
e2e/graphos-polling/services/graphos.ts (1)
62-66: LGTM!The mutation logic is straightforward and clearly updates
supergraphSDL.internal/e2e/src/tenv.ts (4)
270-270: Interface method signature updated to returnComposeobject.The return type for
composeWithApollowas updated to match the actual implementation which returns a fullComposeobject instead of just a string. This change provides more control over the composed schema lifecycle.
383-383: Updated to destructure the object returned bycomposeWithApollo.This update properly handles the new return type of
composeWithApollo, extracting just theoutputproperty which is used to set thesupergraphvariable. This change ensures compatibility with the updated interface.
1-1057: Linear issue reference check for the PR.Reviewing the PR objectives, I don't see a reference to a Linear issue (GW-*) in the PR description. This should be added to align with the team's workflow requirements.
This PR implements a feature to retry requests when schema reloads occur. Please ensure a Linear issue is linked in the PR description with the format "GW-*".
Additionally, since this appears to be a new functionality, documentation should be added to the corresponding
consoleproject.🧰 Tools
🪛 Biome (1.9.4)
[error] 826-826: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
[error] 839-839: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
972-994:❓ Verification inconclusive
Added proper resource cleanup for Apollo composition.
The updated implementation stores the
introspectAndComposeinstance rather than just extracting thesupergraphSdlproperty. This allows for proper cleanup via the added[DisposableSymbols.asyncDispose]method. This change is significant because it:
- Returns a proper
Composeobject with all required properties- Implements resource cleanup through the disposal method
- Prevents memory leaks by cleaning up the composition resources
The implementation aligns with the PR objective to enhance retry behavior during schema reloads, as proper resource cleanup is essential for reliable retries.
🏁 Script executed:
#!/bin/bash # Check if the feature is tested in end-to-end tests # Look for tests that might be exercising the schema reload retry rg -l "schema.reload" --type ts internal/e2e/Length of output: 45
Manual Verification: Confirm Schema Reload Test Coverage & Resource Cleanup
In file
internal/e2e/src/tenv.ts(lines 972-994), the updated implementation now instantiates anIntrospectAndComposeobject and retains it so that cleanup can be properly handled through the[DisposableSymbols.asyncDispose]method. This change:
- Returns a
Composeobject with all the required properties.- Implements resource cleanup via the asynchronous disposal method.
- Helps prevent potential memory leaks by ensuring resources are released properly during retries.
However, our initial search for tests explicitly exercising the schema reload behavior (using the term
"schema.reload") didn’t produce any results. Please manually verify that the intended retry behavior – and the related cleanup mechanism – is covered by appropriate end-to-end tests, or update the test suite accordingly.packages/runtime/src/createGatewayRuntime.ts (4)
101-101: LGTM: Import of new schema reload retry pluginThe new plugin import is correctly added.
195-197: LGTM: Well-implemented schema replacement callbackThis implementation of
replaceSchemaproperly updates theunifiedGraphvariable when the schema changes.
690-693: LGTM: Appropriate handling of unified graph changesThe
onUnifiedGraphChangecallback correctly updates the schema and calls thereplaceSchemacallback, which ensures that components using the schema are properly updated when it changes.
973-973: LGTM: Schema reload retry plugin correctly added to base pluginsThe
useRetryOnSchemaReloadplugin is correctly added to the base plugins array, ensuring it's applied to all requests.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/e2e/src/tenv.ts (1)
985-1018: Consider extracting the logger implementationThe current implementation repeats similar logging code for different log levels (debug, error, info, warn). Consider extracting this into a reusable logger factory function to reduce code duplication.
+ function createLogger(stdout: string, stderr: string, stdboth: string, pipeLogs: boolean) { + return { + debug(msg: any) { + if (isDebug()) { + const line = inspect(msg) + '\n'; + stdout += line; + stdboth += line; + if (pipeLogs) { + process.stdout.write(line); + } + } + }, + error(msg: any) { + const line = inspect(msg) + '\n'; + stderr += line; + stdboth += line; + if (pipeLogs) { + process.stderr.write(line); + } + }, + info(msg: any) { + const line = inspect(msg) + '\n'; + stdout += line; + stdboth += line; + if (pipeLogs) { + process.stdout.write(line); + } + }, + warn(msg: any) { + const line = inspect(msg) + '\n'; + stdout += line; + stdboth += line; + if (pipeLogs) { + process.stdout.write(line); + } + } + }; + } + let stderr = ''; let stdout = ''; let stdboth = ''; let supergraphSdl: string; const introspectAndCompose = new IntrospectAndCompose({ subgraphs, - logger: { - debug(msg) { - if (isDebug()) { - const line = inspect(msg) + '\n'; - stdout += line; - stdboth += line; - if (pipeLogs) { - process.stdout.write(line); - } - } - }, - error(msg) { - const line = inspect(msg) + '\n'; - stderr += line; - stdboth += line; - if (pipeLogs) { - process.stderr.write(line); - } - }, - info(msg) { - const line = inspect(msg) + '\n'; - stdout += line; - stdboth += line; - if (pipeLogs) { - process.stdout.write(line); - } - }, - warn(msg) { - const line = inspect(msg) + '\n'; - stdout += line; - stdboth += line; - if (pipeLogs) { - process.stdout.write(line); - } - }, - }, + logger: createLogger(stdout, stderr, stdboth, pipeLogs), });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
e2e/graphos-polling/gateway.config.ts(1 hunks)e2e/graphos-polling/graphos-polling.e2e.ts(1 hunks)e2e/graphos-polling/services/graphos.ts(1 hunks)internal/e2e/src/example-setup.ts(1 hunks)internal/e2e/src/tenv.ts(4 hunks)packages/gateway/src/commands/supergraph.ts(4 hunks)packages/runtime/src/getReportingPlugin.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/e2e/src/example-setup.ts
- e2e/graphos-polling/services/graphos.ts
- packages/gateway/src/commands/supergraph.ts
🧰 Additional context used
📓 Path-based instructions (3)
`packages/**`: In this directory we keep all packages releva...
packages/**: In this directory we keep all packages relevant to the gateway.In most cases, when changes are made to the source code / dependencies in these directories, we also require to have a corresponding changeset file, created using
yarn changesetwith a full description on the changes with an example.
Also, when new features are added the packages, the changeset file should mentionminorsemver change. When a bug is fixed,patchneeds to be used. Themajorbump is used for a PR that has breaking changes.
packages/runtime/src/getReportingPlugin.ts
`**`: For all PRs, we would like to verify that a Linear iss...
**: For all PRs, we would like to verify that a Linear issue is linked. Check if there's a mention for "GW-*" in the PR description, and the us know when it's missing.In most cases, when a PR adds a new functionality, we would like to make sure it has documentation in the corresponding
consoleproject.
Use the global knowledge feature to search for PRs ingraphql-hive/consolerepository that adds documentation. Suggest to create a follow up issue to add the missing documentation.
packages/runtime/src/getReportingPlugin.tse2e/graphos-polling/graphos-polling.e2e.tse2e/graphos-polling/gateway.config.tsinternal/e2e/src/tenv.ts
`e2e/**`: This directory includes end-to-end tests for the g...
e2e/**: This directory includes end-to-end tests for the gateway.
Theexamplesdirectory is generated based on the code in this directory.
e2e/graphos-polling/graphos-polling.e2e.tse2e/graphos-polling/gateway.config.ts
⏰ Context from checks skipped due to timeout of 90000ms (43)
- GitHub Check: Examples / Convert operation-field-permissions
- GitHub Check: Examples / Convert type-merging-batching
- GitHub Check: Examples / Convert subscriptions-with-transforms
- GitHub Check: Examples / Convert programmatic-batching
- GitHub Check: Examples / Convert openapi-subscriptions
- GitHub Check: Examples / Convert openapi-javascript-wiki
- GitHub Check: Examples / Convert openapi-arg-rename
- GitHub Check: Examples / Convert openapi-additional-resolvers
- GitHub Check: Examples / Convert json-schema-subscriptions
- GitHub Check: Examples / Convert interface-additional-resolvers
- GitHub Check: Examples / Convert hmac-auth-https
- GitHub Check: Examples / Convert federation-subscriptions-passthrough
- GitHub Check: Examples / Convert apq-subgraphs
- GitHub Check: Examples / Convert file-upload
- GitHub Check: Examples / Convert federation-mixed
- GitHub Check: Examples / Convert federation-example
- GitHub Check: Examples / Convert extra-fields
- GitHub Check: Binary built on ubuntu-24.04-arm
- GitHub Check: Binary built on windows-latest
- GitHub Check: Binary built on macos-14
- GitHub Check: Binary built on macos-13
- GitHub Check: Bun Docker image
- GitHub Check: Binary built on ubuntu-latest
- GitHub Check: Node Docker image
- GitHub Check: E2E / Bun Docker on Ubuntu
- GitHub Check: E2E / Bun on Ubuntu
- GitHub Check: E2E / Node Binary on Windows
- GitHub Check: E2E / Node Binary on Ubuntu
- GitHub Check: E2E / Node Docker on Ubuntu
- GitHub Check: E2E / Node 23 on Ubuntu
- GitHub Check: E2E / Node 22 on Ubuntu
- GitHub Check: E2E / Node 20 on Ubuntu
- GitHub Check: Leaks / Node v23
- GitHub Check: E2E / Node 18 on Ubuntu
- GitHub Check: Leaks / Node v22
- GitHub Check: Leaks / Node v20
- GitHub Check: Benchmark / bun / 1000 items
- GitHub Check: Benchmark / bun / 100 items
- GitHub Check: Leaks / Node v18
- GitHub Check: Benchmark / bun / 10 items
- GitHub Check: Benchmark / node / 1000 items
- GitHub Check: Benchmark / node / 100 items
- GitHub Check: Benchmark / node / 10 items
🔇 Additional comments (6)
e2e/graphos-polling/gateway.config.ts (1)
1-27: Gateway configuration looks goodThe gateway configuration is well-structured and properly sets up both supergraph and reporting sections using the
defineConfigfunction from@graphql-hive/gateway. The code correctly handles different environment conditions for determining theuplinkHost.e2e/graphos-polling/graphos-polling.e2e.ts (2)
16-88: Well-structured test for schema reload retry behaviorThis test effectively validates the core functionality of the PR - ensuring the system retries requests when the schema reloads during query execution rather than throwing errors. The test first uses a "stuck" subgraph that never returns, then updates the schema to use a "good" subgraph while a request is in flight, verifying that the request is properly retried against the new schema.
8-14: Good cleanup of interval resourcesThe code properly cleans up the interval after tests complete, preventing memory leaks and potential interference with other tests.
internal/e2e/src/tenv.ts (3)
384-387: Updated function call to match new signatureThe call to
composeWithApollohas been properly updated to use the new parameter structure.
966-1060: Enhanced implementation of composeWithApolloThe implementation has been significantly expanded to include detailed logging, proper error handling, and a more structured return value. The method now correctly:
- Determines the appropriate hostname based on the gateway runner
- Sets up comprehensive logging for different levels (debug, error, info, warn)
- Creates a temporary file for the supergraph SDL
- Initializes the IntrospectAndCompose instance with proper handlers
- Returns a structured object with useful methods
This substantial improvement aligns with the PR's goal of enhancing the system's behavior when handling schema reloads.
271-271:✅ Verification successful
API signature changed for composeWithApollo
The method signature has been updated to accept a structured
ComposeOptionsobject and return a more comprehensiveComposeresult object, which is a good improvement for API clarity.
🏁 Script executed:
#!/bin/bash # Check for any other usages of the composeWithApollo function to ensure all callers use the new signature rg -A 3 "composeWithApollo\(" --type tsLength of output: 1703
API Change for
composeWithApolloVerifiedThe updated method now clearly accepts a structured
ComposeOptionsobject and returns a comprehensiveComposeresult. Our shell script check confirmed that all instances—in files likeinternal/e2e/src/tenv.ts,internal/e2e/src/example-setup.ts, and the graphos polling tests—are using the new signature (with consistent destructuring and either anoutputorresultproperty being accessed). This confirms the intended improvement for API clarity.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
e2e/graphos-polling/graphos-polling.e2e.ts (3)
28-45: Add error checking for schema push operations.The
pushSchemafunction doesn't check if the operation was successful. Consider adding error handling to ensure the test fails appropriately if the schema update fails.function pushSchema(schema: string) { - return fetch(`${hostname}:${graphos.port}/graphql`, { + return fetch(`${hostname}:${graphos.port}/graphql`, { method: 'POST', headers: { 'Content-Type': 'application/json', }, body: JSON.stringify({ query: /* GraphQL */ ` mutation SetSupergraphSDL($sdl: String!) { setSupergraphSDL(sdl: $sdl) } `, variables: { sdl: schema, }, }), + }).then(async response => { + const result = await response.json(); + if (!response.ok || result.errors) { + throw new Error(`Failed to push schema: ${JSON.stringify(result.errors || 'Unknown error')}`); + } + return result; }); }
59-67: Consider adding a timeout to the interval.The interval is set to execute queries every 100ms without an upper bound, which could lead to resource exhaustion if the test runs for too long. Consider adding a timeout or maximum number of attempts.
+ let attempts = 0; + const MAX_ATTEMPTS = 100; // 10 seconds total interval = setInterval(() => { + if (attempts >= MAX_ATTEMPTS) { + clearInterval(interval!); + interval = undefined; + console.warn('Max attempts reached for keeping gateway warm'); + return; + } + attempts++; gw.execute({ query: /* GraphQL */ ` { __typename } `, }); }, 100);
84-87: Verify retry behavior more explicitly.The test checks that both services were called but doesn't directly verify that a retry occurred due to the schema reload. Consider adding assertions to verify the specific retry mechanism.
const upstreamStuckStd = upstreamStuck.getStd('both'); expect(upstreamStuckStd).toContain('foo on upstreamStuck'); const upstreamGoodStd = upstreamGood.getStd('both'); expect(upstreamGoodStd).toContain('foo on upstreamGood'); + // Verify retry occurred by checking logs or metrics + // For example, if your gateway logs retries: + const gatewayStd = gw.getStd ? gw.getStd('both') : ''; + expect(gatewayStd).toContain('Retrying request due to schema reload');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
e2e/graphos-polling/graphos-polling.e2e.ts(1 hunks)e2e/graphos-polling/services/upstreamGood.ts(1 hunks)e2e/graphos-polling/services/upstreamStuck.ts(1 hunks)packages/runtime/src/getReportingPlugin.ts(0 hunks)
💤 Files with no reviewable changes (1)
- packages/runtime/src/getReportingPlugin.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- e2e/graphos-polling/services/upstreamStuck.ts
- e2e/graphos-polling/services/upstreamGood.ts
🧰 Additional context used
📓 Path-based instructions (2)
`e2e/**`: This directory includes end-to-end tests for the g...
e2e/**: This directory includes end-to-end tests for the gateway.
Theexamplesdirectory is generated based on the code in this directory.
e2e/graphos-polling/graphos-polling.e2e.ts
`**`: For all PRs, we would like to verify that a Linear iss...
**: For all PRs, we would like to verify that a Linear issue is linked. Check if there's a mention for "GW-*" in the PR description, and the us know when it's missing.In most cases, when a PR adds a new functionality, we would like to make sure it has documentation in the corresponding
consoleproject.
Use the global knowledge feature to search for PRs ingraphql-hive/consolerepository that adds documentation. Suggest to create a follow up issue to add the missing documentation.
e2e/graphos-polling/graphos-polling.e2e.ts
⏰ Context from checks skipped due to timeout of 90000ms (44)
- GitHub Check: Examples / Convert operation-field-permissions
- GitHub Check: Examples / Convert type-merging-batching
- GitHub Check: Examples / Convert subscriptions-with-transforms
- GitHub Check: Examples / Convert programmatic-batching
- GitHub Check: Examples / Convert openapi-subscriptions
- GitHub Check: Examples / Convert openapi-javascript-wiki
- GitHub Check: Examples / Convert openapi-arg-rename
- GitHub Check: Examples / Convert openapi-additional-resolvers
- GitHub Check: Examples / Convert json-schema-subscriptions
- GitHub Check: Examples / Convert interface-additional-resolvers
- GitHub Check: Examples / Convert hmac-auth-https
- GitHub Check: Examples / Convert federation-subscriptions-passthrough
- GitHub Check: Examples / Convert apq-subgraphs
- GitHub Check: Examples / Convert file-upload
- GitHub Check: Examples / Convert federation-mixed
- GitHub Check: Examples / Convert federation-example
- GitHub Check: Examples / Convert extra-fields
- GitHub Check: Binary built on ubuntu-24.04-arm
- GitHub Check: Binary built on windows-latest
- GitHub Check: Binary built on macos-14
- GitHub Check: Binary built on macos-13
- GitHub Check: Binary built on ubuntu-latest
- GitHub Check: Bun Docker image
- GitHub Check: Node Docker image
- GitHub Check: E2E / Bun Docker on Ubuntu
- GitHub Check: E2E / Bun on Ubuntu
- GitHub Check: E2E / Node Binary on Windows
- GitHub Check: E2E / Node Binary on Ubuntu
- GitHub Check: E2E / Node Docker on Ubuntu
- GitHub Check: E2E / Node 23 on Ubuntu
- GitHub Check: E2E / Node 22 on Ubuntu
- GitHub Check: E2E / Node 20 on Ubuntu
- GitHub Check: E2E / Node 18 on Ubuntu
- GitHub Check: Leaks / Node v23
- GitHub Check: Leaks / Node v22
- GitHub Check: Leaks / Node v20
- GitHub Check: Leaks / Node v18
- GitHub Check: Benchmark / bun / 1000 items
- GitHub Check: Bundle
- GitHub Check: Benchmark / bun / 100 items
- GitHub Check: Benchmark / bun / 10 items
- GitHub Check: Benchmark / node / 1000 items
- GitHub Check: Benchmark / node / 100 items
- GitHub Check: Benchmark / node / 10 items
🔇 Additional comments (3)
e2e/graphos-polling/graphos-polling.e2e.ts (3)
10-14: Good cleanup handling in the afterAll block.Properly cleaning up intervals is crucial to prevent memory leaks and interference with other tests.
16-20: Clear test scenario documentation.The comment clearly explains the test scenario, which is good for readability and maintenance.
1-88: Verify Linear issue is linked.I couldn't find a mention of a "GW-*" issue in the PR description. According to the coding guidelines, all PRs should have a linked Linear issue.
Please check if there's a Linear issue linked to this PR and add it to the description if it's missing.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
e2e/graphos-polling/services/upstream_good.ts (1)
1-31: Clean implementation of a properly functioning test service.This "good" upstream service is well-implemented as a counterpart to the stuck service, returning a simple 'bar' response. The pair of services create an ideal testing environment for the schema reload retry mechanism.
For consistency between files, you might consider using the same function style for resolvers as in upstream_stuck.ts:
- foo() { + foo: () => {Both styles work fine, but maintaining the same style across similar files can improve readability and maintenance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
e2e/graphos-polling/services/upstream_good.ts(1 hunks)e2e/graphos-polling/services/upstream_stuck.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`e2e/**`: This directory includes end-to-end tests for the g...
e2e/**: This directory includes end-to-end tests for the gateway.
Theexamplesdirectory is generated based on the code in this directory.
e2e/graphos-polling/services/upstream_good.tse2e/graphos-polling/services/upstream_stuck.ts
`**`: For all PRs, we would like to verify that a Linear iss...
**: For all PRs, we would like to verify that a Linear issue is linked. Check if there's a mention for "GW-*" in the PR description, and the us know when it's missing.In most cases, when a PR adds a new functionality, we would like to make sure it has documentation in the corresponding
consoleproject.
Use the global knowledge feature to search for PRs ingraphql-hive/consolerepository that adds documentation. Suggest to create a follow up issue to add the missing documentation.
e2e/graphos-polling/services/upstream_good.tse2e/graphos-polling/services/upstream_stuck.ts
⏰ Context from checks skipped due to timeout of 90000ms (23)
- GitHub Check: Unit / Bun
- GitHub Check: Unit / Node v20
- GitHub Check: E2E / Bun Docker on Ubuntu
- GitHub Check: E2E / Bun on Ubuntu
- GitHub Check: Leaks / Node v23
- GitHub Check: Leaks / Node v22
- GitHub Check: E2E / Node Binary on Ubuntu
- GitHub Check: Leaks / Node v20
- GitHub Check: E2E / Node Docker on Ubuntu
- GitHub Check: Benchmark / bun / 1000 items
- GitHub Check: Leaks / Node v18
- GitHub Check: E2E / Node 23 on Ubuntu
- GitHub Check: Benchmark / bun / 100 items
- GitHub Check: E2E / Node 22 on Ubuntu
- GitHub Check: Benchmark / bun / 10 items
- GitHub Check: Benchmark / node / 1000 items
- GitHub Check: E2E / Node 20 on Ubuntu
- GitHub Check: Bundle
- GitHub Check: Benchmark / node / 100 items
- GitHub Check: E2E / Node 18 on Ubuntu
- GitHub Check: Benchmark / node / 10 items
- GitHub Check: Build
- GitHub Check: Analyze (javascript-typescript)
ardatan
force-pushed
the
retry
branch
2 times, most recently
from
ac6224e to
7defbda
Compare
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
When the schema is reloaded during a query execution, retry the request instead of throwing a reload event error
Previously, it was throwing an error
SCHEMA_RELOADwhen the schema is changed during the request.Waiting for user's confirmation to merge!