Skip to content

fix(events): set 300s timeout on export HTTP clients#302

Merged
skylarmb merged 3 commits into
federated-sdk-release-candidatefrom
devin/1772775041-fix-export-timeout
Mar 6, 2026
Merged

fix(events): set 300s timeout on export HTTP clients#302
skylarmb merged 3 commits into
federated-sdk-release-candidatefrom
devin/1772775041-fix-export-timeout

Conversation

@devin-ai-integration
Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration Bot commented Mar 6, 2026

fix(events): set 300s timeout on export HTTP clients

Summary

export() and export_async() create httpx.Client / httpx.AsyncClient without specifying a timeout, inheriting httpx's 5-second default. Large event exports (e.g. 7500 events via get_by_session_id()) take 30-80s on the wire, so every such request hits ReadTimeout. The retry loop (3 retries with backoff) amplifies this into ~27s of wasted time before final failure.

Fix: Adds a module-level EXPORT_TIMEOUT constant with split timeout values and passes it to both httpx client constructors. Uses short timeouts for connect/write/pool (10-30s) to fail fast on unreachable hosts, while giving read 300s to accommodate large result sets. Validated against staging — 7500 events now return successfully in ~79s instead of timing out.

Changes:

  • src/honeyhive/api/client.py — add EXPORT_TIMEOUT = httpx.Timeout(connect=10.0, read=300.0, write=30.0, pool=10.0) constant, pass timeout=EXPORT_TIMEOUT to both httpx.Client and httpx.AsyncClient
  • tests_v2/unit/test_events_export_timeout.py — 8 unit tests covering the constant value, sync export(), async export_async(), and get_by_session_id_async() delegation
  • CHANGELOG.md — customer-facing entry under [Unreleased] > Fixed
  • examples/integrations/strands_agents_example.py — unrelated black formatting fix (resolves pre-existing Generated Code Check CI failure)

Review & Testing Checklist for Human

  • End-to-end verification: Install this branch and run a real large export (e.g. get_by_session_id() returning 5000+ events) against staging or production. Unit tests mock the HTTP layer, so they verify the timeout is passed but cannot confirm real-world behavior.
  • Split timeout values are reasonable: connect=10.0, read=300.0, write=30.0, pool=10.0 — particularly verify that 10s connect is enough for high-latency customer networks.
  • Not user-configurable: The timeout is a module constant, not overridable per-call or via env var. Decide if that's acceptable or if it should be plumbed through HoneyHive(timeout=...).

Notes

  • Reproduced on staging with federated SDK 1.0.0rc18: export_async(limit=7500)ReadTimeout after 27s (4 attempts × 5s + backoff). After fix: 7500 events returned in 78.92s.
  • The old published SDK (v0.2.57) did not have this issue because it used timeout=None. This is a regression introduced in the federated SDK's direct httpx usage.
  • Requested by: @dhruv-hhai
  • Devin session

The export() and export_async() methods created httpx.Client /
httpx.AsyncClient without an explicit timeout, inheriting the httpx
default of 5 seconds.  Large result sets (e.g. 7500 events) need
30-80 seconds on the wire, so every such request hit ReadTimeout and
the retry loop amplified it to ~27 s of wasted retries.

Add a module-level EXPORT_TIMEOUT = httpx.Timeout(300.0) constant and
pass it to both client constructors so exports can complete even on
very large queries.

Co-Authored-By: Dhruv <dhruv@honeyhive.ai>
@devin-ai-integration
Copy link
Copy Markdown
Contributor Author

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment and CI monitoring

@snyk-io
Copy link
Copy Markdown

snyk-io Bot commented Mar 6, 2026

Snyk checks have passed. No issues have been found so far.

Status Scanner Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues
Licenses 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Mar 6, 2026

Claude finished @devin-ai-integration[bot]'s task in 1m 0s —— View job


Review of PR #302 — Complete

  • Read changed files and gather context
  • Review src/honeyhive/api/client.py changes
  • Review test file
  • Review CHANGELOG.md
  • Post review feedback

Summary

The fix is correct and well-targeted — adding EXPORT_TIMEOUT to the two export HTTP clients resolves the ReadTimeout regression for large event exports. Tests cover sync, async, and delegation paths.

I left two inline comments:

  1. EXPORT_TIMEOUT split (see comment) — consider using separate connect/read/write/pool values so that unreachable hosts still fail fast (~10s) while only read gets the 300s budget.
  2. Unused asyncio import (see comment) — minor cleanup.

No documentation impact — this is a behavioral bugfix with no public API surface changes.

Comment thread src/honeyhive/api/client.py Outdated
Comment thread tests_v2/unit/test_events_export_timeout.py Outdated
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Mar 6, 2026

Overall Review

The fix is correct and well-targeted. The root cause (httpx 5s default timeout vs 30-80s real-world export times) is clearly identified, and applying a longer timeout to the two export HTTP clients is the right approach.

What looks good:

  • Clean, minimal change — only touches the two call sites that need it
  • Good test coverage for both sync and async paths, including the get_by_session_id_async delegation
  • CHANGELOG entry is clear and customer-friendly

Suggestions (non-blocking):

  1. Split timeout values — see inline comment on EXPORT_TIMEOUT. Using httpx.Timeout(300.0) sets connect/write/pool all to 300s too, which means a DNS or TCP failure takes 5 minutes to surface. Consider keeping connect/pool short (10s) and only extending read.
  2. Unused importasyncio is imported but unused in the test file (line 14). Harmless but easy to clean up.

No documentation impact — this is a behavioral bugfix with no public API changes; no updates needed to docs/ or examples/.

Overall this is a solid, focused bugfix. Approve once the optional timeout split is considered.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Mar 6, 2026

📚 Documentation preview built — Download artifact

Review instructions & validation status

How to Review

  1. Download the artifact from the link above
  2. Extract the files
  3. Open index.html in your browser

Validation Status

  • API validation: ✅ Passed
  • Build process: ✅ Successful
  • Import tests: ✅ All imports working

Apply review suggestions:
- Use split httpx.Timeout(connect=10, read=300, write=30, pool=10) so
  unreachable hosts fail fast while only read gets the 300s budget
- Remove unused asyncio import from test file

Co-Authored-By: Dhruv <dhruv@honeyhive.ai>
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Mar 6, 2026

📚 Documentation preview built — Download artifact

Review instructions & validation status

How to Review

  1. Download the artifact from the link above
  2. Extract the files
  3. Open index.html in your browser

Validation Status

  • API validation: ✅ Passed
  • Build process: ✅ Successful
  • Import tests: ✅ All imports working

Co-Authored-By: Dhruv <dhruv@honeyhive.ai>
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Mar 6, 2026

📚 Documentation preview built — Download artifact

Review instructions & validation status

How to Review

  1. Download the artifact from the link above
  2. Extract the files
  3. Open index.html in your browser

Validation Status

  • API validation: ✅ Passed
  • Build process: ✅ Successful
  • Import tests: ✅ All imports working

@skylarmb skylarmb merged commit cf0f3da into federated-sdk-release-candidate Mar 6, 2026
18 checks passed
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