feat(memory): expose JSC extraMemorySize in system memory API, watchdog, and doctor - #2302
feat(memory): expose JSC extraMemorySize in system memory API, watchdog, and doctor#2302lidge-jun wants to merge 1 commit into
Conversation
…og samples, and doctor Bun 1.4 external-memory reporting fixes move extraMemorySize; surface it so the 6h watchdog ring and /api/system/memory can attribute native-memory changes. Static bun:jsc import keeps the sampler synchronous; the call is guarded so introspection failure never breaks sampling. Thresholding (observedMemoryCounter) is unchanged — observability only. Unit: devlog/_plan/260822_260822-bun14-followup-memory/010
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ Deterministic PR hygiene checks passed. |
리뷰 · 우선순위 48 / 80지금 핵심. 구멍. 정적 import 실패는 try/catch가 못 잡음. 로드 타임. Bun에선 살아 있음. node 툴링이 이 모듈을 가져오면 프로세스 기동이 죽음. 기존
해결방안: #2301 머지 다음에 이 패치로 가라. 쓰레스홀드에 extraMemorySize 넣지 말 것. 정적 import는 Bun 전제면 유지. 주석에 node에서 이 모듈 import 금지를 남겨라. API 0 vs 워치독 omit을 맞춰라. 없으면 필드 생략이 기존 이 댓글은 grok-bot이 작성했습니다 |
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
Reviewed exact stacked head cac21afb726a25c3e35ef072e5c907e429df443e. Exposing the JSC native-memory counter is useful, but I am requesting changes before this becomes an integration candidate:
-
This PR targets
codex/bun14-followup-memory-docs, so normaldevCross-platform CI does not run. Land/fix #2301 first, then retarget or rebase this runtime PR onto currentdevand obtain exact-head code CI. Do not merge this into the docs parent first, because that would make the runtime diff enterdevthrough the docs PR without the normal code gate. -
src/server/management/system-routes.tsconverts a missing/non-numericheapStats().extraMemorySizeinto0, while the watchdog and doctor types correctly treat this field as optional. “Unavailable” is not the same measurement as zero. Keep one contract across all three surfaces: includeextraMemorySizeonly when the runtime returned a finite numeric value, otherwise omit it. Add a fail-closed regression for the unavailable/malformed counter rather than pinning a fabricated zero. -
The doctor parsing/formatting change has no focused regression in
tests/doctor.test.ts. Please assert both that a numericjscHeap.extraMemorySizeis retained/rendered asjscExtra, and that an absent or malformed field is omitted without changing the existing memory heuristic. -
Please adjust the wording that calls this “Bun 1.4 extraMemorySize visibility”. The current local Bun 1.3.14
heapStats()already exposes a numericextraMemorySize; the 1.4-specific claim should be about the verified reporting/allocator behavior change, not the existence of the property itself.
After those points and exact-head dev CI, the sampling addition looks small and appropriately scalar-only.
…ockers closed Rebuilds the harness halves of #2303 and #2304 directly on dev, without the #2302 runtime commit those PRs were stacked on. Merging them as stacked would have dragged in the extraMemorySize: 0 fabrication that #2302 still carries, and would also have reverted unrelated coordinator work that landed on src/cli/doctor.ts after the stack was cut. scripts/bun-gc-relief-eval.ts Records rssBeforeLoad and derives postLoadGrowth and recoveryFraction. The controlling 260731 gate is "at least 50% of post-load RSS GROWTH is gone", and the previous shape could not express that: rssAfterLoad - rssPlus60s cannot separate recovery from ordinary drift, and the recorded verdict divided recovered bytes by total post-load RSS, which answers a different question than the gate asks. recoveryFraction is null when growth was not measurable, so a cell that proves nothing does not read as 0% recovery. A child-side gc-error now rejects the waiting cell instead of expiring into a ten-second "gc receipt timeout" that hides the real cause. scripts/macos-rss-retention-harness-child.ts The SIGUSR2 collector is installed only under OCX_GC_EVAL=1. It was gated by a comment saying the 7h retention protocol never sends that signal, which is a claim about one sender rather than a property of the process; a stray signal would have collected inside the measurement that protocol exists to take. scripts/smol-worker-ab.ts payloadMb and runs are validated as bounded integers. Previously runs=0 produced a report claiming completionSuccess over an empty result set with the median fields silently absent, and a negative payload ran a meaningless workload instead of refusing. Medians are computed only once both arms are complete, so a verdict can never be derived from a partial set. The header claimed to measure the audited shapes of history, restore and policy workers; it imports none of them. It now says what it is: a synthetic screening of the array-plus-JSON burst shape those workers share. The FAIL verdicts both harnesses recorded stand. No production Bun.gc(true) call and no smol: true flag is landed here. The GC harness needs a live upstream fixture to produce new numbers, so the recorded RSS cells are NOT regenerated by this commit and the 020 table still carries the old denominator. Re-running the cells and rewriting that table around recoveryFraction is deliberately left as the next measurement pass rather than claimed here.
|
|
Records why the four-PR stack was rebuilt on dev rather than merged: only lidge-jun#2301 targeted dev, so dev CI never ran on the runtime diff, and a stacked merge would have reverted coordinator work that landed on src/cli/doctor.ts after the stack was cut (-94/+6 against current dev). lidge-jun#2302 was closed rather than landed. It coerces a missing or non-numeric extraMemorySize into 0 while the watchdog and doctor both type the field optional, so a counter that was never read would surface as jscExtra=0MB inside a series whose only purpose is showing whether native memory grows. Also records the wp1 holdout lidge-jun#2359 landing after the author fixed the exclusion that broke provider-live-models.test.ts:163, and the close of issue lidge-jun#2330 with the reasoning for the two slugs deliberately left exposed.
2 participants
Summary
Verification
Checklist