fix(gc): admit array-growth forwarding stubs to the budgeted-cycle classifier (#9717) - #9732
fix(gc): admit array-growth forwarding stubs to the budgeted-cycle classifier (#9717)#9732proggeramlug wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 8 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughThe GC now recognizes plausible array-growth forwarding stubs during budgeted full tracing. It records recoveries in a thread-local counter, reports the count in diagnostics, and adds regression tests. Generated Bun API documentation also adds ChangesForwarded Stub Membership
Bun API Documentation
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This change preserves arrays reached through valid forwarding stubs during budgeted collection, preventing late route matching from losing routes. Regression coverage and reported runtime checks confirm the intended behavior, with no remaining merge-blocking risk. Sequence Diagram(s)sequenceDiagram
participant BudgetedFullTrace
participant classifier_valid_object_start
participant plausible_forwarded_arena_stub
participant trace_one_worklist_header
participant emit_incremental_liveness_diag
BudgetedFullTrace->>classifier_valid_object_start: classify candidate pointer
classifier_valid_object_start->>plausible_forwarded_arena_stub: validate forwarded arena header
plausible_forwarded_arena_stub-->>classifier_valid_object_start: accept plausible stub
BudgetedFullTrace->>trace_one_worklist_header: validate forwarding target
trace_one_worklist_header-->>BudgetedFullTrace: mark reachable object
emit_incremental_liveness_diag->>classifier_valid_object_start: read recovery counter
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
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 |
…assifier (PerryTS#9717) A `#private` array pushed past its inline capacity leaves a permanent forwarding stub at the pre-grow address, and the reference pointing at it is never rewritten (PerryTS#6228/PerryTS#233), so a live slot can keep naming the stub. A synchronous full trace handles this: its exact census (`record_arena_header`) admits every arena object, stubs included, so `mark_field_into_worklist` marks the stub and `trace_one_worklist_header` follows it to the live array. A budgeted full trace — what the idle-time reducer (`PERRY_GC_IDLE_RECLAIM`) runs when a server goes quiet — resolves membership through the page-metadata classifier instead. `classifier_valid_object_start` rejected every FORWARDED header (a dead metadata key's recycled bytes can set the bit, PerryTS#8040), so the field->stub edge was dropped: the stub was never marked, the FORWARDED-follow never ran, and the array reachable only through it was swept. The private field then resolved to reused memory as an empty array, so every hono route `match()` returned 404 for the life of the process — but only when the first request arrived ~10-20s after startup while background work allocated. The classifier is documented as a census superset; for growth stubs it was not. It now admits a plausible forwarded arena stub (`GC_FLAG_ARENA` set, valid obj_type/size), the shape a real growth stub has. The forwarding target is still validated in the follow, so a garbage target stops the walk. A `PERRY_GC_DIAG` counter (`forwarded_stub_recoveries=`) reports recoveries. Regression: gc::tests::forwarded_stub_membership plants the edge, asserts the pre-fix gate would have rejected the stub, drives a budgeted full cycle, and checks the stub-reached array survives; a synchronous control keeps it without recovery. The budgeted test fails without the fix and passes with it. Claude-Session: https://claude.ai/code/session_01GkugRUwRCCjfYYNfzyyvQv
b51c51b to
e06bb82
Compare
Black-box confirmation on the reporter's
|
| runtime | /v1/health socket @ t=16 s |
in-process probe @ t=20 s |
|---|---|---|
pre-fix (api-gate4) |
404 (5/5) | 404, router permanently empty (5/5) |
| this branch | 200 (5/5) | 200, routes=103 (5/5) |
The permanent 404 is gone and the private-field array survives the idle-time collection with all 103 routes intact.
For reference, on the pre-fix binary PERRY_GC_IDLE_RECLAIM=0 alone also flips it to 200 (2/2), which is what localized the trigger to the budgeted full cycle this PR fixes.
Validation: cargo test -p perry-runtime gc:: → 1015 passed, 0 failed; the new gc::tests::forwarded_stub_membership budgeted case fails without the fix (verified by disabling the new branch) and passes with it. The full local lint-gate runner reports one failure — warnings: cargo check --workspace --all-targets — which is the known Linux-only pthread_* redeclaration on origin/main (CI runs on macOS, where those #[cfg(target_os = "linux")] blocks compile out); perry-runtime --lib is clean under -D warnings with only that clash allowed. The red self-test-checkers context is a pre-existing check_thread_locals.py failure on files byte-identical to origin/main, unrelated to this change.
Pre-existing drift on main: the runtime exports `bun.connect` and `bun.listen` (manifest 2089->2091 entries) but `docs/api/perry.d.ts` and `docs/src/api/reference.md` were not regenerated, so the `check` job's API-docs drift gate is red for every PR branched from main. `scripts/regen_api_docs.sh` produces exactly this diff (deterministic; unrelated to the PerryTS#9717 GC fix in this PR). Committing the generated artifacts as the gate instructs. Claude-Session: https://claude.ai/code/session_01GkugRUwRCCjfYYNfzyyvQv
|
Landed on |
CI note:
|
Second pre-existing
|
| job | result | cause |
|---|---|---|
check (API-docs drift) |
pass | fixed by this PR's docs commit |
lint, gap-suite, e2e-scoped, gc-stress-build |
pass | — |
cargo-test |
fail | #9514 — PERRY_CONCAT_SITE_CACHE unregistered (perry-runtime itself, incl. this PR's tests, is green) |
warnings |
fail | #9521 — clashing pthread_getattr_np extern decls (Ubuntu -D warnings) |
Both need a one-line fix each, separate from this GC change.
1 participant
Fixes #9717.
Symptom
In a compiled hono server, every route returns 404 forever if the first request
arrives ~10–20 s after startup while background work (a
setIntervalscheduler)is allocating. An early first request and the process is healthy for its whole
life.
app.routesstill lists all 103 routes; only matching fails. The privatefield
SmartRouter.#routes"is still an array, but empty."Root cause
hono/router/smart-routerkeeps not-yet-installed routes in a#privatearrayand replays them into the concrete router on the first
match(). That array ispushed past its inline capacity, so array growth leaves a permanent forwarding
stub at the pre-grow address, and the reference is never rewritten (#6228 /
#233) — the live
#routesfield keeps pointing directly at the stub.A synchronous full trace handles this: its exact census
(
ValidPointerSetBuilder::record_arena_header) admits every arena object, stubsincluded, so
mark_field_into_worklistmarks the stub andtrace_one_worklist_headerfollows it to the live array.A budgeted full trace — the one the idle-time reducer (
PERRY_GC_IDLE_RECLAIM)runs when the server goes quiet between requests — resolves membership through
the page-metadata classifier instead.
classifier_valid_object_startrejectedevery
GC_FLAG_FORWARDEDheader by design (a dead metadata key's recycled bytescan set that bit, #8040). So the field→stub edge was silently dropped: the stub
was never marked, the FORWARDED-follow never ran, and the array reachable only
through the stub was swept. The field then resolved to reused memory — an empty
array — and every
match()returned 404. It reproduces only on a late firstrequest because an early one builds the router before any idle collection runs.
PERRY_GC_IDLE_RECLAIM=0makes the bug vanish on the reporter's own binary(0/2 vs 2/2 by request timing), which localizes it to the budgeted full cycle.
Fix
The classifier is documented as a census superset; for growth stubs it was
not.
classifier_valid_object_startnow admits a plausible forwarded arena stub(
GC_FLAG_ARENAset, validobj_type/size — the shape a real growth stub has,which separates it from off-heap bytes that coincidentally set the bit). The
forwarding target is still validated where it always was, in
trace_one_worklist_header's follow, so a garbage target simply stops the walk.A
PERRY_GC_DIAGcounter (forwarded_stub_recoveries=on the[gc-incremental]line) reports how many such stubs a budgeted cycle recovered.
Testing
gc::tests::forwarded_stub_membership(two cases): plantsthe field→stub→array edge, asserts the pre-fix census-superset gate would have
rejected the stub, drives a budgeted full cycle to completion, and checks
the array reached only through the stub survives with its contents intact; a
synchronous control keeps it without needing the recovery path. The
budgeted case fails without the fix (verified by disabling the new branch)
and passes with it.
cargo test -p perry-runtime gc::— 1015 passed, 0 failed.apps/apibinary returns 404 on a t=16 sfirst request before this change and 200 after (see PR thread).
https://claude.ai/code/session_01GkugRUwRCCjfYYNfzyyvQv
Summary by CodeRabbit
Bug Fixes
Diagnostics
Documentation
connectandlistenmethods in thebunAPI reference and type declarations.Tests