Skip to content

fix(main): stream hygiene for dweb fetch error paths - #182

Open
meinharrd wants to merge 2 commits into
mainfrom
fix/http-fetch-stream-hygiene
Open

fix(main): stream hygiene for dweb fetch error paths#182
meinharrd wants to merge 2 commits into
mainfrom
fix/http-fetch-stream-hygiene

Conversation

@meinharrd

Copy link
Copy Markdown
Contributor

Follow-up to #158, addressing the two non-blocking hygiene notes from its final alan review round:

  1. Abandoned body stream on error paths — the response body is now cancelled on a non-OK response, and the reader is cancelled when a chunk callback throws (e.g. ENOSPC mid-save). Previously the protocol handler would keep pulling the rest of the transfer from the gateway in the background until GC collected the locked stream.
  2. Unchecked partial writesFileHandle.write may in theory write fewer bytes than requested without throwing; sessionFetchToFile now loops until the whole chunk is on disk instead of silently truncating the saved file.

Each behavior has a unit test (body-cancel on non-OK, reader-cancel on write failure, partial-write retry loop). http-fetch.test.js + ipc-handlers.test.js: 37/37 pass locally.

Follow-up to #158, addressing the two non-blocking notes from its final
review round:

- Cancel the response body on a non-OK dweb response, and cancel the
  reader when a chunk callback throws (e.g. disk full mid-save), so the
  protocol handler stops pulling from the gateway instead of draining
  the rest of the transfer in the background until GC.
- Loop on FileHandle.write until the whole chunk is on disk; a partial
  write without an error would previously truncate the saved file
  silently.
@meinharrd meinharrd added the alan:reviewing alan loop currently running on this PR label Aug 10, 2026
@meinharrd

Copy link
Copy Markdown
Contributor Author

[alan-review R1] No blocking findings. Verified locally on the PR branch: http-fetch.test.js + ipc-handlers.test.js 37/37 pass, eslint clean, and I confirmed the FileHandle.write(buffer, offset, length) retry-loop semantics against real Node (with 'w' and no explicit position, sequential writes advance the fd position, so resuming from the buffer offset writes the file correctly — reproduced abcdef via forced 2-byte partial writes). All three dweb error exits now cancel the stream: non-OK → body.cancel(), chunk-callback throw → reader.cancel(), timeout → the pre-existing onAbort cancel. No user-visible behavior change, so unit tests are the appropriate evidence here (no e2e/screenshot pass needed).

Two minor notes, neither blocking:

  1. src/main/http-fetch.js:121-124 — write-retry loop can spin forever if bytesWritten is ever 0. while (offset < chunk.length) assumes each iteration makes progress. On regular files a >0-byte write essentially never returns 0 without erroring (quota/ENOSPC surfaces as an error on the next call), so this is theoretical — but since the loop exists precisely to handle behavior that's already theoretical, a if (bytesWritten === 0) throw new Error('write made no progress') guard would turn a hypothetical silent hang into a loud failure. Failure scenario: an exotic filesystem (FUSE/network mount) returns a 0-byte partial write; the save-image IPC handler then hangs until the inactivity timeout aborts the transfer, rather than failing with a clear error.

  2. src/main/http-fetch.js:170-173, 219-222 (plus the redirect branches at 163/209) — the http(s) sibling paths still abandon the response on non-200 and on redirects. reject(...) / recursing into the redirect target never calls response.resume() or request.destroy(), so the socket lingers with unconsumed data until the inactivity-timeout handler destroys it (up to timeout ms, default 30s). This is the same hygiene class this PR fixes in the dweb path, though pre-existing, bounded by the timeout, and outside the PR's stated dweb scope — flagging per the sibling-sites lesson rather than asking for it in this PR. Failure scenario: a burst of failed http(s) image saves (404s or redirect chains) each pins a socket + buffered body for up to 30s; the dweb path no longer does.

@meinharrd meinharrd added alan:clean alan loop finished: no confirmed blocking findings and removed alan:reviewing alan loop currently running on this PR labels Aug 10, 2026
Per review: if FileHandle.write ever returns bytesWritten 0 (exotic
FUSE/network mounts), fail loudly instead of hanging in the retry loop
until the inactivity timeout aborts the transfer.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

alan:clean alan loop finished: no confirmed blocking findings

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant