Skip to content

benchmarks: propagate S3 GET stream errors through byte counter - #76

Open
adstep wants to merge 1 commit into
yjs:masterfrom
adstep:adstep-benchmark-harness-hang-fixes
Open

adstep wants to merge 1 commit into
yjs:masterfrom
adstep:adstep-benchmark-harness-hang-fixes

Conversation

@adstep

@adstep adstep commented Oct 14, 2026

Copy link
Copy Markdown

Problem

The benchmark wraps the MinIO client to count S3 bytes. On reads, countS3Ops returned stream.pipe(counting). .pipe() doesn't pass source errors on to the destination. If the S3 store reset a connection partway through a response body (ECONNRESET), the source stream died but the counting Transform was never ended or errored. yhub's for await in S3PersistenceV1.retrieve then waited forever and never reached its retry for transient errors.

Seen in Y2.5 (300 clients joining a 20 MB doc at 100 conn/s, local MinIO): 60-100 joins never received syncStep2, so the benchmark hung. All 600 GETs received headers but only ~498 bodies finished, and no error was reported. A standalone script without the wrapper saw the same resets surface as ordinary ECONNRESET errors.

Fix

Count bytes in an async generator that passes each chunk through, wrapped with Readable.from:

for await (const chunk of stream) {
  counters.getBytes += chunk.length
  yield chunk
}

A read error on the source now throws out of the generator and into the caller's for await, so yhub's existing retry runs. If the caller stops reading early, the source stream is destroyed and its socket released (it wasn't with .pipe()).

Testing

  • npx standard benchmarks/src/ and npx tsc --skipLibCheck pass.
  • Ran countS3Ops against a local HTTP server:
    • a full response is read completely and the byte count matches;
    • a mid-body connection reset throws ECONNRESET instead of hanging (it hung before this change);
    • when the reader stops early, the source stream is destroyed.
  • Y2.5 against local MinIO, with hub logging on (BENCH_DEBUG=1): all three rates completed with no hang. MinIO reset 185 GETs mid-body; each surfaced as ECONNRESET and hit yhub's retry. 139 retries also failed, which exactly matches the dropped-client count (0 / 27 / 112), so every drop is a failed retry rather than a hang.

Only the benchmark harness changes; yhub itself is untouched.

Copilot-Session: c3579153-958b-482e-bd14-025a1a9c09af
@adstep adstep closed this Oct 14, 2026
@adstep adstep reopened this Oct 14, 2026
@adstep
adstep marked this pull request as ready for review October 14, 2026 02:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant