Deduplicate concurrent credential verification requests via singleflight

[kashifkhan0771]

Description:

Details: #2262

When TruffleHog scans at high concurrency, the same credential can appear in multiple chunks being processed simultaneously. Before the engine-level VerificationCache has stored a result, several detector workers can race to verify the same credential, each making an identical outbound HTTP request. This PR eliminates those redundant calls by introducing a singleflightTransport - an http.RoundTripper wrapper that coalesces concurrent requests sharing the same detector-type-namespaced dedup key into a single network call, buffering and replaying the response to all waiting goroutines. Detectors opt in with two lines: wrapping their client with detectors.NewClientWithDedup and stamping the request context with detectors.WithDedupKey(ctx, s.Type(), credential) before calling client.Do. This PR adopts the pattern across 16 detectors as an initial rollout.

Note: This is the first example implementation, if approved we will implement this in the most used detectors.

Checklist:

• Tests passing (make test-community)?
• Lint passing (make lint this requires golangci-lint)?

---

Note

Medium Risk
Adds a new http.RoundTripper that buffers and replays responses while coalescing concurrent verification requests; mistakes could change verification behavior, timeouts, or cancellation semantics across multiple detectors.

Overview
Introduces opt-in HTTP request deduplication for secret verification using a singleflight-backed transport (NewClientWithDedup + DoWithDedup), so concurrent verifications of the same detector type + credential share one outbound request and replay a buffered response.

Rolls this pattern out across several detectors by wrapping their clients and routing verification requests through DoWithDedup, and adds focused tests for coalescing, cancellation, and deadline behavior. Also tweaks plain output messaging to explicitly indicate when cached verification is used.

^{Reviewed by Cursor Bugbot for commit f6edad9. Bugbot is set up for automated code reviews on this repo. Configure here.}

[bugbaba]

Hi @kashifkhan0771,

Just gave it a quick test, its calling verify function twice if same key is found in two separate files.

[kashifkhan0771]

Just gave it a quick test, its calling verify function twice if same key is found in two separate files.

I believe this is related to the issue you mentioned here

[trufflesteeeve]

This is a cool caching implementation. I did a quick read of the related discussions (thank you very much for the links to additional context) and think that prior to us trying this idea, we should probably investigate improvements to the existing verification cache (e.g. making it worker-aware/thread-safe) before adding a second cache implementation.

Something like this very well might be where we end up, but first trying to improve what we already have could help reduce potential future complexity.

[kashifkhan0771]

Just gave it a quick test, its calling verify function twice if same key is found in two separate files.

@bugbaba For me it was using cached result for duplicate findings from different files with previous changes as well. However, as you mentioned, I implemented per-secret locking. This ensures that each secret is locked during verification until it's either verified or cached.

Please give it a try and let me know how it goes.

Also, I'm converting this PR back to a draft based on @trufflesteeeve comment. I agree that we should first focus on fixing the existing caching implementation. This PR will remain here for reference or in case we need it in the future.

[ahrav]

@kashifkhan0771 I’m not sure if you’ve considered using Go’s singleflight package to consolidate concurrent requests to the same endpoint into one. We could build a shared abstraction that all detectors use: whenever multiple goroutines ask for the same data at the same time, singleflight coalesces them into a single API call and fans out the response.

This won’t eliminate every duplicate request, requests separated by cache-miss delays can still slip through, but it’s essentially “free” to layer on and can cut down a lot of in-flight calls. For more background, see VictoriaMetrics’ blog on singleflight.

[kashifkhan0771]

@kashifkhan0771 I’m not sure if you’ve considered using Go’s singleflight package to consolidate concurrent requests to the same endpoint into one. We could build a shared abstraction that all detectors use: whenever multiple goroutines ask for the same data at the same time, singleflight coalesces them into a single API call and fans out the response.

This won’t eliminate every duplicate request, requests separated by cache-miss delays can still slip through, but it’s essentially “free” to layer on and can cut down a lot of in-flight calls. For more background, see VictoriaMetrics’ blog on singleflight.

Hands down, you always bring great insights 🙌🏻 I really appreciate it. I hadn’t come across singleflight before, but I did have a similar idea in mind about collapsing multiple requests into one. I didn’t realize Go had a package for this, so thanks a lot for pointing it out.

I’ll read through the documentation and blog post to get a better understanding of how to use it effectively. For now, I’ll keep this PR in draft and start incorporating some changes. Let’s see how things go.

[kashifkhan0771]

I have implemented singleflight for the GitHub detector for now. Please review and give it a try when you get a chance. I'm also thinking through how we can generalize this for all detectors, as @ahrav suggested.

[amanfcp]

@kashifkhan0771 I’m not sure if you’ve considered using Go’s singleflight package to consolidate concurrent requests to the same endpoint into one. We could build a shared abstraction that all detectors use: whenever multiple goroutines ask for the same data at the same time, singleflight coalesces them into a single API call and fans out the response.

This won’t eliminate every duplicate request, requests separated by cache-miss delays can still slip through, but it’s essentially “free” to layer on and can cut down a lot of in-flight calls. For more background, see VictoriaMetrics’ blog on singleflight.

Great read @ahrav. Thanks for sharing

[kashifkhan0771]

@ahrav I switched to using only singleflight for the verification logic. I chose a much simpler detector which includes response body handling. I believe this approach should effectively filter out duplicate verification requests happening concurrently. What do you think?

[rosecodym]

This is pretty cool! My main comment has to do with composability of this new HTTP request logic with some existing custom HTTP request logic - I flagged it inline.

[rosecodym]

Is there any way to compose this new deduplication logic with the loopback prevention enabled by some of the code in detectors/http.go? Even if it is possible, I find it confusing that they're located in different files. How much work would unifying them be? I think that would aid maintainability.

[kashifkhan0771]

Good Call - unifying them should only take a small amount of effort.

[rosecodym]

Does this mean that if a detector uses the singleflight transport but doesn't attach a dedup key, they won't be deduped, and this behavior will be silent? That's better than an error, but I still worry that people will do one but not the other. Unfortunately, I can't think of an obvious solution - static analysis would be great, but I don't know if it's feasible here, and it doesn't look like you have a logger to use to notify users at runtime.

[kashifkhan0771]

WithDedupKey is now unexported withDedupKey.
The only public interface is DoWithDedup(client,detType, credential, req) - a single call that atomically stamps the context key and executes the request. There is no longer a two-step pattern to get wrong.
Calling client.Do(req) instead of DoWithDedup still exists, but it's now a much more visible mistake: it's a different function name right at the call site, easy to catch in review.

[cursor]

Cursor Bugbot has reviewed your changes and found 1 potential issue.

^{Reviewed by Cursor Bugbot for commit 0df99b0. Configure here.}

[cursor]

Reconstructed HTTP response missing key fields

Low Severity

The http.Response constructed for coalesced callers omits ContentLength (defaults to 0 despite body having data), Proto, ProtoMajor, and ProtoMinor. While current detectors only inspect StatusCode and Body, this creates an incomplete response object. Since the PR plans to roll this out across many more detectors, any future detector or middleware that reads resp.ContentLength or protocol version fields will silently get incorrect zero values.

 

^{Reviewed by Cursor Bugbot for commit 0df99b0. Configure here.}

[rosecodym]

This logic looks both pretty gnarly and pretty important. Is it covered by the tests you added?

[kashifkhan0771]

Nope, I'll add them.

[rosecodym]

I expected to see this test alongside http.go, not here. Should it have been moved when you moved the new transport code?

[rosecodym]

Thanks for this!

[MuneebUllahKhan222]

This PR is really nice especially the place where you are handling and respecting the context of each caller is top tier. Also I got to learn something new i.e singleFlight so yay for that.

Just left a non-blocking comment you should or shouldn't handle depending on how you feel about it.

[MuneebUllahKhan222]

While we are at it I would suggest that we handle the issue bugbot is pointing at by updating this struct to

type bufferedResponse struct {
	statusCode   int
	header       http.Header
	body         []byte
	proto        string
	protoMajor   int
	protoMinor   int
	contentLength int64
}

and then populating it like this

return &bufferedResponse{
	statusCode:   resp.StatusCode,
	header:       resp.Header.Clone(),
	body:         body,
	proto:        resp.Proto,
	protoMajor:   resp.ProtoMajor,
	protoMinor:   resp.ProtoMinor,
	contentLength: int64(len(body)),
}, nil

and finally we can fully construct the http.Response

return &http.Response{
	StatusCode:    br.statusCode,
	Status:        fmt.Sprintf("%d %s", br.statusCode, http.StatusText(br.statusCode)),
	Header:        br.header.Clone(),
	Body:          io.NopCloser(bytes.NewReader(br.body)),
	Proto:         br.proto,
	ProtoMajor:    br.protoMajor,
	ProtoMinor:    br.protoMinor,
	ContentLength: br.contentLength,
}, nil