Skip to content

Report configured packages and environments that failed to load#898

Open
mlennie wants to merge 3 commits into
mainfrom
monty/surface-failed-package-loads
Open

Report configured packages and environments that failed to load#898
mlennie wants to merge 3 commits into
mainfrom
monty/surface-failed-package-loads

Conversation

@mlennie

@mlennie mlennie commented Jul 17, 2026

Copy link
Copy Markdown
Collaborator

What this fixes

A package or environment that fails to load is skipped rather than fatal, so the server reports operationalState: "serving" and simply omits it. The only signal is a log line. That has hidden real bugs repeatedly: a server can serve one of three configured packages and still look completely healthy.

What this adds

An optional loadErrors array on ServerStatus, listing what did not load and why:

{
  "operationalState": "serving",
  "loadErrors": [
    { "environment": "sales", "message": "Failed to download or mount location: ./sales-models" },
    { "environment": "examples", "package": "storefront", "message": "Package manifest ... does not exist" }
  ]
}

An entry with no package means the whole environment was skipped, which also takes down its healthy packages. An entry with a package means that package alone is missing while its siblings serve.

Scope: signal only

Nothing changes about what loads or what serves. operationalState is deliberately untouched: the control plane maps unrecognised states to HEALTHY, the example compose healthcheck greps the raw "operationalState":"serving" substring, and the Playwright global setup gates on it with a 5 minute ceiling, so reporting a new state on partial load would break more than it fixed. Whether an unmountable location should take down healthy sibling packages is a real question and a separate change.

The field is collected at read time in getStatus from live state, following the existing throttled precedent, so there is no fourth stored status to keep in sync. It is omitted entirely when nothing failed, so a healthy server's status body is byte-for-byte what it was before.

Notes for review

Both boot paths record through one helper. The config path runs on --init or an empty database; the database-restore path runs on every restart of a server with a persisted publisher.db. Recording in only one of them would drop failures on the path production actually takes, so they share a single funnel rather than two call sites that can drift apart.

An environment enters the live map before its database sync runs, so a throw from that tail would otherwise report a serving environment as skipped and contradict the environments list. Only an environment that is genuinely absent is recorded.

Messages pass through the existing redactPgSecrets before reaching the response body. Compiling a model resolves the package's connections, so a Postgres or DuckLake ATTACH failure surfaces here and that command embeds the connection string. Worth knowing, and called out in a comment: the helper only covers keyword-form password=, which is what buildPgConnectionString emits, so a URL-form connectionString supplied verbatim in config still carries its credentials through. The fix belongs in the helper rather than at this call site.

Verification

Full gate green: typecheck, lint, prettier, 1130 unit tests, 207 integration tests. Each fix is pinned by a test that fails without it. Verified live against a running server with a healthy environment, an unmountable environment, and an environment with one bad package: both failure modes reported, siblings still serving, a fixed package cleared by reload, a healthy server omitting the field entirely, and the database-restore path reporting correctly after a restart.

mlennie added 3 commits July 16, 2026 17:40
A package or environment that fails to load is skipped rather than fatal, so
the server reports operationalState "serving" and simply omits it. The only
signal was a log line, which has hidden real bugs repeatedly: a server can
serve one of three packages and still look healthy.

Add an optional loadErrors array to ServerStatus listing what did not load and
why. An entry with no package name means the whole environment was skipped; an
entry with one means that package alone is missing while its siblings serve.

This is signal only. Nothing changes about what loads or serves, and
operationalState is untouched: the control plane maps unknown states to
HEALTHY, the example compose healthcheck greps the serving substring, and the
Playwright setup gates on it, so a new state would break more than it fixed.
The field is collected at read time in getStatus from live state, following the
throttled precedent, and is omitted entirely when nothing failed, so a healthy
server's status body is unchanged.

Both boot paths record failures through one helper. The config path runs on
--init or an empty database; the database-restore path runs on every restart of
a server with a persisted publisher.db, and recording in only one of them would
drop failures on the path production actually takes.

An environment enters the live map before its database sync runs, so a throw
from that tail would otherwise report a serving environment as skipped and
contradict the environments list. Only an environment that is genuinely absent
is recorded.

Messages are redacted with redactPgSecrets before they reach the response body.
Compiling a model resolves the package's connections, so a Postgres or DuckLake
attach failure surfaces here, and that command carries a password.

Signed-off-by: Monty Lennie <[email protected]>
The comment read as though the connection string only ever carries
`password=`, which is the one form redactPgSecrets handles. A reader could
take that as the redaction being complete. It is not: buildPgConnectionString
returns a configured connectionString verbatim, so a URL-form
postgres://user:pw@host keeps its credentials through the helper.

No behavior change. The point is that the next person widens the helper
instead of trusting the call site.

Signed-off-by: Monty Lennie <[email protected]>

@housejester housejester left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Strong PR — the three load-bearing design calls (signal-only, read-time overlay over live state, absent-when-healthy) are each right and each explained honestly in comments, and the tests cover both boot paths including the database-restore one a restarted server actually takes. The findings below are lifecycle edges on the new maps, not design problems.

Mental-model delta

In one sentence: knowledge of configured-but-absent packages moves from ephemeral log lines to a durable, queryable field on ServerStatus — "serving" officially stops implying "complete," and /status becomes the single place that difference is visible.

Dimension Before After
Truth about a failed load log stream only; gone once scrolled in-memory maps (failedEnvironments on the store, failedPackages per env), surfaced at getStatus read time
Failure observability lifetime observable exactly once (the failing load deletes the packageStatuses entry it would be enumerated by) lasting record, cleared on successful load / publish of the same name
operationalState semantics "serving," ambiguously read as "all good" unchanged by design (existing consumers gate on it; right call)
Healthy-server status body as-is byte-for-byte identical — field absent, never []; verified in code, not just claimed

Since the field is optional and additive, JSON consumers that ignore unknown keys (the common case) are unaffected; typed SDK consumers pick it up on their next regen.

Worth acting on

[MED] The new signal can report ghosts: both delete paths early-return before touching the failure maps. A failed load is precisely the case where the early return fires. deletePackage starts if (!this.packages.get(packageName)) return; — a boot-failed package is not in this.packages, so deleting it from config is a silent no-op that leaves the failedPackages entry behind, and loadErrors then reports a package that is no longer configured, until restart or a same-name republish. deleteEnvironment has the identical shape (if (!environment) return;) for a boot-failed environment. This inverts the field's own contract ("configured packages and environments that did not load") in exactly the sequence an operator is most likely to run: fail → give up → remove from config. Suggest failedPackages.delete(packageName) / failedEnvironments.delete(environmentName) ahead of the early returns, plus one test each (fail → delete → loadErrors gone) — the clear-on-success sites already show the pattern.

[MED] The redactPgSecrets URL-form gap deserves a firmer commitment now that these strings are HTTP-bound. The comment flags it honestly (keyword-form password= covered; a URL-form connectionString supplied verbatim in config passes through). What changes the pricing is this PR itself: it's the change that moves those messages from a log line to an HTTP response body — and the docs now actively tell users to curl .loadErrors. Widening the helper feels like a pre-release fast-follow rather than a someday, ideally landed before a release ships this field.

Confirm

[LOW] update() instance semantics vs. package-level failure carryover. addEnvironment's existing-env branch and updateEnvironment both do this.environments.set(name, await existingEnvironment.update(...)) without touching the failure maps. The record-time guard means a live environment can't be sitting in failedEnvironments, so no contradiction there. But at the package level: if update() returns a new Environment instance, the old instance's failedPackages drops silently — whether a still-broken package resurfaces in loadErrors then depends on the new instance re-attempting the load. Can you confirm which way update() goes, and that a still-failing package reappears rather than vanishing after an environment update?

Non-blocking

  • [LOW] Entries carry no timestamp. Fine for the stated scope, but the first status-polling monitor built on this won't be able to distinguish "failed at boot three days ago" from "failed on the publish that just happened." A failedAt per entry is cheap now and annoying to retrofit.
  • Things I verified rather than took on faith, all clean: the not-yet-in-environments guard genuinely prevents the status contradicting its own environments list (and the DB-sync-tail test pins it); the "both boot paths funnel through one helper" claim matches the code; mockDbEnvironments is reset in both setup and teardown against the shared-process leak; the stub-the-prototype-before-constructing race note is correct.

Top three: 1) clear the failure maps in the delete paths; 2) commit the redaction-helper widening as a pre-release fast-follow; 3) confirm update() carryover so a still-broken package can't vanish from loadErrors.

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.

2 participants