Repository navigation
fix(docx): close display-list handles instead of leaving them to finalizers - #1323
Merged
Merged
Conversation
…held, close a line's handles when it ends
…otype; pin that unmount closes display-list handles
Contributor
|
All contributors have signed the CLA — thank you! ✍️ ✅ Posted by the CLA bot. |
|
kyora · 5/5 · low risk Closes docx display-list store handles and worker selection reads explicitly instead of relying on finalizers. Summary:
Reviewer models
Review 1 · d70fe7d · comment |
Merged
eliahilse
added a commit
that referenced
this pull request
Oct 15, 2026
TL;DR: every workflow runs Bun 1.4.3 instead of 1.4.2. - 1.4.3 fixes the `bun test --isolate` segfault when a finished test file's global is collected while FinalizationRegistry callbacks are still queued (oven-sh/bun#44279). Our CI crash has the same stack; #1323 avoids it on 1.4.2 by disposing wasm handles eagerly. - The node:vm repro from oven-sh/bun#44279 segfaults 10/10 on 1.4.2 and 0/10 on 1.4.3 (Linux x64). - CI's test step on main, 4 vCPU Linux VMs, arms interleaved: no segfault on either; failed tests per run 16, 4, 7, 1, 5, 6 on 1.4.2 vs 7, 2, 0, 1, 0, 0, 0 on 1.4.3; median wall time 1189 s vs 1157 s. - `scripts/native-bindings.test.ts` expects the new pin. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TL;DR: the docx editor closes its display-list store handles when they stop being used, so
bun test --isolateno longer leaves hundreds of finalizer registrations behind per test file.Why
CI's Test step crashes now and then on Bun 1.4.2 with a segfault in
JSFinalizationRegistry::runFinalizationCleanup(bun.report traces of three CI crashes). Cleanup jobs for registrations that are still live when an--isolatefile's realm is torn down run against a freed callback. A standalone repro on Bun 1.4.2 crashed 5 of 5 runs when registrations were left live as their context died, and 0 of 5 when they were unregistered first. The upstream fix (oven-sh/bun#44279) landed after 1.4.2. The crash depends on timing and never reproduced on our test VMs, so this PR measures live finalizer registrations at the end of each file instead.The largest source was the docx display-list query facade. Every facade registered with a
FinalizationRegistrywhen it was created, even if it never opened a store handle. Nothing disposed facades when the editor unmounted.What changed
displayListQueries.ts: a facade registers for finalization only while it holds a store handle, and the handle cell is the unregister token.endDisplayListQueriesLinecloses every handle held by that line's facades.useDisplayListalready ends its open lines on unmount, release, layout reset and worker fallback, so those paths now close their handles. A query method called after its facade has been garbage collected answers over the JSON path and opens no handle.residentEngineWorker.ts/residentCommandSelection.ts: the worker releases its selection queries and per-engine selection reads when the command host is rebound and when the session is destroyed.Tests
New (red on main, green here):
displayListQueries.finalizer.test.ts: only a facade that holds a handle is registered; adoption moves the registration; a failed by-handle query drops it; ending a line closes and unregisters every handle; a method kept past its collected facade opens no handle.sessionLifetime.test.tsx: unmounting the editor closes every display-list store handle it opened.residentEngineWorker.navigation.test.ts: rebinding the command host disposes its selection queries.Changed (teardown only):
displayListQueries.test.ts,frameDelta.test.ts,showHiddenText.test.ts,useYrsCoreSession.workerOpen.test.tsx,residentCommandSelection.test.ts,residentEngineWorker.navigation.test.ts,XlsxEditorPlugins.test.tsx,commandSwap.test.tsx,commandHost.shared.test.ts,commandSwap.test.ts,recovery.test.ts.Verification
Measured in one run on an x86_64 Linux VM with Bun 1.4.2, at d70fe7d. That VM was loaded: CI's test command took 2765 s there. Pure main 7166688 took 1001 s on a quiet VM.
docx,docx-react,xlsxandxlsx-reactbuilds pass.displayListQueries.finalizer.test.ts, 3 runs per source:displayListQueries.ts: 0 pass / 5 failresidentEngineWorker.navigation.test.tswith main's worker sources: 11 pass / 1 fail ("rebinding_command_host_disposes_selection_queries"). Head: 12 / 0.bun run test: 9336 pass / 65 fail / 120 skip / 10 todo across 658 files, and no segfault. Pure main on the quiet VM: 9391 pass / 3 fail, with no unhandled errors.DocxEditorPlugins.test.tsxuseFileIO.comments.test.tsxuseDisplayList.workerLayout.test.tsviewportFrames.parity.test.ts,previewChain.parity.test.ts,useYrsCoreSession.workerOpen.test.tsx("a successful worker-recovery main open ...") andVsdxEditor.lifecycle.test.tsx(font face restore, which takes 2.4 to 3.1 s even on GitHub's runners)version()call on a freed session inDocxEditorPlugins.test.tsx,window is not definedin a React act flush inuseFileIO.comments.test.tsx, and awaitForinuseDisplayList.workerLayout.test.ts. None comes from a finalization callback.DocxEditorPlugins.test.tsxdiffered. Main's sources gave 52 / 0 twice. Head gave 1 / 51, then 51 / 1. In both head runs the in-thread first test timed out on its own, at 5147 and 5050 ms, and in the 1 / 51 run every later mount timed out after it (see Deferred). In the other five files head had no more failures than main's sources in either round.Live finalizer registrations at file end (measured)
Each test file runs on its own, with a preload that counts live
FinalizationRegistryregistrations when the file ends. "Handles" counts display-list store handle registrations. "Other" counts edit sessions and xlsx documents. Before = pure main 7166688; after = this head, from the run above. The crash never reproduced on our VMs (no segfault in either run), so this count is the measure.useYrsCoreSession.workerOpen.test.tsx*useDisplayList.lazyPages.test.tsDocxEditorPlugins.test.tsx*useDisplayList.workerLayout.test.ts*useFileIO.untouchedParts.test.tsxuseDisplayList.provenance.test.tsxpreviewFailure.test.tsxuseDisplayList.outOfMemory.test.tsdisplayListQueries.test.tsresidentEngineWorker.navigation.test.tswhenLayoutComplete.test.tsxuseDisplayList.handoff.test.tsuseDisplayList.fallback.test.tsgeometry.stories.test.tsfirstPagePainted.test.tsx*EditorToolbarProp.test.tsx*hostProposals.test.tsxsaveCycles.test.tsx*sessionLifetime.test.tsxviewerSelectionPublication.test.tsxframeDelta.test.tsresidentCommandSelection.test.tsproposalToggle.geometry.test.tsx*useDocxPluginHost.lazyPages.test.tsxuseDocxEditorRefApi.worker.test.tsxuseFileIO.pickedImage.test.tsxloadingIndicator.test.tsxresidentCommandWorker.test.tsXlsxEditorPlugins.test.tsxcommandHost.shared.test.tscommandSwap.test.tscommandSwap.test.tsxrecovery.test.tsshowHiddenText.test.tsFiles at 0 on both sides are left out. A file marked * had failures in the after run, so its count only covers the tests that completed. With this branch merged into #1318's head, every marked file passes and holds 0 handles. Across 147 files, #1318's head alone holds 1119 + 11 and the merge holds 13 + 0; 9 of the 13 are in
workerPluginGeometry.test.ts(see Deferred).Deferred
workerPluginGeometry.test.ts(from feat(docx-react): editing in the worker-only docx editor #1319) never disposes its plugin harness and leaves 9 handle registrations live. feat(docx): plugin layout reservations #1318's next head fixes it.residentCommandWorker.test.tsfrom its fixture, and 1 each inuseYrsCoreSession.workerOpen.test.tsx,useFileIO.untouchedParts.test.tsxanddisplayListQueries.test.ts. One edit session also stays live inuseDisplayList.workerLayout.test.ts. That count comes from a run where two of the file's tests failed; in the merge with feat(docx): plugin layout reservations #1318 the file passes and holds 0.deadSources.has is not a functionatdisplayListQueries.ts:339, on a main-based branch.saveCycles.test.tsx: on loaded VMs, all 4 tests fail together in about 1 of 3 runs of the file on main, and they failed on this head too. Each is a mount or startup timeout: the in-thread arm hits the 5 s test timeout while it waits for Save to enable. No comment-range or paragraph assertion fails.useYrsCoreSession.workerOpen.test.tsx, "the editor state prefetch waits for a sliced background page-build reply with metadata bootstrap": run alone with a name filter, it fails 3 of 3 runs on main 89a5631. It passes in full-file runs.DocxEditorPlugins.test.tsxon loaded VMs: the first test times out at 5 s and its body keeps running. Every later mount in the file then times out, giving 1 pass / 51 fail on main and on this head.🤖 Generated with Claude Code
Co-authored-by: Claude Opus 5.5 noreply@anthropic.com
Co-authored-by: GPT 6.1 Sol codex@openai.com