Skip to content

fix: a failed published-atlases fetch can half-generate a tracker atlas and still exit 0 (#3203) - #3239

Merged
frano-m merged 5 commits into
mainfrom
fran/3203-published-atlases-cache
Oct 9, 2026
Merged

frano-m merged 5 commits into
mainfrom
fran/3203-published-atlases-cache

Conversation

@frano-m

@frano-m frano-m commented Sep 21, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Closes #3203

getPublishedAtlases used to swallow a failed /api/published-atlases fetch, 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 build runs getStaticPaths and getStaticProps across 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 so next dev can 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.ts only fetches, validates and caches: every item must carry a string shortNameSlug and version, while id is checked only for an atlas the portal resolves.

Verification

  • Lint, type check, and production build (build-dev:data-portal) pass.
  • Pointing the tracker URL at a closed port fails the build, with the underlying reason in the message (Next's build workers drop cause):
    Error: [tracker] Published atlases are unavailable: TypeError: fetch failed (cause: Error: connect ECONNREFUSED 127.0.0.1:59999)
    > Build error occurred
    

🤖 Generated with Claude Code

https://claude.ai/code/session_01AsKeyxfv4ZT3FdQaqtGBdV

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 High severity

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.

Comment thread apis/tracker/api.ts Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟢 Approval recommended

No unresolved review issues were identified.

Review effort: Lite
Findings: None

Resolved since last review (1)

@NoopDog NoopDog 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.

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.

Comment thread apis/tracker/api.ts Outdated
Comment thread apis/tracker/api.ts Outdated
@frano-m

frano-m commented Sep 24, 2026

Copy link
Copy Markdown
Contributor Author

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. filterPublishedAtlases(atlases) in utils/availableNetworks.ts is now used by both getAvailableNetworks and networkPages.getContentStaticProps.

@NoopDog NoopDog 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.

Thanks @frano-m, rethrowing instead of degrading makes sense given the worker pool. A few changes before this goes in:

Please fix

  1. 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 only name, message and stack when they send an error back to the main process (PARENT_MESSAGE_CLIENT_ERROR,e.name,e.message,e.stack in next/dist/compiled/jest-worker), and stack doesn't include cause. So a 503 or fetch failed would 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?
  2. 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

  1. One malformed item fails the whole build. assertPublishedAtlases throws 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.
  2. Atlas pages can still list unpublished siblings (utils/trackerAtlasPages.ts, and processNetwork in utils/atlasPages.ts). The network prop on an atlas page is built from the unfiltered network.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

  1. The published check is written twice: atlas.tracker ? isPublishedTrackerAtlas(atlas) : true in utils/atlasPages.ts and inside filterPublishedAtlases. One shared isAtlasPublished(atlas) would stop them drifting apart.
  2. filterPublishedAtlases awaits the promise and scans the list once per atlas. Awaiting getPublishedAtlases() once and building a Set of shortNameSlug@version would do it in one pass.
  3. The gate is now spread across apis/tracker/api.ts, utils/availableNetworks.ts and utils/atlasPages.ts. Keeping it in one module would make it easier to find.

@frano-m

frano-m commented Sep 28, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @NoopDog, addressed in 9c14a09:

  1. Failure reason in the build log. The reason now goes in the message rather than cause. The underlying error's own cause is appended too, because ${err} alone gives only TypeError: fetch failed. Re-ran against a closed port:
    Error: [tracker] Published atlases are unavailable: TypeError: fetch failed (cause: Error: connect ECONNREFUSED 127.0.0.1:59999)
    
  2. PR description. Updated with the new output.
  3. Strictness. Narrowed partly. Every item must still carry a string shortNameSlug and version, because checking only matched items can't catch a renamed match field: nothing matches, so every tracker atlas would silently read as unpublished (the failure this check exists for). id is now checked only in resolveTrackerAtlas, so a bad id on an atlas this portal doesn't configure no longer fails the build.
  4. Unpublished siblings. Atlas pages (tracker and non-tracker) now build network from the filtered list. The atlas itself is still looked up in the configured network, so a paths/props mismatch hits the clear "No published atlas found" error rather than an undefined atlas. The network publications page had the same gap and is filtered too.
  5. One published check. It's now isAtlasPublished(atlas), shared by getStaticPaths, filterPublishedAtlases and the tracker-only paths.
  6. One pass. The cache is now a Map keyed by shortNameSlug@version, built once, and isTrackerAtlasPublished and resolveTrackerAtlas are both lookups against it. Duplicate records keep the first one, matching the old .find.
  7. One module. The Atlas-level gate lives in utils/availableNetworks.ts (isAtlasPublished, isPublishedTrackerAtlas, filterPublishedAtlases, getAvailableNetwork/getAvailableNetworks). apis/tracker/api.ts only fetches, validates and caches.

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 NoopDog 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.

Requesting changes. Summary of the inline comments:

  1. The build can still pass with pages that disagree about which atlases are published, if the tracker changes mid-build (per-worker fetches).
  2. Malformed records for unconfigured atlases fail the whole build.
  3. slug@version keys can collide.
    4-5. Error-message quality: describeError loses AggregateError/nested causes, and the stack points at the catch.
    6-7. Simplification: the Map cache 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.

Comment thread apis/tracker/api.ts Outdated
Comment thread apis/tracker/api.ts Outdated
Comment thread apis/tracker/api.ts Outdated
Comment thread apis/tracker/api.ts Outdated
Comment thread apis/tracker/api.ts Outdated
Comment thread apis/tracker/api.ts Outdated
Comment thread utils/networkPublicationPages.ts Outdated
@NoopDog NoopDog assigned frano-m and unassigned NoopDog Sep 28, 2026
@frano-m
frano-m force-pushed the fran/3203-published-atlases-cache branch from 9c14a09 to 90edbe9 Compare September 29, 2026 06:51
@frano-m

frano-m commented Sep 29, 2026

Copy link
Copy Markdown
Contributor Author

Re the hooks/useNetworkList.ts point from the review summary: done in f1d9922. NetworkListContext now defaults to [], so a page that renders SectionBioNetworkAtlases or the networks index MainColumn without a NetworkListProvider lists nothing rather than unpublished tracker atlases. I left networkContext and atlasContext as they are: their defaults are single placeholders (NETWORKS[0]), not atlas lists, and every page that uses them provides a value. I reworded the gate comment in utils/availableNetworks.ts to say callers should go through it, and that the empty default is what catches a page that skips it.

@NoopDog NoopDog 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.

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).

Comment thread apis/tracker/api.ts
Comment thread package.json Outdated
Comment thread hooks/useNetworkList.ts
Comment thread apis/tracker/api.ts Outdated
Comment thread apis/tracker/api.ts Outdated
Comment thread apis/tracker/constants.ts Outdated
Comment thread package.json Outdated
Comment thread apis/tracker/api.ts Outdated
Comment thread apis/tracker/api.ts Outdated
Comment thread utils/availableNetworks.ts Outdated
@frano-m
frano-m force-pushed the fran/3203-published-atlases-cache branch from f1d9922 to 12d2126 Compare October 5, 2026 06:04
@frano-m frano-m assigned NoopDog and unassigned frano-m Oct 6, 2026

@NoopDog NoopDog 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.

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.

Comment thread apis/tracker/api.ts
);
return false;
});
if (data.length > 0 && atlases.length === 0) {

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.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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 breast is 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:
    1. 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.
    2. Fail when a malformed record's shortNameSlug matches 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 of constants/networks.ts into a module with no MDX imports, so the snapshot script can import it (esrun can't load the MDX that NETWORKS pulls in). networks.ts would then reference that module.
    • Gap: a configured atlas's record that is missing shortNameSlug entirely can't be matched to it. It would still only warn, unless every item is malformed, which already fails the build.
  • 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.

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.

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.

frano-m and others added 5 commits October 9, 2026 15:50
…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>
@frano-m
frano-m force-pushed the fran/3203-published-atlases-cache branch from 12d2126 to 05b28d9 Compare October 9, 2026 06:00
@frano-m
frano-m merged commit ebc6f7d into main Oct 9, 2026
1 check passed
@frano-m
frano-m deleted the fran/3203-published-atlases-cache branch October 9, 2026 06:03
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.

fix: a failed published-atlases fetch can half-generate a tracker atlas and still exit 0

3 participants