Skip to content

test(vm): observe collected realms before asserting cancellation - #151

Merged
steipete merged 1 commit into
mainfrom
claude/vm-finalization-collected-realms
Oct 9, 2026
Merged

steipete merged 1 commit into
mainfrom
claude/vm-finalization-collected-realms

Conversation

@steipete

@steipete steipete commented Oct 9, 2026 •

Copy link
Copy Markdown

The FinalizationRegistry fixture assumed that dropping four VM contexts meant one full collection had found all four realms dead. The scheduler only cancels pending work for unmarked realms, so a surviving context may legitimately run its cleanup job.

Adapts the FinalizationRegistry portion of oven-sh/bun#44544, currently open; thanks @robobun. Each dropped sandbox gets a WeakRef, checked synchronously after the existing full collection. The owning NodeVMGlobalObject strongly visits its sandbox, so a cleared witness establishes that its realm was unmarked. Every observed-collected realm must have zero callbacks. At least one of four must be collected: that exercises cancellation and prevents a vacuous pass without requiring a particular conservative-root layout.

The fixture also requires zero callbacks before the collection under test. It preserves the collection schedule, live-context completion, following turn, child exit/stderr checks, and the adjacent Atomics assertions. Production code is unchanged.

Validation:

  • The revised fixture passes in a local debug/ASAN build using pinned WebKit cb8d6f202b5a396caa204ee1bb75d78175aa841a.
  • With only JSCTaskScheduler::cancelWorkOfDeadRealms disabled, that same test fails. The runner reports its existing 5-second timeout; the recovered crash report matches the disabled executable UUID and records an ASAN abort through runPendingWork → JSFinalizationRegistry::runFinalizationCleanup → takeDeadHoldingsValue. Disassembly confirms the disabled function returns without cancellation. This is one failed negative-control invocation, not a retry-to-green.
  • The full debug VM directory reports 371 pass, 4 skip, 61 todo and one failure: unchanged script-leak.test.ts exceeds its existing 5-second timeout (35.987 seconds). Its assertions and timeout have not been changed, and the failed suite has not been retried. The revised cancellation test passes within that suite.
  • Independent P2 review is scoped-clean. The proof-only native mutation is removed and the checkout is clean.

Exact-head native fork CI passes on Linux x64 and macOS ARM64. Linux reports 16 passing result entries, including 340 pass / 3 existing skip / 60 existing todo / 0 fail in vm.test.ts. macOS reports 12 passing result entries, including 339 pass / 4 existing skip / 60 existing todo / 0 fail in the same file. CI merge commit 53b3c165aa98fbd6e850493b1b7f4536fa128a12 has exactly the same tree as PR head 14898da7a6cf46c13c0b5ef282a9abd0f54a3118: 187496412861ac8fbce891b58f6f1c6fe020c370.

Formatting and JavaScript lint pass. The inherited issue-finding bot fails credential validation before code analysis; it has not been rerun. The supplementary full-directory debug timeout above remains a recorded limitation, not an all-tests-green claim. This test-only PR preserves every other assertion and makes no runtime changes.

@steipete
steipete marked this pull request as ready for review October 9, 2026 17:11
@steipete
steipete merged commit 4c8accd into main Oct 9, 2026
6 of 7 checks passed
@steipete
steipete deleted the claude/vm-finalization-collected-realms branch October 9, 2026 17:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant