Repository navigation
fix(cmux-next pages): a failed subscribe releases its early-event list on every exit - #27500
Conversation
…s provider still sends (red) PageRouterSubscribeEndTests: a provider keeps onEvent and its subscribe throws, or the page closes while it runs. 10 000 events it sends afterwards must make the router allocate nothing (main-thread malloc_logger count) and reach no page. Red on feat-cmux-next 3b3ea3b: the throw and closed exits of PageRouter.subscribe never release the PageEarlyEvents its onEvent captured (review of fix/page-router-early-events; found on perf/chrome-parity, cp/fix-tip-page-router-release-on-throw).
…t on every exit PageRouter.subscribe released the PageEarlyEvents its onEvent captures only on the success paths. A subscribe that threw, or ran while the page closed, left it holding, so a provider that kept onEvent appended every later event to a list nobody reads, for the rest of the session. A defer now releases it on every exit unless the posted flush took it over. Regression test: PageRouterSubscribeEndTests (red in the previous commit).
…ver an empty loop Probe 5529e509 (fleet, Debug test build) counted each step of the drop path over 10 000 iterations: the empty loop alone made 10 000 allocations, and every step, the whole drop path included, made the same 10 000. So the one per event in the green proofs (3e081c97, de37d97f) is the Debug for-in loop, not the router: a dropped event allocates nothing. The test now subtracts that empty loop and still expects 0. The bound is unchanged: the red code still adds 13 (the held list growing, 10013 - 10000 in reds 3847df43 and fb666e88).
Bugbot needs on-demand usage enabledBugbot uses usage-based billing for this team and requires on-demand usage to be enabled. A team admin can enable on-demand usage in the Cursor dashboard. |
|
cmux status (one comment, edited in place; each line is updated by the job that owns it) Review App of this head (app.zip, kept 1 day)
red: cmux-next checks (god files, concurrency, crash safety, l10n); base red: the same failure on the base 30c653a; does not block Shipped in:
|
|
Found 6 test failures on Blacksmith runners: Failures
|
|
Bead |
|
Review (codex gpt-6-astra high fast, drain lane): SHIP at ea37a89 No blocking findings. |
![Fix with [code]smith](https://pr-comments-assets.blacksmith.sh/codesmith/fix-with-codesmith-light-3x.png)
Summary
A page whose provider's
subscribefails (it throws, or the page closes while it runs) leaked memory for the rest of the session when that provider kept itsonEventclosure: every event it still sent was appended to a list nobody reads.PageRouter.subscribemakes aPageEarlyEventslist that the provider'sonEventcaptures, to hold events sent before the subscribe reply. It released the list only on the success paths. On the throw andclosedexits it stayed open, so later events kept growing it. Now one owner frees it on every exit: adeferreleases it unless the posted flush has taken it over. After the release, a late event of a failed subscribe goes todeliver, which drops it (the stream is not live).Found in the review of #27468 (same function; the two merge cleanly). The same leak is fixed on perf/chrome-parity in
cp/fix-tip-page-router-release-on-throw.Changelog
Fixed: A page whose stream failed to start no longer keeps memory for every event its provider still sends.
Testing
Regression test
PageRouterSubscribeEndTests(red commit fee857b first, fix b3a4dff, test shape ea37a89). A provider keepsonEventand its subscribe throws, or closes the page; 10 000 events it sends afterwards must add 0 main-thread allocations over the same empty loop (counted through libmalloc'smalloc_logger; a self-check fails the test when the hook sees nothing) and reach no page. The empty loop is subtracted because a Debug for-in allocates once per iteration on the fleet (probe 5529e509: an empty loop and every step of the drop path each counted exactly 10 000).cmux-ci run --class light --script scripts/ci/package-test-lane.sh --arg suite --arg Packages/macOS/CmuxNext --arg PageRouterSubscribeEndTests(throughproof-step.py):kept → 13in both cases (the held list growing).CMUX_SWIFT_SUITE_CONFIGURATION=release), on perf (base 96cdd91, where the Release test build compiles):kept → 13in both cases. The leak is real in an optimized build.Compile:
cmux-ci compile-check --app --package CmuxNext --recipe-version 2at ea37a89 (fleet 6dc98323567433da9601e981): passed.Observed / Not run
Observed: the fleet runs above;
scripts/cmux-next/prepush-gate.pyclean.Not run: a Release run on feat itself (feat's Release test build does not compile yet:
UiLayerPreviewTestsand two allocation tests; fixed on perf incp/fix-tip-release-test-compile).Proof
No capture: memory-only fix in the page router; nothing on screen changes.
Review (independent subagent, correctness first): SHIP at ea37a89
Follow-ups noted in review, not in this PR: a test comment names
CrashKeySlotsTests, which is on perf only; after #27468 the nothing-held path releases the list twice (harmless,release()is idempotent); and an older issue: areset()while a provider'ssubscribeis suspended registers the old subscription in the new document.Checklist
PageRouterSubscribeEndTests)Need help on this PR? Tag
@codesmith-botwith what you need.Summary by cubic
Fixes a memory leak in
PageRouter.subscribewhere a failed subscription kept its early-event list alive for the rest of the session.When a provider's
subscribethrew, or the page closed while it ran, thePageEarlyEventslist captured byonEventwas never released. A provider that keptonEventwould append every later event to a list nobody reads. Adefernow releases the list on every exit unless the posted flush has taken ownership, so late events of a failed subscribe are dropped immediately.Bug Fixes
PageRouterSubscribeEndTeststo verify a failed subscribe allocates nothing for 10 000 events sent afterwards.Written for commit ea37a89. Summary will update on new commits.