Skip to content

Conversation

@Robdel12
Copy link
Contributor

Summary

  • Add open_timeout: 10 to Ruby SDK HTTP calls to prevent hanging on unreachable servers

Without this, connection attempts to servers that don't actively refuse (e.g., firewall dropping packets) could hang for 60+ seconds, potentially slowing down CI runs.

Test plan

  • Ruby SDK unit tests pass
  • Verified timeout parameters in Net::HTTP.start call

Without open_timeout, connection attempts to unreachable servers
(e.g., firewall dropping packets) could hang for 60+ seconds.
Now fails fast with 10s open timeout + 30s read timeout.
@vizzly-testing

This comment has been minimized.

@vizzly-testing

This comment has been minimized.

@claude
Copy link

claude bot commented Jan 25, 2026

Code Review

This PR adds open_timeout: 10 to the Ruby SDK's HTTP client to prevent hanging on unreachable servers. The change is well-motivated and addresses a real CI/CD pain point.

✅ Positive Aspects

  1. Good problem identification: Without open_timeout, Ruby's Net::HTTP will wait indefinitely for TCP connection establishment, which can stall tests for 60+ seconds when servers are unreachable due to firewall/network issues.

  2. Consistent with JS SDK: The JavaScript SDK in src/client/index.js:178 uses req.setTimeout(timeoutMs) with a 30-second default timeout, so this brings the Ruby client closer to parity.

  3. Well-documented: The commit message clearly explains the "why" behind the change.

  4. Minimal, focused change: Single-line modification that doesn't introduce scope creep.

🔍 Observations & Suggestions

1. Timeout Value Consideration

The open_timeout: 10 (10 seconds for connection) + read_timeout: 30 (30 seconds for response) creates a 40-second max timeout. Some considerations:

  • 10 seconds for open_timeout seems reasonable for local TDD servers (should connect instantly) and CI environments (generally fast networks)
  • However, in slow network conditions (VPN, poor connectivity), 10 seconds might be too aggressive
  • The JS SDK uses a single 30-second timeout covering the entire request lifecycle

Recommendation: The current values are fine for the common case. If timeout issues arise, consider making these configurable via environment variables (e.g., VIZZLY_OPEN_TIMEOUT, VIZZLY_READ_TIMEOUT).

2. Error Handling Gap

The current error handling at clients/ruby/lib/vizzly.rb:129 catches StandardError, which will catch timeout exceptions. However, the error messages don't specifically mention timeouts.

Suggestion: Add a specific check for timeout errors to provide clearer guidance to help users diagnose timeout vs. connection refused vs. 404 issues.

3. Test Coverage

The PR description mentions "Ruby SDK unit tests pass," but looking at clients/ruby/test/vizzly_test.rb, there don't appear to be tests that verify timeout behavior or HTTP error handling.

Recommendation: Consider adding a test that verifies the timeout parameters are set correctly. While testing actual timeout behavior can be flaky, you could at least verify the configuration with mocking or an intentionally slow test server. Not critical for this PR, but would improve robustness.

4. Documentation

Consider updating the Ruby SDK README (if it exists) to document the timeout behavior and any relevant environment variables.

🐛 No Bugs Found

The implementation is correct. Net::HTTP's open_timeout specifically handles the TCP connection phase, which is exactly what's needed here.

🔒 Security

No security concerns. Timeouts generally improve security by preventing resource exhaustion from hanging connections.

⚡ Performance

Improvement: This change actually improves performance by failing fast (10s) instead of hanging for 60+ seconds on unreachable servers.

Summary

Approve with minor suggestions. The core change is solid and addresses a real problem. The suggestions above are enhancements for future consideration, not blockers for this PR.

Key metrics:

  • Risk: Low (single-line, well-understood change)
  • Impact: Positive (prevents CI hangs)
  • Test coverage: Adequate (existing tests pass, though timeout-specific tests would be nice-to-have)

Separate handling for Net::OpenTimeout and Net::ReadTimeout
with actionable guidance for each scenario.
@vizzly-testing

This comment has been minimized.

1 similar comment
@vizzly-testing

This comment has been minimized.

@vizzly-testing

This comment has been minimized.

- Delete integration_test.rb (used useless 1x1 red pixels)
- Update sdk-e2e.yml to run e2e_test.rb with Selenium + Chrome
- Add selenium-webdriver to Gemfile
- Remove fake integration tests from sdk-unit.yml

The e2e tests capture real screenshots from the test-site using
Selenium WebDriver, which actually validates the visual testing flow.
@vizzly-testing

This comment has been minimized.

@vizzly-testing

This comment has been minimized.

- Remove unused exception variables in timeout rescues
- Add rubocop:disable for metrics on screenshot method
- Add webrick gem (removed from stdlib in Ruby 3.0)
@vizzly-testing
Copy link

Vizzly - Visual Test Results

CLI Reporter - Waiting for build

No builds received yet for this pull request.

CLI TUI - 1 change needs review
Status Count
Changed 1
Auto-approved 4
Changes needing review (1)

vizzly-help · 1202×1430 · 556.8% diff

vizzly-help

[Review changes →](https://app.vizzly.dev/vizzly/cli-tui/builds/23adce53-5d38-4162-9252-e405de509d01)

fix/ruby-sdk-open-timeout · 3104be98

@vizzly-testing
Copy link

Vizzly - Visual Test Results

CLI Reporter - 14 changes need review
Status Count
Passed 5
Changed 14
Auto-approved 5
Changes needing review (14)

search-no-results · Firefox · 1920×1080 · 0.2% diff

search-no-results

filter-failed-only · Firefox · 1920×1080 · 1.1% diff

filter-failed-only

search-homepage · Firefox · 1920×1080 · 0.2% diff

search-homepage

fullscreen-viewer · Firefox · 1920×1080 · 5.8% diff

fullscreen-viewer

bulk-accept-dialog · Firefox · 1920×1080 · 0.5% diff

bulk-accept-dialog

viewer-zoomed-100 · Firefox · 1920×1080 · 0.7% diff

viewer-zoomed-100

...and 8 more in Vizzly.

[Review changes →](https://app.vizzly.dev/vizzly/cli-reporter/builds/8623b6f4-d924-42f7-9bf1-18961f7791b6)
CLI TUI - Waiting for build

No builds received yet for this pull request.


fix/ruby-sdk-open-timeout · 3104be98

1 similar comment
@vizzly-testing
Copy link

Vizzly - Visual Test Results

CLI Reporter - 14 changes need review
Status Count
Passed 5
Changed 14
Auto-approved 5
Changes needing review (14)

search-no-results · Firefox · 1920×1080 · 0.2% diff

search-no-results

filter-failed-only · Firefox · 1920×1080 · 1.1% diff

filter-failed-only

search-homepage · Firefox · 1920×1080 · 0.2% diff

search-homepage

fullscreen-viewer · Firefox · 1920×1080 · 5.8% diff

fullscreen-viewer

bulk-accept-dialog · Firefox · 1920×1080 · 0.5% diff

bulk-accept-dialog

viewer-zoomed-100 · Firefox · 1920×1080 · 0.7% diff

viewer-zoomed-100

...and 8 more in Vizzly.

[Review changes →](https://app.vizzly.dev/vizzly/cli-reporter/builds/8623b6f4-d924-42f7-9bf1-18961f7791b6)
CLI TUI - Waiting for build

No builds received yet for this pull request.


fix/ruby-sdk-open-timeout · 3104be98

@Robdel12 Robdel12 merged commit f484e71 into main Jan 25, 2026
30 of 33 checks passed
@Robdel12 Robdel12 deleted the fix/ruby-sdk-open-timeout branch January 25, 2026 22:16
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