Repository navigation
DocgenServer: Fix OOM by recycling the shared TS program - #35305
Conversation
Package BenchmarksCommit: No significant changes detected, all good. 👏 |
|
@JReinhold, what do you think the impact of the perf regression would be for the mcp-server? |
66b380c to
78a904d
Compare
e89bf32 to
e4903ff
Compare
There was a problem hiding this comment.
Pull request overview
This PR addresses Node.js OOM crashes in the experimental docgen-server workflow by adding heap-pressure-based recycling of the shared TypeScript program used for react-component-meta extraction, and by introducing a CI regression gate to ensure the crash→survive behavior remains stable over time.
Changes:
- Add heap-pressure-triggered disposal + lazy rebuild of
ComponentMetaManager’s shared TS projects to reclaim TypeScript type-resolution caches under memory pressure. - Add a deterministic benchmark harness + CI gate (
bench:docgen-memory) to assert both steady-state behavior and a crash→survive negative control. - Wire the new gate into the daily CircleCI workflow.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/package.json | Adds bench:docgen-memory script to run the new regression gate. |
| scripts/ci/main.ts | Includes the new docgen memory gate job in the daily workflow. |
| scripts/ci/common-jobs.ts | Defines the docgenMemoryGate CircleCI job to execute the gate. |
| scripts/bench/docgen-memory/memory-harness.ts | Implements an in-process harness to reproduce/measure docgen-server memory pressure deterministically. |
| scripts/bench/docgen-memory/generate-project.ts | Generates a Storybook-shaped React project used by the harness/gate. |
| scripts/bench/docgen-memory/gate.ts | Runs the harness in child processes and enforces regression assertions (metrics + crash→survive pair). |
| code/renderers/react/src/componentManifest/componentMeta/ComponentMetaManager.ts | Implements heap-pressure recycling of TS projects after extraction to prevent OOM in repeated extraction scenarios. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds heap-pressure-based recycling of TypeScript projects in ChangesDocgen memory fix and benchmark tooling
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CI as CircleCI (docgenMemoryGate)
participant Gate as gate.ts
participant Harness as memory-harness.ts
participant Manager as ComponentMetaManager
CI->>Gate: yarn bench:docgen-memory
loop for each CONFIGS scenario
Gate->>Harness: spawn child process (mode, recycle, heap cap)
Harness->>Manager: batchExtract() across save/wave cycles
Manager->>Manager: recycleProjectsIfHeapPressured()
Manager-->>Harness: extraction results
Harness->>Harness: sample RSS/heap, compute slopes
Harness-->>Gate: exit code + result.json
end
Gate->>Gate: checkMetricConfig / checkNegativeControl
Gate-->>CI: exit 0 (pass) or exit 1 (failures)
Possibly related issues
Possibly related PRs
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@scripts/bench/docgen-memory/generate-project.ts`:
- Around line 15-17: The direct-run example in generate-project.ts points to the
wrong script name, so update the usage text to reference generate-project.ts
instead of generate.ts. Keep the example aligned with the actual entrypoint
shown in the file comment, and verify the command under the run-directly section
matches the script’s real filename.
- Around line 295-303: The self-exec guard in generateProject’s CLI entrypoint
is using a raw file URL string that breaks on Windows. Update the
import.meta.url comparison to build the process.argv[1] value with
pathToFileURL() so the direct-run check works cross-platform, and keep the logic
in the existing generateProject/parseArgs entrypoint block unchanged otherwise.
In `@scripts/bench/docgen-memory/memory-harness.ts`:
- Around line 333-346: The save-and-reextract loop in memory-harness.ts updates
component files on disk but never notifies ComponentMetaManager that files
changed. After fs.writeFileSync in the mutation path, call
manager.onFilesChanged() before manager.batchExtract(toExtract) so project
versions are bumped and refresh-mode measures the post-save state. Use the
existing manager and batchExtract flow as the insertion point.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b202105c-b95e-4768-82be-acf55ffa9580
📒 Files selected for processing (7)
code/renderers/react/src/componentManifest/componentMeta/ComponentMetaManager.tsscripts/bench/docgen-memory/gate.tsscripts/bench/docgen-memory/generate-project.tsscripts/bench/docgen-memory/memory-harness.tsscripts/ci/common-jobs.tsscripts/ci/main.tsscripts/package.json
|
@shilman, shall we add telemetry so we can know how often this happens? That would help prioritize, maybe investigating an alternate fix? |
- Repoint the recycle-ratio comment from the non-existent FINDINGS.md to the bench:docgen-memory gate that actually verifies the crash→survive behavior - Drop the stale INVESTIGATION.md reference in the project generator - Fix the generator's run-directly example to the real entrypoint and loader (generate-project.ts via jiti/register) - Use pathToFileURL() for the generator's self-exec guard so it fires on Windows - Notify ComponentMetaManager via onFilesChanged() after mutating files in the harness so refresh-mode measures a real post-save re-extract - Add unit tests asserting heap-pressure recycling disposes/rebuilds the shared program above the threshold and reuses it below Co-authored-by: Cursor <cursoragent@cursor.com>
Drop drift-prone measurements (per-component MB density, retained-set size, observed transient ranges) and "proving X not Y" narrative from the docgen memory scripts and recycle-ratio comment, keeping the maintenance rationale concise per the repo's comment guidance. Co-authored-by: Cursor <cursoragent@cursor.com>
…ssure Recycling the shared TS program mostly prevents the docgen-server OOM, but a single extraction can still exceed the cap. Add a categorized ReactDocgenServerMemoryLimitError (SB_CORE-COMMON_0016, handled) and emit its message once via the node-logger `once.warn` channel when recycling fires, so users get actionable guidance to raise NODE_OPTIONS=--max-old-space-size. Co-authored-by: Cursor <cursoragent@cursor.com>
Closes #35260
Related: #34824
What I did
Fixes an out-of-memory crash in the new docgen-server mode (
STORYBOOK_EXPERIMENTAL_DOCGEN_SERVER=true).On a large Storybook, editing a widely-imported component (e.g.
Button.tsx) can crash the dev server with:It's not a leak
I first checked whether this was a memory leak. It isn't: post-GC (garbage collection) retained memory stays flat across saves. What's actually happening is a spike problem, not a growth problem.
So, during a file save, the TS server is allocating memory, as well as the part trying to extract; there isn't enough headroom left, and the process runs out of memory.
This is why the crash felt random: it's not that memory keeps climbing, it's that two independent sawtooths occasionally stack high enough to blow the ceiling.
The fix
We need to create more memory headroom by running the GC.
I added a mechanism that lets the shared TypeScript program drop its internal cache once extract is complete, freeing up the memory it was holding. Concretely: the docgen server now checks memory usage, and once it crosses 70% of the available heap, it disposes the cached program (and lazily rebuilds it on the next request). This is on by default.
A couple of things worth calling out about the approach:
--max-old-space-sizeis set, and so for small projects it should never trigger.I verified that at a heap cap where the bug reproduces reliably (
--max-old-space-size=3072), the server without this fix crashes by the 7th edit. With the fix, it survives 20 edits in a row, recycling the cache twice along the way and settling back down each time instead of climbing to the cap.Trade-offs
Testing
Since the original bug report was on our internal Storybook, I didn't want to test against that directly, since it changes over time as stories/code/features are added/removed. A fix verified against it today could look "fixed" or "broken" later for unrelated reasons.
Instead, I built a synthetic generator that produces a large, TypeScript-heavy fixture project (many components + stories, no real UI) specifically for reproducing this memory profile on demand.
I added this as a daily CI check (
yarn bench:docgen-memory) that asserts two things:To run it locally:
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
A daily CI job (
yarn bench:docgen-memory) runs a synthetic in-process gate asserting:on must survive. Asserting the flip — not just survival — guards the fix itself.
under budget — catches a regression that reintroduces whole-index re-extraction.
Manual testing
yarn nx compile react --skip-nx-cacheReal-environment proof (not in repo — ad hoc script to reproduce against the real internal Storybook)
Boots
code/in docgen-server mode, editsButton.tsxin a loop, samples process-tree RSS.Not committed — tied to internal Storybook structure — but preserved here for reference.
Documentation
MIGRATION.MD
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli-storybook/src/sandbox-templates.tsDeclare whether manual QA will be needed for this PR during the next release, through
qa:neededorqa:skipMake sure this PR contains one of the labels below:
Available labels
bug: Internal changes that fixes incorrect behavior.maintenance: User-facing maintenance tasks.dependencies: Upgrading (sometimes downgrading) dependencies.build: Internal-facing build tooling & test updates. Will not show up in release changelog.cleanup: Minor cleanup style change. Will not show up in release changelog.documentation: Documentation only changes. Will not show up in release changelog.feature request: Introducing a new feature.BREAKING CHANGE: Changes that break compatibility in some way with current major version.other: Changes that don't fit in the above categories.🦋 Canary release
This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the
@storybookjs/coreteam here.core team members can create a canary release here or locally with
gh workflow run --repo storybookjs/storybook publish.yml --field pr=<PR_NUMBER>Summary by CodeRabbit
New Features
Bug Fixes
Tests