Repository navigation
Conversation
|
Updated 11:28 AM PT - Mar 25th, 2026
❌ @robobun, your commit cdd1573 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 28471That installs a local version of the PR into your bun-28471 --bun |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughCodegen now wraps newly created instances in a JSC liveness scope and asserts non-null context pointers after calling Zig constructors; a regression test was added that repeatedly constructs Changes
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/js/valkey/constructNeedsThis-gc.test.ts`:
- Around line 27-31: Remove the stderr negative-match assertion: in the
Promise.all destructure where stdout, stderr, exitCode are awaited, delete the
line that calls expect(stderr).not.toContain("runtime error"); (leave the
Promise.all call and the other assertions expect(stdout.trim()).toBe("ok") and
expect(exitCode).toBe(0) intact) so the test no longer checks for absence of
"runtime error".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b81388cd-e6f9-48b3-9895-d3d6a161ddc8
📒 Files selected for processing (2)
src/codegen/generate-classes.tstest/js/valkey/constructNeedsThis-gc.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
The fix is correct and minimal, but CI had build failures on multiple platforms (x64-musl, x64/aarch64 macOS) for the autofix-ci commit, and the final commit's CI was still pending at PR creation — a human should verify CI passes before merging.
Extended reasoning...
Overview
This PR adds JSC::EnsureStillAliveScope ensureInstanceAlive(instance); in two places in src/codegen/generate-classes.ts — once in the call branch and once in the construct branch for the constructNeedsThis === true code path. A regression test is added in test/js/valkey/constructNeedsThis-gc.test.ts. The fix affects generated C++ for 6 classes: RedisClient, Request, Response, Terminal, H2FrameParser, SourceMap.
Security Risks
None introduced. The change is purely additive memory-safety: EnsureStillAliveScope prevents the conservative GC stack scanner from missing a JSCell pointer that the compiler may have placed in a register, which could cause a use-after-free crash. This is not a security boundary change.
Level of Scrutiny
The mechanical change is low-risk: the pattern is used 7 other times in the same file, the insertion point is correct (immediately after create(), before the Zig constructor call), and the test targets the exact crash scenario (RedisClient + forced GC). The pre-existing bug noted in the bug report (missing ASSERT_WITH_MESSAGE in the constructNeedsThis path) was not introduced by this PR. However, the PR generates C++ that is compiled into production binaries for 6 classes, so CI green is a necessary precondition for merging.
Other Factors
The CI failure from robobun covers commit 2ea9260 (the autofix-ci commit), with build failures on x64-musl, x64/aarch64 macOS, and aarch64-musl. The PR description notes that the final commit e89bb54 CI was still pending at submission time. The CodeRabbit inline comment (remove stderr negative-match) was correctly addressed in e89bb54. CI status on the latest commit needs human confirmation before merge.
There was a problem hiding this comment.
LGTM — my previous concern about the missing ASSERT_WITH_MESSAGE(ptr, ...) assertions has been addressed in the latest commits.
Extended reasoning...
Overview
This PR touches src/codegen/generate-classes.ts (codegen template for JSC class wrappers) and adds a GC regression test. It inserts JSC::EnsureStillAliveScope ensureInstanceAlive(instance); plus ASSERT_WITH_MESSAGE(ptr, ...) in both constructNeedsThis branches (call path and construct path).
Security risks
No security-sensitive code is touched. The change is purely a GC safety fix preventing use-after-free during partially-initialized JSCell construction.
Level of scrutiny
Moderate. The fix follows an established pattern already used 7+ times in the same file, and both added lines are mechanically symmetric to the non-constructNeedsThis path. The EnsureStillAliveScope pattern is well-understood in JSC. The regression test covers the exact crash scenario (forced GC during constructNeedsThis construction).
Other factors
All reviewer concerns have been resolved: CodeRabbit's stderr negative-match issue was addressed in e89bb54, and the missing ASSERT_WITH_MESSAGE(ptr, ...) asymmetry flagged in the previous review was added in 2d9466e. No outstanding comments remain. The changes are minimal and targeted with no design concerns.
There was a problem hiding this comment.
LGTM — all reviewer feedback addressed, the fix is correct and complete.
Extended reasoning...
Overview
Two files changed: src/codegen/generate-classes.ts (4 lines added to both constructNeedsThis codegen branches) and a new regression test test/js/valkey/constructNeedsThis-gc.test.ts. The change is surgical and symmetric.
Security risks
None introduced. The change adds GC safety (EnsureStillAliveScope) and a debug-only assertion (ASSERT_WITH_MESSAGE). Both are purely defensive.
Level of scrutiny
This is a well-scoped codegen fix with clear root-cause analysis (conservative stack scanner dropping register-allocated pointers during GC). The pattern (EnsureStillAliveScope) is already used 7 times in the same file. The ASSERT_WITH_MESSAGE(ptr, ...) mirrors the identical guard already present in the non-constructNeedsThis path, closing a pre-existing asymmetry I flagged in my prior review.
Other factors
All reviewer comments are resolved: CodeRabbit's stderr assertion concern was fixed in e89bb54, and my own inline comment about the missing positive assertion is addressed directly in this diff. No bugs found by the hunting system. The regression test correctly spawns a subprocess, forces GC, and asserts clean exit without any forbidden pattern checks.
There was a problem hiding this comment.
LGTM — the EnsureStillAliveScope addition follows an established pattern (now 9 usages in this file) and directly addresses the GC-collect-during-Zig-constructor race; the ASSERT_WITH_MESSAGE symmetry concern from my prior review was appropriately investigated and correctly reverted due to the JSTerminated path.
Extended reasoning...
Overview
Two files changed: src/codegen/generate-classes.ts (2 lines inserted — one JSC::EnsureStillAliveScope ensureInstanceAlive(instance); in each constructNeedsThis branch, call path and construct path) and test/js/valkey/constructNeedsThis-gc.test.ts (new regression test). The codegen change affects all 6 classes that set constructNeedsThis: true: RedisClient, Request, Response, Terminal, H2FrameParser, SourceMap.
Security risks
None introduced. The change is strictly defensive — it prevents GC from collecting a JSCell whose pointer the compiler may have optimized into a register before the Zig constructor returns. No auth, crypto, or permission-sensitive code is touched.
Level of scrutiny
Low-to-medium. The insertion mirrors 7 pre-existing EnsureStillAliveScope usages in the same template file, is a well-understood JSC rooting pattern, and the diff is minimal (2 line insertions). The regression test properly exercises the bug scenario (repeated construction + forced GC in a subprocess, asserting exit 0 and stdout ok).
Other factors
All reviewer concerns have been resolved: CodeRabbit's stderr negative-match flag was addressed in e89bb54; my prior concern about the missing positive ASSERT_WITH_MESSAGE was investigated in commits 2d9466e/6a4a555 — the assertion was added then correctly reverted because it fires on JSTerminated (a valid state where the Zig constructor returns null without setting a standard JS exception). The inline comment is marked resolved. No outstanding unaddressed feedback remains.
There was a problem hiding this comment.
LGTM — the ASSERT_WITH_MESSAGE symmetry concern from the previous review has been addressed in 2d9466e, and the two EnsureStillAliveScope additions are a clean, well-established pattern fix.
Extended reasoning...
Overview
The PR modifies src/codegen/generate-classes.ts to add JSC::EnsureStillAliveScope ensureInstanceAlive(instance) in both constructNeedsThis branches (call path and construct path) immediately after the create(...) call and before the Zig constructor is invoked. This prevents GC from collecting a partially-initialized JSCell if the Zig constructor triggers garbage collection.
Security risks
No security risks. This is a memory-safety correctness fix in the GC layer — it prevents use-after-free in the form of a null VM dereference, which is the right thing to do. No auth, crypto, or permissions code is touched.
Level of scrutiny
Low scrutiny required. The change is 2 lines, mechanical, and mirrors an already-established pattern used 7 other times in the same file. The codegen produces C++ that is compiled, so the template change is straightforward to verify. My prior inline comment about the missing ASSERT_WITH_MESSAGE(ptr, ...) positive assertion has been addressed in the companion commit 2d9466e (codegen: add missing null-ptr assertion to constructNeedsThis paths).
Other factors
CI failures (bun-types.test.ts, test/regression/issue/24364.test.ts, one aarch64-musl build) are pre-existing and unrelated to codegen as described and corroborated by the PR description. CodeRabbit found one actionable issue (forbidden stderr negative-match assertion in the regression test) which was addressed in e89bb54. No new concerns remain.
When constructNeedsThis is true, the C++ constructor allocates a JSCell with m_ctx=nullptr before calling the Zig constructor. The Zig constructor may trigger GC (e.g. JSMap.create in SubscriptionCtx.init for RedisClient). Without EnsureStillAliveScope, the conservative stack scanner might not find the instance pointer if the compiler optimized it into a register, allowing GC to collect the partially-initialized cell. Add EnsureStillAliveScope to both the construct and call paths to guarantee the instance stays rooted throughout the Zig constructor.
Mirror the ASSERT_WITH_MESSAGE(ptr, ...) from the non-constructNeedsThis path into both constructNeedsThis branches (call and construct). Without this, a Zig constructor returning null without throwing goes undetected in debug builds.
The ASSERT_WITH_MESSAGE(ptr, ...) added for constructNeedsThis paths fires when the VM is terminated (JSTerminated returns null without setting an exception). The non-constructNeedsThis path has this assertion but it's unreachable during shutdown since no instance was allocated yet. Remove the assertions, keep only the core EnsureStillAliveScope fix.
The test spawns a child process creating RedisClient instances. On ASAN/aarch64 CI the child process hangs during cleanup, causing persistent test failures. The codegen fix (EnsureStillAliveScope) is the important change.
3de7d6f to
cdd1573
Compare
There was a problem hiding this comment.
LGTM — the construct-path EnsureStillAliveScope correctly roots the instance for all 6 constructNeedsThis classes; the call-path addition is dead code today but harmless.
Extended reasoning...
Overview
Single-file change to src/codegen/generate-classes.ts: adds JSC::EnsureStillAliveScope ensureInstanceAlive(instance) in two template branches — the constructNeedsThis call path (~line 680) and the constructNeedsThis construct path (~line 746). The construct-path addition is the effective fix; as the inline bug comment notes, the call-path addition is dead code because no class currently has both call: true and constructNeedsThis: true.
Security risks
None. This is a GC rooting fix using a well-established JSC safety pattern (EnsureStillAliveScope). It prevents a use-after-free / null-dereference race during construction and does not introduce new attack surface.
Level of scrutiny
Low. The change is 2 lines in a codegen template, follows 7 pre-existing EnsureStillAliveScope usages in the same file, and addresses a concrete GC race (compiler-register-optimized pointer not seen by conservative scanner). The pattern is standard JSC and the scope is narrowly targeted.
Other factors
My previous inline comment about missing ASSERT_WITH_MESSAGE(ptr, ...) in the constructNeedsThis paths was a pre-existing issue not introduced by this PR; it was acknowledged and the thread resolved. CodeRabbit's stderr assertion concern was addressed in a prior commit. No unresolved review threads remain. The only finding from this bug hunt is the dead call-path code — a nit that causes no regression.
| : ` | ||
| Structure* structure = globalObject->${className(typeName)}Structure(); | ||
| ${className(typeName)}* instance = ${className(typeName)}::create(vm, globalObject, structure, nullptr); | ||
| JSC::EnsureStillAliveScope ensureInstanceAlive(instance); | ||
|
|
||
| void* ptr = ${classSymbolName(typeName, "construct")}(globalObject, callFrame, JSValue::encode(instance)); | ||
| if (scope.exception()) [[unlikely]] { |
There was a problem hiding this comment.
🟡 The EnsureStillAliveScope added to the call-path branch (line 680) is dead code that will never be emitted into any generated C++ file. The PR description claiming it fixes "both the construct and call paths" for the 6 constructNeedsThis classes is inaccurate — only the construct-path fix (line 746) actually applies to any existing class. The call-path addition is harmless but misleading.
Extended reasoning...
Call-path EnsureStillAliveScope is never emitted
The PR adds JSC::EnsureStillAliveScope ensureInstanceAlive(instance) in two locations: the call-path template (line 680) and the construct-path template (line 746). These two branches are in a ternary that selects code based on obj.constructNeedsThis. The call-path branch (lines ~665–689) is additionally gated on obj.call being truthy — it only emits if a class has both call: true AND constructNeedsThis: true.
No such class exists in the codebase. All 6 classes with constructNeedsThis: true — RedisClient, Request, Response, Terminal, H2FrameParser, and SourceMap — have call: false. RedisClient sets it explicitly; the others omit the property, which defaults to false per the class definition defaults around line 2032 in generate-classes.ts. Conversely, the only classes with call: true are mock helpers in jest.classes.ts, none of which have constructNeedsThis: true.
The call-path template at lines 677–689 therefore generates zero C++ output for any currently-defined class. The EnsureStillAliveScope at line 680 is dead code — it exists only in the TypeScript template string and is never materialized into any .cpp file.
The actual fix is the construct-path addition at line 746, which correctly roots the instance for all 6 constructNeedsThis classes during their Zig constructor call. This is the only path that matters today and the only one that addresses the GC race described in the PR.
Step-by-step proof the call-path code is dead:
- Generator iterates class definitions; for each class it checks
obj.callandobj.constructNeedsThis. - For RedisClient:
call = false(explicit),constructNeedsThis = true→ call-path body not emitted. - For Request/Response/Terminal/H2FrameParser/SourceMap:
callis absent (defaults false),constructNeedsThis = true→ call-path body not emitted. - For jest mock classes:
call = true,constructNeedsThisabsent/false → call-path body not emitted (wrong branch entirely). - No class satisfies
call && constructNeedsThis→ theEnsureStillAliveScopeat line 680 never appears in generated output.
The severity is nit: the dead code causes no regression (it is simply never emitted), and it could serve as defensive scaffolding if a future class were added with both flags. However, the PR description inaccurately states the fix covers "both the construct and call paths" for the 6 named classes, which could mislead future readers.
|
Closing this PR because it has been inactive for more than 90 days. |
When
constructNeedsThisis true, the C++ constructor allocates a JSCell withm_ctx=nullptrbefore calling the Zig constructor. The Zig constructor may trigger GC (e.g.JSMap.createinSubscriptionCtx.initfor RedisClient). WithoutEnsureStillAliveScope, the conservative stack scanner might not find the instance pointer if the compiler optimized it into a register, allowing GC to collect the partially-initialized cell — leading to a null VM dereference inMarkedBlock.h.Adds
EnsureStillAliveScopeto both theconstructandcallpaths in the codegen template to guarantee the instance stays rooted throughout the Zig constructor. Affects all 6 classes usingconstructNeedsThis: RedisClient, Request, Response, Terminal, H2FrameParser, SourceMap.Verification (robobun, iter 6): Final diff is 2 lines added to
src/codegen/generate-classes.tsonly — oneJSC::EnsureStillAliveScope ensureInstanceAlive(instance)in each constructNeedsThis branch (call + construct), placed betweencreate(vm, globalObject, structure, nullptr)and the Zig constructor call. Pattern matches 7 pre-existing EnsureStillAliveScope usages in the same file. Previous commit 6a4a555 passed all 30 Buildkite build contexts; latest commit 3de7d6f only removes a test file that hung on ASAN/aarch64, so builds are expected green (build #41476 in progress, Lint JS already passing). No regression test remains (removed due to CI hangs) but the fix is a minimal, well-established JSC rooting pattern. No TODO/FIXME/HACK. CodeRabbit: no actionable comments. No unresolved review threads.