fix(js): interrupt goja runtime on context cancel#7343
Conversation
mikhail5555
changed the title
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughJS execution removed the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant Compiler as Compiler
participant Pool as Pool
participant Runtime as Runtime
Caller->>Compiler: ExecuteWithOptions(ctx, program, args, opts)
Compiler->>Pool: ExecuteProgram(ctx, program, args, opts)
Pool->>Pool: Acquire slot using ctx (AddWithContext / sgResizeCheck)
Pool->>Runtime: Get or create runtime
Pool->>Runtime: run program in goroutine
Runtime->>Pool: send result or panic-as-error
alt ctx cancelled before completion
Pool->>Runtime: Interrupt(ctx.Err())
Runtime-->>Caller: return error (deadline/cancel)
Pool->>Pool: Return runtime to pool / Done()
else normal completion
Runtime-->>Caller: return value
Pool->>Pool: Return runtime to pool / Done()
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/js/compiler/compiler.go (1)
102-121:⚠️ Potential issue | 🔴 CriticalNormalize optional inputs before calling
context.WithTimeoutCause.Line 103 initializes
optsto an empty&ExecuteOptions{}when nil, but line 120 then dereferencesopts.TimeoutVariantswithout checking if it's nil. SinceTimeoutVariantsis a pointer field, this will panic. Additionally, a nilctxpassed to line 120 will also panic. Ensure both are properly initialized or rejected.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/js/compiler/compiler.go` around lines 102 - 121, In ExecuteWithOptions, calling context.WithTimeoutCause using opts.TimeoutVariants may panic because opts.TimeoutVariants can be nil and ctx can be nil; ensure inputs are normalized before that call by: if ctx == nil set ctx = context.Background(); if opts == nil set opts = &ExecuteOptions{} (already done) and if opts.TimeoutVariants == nil set opts.TimeoutVariants = &TimeoutVariants{JsCompilerExecutionTimeout: defaultTimeout} (or return an error if missing); then call context.WithTimeoutCause(ctx, opts.TimeoutVariants.JsCompilerExecutionTimeout, ErrJSExecDeadline). Reference ExecuteWithOptions, ExecuteOptions.TimeoutVariants, context.WithTimeoutCause and ErrJSExecDeadline when applying the fix.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/js/compiler/pool.go`:
- Around line 157-169: The watchdog goroutine currently calls
gojapool.Put(runtime) (in the ctx.Done and done cases), which can return the
same *goja.Runtime to the pool before the outer cleanup defers run—introduce a
watchdogDone channel, have the watchdog defer close(watchdogDone) and remove all
gojapool.Put(runtime) calls from inside the watchdog; then in the main cleanup
path wait for watchdogDone to be closed before calling gojapool.Put(runtime) so
runtime is only returned after all cleanup defers (and slotReleased/pooljsc
handling) have completed, ensuring runtime, runtime.ClearInterrupt(),
runtime.Interrupt(ctx.Err()), slotReleased.CompareAndSwap, and pooljsc.Done are
synchronized.
---
Outside diff comments:
In `@pkg/js/compiler/compiler.go`:
- Around line 102-121: In ExecuteWithOptions, calling context.WithTimeoutCause
using opts.TimeoutVariants may panic because opts.TimeoutVariants can be nil and
ctx can be nil; ensure inputs are normalized before that call by: if ctx == nil
set ctx = context.Background(); if opts == nil set opts = &ExecuteOptions{}
(already done) and if opts.TimeoutVariants == nil set opts.TimeoutVariants =
&TimeoutVariants{JsCompilerExecutionTimeout: defaultTimeout} (or return an error
if missing); then call context.WithTimeoutCause(ctx,
opts.TimeoutVariants.JsCompilerExecutionTimeout, ErrJSExecDeadline). Reference
ExecuteWithOptions, ExecuteOptions.TimeoutVariants, context.WithTimeoutCause and
ErrJSExecDeadline when applying the fix.
🪄 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: f9877b83-5790-49d6-903a-1a37d2e8159f
📒 Files selected for processing (7)
pkg/js/compiler/compiler.gopkg/js/compiler/compiler_test.gopkg/js/compiler/non-pool.gopkg/js/compiler/pool.gopkg/js/compiler/pool_test.gopkg/protocols/code/code.gopkg/protocols/javascript/js.go
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pkg/js/compiler/pool.go (1)
162-177:⚠️ Potential issue | 🔴 CriticalDon't return the runtime to
gojapoolfrom the watchdog.On cancellation, this makes the same
*goja.Runtimereusable beforeexecuteWithRuntimeand the local cleanup defers have finished. The next checkout can clear the interrupt or overwrite globals while the canceled execution is still unwinding, which reintroduces a real concurrent-reuse/state-leak race.🛠️ Suggested fix
var slotReleased atomic.Bool done := make(chan struct{}) + watchdogDone := make(chan struct{}) go func() { + defer close(watchdogDone) select { case <-ctx.Done(): runtime.Interrupt(ctx.Err()) - gojapool.Put(runtime) if slotReleased.CompareAndSwap(false, true) { pooljsc.Done() } case <-done: - gojapool.Put(runtime) if slotReleased.CompareAndSwap(false, true) { pooljsc.Done() } } }() - defer close(done) + defer func() { + close(done) + <-watchdogDone + gojapool.Put(runtime) + }()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/js/compiler/pool.go` around lines 162 - 177, The watchdog goroutine must not return the *goja.Runtime to gojapool while the canceled execution may still be unwinding; remove the gojapool.Put(runtime) calls from the goroutine that handles <-ctx.Done() and <-done, so the watchdog only calls runtime.Interrupt(ctx.Err()) and handles slotReleased/pooljsc.Done signaling; rely on the existing deferred cleanup in executeWithRuntime (the defer that returns the runtime) to Put(runtime) once local defers have completed. Ensure the goroutine still closes done or signals as needed but does not call gojapool.Put(runtime).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pkg/js/compiler/pool.go`:
- Around line 162-177: The watchdog goroutine must not return the *goja.Runtime
to gojapool while the canceled execution may still be unwinding; remove the
gojapool.Put(runtime) calls from the goroutine that handles <-ctx.Done() and
<-done, so the watchdog only calls runtime.Interrupt(ctx.Err()) and handles
slotReleased/pooljsc.Done signaling; rely on the existing deferred cleanup in
executeWithRuntime (the defer that returns the runtime) to Put(runtime) once
local defers have completed. Ensure the goroutine still closes done or signals
as needed but does not call gojapool.Put(runtime).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f68bc212-ef42-4e43-b26f-13547832a769
📒 Files selected for processing (2)
pkg/js/compiler/non-pool.gopkg/js/compiler/pool.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/js/compiler/non-pool.go
…tees the same promises
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/js/compiler/pool.go (1)
97-106:⚠️ Potential issue | 🔴 CriticalMove the panic guard into the worker goroutine.
The recover block at lines 98-106 is registered in the parent goroutine and cannot catch panics from
runtime.RunProgramat line 128, which executes in a child goroutine. Go's panic/recover mechanism is goroutine-scoped, so a panic in the child goroutine will bypass this guard entirely and can crash the process.Proposed fix
resultChan := make(chan gojaRunResult) go func() { + defer func() { + if ci.IsCI() { + return + } + if r := recover(); r != nil { + resultChan <- gojaRunResult{nil, fmt.Errorf("panic: %v", r)} + } + }() result, err := runtime.RunProgram(p) resultChan <- gojaRunResult{result, err} }()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/js/compiler/pool.go` around lines 97 - 106, The current defer recover is registered in the parent goroutine and won't catch panics from runtime.RunProgram which runs inside the worker goroutine; move the panic guard into the worker goroutine closure that calls runtime.RunProgram so it runs in the same goroutine, keep the ci.IsCI check, and ensure any recovered panic is converted into the existing err reporting path (e.g., set err or send the error on the worker error channel) so the panic is propagated/handled rather than crashing the process.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/js/compiler/pool.go`:
- Around line 132-138: Context deadline cancellation currently only calls
runtime.Interrupt() and then waits on resultChan, which can leave a pool slot
held if a blocking native callback runs outside the VM; restore a slot-only
watchdog so the slot is released immediately on ctx.Done without waiting for
RunProgram to return. Reintroduce a lightweight watchdog goroutine that watches
ctx.Done() and calls the pool slot-release method (the same release/unblock
logic used when RunProgram completes) as soon as the deadline fires, keep
runtime.Interrupt() and the existing resultChan handling for eventual program
termination, and add/update TestWatchdogPreventsStarvation to assert that slots
are freed at deadline even when native callbacks remain blocking. Ensure you
reference the select block using ctx.Done(), runtime.Interrupt, resultChan, and
RunProgram when locating where to add the watchdog and where to assert behavior
in tests.
---
Outside diff comments:
In `@pkg/js/compiler/pool.go`:
- Around line 97-106: The current defer recover is registered in the parent
goroutine and won't catch panics from runtime.RunProgram which runs inside the
worker goroutine; move the panic guard into the worker goroutine closure that
calls runtime.RunProgram so it runs in the same goroutine, keep the ci.IsCI
check, and ensure any recovered panic is converted into the existing err
reporting path (e.g., set err or send the error on the worker error channel) so
the panic is propagated/handled rather than crashing the process.
🪄 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: c64e3c70-a855-43b1-a17a-9fb3f0530810
📒 Files selected for processing (2)
pkg/js/compiler/non-pool.gopkg/js/compiler/pool.go
| select { | ||
| case <-ctx.Done(): | ||
| runtime.Interrupt(ctx.Err()) | ||
| r := <-resultChan | ||
| return r.result, r.err | ||
| case r := <-resultChan: | ||
| return r.result, r.err |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f "\.go$" pkg/js/compiler/Repository: projectdiscovery/nuclei
Length of output: 272
🏁 Script executed:
wc -l pkg/js/compiler/pool.go pkg/js/compiler/non-pool.go pkg/js/compiler/pool_starvation_test.goRepository: projectdiscovery/nuclei
Length of output: 188
🏁 Script executed:
sed -n '125,180p' pkg/js/compiler/pool.goRepository: projectdiscovery/nuclei
Length of output: 1884
🏁 Script executed:
cat -n pkg/js/compiler/non-pool.goRepository: projectdiscovery/nuclei
Length of output: 1050
🏁 Script executed:
sed -n '60,130p' pkg/js/compiler/pool_starvation_test.goRepository: projectdiscovery/nuclei
Length of output: 2201
🏁 Script executed:
sed -n '1,306p' pkg/js/compiler/pool.go | grep -i "watchdog" -A 5 -B 5Repository: projectdiscovery/nuclei
Length of output: 49
🏁 Script executed:
sed -n '140,175p' pkg/js/compiler/pool.goRepository: projectdiscovery/nuclei
Length of output: 1429
🏁 Script executed:
git log --oneline -20 pkg/js/compiler/pool.goRepository: projectdiscovery/nuclei
Length of output: 167
🏁 Script executed:
git show HEAD:pkg/js/compiler/pool.go | sed -n '125,180p'Repository: projectdiscovery/nuclei
Length of output: 1884
🏁 Script executed:
git show eabf0ecf^:pkg/js/compiler/pool.go | sed -n '125,180p'Repository: projectdiscovery/nuclei
Length of output: 106
🏁 Script executed:
git show eabf0ecf | head -100Repository: projectdiscovery/nuclei
Length of output: 4277
🏁 Script executed:
git show eabf0ecf -- pkg/js/compiler/pool.go | head -300Repository: projectdiscovery/nuclei
Length of output: 10371
🏁 Script executed:
git show eabf0ecf -- pkg/js/compiler/pool.go | tail -200Repository: projectdiscovery/nuclei
Length of output: 6571
🏁 Script executed:
git log --oneline --all pkg/js/compiler/pool.go | head -20Repository: projectdiscovery/nuclei
Length of output: 1051
🏁 Script executed:
git show eabf0ecf~1:pkg/js/compiler/pool.go 2>/dev/null | head -150Repository: projectdiscovery/nuclei
Length of output: 49
🏁 Script executed:
git show 3537030e:pkg/js/compiler/pool.go | sed -n '80,200p'Repository: projectdiscovery/nuclei
Length of output: 3930
🏁 Script executed:
sed -n '67,122p' pkg/js/compiler/pool_starvation_test.goRepository: projectdiscovery/nuclei
Length of output: 1789
🏁 Script executed:
git show 3537030e:pkg/js/compiler/pool.go | sed -n '140,200p'Repository: projectdiscovery/nuclei
Length of output: 2213
Watchdog removal reintroduces starvation risk for blocking native callbacks.
The recent refactoring removed the watchdog goroutine (commit 3537030) that released pool slots when context deadlines expired. The current implementation relies on runtime.Interrupt() and waiting for resultChan, but Interrupt() does not preempt code executing outside the VM. If a blocking native callback is running when the context deadline expires, the slot remains held (zombie scenario) until RunProgram() actually returns, not until the deadline fires.
The TestWatchdogPreventsStarvation test (lines 67-122) explicitly validates the scenario being reintroduced: slots must be freed at the deadline to prevent pool exhaustion, even if the goroutine continues running.
Either restore a slot-only watchdog mechanism, or add a regression test demonstrating that every long-running blocking callback respects the context deadline and releases the slot promptly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/js/compiler/pool.go` around lines 132 - 138, Context deadline
cancellation currently only calls runtime.Interrupt() and then waits on
resultChan, which can leave a pool slot held if a blocking native callback runs
outside the VM; restore a slot-only watchdog so the slot is released immediately
on ctx.Done without waiting for RunProgram to return. Reintroduce a lightweight
watchdog goroutine that watches ctx.Done() and calls the pool slot-release
method (the same release/unblock logic used when RunProgram completes) as soon
as the deadline fires, keep runtime.Interrupt() and the existing resultChan
handling for eventual program termination, and add/update
TestWatchdogPreventsStarvation to assert that slots are freed at deadline even
when native callbacks remain blocking. Ensure you reference the select block
using ctx.Done(), runtime.Interrupt, resultChan, and RunProgram when locating
where to add the watchdog and where to assert behavior in tests.
There was a problem hiding this comment.
I added the case <-time.After(time.Second): to help prevent possible starvation, but from testing after calling runtime.Interrupt(ctx.Err()) the runtime pretty much instantly terminates, meaning that the context cancellation pretty much instantly free up the spot.
There was a problem hiding this comment.
@mikhail5555, that's a fair point — for pure JS execution (e.g., while(true) {}), runtime.Interrupt() is checked between VM opcodes, so the runtime terminates essentially instantly. The time.After(time.Second) fallback is a practical safety net.
There is one remaining edge case worth being aware of: if a blocking native Go callback (e.g., a long-running network dial) is executing when the context deadline fires, runtime.Interrupt() won't preempt it — the interrupt signal is only checked on the next JS opcode boundary. If that native call takes longer than 1 second, the timeout branch fires and executeWithRuntime returns, freeing the slot. However, the background goroutine (still holding a reference to runtime) continues running. Since executeWithPoolingProgram defers gojapool.Put(runtime), the runtime could be returned to the pool and acquired by another goroutine while the orphaned goroutine is still using it — a potential data race on the runtime.
In practice this is unlikely since:
- Most nuclei JS templates don't have blocking native callbacks that take >1 second.
- The interrupt is called, so the goroutine should terminate as soon as the native call returns.
But if you want to be safe, one option is to not return the runtime to the pool in the timeout path (i.e., only Put it when the goroutine confirmed completion via resultChan), accepting that the leaked goroutine means that runtime instance is discarded rather than reused. The sync.Pool will create a new one as needed.
That said, the current implementation is a clear improvement over the pre-PR behavior (unbounded zombie runtimes), so it's reasonable to ship this and revisit if the edge case ever surfaces in practice.
🧠 Learnings used
Learnt from: ayuxsec
Repo: projectdiscovery/nuclei PR: 6769
File: pkg/installer/template.go:217-217
Timestamp: 2026-01-15T10:44:22.186Z
Learning: In projectdiscovery/gologger, the Print() function writes to stderr and Silent() writes to stdout. During reviews, ensure code uses Print() for error/output going to stderr and Silent() for stdout; verify the chosen function matches the intended destination and that logs are directed to the correct stream for proper piping/redirects.
…now can use the channel
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pkg/js/compiler/pool.go (1)
126-130:⚠️ Potential issue | 🟠 MajorRelease the pool slot on cancellation instead of waiting for
RunProgram()to finish.On Line 129 the cancel path still blocks on
<-resultChan>, and Lines 164-166 only callpooljsc.Done()afterexecuteWithRuntime()returns. If a registered Go helper is still blocked after the deadline, this execution keeps its slot anyway and can starve the pool despite the canceled context. Reintroduce an early slot-release path onctx.Done()and keepgojapool.Put(runtime)synchronized with actual runtime cleanup.Also applies to: 164-166
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/js/compiler/pool.go` around lines 126 - 130, The cancel branch in the select inside the function using resultChan currently blocks on `<-resultChan>` and only calls pooljsc.Done()/gojapool.Put(runtime) after executeWithRuntime() returns, so a canceled execution holds its pool slot; change the ctx.Done() path to immediately release the pool slot (call pooljsc.Done() or gojapool.Put(runtime) as appropriate) before waiting for the execution result, and then wait for result asynchronously (e.g., spawn a goroutine to drain resultChan and perform runtime cleanup after executeWithRuntime() finishes) so that executeWithRuntime()/runtime cleanup remains synchronized but the pool slot is freed on cancellation; reference the select handling resultChan, runtime.Interrupt(ctx.Err()), executeWithRuntime(), pooljsc.Done(), and gojapool.Put(runtime).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/js/compiler/pool.go`:
- Around line 138-145: ExecuteProgram dereferences opts immediately but the API
allows nil; guard for opts == nil at the start of ExecuteProgram and normalize
it to a default value (e.g. set opts = &ExecuteOptions{} or call a helper that
returns default options) before any use so subsequent checks like opts.Source
and calls behave correctly; update the function (referencing ExecuteProgram,
opts, and ExecuteOptions) to perform this nil-check/normalization at the top of
the function.
---
Duplicate comments:
In `@pkg/js/compiler/pool.go`:
- Around line 126-130: The cancel branch in the select inside the function using
resultChan currently blocks on `<-resultChan>` and only calls
pooljsc.Done()/gojapool.Put(runtime) after executeWithRuntime() returns, so a
canceled execution holds its pool slot; change the ctx.Done() path to
immediately release the pool slot (call pooljsc.Done() or gojapool.Put(runtime)
as appropriate) before waiting for the execution result, and then wait for
result asynchronously (e.g., spawn a goroutine to drain resultChan and perform
runtime cleanup after executeWithRuntime() finishes) so that
executeWithRuntime()/runtime cleanup remains synchronized but the pool slot is
freed on cancellation; reference the select handling resultChan,
runtime.Interrupt(ctx.Err()), executeWithRuntime(), pooljsc.Done(), and
gojapool.Put(runtime).
🪄 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: 2a961b64-21fb-44f2-9631-8d9d77c305ad
📒 Files selected for processing (1)
pkg/js/compiler/pool.go
| func ExecuteProgram(ctx context.Context, p *goja.Program, args *ExecuteArgs, opts *ExecuteOptions) (result goja.Value, err error) { | ||
| if opts.Source == nil { | ||
| // not-recommended anymore | ||
| return executeWithoutPooling(p, args, opts) | ||
| return executeWithoutPooling(ctx, p, args, opts) | ||
| } | ||
| if !stringsutil.ContainsAny(*opts.Source, exportAsToken, exportToken) { | ||
| // not-recommended anymore | ||
| return executeWithoutPooling(p, args, opts) | ||
| return executeWithoutPooling(ctx, p, args, opts) |
There was a problem hiding this comment.
Guard opts == nil in the exported entrypoint.
ExecuteProgram is documented as using default options, but Line 139 dereferences opts immediately. A direct caller passing nil will panic before reaching either execution path. Normalize opts here so the API contract matches the implementation.
Proposed fix
func ExecuteProgram(ctx context.Context, p *goja.Program, args *ExecuteArgs, opts *ExecuteOptions) (result goja.Value, err error) {
+ if opts == nil {
+ opts = &ExecuteOptions{}
+ }
if opts.Source == nil {
// not-recommended anymore
return executeWithoutPooling(ctx, p, args, opts)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/js/compiler/pool.go` around lines 138 - 145, ExecuteProgram dereferences
opts immediately but the API allows nil; guard for opts == nil at the start of
ExecuteProgram and normalize it to a default value (e.g. set opts =
&ExecuteOptions{} or call a helper that returns default options) before any use
so subsequent checks like opts.Source and calls behave correctly; update the
function (referencing ExecuteProgram, opts, and ExecuteOptions) to perform this
nil-check/normalization at the top of the function.
dwisiswant0
left a comment
dwisiswant0
left a comment
There was a problem hiding this comment.
I'm pretty sure this was already addressed in #7299, what makes you think otherwise? Honestly, you should probably open an issue first before jumping straight to this.
I don't believe it was addresses sufficiently, the two tests (TestPooledRuntimeTerminatesOnContextExpiry, TestPooledRuntimeTerminatesOnContextExpiry) that i added do not terminate (aka keep running indefinitely) in the existing code. The runtime is indeed returned to the pool, but is in fact still in use (also see pprof dump in which a poorly crafted nuclei template lead to 1 cpu being utilized all the time in a zombie process). Having the If you prefer i can indeed open up an issue first. Edit: Just double checked on our cluster, we are still getting the goja (ghost) runtimes on the latests #dev version. So I am very certain this PR is necessary to fully resolve that issue. |
Yes, please drop a concrete example that goja runtime did not terminate in any realistic execution path while you're at it, e.g., a template + exact nuclei command that hangs here, or a custom runner (via SDK) PoC. It would be a huge help for us to actually reproduce the bug on our end. Thanks! |
Just a double check, do you consider an accidental infinite loop (without requests) not an realistic template? (I would expect that after the timeout has been reached any template should be terminated even if it is poorly crafted). (Say faulty parsing logic that never terminates due to an off by 1 error), because I would say even though the test ofcourse uses an while true loop, nothing is preventing this from happening in an real manually crafted template. |
Yup, we do consider that situation (even one without any requests) as long as the infinite execution/non-terminating path is reachable in an end-to-end run, it's a legit enough bug for us to care about. |
dwisiswant0
left a comment
dwisiswant0
left a comment
There was a problem hiding this comment.
LGTM! Tested the patch against the reproduction case in #7348, and can confirm the e2e tests are passing after applying the patch.
Great catch on that!
Thanks for your contributions!
dwisiswant0
changed the title
2 participants
Proposed changes
Explicitly propagate context to goja runtime functions.
Based on context, emit an runtime.Interrupt https://github.com/dop251/goja?tab=readme-ov-file#interrupting, so that the runtime does not stay running in the background, even if the function around it has returned.
Proof
The tests added show that on context.cancel the runtime terminates (This was not the case before, removing the
runtime.Interrupt(ctx.Err())will make the tests fail.)Checklist
Summary by CodeRabbit
Bug Fixes
Refactor
Tests