fix(gdelt-intel): share one timestamp validator between the fetch-ordering and health paths (#5858) - #6044
Conversation
|
@Yigtwxx is attempting to deploy a commit to the World Monitor Team on Vercel. A member of the Team first needs to authorize it. |
… readers (koala73#5858) seed-gdelt-intel had two readers of the same stored topic stamps and only one clamped forward skew. rankTopicsForFetch took the run clock and did Math.min(parsed, nowMs); contentMeta accepted any finite positive parse — and contentMeta is the one maxContentAgeMin is evaluated against. Extracts parseStampMs and routes both paths through it, so a container with a skewed clock cannot mint a fetchedAt that reads fresher than the moment it is read at, and the ordering view and the health view of one stamp can no longer diverge. Covers defect 2 of koala73#5858 only. Defect 1 (newestItemAt is a max, so one refreshed topic hides five starving ones) is left for a maintainer decision: the issue offers two shapes for it and the preferred one changes the seed-envelope content-age contract, which is mirrored in three files and read by the health classifier.
c0fcdb5 to
e50d59a
Compare
|
@Yigtwxx can you allow edits on this PR please so I push fixes instead of creating a new PR and pulling your commit in |
|
Done — Branch state, so you don't have to check it yourself: head Sorry this cost you the extra round on #6042 — the box was the only thing missing there. Still open for you in this PR, if it is what you were about to push: defect 1 of #5858 ( |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Thanks for taking the branch over — The
|
|
Sent the flake fix as its own PR so it stays off this branch: #6405. It widens the This PR still just needs a |
Summary
Addresses defect 2 of #5858. Defect 1 is deliberately not in this PR — see below.
scripts/seed-gdelt-intel.mjshad two readers of the same stored topic stamps, and only one of them clamped forward skew.rankTopicsForFetch(:264-268) took the run clock:contentMeta(:944-946) did not:And
contentMetais the one that matters for the alarm:RUN_SEED_OPTS.maxContentAgeMin: 1440(:966) is evaluated against thenewestItemAtit returns. The path with the guard was the path that did not need it.This extracts
parseStampMs(value, nowMs)and routes both readers through it, which is what the issue asks for — "The two should share one timestamp validator", "ideally via a shared helper so the ordering path and the health path cannot diverge."One correction to the issue's analysis, offered as evidence
The issue says a future-dated stamp keeps
newestItemAt"falsely fresh indefinitely". That overstates it, and I would rather say so than quietly ship against it:api/health.js:1009already computesisFutureDatedfrom a negative content age and folds it intocontentStale(:1015), with a comment saying exactly why. So a wildly future stamp does not hide staleness today — it surfaces asSTALE_CONTENT.What the missing clamp actually costs is narrower, and still worth closing:
newestItemAtan operator cannot reconcile. The value goes out on the wire. Two components in the same seeder resolving one stored stamp to two different instants is the divergence the issue is about, independent of whether either happens to alarm.Date.parseaccepted stamps at or before the epoch.contentMeta'sms > 0filter caught exactly0; a pre-epoch ISO string parses negative, passedNumber.isFinite, and would have been silently taken as theoldestItemAt.parseStampMsrejects the whole range.The clamp also has to be conditional:
rankTopicsForFetchis called with an injected clock in tests andDate.now()in production, so a non-finitenowMsdegrades to the raw parse rather than swallowing every stamp. That behaviour is preserved and pinned.Why defect 1 is not here
Defect 1 —
newestItemAtis aMath.max, so any single GDELT success holds the alarm green while five topics starve — is real and still open. I am not implementing it in this PR because it is a decision that should be yours, not mine, and the issue names two different shapes for it:Here is what I found scoping each, so the decision is cheap when you make it:
maxContentAgeMinagainstoldestItemAt. Two lines, no contract change — and it is the shape your own note warns against, because one starved topic then holds the whole cohort atSTALE_CONTENTwith no detail about which topic or how many.scripts/_seed-utils.mjs:555-557and:620-623on the write side,:723-727on the read side,buildEnvelopeinscripts/_seed-envelope-source.mjsplus its two mirrors (server/_shared/seed-envelope.ts,api/_seed-envelope.js) underscripts/verify-seed-envelope-parity.mjs, andapi/health.js:997-1016where it becomescontentAge. A fourth field touches all of them, and the new status it implies applies to every seeder that opted into content-age, not just this one.api/health.jsalready carriesoldestItemAtthrough tocontentAge(:1012) and does nothing with it. A distinctcoverageStalederived there — separate fromcontentStale, so no existing signal is OR'd into — needs no envelope change at all, only a decision about what bound the starved end is judged against and whether a new status appears in the entry.Happy to implement any of the three on your word, in this PR or a follow-up.
Verification
Every guard is mutation-proven:
contentMetaback to a bareDate.parse(the pre-fix reader)contentMetaloses its run-clock defaultMath.maxNo survivors.
The fixture in the first
contentMetacase is the starvation from #5848 itself — military 5 hours old, cyber 18 days, energy 29 days — so the test reads as the incident rather than as invented numbers. The last two tests exist only to pin the divergence closed: they feed one skewedfetchedAttorankTopicsForFetchand tocontentMetaand assert both resolve it to the same instant, and that ordinary past stamps still produce the same fetch order as before.contentMetanow takesnowMs = Date.now().runSeedinvokes it ascontentMeta(data)(scripts/_seed-utils.mjs:2001), so the default is the production path; there is a test asserting the one-argument call still clamps.Other gates:
npm run test:data: identical failure set toorigin/main— 47 failing test names on both,commdiff empty in both directions.Out of scope
api/health.js. Its existing future-date handling is correct and untouched; this fix is upstream of it, at the seeder that writes the number.seed-gdelt-bulk-materializer(GDELT 429s are supply-side load shedding — proxy fixes are exhausted; migrate all consumers to the mirrored bulk datasets (bulk-first) #5843). As the issue's scope note says, the fetch-ordering code becomes dead once that lands butcontentMetaand its health contract survive — which is why the shared validator is worth having on both sides now rather than only on the one that stays.maxContentAgeMin, to the ranking comparator, or to any published field's meaning.Type of change
Affected areas
/api/*)scripts/seed-gdelt-intel.mjs(Railway seeder), healthnewestItemAt/oldestItemAtvaluesChecklist
api/rss-proxy.jsallowlist (if adding feeds) — N/A, no feeds added.npm run typecheck)Documentation Alignment Checklist
N/A — no published documentation claim changes.
newestItemAtandoldestItemAtkeep their documented meanings; the fix only stops a skewed source stamp from producing a value ahead of the run clock. The schema-description update the issue asks for belongs with defect 1's new field, which is not in this PR. Listed for completeness:seed-meta:for this seeder keeps the samenewestItemAt/oldestItemAt/maxContentAgeMinfields, written byrunSeedfromcontentMetaand read byapi/health.js:997-1016.