Skip to content

fix(providers): increase HTTP connection pool for embedding provider#1123

Open
ronaldtangg wants to merge 1 commit into
nextlevelbuilder:devfrom
ronaldtangg:fix/embedding-http-pool
Open

fix(providers): increase HTTP connection pool for embedding provider#1123
ronaldtangg wants to merge 1 commit into
nextlevelbuilder:devfrom
ronaldtangg:fix/embedding-http-pool

Conversation

@ronaldtangg

Copy link
Copy Markdown

Summary

Fix HTTP connection pool starvation in embedding provider that caused
team_tasks(action="search") to block when >5 concurrent requests hit
the embedding API simultaneously.

Type

  • Bug fix

Target Branch

dev

Checklist

  • go build ./... passes
  • go build -tags sqliteonly ./... passes
  • go vet ./... passes
  • No hardcoded secrets or credentials
  • SQL queries use parameterized $1, $2 (no string concat)

Test Plan

  1. Deploy to staging
  2. Trigger 10+ concurrent chat sessions that call team_tasks(action="search")
  3. Verify all requests complete without blocking
  4. Previously: >5 concurrent requests would hang indefinitely
  5. After fix: 30 concurrent connections supported per host

@mrgoonie mrgoonie left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the focused fix. I confirmed the linked issue is real and this PR touches the right provider path, but I do not want to merge this as a bespoke transport literal yet.

Please align this with the repo-standard provider HTTP defaults instead of introducing a second, narrower transport shape here:

  • internal/providers/defaults.go already centralizes provider transport behavior in NewDefaultTransport() / NewDefaultHTTPClient() with proxy support, stage timeouts, TLS handshake timeout, and keep-alive settings.
  • This PR's local &http.Transport{MaxIdleConns: 100, MaxIdleConnsPerHost: 30, IdleConnTimeout: 90s} omits those shared defaults and will drift from the rest of the provider stack.
  • If embeddings genuinely need a higher per-host pool than the default 10, add a small named helper such as NewEmbeddingHTTPClient()/NewEmbeddingTransport() that starts from the shared provider defaults and documents why embeddings use a higher MaxIdleConnsPerHost.
  • Please add a focused regression/unit test asserting the embedding provider client uses a transport with increased per-host idle capacity, so the starvation fix cannot silently regress.

After that alignment this should be mergeable.

@clark-cant

Copy link
Copy Markdown
Contributor

🦸‍♂️ Maintainer check-in — PR #1123 has been in CHANGES_REQUESTED state for 2 days (since 2026-06-24) and the branch now has merge conflicts.

@mrgoonie's review feedback is specific and actionable:

  1. Use shared provider defaultsinternal/providers/defaults.go already centralizes transport behavior via NewDefaultTransport() / NewDefaultHTTPClient(). The bespoke &http.Transport{...} literal in this PR will drift from the rest of the provider stack.
  2. Create a named helper — something like NewEmbeddingHTTPClient() / NewEmbeddingTransport() that starts from the shared defaults and documents why embeddings need higher MaxIdleConnsPerHost.
  3. Add a regression test — assert the embedding provider client uses a transport with increased per-host idle capacity so the starvation fix can't silently regress.

@ronaldtangg — the fix targets a real issue (connection pool starvation at >5 concurrent embedding requests) and the scope is small (+9/-2). Addressing the review feedback + resolving the merge conflict should get this close to merge-ready. Need any help?

Posted by /github-maintain at $(date -u +%Y-%m-%dT%H:%M:%SZ)

@clark-cant

Copy link
Copy Markdown
Contributor

🦸‍♂️ Maintainer guidance — Good news: the review feedback from @mrgoonie is clear and actionable. Here is exactly what you need to do to get this merged:

1. Rebase onto current dev
Your branch is behind (DIRTY merge state). Run:

git fetch origin
git rebase origin/dev
git push --force-with-lease

2. Replace bespoke transport with shared defaults
Instead of hardcoding &http.Transport{MaxIdleConns: 100, MaxIdleConnsPerHost: 30, IdleConnTimeout: 90s}, use the repo-standard helper from internal/providers/defaults.go:

client: NewDefaultHTTPClient(),  // or NewDefaultTransport() + override MaxIdleConnsPerHost

If embeddings genuinely need a higher per-host pool than the default, create a small named helper like NewEmbeddingHTTPClient() that starts from NewDefaultTransport() and documents why embeddings use a higher value.

3. Add a regression test
Assert the embedding provider client uses a transport with increased per-host idle capacity, so the starvation fix cannot silently regress.

This is a contained change — should be mergeable within a day after these adjustments. Let us know if you need help with the rebase!

Posted by github-maintain at $(date -u +%Y-%m-%dT%H:%M:%SZ)

@clark-cant clark-cant left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦸‍♂️ Maintainer review — Small, correct fix for HTTP connection pool starvation in the embedding provider.

Summary: Adds a custom http.Transport with MaxIdleConnsPerHost: 30 to prevent concurrent embedding requests from blocking.

Risk level: Low — single file, +9/-2, well-scoped.

Findings:

  • Suggestion: The bare http.Transport{} replaces http.DefaultTransport settings. Consider preserving ForceAttemptHTTP2: true, TLSHandshakeTimeout, and DialContext defaults (or embed the default transport). Unlikely to cause issues for a single-endpoint embedding provider, but worth noting.

Verdict: Comment — Fix is correct and addresses a real production issue. Branch is stale (DIRTY merge state) and needs a rebase onto current dev before merge.

@ronaldtangg Could you rebase onto latest dev? The fix itself looks good.

Posted by github-maintain at 2026-06-29T16:10:00Z

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants