Fix PDS uploadBlob request stream stalls - #5360

APP-2836

Reviewing PR #5360

• Read review guidance & PR diff
• Analyze blob/transactor.ts stream fan-out
• Check test placement/runner conventions
• Post review

View job run

Reviewing PR #5360

• Read .github/claude-review-prompt.md guidance
• Fetch PR diff
• Analyze transactor.ts + cloneStream semantics
• Verify the new test actually reproduces the stall without the fix
• Post review

View job run

Reviewing PR #5360

• Read .github/claude-review-prompt.md and the PR diff
• Analyze transactor.ts stream fan-out / MIME detection change
• Check the new test (runner choice, layout, tsconfig per testing skill)
• Verify changeset coverage
• Post review

View job run

Reviewing PR #5360

• Read .github/claude-review-prompt.md and the PR diff
• Analyze packages/pds/src/actor-store/blob/transactor.ts stream fan-out logic
• Verify the new test's runner/layout conventions (testing skill, vitest vs jest)
• Check changeset coverage
• Post review

View job run

Claude finished @blackmichael's task in 6m 4s —— View job

Review

• Read .github/claude-review-prompt.md, CLAUDE.md, STYLE_GUIDE.md, .agents/skills/testing
• Read the diff and traced the upload fan-out (cloneStream → 4 consumers → uploadBlob route)
• Checked changeset, test placement/runner conventions, existing coverage
• Posted review

The fix itself looks right. cloneStream (packages/common/src/streams.ts:21) is a plain src.pipe(new PassThrough()), so with four destinations the source is paused as soon as the slowest one exerts backpressure. Once fileTypeFromStream stops reading (it only needs the first ~4 KiB), that clone's buffer crosses its 16 KiB HWM and pauses the source; destroy()ing it means no 'drain' ever fires, so the source stays paused and putTemp/streamSize/sha256Stream hang forever. resume() keeps the clone draining, which lets the pipe's flow control unwind. Good catch, and the changeset (@atproto/pds patch, no public API change) is correct.

Two things worth doing before merge, neither blocking on the fix's correctness:

1. The abort path wasn't exercised — please get tests/file-uploads.test.ts green. (non-blocking, but the failure mode is high impact — flagging with my uncertainty stated)

The PR description says only tests/blob-transactor.test.ts was run, and file-uploads.test.ts needs docker infra. That file's first case — packages/pds/tests/file-uploads.test.ts:50 'handles client abort' — is precisely the one where destroy() and resume() differ, and it already carries a comment saying it exists to catch uncaught exceptions:

// This test would fail from an uncaught exception: this grace period gives time for that to surface

The behavioral difference: with destroy(), the MIME clone was unpiped from the request stream as soon as sniffing finished; with resume() it stays piped for the whole upload. forwardStreamErrors (packages/common/src/streams.ts:12) forwards a source error onto that clone with pt.emit('error', err), and the only 'error' listener left on it is pipe()'s internal one, which unpipes, removes itself, and then errorOrDestroy(dest, err) → re-emits on a listener-less stream. I'm not confident this is a new exposure (the old code was reachable the same way once the clone was detached but once('error') was still armed on the source), and the timing window differs rather than the exposure existing only post-change — which is exactly why running the existing abort test on a real request stream is the cheap way to settle it. An uncaught 'error' here is a PDS process crash, so it's worth the CI cycle rather than reasoning.

2. it(..., 5000) on a 25 MiB payload is a CI flake risk, and 25 MiB is far more than the bug needs. (non-blocking) — packages/pds/tests/blob-transactor.test.ts:11,55

The package default is testTimeout: 60000 (jest.config.cjs), overridden here to 5 s. I understand why — the regression manifests as a hang, so you want to fail fast rather than sit for a minute. But the work under that 5 s cap is 1600 chunks pushed through four PassThroughs plus two SHA-256 passes over 25 MiB (one in the transactor, one to build the expected CID on line 52), and the pds suite runs 32 files across parallel workers alongside postgres/redis in docker. That margin is thinner than it looks on a loaded runner, and a timeout-based failure will read as "the stall is back" rather than "the box was busy".

The stall only needs the source to reach backpressure before file-type finishes its ~4 KiB peek — i.e. a few 16 KiB chunks. Something like 1 MiB reproduces just as deterministically, runs ~25× faster, and makes the 5 s bound unambiguous. Fix this →

While there: a one-line comment saying the regression surfaces as a timeout would save the next person real time — a bare timeout on a test named "drains the MIME stream without stalling other consumers" is decipherable but not obvious.

Smaller notes, take or leave:

• packages/pds/src/actor-store/blob/transactor.ts:352 — the @NOTE says "a Node pipe cleanup bug" without naming the mechanism, and this is a line someone will be tempted to "clean up" back to destroy(). One more clause (destroying a backpressured pipe destination means the source never receives 'drain') or a link to the upstream issue would make it defensible against a future drive-by.
• The fix depends on file-type/strtok3 leaving the clone alive and piped after the peek — if a transitive update under file-type: ^16.5.4 ever destroys or unpipes it, resume() becomes a no-op and the stall silently returns. Worth noting that the new test is what guards this, which is a good argument for keeping it.
• Optional: this is a pure unit test (mocked BlobStore, no db, no dev-env), and the testing skill puts those next to their subject — src/actor-store/blob/transactor.test.ts, which tsconfig.test.json already includes via ./src/**/*.test.ts. The same skill also says jest packages keep everything in ./tests, so tests/ is consistent with its neighbours. Either is defensible; not worth churn.

Not flagged after checking: bare describe/it/expect without @jest/globals is explicitly allowed (STYLE_GUIDE.md — "The only exception is jest's ambient test globals"), and 23 of the 32 existing pds test files do the same. No lexicon, migration, or auth surface touched.
· branch app-2836-fix-pds-uploadblob-request-stream-stall