fix: Warn when a requested timeout is capped at timeout_max - #962
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #962 +/- ##
==========================================
+ Coverage 94.64% 94.86% +0.22%
==========================================
Files 58 58
Lines 5263 5357 +94
==========================================
+ Hits 4981 5082 +101
+ Misses 282 275 -7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Pijukatel
left a comment
There was a problem hiding this comment.
Can you please verify in the API code that a timeout larger than 360 s has any meaning?
If there is some hard-coded or implementation limit on the API side, it makes no sense to lift the limit in the client.
(But a warning might be good in those cases)
|
Probably, there are mostly idle timeouts, not wall-clock timeouts, so this could make sense. See:
I don't want to waste much time here. IMO, silently truncating it is definitely wrong. If we don't want to allow longer timeouts, we should at least log a warning. However, allowing longer timeouts wouldn't be harmful anyway. I'll wait for your response, but I don't have a strong preference here. |
### Issue - Closes #1029 - This is a port of the following: - apify/apify-client-python#653 - apify/apify-client-python#664 - apify/apify-client-python#962 ### Description The client already chose a timeout per method internally, but the only public knob was the global `timeoutSecs`, so a call that legitimately takes longer had no way to ask for more time. The tiers are now public, following the Python client, and every method that sends a request takes a `timeoutSecs` option. | Tier | Default | Covers | | --- | --- | --- | | `short` | 5 s | metadata reads and writes | | `medium` | 30 s | listing, batch and trigger calls | | `long` | 360 s | downloads, uploads and streaming | | `noTimeout` | none | polling that waits for a job to finish | Every method keeps the tier its Python counterpart has. `timeoutMaxSecs` caps a single attempt and bounds the growth across retries, which doubles from the requested value (T, 2T, 4T, 8T). The six `waitForFinish` methods get the API's one-minute hold on top of their tier, so the client doesn't abort a request the API is still holding and start a second run on retry. `timeoutSecs` takes a tier name, a number of seconds, or `'noTimeout'`. The unit lives in the option name rather than in the value, the way `waitSecs` and `timeoutMaxSecs` already do. The last commit also drops the leading underscore from non-public members, as agreed in review. Where the underscore was covering a collision with a public member, the helper got a new name: `_url` to `buildUrl`, `_params` to `buildParams`, `_get` to `getResource`, `_list` to `listResources`, `_waitForFinish` to `waitForJobFinish`, `_batchAddRequests` to `addRequestBatch`, and `ApifyClient._options` to `subClientOptions`. `src/apify_api_error.ts` and `src/statistics.ts` are left for #1057. ## Testing `test/client_timeouts.test.ts` covers the tier defaults, per-call overrides, the `waitForFinish` hold, `runTimeoutSecs` and the `requestQueue(id, { timeoutSecs })` cap. `test/http_client.test.ts` covers the cap warning and the retry growth. `test/apify_api_error.test.ts` guards `clientMethod`, which reads the renamed helpers off the stack. ## Breaking changes - `timeoutSecs` on the constructor is replaced by `timeoutShortSecs`, `timeoutMediumSecs`, `timeoutLongSecs` and `timeoutMaxSecs`. - Outside the storage clients, methods used the global 360 s and now use `short` or `medium`, so a call that waited out a slow API can fail sooner. - The run timeout of `ActorClient.start()` / `call()`, `TaskClient.start()` / `call()` and `RunClient.resurrect()` is now `runTimeoutSecs`. TypeScript reports a leftover `timeout` at compile time, and the option schemas are strict, so JavaScript gets an `ArgumentValidationError` instead of a silently dropped run limit. - `client.httpClient.call()` takes `timeoutSecs` in seconds, where the axios field it replaces took milliseconds. A direct call that passed `timeout: 30000` becomes `timeoutSecs: 30`. - Methods without options gained an options parameter, and methods that take a payload gained a second one. - The `protected` helpers of `ApiClient`, `ResourceClient` and `ResourceCollectionClient` are renamed, which reaches anyone subclassing them. `KeyValueStoreClient.setRecord()` keeps its `timeoutSecs`, and a plain number still means what it did in v2, so Crawlee's `setValue()` needs no paired PR. The option now takes a tier name or `'noTimeout'` as well. The same goes for `client.requestQueue(id, { timeoutSecs })`, which caps the tier of every request that queue client sends. *✍️ Drafted by Claude Code*
An explicit per-call
timeouttimedelta larger thantimeout_max(default 360 s), or a tier configured above it, is clamped totimeout_maxon every attempt. For example,dataset.get_items_as_bytes(timeout=timedelta(minutes=30))runs every attempt with a 360 s timeout and surfaces as repeatedimpit.TimeoutException, with no hint that the requested 30 minutes never took effect.The behavior stays unchanged:
timeout_maxremains a hard ceiling for any single request attempt. Only visibility changes._compute_timeoutnow logs a warning when the resolved base timeout exceedstimeout_max, naming both values and pointing attimeout_maxas the knob to raise.The warning goes through
LoggerOnce, a small dedup helper ported from Crawlee for Python, so a recurring cut-off is reported once per timeout kind rather than on every attempt of every call.Docs and docstrings now state that the cap applies to tier and per-call timeouts alike.
✍️ Drafted by Claude Code