fix: use timeout-bounded HTTP client for unary Read and Append

[infiniteregrets]

closes #234

Summary

• Unary Read and Append were using getHTTPClient() which returns the streaming HTTP client (Timeout: 0), ignoring the configured RequestTimeout.
• Changed both to use s.basinClient.httpClient (the timeout-bounded client), consistent with CheckTail and other unary operations.
• Streaming sessions (AppendSession, ReadSession) continue to use the no-timeout streaming client via getHTTPClient().

Test plan

• Verify go build ./... passes
• Confirm unary Read/Append now respect RequestTimeout under network delays

🤖 Generated with Claude Code

[greptile-apps]

Greptile Summary

This PR fixes unary Read and Append on StreamClient to use the timeout-bounded HTTP client (basinClient.httpClient, default 5s) instead of the no-timeout streaming client (getHTTPClient() → streamingClient), making them consistent with CheckTail and other unary operations. The fix is correct and complete for Append, but introduces a subtle behavioral regression for unary Read calls that use the Wait long-poll option.

Key changes:

• s2/append.go: s.getHTTPClient() → s.basinClient.httpClient — straightforward and correct, Append has no wait semantics.
• s2/read.go: Same substitution — correct for standard reads, but the ReadOptions.Wait field supports up to 60 seconds of server-side long polling. With the default RequestTimeout of 5 seconds, any call with opts.Wait > 5 will now be terminated by the HTTP client deadline before the server's wait period completes. This is a behaviour change from the pre-fix state (where reads never timed out).

Streaming sessions are unaffected: AppendSession (connectAndRead) and ReadSession (streamReader.runOnce) continue to call getHTTPClient() and use the no-timeout streaming client, which is correct.

Confidence Score: 4/5

Safe to merge for standard unary use; however, existing callers using Read with a Wait value larger than RequestTimeout will silently time out after the fix.

The Append fix is clean and correct. The Read fix has a P1 edge case: the ReadOptions.Wait long-poll parameter allows up to 60 seconds, but the newly applied requestTimeout (default 5s) will terminate those requests prematurely — a behaviour regression for any caller currently relying on long-poll reads.

s2/read.go — the interaction between opts.Wait and basinClient.httpClient.Timeout needs to be addressed before merging if long-poll Read is a supported use-case.

Important Files Changed

Filename	Overview
s2/append.go	Switches unary Append from getHTTPClient() (streaming client, no timeout) to s.basinClient.httpClient (timeout-bounded client); correct and consistent with CheckTail.
s2/read.go	Switches unary Read to the timeout-bounded httpClient, which is correct for standard reads but can prematurely cut off long-poll reads where opts.Wait > RequestTimeout (default 5s).

Sequence Diagram

sequenceDiagram
    participant Caller
    participant StreamClient
    participant httpClient (timeout-bounded)
    participant streamingClient (no timeout)
    participant S2 API

    Note over httpClient (timeout-bounded): Timeout = RequestTimeout (default 5s)
    Note over streamingClient (no timeout): Timeout = 0

    Caller->>StreamClient: Append(ctx, input)
    StreamClient->>httpClient (timeout-bounded): requestProto (POST /streams/{name}/records)
    httpClient (timeout-bounded)->>S2 API: HTTP POST
    S2 API-->>httpClient (timeout-bounded): AppendAck
    httpClient (timeout-bounded)-->>StreamClient: AppendAck
    StreamClient-->>Caller: *AppendAck

    Caller->>StreamClient: Read(ctx, opts)
    StreamClient->>httpClient (timeout-bounded): requestProto (GET /streams/{name}/records)
    Note right of httpClient (timeout-bounded): If opts.Wait > RequestTimeout, client deadline fires first
    httpClient (timeout-bounded)->>S2 API: HTTP GET
    S2 API-->>httpClient (timeout-bounded): ReadBatch
    httpClient (timeout-bounded)-->>StreamClient: ReadBatch
    StreamClient-->>Caller: *ReadBatch

    Caller->>StreamClient: AppendSession(ctx)
    StreamClient->>streamingClient (no timeout): POST /streams/{name}/records (streaming)
    streamingClient (no timeout)->>S2 API: HTTP/2 bidirectional stream
    S2 API-->>streamingClient (no timeout): AppendAck (stream)

    Caller->>StreamClient: ReadSession(ctx, opts)
    StreamClient->>streamingClient (no timeout): GET /streams/{name}/records (streaming)
    streamingClient (no timeout)->>S2 API: HTTP/2 SSE/S2S stream
    S2 API-->>streamingClient (no timeout): ReadBatch (stream)

Loading

Comments Outside Diff (1)

1. s2/read.go, line 98-114 (link)

   Wait parameter can exceed RequestTimeout, causing premature timeouts

   The unary Read method explicitly supports long-poll semantics via the Wait option (up to 60 seconds per the ReadOptions doc comment). After this fix, Read uses s.basinClient.httpClient which carries Timeout: requestTimeout (default: 5 seconds). This means any call with opts.Wait > requestTimeout will be killed by the HTTP client's deadline before the server's wait period expires.

   Before this PR, getHTTPClient() returned the streamingClient with Timeout: 0 (no timeout), so long-poll reads with Wait = 30 or Wait = 60 worked as expected. After this fix, those same calls will silently time out after 5 seconds (or whatever RequestTimeout is configured to).

   The fix is correct for Append (no wait semantics) and for Read calls without a Wait parameter. However, for Read with a non-zero Wait, the timeout-bounded client may be too aggressive.

   Consider adjusting the effective timeout dynamically when opts.Wait is set — for example, using the larger of requestTimeout and Wait + some buffer:

   effectiveClient := s.basinClient.httpClient
   if opts != nil && opts.Wait != nil && *opts.Wait > 0 {
       waitDuration := time.Duration(*opts.Wait) * time.Second
       if waitDuration+time.Second > s.basinClient.requestTimeout {
           effectiveClient = s.basinClient.client.streamingClient
       }
   }
   httpClient := &httpClient{
       client:      effectiveClient,
       // ...
   }

   Or, consider documenting that users should set RequestTimeout large enough to accommodate their Wait values when using long-poll reads.

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: s2/read.go
   Line: 98-114
   
   Comment:
   **`Wait` parameter can exceed `RequestTimeout`, causing premature timeouts**
   
   The unary `Read` method explicitly supports long-poll semantics via the `Wait` option (up to 60 seconds per the `ReadOptions` doc comment). After this fix, `Read` uses `s.basinClient.httpClient` which carries `Timeout: requestTimeout` (default: 5 seconds). This means any call with `opts.Wait > requestTimeout` will be killed by the HTTP client's deadline before the server's wait period expires.
   
   Before this PR, `getHTTPClient()` returned the `streamingClient` with `Timeout: 0` (no timeout), so long-poll reads with `Wait = 30` or `Wait = 60` worked as expected. After this fix, those same calls will silently time out after 5 seconds (or whatever `RequestTimeout` is configured to).
   
   The fix is correct for `Append` (no wait semantics) and for `Read` calls without a `Wait` parameter. However, for `Read` with a non-zero `Wait`, the timeout-bounded client may be too aggressive.
   
   Consider adjusting the effective timeout dynamically when `opts.Wait` is set — for example, using the larger of `requestTimeout` and `Wait + some buffer`:
   
   ```go
   effectiveClient := s.basinClient.httpClient
   if opts != nil && opts.Wait != nil && *opts.Wait > 0 {
       waitDuration := time.Duration(*opts.Wait) * time.Second
       if waitDuration+time.Second > s.basinClient.requestTimeout {
           effectiveClient = s.basinClient.client.streamingClient
       }
   }
   httpClient := &httpClient{
       client:      effectiveClient,
       // ...
   }
   ```
   
   Or, consider documenting that users should set `RequestTimeout` large enough to accommodate their `Wait` values when using long-poll reads.
   
   How can I resolve this? If you propose a fix, please make it concise.

Prompt To Fix All With AI

This is a comment left during a code review.
Path: s2/read.go
Line: 98-114

Comment:
**`Wait` parameter can exceed `RequestTimeout`, causing premature timeouts**

The unary `Read` method explicitly supports long-poll semantics via the `Wait` option (up to 60 seconds per the `ReadOptions` doc comment). After this fix, `Read` uses `s.basinClient.httpClient` which carries `Timeout: requestTimeout` (default: 5 seconds). This means any call with `opts.Wait > requestTimeout` will be killed by the HTTP client's deadline before the server's wait period expires.

Before this PR, `getHTTPClient()` returned the `streamingClient` with `Timeout: 0` (no timeout), so long-poll reads with `Wait = 30` or `Wait = 60` worked as expected. After this fix, those same calls will silently time out after 5 seconds (or whatever `RequestTimeout` is configured to).

The fix is correct for `Append` (no wait semantics) and for `Read` calls without a `Wait` parameter. However, for `Read` with a non-zero `Wait`, the timeout-bounded client may be too aggressive.

Consider adjusting the effective timeout dynamically when `opts.Wait` is set — for example, using the larger of `requestTimeout` and `Wait + some buffer`:

```go
effectiveClient := s.basinClient.httpClient
if opts != nil && opts.Wait != nil && *opts.Wait > 0 {
    waitDuration := time.Duration(*opts.Wait) * time.Second
    if waitDuration+time.Second > s.basinClient.requestTimeout {
        effectiveClient = s.basinClient.client.streamingClient
    }
}
httpClient := &httpClient{
    client:      effectiveClient,
    // ...
}
```

Or, consider documenting that users should set `RequestTimeout` large enough to accommodate their `Wait` values when using long-poll reads.

How can I resolve this? If you propose a fix, please make it concise.

_{Reviews (1): Last reviewed commit: "fix: use timeout-bounded HTTP client for..." | Re-trigger Greptile}