fix leaks - #7502
Merged
Merged
Conversation
Contributor
WalkthroughThe SDK now records parser ownership and purges parser caches on shutdown. The parser exposes a purge method, the HTTP→HTTPS tracker uses a bounded LRU cache, and protocol shutdown now closes the per-host rate-limit pool. ChangesParser cache ownership and purge
HTTP→HTTPS tracker cache bounds
Per-host rate-limit shutdown
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ 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 |
Member
Author
|
@coderabbitai create an issue for this PR |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="projectdiscovery/nuclei"
pr_number="7502"
requester="Mzack9999"
pr_json=$(gh pr view "$pr_number" --repo "$repo" --json title,url,body --jq '.')
pr_title=$(jq -r '.title' <<<"$pr_json")
pr_url=$(jq -r '.url' <<<"$pr_json")
comment_url=$(
gh api "repos/$repo/issues/$pr_number/comments" --paginate \
--jq '.[] | select(.user.login=="'"$requester"'") | select(.body | contains("`@coderabbitai` create an issue for this PR")) | .html_url' \
| tail -n1
)
issue_title="Memory and goroutine leaks in long-running embedded engine usage"
issue_body=$(cat <<EOF
## Summary
The embedded engine can leak memory and goroutines over time during long-running usage.
## Required changes
Implement the leak-prevention work described in #$pr_number:
- bound the \`HTTPToHTTPS\` tracker with an LRU
- release the per-host rate-limit pool goroutines on close
- purge the template caches on engine close
## Rationale
Without explicit cleanup and bounded caching, long-running embedders can accumulate memory usage and leave background goroutines running indefinitely.
## Affected areas
- \`HTTPToHTTPS\` tracking / redirect bookkeeping
- per-host rate limit pool lifecycle and shutdown
- template cache lifecycle during engine close
## Acceptance criteria
- The \`HTTPToHTTPS\` tracker is size-bounded and evicts old entries.
- Per-host rate-limit pool goroutines are released when the engine closes.
- Template caches are purged on engine close.
- Long-running embedded usage no longer shows continued growth from these resources.
## Backlinks
- Pull request: $pr_url
- Request comment: ${comment_url:-Unavailable}
- Requested by: @$requester
## Additional context
PR title: $pr_title
EOF
)
issue_url=$(gh issue create \
--repo "$repo" \
--title "$issue_title" \
--body "$issue_body" \
--assignee "$requester")
printf 'ISSUE_URL=%s\n' "$issue_url"
printf 'PR_URL=%s\n' "$pr_url"
printf 'COMMENT_URL=%s\n' "${comment_url:-Unavailable}"Length of output: 2331
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/protocols/http/httpclientpool/http_to_https_tracker_test.go (1)
43-59: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover the
RequiresHTTPSrecency-refresh contract.This test proves capacity eviction, but it would still pass if lookups stopped refreshing LRU recency. Add a small capacity-2 regression case so the
Getbehavior stays protected.Proposed test addition
func TestHTTPToHTTPSPortTracker_BoundedLRU(t *testing.T) { const size = 100 tr := newHTTPToHTTPSPortTrackerWithSize(size) @@ require.False(t, tr.RequiresHTTPS("http://host0.example.com:8443/"), "oldest entry should be evicted") require.True(t, tr.RequiresHTTPS(fmt.Sprintf("http://host%d.example.com:8443/", size*3-1)), "newest entry should be retained") } + +func TestHTTPToHTTPSPortTracker_RequiresHTTPSRefreshesLRU(t *testing.T) { + tr := newHTTPToHTTPSPortTrackerWithSize(2) + + tr.RecordHTTPToHTTPSPort("http://a.example.com:8443/") + tr.RecordHTTPToHTTPSPort("http://b.example.com:8443/") + + require.True(t, tr.RequiresHTTPS("http://a.example.com:8443/"), "lookup should refresh recency") + + tr.RecordHTTPToHTTPSPort("http://c.example.com:8443/") + + require.True(t, tr.RequiresHTTPS("http://a.example.com:8443/"), "refreshed entry should be retained") + require.False(t, tr.RequiresHTTPS("http://b.example.com:8443/"), "untouched least-recently-used entry should be evicted") + require.True(t, tr.RequiresHTTPS("http://c.example.com:8443/")) +}🤖 Prompt for 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. In `@pkg/protocols/http/httpclientpool/http_to_https_tracker_test.go` around lines 43 - 59, The bounded LRU test in TestHTTPToHTTPSPortTracker_BoundedLRU only verifies eviction, so it does not protect the recency-refresh behavior of RequiresHTTPS/Get. Add a small regression case with capacity 2 using newHTTPToHTTPSPortTrackerWithSize and RecordHTTPToHTTPSPort, then call RequiresHTTPS on the older entry before inserting a third host so that the accessed entry stays tracked while the least-recently used one is evicted. Use the existing HTTPToHTTPSPortTracker methods and Stats to assert both eviction and that lookups refresh recency.
🤖 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/protocols/http/httpclientpool/http_to_https_tracker.go`:
- Around line 24-26: `HTTPToHTTPSPortTracker` is now unsafe as a zero-value
because `ports` is a pointer to `lru.Cache`, so public methods can panic if the
type is constructed directly. Update the tracker methods on
`HTTPToHTTPSPortTracker` to defensively handle a nil `ports` field, or enforce
initialization through `NewHTTPToHTTPSPortTracker`/`GetHTTPToHTTPSPortTracker`
by ensuring all exported entry points guard against uninitialized instances.
Keep the fix centered on the `HTTPToHTTPSPortTracker` type and its public
accessors so future external use remains safe.
In `@pkg/protocols/http/httpclientpool/perhost_ratelimit_pool_close_test.go`:
- Line 30: The pre-close goroutine check in the rate-limit pool close test is
flaky because it relies on a single runtime.NumGoroutine() snapshot. Update the
assertion in the test around pool.Close() to use require.Eventually, just like
the post-close goroutine cleanup check, so it waits for limiter goroutines to
actually appear before proceeding.
---
Nitpick comments:
In `@pkg/protocols/http/httpclientpool/http_to_https_tracker_test.go`:
- Around line 43-59: The bounded LRU test in
TestHTTPToHTTPSPortTracker_BoundedLRU only verifies eviction, so it does not
protect the recency-refresh behavior of RequiresHTTPS/Get. Add a small
regression case with capacity 2 using newHTTPToHTTPSPortTrackerWithSize and
RecordHTTPToHTTPSPort, then call RequiresHTTPS on the older entry before
inserting a third host so that the accessed entry stays tracked while the
least-recently used one is evicted. Use the existing HTTPToHTTPSPortTracker
methods and Stats to assert both eviction and that lookups refresh recency.
🪄 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: e84fff22-06e5-41bb-86d0-4e8604584476
📒 Files selected for processing (8)
lib/sdk.golib/sdk_private.gopkg/protocols/common/protocolstate/state.gopkg/protocols/http/httpclientpool/http_to_https_tracker.gopkg/protocols/http/httpclientpool/http_to_https_tracker_test.gopkg/protocols/http/httpclientpool/perhost_ratelimit_pool_close_test.gopkg/templates/parser.gopkg/templates/parser_purge_test.go
dwisiswant0
requested changes
Jun 28, 2026
dwisiswant0
left a comment
dwisiswant0
left a comment
Member
There was a problem hiding this comment.
var t HTTPToHTTPSPortTracker; t.Stats() was panic'ing at t.ports.Len(), and RecordHTTPToHTTPSPort, RequiresHTTPS, Evict all had the same nil-cache problem, unlike sync.Map that's safe from zero-value.
I think we need to lazily init the cache in the public methods with a tiny synchronized helper.
dwisiswant0
reviewed
Jun 28, 2026
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/protocols/http/httpclientpool/http_to_https_tracker_test.go (1)
110-118: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a start barrier so this race-sensitive test actually overlaps the writers.
WaitGrouponly waits for completion; it does not force theRecordHTTPToHTTPSPortcalls to contend. A broken check-then-add implementation can still pass here if the goroutines happen to run mostly sequentially. Releasing the workers through a shared channel makes this test much more likely to exercise the atomicity contract it is asserting.Suggested change
const goroutines = 64 var wg sync.WaitGroup + start := make(chan struct{}) wg.Add(goroutines) for i := 0; i < goroutines; i++ { go func() { defer wg.Done() + <-start tr.RecordHTTPToHTTPSPort("http://race.example.com:8443/") }() } + close(start) wg.Wait()🤖 Prompt for 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. In `@pkg/protocols/http/httpclientpool/http_to_https_tracker_test.go` around lines 110 - 118, The race-sensitive test in http_to_https_tracker_test.go needs a start barrier so the RecordHTTPToHTTPSPort calls overlap instead of running mostly sequentially. Update the concurrent goroutine setup around the WaitGroup and RecordHTTPToHTTPSPort invocation to block all workers on a shared release signal before entering the write path, so the test actually stresses the atomicity behavior it is meant to verify.
🤖 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.
Nitpick comments:
In `@pkg/protocols/http/httpclientpool/http_to_https_tracker_test.go`:
- Around line 110-118: The race-sensitive test in http_to_https_tracker_test.go
needs a start barrier so the RecordHTTPToHTTPSPort calls overlap instead of
running mostly sequentially. Update the concurrent goroutine setup around the
WaitGroup and RecordHTTPToHTTPSPort invocation to block all workers on a shared
release signal before entering the write path, so the test actually stresses the
atomicity behavior it is meant to verify.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7bd3d39d-9c3f-4294-b597-3a6228c87078
📒 Files selected for processing (3)
pkg/protocols/http/httpclientpool/http_to_https_tracker.gopkg/protocols/http/httpclientpool/http_to_https_tracker_test.gopkg/protocols/http/httpclientpool/perhost_ratelimit_pool_close_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/protocols/http/httpclientpool/perhost_ratelimit_pool_close_test.go
- pkg/protocols/http/httpclientpool/http_to_https_tracker.go
dwisiswant0
approved these changes
Jun 29, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
bound the HTTPToHTTPS tracker with an lru release the per host rate limit pool goroutines on close and purge the template caches on engine close so a long running embedder stops leaking memory and goroutines
Summary by CodeRabbit