Repository navigation
Conversation
Coverage Report for CI Build 37953862525Coverage increased (+0.03%) to 85.288%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline finding on the unvalidated TUS_BODY_IDLE_TIMEOUT_MS parse, I also checked whether a slow backend write after the request body is fully received could trigger a false 408, and whether the socket.setTimeout(0) cleanup could clobber Node's own keep-alive idle timer on persistent connections — both were examined and ruled out as not introducing separate bugs.
Extended reasoning...
The diff adds a socket-level idle timeout for TUS uploads (config.ts, http/routes/tus/index.ts) plus new tests; it touches request-handling/socket lifecycle code (a security/availability-relevant surface, since it destroys sockets and sets Fastify error state) but not auth or data exposure. A confirmed finding (unvalidated parseInt of the new env var silently disables the protection on a typo) is being posted inline, and two related socket-timeout-interaction scenarios were separately investigated and ruled out, so this is reported as defer rather than a second full review body.
ecf942f to
323384a
Compare
14a225b to
0b9155c
Compare
|
@claude review always |
There was a problem hiding this comment.
Beyond the inline findings, I checked two other scenarios and ruled them out: a throw from tusServer.handle() before the body completes can't leak the idle timer, since disarm() runs in a finally block; and a PATCH/POST queued behind another upload's postgres lock can't be killed by the idle timer while waiting, since the timer is disarmed on the request's end event (which fires once the body is fully received, before lock acquisition is awaited).
Extended reasoning...
The diff adds a TUS body idle-timeout feature (config.ts, http/routes/tus/index.ts) plus matching unit/integration tests; no auth/crypto surface is touched, but it does touch connection-lifecycle and DB-lock-adjacent code. Two confirmed bugs are already queued as inline comments (false-positive 408 under slow storage-backend writes, and an unbounded env value causing near-instant timeouts), so this run does not approve. My prior review's NaN-parsing concern appears addressed in later commits (envNumber now falls back to the default on NaN instead of silently disabling the timer).
Findings marked 🟡 are optional suggestions and need no follow-up push.
6972517 to
205c4c6
Compare
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline findings, I also checked whether operators can use TUS_BODY_IDLE_TIMEOUT_MS=0 to disable the idle timeout (since handleTusRequestWithIdleTimeout treats tusBodyIdleTimeoutMs <= 0 as "disabled") — envPositiveInteger in config.ts falls back to the 60s default for any value <= 0, so 0 is silently unreachable from env config rather than a usable opt-out; not flagging as a separate bug since it only removes a disable option, it doesn't cause unsafe behavior.
Extended reasoning...
The PR adds an idle-timeout watchdog for TUS uploads (config.ts, src/http/routes/tus/index.ts) that polls socket.bytesRead to detect stalled clients and destroy the connection with a 408. A CONFIRMED finding is being posted inline: checkIdle() reschedules whenever req.raw.readableLength > 0, so a permanently-stalled destination write (not a stalled client) can keep the buffer non-empty forever and the timeout never fires, which undermines the PR's stated purpose of always releasing the DB lock. Beyond that, I checked the env-var disable path (0 to disable) and found it unreachable due to envPositiveInteger's fallback, but judged that a minor gap rather than a new bug worth separate inline comment.
| // since Node stops reading the socket until the backlog drains. | ||
| // A non-zero readableLength means there's still unconsumed data sitting in the | ||
| // buffer, so the client did send something | ||
| if (req.raw.readableLength > 0) { |
There was a problem hiding this comment.
I think we need backpressure buffer move tracking here.
For example
- client sends fast, backend is slow
- client stops, then backend clears the buffer
- timer is checked right away and breaks the flow before socket reads continues
There was a problem hiding this comment.
I think the latest version covers this now, but let me know if you still see any gaps.
c06e979 to
d008c6c
Compare
There was a problem hiding this comment.
Beyond the inline nit, I checked the three substantive issues flagged in earlier rounds against the current code and they're fixed: tusBodyIdleTimeoutMs now goes through envBoundedPositiveInteger (regex-rejects non-numeric input instead of yielding NaN, and clamps to MAX_TIMER_DELAY_MS so it can't collapse to a near-instant timer), and checkIdle() now tracks both socket.bytesRead and a derived bytesConsumed so a slow-but-progressing destination write (S3/disk backpressure) no longer trips a false 408, while a destination write that's truly stuck still times out once bytesRead plateaus.
Extended reasoning...
The diff (db42280..HEAD) is the TUS idle-timeout feature across src/config.ts and src/http/routes/tus/index.ts plus tests; it touches raw-socket/stream handling and a resource-exhaustion-prevention mechanism (DB lock release) but no auth/crypto surface. Five prior rounds of review (mine and ferhatelmas's) flagged NaN-silent-disable, missing upper bound, and two backpressure false-positive/false-negative bugs in checkIdle's liveness logic; reading the current code confirms envBoundedPositiveInteger and the dual bytesRead/bytesConsumed check now address all of those. One new minor finding (the <= 0 disable path is unreachable given the regex) is already queued as an inline comment, so I'm not duplicating it here; given the history of real bugs found and fixed in this area, a human look is still warranted rather than an outright approval.
This pull request has been reviewed before and this review found new issues. Where they share a root cause, one fix may close them together.
d008c6c to
0018ee5
Compare
There was a problem hiding this comment.
No new bugs found this run. Beyond the threads already on this PR, I checked whether req.raw.destroy(err) in checkIdle (src/http/routes/tus/index.ts) could crash the whole server process via an unhandled error event — IncomingMessage.prototype.destroy forwards to the underlying socket's destroy(), and Node's http server already attaches an error listener to every accepted socket, so this does not throw uncaught. Also confirmed the HEAD handler's direct tusServer.handle() call (not wrapped) is functionally fine, since HEAD requests carry no body and hasDeclaredBody would make the wrapper a no-op anyway.
Extended reasoning...
Final-state diff of a long-running PR adding a configurable idle timeout that destroys stalled TUS request sockets; touches request-body handling and config parsing, no auth/crypto surface. No bugs were reported this run; I independently re-verified the previously-contentious idle-detection logic and config bounds by reading the current code, and separately investigated a new question (process-crash risk from destroy(err)) which checked out safe due to Node's socket-level error handling. Given the PR's long review history and a ferhatelmas thread on index.ts:321 whose resolution status isn't clearly verifiable from available metadata, a human pass is still worthwhile before merge.
0018ee5 to
00d8be3
Compare
There was a problem hiding this comment.
I reviewed this PR again and found no new bugs. Since the last review, the previously-flagged issues all now have corresponding code fixes, and this is still a non-trivial, concurrency-sensitive change, so a human look is worthwhile as a final check.
What was examined this run:
handleTusRequestWithIdleTimeoutin src/http/routes/tus/index.ts — the liveness check now tracks bothbytesReadand a derivedbytesConsumed(bytesRead minus readableLength), so a stalled destination write with a full, undraining buffer is correctly detected as idle instead of looping forever (fixes the earlier "stuck consumer never times out" case), while a slow write with data still arriving on the socket is correctly treated as alive.- src/config.ts
tusBodyIdleTimeoutMsnow usesenvIntegerInRange(..., 0, MAX_TIMER_DELAY_MS): 0 is accepted as an explicit disable, values aboveMAX_TIMER_DELAY_MSfall back to the default instead of clamping to ~1ms, and invalid/non-numeric input falls back to the 60s default rather than silently disabling the timeout. - New tests in index.test.ts exercise the backpressure-drain-then-stall case, disconnect-without-
end, and the body-completes-during-the-same-tick race.
Extended reasoning...
The diff adds a ~70-line idle-timeout wrapper around TUS request handling (src/http/routes/tus/index.ts) plus a bounded env-config option (src/config.ts); it touches no auth/crypto surface but does touch resource-exhaustion/DoS-mitigation logic (releasing DB locks on stalled uploads) via timer- and stream-backpressure-sensitive code, which is inherently easy to get subtly wrong. All four issues raised across my prior reviews on this PR (NaN silently disabling the timeout, missing upper bound causing near-instant 408s, TUS_BODY_IDLE_TIMEOUT_MS=0 not actually disabling it, and the stuck-consumer/backpressure misclassification) are now addressed by the final commits (switch to envIntegerInRange with a 0..MAX_TIMER_DELAY_MS range, and bytesConsumed-based liveness tracking), with new tests covering the backpressure-drain scenario specifically. Given the complexity and the history of five iterative fixes to get the timer logic right, a human confirmation is still worthwhile even though this run found nothing new.
This branch has not been deployed
3 participants
What kind of change does this PR introduce?
Bug fix
What is the current behavior?
If an open TUS request stops sending bytes, but never sends a FIN or RST (the TCP signals that close a connection) the server keeps the upload's database lock open forever. This can slow down the database and block other uploads.
This can happen if the client's network dies or the app gets backgrounded.
What is the new behavior?
Close the connection if no bytes are received for 60 seconds (configurable). This releases the lock automatically, even if the client never sends a FIN/RST and never comes back.