jiengup opened a new pull request, #4345:
URL: https://github.com/apache/iggy/pull/4345
Closes #4262
Relates to #3419
## Rationale
A TCP connection can stay open while the server never replies to login. The
caller then has no bound on the complete login operation, particularly when it
waits for a lock, reconnects, or follows a leader redirect. This is the
client-side failure reported in #4262. Released clients will still need a
server-side response or disconnect when an older, incompatible login frame
arrives.
This is an incremental implementation of #3419's timeout proposal. It
introduces a place for each transport and request category to choose a deadline
and define what expiration does. The configured deadline currently applies only
to TCP username/password login, TCP personal-access-token login, and
`connect()` when configured or remembered credentials require sign-in.
Extending it to other request types and transports is follow-up work.
## What changed?
TCP now has a nonzero `request_timeout` setting (30 seconds by default),
exposed through the TCP config builder, connection string, and shared client
arguments. A task-local `RequestBudget` gives nested login, reconnect, and
redirect calls one deadline; a transport-neutral `ClientRequestPolicy` supplies
the timeout and expiration action without making the high-level client own TCP
connection state. On expiry, TCP marks the connection invalid and reconciles
its stream and VSR session before reconnecting.
### Deadline coverage
- For `IggyClient` login and credentialed `connect()`, the deadline starts
before acquiring the high-level client read lock. It spans the awaited login or
PAT login, logout-before-relogin, leader metadata lookup and redirection,
recursive sign-in, and the connect coordinator.
- During TCP connection setup, it spans waits for another connect owner,
reconnect and `reestablish_after` sleeps, endpoint selection, TCP dial,
optional TLS handshake, automatic or remembered sign-in, and leader settlement.
All attempts share the caller's deadline.
- During a budgeted TCP request, it spans the routing and stream locks,
socket write and flush, response-header and response-body reads,
transient-response retry sleeps, and any permitted reconnect. **The spawned
send task captures the deadline explicitly**; header and body reads share it
rather than restarting a timer.
The configured `request_timeout` does **not** yet apply to ordinary TCP
requests or to a `connect()` that has no sign-in credentials. Ordinary TCP
requests retain the existing response-read limit, which does not cover the
entire operation. QUIC, WebSocket, and HTTP do not yet supply a request policy.
**The deadline is observed by async polling, so a blocked executor or long
synchronous section can delay it**; arbitrary spawned tasks do not inherit the
task-local budget. A caller-supplied outer budget takes precedence over nested
configured budgets. **A send worker that cannot make progress may also delay
deferred stream cleanup and the next reconnect, even though the caller has
received the timeout error**.
**Replacing the underlying client is another uncovered case**.
`IggyClient::client()` exposes a shared lock, and a caller can use its write
guard to replace an HTTP `ClientWrapper` with a TCP one. `IggyClient` caches
the timeout policy only when it is constructed, so the cached policy remains
`None` after this replacement. A subsequent `login_user()` waits for the shared
lock's read guard without starting the new TCP client's deadline; the TCP login
code cannot enforce its timeout until it acquires that guard. In the regression
test, a replacement TCP client configured for 50 ms still produced no login
timeout after an outer 500 ms wait.
### Behavior after timeout and proposed extension
For the supported TCP login and credentialed-connect paths, the caller
receives `RequestTimeoutOutcomeUnknown`. That error deliberately does not claim
the server did nothing. Expiration sets a persistent invalidation flag and
tries to drop the socket and mark the client disconnected without waiting for
locks held by an in-flight send. **If cleanup cannot acquire a lock
immediately, the send worker or the next `connect()` discards the old stream**;
reconnection resets the VSR session before a new request uses it. The expired
login is not replayed across a new session after its deadline.
For future coverage of all TCP requests, the same full-operation deadline
and invalidation path could be reused, **but an operation whose outcome is
unknown must not be automatically replayed under a new VSR session**. For QUIC,
WebSocket, and HTTP, **each transport needs its own expiration policy** before
the same caller-facing timeout contract is enabled; the right cleanup scope may
differ for multiplexed connections and independent HTTP requests.
### Follow-up API and design questions
- This PR provides only a client-level timeout. #3419 also proposes a
per-request override such as `client.login(...).with_timeout(10ms)`. The
current async trait methods return futures directly, so fluent overrides **need
an API change or a new request-builder/operation-specific entry point**. A
prototype is deliberately excluded from this PR.
- Is a task-local budget plus transport-specific policy the right seam? Is
there a simpler end-to-end way to include every awaited call without adding
timeout wrappers at multiple layers?
- Should expiry be a hard caller deadline that cancels the awaited operation
and invalidates the connection, as implemented here, or a soft deadline that
reports slowness while allowing an in-flight request to finish? The spawned TCP
send worker is not forcibly aborted; it retains the stream until it can clean
up safely. How should either choice report an operation that may have committed?
## Local Execution
- **Unit and local checks:** `cargo fmt --all`, `cargo sort --no-format
--workspace`, `cargo clippy --all-features --all-targets -- -D warnings`,
`cargo test -p iggy_common -p iggy`, `./scripts/ci/taplo.sh --check`, and
commit-time hooks passed on the implementation before the latest rebase. The
timeout tests cover silent peers, partial replies, stalled writes, stream and
connect-owner lock waits, PAT and auto login, retry bounds, and reconnect after
expiration. The timeout implementation is unchanged by the rebase, but the
exact rebased `HEAD` has not had this full sequence or a full `prek run` rerun.
- **Review:** `team-review-lite` was run, and the implementation was revised
to address its findings.
- **End-to-end:** Tested against a real `iggy-server:0.9.0`. In the no-reply
case, the server was paused after the TCP connection was established.
| Scenario | Client | Observed result |
| --- | --- | --- |
| Server paused after TCP connect | Rust SDK from an earlier
`upstream/master` snapshot | Login did not finish within 35 s. |
| Server paused after TCP connect | Rust SDK from the tested branch |
Returned `RequestTimeoutOutcomeUnknown` in about 30.01 s. |
| Server resumed after timeout | Same Rust client from the tested branch |
Reconnected and logged in successfully. |
| Server paused after TCP connect | Published Python SDK 0.9.0 | Login did
not finish within 35 s. |
| Server paused after TCP connect | Python SDK built from the tested
branch | Returned a timeout error in about 30 s. |
| Healthy server | Published Python SDK 0.9.0 | Logged in successfully. |
- **CI:** No GitHub Actions result was available when this message was
drafted. The repository's pre-merge CI must still be run and pass on the
submitted commit.
## AI Usage
- **Tool and scope:** OpenAI Codex assisted with the timeout implementation,
refactoring, tests, validation analysis.
- **Verification:** The local checks and end-to-end observations are listed
above; the author should review them against the final submitted commit.
- **Author confirmation before posting:** I have reviewed the complete diff
and can explain every change and its timeout semantics.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
