Skip to content

Fix/connection introspection diagnostics#850

Open
girishjeswani wants to merge 4 commits into
malloydata:mainfrom
girishjeswani:fix/connection-introspection-diagnostics
Open

Fix/connection introspection diagnostics#850
girishjeswani wants to merge 4 commits into
malloydata:mainfrom
girishjeswani:fix/connection-introspection-diagnostics

Conversation

@girishjeswani

Copy link
Copy Markdown
Collaborator

Summary

When a connection fails to introspect a schema for an infrastructure reason —
an expired/invalid token (HTTP 401/403) or an unreachable/timing-out data plane
(5xx / network / timeout) — Publisher surfaced it as a cascade of Malloy
compile errors
(import reference failure + dozens of 'X' is not defined).
That sends developers (or their agents) to debug their .malloy model when the real cause is the
connection. This PR classifies such failures during package load and fails with a
single, accurate connection diagnostic instead — while leaving genuine model
errors (and real "table not found") untouched.

Before

A package whose source references a connection with an expired token fails to load with:

HTTP 424
Error(s) compiling model:
line 4: import reference failure
line 5: 'customer_id' is not defined
line 6: 'order_total' is not defined
line 7: Reference to undefined value revenue
... (one per field referencing the unresolved source)

Nothing here names the connection, the status, or the fact that a token expired.

Root cause

Two-stage information loss:

  1. The connection layer flattens the error. @malloydata/db-publisher's
    PublisherConnection collapses an AxiosError to a bare
    Error("Request failed with status code 401") — no status, connection, table,
    or auth-vs-not-found signal.
  2. Malloy turns that into a cascade. A failed table/SQL introspection leaves
    conn.table(...) unresolved, so the source and every field referencing it emit
    their own "X is not defined".

Two findings that shaped the fix (both verified empirically):

  • Throwing from the connection does not abort the compile — Malloy catches it
    internally and still produces the full cascade. The failure must be
    short-circuited on the main thread, before the worker's result is consumed.
  • A real expired token fails at connection resolution, not schema-fetch.
    PublisherConnection.create() validates the token eagerly (getConnection() +
    test()), so the 401 throws out of lookupConnection — the worker's
    connection-metadata RPC — before any fetchSchemaForTables.

The fix

  • New classifier (connection_error.ts): maps a flattened error string to
    auth / transport / not_found / other (HTTP-status-first, with
    network/timeout keyword fallback) and builds a developer-facing diagnostic.
  • Short-circuit in the package-load pool: records the first auth/transport
    failure across all three introspection RPC handlers — handleSchemaForTables,
    handleSchemaForSql, and handleConnectionMetadata (the resolution path) —
    covering both the per-table errors map and thrown-error paths. The load then
    fails with one typed error instead of resolving the cascade.
  • Typed errors, existing HTTP mapping: auth → ConnectionAuthError (422),
    transport → ConnectionError (502). Package.loadViaWorker / reloadAllModels
    let these bubble through with their 4xx/5xx mapping instead of rewrapping as 503.
  • Genuine model errors preserved: not_found (404 / "table does not exist")
    and anything unclassified are not short-circuited — they still compile to
    normal Malloy diagnostics.

After

HTTP 422
Connection "prod_warehouse" returned 401 Unauthorized while establishing the
connection. The access token is likely expired or invalid; refresh the connection
and reload. This is a connection problem, not an error in your Malloy model
(underlying error: Request failed with status code 401).

Testing

  • Unit tests for the classifier (401/403/404/5xx/408, raw network/timeout,
    keyword fallbacks, connection-level vs table-level phrasing).
  • Real-worker integration tests covering both the schema-fetch path and the
    connection-resolution path, for auth (→ one ConnectionAuthError, no cascade)
    and transport (→ one ConnectionError); not_found (404) asserted to remain
    a normal in-band per-model compile error. The resolution-path tests were
    confirmed non-vacuous (they fail without the fix).
  • Verified end-to-end against a live publisher-type proxy connection with an
    invalid token: branch-before returned the 424 cascade; branch-after returns the
    single 422 diagnostic above.
  • Full server unit suite green; lint and production build clean.

Follow-ups (next steps)

This PR makes the failure legible; it does not reduce how often it happens.
The most common trigger — a token that aged out — is best removed by the next two
changes, which this PR is a prerequisite for (they previously presented as a
model cascade and so were hard to even identify as connection problems):

  1. Evict the stale connection/token cache. Publisher persists the built
    connection — token and all — under publisher_data/ and reuses it across
    restarts, so injecting a fresh token and reloading can keep using the expired
    one until publisher_data/ is cleared. This makes the "refresh the connection
    and reload" remediation in the new diagnostic only partially effective today.
    Next PR: reconcile/evict the persisted connection on reload so a config token
    change actually takes effect.
  2. Token auto-refresh. Tokens are short-lived and don't refresh on their own
    (a window reload/re-login doesn't refresh the connection's token either). The
    connection/credential layer that owns the token should refresh it proactively
    so the expired-token case largely stops occurring. Separate layer/repo from
    this server-side fix.

Out of scope (this PR)

  • The ideal upstream fix — a typed error from @malloydata/db-publisher carrying
    { status, kind } — lives in a separate repo; this PR classifies server-side
    from the (status-bearing) error string.
  • Grouping derivative "not defined" errors under the root cause for
    unclassified connection failures (defense-in-depth).

Compatibility / risk

No behavior change for other connection types (BigQuery, DuckDB, Postgres, …) on
success, and genuine model/compile errors are unchanged — only infrastructure
auth/transport failures are short-circuited.

Introduces connection_error.ts: classifies a (flattened) schema-fetch
error string into auth / transport / not_found / other, and builds a
single developer-facing diagnostic naming the connection, table, HTTP
status, and likely cause. Only auth/transport are flagged as
infrastructure failures (the ones we'll short-circuit); not_found and
"other" are left to produce normal Malloy compile diagnostics.

Maps auth -> ConnectionAuthError (HTTP 422) and transport ->
ConnectionError (HTTP 502), reusing the existing error types. Pure
functions with unit tests covering 401/403/404/5xx/408, raw
network/timeout errors, keyword fallbacks, and the not-a-model-error
messaging.

Groundwork for fixing the "expired token -> import reference failure ->
dozens of 'X is not defined'" cascade.

Signed-off-by: Girish Jeswani <[email protected]>
When a connection can't introspect a table or SQL schema for an
infrastructure reason (expired token, unreachable data plane), the pool
now records the classified failure on the job (handleSchemaForTables and
handleSchemaForSql, both the errors-map and thrown-error paths) and, in
completeJob/errorJob, fails the whole package load with a single
ConnectionAuthError (422) / ConnectionError (502) instead of resolving
the worker's cascade of derivative "X is not defined" errors.

Package.loadViaWorker and reloadAllModels now let those typed connection
errors bubble through with their 4xx/5xx mapping rather than rewrapping
them as a misleading 503.

Genuine model errors are untouched: a 404 / "table does not exist" and
any unclassified error are NOT recorded, so they still compile to normal
Malloy diagnostics.

Signed-off-by: Girish Jeswani <[email protected]>
Adds a FlakyConnection (throws like PublisherConnection on an expired
token) and three real-worker tests over a model that references it:

- auth (401)      -> loadPackage rejects with a single ConnectionAuthError
                     naming the connection + table + status; the "X is not
                     defined" / "import reference failure" cascade does not
                     leak into the message.
- transport (503) -> rejects with a single ConnectionError.
- not-found (404) -> NOT short-circuited; resolves with the normal in-band
                     per-model compilationError (genuine model errors are
                     preserved).

Signed-off-by: Girish Jeswani <[email protected]>
… path)

The first cut classified failures only in handleSchemaForTables /
handleSchemaForSql. But @malloydata/db-publisher's
PublisherConnection.create() validates the token eagerly (getConnection
+ test) at connection-resolution time, so an expired token (401) /
unreachable plane (5xx) throws out of lookupConnection — the worker's
connection-metadata RPC — BEFORE any schema fetch. That path was
unclassified, so a real expired token still produced the full
"import reference failure" + "X is not defined" cascade (HTTP 424),
verified live against a live publisher-type data plane.

Fix: classify infra failures in handleConnectionMetadata's catch too,
and make SchemaFetchFailure.target optional — at resolution no specific
table is in hand, so the diagnostic reads "while establishing the
connection". Result, verified live: one ConnectionAuthError (422) /
ConnectionError (502), no cascade.

Tests: add real-worker regression tests that fail at RESOLUTION (a
lookupConnection that throws, mirroring create()), confirmed to fail
without this change and pass with it; add a unit test for the
connection-level phrasing; and correct FlakyConnection's doc comment,
which wrongly claimed it failed the way a real expired token does.

Signed-off-by: Girish Jeswani <[email protected]>
@girishjeswani
girishjeswani force-pushed the fix/connection-introspection-diagnostics branch from fc06ed2 to 6057843 Compare June 26, 2026 20:00
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.

1 participant