Skip to content

fix: bound unbounded retention in the run supervisor, blob URLs, and the caches - #702

Merged
colinhacks merged 1 commit into
mainfrom
fix-memory-leaks
Aug 8, 2026
Merged

fix: bound unbounded retention in the run supervisor, blob URLs, and the caches#702
colinhacks merged 1 commit into
mainfrom
fix-memory-leaks

Conversation

@colinhacks

Copy link
Copy Markdown
Contributor

Seven memory/disk defects, each reproduced and re-measured against plain Node.

Defect Before After
run -r retained every output line 45→325 MB/50s flat 15 MB
Non-TTY buffered the whole run climbing flat 29 MB
Object URLs kept a decoded heap copy +43 MB/20k +5.6 MB
Blobs pinned source buffers 504 MB ext 4 MB
Sweep never ran for sync runs never 527→383 MB
Compile-cache dir never evicted 6.9 GB swept daily
tsconfig memo unbounded unbounded 8192 cap

Also fixes data: URLs with an extension-shaped tail throwing ERR_INVALID_URL_SCHEME.

Gates clean, 220/220 integration, bootstrap module list byte-identical.

A blob: worker source over 4 MB is no longer accepted; documented.

…the caches

Seven defects, each reproduced with a fixture before and after against a plain
Node control.

`nub run -r` retained every prefixed output line for the child's whole life,
including in streaming mode where the line had already been printed and nothing
read the copy back. For a script that never exits — `nub run -r dev`, the
standard monorepo workflow — the supervisor grew 1:1 with child output: 45 ->
325 MB in 50 s, now flat at 15 MB. The aggregate path buffered for the entire
run, and a non-TTY stdout selects it too, so CI and piped runs took it with a
dev server; it now flushes early past 8 MiB, measured 250 MB climbing -> flat
29 MB with all output still delivered in order. The dead `output_buf` return,
discarded at all three call sites, is gone.

URL.createObjectURL stored a decoded UTF-8 copy of every blob on the V8 heap,
and every Blob pinned whatever its construction parts referenced. Nub now
captures one compact Buffer when the Blob is built, capped at 4 MiB, and decodes
only when a Worker asks for the source: heap growth for 20k object URLs +43.1 ->
+5.6 MB (plain Node: +1.1), and 2000 Blobs holding 64 KB of real data between
them now retain 4 MB of external memory instead of 504 MB.

The transpile-cache sweep never ran for a purely synchronous program: it was
armed on an unref'd setImmediate that the process exited before running, so a
user whose runs are all synchronous never swept at all. It is now scheduled only
when a sweep is due, and ref'd. The async-loader tier and the compat tier never
swept either; both do now.

Nub's own V8 compile-cache dir had no eviction of any kind — 6.9 GB across
594k files after 12 days on one machine, one subdirectory per Node build ever
run. It is swept on the same daily schedule, and only when NODE_COMPILE_CACHE
points at Nub's own directory, never at one the user chose.

The cache cap counted stat.size while the disk allocates whole blocks, so a
nominal 512 MiB cap permitted ~594 MB of real disk; it now counts blocks.

The tsconfig memo is keyed by directory with no bound, and it lives in the addon
inside the user's own process, so a program that creates directories at runtime
grew it forever. Capped at 8192 entries.

A `data:` URL whose payload ends in something extension-shaped (a trailing
`//x.ts` comment, a sourceMappingURL) threw ERR_INVALID_URL_SCHEME where plain
Node imports it without complaint.

Not changed: transpiled output carries an inline source map unconditionally,
which costs ~2x per-module heap against plain Node. That is a deliberate feature
with no config surface, so it is a product decision rather than a defect.
Copilot AI lite review requested due to automatic review settings August 8, 2026 21:30
@vercel

vercel Bot commented Aug 8, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
nub Ready Ready Preview Aug 8, 2026 9:30pm

Request Review

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

Three of the seven fixes have gaps worth resolving before merge: the Blob capture trades one retention bug for a new unconditional per-Blob copy, the aggregate ceiling misses one of the two drain paths, and the new compile-cache eviction cannot fire on Windows.

Reviewed changes — full read of the single commit e0106ad across all 12 files, plus the surrounding supervisor, preload, and cache-path code it depends on.

  • Streaming drain retains nothing. DrainPolicy::emit returns None outside aggregate mode, and spawn_script_prefixed drops its String return. Traced every consumer of the drained Vec<String> and the removed tuple element — nothing else read them back, and the failure path prints only exit {code}.
  • Aggregate hold ceiling. AGGREGATE_MAX_HELD_BYTES (8 MiB) plus flush_aggregated() early-flush inside the new run_capped, so a never-exiting child under the non-TTY aggregate default can no longer grow the supervisor for the life of the run.
  • Blob capture reworked. decode()encodeParts(): a compact off-heap Buffer taken in the Blob construct trap, capped at 4 MiB, with the UTF-8 decode deferred to the new blobUrlSource() accessor that worker-polyfill.mjs now calls.
  • Sweep actually runs. New sweepDue() gates a ref'd setImmediate in both tiers, replacing the unconditional .unref()'d one that a synchronous program always outran — and the async/compat tier now sweeps at all.
  • Compile-cache eviction. sweepCompileCache() walks Node's nested <version>/<entry> layout, prunes emptied version dirs, and only ever touches nub's own dir.
  • Real on-disk accounting. diskBytes() uses blocks * 512 with a logical-size fallback where blocks is unreported.
  • tsconfig memo capped at 8192 entries, cleared wholesale — reasonable for a pure memo.
  • data: URL fix. extname() short-circuits on data:, so an extension-shaped payload tail no longer routes the load hooks into the branches whose fileURLToPath threw ERR_INVALID_URL_SCHEME.

⚠️ The daily sweep now runs on the critical path of a user's process exit

Making the setImmediate ref'd is what fixes the never-swept-for-synchronous-programs defect, but it also means the once-a-day run pays for the whole sweep after user code has finished. The scan is readdir + one stat per entry with no time or count bound, over a tree the PR itself measured at ~594k files — so one nub script.ts per day appears to hang for seconds with no output, on a run that asked for none of this.

Technical details
# Daily sweep blocks process exit with an unbounded scan

## Affected sites
- `runtime/preload.cjs:220-224``if (core && core.sweepDue()) { setImmediate(...) }`, no longer `.unref()`'d.
- `runtime/preload.mjs:161-165` — same shape on the compat tier.
- `runtime/cache-evict.mjs:96-126``sweepCompileCache` stats every entry under every version dir before deciding whether anything is over cap; the scan runs even when `total <= maxBytes`.

## Required outcome
- A user's short-lived run should not be able to stall for an unbounded time on cache maintenance it did not ask for, while still guaranteeing the sweep actually happens for a purely synchronous program.

## Open questions for the human
- Is a multi-second, output-free stall once per day acceptable, or should the sweep be detached (spawned as a background process) / time-boxed so the foreground run exits promptly?
- Would writing the `.sweep` sentinel and bailing when the scan exceeds a budget be preferable to completing it inline?

ℹ️ Nitpicks

  • crates/nub-cli/src/cli.rs:5824-5825 — the doc comment still says "Returns (exit_code, collected_output)"; the function now returns Result<i32>.
  • runtime/worker-blob-url.cjs:175blobUrlSources is still exported, but worker-polyfill.mjs now imports blobUrlSource instead and no other consumer remains. Worth dropping from the export list so the accessor is the only door in.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using Claude Opus𝕏

// with no entry simply is not usable as a worker entry. Never let our
// accounting fail a Blob construction that vanilla Node would allow.
try {
const bytes = encodeParts(args[0]);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

encodeParts runs in the construct trap, so every new Blob(...) on every nub run now allocates a fresh Buffer.concat copy of up to 4 MiB and retains it for the Blob's lifetime — where the old code stored a reference to args[0] and only decoded inside createObjectURL. For a program that keeps its source buffers (or never calls createObjectURL at all, which the comment above says is the overwhelming majority), that is a strict memory and CPU regression against both pre-PR nub and plain Node.

Technical details
# Eager Blob capture imposes a copy on every construction

## Affected sites
- `runtime/worker-blob-url.cjs:117-128` — the construct trap calls `encodeParts(args[0])` unconditionally.
- `runtime/worker-blob-url.cjs:52-69``encodeParts` always ends in `Buffer.concat(chunks, total)`, which allocates and copies even for a single-chunk list (verified: `Buffer.concat([b], b.length) !== b`).
- `runtime/preload.cjs:303-306` and `runtime/worker-polyfill.mjs:450``installBlobUrlSupport()` is called unconditionally on the main thread on both tiers, so the trap is live in every nub process.

## Behavior delta
- `new Blob([sharedBuf])` in a loop, caller retains `sharedBuf`: before 0 marginal bytes, after one full copy per Blob (bounded at 4 MiB each).
- `new Blob([str])` never passed to `createObjectURL`: before no UTF-8 encode at all, after `Buffer.from(p, "utf8")` per construction.
- `new Blob([big.subarray(0, 32)])`: genuinely fixed by the copy — this is the case the PR measured.

## Required outcome
- Keep the subarray-pinning fix without charging every Blob construction a copy it will never use.

## Suggested approach (optional)
- Keep the WeakMap pointing at the parts and run `encodeParts` inside `wrappedCreate` instead, where the URL is minted. The capture is still synchronous and still compact, but only the Blobs that actually become object URLs pay for it. The residual pinning would then last only as long as the Blob the caller is holding anyway.

Comment thread crates/nub-cli/src/cli.rs

/// `run` with an explicit hold ceiling, so a test can exercise the early
/// flush without pushing the shipped 8 MiB through the harness's capture.
fn run_capped<R: std::io::Read>(&self, stream: R, max_held: usize) -> Vec<String> {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The hold ceiling only exists in run_capped. LinePipe — the unix poll(2) single-thread fallback used when thread-create returns EAGAIN — accumulates into self.lines with no cap and no flush_aggregated call (cli.rs:5797-5809 and 5813-5821), so on that path aggregate mode still grows for the life of a never-exiting child. That fallback exists precisely for resource-constrained CI containers, which is the same environment where the non-TTY aggregate default and a long-lived dev script meet.

Technical details
# Aggregate hold ceiling is missing on the poll(2) drain path

## Affected sites
- `crates/nub-cli/src/cli.rs:5478-5496``run_capped` tracks `held` and calls `flush_aggregated` past the ceiling.
- `crates/nub-cli/src/cli.rs:5797-5809``LinePipe::drain_complete_lines` pushes every emitted line with no accounting.
- `crates/nub-cli/src/cli.rs:5813-5821``LinePipe::finish` does the same for the trailing partial line.
- `crates/nub-cli/src/cli.rs:5647-5724``drain_both_inline` is the only caller; reached from `PipeReaders::drain` when `libc::dup` or `Builder::spawn` fails.

## Required outcome
- Both drain implementations enforce the same `AGGREGATE_MAX_HELD_BYTES` ceiling, so no reachable path can grow the supervisor 1:1 with child output.

## Suggested approach (optional)
- Give `LinePipe` a `held: usize` field, incremented where it pushes, and call `flush_aggregated(&mut self.lines, self.policy.is_stderr)` past the ceiling — the same three lines `run_capped` uses.

## Open questions for the human
- Neither new unit test exercises `LinePipe`; is it worth a test there, or is the EAGAIN path considered out of reach for the suite?

// sweep-due probe and the compile-cache check can name a directory without a
// mkdir side effect on every startup.
function cacheRoot() {
const base = process.env.XDG_CACHE_HOME || (process.env.HOME ? join(process.env.HOME, ".cache") : null);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cacheRoot() derives from XDG_CACHE_HOME || HOME, but the Rust side that creates the dir and sets NODE_COMPILE_CACHE uses dirs_next::home_dir() — the Known Folder API on Windows, with a %LOCALAPPDATA% fallback — so process.env.HOME is normally unset there and cacheRoot() returns null. sweepDue() then returns false unconditionally and the new compile-cache eviction never runs on Windows, which is the platform-independent half of the 6.9 GB growth this PR is fixing.

Technical details
# JS cache-root derivation diverges from the Rust one on Windows

## Affected sites
- `runtime/transform-core.mjs:663-667``cacheRoot()`: `XDG_CACHE_HOME || (HOME && HOME/.cache)`, else `null`.
- `runtime/transform-core.mjs:697-704``sweepDue()` returns `false` when `cacheRoot()` is `null`, so the deferred sweep is never scheduled at all.
- `runtime/transform-core.mjs:731-734``ownCompileCache` is `null` for the same reason, so `sweepCompileCache` is skipped even if `maybeSweepCache` were reached.
- `crates/nub-core/src/node/discovery.rs` `cache_dir()` — Windows branch routes through `windows_cache_dir(dirs_next::home_dir(), LOCALAPPDATA, SystemRoot)`.
- `crates/nub-core/src/node/spawn.rs` `default_compile_cache_dir()``cache_dir()?.join("v8-compile-cache")`, created and pointed at by `NODE_COMPILE_CACHE` on every augmented run.

The repo already documents the mismatch: `crates/nub-cli/tests/pm_shim_windows.rs:9-11` and `.github/workflows/ci.yml:667-669` both note that `dirs_next::home_dir()` reads the Known Folder API and ignores env `HOME` on Windows. A second, narrower mismatch exists even when `HOME` *is* set (Git Bash / MSYS): a service account trips `is_windows_system_home`, Rust routes to `%LOCALAPPDATA%\nub`, and the JS side has no `LOCALAPPDATA` branch at all.

## Required outcome
- The JS-side root resolves to the same directory the Rust side created, on every supported platform — or the sweep learns the dir from the Rust side rather than recomputing it.

## Suggested approach (optional)
- Since `NODE_COMPILE_CACHE` already carries the authoritative absolute path, consider deriving nub-ownership from it (e.g. the Rust side marking the dir, or comparing against a value threaded through the existing sentinel) instead of recomputing the root in JS. That removes the whole class of platform-derivation drift.

## Open questions for the human
- Is the transpile cache in fact inert on native Windows today for the same reason (`getCacheDir()` shares this base derivation)? If so that is pre-existing, but the new eviction inherits it and it is worth confirming rather than assuming.

Comment thread crates/nub-cli/src/cli.rs

const CAP: usize = 2 * 1024;
// ~16 KiB of input — eight times the ceiling, small enough that the early
// flushes this deliberately triggers stay out of the suite's output.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment claims the early flushes "stay out of the suite's output", but flush_aggregated writes through std::io::stdout().lock() directly. libtest's capture only intercepts the print!/println! macro family via the OUTPUT_CAPTURE hook, not raw handle writes — so this test dumps ~14 KB of synthetic zzz… lines into every cargo test run. Either shrink the input to a few hundred bytes with a proportionally smaller CAP, or make the flush sink injectable so the test can assert without writing anywhere.

@colinhacks
colinhacks merged commit d8e30f1 into main Aug 8, 2026
66 checks passed
@colinhacks
colinhacks deleted the fix-memory-leaks branch August 8, 2026 21:58
@pullfrog pullfrog Bot mentioned this pull request Aug 8, 2026
@colinhacks

Copy link
Copy Markdown
Contributor Author

Shipped in v0.7.5: https://github.com/nubjs/nub/releases/tag/v0.7.5

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.

2 participants