Repository navigation
fix(timeline): cap exportMs at Node's timer ceiling so a 30-day interval is not a busy loop - #2828
Conversation
…FlowLab#2827) an exportMs above 2147483647ms was handed to setInterval, which shortens it to 1ms: the exporter ran on every tick instead of never, mapping the whole trace each time. clamp to the ceiling and warn with the interval in force.
3447290 to
c152449
Compare
|
@Birfy — review request at tip Tier: T2 by path ( Checks at this tip: 19 runs = 16 success + 2 skipped + 0 cancelled + 1 failure, and the single failure is State: base Two things a reader will want, stated rather than left to be discovered:
What to check: that |
|
Verdict: PASS at c152449 Ran at this tip: Red first, with the tip's own test and the base source: Review points:
Not run: |
fix(timeline): cap exportMs at Node's timer ceiling so a 30-day interval is not a busy loop
Fixes #2827
Owner: Dev-5
Reviewer: Reviewer
Written-by: Dev-5
Bot: Dev-5
Risk tier: T2
Base: a4d811a
Claim-Id: 35d94fbb4ac0482489bbbac3ba5a8059
What changed
exportMswas validated as "a finite number of milliseconds above 0" and then handed straight tosetInterval. Node cannot hold a delay above a signed 32-bit millisecond count: it rewrites the delay to 1 ms (the value the plugin passed is still whatsetIntervalwas asked for) and the exporter starts running on every tick. SoexportMs: 2592000000— 30 days, the natural way to ask for "effectively never" — turned the exporter into a busy loop that mapped the whole trace on every turn of the event loop, while the config said "every 30 days". The collector is not flooded (createExporterdedupes spans by id, so only the first flush posts anything); the cost is the host's event loop, silently. Product measured it on this base: 6000 records, 150 ms idle —158.8 msCPU (106%) withexportMs: 2592000000against3.1 ms(2.1%) at the 5000 ms default; 3000 records,87.1 msvs0.3 ms.This is the same shape as #2811/#2823 in
plugins/tree, which clamped the scheduler's own interval;plugins/timelinewas not covered by that change.plugins/timeline/src/timeline.js— two constants (DEFAULT_EXPORT_MS = 5000,MAX_EXPORT_INTERVAL_MS = 2_147_483_647) and one clamp. The validation is unchanged (Number.isFinite && > 0); a value above the ceiling is clamped to it and one construction-time warning names both values:flybot-timeline: exportMs=2592000000ms is above Node's 2147483647ms timer ceiling; using 2147483647ms.0,-5,'30',NaN,Infinity,nulland an unset key still fall back silently to 5000 ms;2147483647itself is accepted silently.plugins/timeline/test/export-interval-ceiling.test.js(new, 5 cases) — it records the delay actually handed tosetInterval(monkeypatched) rather than asserting a constant, and its first case hands the wrapped callback to the real timer.docs/subsystems/timeline.md— invariant 11 now states the cap, why it exists, and names the new test file in the Tests paragraph.plugins/timeline/README.md— theexportMscomment says "capped at 2147483647 ms".changelog.d/2827-timeline-export-interval-ceiling.fixed.md.plugins/timeline/index.jsis untouched: both exits (otlpandjson) keep passingintervalMs: config.exportMs, so the clamp lives in one place and applies to both.Why clamp, not fall back to 5000
Both were acceptable per the issue. Clamping keeps the caller's intent: a value above the ceiling is someone asking for a very long interval, and 2147483647 ms (about 24.8 days) is the longest
setIntervaldelay Node can honour. Falling back to 5000 would instead turn "effectively never" into the shortest interval the plugin has — a change in the opposite direction, and the busiest setting available. The warning names the effective value, so the difference between what was asked and what is used is never silent.Generated artifacts (not smuggling)
node scripts/ds-bot-patches.mjs generatechanged nothing:plugins/**,docs/**andchangelog.d/**are not in the patch set (plugins/timelineappears in nopatches/ds-bot/topics.jsonentry), so there is no mirror to rewrite.git status --porcelainis empty aftergenerate, andcanonicalreportspatches/ds-bot/ is what generate writes (generate is a no-op)— the absence of drift is checked, not assumed.git diff --checkis clean.Red first
Base
e7cffe1928.plugins/timeline/src/timeline.jswas put back to its base content while the new test stayed, and the run was made twice.Both runs have the same shape —
5 tests / 2 pass / 3 fail— and the red sentences are identical apart from the tick count in the first one (52 the first time, 54 the second):The first case is not a mock of the timer's behaviour: the monkeypatched
setIntervalrecords its delay argument and forwards the callback to the real timer, so on the unfixed source the exporter genuinely fires ~52 times in 60 ms; after the fix it fires zero times in the same window. The other two cases assert the recorded delay itself. The two cases that are green on the base by design are the ceiling being a valid interval and the unusable values falling back to 5000 ms — they pin the behaviour the clamp must not change.Ran at this tip
Checks ran at the local commit
2cc079accde5436dbd8ae2b48bbe08ef69dd0278, based ona4d811a4(the currentmain). The fork head is the same tree, parent and message rebuilt through the Git Data API, so its sha differs from the local one; the fork head isc1524493e888d1a3f1ad1711fdc78ad5c2e3bf12.TZ=UTCthroughout.(verify then stops on the
.github/scriptsface — the#749red below,673/672/1— so the faces after it were run on their own.)The faces after the core suite were run on their own:
The base moved between the red-first run and the push:
mainadvanced frome7cffe1928toa4d811a4(#2824, which touchescore/test/workflow-store.test.jsand its patch mirror only — disjoint from these five files). The branch was rebased ontoa4d811a4and every reading above was re-taken there; the red-first sentences are frome7cffe1928, where the implementation under test is the same.Local readings that are environment, not this change
Neither is introduced here and neither exists in CI; both reproduce on the base
e7cffe1928too.core/test/bench-preflight.test.js:88— on this machine pnpm lives in/usr/bin, and the fixture drops everyPATHdirectory that contains pnpm, which takes/usr/binand/binwith it and leaves the script withoutmkdir(bench.sh: 91: mkdir: not found). With a pnpm-free tool directory in front ofPATHthe file is7/7green. The file is byte-identical between the base and this tip; no code was changed for it..github/scriptssuite,testing-layers.test.mjs:31(#749) — it walks worktrees and finds.cache/wt-2547, a nested worktree this machine has and CI does not.Not run
npm run pack, typecheck and actionlint: not part ofnpm run verify, and nothing here reachesindex.jsorsrc/host/module loading.otlp-redirect.test.jsandotel.test.jsalready cover the send path and the cursor, and this change touches only the interval value handed to the timer.Checkpoint
{ "pr": 2828, "claim": "Dev-5", "issue": 2827, "summary": "An `exportMs` above Node's timer ceiling \u2014 2592000000 ms, the natural way to say \"every 30 days\" \u2014 reached `setInterval`, which re-arms that delay at 1 ms: the exporter then mapped the whole trace on every tick while the config said \"almost never\". `timeline.js` now clamps to 2147483647 ms and warns once with the interval in force; both exporter exits, the OTLP payload, the dedupe and the JSON size cap are untouched. Invalid values still fall back silently to 5000 ms, and the timeline doc and README state the cap.", "files": [ "plugins/timeline/src/timeline.js", "plugins/timeline/test/export-interval-ceiling.test.js", "docs/subsystems/timeline.md", "plugins/timeline/README.md", "changelog.d/2827-timeline-export-interval-ceiling.fixed.md" ], "checks": { "ran": "export-interval-ceiling 5/5; plugins/timeline full 166/166; tree 149, social 619, workflow 528; core suite 3207/3207; core lint + arch-docs OK; plugin lint 0 errors; ds-bot-patches check + canonical; changelog check", "result": "green", "at": "2cc079accde5436dbd8ae2b48bbe08ef69dd0278" }, "next": "none \u2014 ready for review" }