Skip to content

Commit c777c8e

Browse files
committed
docs(plan): fold the five audit blockers back into the RSS unit
The adversarial review caught two factual errors in the upstream record and three design gaps. All five are folded in rather than argued away. Bun #32111 is closed, not open -- merged #32120 closed it. The honest status is fixed upstream but present in no stable release, and it still reproduces on Darwin arm64 under the bundled v1.3.14, so the crash risk stays live even though the issue is shut. #28743 was likewise closed unmerged, not open; it now reads as historical diagnosis instead of an active upstream fix path. The cancellation gap is the substantive one. The tee path aborts upstream on client cancel and drops a terminal that arrives moments later; the eager path keeps draining until a bounded terminal/byte/time limit and records it. Rather than force parity, the unit adopts the eager behavior deliberately: the #44 comment already argues a turn that genuinely reached a terminal must log as completed rather than be downgraded to a cancel. Three endpoint tests pin it, including a no-terminal control proving the drain bound itself fires. Phase 4 cells D/E/F now carry a real harness contract -- one controller, one spawned process per role, mandatory vmmap/footprint, and a checked property that the standalone cells never import src/server/. Measuring proxy-free topologies inside a proxy process would contaminate the attribution. The watchdog rationale was also wrong: warnings are already rate-limited to once per 30 minutes, so the defect is not noise but that the signal is level-only and cannot tell a harmless plateau from a live climb.
1 parent 8f53fee commit c777c8e

3 files changed

Lines changed: 104 additions & 14 deletions

File tree

devlog/_plan/260731_macos_rss_retention/000_plan.md

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,9 @@ action stays as an operator escape hatch and must never be reported as the fix.
4242
(closed unmerged 2026-06-26) attributes it to freed pages retained in libpas
4343
caches and mimalloc per-thread heaps — allocator retention, not a live-object leak.
4444
- Closer to the fetch path: [oven-sh/bun#28743](https://github.com/oven-sh/bun/pull/28743),
45-
open — HTTP-thread allocations freed on the JS thread land in a delayed-free list.
45+
**closed unmerged** — HTTP-thread allocations freed on the JS thread land in a
46+
delayed-free list. Historical diagnosis only; it is not an active upstream fix
47+
path and must not be cited as one.
4648
- No Bun-specific `tee()` retention issue or fix exists. Do not conflate #1235 with one.
4749

4850
### E2 — two corrections to earlier assumptions
@@ -51,9 +53,13 @@ action stays as an operator escape hatch and must never be reported as the fix.
5153
merged 2026-06-21 but ships in no stable release. Latest stable is v1.3.14
5254
(2026-05-13), which the repository already bundles. Adopting a canary as the
5355
default runtime was rejected by the user and is moot regardless.
54-
2. **#32120 is not Windows-only.** Its issue [#32111](https://github.com/oven-sh/bun/issues/32111)
55-
reproduces on Darwin arm64. It is an async-stream client-abort crash, not a
56-
retention defect, so it does not explain this symptom either way.
56+
2. **#32120 is not Windows-only, and #32111 is closed rather than open.**
57+
[#32111](https://github.com/oven-sh/bun/issues/32111) was CLOSED by merged
58+
#32120. The precise status is "fixed upstream, present in no stable release" —
59+
not "open defect". It still reproduces on Darwin arm64 under Bun 1.3.14, which
60+
is the bundled runtime, so the crash risk is live for us even though the issue
61+
is closed. It is an async-stream client-abort crash, not a retention defect, so
62+
it does not explain the RSS symptom either way; it constrains Phase 3's gate.
5763

5864
### E3 — isolated reproduction (Bun 1.3.14, macOS arm64, 3 runs each)
5965

devlog/_plan/260731_macos_rss_retention/030_macos_single_reader.md

Lines changed: 50 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -5,11 +5,15 @@
55
Enable the existing bounded eager relay for **Darwin and win32 only** when the
66
client does not require a payload rewrite. `streamMode: "auto"` remains tee on
77
Bun 1.3.14; macOS can use the relay now only through the existing explicit
8-
`streamMode: "eager-relay"` operator opt-in. That is deliberate: Bun#32111 is
9-
reproducible on Darwin arm64, and a process crash is worse than the measured
10-
memory growth. Once a released Bun version is independently verified to contain
11-
the async-pull cancellation fix, setting `MIN_FIXED_BUN_VERSION` makes `auto`
12-
select this path on both supported platforms.
8+
`streamMode: "eager-relay"` operator opt-in. That is deliberate, and the precise
9+
upstream status matters: Bun#32111 is **closed**, having been fixed by merged
10+
PR #32120 (2026-06-21) — but that fix ships in **no stable release**, and the
11+
bundled runtime is v1.3.14 (2026-05-13). So the crash is fixed upstream and still
12+
live for us, and it reproduces on Darwin arm64, not only win32. A process crash
13+
is worse than the measured memory growth, so `auto` stays on tee. Once a released
14+
Bun version is independently verified to contain #32120, setting
15+
`MIN_FIXED_BUN_VERSION` makes `auto` select this path on both supported platforms —
16+
that is the single trigger that retires this gate.
1317

1418
This does not claim to solve all macOS retention. The isolated HTTP experiment
1519
reduced peak `external` from 363–461 MiB to 10–22 MiB, while retained RSS still
@@ -43,6 +47,47 @@ the proven tee amplifier; Phase 4 owns the allocator/HTTP residual.
4347

4448
## Shape and safety boundary
4549

50+
### Cancellation semantics: an intentional #44 correction, not parity
51+
52+
Audit blocker 3 surfaced a real behavioral divergence, and the resolution is to
53+
ACCEPT it deliberately rather than paper over it.
54+
55+
Today the tee path cancels hard: `relay.ts:84-87` aborts `upstream` on client
56+
cancel, which propagates through `core.ts:2573-2581` to `turnAc` and cancels the
57+
inspection reader at `relay.ts:537-554`. A terminal that becomes observable just
58+
after the client disconnects is therefore DROPPED and the turn logs as a cancel.
59+
60+
The eager path instead keeps reading inspection-only until a terminal, byte, or
61+
time bound (`relay-eager.ts:130-138`), and reports cancel only when no terminal
62+
was seen (`relay-eager.ts:161-168`).
63+
64+
The #44 comment at `core.ts:1691-1694` states the intended policy directly:
65+
66+
> A real terminal was parsed from the (teed) inspection stream — record it as the
67+
> outcome even if the client has already disconnected: the turn genuinely reached
68+
> that terminal, so it must log as completed/failed, not be dropped or downgraded
69+
> to a cancel.
70+
71+
By that standard the eager behavior is MORE correct than the tee behavior it
72+
replaces. We adopt it knowingly: eager mode will record terminals the tee path
73+
drops. This is a deliberate, opt-in-scoped accounting change and must be called
74+
that in the release note — not described as parity.
75+
76+
Required endpoint tests (all three; the third is the activation control):
77+
78+
1. **Eager, terminal after cancel.** Upstream sends one delta, waits for the
79+
client reader to cancel, then emits `response.completed`. Assert: the client
80+
cancelled; the terminal lands inside the drain window;
81+
`onNativePassthroughTerminal === ["completed"]`; `onNativePassthroughCancel`
82+
was NOT called; continuation state was recorded; active turn count is back to 0.
83+
2. **Legacy tee characterization, same ordering.** Assert the opposite:
84+
`onNativePassthroughCancel` fires, and no post-cancel terminal or continuation
85+
is recorded. This pins the behavior we are knowingly changing.
86+
3. **No-terminal control.** After cancellation, hold the upstream open past the
87+
drain byte/time bound. Assert: cancel is invoked exactly once, no synthetic
88+
terminal is emitted, the turn is unregistered, and upstream reads stop. This
89+
proves the drain bound itself fires rather than running unbounded.
90+
4691
`decideEagerRelayForPlatform` is the single policy owner. It does not weaken
4792
`decideEagerRelay`: the latter still treats `auto` as unsafe until an explicit,
4893
released minimum is set, and treats a prerelease as unsafe even when its number

devlog/_plan/260731_macos_rss_retention/040_allocator_residual.md

Lines changed: 44 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ The upstream statuses supplied to this unit are unchanged:
2222
mimalloc per-thread heaps; without scavenging/purge, that is allocator retention,
2323
not proof of a live JS object leak. It also explicitly made its RSS assertion
2424
Linux-only because macOS reclaim is lazy.
25-
- [Bun PR #28743](https://github.com/oven-sh/bun/pull/28743) remains **open**. It
25+
- [Bun PR #28743](https://github.com/oven-sh/bun/pull/28743) is **closed unmerged**. It
2626
is the closest ownership path: fetch response bytes allocated on an HTTP thread
2727
can be freed after GC on a different thread and remain in mimalloc delayed/free
2828
caches. It says macOS RSS is the wrong immediate pass/fail signal because
@@ -61,6 +61,34 @@ allocator page.
6161

6262
### Reproducible matrix
6363

64+
#### Harness contract for cells D/E/F (closes audit blocker 4)
65+
66+
Phase 1 implements only the proxy topology (cell B). Cells D/E/F must measure a
67+
process with NO proxy code in it, so "extend the harness" is not a sufficient
68+
instruction — mixing roles into one process would contaminate exactly the
69+
attribution this phase exists to produce.
70+
71+
Shape: ONE controller CLI, but every measured role runs as its own spawned child.
72+
"One binary" means one orchestrator, not one measured process.
73+
74+
| File | Role |
75+
|---|---|
76+
| `scripts/macos-rss-retention-harness.ts` | Controller. Parses `--cell A..F`, creates the run directory, spawns and stops role children, collates JSONL, invokes `ps` / `vmmap` / `footprint`. Orchestration only — never hosts a measured workload. |
77+
| `scripts/macos-rss-retention-harness-child.ts` | The proxy child from Phase 1, unchanged. Used by cells A-C and F. |
78+
| `scripts/macos-rss-retention-harness-worker.ts` | Role process selected by `--role`: `fixture-upstream`, `load-client`, `standalone-fetch-client` (cell D), `standalone-response-server` (cell E). |
79+
| `scripts/macos-rss-retention-harness-metrics.ts` | Shared sampler and record schema: `process.memoryUsage()`, `heapStats`, lifecycle markers, JSONL/IPC emission. Imported by every measured child so all cells emit directly comparable records. |
80+
81+
Invariants that make the matrix trustworthy:
82+
83+
- Each cell starts FRESH processes; no cell reuses a process from another cell,
84+
because high-water-mark behavior makes a reused process carry prior peaks.
85+
- `vmmap` and `footprint` capture is MANDATORY per cell, not optional — RSS alone
86+
cannot separate resident-but-reusable pages from live allocation.
87+
- The controller writes one machine-readable comparison artifact per run so cells
88+
are diffed by tooling rather than by eye.
89+
- Cells D and E must contain no import path that reaches `src/server/`. State this
90+
as a checked property, not an intention.
91+
6492
Extend the Phase-1 harness; do not infer this from the production watchdog's
6593
60-second cadence (`DEFAULT_INTERVAL_MS` at
6694
[`src/server/memory-watchdog.ts:53-56`](../../../src/server/memory-watchdog.ts#L53-L56)).
@@ -184,10 +212,21 @@ The existing watchdog samples one scalar every 60 seconds and warns when the
184212
largest of RSS/external/ArrayBuffers crosses a flat 4 GiB threshold
185213
([`src/server/memory-watchdog.ts:31-45`](../../../src/server/memory-watchdog.ts#L31-L45),
186214
[`src/server/memory-watchdog.ts:123-139`](../../../src/server/memory-watchdog.ts#L123-L139)).
187-
That makes a first, stable high-water plateau look continuously alarming and misses
188-
whether the process is still growing. Make **sustained RSS growth rate** the
189-
primary warning. Retain 4 GiB as a separately rate-limited safety ceiling, not as
190-
evidence of a leak.
215+
Audit blocker 5 corrected an earlier misreading here and the rationale is amended
216+
accordingly. The current warning is NOT continuously noisy: `WARN_INTERVAL_MS`
217+
rate-limits it to once per 30 minutes
218+
([`src/server/memory-watchdog.ts:134-139`](../../../src/server/memory-watchdog.ts#L134-L139)).
219+
The real defect is different and narrower — the signal is **level-only**, so it
220+
cannot distinguish a process sitting at a harmless high-water plateau from one
221+
that is still climbing. Given that E3 proved plateau behavior is expected on this
222+
runtime, level alone is the wrong verdict input. Make **sustained RSS growth rate**
223+
the primary warning and retain 4 GiB as a separately rate-limited safety ceiling
224+
that reports a ceiling, not a leak.
225+
226+
Tests must encode that corrected distinction: a stable ceiling still emits its
227+
rate-limited safety warning (unchanged behavior), while sustained growth emits the
228+
new growth reason. A test that merely asserts "no warning at plateau" would encode
229+
the misreading rather than the fix.
191230

192231
Proposed diff contract (constants are deliberately conservative until Phase-1
193232
series calibrates them):

0 commit comments

Comments
 (0)