Skip to content

fix(server): a bad package location drops that package, not the environment#903

Open
mlennie wants to merge 1 commit into
monty/surface-failed-package-loadsfrom
monty/isolate-package-failures
Open

fix(server): a bad package location drops that package, not the environment#903
mlennie wants to merge 1 commit into
monty/surface-failed-package-loadsfrom
monty/isolate-package-failures

Conversation

@mlennie

@mlennie mlennie commented Jul 17, 2026

Copy link
Copy Markdown
Collaborator

Stacks on #898 (uses its per-package loadErrors channel), so please review and merge this after #898.

What

A package whose location cannot be mounted (a path that does not exist, a ~/ typo, an unreachable URL) used to abort the entire environment: loadEnvironmentIntoDisk re-threw, addConfiguredEnvironment caught it and skipped the whole environment, and every healthy sibling package went down with it, while the server still reported serving. A bad manifest (a mounted directory with no publisher.json, or a compile error) has always been isolated to just that one package. Same class of config mistake, opposite blast radius.

This makes the mount path match the manifest path: when a location fails to mount, log it and continue instead of re-throwing. The environment is still created with the packages that mounted, and each package that failed to mount is reported per-package in /api/v0/status loadErrors (the channel #898 added) rather than the whole environment vanishing.

Why

It is the same principle as #890: a failure in one thing must not destroy what already works. It is also easy to hit in practice: one typo in a config path took down three healthy packages. We found it dogfooding, where a fresh clone served one of three packages while looking healthy.

The change

One behavioral change in environment_store.ts: the per-location mount catch no longer re-throws PackageNotFoundError; it logs and continues the loop. No new plumbing was needed, the configured packages are seeded into packageStatuses from config (not scanned from disk), so a package whose directory is absent is still enumerated and its lazy load records the per-package failure that getFailedPackages() surfaces.

Environment-wide failures (a bad connection, an unresolvable environment path) still skip the environment and still report at the environment level; only per-package mount failures move to per-package.

Verification

Full root gate green (typecheck, lint, prettier, 1164 unit / 222 integration, 0 fail). Two existing tests that asserted the old "a missing path skips the whole environment" behavior were updated to the new behavior; the environment-level connection-failure test is unchanged and still passes. Added a test that a bad location alongside two good packages leaves both good ones serving and reports the bad one per-package. Verified live against a running server: bad location plus three good packages leaves all three serving with a per-package loadError; a healthy config reports no loadErrors.

One caveat worth knowing

Per-package loadErrors are collected on the first-boot path (config / --init / empty-db fallback) and are not durable across a restart that restores from a populated publisher.db. This is pre-existing and symmetric with the bad-manifest case (failed packages are not persisted either way), and this change makes a restart strictly better, since the healthy siblings now survive instead of the whole environment being skipped. Flagging it only so the per-package signal's scope is clear.

…onment

A package whose location could not be mounted (a path that does not exist, a
`~/` typo, an unreachable URL) aborted the entire environment: loadEnvironmentIntoDisk
re-threw, addConfiguredEnvironment caught it and skipped the environment, and
every healthy sibling package went down with it. Meanwhile a bad *manifest*
(a mounted directory with no publisher.json, or a compile error) has always been
isolated per-package. Same class of user error, opposite blast radius.

Make the mount path match the manifest path: when a location fails to mount,
log it and continue instead of re-throwing. The environment is still created
with the packages that mounted; each package that failed to mount keeps its
configured status entry, whose lazy load then records a per-package failure that
getFailedPackages() surfaces in /api/v0/status loadErrors (added in the parent
PR). So a bad location now behaves exactly like a bad manifest: one package
dropped, siblings unaffected, and the failure named per-package rather than the
whole environment vanishing while the server still reports "serving".

Environment-level failures that are genuinely environment-wide (a bad connection,
an unresolvable environment path) still skip the environment and report at the
environment level; only per-package mount failures move to per-package.

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

Copy link
Copy Markdown
Collaborator

Hey — nice fix on the blast radius. One bad location taking down every sibling was a rough dogfood footgun, and making mount failures match the bad-manifest behavior is the right call.

Two things before merge:

1. loadErrors tells the wrong story

Today the catch logs the useful error, then throws it away:

Failed to mount local directory: /non/existent/path

The package still gets seeded as SERVING, so the later lazy load fails for a different reason — missing publisher.json under publisher_data/. So /status ends up looking like:

{
  "package": "bad",
  "message": "Package manifest for .../publisher_data/local/bad does not exist."
}

…when the real problem was "location": "~/typo-packages/sales".

Someone staring at status will go hunting for a missing manifest, not a typo’d path.

Suggestion: when the mount/download catch fires, remember that error per package and put that message on failedPackages (after Environment.create), instead of waiting for the “manifest missing” fallout.

2. Packages that share a location still sink together

The catch wraps both the shared download and the whole extract loop. So this config:

{
  "packages": [
    { "name": "storefront", "location": "https://github.com/org/repo/tree/main/examples/storefront" },
    { "name": "broken", "location": "https://github.com/org/repo/tree/main/examples/does-not-exist" },
    { "name": "html-data-app", "location": "https://github.com/org/repo/tree/main/examples/html-data-app" }
  ]
}

Clone succeeds, then extracting broken throws → html-data-app never gets copied, even though it’s fine. Distinct locations are isolated (the headline case); same-repo siblings aren’t yet.

Suggestion: keep the group-level catch for the shared download, and put a per-package try/catch around the extract/copy body.


Otherwise this looks good — headline test is solid, env-wide failures still skip the env, and the restart/durability caveat in the PR body is helpful. Happy to re-review after the above.

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