Repository navigation
fix: a failed published-atlases fetch can half-generate a tracker atlas and still exit 0 (#3203) - #3239
Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Fatal tracker failures also prevent non-tracker pages from building, contrary to the stated acceptance criterion.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (1)
What changed in this PR
Updates tracker publication fetching to fail builds explicitly instead of silently generating incomplete atlas pages.
Changes:
- Rethrows fetch, HTTP, response, and configuration failures.
- Clears rejected promises to allow development retries.
- Improves failure diagnostics.
| File | Summary |
|---|---|
apis/tracker/api.ts |
Makes published-atlas fetch failures explicit and build-blocking. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
The change does what it sets out to do. None of the four callers of getPublishedAtlases catches the error, so a tracker failure now reliably fails the build. I didn't find anything that lets a build pass when it should fail. The inline comments cover checking the items in the response and making the error message clearer.
One point that's outside the diff: the publish filter (a.tracker ? isTrackerAtlasPublished(...) : true plus the index filter) is copied in utils/networkPages.ts (getContentStaticProps) and utils/availableNetworks.ts (getAvailableNetworks). If the two drift apart, the network pages and the home page will disagree on which atlases are published, which is the same kind of mismatch as #3203. A shared helper such as filterPublishedAtlases(atlases) would prevent that. It could be a follow-up.
|
Re the duplicated publish filter from the review summary: done in da9b63f rather than as a follow-up, since it was small and is the same class of mismatch as #3203. |
NoopDog
left a comment
There was a problem hiding this comment.
Thanks @frano-m, rethrowing instead of degrading makes sense given the worker pool. A few changes before this goes in:
Please fix
- The failure reason probably won't reach the build log (
apis/tracker/api.ts). The second commit moved the underlying error into{ cause: err }. Next's build workers serialize onlyname,messageandstackwhen they send an error back to the main process (PARENT_MESSAGE_CLIENT_ERROR,e.name,e.message,e.stackinnext/dist/compiled/jest-worker), andstackdoesn't includecause. So a 503 orfetch failedwould likely show up as just[tracker] Published atlases are unavailable. Could you put the reason back in the message, e.g.`[tracker] Published atlases are unavailable: ${err}`, and re-run the unreachable-port check? - The PR description quotes the old message. The verification output is from the first commit (
Failed to fetch published atlases; aborting build. TypeError: fetch failed). Please update it after re-running.
Please take a look
- One malformed item fails the whole build.
assertPublishedAtlasesthrows on any bad record in/api/published-atlases, even for an atlas this portal doesn't configure. If that strictness is intended, fine, but checking only the items a configured atlas matches would be narrower. - Atlas pages can still list unpublished siblings (
utils/trackerAtlasPages.ts, andprocessNetworkinutils/atlasPages.ts). Thenetworkprop on an atlas page is built from the unfilterednetwork.atlases, while the network and home pages filter it. An unpublished sibling could show up there with a link to a page that 404s, which is the same kind of disagreement #3203 is about.
Optional cleanups
- The published check is written twice:
atlas.tracker ? isPublishedTrackerAtlas(atlas) : trueinutils/atlasPages.tsand insidefilterPublishedAtlases. One sharedisAtlasPublished(atlas)would stop them drifting apart. filterPublishedAtlasesawaits the promise and scans the list once per atlas. AwaitinggetPublishedAtlases()once and building aSetofshortNameSlug@versionwould do it in one pass.- The gate is now spread across
apis/tracker/api.ts,utils/availableNetworks.tsandutils/atlasPages.ts. Keeping it in one module would make it easier to find.
|
Thanks @NoopDog, addressed in 9c14a09:
Not changed: build workers can still fetch different snapshots if the tracker's published list changes mid-build. Fixing that properly means a prebuild step that writes one snapshot for all workers, which feels out of scope here. Happy to open a follow-up if you want it. |
NoopDog
left a comment
There was a problem hiding this comment.
Requesting changes. Summary of the inline comments:
- The build can still pass with pages that disagree about which atlases are published, if the tracker changes mid-build (per-worker fetches).
- Malformed records for unconfigured atlases fail the whole build.
slug@versionkeys can collide.
4-5. Error-message quality:describeErrorloses AggregateError/nested causes, and the stack points at the catch.
6-7. Simplification: theMapcache isn't needed, and the publications page gates atlases it never renders.
One more that's outside the diff: hooks/useNetworkList.ts, where the gate is still opt-in. NetworkListContext, networkContext and atlasContext all default to the unfiltered NETWORKS, so the new comment's claim that every decision goes through availableNetworks.ts isn't enforced. A future page that renders SectionBioNetworkAtlases or the networks index MainColumn without a NetworkListProvider would quietly show unpublished tracker atlases linking to pages that were never generated. Suggest making the context defaults empty, or exporting only a gated accessor from the constants.
9c14a09 to
90edbe9
Compare
|
Re the |
NoopDog
left a comment
There was a problem hiding this comment.
I found no crash or build-breaking bug. The comments are places where the PR's stated guarantees don't fully hold, plus some cleanup. The main ones: an empty or partly malformed tracker response passes validation (api.ts:249), a failed build leaves a stale .tracker snapshot (package.json:15), and NetworkContext / atlasContext still default to the ungated NETWORKS[0] (useNetworkList.ts).
f1d9922 to
12d2126
Compare
NoopDog
left a comment
There was a problem hiding this comment.
Thanks, this round is a big improvement. Nine of my ten comments are addressed in 12d2126: the per-build snapshot path with the trap cleanup, the NEXT_PHASE check instead of NODE_ENV, the ENOENT-only hint, loading @next/env through next, util.inspect in place of the custom error formatting, dropping the stack-frame heuristic, the empty context defaults, and the inlined predicate.
I'm requesting changes for the one that's left, which is the main one (inline on apis/tracker/api.ts). Everything else looks good to me.
| ); | ||
| return false; | ||
| }); | ||
| if (data.length > 0 && atlases.length === 0) { |
There was a problem hiding this comment.
This is still the open point from my last review. The check only fails when the response has items and none of them is valid. If the tracker returns [] (an empty or staging database, or a tracker bug), every page agrees that no tracker atlases are published, and the build ships without them and exits 0. The same happens if only the configured atlas's record is malformed: it's skipped with a console.warn. That's the silent omission this PR is meant to prevent.
The stronger fix is to check the response against the tracker atlases configured in NETWORKS, so each configured slug and version must be present and valid. A minimal version would be to fail on an empty response. If you'd rather not do either here, please say why in this thread, and open a follow-up issue.
There was a problem hiding this comment.
Agreed that both cases still exit 0. Before I change anything, here's why the two suggested checks don't work as stated, then three options. Which do you want?
Why the suggested checks would fail correct builds. I checked the live tracker today. It publishes only breast@v1.0, while gut@v1.0 and liver@v1.0 are configured in NETWORKS and deliberately unpublished. Hiding those is the gate's job.
- Requiring every configured slug and version to be present would fail every build today.
- A plain "fail on empty response" would fail every deploy once
breastis unpublished, which is a legitimate state.
Options
- A. No code change, plus a follow-up issue. Leave the check as it is and open an issue for a stronger check.
- B. Partial fix, covering both of your cases:
- Fail on an empty response unless the build sets an explicit override, e.g.
ALLOW_NO_PUBLISHED_ATLASES=true. The error message would name the override, so unpublishing the last atlas is a deliberate step rather than a silent omission. - Fail when a malformed record's
shortNameSlugmatches a configured tracker atlas, instead of skipping it with a warning. Unrelated malformed records would still only warn. This means moving the{ shortNameSlug, version }tracker configs out ofconstants/networks.tsinto a module with no MDX imports, so the snapshot script can import it (esruncan't load the MDX thatNETWORKSpulls in).networks.tswould then reference that module.
- Gap: a configured atlas's record that is missing
shortNameSlugentirely can't be matched to it. It would still only warn, unless every item is malformed, which already fails the build.
- Fail on an empty response unless the build sets an explicit override, e.g.
- C. Only B.1 (the empty-response check with the override), plus a follow-up issue for the malformed-configured-record case.
My recommendation is B. It makes both of your cases fail loudly without breaking today's builds. The cost is the small config extraction and one env var to set on the rare build where the tracker should have nothing published.
There was a problem hiding this comment.
Thanks for checking the live tracker, Fran. That settles it: we'll trust the tracker. Whatever it says is published, including nothing, is authoritative. So the empty-response and malformed-record cases are accepted by design, and there's no code change here and no follow-up issue.
…as and still exit 0 (#3203) Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AsKeyxfv4ZT3FdQaqtGBdV
…the publish filter (#3203) check that every published-atlases item has string id, shortnameslug and version so a renamed field fails the build instead of hiding every atlas. rethrow with the original error as `cause` and a message that suits both build and dev. extract filterpublishedatlases so the network pages and the home page network list cannot disagree on which atlases are published. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AsKeyxfv4ZT3FdQaqtGBdV
…e place (#3203) Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…dback (#3203) Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…logging (#3203) write the published-atlases snapshot from scripts/build.sh to a per-build temp path passed via published_atlases_snapshot, delete it however the build ends, and read it only during next build. load @next/env through next, print errors with util.inspect, default network contexts to no atlases, and inline the tracker-only static paths predicate. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
12d2126 to
05b28d9
Compare

Summary
Closes #3203
getPublishedAtlasesused to swallow a failed/api/published-atlasesfetch, return[], and clear its cached promise, so callers within one build could disagree on whether an atlas was published and ship a half-generated atlas with tabs that 404, all with exit 0.This departs from the issue's suggested fix (cache the empty result for the build). That cannot work:
next buildrunsgetStaticPathsandgetStaticPropsacross a pool of worker processes, each with its own module-level cache, so no in-process cache can make every caller agree.Instead the gate now rethrows on any failure (network error, non-2xx, non-array body, missing
NEXT_PUBLIC_ATLAS_TRACKER_URL), so the build aborts deterministically with a clear[tracker]error. The rejected promise is cleared sonext devcan retry on the next request.The resilience this gives up was already thin: with
output: "export", any failure in the other tracker fetches (component atlases, source datasets) already failed the whole build. The gate was the only tracker call that degraded.The publication gate lives in
utils/availableNetworks.ts(isAtlasPublished,filterPublishedAtlases,getAvailableNetwork), and every page that serializes a network (home, networks index, network, publications, atlas pages) builds it from the filtered list.apis/tracker/api.tsonly fetches, validates and caches: every item must carry a stringshortNameSlugandversion, whileidis checked only for an atlas the portal resolves.Verification
build-dev:data-portal) pass.cause):🤖 Generated with Claude Code
https://claude.ai/code/session_01AsKeyxfv4ZT3FdQaqtGBdV