fix(js): abandon runtimes that outlive interrupt grace period - #7378
Conversation
WalkthroughAbandons goja runtimes that fail to terminate after an interrupt, delegates orphan cleanup to a reaper, ensures pool slots aren't released until orphans exit, and adds tests verifying abandonment, slot release ordering, and cleanup after callback panic. ChangesRuntime Pool Race Condition Fix
sequenceDiagram
participant Caller
participant executeWithPoolingProgram
participant executeWithRuntime
participant Context as ctx.Done
participant GojaRuntime as goja.Runtime
participant Pool as sync.Pool
participant Reaper
Caller->>executeWithPoolingProgram: ExecuteProgram(program, opts)
executeWithPoolingProgram->>Pool: Get runtime
executeWithPoolingProgram->>executeWithRuntime: run program with ctx
executeWithRuntime->>Context: ctx.Done() (deadline)
executeWithRuntime->>GojaRuntime: Interrupt()
executeWithRuntime->>GojaRuntime: wait for run result (up to 1s)
alt run returns within grace
executeWithRuntime-->>executeWithPoolingProgram: normal result
executeWithPoolingProgram->>Pool: Put runtime back
else run does not return within 1s
executeWithRuntime-->>executeWithPoolingProgram: errRuntimeTerminationTimeout
executeWithPoolingProgram->>executeWithPoolingProgram: mark runtimeAbandoned
executeWithPoolingProgram->>Reaper: spawn onOrphanExit to wait for orphan and release slot
end
executeWithPoolingProgram-->>Caller: final result or error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Neo - PR Security ReviewNo security issues found Hardening Notes
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/js/compiler/pool.go`:
- Around line 204-208: The defer currently calls pooljsc.Done() even when
runtimeAbandoned is true, freeing the concurrency slot while the RunProgram
goroutine is still alive (see runtimeAbandoned, errRuntimeTerminationTimeout,
RunProgram, pooljsc.Done(), gojapool.Put); change the logic so that when
runtimeAbandoned==true you do NOT call pooljsc.Done() in this defer but instead
hand the abandoned runtime to a reaper: spawn a goroutine (or enqueue to a
reaper worker) that waits for the orphaned RunProgram to fully exit (or for the
runtime to be Put back via gojapool.Put) and only then calls pooljsc.Done();
apply the same conditional fix to the other defer block referenced (around the
261-264 region).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7f4d1dd9-31fc-4e50-8649-32a1c777f8fe
📒 Files selected for processing (2)
pkg/js/compiler/compiler_test.gopkg/js/compiler/pool.go
| if err := opts.Callback(runtime); err != nil { | ||
| // Inner goroutine has not been spawned yet — safe to clean up | ||
| // synchronously and return the runtime to the pool. | ||
| cleanupRuntime(runtime, args, opts) |
There was a problem hiding this comment.
I'm leaning towards deferring this cleanup to preserve UB (like panic) while cleaning runtime state before gojapool reuse. Wdyt?
There was a problem hiding this comment.
$ go test -v -run "TestExecuteWithRuntimeCleansUpAfterCallbackPanic" ./pkg/js/compiler/
=== RUN TestExecuteWithRuntimeCleansUpAfterCallbackPanic
compiler_test.go:140:
Error Trace: /home/dw1/Development/PD/nuclei/pkg/js/compiler/compiler_test.go:140
Error: Should be true
Test: TestExecuteWithRuntimeCleansUpAfterCallbackPanic
--- FAIL: TestExecuteWithRuntimeCleansUpAfterCallbackPanic (0.00s)
FAIL
FAIL github.com/projectdiscovery/nuclei/v3/pkg/js/compiler 0.107s
FAILPatch: TestExecuteWithRuntimeCleansUpAfterCallbackPanic.patch
When a runtime is abandoned, we must not touch the goja runtime after `RunProgram` has exceeded the interrupt grace period. At the same time, cleanup still needs to run on every normal exit before returning the runtime to the pool, including setup callback panics. Signed-off-by: Dwi Siswanto <git@dw1.io>
Proposed changes
Prevent pooled and non-pooled goja runtimes from being reused or cleaned up when
the RunProgram goroutine fails to exit after context cancellation and Interrupt().
In that state, the goroutine may still be mutating runtime state, so touching the
runtime or returning it to the pool can cause fatal concurrent map access panics.
Add an explicit errRuntimeTerminationTimeout path that abandons the runtime,
keeps the related concurrency slot reserved, and releases that slot only from a
reaper after the orphaned goroutine exits. Preserve errors.Is compatibility with
the original context cancellation cause.
Keep normal cleanup behavior unchanged, including callback panic cleanup, and add
regression tests for stuck-interrupt handling that verify runtime abandonment and
delayed slot release.
Fixes #7376
Checklist
Summary by CodeRabbit