fix: improve cleanup in parallel execution

[knakul853]

• Added nil check for ticker in StartActiveMemGuardian to prevent potential nil pointer dereference.
• Updated StopActiveMemGuardian to ensure cancelFunc is only called if it's not nil.
• Introduced goleak tests in request_test.go to detect goroutine leaks during parallel HTTP execution scenarios.

Proposed changes

Checklist

• Pull request is created against the dev branch
• All checks passed (lint, unit/integration/regression tests etc.) with my changes
• I have added tests that prove my fix is effective or that my feature works
• I have added necessary documentation (if appropriate)

Summary by CodeRabbit

• Bug Fixes

  • Improved stability in parallel HTTP execution by ensuring worker pools always shut down on early exits, preventing leaks and races.
  • Safer lifecycle handling for in-memory monitoring to avoid nil access and ensure proper cancellation and cleanup.

• Tests

  • Added leak-detection tests for HTTP parallel execution and memory monitoring, covering cancellation, unresponsive targets, and repeated start/stop cycles.

• Chores

  • Added a test utility to clean up protocol state after runs to improve test teardown reliability.

[coderabbitai]

Walkthrough

Adds nil-safety and safer cancellation/teardown to MemGuardian start/stop logic and introduces lifecycle tests. Ensures the HTTP parallel executor closes the task channel and waits for worker goroutines on all early exits and completion; adds goroutine-leak tests for that executor. Exposes a test cleanup helper for protocol state.

Changes

Cohort / File(s)	Summary
MemGuardian lifecycle safety pkg/protocols/common/protocolstate/memguardian.go	Add local ticker alias and nil-check in StartActiveMemGuardian; use local ticker.C in select; safely invoke+clear cancelFunc and set memTimer to nil in StopActiveMemGuardian.
MemGuardian tests pkg/protocols/common/protocolstate/memguardian_test.go	New tests validating goroutine cleanup, idempotent start, context cancellation, and reset/restart using goleak and require assertions.
HTTP parallel execution cleanup pkg/protocols/http/request.go	Ensure tasks channel is closed and workers are waited on for all early exits (context done, resize error, request gen error, unresponsive target) and at function end; synchronize teardown with spmHandler.
HTTP leak tests pkg/protocols/http/request_test.go	Add goleak-based tests for StopAtFirstMatch, UnresponsiveHost, and ContextCancellation scenarios; configure ignores and assert expected outcomes.
Test utilities pkg/testutils/testutils.go	Add exported Cleanup(options *types.Options) to close protocol state for a given execution ID via protocolstate.Close.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor Caller
  participant Request as ExecuteParallelHTTP
  participant Workers as Worker Pool
  participant Tasks as tasks chan
  participant SPM as spmHandler

  Caller->>Request: Execute(...)
  Request->>Workers: start N workers
  Request->>SPM: init/resize spmHandler
  alt early exit (ctx done / resize error / gen error / unresponsive)
    Request->>Tasks: close(tasks)
    Request->>Workers: wait for all workers
    Request->>SPM: wait (if applicable)
    Request-->>Caller: return (err or nil)
  else normal processing
    loop process inputs
      Workers-->>Request: result/error
      opt stop condition met
        Request->>Tasks: close(tasks)
        Request->>Workers: wait for workers
      end
    end
    Request->>SPM: wait for SPM completion
    Request-->>Caller: aggregated result
  end

Loading

sequenceDiagram
  autonumber
  actor Sys
  participant MG as MemGuardian
  participant T as ticker
  participant Ctx as context

  Sys->>MG: StartActiveMemGuardian()
  MG->>T: create ticker -> local alias used in loop
  MG->>Ctx: create cancelable context
  MG->>MG: run loop selecting on local ticker.C and ctx.Done()
  Sys->>MG: StopActiveMemGuardian()
  alt cancelFunc set
    MG->>Ctx: invoke cancelFunc()
    MG->>MG: clear cancelFunc
  end
  MG->>T: stop ticker
  MG->>MG: set memTimer = nil

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

• Address race conditions in http.Request and MemGuardian #6321 — Also adjusts MemGuardian start/stop concurrency (mutex-based serialization) and related ticker/cancel handling.
• cache, goroutine and unbounded workers management #6420 — Overlaps with changes to parallel HTTP execution and worker teardown behavior.

Suggested reviewers

• dwisiswant0
• dogancanbakir
• ehsandeep

Poem

I twitch my ears at ticking time,
Close the chans, let workers rhyme.
No leaks hop loose, the garden’s clean,
Mem-guard stands calm, composed, serene.
With tidy tails and graceful stops, we bound—no dangling hops. 🐇

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title Check	✅ Passed	The title "fix: improve cleanup in parallel execution" is concise and accurately captures the main intent of the changeset — ensuring proper teardown and cleanup for parallel HTTP execution (and accompanying goroutine-leak tests); the smaller memguardian and testutils cleanup changes are consistent with this goal and do not conflict with the title. It uses a clear conventional-commit prefix and avoids vague language. A teammate scanning history would understand the primary change from the title.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

✨ Finishing touches

• 📝 Generate Docstrings

🧪 Generate unit tests

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch cleanup

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (4)

pkg/testutils/testutils.go (1)

36-39: Add nil/empty guard; use protocolinit.Close for symmetry with Init()

Avoid NPEs and keep lifecycle symmetric with Init().

-// Cleanup cleans up the protocols and their configurations
-func Cleanup(options *types.Options) {
-	protocolstate.Close(options.ExecutionId)
-}
+// Cleanup cleans up the protocols and their configurations
+func Cleanup(options *types.Options) {
+	if options == nil || options.ExecutionId == "" {
+		return
+	}
+	// Keep symmetry with Init()
+	protocolinit.Close(options.ExecutionId)
+}

pkg/protocols/http/request.go (1)

345-355: Good: drain on SPM Done

Closing tasks, waiting workers, then aggregating results is correct.

You repeat close(tasks)+workersWg.Wait() in several branches. Consider a local drain helper guarded by sync.Once to centralize and future‑proof against accidental double closes.

pkg/protocols/common/protocolstate/memguardian_test.go (2)

13-97: Leak tests look good; prefer Eventually over fixed sleeps

Replace fixed sleeps with require.Eventually to avoid flakes while asserting memTimer/cancelFunc transitions.

---

99-124: Reset test is valuable; same flakiness note

Use Eventually for post-stop/post-start assertions.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8ea5061 and 956f17e.

📒 Files selected for processing (5)

• pkg/protocols/common/protocolstate/memguardian.go (2 hunks)
• pkg/protocols/common/protocolstate/memguardian_test.go (1 hunks)
• pkg/protocols/http/request.go (3 hunks)
• pkg/protocols/http/request_test.go (2 hunks)
• pkg/testutils/testutils.go (1 hunks)

🧰 Additional context used

📓 Path-based instructions (2)

**/*.go

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.go: Format Go code using go fmt
Run static analysis with go vet

Files:

• pkg/testutils/testutils.go
• pkg/protocols/http/request_test.go
• pkg/protocols/common/protocolstate/memguardian.go
• pkg/protocols/http/request.go
• pkg/protocols/common/protocolstate/memguardian_test.go

pkg/protocols/**/*.go

📄 CodeRabbit inference engine (CLAUDE.md)

Each protocol implementation must provide a Request interface with methods Compile(), ExecuteWithResults(), Match(), and Extract()

Files:

• pkg/protocols/http/request_test.go
• pkg/protocols/common/protocolstate/memguardian.go
• pkg/protocols/http/request.go
• pkg/protocols/common/protocolstate/memguardian_test.go

🧬 Code graph analysis (3)

pkg/testutils/testutils.go (2)

pkg/protocols/common/protocolstate/state.go (1)

• Close (262-277)

pkg/protocols/common/protocolinit/init.go (1)

• Close (41-43)

pkg/protocols/http/request_test.go (3)

pkg/testutils/testutils.go (5)

• DefaultOptions (42-84)
• Init (31-34)
• Cleanup (37-39)
• NewMockExecuterOptions (94-111)
• TemplateInfo (87-91)

pkg/output/output.go (2)

• InternalEvent (93-93)
• InternalWrappedEvent (100-113)

pkg/protocols/common/contextargs/contextargs.go (1)

• NewWithInput (48-64)

pkg/protocols/common/protocolstate/memguardian_test.go (1)

pkg/protocols/common/protocolstate/memguardian.go (2)

• StartActiveMemGuardian (22-49)
• StopActiveMemGuardian (51-67)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: Lint

🔇 Additional comments (9)

pkg/protocols/http/request.go (4)

293-299: Good: drain workers on context cancellation

Closing tasks and waiting eliminates leaks when input context is done.

---

303-307: Good: drain on resize error

Ensures worker pool is torn down before returning the error.

---

329-333: Good: drain on generator error

Prevents goroutine leaks when Make() fails.

---

340-343: Good: cancel + drain on unresponsive host

Matches the SPM cancel path and avoids stranding workers.

pkg/protocols/common/protocolstate/memguardian.go (2)

32-35: Good: local ticker alias and nil check

Removes race on memTimer and avoids nil deref in the goroutine.

---

59-66: Good: safe cancellation and state reset

Cancel-if-non-nil and nil out both cancelFunc and memTimer; prevents lingering goroutines and enables clean restarts.

pkg/protocols/http/request_test.go (3)

10-10: Import time is needed by new goleak tests

All good.

---

13-13: Goroutine leak detection added

Using goleak improves reliability of parallel execution paths.

---

377-527: Reduce flakiness: lengthen timing windows or use require.Eventually; re-run race test with CGO enabled

• 10–50ms sleep windows are tight for CI — use require.Eventually or increase sleeps (e.g., ~100–200ms) to reduce flakes.
• Verification: your run failed with "-race requires cgo". Re-run locally with CGO enabled:
  CGO_ENABLED=1 go test -race -run TestExecuteParallelHTTP_GoroutineLeaks -count=1 ./pkg/protocols/http

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (2)

pkg/protocols/http/request_test.go (2)

516-523: Simplify cancellation with context.WithTimeout (drops extra goroutine).

Cleaner and avoids a helper goroutine.

-        ctx, cancel := context.WithCancel(context.Background())
-        ctxArgs := contextargs.NewWithInput(ctx, ts.URL)
-
-        go func() {
-            time.Sleep(50 * time.Millisecond)
-            cancel()
-        }()
+        ctx, cancel := context.WithTimeout(context.Background(), 50*time.Millisecond)
+        defer cancel()
+        ctxArgs := contextargs.NewWithInput(ctx, ts.URL)

---

516-527: Use ErrorIs for cancellation checks (avoid brittle equality).

Testify v1.11.1 includes require.ErrorIs — replace the equality assertion with require.ErrorIs.

-        err = req.ExecuteWithResults(ctxArgs, metadata, previous, func(event *output.InternalWrappedEvent) {})
-        require.Error(t, err)
-        require.Equal(t, context.Canceled, err)
+        err = req.ExecuteWithResults(ctxArgs, metadata, previous, func(event *output.InternalWrappedEvent) {})
+        require.ErrorIs(t, err, context.Canceled)

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between dca3125 and d491d13.

📒 Files selected for processing (1)

• pkg/protocols/http/request_test.go (2 hunks)

🧰 Additional context used

📓 Path-based instructions (2)

**/*.go

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.go: Format Go code using go fmt
Run static analysis with go vet

Files:

• pkg/protocols/http/request_test.go

pkg/protocols/**/*.go

📄 CodeRabbit inference engine (CLAUDE.md)

Each protocol implementation must provide a Request interface with methods Compile(), ExecuteWithResults(), Match(), and Extract()

Files:

• pkg/protocols/http/request_test.go

🧬 Code graph analysis (1)

pkg/protocols/http/request_test.go (3)

pkg/testutils/testutils.go (5)

• DefaultOptions (42-84)
• Init (31-34)
• Cleanup (37-39)
• NewMockExecuterOptions (94-111)
• TemplateInfo (87-91)

pkg/output/output.go (2)

• InternalEvent (93-93)
• InternalWrappedEvent (100-113)

pkg/protocols/common/contextargs/contextargs.go (1)

• NewWithInput (48-64)

🔇 Additional comments (3)

pkg/protocols/http/request_test.go (3)

379-391: Goroutine-leak guard looks solid.

Good placement (deferred at test end) and comprehensive ignore list to avoid false positives.

---

393-396: Proper lifecycle cleanup before verification.

Deferring testutils.Cleanup before VerifyNone ensures teardown runs first. LGTM.

---

13-13: Forked goleak is pinned in go.mod — no action required

Tests import github.com/tarunKoyalwar/goleak (e.g. lib/tests/sdk_test.go, pkg/protocols/http/request_test.go, pkg/protocols/common/protocolstate/memguardian_test.go) and go.mod already pins github.com/tarunKoyalwar/goleak v0.0.0-20240429141123-0efa90dbdcf9; switch to go.uber.org/goleak only if you prefer upstream.