Skip to content

Conversation

@sevbch
Copy link
Collaborator

@sevbch sevbch commented Oct 29, 2025

Context

A customer reported this issue when scanning with ggshield, on a self-hosted instance (XXX= host url).

59089:605129 [E] ggshield.core.errors:209 status_code=None detail=HTTPSConnectionPool(host='XXX', port=443): Read timed out. (read
timeout=60)
2025-10-28 11:13:04 59089:605240 [W] urllib3.connectionpool:329 Connection pool is full, discarding connection: XXX. Connection pool size: 10
2025-10-28 11:13:23 59089:605238 [W] urllib3.connectionpool:329 Connection pool is full, discarding connection: XXX. Connection pool size: 10
Scanning... ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━╺━━ 93% 529 / 569
Error: Scanning failed: HTTPSConnectionPool(host='XXX', port=443): Read timed out. (read timeout=60)

What has been done

Adding a HTTPAdapter to the session configuration to better handle many parallel requests. The risk is that it could put additional pressure on the server. After reading this article, I left pool_connections to default value, as it shouldn't have much of an impact for our case, but increased pool_maxsize as we can see in the logs that this is the param that was blocking for the customer.

Another option was to increase the 60s timeout, but I'm afraid it would have too much impact. For instance, when a ggshield command fail, it could result in a longer waiting time for the caller to fetch the output.

I'm open to suggestions if you see another way of handling this.

Validation

Validated with the unit test.

PR check list

  • As much as possible, the changes include tests (unit and/or functional)
  • If the changes affect the end user (new feature, behavior change, bug fix) then the PR has a changelog entry (see doc/dev/getting-started.md). If the changes do not affect the end user, then the skip-changelog label has been added to the PR.

@codecov
Copy link

codecov bot commented Oct 29, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.95%. Comparing base (98f0c17) to head (2c04ebc).
⚠️ Report is 14 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1141   +/-   ##
=======================================
  Coverage   91.94%   91.95%           
=======================================
  Files         144      144           
  Lines        6196     6200    +4     
=======================================
+ Hits         5697     5701    +4     
  Misses        499      499           
Flag Coverage Δ
unittests 91.95% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@sevbch sevbch self-assigned this Oct 29, 2025
@sevbch sevbch marked this pull request as ready for review October 29, 2025 15:30
@sevbch sevbch requested a review from a team as a code owner October 29, 2025 15:30
@sevbch sevbch requested review from 6d7a and agateau-gg October 29, 2025 15:30
@sevbch sevbch force-pushed the severine/use-http-adapter-for-concurrency branch from 3323d34 to 567c1cf Compare November 4, 2025 15:58
@sevbch sevbch force-pushed the severine/use-http-adapter-for-concurrency branch from 567c1cf to 2c04ebc Compare November 4, 2025 16:08
@sevbch
Copy link
Collaborator Author

sevbch commented Nov 4, 2025

@agateau-gg I updated the PR, I think it should make more sense now.

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.

2 participants