fix(seed-portwatch-port-activity): cap cold-fetch at 30/run to prevent 174-country cliff#3676
Merged
Merged
Conversation
…t 174-country cliff
WM 2026-05-13 incident: portwatchPortActivity went STALE_SEED for 37
hours, failing 3 consecutive cron runs. Root cause was the cache-
cliff trap from the existing memory `per-item-cache-cliff-masks-
upstream-regression`, in this specific form:
Three previous runs all reported "Cache: 174 hits, 0 misses" — they
re-wrote all 174 country keys with the same 3d TTL on each successful
run. After two days of upstream-frozen ArcGIS data, the May 13 00:04
UTC run hit an upstream-advance event: every cached country's `asof`
mismatched the new upstream maxDate, producing "Cache: 0 hits, 174
misses". The preflight alone took 360s (vs 6s when cached), the
activity queue then had to cold-fetch all 174 with concurrency=12,
per-country-cap=90s. Container died at the 570s bundle budget.
Log signature:
Preflight maxDate for 174 countries (360.3s)
Cache: 0 hits, 174 misses
Activity queue: 174 countries (skipped 0 via cache, ...)
batch 1/15: 0 countries published, 12 errors (45.0s)
[log truncates — SIGKILL at the bundle 570s cap]
Fix: introduce MAX_COLD_FETCH_PER_RUN = 30. When needsFetch exceeds
this cap (the "everything stale" case), refresh a random subset and
serve the rest from prior cache marked staleAsof=true. The canonical
payload shape is unchanged; downstream can read staleAsof to decide
whether to display "data may be one window behind" hints.
Math:
- 30 × ~5s/country × concurrency 12 ≈ 13s cold-fetch budget — easily
fits the 570s bundle budget even when ArcGIS is slow
- Full rotation: ceil(174 / 30) = 6 runs at 12h cadence = 72h, well
under MAX_CACHE_AGE_MS (7d = 168h) so no country ever ages out
- A new upstream-advance event refreshes 30 countries immediately +
144 within ~3 days, instead of 0/174 + bundle SIGTERM
Also adds prevPayload to needsFetch entries so the cap path can fall
back to the prior payload per-country. needsFetch was const; this
change makes it let (regression guard via test). One operator-
visibility log line documents the cap fire ("refreshing N, serving
M on stale cache, X dropped").
Tests: 77/77 pass (was 70, +7 for cap behavior incl. constant value,
trigger condition, fallback semantics, contract shape, log format,
declaration mutability, and rotation math).
This is the resilience pattern documented in the memory
`per-item-cache-cliff-masks-upstream-regression` — applied here to
the specific seeder that hit it on 2026-05-13. Generalizable to
other multi-source seeders that share a TTL-synced per-item cache.
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Contributor
Greptile SummaryThis PR caps the portwatch-port-activity seeder's cold-fetch loop at 30 countries per run to prevent the 174-country all-miss cliff that caused a 37-hour
Confidence Score: 3/5The cold-fetch cap correctly prevents the catastrophic 174-country all-miss scenario, but the stale-serve path has no age gate and contradicts the PR's claim that MAX_CACHE_AGE_MS hard-drop behavior is unchanged. The structural fix is sound and the tests are well-targeted. However, deferred countries served from prevPayload bypass the 7-day hard-drop threshold — a country deferred across enough runs when upstream was frozen for 4+ days could be published with window-drifted data older than intended. Additionally, preflight (360s in the incident) is unaffected by the cap, and combined with worst-case per-country fetch times the total can still exceed the 570s budget. scripts/seed-portwatch-port-activity.mjs — specifically the deferred stale-serve block (lines 622-632) and whether prevPayload age should be bounded before serving. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[fetchAll start] --> B[Preflight: fetch maxDate for all 174 countries]
B --> C[Partition: cacheFresh check per country]
C --> D{needsFetch.length > MAX_COLD_FETCH_PER_RUN 30?}
D -- No --> G[Expensive fetch: all misses]
D -- Yes --> E[Fisher-Yates shuffle needsFetch]
E --> F1[slice 0..30 to refresh now]
E --> F2[slice 30..N deferred]
F2 --> H{prevPayload exists?}
H -- Yes --> I[countryData.set: ...prevPayload staleAsof true - no MAX_CACHE_AGE_MS guard]
H -- No --> J[dropped: droppedNoCache++]
F1 --> G
G --> K[Batch fetch CONCURRENCY=12]
K --> L[Write results to countryData]
L --> M[validateFn: countries >= MIN_VALID_COUNTRIES 50]
M -- Pass --> N[Redis pipeline: SET all countryData entries]
M -- Fail --> O[extendExistingTtl + return]
I --> M
J --> M
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+622
to
+632
| for (const item of deferred) { | ||
| if (item.prevPayload && typeof item.prevPayload === 'object') { | ||
| // Mark as stale-asof so downstream consumers know this country is one | ||
| // window behind. The canonical payload structure is unchanged. | ||
| countryData.set(item.iso2, { ...item.prevPayload, staleAsof: true }); | ||
| servedStale++; | ||
| } else { | ||
| // No prior payload (new country, first-ever miss). Drop — same as | ||
| // pre-fix behavior for hard misses. | ||
| droppedNoCache++; | ||
| } |
Contributor
There was a problem hiding this comment.
MAX_CACHE_AGE_MS hard-drop
The PR description states "countries past [MAX_CACHE_AGE_MS] still drop hard (same pre-fix behavior)," but the deferred-stale path has no age gate. Any deferred item with a valid prevPayload object is served stale regardless of prevPayload.cacheWrittenAt. For a country repeatedly deferred whose last fresh write was more than 7 days ago (realistic if upstream was frozen ≥4 days before advancing), this serves window-drift–affected data past the intended hard-drop threshold — exactly what MAX_CACHE_AGE_MS was designed to prevent. Adding && (now - item.prevPayload.cacheWrittenAt) < MAX_CACHE_AGE_MS to the if guard would enforce the same hard-drop boundary on the stale-cap path.
Comment on lines
+635
to
+639
| console.warn( | ||
| ` [port-activity] Cold-fetch capped at ${MAX_COLD_FETCH_PER_RUN}/run — ` + | ||
| `refreshing ${needsFetch.length} now, serving ${servedStale} on stale cache (asof behind), ` + | ||
| `${droppedNoCache} dropped (no prior payload). Rotation: ~${Math.ceil((needsFetch.length + servedStale + droppedNoCache) / MAX_COLD_FETCH_PER_RUN)} runs to fully refresh.`, | ||
| ); |
Contributor
There was a problem hiding this comment.
needsFetch.length, but needsFetch was already reassigned to shuffled.slice(0, MAX_COLD_FETCH_PER_RUN) on the preceding line, so needsFetch.length is always 30 here. The arithmetic is coincidentally correct because 30 + deferred.length == originalTotal, but if this log line is moved above the reassignment, the rotation estimate would silently be wrong. Using MAX_COLD_FETCH_PER_RUN directly makes the intent unambiguous.
Suggested change
| console.warn( | |
| ` [port-activity] Cold-fetch capped at ${MAX_COLD_FETCH_PER_RUN}/run — ` + | |
| `refreshing ${needsFetch.length} now, serving ${servedStale} on stale cache (asof behind), ` + | |
| `${droppedNoCache} dropped (no prior payload). Rotation: ~${Math.ceil((needsFetch.length + servedStale + droppedNoCache) / MAX_COLD_FETCH_PER_RUN)} runs to fully refresh.`, | |
| ); | |
| console.warn( | |
| ` [port-activity] Cold-fetch capped at ${MAX_COLD_FETCH_PER_RUN}/run — ` + | |
| `refreshing ${MAX_COLD_FETCH_PER_RUN} now, serving ${servedStale} on stale cache (asof behind), ` + | |
| `${droppedNoCache} dropped (no prior payload). Rotation: ~${Math.ceil((MAX_COLD_FETCH_PER_RUN + servedStale + droppedNoCache) / MAX_COLD_FETCH_PER_RUN)} runs to fully refresh.`, | |
| ); |
…rotation log (P1+P2) Greptile review of PR #3676 surfaced one P1 (correctness) and one P2 (fragility) — both in the cap block. Plus a P2 outside-diff on preflight scope that I'll address inline (math + reply, no code). P1 — Stale-serve path bypassed MAX_CACHE_AGE_MS hard-drop. The PR description claimed "countries past [MAX_CACHE_AGE_MS] still drop hard (same pre-fix behavior)" but the deferred-stale path had no age gate. Any deferred item with a valid prevPayload object was served stale regardless of cacheWrittenAt. For a country repeatedly deferred across enough runs while upstream was frozen ≥4 days, this would publish window-drift–affected data past the intended 7d hard-drop threshold — exactly what MAX_CACHE_AGE_MS was designed to prevent. Added the same `(now - cacheWrittenAt) < MAX_CACHE_AGE_MS` gate the cacheFresh check enforces. Plus a separate `droppedTooOld` bucket in the operator log so the boundary fires are visible. P2 — Rotation arithmetic used needsFetch.length after reassignment. `needsFetch = shuffled.slice(0, MAX_COLD_FETCH_PER_RUN)` makes the two numerically equal, so the arithmetic was coincidentally correct. But if the log block is reordered (or the cap value ever becomes dynamic), the calc silently breaks. Switched to the constant directly with an `originalMisses = MAX_COLD_FETCH_PER_RUN + ...` intermediate so the intent is unambiguous. Tests: - Updated staleAsof assertion to match the new `...prev` spread shape (was `...item.prevPayload` — semantically identical but the refactor narrowed the variable name) - Added 2 new tests: the MAX_CACHE_AGE_MS gate is present inside the cap block; the rotation expression uses the constant - 79/79 pass (was 77) Out-of-scope (replied inline on the PR): Greptile's preflight-cap suggestion. Math: post-fix worst case is preflight 360s + 30-country batch ~13s = 373s vs 570s budget = 200s headroom. Even at the absolute worst preflight time observed (360s on 2026-05-13), the cap creates enough slack that the structural fix is complete.
Owner
Author
|
All three findings addressed in P1 inline @ line 632 — Stale-serve bypassed MAX_CACHE_AGE_MS hard-drop Greptile's catch was correct. The deferred-stale path had no age gate, so a country repeatedly deferred across runs while upstream was frozen ≥4 days could publish window-drift–affected data past the 7d hard-drop threshold. Added the same P2 inline @ line 639 — Rotation arithmetic was fragile Right call on the move-the-log-block-and-it-breaks-silently concern. Refactored to compute P2 outside-diff @ 560-576 — Preflight not capped Real concern but I'm leaving it out of this PR. Math:
Even at the absolute worst preflight time observed in the incident (360s), the cold-fetch cap creates enough slack that the structural fix is complete. The preflight cost is bounded by upstream — capping our side wouldn't help past ~10-15s (preflight is 174 tiny outStatistics queries at If preflight ever exceeds ~200s consistently AND the cap is firing, a follow-up PR could (a) skip preflight for countries with very recent cache (< 6h), or (b) time-budget preflight and treat the remainder as cache-fresh. Tracking as a backlog item, not blocking this PR. Tests: 79/79 pass (was 77, +2 for the new assertions verifying MAX_CACHE_AGE_MS appears in the cap block + rotation uses the constant). |
3 tasks
koala73
added a commit
that referenced
this pull request
May 13, 2026
… 429) (#3681) * fix(seed-portwatch): retry via Decodo proxy on TIMEOUT (not just HTTP 429) WM 2026-05-13 incident — second-order finding from PR #3676's manual re-run. Even with the cold-fetch cap in place, 30/30 attempted country fetches timed out at the 45s FETCH_TIMEOUT. Coverage gate failed at 27/50, seed-meta stayed stale, /api/health remained WARNING. Root cause: the existing proxy fallback only fires on HTTP 429. ArcGIS was silently STALLING our seed-server IP instead of returning 429 — every fetch hit the 45s AbortSignal.timeout, exited via thrown AbortError, and the 429-only retry branch never ran. Production log shape: Activity queue: 30 countries batch 1/3: 27 published, 12 errors (45.0s) batch 3/3: 27 published, 30 errors (135.1s) 30 country errors: VIR: timeout; GHA: timeout; MNE: timeout; WSM: ... COVERAGE GATE FAILED: only 27 countries, need >=50 Fix: wrap `await fetch(...)` in try/catch and dispatch both 429 AND timeout-class errors to the same proxy retry. Refactored the proxy retry body into `arcgisProxyRetry(url, reason, opts)` so both branches share identical Decodo creds resolution, proxy-side FETCH_TIMEOUT budget, and error message format. Caller-signal-aborts (real cancellation from SIGTERM or per-country 90s timeout) still propagate as-is via `if (signal?.aborted) throw err` before the proxy-retry decision — so a shutdown can't be silently extended into a proxy attempt. Timeout-class detection covers: - DOMException [TimeoutError] (from AbortSignal.timeout(FETCH_TIMEOUT)) - AbortError (from network-level aborts) - undici UND_ERR_* (transient network) - ECONNRESET (mid-response disconnect) - 'fetch failed' / 'aborted' / 'timeout' in error message Operator visibility: warn log distinguishes "HTTP 429 rate-limited" from "direct TimeoutError" / "direct AbortError" — different mitigations apply (rate-limit relief vs IP block / regional routing). Tests: 83/83 pass (was 79, +4 for timeout-fallback). - try/catch wraps the direct fetch - errName === 'TimeoutError' / isTimeoutLike detected - arcgisProxyRetry helper exists and is called from both 429 and timeout paths - signal-abort propagates without proxy retry - Both error sources surface distinct log reasons Companion to: - PR #3676 (cold-fetch cap, merged) — without this, the cap couldn't help because every direct fetch silently timed out - Memory `decodo-proxy-blocks-all-gov-tld-by-policy` — pattern is "use Decodo when direct hits IP-level blocks"; this extends the fallback trigger from explicit-429 to also-on-stall * review: tighter proxy timeout + error-message fallback (P2×2) Greptile PR #3681 review round 2 — two valid P2 findings, both addressed in this commit. The third (behavioral tests via mocked fetch) acknowledged in PR reply; staying with structural-match tests for this PR's scope. P2 — Direct + proxy budget summed to exactly PER_COUNTRY_TIMEOUT_MS. Pre-fix: FETCH_TIMEOUT (45s) on both direct AND proxy legs. If direct stalls full 45s and proxy then stalls full 45s, total = 90s = exactly PER_COUNTRY_TIMEOUT_MS. The proxy-side timeoutMs and the per-country signal-abort race each other at exactly the same instant, with no slack for Decodo's TCP handshake + CONNECT setup (~3-5s on a cold connection). Fix: introduce PROXY_FETCH_TIMEOUT = 35_000. Combined 45 + 35 = 80s, leaves 10s slack under PER_COUNTRY_TIMEOUT_MS (90s) for proxy setup overhead and the surrounding pagination accounting. P2 — proxied.error.message could be undefined. ArcGIS can return error objects without a `message` field (observed `{"error":{"code":400}}`). Pre-fix: `proxied.error.message` evaluated to undefined, the thrown error said "ArcGIS error (via proxy after direct TimeoutError): undefined" — useless for diagnosis. Fix: nullish-coalesce through message → code → JSON.stringify so the thrown message stays informative regardless of upstream shape. Tests: - New value-pin assertion for PROXY_FETCH_TIMEOUT (35_000) - New computed invariant: FETCH_TIMEOUT + PROXY_FETCH_TIMEOUT < PER_COUNTRY_TIMEOUT_MS with ≥5s slack (regression-guards any future bump of either constant) - New fallback chain assertion for proxy.error.message handling 85/85 pass (was 83, +2 for these invariants).
Merged
3 tasks
koala73
added a commit
that referenced
this pull request
May 14, 2026
Greptile PR #3694 round 3 P1: with the temp coverage gate lowered to 25, a cap-mode partial-success run would CLEAR the coverage gate (countryData.size ≥ 25) but STILL fail the unchanged degradation guard (countryData.size < prevCount × 0.8 ≈ 139 in the incident state) — seed-meta never advances, WARNING persists, PR's main recovery claim broken. Math at the current incident state: prevCount 174 degradation threshold 0.8 × 174 = 139 cap-mode realistic 30 cold-fetch + ~24 stale-served = ~54 → 54 << 139 → DEGRADATION GUARD FAILS → extendTtl + return → seed-meta stays frozen, WARNING persists. Fix shape: signal cap-mode from fetchAll() back to main() via a new `capTriggered: bool` field plus `servedStaleCount` / `droppedTooOldCount` / `droppedNoCacheCount` counters. main() bypasses the 80% guard for cap-mode runs (intentional partial coverage by design — see #3676's rotation contract) and logs a `PARTIAL PUBLISH (cap-mode)` line with the fresh/stale split. The bypass is scoped: non-cap runs (where needsFetch ≤ 30 and we fetched all misses normally) STILL apply the 80% guard so silent data-loss scenarios (ArcGIS regression silently drops 100 → 50 countries) remain caught. Logic shape: if (!validateFn) → extendTtl + return [coverage gate] if (capTriggered) → log PARTIAL PUBLISH + fall through [bypass] else if (countryData.size < prevCount × 0.8) → extendTtl + return [silent-loss guard] → publish + advance seed-meta Tests (3 new): fetchAll-shape (returns the 4 new fields), bypass shape (else-if structure ensures cap-mode skips the guard), bypass exclusivity (non-cap runs still enforce the guard with exactly 2 references to `prevCount × 0.8` — one in condition, one in error msg). 90/90 pass (was 87).
koala73
added a commit
that referenced
this pull request
May 14, 2026
…ackoff + temp gate (#3694) * fix(seed-portwatch): rate-limit recovery — concurrency 12→6 + batch backoff + temporary gate Option E from the 2026-05-14 incident triage: combined response to the ongoing ArcGIS degradation where both direct AND proxy paths are throttled. Background (post-#3676 + #3681 deployed): Run #1 (19:55 UTC May 13): 24/30 proxy successes, gate fails 24<50 Run #2 (00:03 UTC May 14): 5/30 proxy successes, gate fails 5<50 Both #3676 (cold-fetch cap) and #3681 (proxy fallback on timeout) are working as designed. The remaining problem: Decodo proxy is now also being rate-limited — successive runs degrade as our usage spikes the proxy's bucket. ArcGIS itself appears degraded for this dataset. Three coordinated changes: (B) CONCURRENCY 12 → 6 Halve in-flight fetches so neither ArcGIS-direct nor Decodo-proxy sees a 12-concurrent burst. Math at cold-fetch cap 30: 5 batches × ~60s realistic + 4×5s backoff ≈ 320s 5 batches × ~90s worst case + 4×5s backoff ≈ 470s Both fit the 570s bundle budget. (C) BATCH_BACKOFF_MS = 5_000 (new) Sleep 5s between batches (skip on last, skip on signal-abort to preserve SIGTERM responsiveness). Spaces out per-batch bursts so neither service hits its rate-limit window from our run alone. 20s total added — negligible against the 570s budget. (Temporary) MIN_VALID_COUNTRIES 50 → 25 Coverage gate lowered so partial-success runs (5-25 successful fetches + stale-served from cache) can advance seed-meta. Pre-fix, seed-meta was frozen at 2026-05-12 00:03 UTC for 60+ hours because no run reached 50. The 25 floor is sized so a 5-success run still fails (real outage) but a 25+ partial-success run advances meta and clears the operator-facing WARNING. Marked TEMPORARY in the inline comment with a 2026-05-20 review target — must revert to 50 once ArcGIS recovers and runs reliably exceed 50 again. Test enforces the comment shape so a future reviewer can't silently normalise 25 as the new permanent floor. Tests: 87/87 pass (was 85, +2 for BATCH_BACKOFF + MIN_VALID_COUNTRIES invariants; +1 modification for CONCURRENCY value). This week's portwatch series: #3676 Structural cold-fetch cap (merged) #3681 Proxy retry on timeout (merged) This Rate-limit recovery (concurrency + backoff + gate) * review: break loop on signal-abort + fix wrong PR number in test (P2×2) Greptile PR #3694 review: P2 — Signal-abort skipped the sleep but kept the loop running. Pre-fix: `if (batchIdx < batches && !signal?.aborted) await sleep(...)` correctly skipped the 5s sleep on abort, but the for-loop continued to the next iteration and started a fresh batch of up to 6 concurrent fetches via withPerCountryTimeout (which creates its own AbortController not linked to the parent signal). Net effect: the actual SIGTERM backstop was onSigterm → process.exit(1), not the abort guard. Fix: `if (signal?.aborted) break;` before the sleep — exits the loop immediately so SIGTERM doesn't start additional in-flight work. P2 — Test comment cited wrong PR number (#3683 vs #3694). The "Halved from 12 → 6 on 2026-05-14 (PR #3683)" comment in the CONCURRENCY test referenced a non-existent PR in this series. Fixed to #3694 so a future reviewer following the comment lands on the right diff. Tests: 87/87 still pass. Updated the BATCH_BACKOFF_MS test to assert both the new `break` shape and the simplified sleep condition; the previous combined `batchIdx < batches && !signal?.aborted` pattern is now split across the two checks. * review: bypass 80% degradation guard in cap-mode (P1) Greptile PR #3694 round 3 P1: with the temp coverage gate lowered to 25, a cap-mode partial-success run would CLEAR the coverage gate (countryData.size ≥ 25) but STILL fail the unchanged degradation guard (countryData.size < prevCount × 0.8 ≈ 139 in the incident state) — seed-meta never advances, WARNING persists, PR's main recovery claim broken. Math at the current incident state: prevCount 174 degradation threshold 0.8 × 174 = 139 cap-mode realistic 30 cold-fetch + ~24 stale-served = ~54 → 54 << 139 → DEGRADATION GUARD FAILS → extendTtl + return → seed-meta stays frozen, WARNING persists. Fix shape: signal cap-mode from fetchAll() back to main() via a new `capTriggered: bool` field plus `servedStaleCount` / `droppedTooOldCount` / `droppedNoCacheCount` counters. main() bypasses the 80% guard for cap-mode runs (intentional partial coverage by design — see #3676's rotation contract) and logs a `PARTIAL PUBLISH (cap-mode)` line with the fresh/stale split. The bypass is scoped: non-cap runs (where needsFetch ≤ 30 and we fetched all misses normally) STILL apply the 80% guard so silent data-loss scenarios (ArcGIS regression silently drops 100 → 50 countries) remain caught. Logic shape: if (!validateFn) → extendTtl + return [coverage gate] if (capTriggered) → log PARTIAL PUBLISH + fall through [bypass] else if (countryData.size < prevCount × 0.8) → extendTtl + return [silent-loss guard] → publish + advance seed-meta Tests (3 new): fetchAll-shape (returns the 4 new fields), bypass shape (else-if structure ensures cap-mode skips the guard), bypass exclusivity (non-cap runs still enforce the guard with exactly 2 references to `prevCount × 0.8` — one in condition, one in error msg). 90/90 pass (was 87).
8 tasks
koala73
added a commit
that referenced
this pull request
May 15, 2026
…e retries (#3701) * fix(seed-portwatch): surface degraded ArcGIS 400 body + cap degenerate retries ArcGIS Daily_Ports_Data, during degradation episodes, returns HTTP 200 with a 400 error body (`Cannot perform query. Invalid query parameters.`) after 30-56s of server processing. Our 45s FETCH_TIMEOUT fires before the body lands, callers see AbortError, the existing circuit-breaker that pattern-matches the error message never fires, and the seeder treats every degraded country as a generic timeout (then a doomed proxy retry, then another doomed first-direct-retry from the one-shot fetchWithRetryOnInvalidParams). Two surgical fixes: 1. **Diagnostic body-capture on timeout.** When the direct fetch hits its FETCH_TIMEOUT (not a caller-signal abort), make ONE extra fetch with +20s budget purely to read the response body. If it contains `body.error`, throw with the real ArcGIS message so the upstream circuit-breaker can act on the upstream-degradation signal. Gated to fire once per process — the proxy budget Greptile P2'd on PR #3681 stays intact for subsequent timeouts. 2. **Bail-don't-retry threshold on Invalid-query-parameters.** Preserve the legit one-shot retry for transient single-call flakes (2026-04-20 BRA/IDN/NGA pattern), but cap total retries-on-this-error per process at 5. Beyond that, throw a clear `ArcGIS degraded — N errors` message so the operator sees the regression class instead of N identical retries × 45s burning the 540s container budget. Investigation that drove this: direct curl probes against services9.arcgis.com revealed (a) HTTP 200 with a 400 error body, (b) non-deterministic results across attempts within minutes (same query, ERR → OK → ERR → 504), (c) WHERE-clause reshaping helps unevenly but never reliably. The right frame is "upstream is degraded, surface it and protect the container budget" not "we're being rate-limited." The existing cap-mode + stale-serve + temp-gate-25 logic (PRs #3676/#3681/#3694) already rides through partial coverage — these changes just make sure we don't waste the container budget while ArcGIS recovers, and that the next diagnostic round has the real error message instead of generic AbortError. Tests: - 3 new structural pattern-match tests covering ERROR_BODY_CAPTURE_EXTRA_MS, _captureErrorBodyAfterTimeout helper, once-per-run gating, INVALID_PARAMS_RETRY_THRESHOLD, counter increment + threshold check, reset helpers. - 96/96 tests pass (was 93 before). * review: gate cap-mode bypass on fresh upstream contact + abort-aware backoff sleep (P1+P2) P1 — Cap-mode bypass could publish a stale-only canonical list as healthy. Pre-fix the capTriggered bypass of the 80% degradation guard was unconditional. With the lowered MIN_VALID_COUNTRIES=25 + cap-mode + servedStale entries seeding countryData, a run with all-stale entries (freshFetched=0, cacheHits=0, servedStale=27, dropped=147) could: - Pass validateFn (countryData.size >= 25 — all from stale-cache) - Bypass the 80% guard (capTriggered=true) - Shrink the canonical list from ~174 → 27 stale-only entries - Advance seed-meta, clearing the operator-facing WARNING Fix: gate the bypass on freshFetchedCount + cacheHitCount ≥ MIN_FRESH_FETCH_FOR_CAP_BYPASS (5). Below that floor, cap-mode is NOT rotational steady-state — it's an ArcGIS-completely-down scenario, and the run falls through to the 80% guard (which extendExistingTtl-only, preserves canonical list, keeps WARNING visible). New log line "CAP-MODE BYPASS REFUSED: only N fresh upstream contacts" makes the refusal observable to operators. This composes with the INVALID_PARAMS_RETRY_THRESHOLD=5 fix earlier in this PR — that change increases the rate at which cold fetches return errors during upstream degradation, making the stale-only scenario this P1 guards against MORE likely, not less. P2 — Inter-batch backoff sleep is now abort-aware. The 5s BATCH_BACKOFF_MS wait used a plain setTimeout that ignored the caller signal mid-sleep, so a SIGTERM during the wait still forced the full 5s before observing the abort. Now races the setTimeout against signal's abort event so the loop exits immediately on a real cancellation (the signal?.aborted check at the top of the next iteration then `break`s the loop). Cosmetic change but matches the PR-body intent that the loop is SIGTERM-responsive. Tests: - 2 new structural pattern-match tests covering MIN_FRESH_FETCH_FOR_CAP_BYPASS, freshFetchedCount counter, upstream-contact gate, CAP-MODE BYPASS REFUSED branch, and the abort-aware sleep shape. - 4 existing tests updated to match the new destructure shape + new prevCount × 0.8 reference count (now 4: 2 conditions + 2 Math.ceil sites, one set per guard branch). - 95/95 portwatch tests pass; 8804/8805 npm run test:data (1 flake is the pre-existing scripts/_bundle-fixture-env-*.mjs race between bundle-runner.test.mjs and news-classify-cache-prefix-audit.test.mjs, unrelated to this change). * review: gate body-capture on success not attempt + short-circuit proxy-confirmed errors (P2×2) Two Greptile P2 comments on PR #3701 review round 2. P2 #1 — Body-capture gate fired on first ATTEMPT, not first SUCCESS. During consistent degradation, the 20s ERROR_BODY_CAPTURE_EXTRA_MS window isn't long enough — ArcGIS needs 30-56s of server processing per query from connection start. A failed first capture (capture also timing out) flipped the once-per-run flag, locking out every subsequent timing-out country from ever capturing the body. The diagnostic value could be lost for an entire run, defeating the whole point of the capture path. Fix: track SUCCESS count (cap at 1 — we only need the body shape once) AND ATTEMPT count (cap at 3 to bound total cost). Null captures consume an attempt but not a success, so subsequent timing-out countries keep trying. Worst-case cost: 3 × 20s wall-clock per run, still within PER_COUNTRY_TIMEOUT_MS=90s caps. P2 #2 — Proxy-confirmed "Invalid query parameters" still triggered retry. When arcgisProxyRetry returns an error body, the thrown message has the `(via proxy after ${reason})` prefix and still matches `/Invalid query parameters/i`. fetchWithRetryOnInvalidParams counted it, then retried via fetchWithTimeout — which would timeout → proxy → hit the same authoritative error, burning a full direct+proxy cycle for nothing. The proxy response IS the definitive upstream signal; retrying is meaningless. Fix: counter still increments (proxy-confirmed errors are valid degradation signals for the threshold), but short-circuit the retry with `if (/via proxy after/i.test(msg)) throw err`. Tests: 2 new structural pattern-match tests + 1 existing test relaxed to accept the new `if (captured?.error) { successCount++; throw ... }` shape. 96/96 portwatch tests pass; 8806/8806 npm run test:data (unrelated bundle-fixture race cleared this run). * review: threshold-before-short-circuit + realistic capture budget (P1+P2 round 3) P1 — Proxy-confirmed Invalid Params never reached the degradation threshold. Pre-fix the proxy-confirmed short-circuit (`/via proxy after/i` → throw) ran BEFORE the threshold check. So during the actual incident — where nearly every error arrives via the proxy fallback path — every error threw the per-country proxy message and the clean `ArcGIS degraded — N 'Invalid query parameters' errors` message was unreachable. Fix: reorder so threshold check fires first, then proxy short-circuit. Counter still increments for proxy-confirmed errors (they contribute to the threshold), and once the threshold is exceeded the degraded message surfaces regardless of whether the error came from direct or proxy. P2 — 20s capture budget didn't realistically catch 30-56s responses. The 3-attempts-× 20s fix was bounded but each attempt was a FRESH request starting from t=0 (the original 45s direct fetch was already aborted). For the observed degraded response class (30-56s server-side from request start), each 20s capture aborts before the body lands. Fix: bump ERROR_BODY_CAPTURE_EXTRA_MS 20s → 40s. Math under PER_COUNTRY_TIMEOUT_MS=90s: direct (45s timeout) + capture (40s budget) = 85s, leaving 5s before the per-country wrap fires. Proxy retry effectively dies for timing-out countries in this mode — accepted tradeoff because the proxy is itself degraded (Decodo throttled per #3694 history), so it wasn't helping anyway. Attempt cap stays at 3 (across the run, not per country) so the diagnostic value is captured by 3rd timing-out country; subsequent countries go straight to proxy. Tests: 1 new ordering test using src.search() index comparison to assert threshold-throw-before-proxy-short-circuit; 2 existing tests updated for the bumped constant and the longer span between increment and short-circuit. 97/97 portwatch tests pass; 8806/8807 npm run test:data (1 flake = pre-existing _bundle-fixture-* race between bundle-runner and news-classify-cache-prefix-audit).
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.
Summary
Resolves WM 2026-05-13 incident where
portwatchPortActivitywentSTALE_SEEDfor 37 hours after a single upstream-advance event triggered a 174-country cold-fetch that couldn't complete inside the 570s bundle budget.Evidence
Three consecutive cron runs from the Railway log (
portwatch-port-activity-seed):The "all-cache-hits" runs re-wrote all 174 country keys with the same 3d TTL, keeping their expirations synchronized. When ArcGIS finally advanced its data on May 13, every cached country's
asoffield mismatched the newupstreamMaxDate— flipping all 174 from HIT to MISS simultaneously. The cold-fetch couldn't complete in 570s.This is exactly the pattern documented in the existing memory
per-item-cache-cliff-masks-upstream-regression(2026-05-06), applied to a different cache-invalidation axis: TTL was fine here, but asof-sync produced the same effective cliff.Fix
Introduce
MAX_COLD_FETCH_PER_RUN = 30. WhenneedsFetch.length > cap:capcountries via the normal expensive pathstaleAsof: trueneedsFetchentries gain aprevPayloadfield so per-country fallback works.Math
Why not just bump the bundle budget?
I considered raising 570s → 900s, but:
The bundle budget stays at 570s.
What this DOES NOT change
MAX_CACHE_AGE_MS(7d) — countries past this still drop hard (same pre-fix behavior)asof === upstreamMaxDate+ age checkMIN_VALID_COUNTRIES = 50) — same coverage requirementerrors[]via eager.catch) — unchangedTest plan
node --test tests/portwatch-port-activity-seed.test.mjs— 77/77 pass (was 70, +7 for the cap behavior)MAX_COLD_FETCH_PER_RUN = 30)> cap, not>=)staleAsof: truemarker on deferred countriesprevPayloadfield on needsFetch entrieslet needsFetch(mutable) regression guardnode --check scripts/seed-portwatch-port-activity.mjs— syntax OK/api/health?history=1for the next 7 days. Expectation: no more 37h gaps. Brief flips OK only if MORE than 30 countries miss in the same single run cadence.Operator action AFTER merge
The current Redis state still has the 174 country keys cached. Once the seeder ships and Railway redeploys, the next 00:0X UTC run will:
upstreamMaxDateIf you want to force a clean cold-fetch right now (before merge lands), running
node scripts/seed-portwatch-port-activity.mjslocally with prod env vars and no bundle budget will populate the cache cleanly (likely 1-2 min runtime without the 570s ceiling).Companion
Sixth PR in this week's resilience series. The previous five fixed budget knobs sized at the safety floor; this one fixes a structural failure mode that no budget knob can solve.
/api/health?history=1