Suppress the LSan false positive from Temporal's ICU calendar template cache - #37034
Conversation
…emplate cache TemporalCore::withCalendar caches up to 8 open UCalendar templates for the process lifetime in a LazyNeverDestroyed TinyLRUCache. The CalendarCacheEntry owning them is TZone-allocated, which LeakSanitizer cannot scan, so the libc-allocated UCalendars are intermittently reported as direct leaks at exit on the asan lanes. Mirror the existing TemporalCore::withTimeZone suppression for the calendar twin and pin it with an ASAN-only regression test.
WalkthroughChangesTemporal leak validation
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — test-infrastructure only, mirroring the existing TemporalCore::withTimeZone suppression for its calendar twin.
What was reviewed:
- Suppression entry matches the adjacent
withTimeZoneprecedent (boundedLazyNeverDestroyedLRU cache, TZone-allocated owner LSan can't scan) — a documented false positive, not a real leak being silenced. - New test follows harness conventions:
skipIf(!isASAN), spreadsbunEnv, drains stdout/stderr/exited concurrently, asserts a combined{stdout, stderr, signal, exitCode}object;LSAN_OPTIONSpath construction matches other tests at the same depth (e.g.timer-heap-race.test.ts). - Checked that the timer-callback workload avoids the pre-existing
JSModuleLoader::evaluateNonVirtualblanket suppression so the test actually exercises the new entry.
Extended reasoning...
Overview
Two files touched, both test-only:
test/leaksan.supp: addsleak:TemporalCore::withCalendarwith a comment mirroring the immediately-precedingTemporalCore::withTimeZoneentry.test/js/web/temporal/temporal.test.ts: adds an ASAN-gated subprocess test that exercises 8 non-ISO calendars underdetect_leaks=1with the repo suppression file to pin the new entry.
No runtime, native, or bindings code is modified.
Security risks
None. This is a LeakSanitizer suppression for a WebKit-owned, process-lifetime, size-bounded (≤8 entries) ICU calendar template cache. It does not affect user-facing behavior, and the suppression is scoped to a specific JSC symbol rather than a broad wildcard.
Level of scrutiny
Low. REVIEW.md warns against silently weakening sanitizers, so the key question is whether this is a genuine false positive or papering over a real leak. The PR establishes it is the former: the cache is LazyNeverDestroyed by design, bounded by LRU eviction, and the owning CalendarCacheEntry lives in bmalloc (TZone-allocated) which LSan does not treat as a root region — the exact same rationale already accepted for withTimeZone directly above. The PR also cites a concrete CI failure (build 89504) with the ICU stack trace.
Other factors
- The regression test is well-constructed per REVIEW.md: it deliberately runs the workload in a timer callback so the allocation stack does not carry
JSModuleLoader::evaluateNonVirtual(already blanket-suppressed), then scrubs the stack so a clean exit depends on the suppression rather than a lucky stale pointer. The PR reports 10/10 failures without the entry and 10/10 passes with it. - Test hygiene checks out against the repo's other LSan-pinning tests (
serve-body-leak.test.ts,worker-terminate-lifetime.test.ts,timer-heap-race.test.ts): sameASAN_OPTIONS/LSAN_OPTIONSshape, same suppressions-path construction, sameskipIf(!isASAN)gating. - 20s timeout is reasonable for a debug-ASAN subprocess spawn;
BUN_JSC_validateExceptionChecksis explicitly unset to keep it fast, with a comment explaining why. - No prior human review comments to address; only a CodeRabbit rate-limit notice.
|
Now that #37024 is on main, After rebasing onto main, this PR can drop that skip-list entry: with |
…time zone twin leak:TemporalCore::buildCalendarTemplate instead of withCalendar: every reported allocation carries the builder frame, and the narrower pattern keeps a future real leak in one of the op lambdas withCalendar runs visible to the ASAN lane. withTimeZone stays as is; its ucal_open is inline with no builder frame. The child workload now also exercises named time zones, deterministically pinning the existing withTimeZone entry (previously exercised only in-process where module-level stacks are blanket-suppressed), and pure-ISO PlainDateTime.prototype.with, which opens the iso8601 template despite ISO arithmetic otherwise skipping ICU. The supp comment no longer claims ISO never opens one.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/web/temporal/temporal.test.ts`:
- Around line 146-151: In the temporal test assertion, remove the inline
LeakSanitizer matcher from the combined object passed to toEqual. Add a separate
expect(stderr).not.toContain("LeakSanitizer") assertion first, then assert
stdout, signal, and exitCode in the existing result object.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 14f29fca-103b-4d97-a71d-c44bcf146204
📒 Files selected for processing (2)
test/js/web/temporal/temporal.test.tstest/leaksan.supp
|
drop the skipped test now |
Requested in review. The probe evidence stays in the PR description; the suppression's first-seen citation now points at the CI test file that hit it.
The no-validate entry was the escape hatch for the calendar-template false positive this PR suppresses. With the suppression in place the file is leak-clean under the CI leak-validation env (5/5 runs on a debug ASAN build; 3/3 aborts without the entry), and it exercises the suppressed path on the asan lane.
|
Dropped the test. Since the merge from main brought in #37024, this PR now also removes its test/no-validate-leaksan.txt entry for deep-equals-temporal.test.ts, so that file gets leak validation again and exercises the suppression on the asan lane. Verified on a debug ASAN build under the CI leak-validation env: 5/5 clean runs with the entry, 3/3 LSan aborts without it. Happy to drop that hunk too if you want the suppression alone. |
There was a problem hiding this comment.
LGTM — test-infra-only change that mirrors the existing withTimeZone suppression right above it.
What was reviewed:
- Suppression pattern
leak:TemporalCore::buildCalendarTemplateis narrowly anchored on the builder frame (not the broaderwithCalendar), matching the design of the adjacentwithTimeZoneentry. - Removing
deep-equals-temporal.test.tsfromno-validate-leaksan.txtre-enables leak validation; the entry was added in #37024 for exactly this leak, so CI now validates the suppression carries it. - Dylan's "drop the skipped test now" was addressed in 0656df6 and f8d4b9a — the ASAN pin test was removed and the opt-out entry dropped.
Extended reasoning...
Overview
This PR touches two test-infrastructure files only: test/leaksan.supp gains one suppression entry (leak:TemporalCore::buildCalendarTemplate) with an explanatory comment block, and test/no-validate-leaksan.txt loses the whole-file LSAN opt-out for deep-equals-temporal.test.ts. No production code, no runtime behavior, no test logic is changed. An earlier revision also added an ASAN-gated subprocess pin test to temporal.test.ts, but that was dropped in 0656df6 per maintainer request, so the final diff is just the suppression + opt-out removal.
Security risks
None. This affects only which allocation stacks LeakSanitizer reports on the ASAN CI lane. The suppression is a substring match on a WebKit/ICU frame name and cannot influence runtime behavior.
Level of scrutiny
Low. The new suppression is the calendar twin of the already-accepted TemporalCore::withTimeZone entry directly above it — same cache design (process-lifetime LazyNeverDestroyed TinyLRUCache of up to 8 UCalendars), same TZone-allocated owner that LSan can't scan, same bounded false positive. The comment explains why it anchors on buildCalendarTemplate rather than withCalendar (so real leaks in the op lambdas would still surface), which is a strictly narrower pattern than the precedent. The opt-out removal is a coverage increase: if the suppression is insufficient or if the file also hits an unrelated leak (the removed entry's comment mentioned Expect wrappers, though robobun's 08:00 comment clarifies the entry was actually added for the calendar leak in build 89504), the ASAN lane will fail and surface it — self-correcting rather than silently masking.
Other factors
The maintainer (dylan-conway) was engaged and requested dropping the pin test; both follow-up commits (0656df6, f8d4b9a) landed after that comment and address it. The CodeRabbit thread on the assertion style was resolved and is moot now that the pin test was removed entirely. The PR description's Verification section is stale (it still describes the dropped test), but the remaining change is simple enough that it stands on its own — the suppression's correctness will be validated by CI running deep-equals-temporal.test.ts under LSan again.
There was a problem hiding this comment.
LGTM — narrowly-scoped LSan suppression mirroring the existing withTimeZone entry, plus re-enabling leak validation for the test that exercises it.
What was reviewed:
- Suppression pattern
TemporalCore::buildCalendarTemplateis specific to the ICU template cache allocation frame; anchoring on the builder rather thanwithCalendarkeeps op-lambda leaks reportable. - Removed no-validate entry: the test file exists and no sibling deep-equals/deep-match files remain in the opt-out list; the dropped comment's Expect-wrapper concern is addressed in the PR description (separate #32180).
- Reviewer request ("drop the skipped test now") is done — the earlier ASAN pin test is gone; only the two config-file hunks remain.
- Self-verifying: if the suppression is insufficient, the re-enabled
deep-equals-temporal.test.tswill fail the asan lane.
Extended reasoning...
Overview
Two test-infrastructure config file changes, no source code touched:
test/leaksan.supp: addsleak:TemporalCore::buildCalendarTemplatewith an explanatory comment, directly below and mirroring the existingleak:TemporalCore::withTimeZoneentry (same cache design: process-lifetimeLazyNeverDestroyedTinyLRUCacheof up to 8UCalendars, owned by TZone-allocated entries that LSan can't scan as roots).test/no-validate-leaksan.txt: removes the whole-file LSAN opt-out fortest/js/bun/bun-object/deep-equals-temporal.test.ts, re-enabling leak validation now that the targeted suppression exists.
Security risks
None. This is purely LeakSanitizer suppression configuration for CI test lanes. No runtime, auth, crypto, or user-facing code is touched.
Level of scrutiny
Low. LSan suppressions match by substring against allocation stack frames; TemporalCore::buildCalendarTemplate is highly specific and won't over-match. The PR deliberately anchors on the builder frame rather than the broader withCalendar so future real leaks in the many op lambdas withCalendar runs remain reportable — this is a tighter suppression than the naive alternative. The change follows the exact pattern of the immediately-preceding withTimeZone entry with an equally detailed rationale comment.
Other factors
- The maintainer's only request ("drop the skipped test now") has been addressed; the earlier ASAN-gated pin test in
temporal.test.tswas removed and the diff is now config-only. - The removed no-validate comment cited "Expect wrappers not yet finalized at process exit" as the reason for skipping. The PR description explains this is a separate issue (#32180) about
bun test's own finalizer memory, distinct from this process-lifetime ICU cache, and reports 5/5 clean runs under the CI leak-validation env with the new suppression — so re-enabling is safe and, if wrong, self-correcting via the asan lane. - No sibling deep-equals/deep-match entries remain in
no-validate-leaksan.txt, so the removal doesn't leave the file inconsistent. - CodeRabbit's inline nitpick was withdrawn and is moot now that the pin test is gone.
…ader pinning Review follow-ups: - The ASan gate on the cc() tests was misattributed to TinyCC's setjmp/longjmp handling; they pass under ASan. What actually breaks the asan CI lane is LSan at exit: FFI::finalize intentionally leaks the wrapper of each un-close()d cc() instance (dlclose on GC is unsound, see src/runtime/ffi/ffi_body.rs). Suppress that one allocation site with an anchored leak:bun_ffi_cc entry in test/leaksan.supp, as #37034 did for Temporal's calendar cache, and run the tests under ASan for real with leak validation still live for everything else in the file. - The install exists to pin cc1/cc2 to upstream node-api-headers rather than the headers cc() bundles; say so, and assert it with a new test (upstream defaults NAPI_VERSION to 8, the bundled copy to 10, and TCC silently drops nonexistent -I dirs, so the pin could otherwise rot). - Lower the hook timeout from 300s to 120s: 300s equals the CI runner's outer per-file budget for test/napi, which would turn a hung install into an unattributable runner-level kill instead of an in-band hook failure. 120s still dwarfs the 5s local default that broke cold runs.
2 participants
Problem
On the
13 x64-asanlane, a test that exercises non-ISO Temporal calendars from a test callback can abort after a fully green run with a LeakSanitizer report. Seen in build 89504 on #37024, whosetest/js/bun/bun-object/deep-equals-temporal.test.tsuses[u-ca=hebrew]:The CI annotation titles this
direct leak of 624b in {closure#0} (src/jsc/JSValue.rs:1664:22)because that is the first in-repo frame (the test-runner'sJSValue::call); everything below it is WebKit/ICU.Cause
TemporalCore::withCalendar(vendor/WebKit/.../temporal/core/CalendarICUBridge.cpp) keeps up to 8 openUCalendartemplates in a process-lifetimeLazyNeverDestroyedTinyLRUCache, one per calendar ID (non-ISO arithmetic, plus pure-ISOPlainDateTime.prototype.with, which reaches the same path unguarded); LRU evictionucal_closes them, so the set is bounded. TheCalendarCacheEntrythat owns eachUCalendarisWTF_MAKE_TZONE_ALLOCATED(bmalloc), which LSan does not scan, so the libc-allocatedUCalendar(and the ICUTimeZoneinside it) is reported as a direct leak even though it is reachable. Whether a given run aborts depends on whether some stale stack or register value still points at the ICU object when LSan scans at exit, hence the intermittence.This is the calendar twin of the already-suppressed
TemporalCore::withTimeZoneentry (same cache design, same TZone-allocated owner).Fix
leak:TemporalCore::buildCalendarTemplatesuppression totest/leaksan.supp, mirroring thewithTimeZoneentry. The pattern anchors on the template builder rather thanwithCalendaritself so that a future real leak inside one of the many op lambdaswithCalendarruns would still be reported; every cached-template allocation carries the builder frame. (withTimeZonehas no such builder frame, itsucal_openis inline, so that entry keeps its existing pattern.)test/no-validate-leaksan.txtescape hatch Compare Temporal objects by value in Bun.deepEquals and toEqual #37024 added fordeep-equals-temporal.test.ts, re-enabling leak validation for it; that file exercises the suppressed path on the asan lane.Verification
On a debug ASAN build, running
bun test test/js/bun/bun-object/deep-equals-temporal.test.tsunder the CI leak-validation env (BUN_DESTRUCT_VM_ON_EXIT=1,detect_leaks=1:abort_on_error=1, repo suppression file):A standalone probe exercising 8 non-ISO calendars plus pure-ISO
PlainDateTime.withfrom a timer callback shows the same split (10/10 aborts without, 10/10 clean with;print_suppressions=1attributes exactly the ICU template allocations to the new entry). Top-level module code cannot reproduce this: its allocation stacks carryJSC::JSModuleLoader::evaluateNonVirtual, which the suppression file already covers wholesale. An ASAN-gated test pinning the entry was part of an earlier revision and was dropped per review; the re-enableddeep-equals-temporal.test.tscovers the path in CI instead.The Expect-wrapper shutdown leak mentioned in the dropped no-validate comment is a separate issue tracked in #32180: that is
bun test's own finalizer-owned memory, while this cache deliberately survives VM teardown, so #32180 would not prevent this report.no test proof · iteration 1 · docs-only change; test-proof not applicable