fix(approuter): eliminate legacy-redirects bootstrap/refresh index race - #1636
Merged
Merged
Conversation
test/unit/legacy-redirects-loader.test.js flaked intermittently on CI (seen on the #1626 and #1627 main runs): "expected undefined to be '/topics/abap-platform.html'". Passes in isolation; fails only under CI parallel load. Root cause: the module-load bootstrap IIFE and refresh() both write the SAME shared _index after awaiting a dynamic import(). #1311/#1409 tried to timing-guard this with a _loadedFromSrv flag that makes the bootstrap a no-op once refresh() wins, but under CI scheduler ordering the shared variable could still end up pointing at the 3-row BOOTSTRAP_MAP, so getIndex() returned an index without /abap. Fix structurally rather than with a timing guard: the bootstrap seed and the live rows live in SEPARATE variables (_bootstrapIndex / _liveIndex); getIndex() returns _liveIndex once _loadedFromSrv is set, else the seed. _liveIndex is assigned before the flag (no await between), so any reader that observes the flag already sees populated live rows, and the bootstrap can never clobber live rows no matter when its import settles. Also closes the boot window where the approuter briefly served only the 3 bootstrap redirects.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
test/unit/legacy-redirects-loader.test.jshas been flaking on CImainruns — e.g. the #1626 merge run and the #1627 merge run both failed on it:It passes in isolation and green on re-run — a classic CI-only async race (documented in #1311 / #1409).
Root cause
In
approuter/lib/legacy-redirects-loader.js, the module-load bootstrap IIFE andrefresh()both write the same shared_indexafter eachawaits a dynamicimport(). #1311/#1409 tried to timing-guard this with a_loadedFromSrvflag that makes the bootstrap a no-op oncerefresh()wins — but two producers mutating one variable acrossawaitpoints remains schedule-dependent, and under CI parallel load_indexcould still end up pointing at the 3-rowBOOTSTRAP_MAP.getIndex()then returned an index without/abap→resolveRedirect('/abap')isundefined.Fix
Remove the shared mutable target instead of timing-guarding it:
_bootstrapIndex/_liveIndex.getIndex()returns_loadedFromSrv ? _liveIndex : _bootstrapIndex._liveIndexis assigned before_loadedFromSrv(noawaitbetween), so any reader that observes the flag already sees populated live rows._bootstrapIndex, so it can never clobber live rows regardless of when itsimport()settles.This also closes the production boot window (noted in #1409) where the approuter briefly served only the 3 bootstrap redirects.
Why no new timing test
The failure is inherently CI-scheduler-dependent and not locally reproducible; the old
_loadedFromSrvguard would also pass a naive "bootstrap-after-refresh" unit test, so such a test gives false confidence. The change is structural — there is no longer a shared variable two producers race on. The existing 3 loader tests (+ 23 related redirect tests) pass deterministically against the new design.Verification
npx vitest run --project unit test/unit/legacy-redirects-loader.test.js test/unit/legacy-redirects-resolver.test.js test/unit/catalog-legacy-redirects.test.js→ 26 passed.Separate from the merged #1626 (island-manifest guard fix).