test(signals): assert retention on bookkeeping, not a GC heap count

[kixelated]

Problem

spawn retention > an effect that never reruns drops settled tasks (added in #4085) fails CI's Test job intermittently with ~8191-8194 retained promises (runs 36104628091, 36104280755). It blocks #4095.

The implementation is correct: spawn deletes each task from its set once it settles. The test is not. It counted Promise cells after Bun.gc(true), called from inside the test's own async stack. JSC scans that stack conservatively, so a stale pointer can keep dead cells alive. The ~8192 is most likely a discarded storage table of the task set (it grows to 10000 promises before any are dropped), pinned with every entry it still holds. Whether such a pointer survives depends on stack/register reuse, which differs by arch (CI is arm64) and load; bun is 1.3.13 in both CI and the local dev shell.

The sibling race > many races against a long-lived promise keep the heap flat (same PR, objectCount-based) has the same flaw. It reproduces locally under parallel load: ~1900 extra Cell Butterfly and ~95 Structure cells after the GC, gone once the stack unwinds.

Approach

Assert on the bookkeeping itself instead of observing the GC. Both tests spy on Set.prototype.add to find the set in question, then check its size:

• spawn: the effect's task set holds all 100 tasks while pending and none once they settle.
• race: closed's listener set received all 1000 listeners, and every set touched is empty afterwards.

Both fail deterministically with the respective delete removed, and passed 160 parallel runs locally.

Impact

• Tests only. No public API or wire change.

Alternatives

• Measure after setTimeout so the stack unwinds, or spawn one task at a time so the set never grows: less flaky, still GC-dependent.
• Loosen the thresholds: papers over it.

Follow-ups

• js/binary, js/json, js/watch (sync.test.ts, retention.test.ts) have end-to-end heap-count tests from fix(js): stop retaining a listener, reaction, or task per frame #4085 with the same exposure. They have not failed in CI yet; left for a separate change since they have no single set to inspect.

(Written by Claude Opus 5.5)

🤖 Generated with Claude Code

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-09-25T08:07:50.818918Z	4ea71a1	Draft marked ready

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: f452c378-b54f-43c9-baca-ece0a414b534

📥 Commits

Reviewing files that changed from the base of the PR and between 75d615d and 4ea71a1.

📒 Files selected for processing (1)

• js/signals/src/index.test.ts

Included review availability: Your plan provides up to 4 included reviews per hour; 0 remain after this review.

---

Walkthrough

The tests remove the bun:jsc heap-stats import and replace heap-growth measurements with checks of listener and task sets. The race test checks that captured listener sets are empty after 1,000 races. The spawn test checks that the effect’s task set is empty after 100 tasks settle. Existing listener-release and pending-task rerun assertions remain.

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to 4ea71

The updated tests check listener and task-set cleanup without changing runtime behavior. No merge-blocking issue is established.

Architecture Summary

Architecture risk: 🔵 Low · up to 4ea71

The change affects 1 system.

Changed systems: js

Architecture concerns
No architecture-level concerns identified.

Review details

Systems and components

• observed — js (service) was modified; 1 changed file maps to changed impact.

Before / after behavior

• observed — Modified behavior in js/signals/src/index.test.ts: Removed the heapStats import from bun:jsc.
• observed — Modified behavior in js/signals/src/index.test.ts: Replaced the heap-growth measurement over 1,000 and 10,000 races with inspection of Set.add contexts: after 1,000 races against a never-settling promise, the test asserts that the last listener set was added to 1,000 times and that every captured set is empty.
• observed — Modified behavior in js/signals/src/index.test.ts: Replaced Promise heap-growth measurement after 10,000 spawns with direct task-set assertions: 100 spawned tasks share the effect’s set, which is empty after settling.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main change: the tests now assert retention through bookkeeping sets instead of GC heap counts.
Description check	✅ Passed	The description directly explains the intermittent GC-based test failures, the bookkeeping-based test changes, and their test-only impact.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1…
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

✨ Simplify code

• Commit to this branch
• Create a new PR

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[kixelated]

MERGE

Positive improvement, complexity justified, approach looks right.

The flaky ~8192 retained Promise counts were a bad test, not a leak: spawn already deletes settled tasks; JSC's conservative stack scan from inside the async test can pin a discarded storage table after Bun.gc(true). Asserting the bookkeeping (Set size via a scoped Set.prototype.add spy) is the right fix—deterministic, fails if delete is removed, and drops GC/arch/load sensitivity that was blocking #4095.

Concrete strengths:

• Spawn: all adds hit one set, size 100 while pending, 0 after settle.
• Race: last set is closed's listeners, 1000 adds to it, every touched set empty afterwards.
• Spy restored in finally; no public API change.

Spying on Set.prototype.add couples the test to "race/spawn use a Set," but that is the retention mechanism under test and far cheaper than living with CI flakes or loosening thresholds. Follow-up for other #4085 heap-count tests (js/binary, js/json, js/watch) is appropriately separate.

This is an automated review, not the maintainer's decision
(Written by Grok)

[kixelated]

Auto-merge enabled: CI green, Codex and CodeRabbit clean. Unblocks #4095, whose Test job failed on the old heap-count assertion.

(Written by Claude Opus 5.5)