Skip to content

fix(graphqlSchema): surface schema-load errors instead of a 30s timeout - #1918

Open
kylebernhardy wants to merge 1 commit into
mainfrom
fix/graphql-load-error-surfacing-1917
Open

fix(graphqlSchema): surface schema-load errors instead of a 30s timeout#1918
kylebernhardy wants to merge 1 commit into
mainfrom
fix/graphql-load-error-surfacing-1917

Conversation

@kylebernhardy

Copy link
Copy Markdown
Member

Summary

A GraphQL schema error during a graphqlSchema component's initial load (bad directive, reserved database name, malformed schema) was swallowed into a 30s hang. handleApplication awaited only the initialLoadComplete success event, so on error it never settled and the component-load watchdog eventually fired with a generic message — handleApplication timed out after 30000ms — that named none of the actual problem.

This rejects the returned promise with the real error, so the component fails fast through the loader's normal failure path: an ErrorResource is installed on the route carrying the true cause, e.g.

Could not load component 'graphqlSchema' for application '<app>' due to: Syntax Error: Expected Name, found <EOF>.

Closes #1917.

Where to look

  • resources/graphql.tshandleApplication now creates an explicit initialLoadPromise that resolves on initialLoadComplete and rejects if processGraphQLSchema throws during the initial load. The initial-load handler deliberately does not rethrow (if (!initialLoadComplete) { rejectInitialLoad(error); return; }): rejecting already fails the component through the loader, and not rethrowing lets the entry handler settle cleanly so the load-tracking machinery doesn't surface the same error a second time as an unhandled rejection. A post-initial-load reload error still rethrows, preserving existing reload behavior.

Verification

Driven at runtime against a booted instance (harper dev) with a malformed schema, before vs. after on identical conditions:

Before (main) After
Time to fail ~33s (watchdog) ~4s
Failure cause surfaced handleApplication timed out … (generic) … due to: Syntax Error: Expected Name, found <EOF>
Unhandled rejections 2 0
Route response 500 after 30s 500 fast, body names the parse error

Valid schemas are unaffected (control: table served, database created, no errors). The fix also eliminates a pre-existing pair of unhandled rejections that main emitted on any schema-load error.

Attention / open items

  • Could not run the new integration test in this local env — the harness requires the loopback address pool (127.0.0.2+), a one-time sudo setup (npx harper-integration-test-setup-loopback) not available here. Its assertions (500 + body matches /Syntax Error/, not /timed out/) mirror the runtime-observed HTTP response exactly; CI has the loopback pool configured.
  • The malformed-schema fixture is added to .prettierignore (its parse failure is the point of the test).
  • Minor, not addressed: the loader logs the failure via error.stack, whose top line still shows the original GraphQLError: message rather than the rewritten Could not load component … prefix. The prefixed message is what the route serves and what error.message carries; this is pre-existing loader behavior, out of scope here.

PR generated by kAIle (Claude Opus 4.8).

…ut (#1917)

When a graphqlSchema component's initial load throws (bad directive, reserved
database name, malformed schema), handleApplication awaited only the success
event and hung until the component-load watchdog fired 30s later with a generic
"handleApplication timed out" message that named none of the actual problem.

Reject the returned promise with the real error so the component fails fast
through the loader's normal path (ErrorResource on the route with the true
cause). The initial-load handler no longer rethrows, so the load-tracking
machinery settles cleanly and the error is not also surfaced as an unhandled
rejection (which the pre-fix path emitted).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0188G62J9fZQg4J9rVuqLzjy
@kylebernhardy
kylebernhardy requested a review from kriszyp July 23, 2026 20:58

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request addresses an issue where a schema error during the initial load of a graphqlSchema component would cause handleApplication to hang and eventually time out, rather than failing fast with the actual error. The changes modify resources/graphql.ts to wrap the schema processing in a try-catch block during the initial load, rejecting the load promise immediately with the real error if a failure occurs. Additionally, an integration test and a malformed schema fixture have been added to verify this behavior and prevent regressions. I have no feedback to provide.

@claude

claude Bot commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Reviewed; no blockers found.

@kylebernhardy
kylebernhardy marked this pull request as ready for review July 24, 2026 00:35
@kriszyp

kriszyp commented Jul 27, 2026

Copy link
Copy Markdown
Member

I think this is fundamentally a Scope/component-loader promise-ordering bug, with GraphQL exposing it.

Scope.handleEntry() already tracks the promise returned by an async entry callback. If that callback rejects, its tracked initial-load promise rejects and initialLoadComplete is not emitted. However, sequentiallyHandleApplication() currently waits in this order:

  1. plugin.handleApplication(scope)
  2. scope.waitForInitialLoads()

GraphQL returns once(entryHandler, "initialLoadComplete"), so a rejected entry callback creates a cycle: the success event is never emitted, handleApplication() never settles, and the loader never reaches the rejected promise in waitForInitialLoads(). The watchdog is therefore the only promise that settles.

The invariant should be generic: any async scope.handleEntry() rejection during the initial scan fails the component with the original cause. I would prefer enforcing that in Scope/the loader rather than adding a GraphQL-specific rejection bridge that catches and suppresses the entry-handler error. The loader should observe initial-load failure while handleApplication() is running, then await all registered initial loads after handler registration completes. We should also ensure the cleanup chains consume rejection; the current ignored .finally() result appears to contribute to the duplicate unhandled rejections.

A generic loader test with scope.handleEntry(async () => { throw cause; }) would cover the invariant; the GraphQL integration test can remain as the regression case.

— KrAIs (GPT-5 Codex)

@kriszyp kriszyp left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you haven't asked Claude a half a dozen times if this is the root cause, the PR might not be ready :)

@kylebernhardy

Copy link
Copy Markdown
Member Author

Kyle's call: please take this one as well, alongside #1936.

Your diagnosis holds — I traced it before conceding:

componentLoader.ts chains the initial-load wait after the handler returns:

Promise.resolve(plugin.handleApplication(scope)).then(async () => {
    await scope.waitForInitialLoads();
})

and graphql.ts's handleApplication only settles on initialLoadComplete. So a rejected entry callback is exactly the cycle you describe: the success event never fires, handleApplication never settles, and the loader never reaches the rejection that waitForInitialLoads() is already holding. The watchdog is the only arm of the race that can settle, which is why the 30s timeout — naming nothing about the actual bad directive — was all the user ever saw.

And my fix is the workaround you named. It catches the error, calls rejectInitialLoad(error), and deliberately does not rethrow; my own comment in the diff says that's so "the load-tracking machinery doesn't surface the same error a second time." That's a GraphQL-specific bridge around a broken generic invariant, and it leaves every other scope.handleEntry() consumer with the same hang.

Your framing is the right one: any async handleEntry rejection during the initial scan should fail the component with the original cause, enforced in the loader — observing initial-load failure while handleApplication is still running, rather than sequentially after it. Your generic test (scope.handleEntry(async () => { throw cause; })) is the one that actually pins the invariant; the GraphQL integration test in this PR is worth keeping as the regression case if it's useful to you.

Branch is fix/graphql-load-error-surfacing-1918. Take it over or open a replacement — I've stopped work on it. The integration fixture reproduces #1917 end-to-end (a reserved database name in a @table directive) if that's a useful starting point.

Worth noting for whoever lands it: this is the second of two where I fixed the symptom at the consumer instead of the contract. Fair hit, and the pattern is noted on my side.

Comment generated by kAIle (Claude Opus 4.8).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

graphqlSchema component load swallows schema errors into a 30s timeout

3 participants