fix(seed-bigmac): parallelize 50-country EXA loop to stop 240s-deadline crashes (#4994)#4995
Conversation
…adline crashes (#4994) The Big Mac seeder fetched all 50 countries SEQUENTIALLY under runSeed's 240s fetch-phase deadline. Whenever EXA latency exceeded ~4.8s/country (240s/50) the run breached the deadline and exited 75 — a spurious "Deploy Crashed!" alert every tick, no data lost but 240s of compute burned. Run the country loop with BOUNDED CONCURRENCY (6) via the existing allSettledWithConcurrency helper (now exported from _seed-utils.mjs). Worst case is ceil(50/6)=9 waves x 15s ~= 135s, comfortably under the deadline. Per-country failures already degrade to available:false, so the run now exits 0 with partial data; only a total EXA outage trips the graceful path. (The old 150ms inter-call throttle is dropped — the concurrency cap of 6 is the rate limiter.) Also guards the seeder's top-level execution behind an isMain check so the core is importable, and adds tests/bigmac-seed.test.mjs (concurrency regression + country-order preservation + single-country-failure isolation). Claude-Session: https://claude.ai/code/session_01XKZZy7bzUNTZeJDVWgXbjj
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Greptile SummaryThis PR fixes recurring Railway "Deploy Crashed!" alerts in
Confidence Score: 4/5Safe to merge. The fix directly addresses the deadline crash with a well-understood concurrency primitive already used elsewhere in the codebase, and the behavior change (dropping the 150ms delay in favour of a concurrency cap) is intentional and documented. The concurrency change is correct: allSettledWithConcurrency preserves index order, per-country EXA failures are isolated inside processCountry's try/catch (the rejected fallback in settled.map is unreachable but harmless), and the isMain guard follows the established repo idiom. Both findings are in the test file only and do not affect production behaviour. tests/bigmac-seed.test.mjs — the timing-ratio assertion and the currency-uniqueness assumption in the failure test are worth a quick look before merging. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Runner as runSeed (240s deadline)
participant FBP as fetchBigMacPrices
participant ASWC as allSettledWithConcurrency(6)
participant PC as processCountry x50
participant EXA as EXA API
Runner->>FBP: fetchBigMacPrices(prevSnapshot)
FBP->>FBP: getFxRatesFn(FX_SYMBOLS, FX_FALLBACKS)
FBP->>ASWC: "items=COUNTRIES, concurrency=6"
note over ASWC: 6 workers drain shared index
loop up to 9 waves of 6 concurrent
ASWC->>PC: processCountry(country, fxRates, searchExaFn)
PC->>EXA: searchExa(query, SPECIALIST_SITES) 15s timeout
EXA-->>PC: results or error caught available:false
PC-->>ASWC: status fulfilled or rejected
end
ASWC-->>FBP: settled[50]
FBP->>FBP: map results, compute WoW/cheapest/mostExpensive
FBP-->>Runner: countries, fetchedAt, wowAvgPct
Runner->>Runner: validate and write to Redis
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant Runner as runSeed (240s deadline)
participant FBP as fetchBigMacPrices
participant ASWC as allSettledWithConcurrency(6)
participant PC as processCountry x50
participant EXA as EXA API
Runner->>FBP: fetchBigMacPrices(prevSnapshot)
FBP->>FBP: getFxRatesFn(FX_SYMBOLS, FX_FALLBACKS)
FBP->>ASWC: "items=COUNTRIES, concurrency=6"
note over ASWC: 6 workers drain shared index
loop up to 9 waves of 6 concurrent
ASWC->>PC: processCountry(country, fxRates, searchExaFn)
PC->>EXA: searchExa(query, SPECIALIST_SITES) 15s timeout
EXA-->>PC: results or error caught available:false
PC-->>ASWC: status fulfilled or rejected
end
ASWC-->>FBP: settled[50]
FBP->>FBP: map results, compute WoW/cheapest/mostExpensive
FBP-->>Runner: countries, fetchedAt, wowAvgPct
Runner->>Runner: validate and write to Redis
Reviews (1): Last reviewed commit: "fix(seed-bigmac): parallelize the 50-cou..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const conMs = Date.now() - conStart; | ||
|
|
||
| // Concurrency 6 should be well under half the sequential wall-clock. (Ideal | ||
| // ratio is ~1/6; assert < 1/3 for CI-scheduler headroom.) BEFORE the fix the | ||
| // only path was `concurrency: 1` — this comparison would be ~1.0 and fail. | ||
| assert.ok( | ||
| conMs < seqMs / 3, | ||
| `concurrent run (${conMs}ms) should be < 1/3 of sequential (${seqMs}ms)`, | ||
| ); | ||
| }); | ||
|
|
||
| it('preserves country order and returns one row per country', async () => { |
There was a problem hiding this comment.
The test measures sequential vs. concurrent wall-clock and asserts conMs < seqMs / 3. With PER_CALL_MS = 25 the expected sequential time is ~1 250ms and concurrent ~225ms (ratio ≈ 0.18), so the 0.33 threshold gives ~80ms of headroom before the assertion trips. In practice this is comfortable, but on a heavily loaded CI runner where the Node scheduler bunches microtasks or the process is CPU-starved, both legs can shift unpredictably and compress the ratio. Consider asserting conMs < (COUNTRIES.length * PER_CALL_MS) / 2 (a time-based upper bound that doesn't depend on the sequential measurement at all) to make the intent clearer and more stable.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
|
||
| it('a single failing country degrades to available:false, never crashing the run', async () => { | ||
| const failCurrencies = new Set([COUNTRIES[3].currency]); // one country's EXA throws | ||
| const data = await fetchBigMacPrices(null, { searchExaFn: makeFakeExa({ failCurrencies }), getFxRatesFn: fakeFx, concurrency: 6 }); | ||
| assert.equal(data.countries.length, COUNTRIES.length); | ||
| const failed = data.countries.find((c) => c.currency === COUNTRIES[3].currency); | ||
| assert.equal(failed.available, false, 'failed country is marked unavailable'); | ||
| assert.ok(data.countries.some((c) => c.available), 'other countries still resolve'); | ||
| }); |
There was a problem hiding this comment.
The comment says "one country's EXA throws", which is accurate today because COUNTRIES[3] is Brazil (BRL, unique in the list). However, makeFakeExa triggers failures by matching the currency code extracted from the query string (query.trim().split(/\s+/).pop()), so if COUNTRIES[3] were ever swapped for a EUR-currency country (DE, FR, IT, ES) the failCurrencies set would silently knock out four countries instead of one, invalidating the "single country" assertion. A short inline comment noting the BRL-is-unique assumption would make the intent durable.
…arden allSettledWithConcurrency (PR #4995 review) Addresses the PR #4995 review: #1 (P2) — the concurrency regression compared explicit 1 vs 6 and never exercised the production default. Added a deterministic max-in-flight test that calls fetchBigMacPrices with NO concurrency override and asserts exactly 6 EXA calls are in flight, so a regression of the EXA_CONCURRENCY default toward sequential fails the suite. The timing test now also uses the default (no override). #2 (P2) — added a total-EXA-outage test: every row available:false, empty cheapest/most-expensive, and declareRecords(data) === 0 (the recordCount contract that drives runSeed's no-publish/retry path rather than publishing 0 records). #3 (P3) — allSettledWithConcurrency is now exported, but Math.min(concurrency, len) let an invalid concurrency (0 / NaN / negative / float) start zero workers and return a sparse, all-unprocessed array. Clamp to a finite integer >=1 (degrade to sequential) and added invalid-concurrency + empty-input coverage. Backward compatible: fetchCoinPaprikaTickersById already passed clamped ints (its test still passes). Claude-Session: https://claude.ai/code/session_01XKZZy7bzUNTZeJDVWgXbjj
…ertion (fix CI unit flake) The prior review-fix commit passed locally under `node --test` but failed the CI `unit` job (tsx --test --test-concurrency=16). Root cause was NOT a logic error: the file was killed mid-run and node:test's own child-process message parser crashed (FileTest.parseMessage). The seeder logs one line per country (×50) plus per-failure warnings; the new total-outage test emits 50 console.warn in a single SYNCHRONOUS burst (allFail throws with no setTimeout yield), and under 16-way concurrency that output flood corrupts the runner's IPC stream. Fix: - Silence console.log/warn/error for the duration of this test file (before/after hooks, restored afterward). Pure test-side change; also de-noises the CI log. - Drop the wall-clock "concurrent < 1/3 of sequential" test. Concurrency is already proven deterministically by the max-in-flight test (asserts exactly 6 in flight at the production default); a timing-ratio assertion is inherently flaky under --test-concurrency=16. Verified: 0 leaked seeder log lines, 10/10 green across repeated + concurrent runs. Claude-Session: https://claude.ai/code/session_01XKZZy7bzUNTZeJDVWgXbjj
1 participant
Fixes #4994.
Problem
seed-bigmacfires recurring Railway "Deploy Crashed!" alerts. The Big Mac seeder fetched all 50 countries sequentially (for (const country of COUNTRIES)), each awaiting asearchExacall with a 15s timeout, all underrunSeed's 240s fetch-phase deadline. The moment average EXA latency exceeds 240s ÷ 50 ≈ 4.8s/country, the cumulative wall-clock breaches the deadline → exit-75 → crash alert. No data is lost (last-good TTL is extended), but each occurrence alerts the operator and burns up to 240s of compute.Caught live 2026-07-07 (deployment
6fc174d4…): the run burned the full 240.5s and was killed mid-loop.Fix
Run the country loop with bounded concurrency (6) via the existing
allSettledWithConcurrencyhelper (now exported from_seed-utils.mjs). Worst case is ⌈50/6⌉ = 9 waves × 15s ≈ 135s, comfortably under the 240s deadline.Per-country EXA failures already degrade to
available: false(existing try/catch), so a single flaky country never fails the run — it now exits 0 with partial data; only a total EXA outage trips the graceful path.Notable behaviour change
The old 150ms inter-call throttle (
EXA_DELAY_MS) is dropped — the concurrency cap of 6 is the rate limiter. EXA now sees up to 6 concurrent requests instead of 1-at-a-time.Structural
loadEnvFile/readSeedSnapshot/runSeed) behind anisMaincheck (repo-wide seeder idiom) so the core is importable by tests.fetchBigMacPrices(injectablesearchExaFn/getFxRatesFn/concurrency) andCOUNTRIES.Tests
New
tests/bigmac-seed.test.mjs:concurrency:1) run vs the default, asserts concurrent < 1/3 of sequential wall-clock. (Reproduced red first: forced-sequential gives ratio 1.00 → fails; concurrent passes.)COUNTRIESorder.available:false, run still completes.Found + triaged via
/diagnose-railway-seeders; the skill's classifier now flags recurring graceful crashes asGRACEFUL·CHRONICinstead of "action: None".https://claude.ai/code/session_01XKZZy7bzUNTZeJDVWgXbjj