fix(core): honour the scan context when acquiring worker slots and JS runtimes - #7678
fix(core): honour the scan context when acquiring worker slots and JS runtimes#7678gnuletik wants to merge 1 commit into
Conversation
… runtimes Four acquires in the scan path ignored the context they were given, so a cancelled scan could not leave until unrelated work finished: - executeTemplateSpray and executeTemplatesOnTarget acquire a worker slot via AdaptiveWaitGroup.Add, which passes context.Background(). - executeHostSpray does the same. - flow.GetJSRuntime acquires from a process-global goja pool with sizedpool.Get(context.TODO()). Each of these can wait a long time. A worker slot is held for as long as a template runs, and a JS runtime stays checked out for the whole flow including any wait on the rate limiter, so the pool saturates under concurrent scans. A scan whose context is already cancelled then queues for a runtime it only needs in order to unwind, and its own cancellation cannot break the wait. Both spray loops already check ctx.Done() at the top of each iteration, so the acquire was the only place cancellation was invisible. GetJSRuntime now takes a context and returns an error. This changes an exported signature; it has one caller in-tree.
Neo - PR Security ReviewNo exploitable security vulnerabilities introduced by this PR — all changes are context propagation for cancellation signaling with no attacker-controlled data flowing through the affected paths. What Neo reviewed
Comment |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. WalkthroughExecution scheduling and JavaScript runtime acquisition now honor cancellation contexts. Canceled scans stop waiting for workpool or runtime capacity, and runtime acquisition errors propagate through flow execution. ChangesExecution cancellation
Estimated code review effort: 2 (Simple) | ~15 minutes Merge Risk: ⚪ Minimal · up to This change makes scan and runtime waits honor cancellation, allowing cancelled scans to exit instead of waiting on unrelated work. No actionable merge-blocking risk remains beyond normal checks and review. Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
1 participant
Problem
Four acquires in the scan path ignore the context they are given, so a cancelled scan cannot leave until unrelated work finishes.
pkg/core/execute_options.goexecuteTemplateSpraywg.Add()→AdaptiveWaitGroup.AddWithContext(context.Background())pkg/core/execute_options.goexecuteHostSpraywp.Add()→ samepkg/core/executors.goexecuteTemplatesOnTargetsg.Add()→ samepkg/tmplexec/flow/vm.goGetJSRuntimesizedgojapool.Get(context.TODO())None of these return quickly under load. A worker slot is held for as long as a template runs, and a JS runtime stays checked out for the whole flow — including any wait on the rate limiter, since
flowacquires the runtime inExecuteWithResultsand only then reachesrequestExecutor→rateLimitTake.Both spray loops already check
ctx.Done()at the top of each iteration, so the acquire is the only place cancellation is invisible. Once a goroutine is inside it, the check above can never be reached again.Why it matters
flow's goja pool is process-global — built once underjsOnce, shared by every scan in the process, and clamped to a minimum of 100. Running the engine as a library with several concurrent scans, the pool saturates, and a scan whose context is already cancelled queues for a runtime it only needs in order to unwind. It cannot leave until an unrelated scan releases one.We hit this in production. Goroutine profiles from seven wedged scans, across six processes and four hosts:
GetJSRuntimeEvery one sits exactly on the pool cap. In each, the scan's own goroutine is parked in:
waiting for a worker slot held by goroutines that are themselves waiting on
sizedpool.Get(context.TODO()). Cancelling the scan changes nothing, because neither wait observes it.Change
AdaptiveWaitGroup.Add()→AddWithContext(ctx)at the three worker-pool sites, returning when it errors.AddWithContextdecrements its own counter and skipswg.Add(1)on failure, so the error paths correctly do not callDone().GetJSRuntimetakes acontext.Contextand returns an error, passing it tosizedpool.Get.pkg/js/compileralready does this correctly viapooljsc.AddWithContext(ctx); onlyflowdid not.Notes for review
Exported signature change.
GetJSRuntime(opts)becomesGetJSRuntime(ctx, opts) (*goja.Runtime, error). It has one in-tree caller. If you would rather not break it, happy to addGetJSRuntimeWithContextand leave a deprecated wrapper — say the word.workflow_execute.godeliberately untouched. It has the sameswg.Add()pattern in two places, but workflow execution is a separate path and I have no evidence of it wedging, so I left it out rather than widen the diff on speculation. Glad to include it if you want the pattern fixed consistently.Test
TestGetJSRuntimeHonoursContextcovers both directions: a live context still gets a runtime, a cancelled one gets an error.semaphore.Acquirereports a cancelled context before it checks whether a slot is free, so the test does not need to saturate the pool to be meaningful. RevertingGet(ctx)toGet(context.TODO())fails it immediately withAn error is expected but got nil.go build ./...,go vet, andgo test ./pkg/core/... ./pkg/tmplexec/...pass locally.Summary by CodeRabbit