fix: use inactivity timeout for watch instead of whole-request timeout - #3022
Conversation
kubernetes-prow
Bot
added
the
do-not-merge/work-in-progress
|
Welcome @duizabojul! |
kubernetes-prow
Bot
added
the
size/L
|
|
kubernetes-prow
Bot
added
the
cncf-cla: no
2fdfe13 to
b8a873e
Compare
Watch.watch() passed AbortSignal.timeout(requestTimeoutMs) to fetch, which counts from request start and stays armed while the response body streams. Any watch, healthy or not, was therefore aborted with a TimeoutError after 30s. Replace it with a resettable timer on the AbortController: it is armed before the fetch (connect timeout), then re-armed on the 200 response and on every body chunk, so requestTimeoutMs now means "max time without data". The timer is unref'd and cleared when the watch is done. No public API change. Also reset ListWatch's reconnectDelayMs to 0 in the TimeoutError branch of doneHandler. A client-side timeout means we already waited the full request timeout, so reconnecting immediately cannot tight-loop, while backing off would leave quiet resources unwatched for up to MAX_RECONNECT_DELAY_MS. Exponential backoff still applies to real errors and server-side disconnects.
b8a873e to
6086604
Compare
|
/easycla |
kubernetes-prow
Bot
added
cncf-cla: yes
kubernetes-prow
Bot
removed
the
do-not-merge/work-in-progress
|
this looks good to me. this exposed a couple other issues to address with watches like lack of jitter on the reconnects which i'll handle in a separate PR. |
kubernetes-prow
Bot
added
the
lgtm
|
Worth also looking at what client-go does here: a randomized timeoutSeconds sent to the server so it ends the watch on its own schedule instead of the client guessing, and watch bookmarks to keep the resourceVersion fresh across restarts. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: duizabojul, mstruebing The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
kubernetes-prow
Bot
added
the
approved
Node 26.8.0 rewrote fs.readFile (nodejs/node#65327) to do open, fstat, read and close in a single thread pool round trip, so binding.open is no longer called from JS. mock-fs recovers the ReadFileContext prototype by intercepting binding.open during a dummy readFile, so it now gets undefined and throws at require time: TypeError: Cannot read properties of undefined (reading 'read') at exports.patchReadFileContext (mock-fs/lib/readfilecontext.js:40:30) Because it throws on require rather than in a test, it takes out config_test.ts and file_auth_test.ts in full. The matrix entries are floating majors, so setup-node resolves '26' to whatever the newest 26.x is at run time. That is why main went red on the merge of kubernetes-client#3022 without any change to the code under test: the branch last ran CI on 26.7.0, and by the time it merged five days later the runner had picked up 26.8.1. Pinning to 26.7 restores a green build. It is a stopgap: mock-fs has had no functional release since February 2025 and the upstream report (tschaub/mock-fs#447) is unanswered, so the durable fix is to stop depending on it.
3 participants
Fixes #3020
What / why
WatchpassesAbortSignal.timeout(requestTimeoutMs)to the fetch that streams the watch body, so the signal fires 30s after the request starts regardless of connection health, and every watch — including one actively receiving events — is aborted with aTimeoutError.Per the discussion in #3020, this PR keeps timeout signaling but changes what it measures:
src/watch.ts— replaces the whole-requestAbortSignal.timeoutwith a resettable inactivity timer. The timer is armed before the fetch (connect timeout, same as today), then re-armed on the 200 response and on every body data chunk. A watch that keeps receiving data is left alone; a connection that goes silent forrequestTimeoutMs(or never connects) is still aborted with the sameTimeoutErroras before. The timer isunref()ed to matchAbortSignal.timeout's event-loop behavior, and is cleared once the watch finishes. No public API change;requestTimeoutMskeeps its name and 30s default, its meaning becomes "max time with no data received".src/cache.ts—ListWatchnow resetsreconnectDelayMsin the existingTimeoutErrorbranch ofdoneHandler. A client-side timeout means the client already waited the fullrequestTimeoutMs, so an immediate reconnect cannot tight-loop — the timeout itself is the throttle. Without this, a watch on a quiet resource (which now times out everyrequestTimeoutMsby design) accumulates exponential backoff and ends up alternating ~30s watching / ~30s not watching. Backoff is unchanged for real errors and server-side disconnects.Tests
requestTimeoutMs, then clean server close →done(null))TimeoutErrorTimeoutErrorreconnects apply no delay, while a subsequent non-timeout reconnect still backs off (via the injectabledelayFn)Both new watch tests and the cache test fail against the previous implementation.
npm test(356/356),npm run lint,npm run buildall pass.