add a timeout layer to our gcp connector - #6793
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0c3e02e387
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| fn gcs_timeout_layer_with(io_timeout: Duration) -> TimeoutLayer { | ||
| TimeoutLayer::default() | ||
| .with_timeout(GCS_CONTROL_TIMEOUT) | ||
| .with_io_timeout(io_timeout) |
There was a problem hiding this comment.
When a GCS upload needs more than five seconds to write or finalize a chunk—even while continuously making progress—OpenDAL's I/O timeout expires the writer operation. This layer is attached to the same operator used by put, whose default multipart policy can buffer very large chunks before close, so normal large-split uploads over slower links will repeatedly fail and prevent indexing from publishing. This is not equivalent to stalled-stream detection; scope the timeout to reads or give writes a throughput-aware upload budget.
Useful? React with 👍 / 👎.
| // we use a different connect and stalled stream timeout in our s3 connector) | ||
| fn gcs_timeout_layer_with(io_timeout: Duration) -> TimeoutLayer { | ||
| TimeoutLayer::default() | ||
| .with_timeout(GCS_CONTROL_TIMEOUT) |
There was a problem hiding this comment.
When the peer accepts the TCP connection but never returns HTTP headers—the exact behavior of the newly added stalling server—the GCS accessor request is governed by this 10-second control timeout rather than the reader I/O timeout. Because the retry layer wraps this layer, each retry gets another 10-second budget, so the configured four attempts can exceed the 30-second leaf-search deadline and the new test's five-second outer deadline panics before even the first attempt finishes. The control timeout and retry count need a combined budget below the caller deadline, and the test helper must make the control timeout configurable if it is intended to exercise this case quickly.
Useful? React with 👍 / 👎.
| async fn test_gcs_storage_get_slice_errors_on_stalled_connection() -> anyhow::Result<()> { | ||
| let (endpoint, server_task) = start_stalling_gcs_server().await?; | ||
|
|
||
| let reqwest_client = reqwest_013::Client::builder().no_proxy().build()?; |
There was a problem hiding this comment.
When this test runs under nextest, it executes in a fresh process and constructs a reqwest client without first selecting rustls's process-wide crypto provider. The neighboring HTTPS test explicitly installs aws_lc_rs::default_provider() for this reason; using an HTTP endpoint does not avoid TLS backend initialization during Client::builder().build(). Consequently, this test can fail while building the client rather than exercising the timeout, so install the provider here or move that setup into a shared helper.
Useful? React with 👍 / 👎.
1 participant
currently on the query path, the timeout hitting is the leaf search one, at 30s, which causes a bunch of work to be loss because of one flaky connection