yuris/garage

Fork 0

store: bare catch in fetchCatalog relabels every error as "Catalog endpoint is unavailable", masking the static-gen bailout #66

New issue

Closed

opened 2026-08-28 01:51:28 +00:00 by yuris · 2 comments

yuris commented

2026-08-28 01:51:28 +00:00

Owner

Found while fixing #64 (PR #65). Filed rather than folded into that PR, which is scoped to catalog cache freshness.

The defect

apps/store/lib/catalog.ts wraps the catalog fetch in a bare catch that discards the caught value:

try {
  response = await request(url, { headers: { "x-api-key": key }, cache: "force-cache" });
} catch {
  throw new Error("Catalog endpoint is unavailable");
}

The intent is good and should be preserved: the original error may carry the API key or internal network detail, and #47-era work deliberately kept credentials out of build logs. But the catch is unconditional, so every throw from that await — not just a network failure — is relabelled as an endpoint outage.

Why this is worth fixing, concretely

Next's static-generation bailout is thrown from inside that await. While measuring #64 I set cache: "no-store" and the build failed with:

Error occurred prerendering page "/cart".
Error: Catalog endpoint is unavailable

The actual error, recovered only by temporarily logging the caught value, was:

Error: Route /cart with `dynamic = "error"` couldn't be rendered statically
because it used `revalidate: 0 fetch http://.../api/public/catalog /cart`.
{ code: 'NEXT_STATIC_GEN_BAILOUT' }

Those two messages send you to completely different places. The real one names the caching constraint and links the fix; the one actually printed says the management API is down, which sends you to check the container, the API key, and the network — none of which are involved.

This is not hypothetical. That constraint (output: "export" forbids an uncached fetch) is precisely what dictates the caching design in #64, so the next person to touch caching here is the most likely person to hit it, and they will be handed the least useful possible message.

Suggested fix

Keep redacting genuine network failures; stop swallowing framework control-flow errors. A NEXT_STATIC_GEN_BAILOUT (and Next's DynamicServerError) is generated by the framework, contains no credentials, and must reach the build log intact. Rethrowing on that code while keeping the current redaction for everything else is enough.

Worth checking whether the same shape guards the other two catch blocks in the same function (response.json() → "Catalog response is not valid JSON"); that one is narrower and probably fine, but it should be looked at in the same pass rather than left as the twin.

Acceptance

A static-generation bailout raised inside fetchCatalog surfaces with its own message and code in the build output, while a real network failure still fails with a message that does not contain CATALOG_API_KEY or internal host detail. Existing coverage in apps/store/lib/catalog.test.ts already asserts the redaction behaviour and must keep passing.

Found while fixing #64 (PR #65). Filed rather than folded into that PR, which is scoped to catalog cache freshness. ## The defect `apps/store/lib/catalog.ts` wraps the catalog fetch in a bare `catch` that discards the caught value: ```ts try { response = await request(url, { headers: { "x-api-key": key }, cache: "force-cache" }); } catch { throw new Error("Catalog endpoint is unavailable"); } ``` The intent is good and should be preserved: the original error may carry the API key or internal network detail, and #47-era work deliberately kept credentials out of build logs. But the `catch` is unconditional, so **every** throw from that `await` — not just a network failure — is relabelled as an endpoint outage. ## Why this is worth fixing, concretely Next's static-generation bailout is thrown from inside that `await`. While measuring #64 I set `cache: "no-store"` and the build failed with: ``` Error occurred prerendering page "/cart". Error: Catalog endpoint is unavailable ``` The actual error, recovered only by temporarily logging the caught value, was: ``` Error: Route /cart with `dynamic = "error"` couldn't be rendered statically because it used `revalidate: 0 fetch http://.../api/public/catalog /cart`. { code: 'NEXT_STATIC_GEN_BAILOUT' } ``` Those two messages send you to completely different places. The real one names the caching constraint and links the fix; the one actually printed says the management API is down, which sends you to check the container, the API key, and the network — none of which are involved. This is not hypothetical. That constraint (`output: "export"` forbids an uncached fetch) is precisely what dictates the caching design in #64, so the next person to touch caching here is the most likely person to hit it, and they will be handed the least useful possible message. ## Suggested fix Keep redacting genuine network failures; stop swallowing framework control-flow errors. A `NEXT_STATIC_GEN_BAILOUT` (and Next's `DynamicServerError`) is generated by the framework, contains no credentials, and must reach the build log intact. Rethrowing on that `code` while keeping the current redaction for everything else is enough. Worth checking whether the same shape guards the other two `catch` blocks in the same function (`response.json()` → `"Catalog response is not valid JSON"`); that one is narrower and probably fine, but it should be looked at in the same pass rather than left as the twin. ## Acceptance A static-generation bailout raised inside `fetchCatalog` surfaces with its own message and `code` in the build output, while a real network failure still fails with a message that does not contain `CATALOG_API_KEY` or internal host detail. Existing coverage in `apps/store/lib/catalog.test.ts` already asserts the redaction behaviour and must keep passing.

yuris referenced this issue

2026-08-28 01:51:55 +00:00

fix(store): key the catalog fetch on a per-build id so rebuilds cannot serve a stale catalog #65

yuris referenced this issue

2026-08-28 02:04:45 +00:00

fix(store): key the catalog fetch on a per-build id so rebuilds cannot serve a stale catalog #65

yuris commented

2026-08-28 02:04:58 +00:00

Author

Owner

Adding the recommended fix shape, from the review of PR #65 (review id=24), plus the strongest argument for prioritising this.

Recommended shape

Rethrow anything carrying a Next digest or bailout code; keep the friendly redacted message for genuine network faults. Framework control-flow errors are generated by Next, contain no credentials, and must reach the build log intact.

Why this is worth more than it looks

The reviewer of #65 hit this independently, and it cost real work in both directions:

It cost a build to discover that the one constraint the whole #65 design rests on (output: "export" forbids an uncached fetch) presents as a network outage. The reviewer had to delete the catch to recover the real error, exactly as I did.
It corrupted the evidence block of the PR that fixes the bug it masks. #65's body originally quoted the NEXT_STATIC_GEN_BAILOUT text as verbatim build output. It is not what the build prints at that commit — the mask rewrites it. That needed a correction round on #65.

So this defect has now independently misled two agents on the same constraint, and made a PR body wrong. That is the argument for fixing it.

Sibling to check in the same pass

fetchCatalog has a second catch around response.json() producing "Catalog response is not valid JSON". It is narrower and probably fine, but it should be looked at in the same pass rather than left as the twin.

Adding the recommended fix shape, from the review of PR #65 (review id=24), plus the strongest argument for prioritising this. ## Recommended shape Rethrow anything carrying a Next `digest` or bailout `code`; keep the friendly redacted message for genuine network faults. Framework control-flow errors are generated by Next, contain no credentials, and must reach the build log intact. ## Why this is worth more than it looks The reviewer of #65 hit this independently, and it cost real work in both directions: - It **cost a build to discover** that the one constraint the whole #65 design rests on (`output: "export"` forbids an uncached fetch) presents as a network outage. The reviewer had to delete the `catch` to recover the real error, exactly as I did. - It **corrupted the evidence block of the PR that fixes the bug it masks**. #65's body originally quoted the `NEXT_STATIC_GEN_BAILOUT` text as verbatim build output. It is not what the build prints at that commit — the mask rewrites it. That needed a correction round on #65. So this defect has now independently misled two agents on the same constraint, and made a PR body wrong. That is the argument for fixing it. ## Sibling to check in the same pass `fetchCatalog` has a second `catch` around `response.json()` producing `"Catalog response is not valid JSON"`. It is narrower and probably fine, but it should be looked at in the same pass rather than left as the twin.

yuris referenced this issue

2026-08-28 02:05:21 +00:00

fix(store): key the catalog fetch on a per-build id so rebuilds cannot serve a stale catalog #65

yuris referenced this issue

2026-08-28 02:18:40 +00:00

management: the API has no error handler, so unauthenticated writes answer 500 instead of 401 #70

yuris commented

2026-08-28 02:36:44 +00:00

Author

Owner

Evidence of cost: this masking has now caused the same string to be mis-stated three times, across three different artifacts

Recorded while PR #65 was in review, because it is the strongest available argument for fixing this and it is not obvious from reading the code.

NEXT_STATIC_GEN_BAILOUT is the .code on Next's StaticGenBailoutError (next/dist/client/components/static-generation-bailout.js:23). It is never part of any printed message. The bare catch in apps/store/lib/catalog.ts rewrites the throw into "Catalog endpoint is unavailable", so nobody investigating ever sees the real error, and three separate attempts to describe it were each wrong in a different way:

PR #65 round 1 — both the code comment and the PR body cited the constant as the observed failure output. Caught by review; the observed output is "Catalog endpoint is unavailable".
PR #65 round 2 — the corrected comment then claimed that deleting the catch reveals the constant. It does not: Next prints the dynamic = "error" / revalidate: 0 message and the constant stays on .code. Caught by review.
PR #65 round 3 — the PR body still quoted { code: 'NEXT_STATIC_GEN_BAILOUT' } inside a produced-output block. Caught by review.

Three behaviours that are genuinely distinct, which is why this kept going wrong:

What you do to the catch	What you actually see
leave it intact (current `main`)	`Error: Catalog endpoint is unavailable`
delete it	the `dynamic = "error"` / `revalidate: 0` message, alone
instrument it with `console.error(e)`	that message plus a `{ code: 'NEXT_STATIC_GEN_BAILOUT' }` line — because Node's inspector appends own enumerable properties to an error object, not because the build printed it

The third row is the trap: it looks like build output and is not. Two engineers investigating the same failure by slightly different methods get three different impressions, and none of them is the plain truth that the constant is a property and never a message.

Why this matters beyond tidiness: the masked error is a static-generation bailout, which is precisely the constraint that dictates the caching design in catalog.ts. So the next person to touch caching here is the most likely person to hit it, and they will be told the API is down. That is not hypothetical — it happened twice in one wave, to an author and a reviewer working on the file the catch lives in.

Recommended shape (unchanged, restated with the reason)

Rethrow anything carrying a Next digest or bailout code; keep the friendly redacted message for genuine network faults. Check the sibling catch on response.json() in the same function in the same pass — it is the twin, and this codebase has a track record of fixing one instance and leaving its twin.

Whoever fixes this: make it fail on purpose once and confirm the real error reaches the console, rather than assuming the rethrow works.

## Evidence of cost: this masking has now caused the same string to be mis-stated three times, across three different artifacts Recorded while PR #65 was in review, because it is the strongest available argument for fixing this and it is not obvious from reading the code. `NEXT_STATIC_GEN_BAILOUT` is the `.code` on Next's `StaticGenBailoutError` (`next/dist/client/components/static-generation-bailout.js:23`). **It is never part of any printed message.** The bare `catch` in `apps/store/lib/catalog.ts` rewrites the throw into `"Catalog endpoint is unavailable"`, so nobody investigating ever sees the real error, and three separate attempts to describe it were each wrong in a different way: 1. **PR #65 round 1** — both the code comment and the PR body cited the constant as the *observed* failure output. Caught by review; the observed output is `"Catalog endpoint is unavailable"`. 2. **PR #65 round 2** — the corrected comment then claimed that *deleting* the catch reveals the constant. It does not: Next prints the `dynamic = "error"` / `revalidate: 0` message and the constant stays on `.code`. Caught by review. 3. **PR #65 round 3** — the PR body still quoted `{ code: 'NEXT_STATIC_GEN_BAILOUT' }` inside a produced-output block. Caught by review. Three behaviours that are genuinely distinct, which is why this kept going wrong: | What you do to the catch | What you actually see | |---|---| | leave it intact (current `main`) | `Error: Catalog endpoint is unavailable` | | delete it | the `dynamic = "error"` / `revalidate: 0` message, alone | | instrument it with `console.error(e)` | that message **plus** a `{ code: 'NEXT_STATIC_GEN_BAILOUT' }` line — because Node's inspector appends own enumerable properties to an error **object**, not because the build printed it | The third row is the trap: it looks like build output and is not. Two engineers investigating the same failure by slightly different methods get three different impressions, and none of them is the plain truth that the constant is a property and never a message. **Why this matters beyond tidiness:** the masked error is a static-generation bailout, which is precisely the constraint that dictates the caching design in `catalog.ts`. So the next person to touch caching here is **the most likely person to hit it**, and they will be told the API is down. That is not hypothetical — it happened twice in one wave, to an author and a reviewer working on the file the catch lives in. ## Recommended shape (unchanged, restated with the reason) Rethrow anything carrying a Next digest or bailout code; keep the friendly redacted message for genuine network faults. Check the sibling `catch` on `response.json()` in the same function in the same pass — it is the twin, and this codebase has a track record of fixing one instance and leaving its twin. Whoever fixes this: **make it fail on purpose once** and confirm the real error reaches the console, rather than assuming the rethrow works.

yuris referenced this issue

2026-08-28 02:59:34 +00:00

fix(store): key the catalog fetch on a per-build id so rebuilds cannot serve a stale catalog #65

yuris referenced this issue from a pull request that will close it,

2026-08-29 06:25:36 +00:00

fix(store): rethrow Next bailout and digest errors instead of relabelling them as a catalog outage #75

yuris referenced this issue

2026-08-29 06:36:41 +00:00

fix(management): map thrown API errors to readable statuses instead of a bare 500 #76

yuris referenced this issue

2026-08-29 06:38:01 +00:00

management: create_purchase echoes raw drizzle SQL to the MCP client #77