Skip to content

feat: download mode for the chunk stream endpoint - #5618

Open
martinconic wants to merge 5 commits into
masterfrom
feat/chunk-stream-download
Open

martinconic wants to merge 5 commits into
masterfrom
feat/chunk-stream-download

Conversation

@martinconic

@martinconic martinconic commented Sep 16, 2026 •

Copy link
Copy Markdown
Contributor

Checklist

  • I have read the coding guide.
  • My change requires a documentation update, and I have done it.
  • I have added tests to cover my changes.
  • I have filled out the description and linked the related issues.

Description

Adds a request/response protocol to GET /chunks/stream: a client opens one websocket and pipelines chunk downloads and uploads over it, instead of paying for an HTTP request per chunk.

Closes #5417, closes #5599.

The first version of this PR used a hand-rolled ['D'][32-byte address]... framing. Following review (@acud, @aloknerurkar) it now uses protobuf with a request id per message, so each request gets its own status and a client can recover after an error. That earlier download mode never shipped and has been removed.

Protocol

Negotiated with Sec-WebSocket-Protocol: swarm-chunk-stream, or ?mode=stream for browser clients that cannot set headers. Messages are defined in pkg/api/pb/chunkstream.proto, one protobuf message per websocket binary message.

  • Request: a client-assigned Id, plus either a GetRequest (32-byte Address, optional CacheOption) or a PutRequest (Data, optional Stamp, required Type).
  • Response: the same Id, a Status, the chunk Address, Data for downloads, and a sanitised Error. Exactly one response per request; responses arrive in completion order, so clients match on Id.
  • Status: OK, NOT_FOUND, ERROR, BAD_REQUEST, and BUSY when a queue is full (retry with backoff). Every enum has an UNSPECIFIED = 0 value that is never sent.
  • Chunk type is required on every upload: CHUNK_TYPE_CAC or CHUNK_TYPE_SOC. SOCs are validated including their signature; an unspecified or unknown type is rejected with BAD_REQUEST.
  • Stamping: Swarm-Postage-Batch-Id on the connection stamps every chunk, or each PutRequest carries its own pre-signed Stamp. Swarm-Tag makes uploads deferred (stored locally, synced later) with either form of stamping.
  • Cache: Swarm-Cache / ?cache= set the connection default; CacheOption overrides it per request.

What OK means for an upload: for a direct upload, the chunk has been pushed to the network; for a tagged upload, it has been stored locally. It is never sent before that.

Errors: a failing request gets its own status and does not close the connection. The connection is closed only for a message that cannot be decoded (1003), a message over 64 KiB (1009), or node shutdown (1001). Responses received before a close are final; a client should resend any request whose Id got no response.

The legacy upload stream (no subprotocol, or ?mode=upload) is unchanged and byte-for-byte identical to master.

Notes for review

  • Encoding: gogo protobuf, following the p2p protocols' pb/ convention. No new dependency.
  • Fairness: downloads and uploads have separate queues (1024 each) and worker pools (8 each), so a backlog of slow uploads can't starve downloads. A full queue answers BUSY instead of blocking the read loop.
  • Timeouts: a download is bounded by getter.DefaultFetchTimeout, an upload by 30s. The delivery write deadline is 5 minutes, so a client that pauses reading while its buffer drains doesn't get disconnected.
  • Shutdown: the socket is closed before waiting on workers, because a worker blocked writing to a client that has stopped reading is released only by the socket closing. Close frames use a 250ms deadline, since gorilla queues them behind an in-progress write.
  • Status mapping: topology.ErrNotFound maps to NOT_FOUND, matching how bzz.go treats the same error from storer.Download.
  • Metrics: open connections, deliveries by outcome (including upload success/error), and fetch duration.
  • OpenAPI bumped to 8.3.0.

Testing

15 tests cover download and upload, interleaving, per-request errors, a failed push, fairness under blocked uploads, a full queue, per-chunk stamps, a feed-sized SOC, tags with per-chunk stamps, cache options, protocol violations, and shutdown — including with a client that has stopped reading. The fairness, queue-full and shutdown tests were checked against deliberately broken code to confirm they fail when they should.

make build, make lint, make vet, make test and the race detector all pass; the redocly problem count is unchanged from master.

AI Disclosure

  • This PR contains code that has been generated by an LLM.
  • I have reviewed the AI generated code thoroughly.
  • I possess the technical expertise to responsibly review the code generated in this PR.

@martinconic
martinconic marked this pull request as draft September 16, 2026 12:44
@martinconic
martinconic marked this pull request as ready for review September 16, 2026 15:30
Comment thread pkg/api/chunk_stream.go Outdated
Comment thread pkg/api/chunk_stream.go Outdated
Comment thread pkg/api/chunk_stream.go Outdated
Comment thread pkg/api/chunk_stream.go Outdated
Comment thread pkg/api/chunk_stream.go Outdated
s.metrics.ChunkStreamDeliveryCount.WithLabelValues("success").Inc()

chunkData := chunk.Data()
resp := make([]byte, 1+swarm.HashSize+len(chunkData))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure about this custom serialization by hand thing... it is just specced out in the openapi spec and assumed that implementers should implement it by hand. it is fragile and breakable. why not use some sort of standard serialization format to both decode the request and encode the response?

@aloknerurkar aloknerurkar left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we are making breaking changes and also building on the serialization thread — I'd suggest going further than reworking the download framing: use one connection for both directions, with request/response semantics and a request id.

Two things this fixes beyond tidiness:

  1. Upload is round-trip bound. The upload loop is strictly sequential — read → put → ack → read, one chunk in flight. Each chunk costs a full RTT, so on a 50ms link that's ~20 chunks/sec regardless of bandwidth. Download already has a 16-worker pool; upload has none. stamper.Stamp already takes issuer.mtx (pkg/postage/stamper.go:43), so concurrent stamping is safe — the ceiling is the protocol, not the storage layer.

  2. Neither direction is recoverable. Download replies carry no id, and the upload ack is successWsMsg = []byte{} — an empty frame with no address at all. Clients correlate purely by ordering. On any error both paths call sendErrorClose and drop the connection, so a client that had N requests outstanding cannot tell which completed. For bulk upload that means restarting from zero.

A shared envelope — [type][8-byte request-id][payload] for requests, [type][8-byte request-id][status][payload] for responses — would give:

  • one read loop feeding one typed job channel, with a worker pool serving both Get and Put (removes the duplicated read/deadline/close-handler logic between the two handlers)
  • pipelined uploads instead of one-at-a-time
  • per-request error status instead of connection teardown, so a single bad chunk no longer kills the stream
  • correlation for recovery after an error close

I'd keep this as raw binary rather than JSON-RPC or protobuf. JSON-RPC costs more on the wire with base64 and ~15x encode/decode — self-defeating for an endpoint that exists to cut per-chunk overhead. Protobuf is bee's p2p convention but has never appeared in pkg/api; requiring a schema compiler would be a new burden on bee-js. The binary envelope stays a four-line DataView parse in the browser with no dependency.

The upload stream endpoint was not written with care. I was going through the code and there is a lot of scope to cleanup. Multiple putters defined, deferred/direct upload semantics, stamped and unstamped chunks etc. I feel that this would make it much more usable for clients and also close to what elad mentioned about having grpc style chunk get/put API.

Comment thread pkg/api/chunk_stream.go Outdated
Comment thread pkg/api/chunk_stream_test.go Outdated
Comment thread pkg/api/chunk_stream.go Outdated
}

// fetchAndSendChunk retrieves a single chunk and writes exactly one response
// frame for it. That one-frame-per-requested-address invariant is what lets a

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly one reply per requested address, and "a dropped frame is indistinguishable from a slow one" because replies carry no request id. But the protocol-error paths in the read loop (sendErrorClose + return on bad message type, bad length, unknown opcode, oversized batch) void every in-flight request silently.

A client that sends 256 addresses and then trips CloseMessageTooBig on its next frame has no way to determine which of the first 256 were answered. With no request id and no per-batch boundary marker in the reply stream, the only recovery is to discard everything and re-request.

The OpenAPI spec documents the close codes, but not that pending replies are lost when they fire. At minimum that consequence belongs in the spec. A framing with a request/batch id — which is largely what @acud point about a standard serialization format would give you for free — would make it recoverable instead.

@martinconic

Copy link
Copy Markdown
Contributor Author

Since we are making breaking changes and also building on the serialization thread — I'd suggest going further than reworking the download framing: use one connection for both directions, with request/response semantics and a request id.

Two things this fixes beyond tidiness:

  1. Upload is round-trip bound. The upload loop is strictly sequential — read → put → ack → read, one chunk in flight. Each chunk costs a full RTT, so on a 50ms link that's ~20 chunks/sec regardless of bandwidth. Download already has a 16-worker pool; upload has none. stamper.Stamp already takes issuer.mtx (pkg/postage/stamper.go:43), so concurrent stamping is safe — the ceiling is the protocol, not the storage layer.
  2. Neither direction is recoverable. Download replies carry no id, and the upload ack is successWsMsg = []byte{} — an empty frame with no address at all. Clients correlate purely by ordering. On any error both paths call sendErrorClose and drop the connection, so a client that had N requests outstanding cannot tell which completed. For bulk upload that means restarting from zero.

A shared envelope — [type][8-byte request-id][payload] for requests, [type][8-byte request-id][status][payload] for responses — would give:

  • one read loop feeding one typed job channel, with a worker pool serving both Get and Put (removes the duplicated read/deadline/close-handler logic between the two handlers)
  • pipelined uploads instead of one-at-a-time
  • per-request error status instead of connection teardown, so a single bad chunk no longer kills the stream
  • correlation for recovery after an error close

I'd keep this as raw binary rather than JSON-RPC or protobuf. JSON-RPC costs more on the wire with base64 and ~15x encode/decode — self-defeating for an endpoint that exists to cut per-chunk overhead. Protobuf is bee's p2p convention but has never appeared in pkg/api; requiring a schema compiler would be a new burden on bee-js. The binary envelope stays a four-line DataView parse in the browser with no dependency.

The upload stream endpoint was not written with care. I was going through the code and there is a lot of scope to cleanup. Multiple putters defined, deferred/direct upload semantics, stamped and unstamped chunks etc. I feel that this would make it much more usable for clients and also close to what elad mentioned about having grpc style chunk get/put API.

Thank you for this, sounds like a good change. @acud do you also agree?

@acud

acud commented Sep 23, 2026

Copy link
Copy Markdown
Contributor

@martinconic i generally encourage the initiative from @aloknerurkar, but i'm still not 100% convinced a custom encoding is right.
@aloknerurkar, to answer some of your points:

  • the per-chunk overhead we are reducing has to do more with TCP socket open/close overhead and all sorts of stamper serialization overhead, not message encoding
  • i agree that it would be better to have things as you suggested
  • i'd still suggest to have a standard encoding library, ideally protobuf, because it has tooling out there and you don't have to send people (or agents) to do endianness or the likes in a browser js library
  • also not sure, if all is managed on one websocket, how much we have to deal with partial reads/flushes/buffering

i think we can progress with this proposal, and make serialization a pluggable implementation. but in general i'd encourage to move away from hand written serialization at least on wire-formats (persistence is a different story).

@aloknerurkar

Copy link
Copy Markdown
Contributor

@martinconic i generally encourage the initiative from @aloknerurkar, but i'm still not 100% convinced a custom encoding is right. @aloknerurkar, to answer some of your points:

  • the per-chunk overhead we are reducing has to do more with TCP socket open/close overhead and all sorts of stamper serialization overhead, not message encoding
  • i agree that it would be better to have things as you suggested
  • i'd still suggest to have a standard encoding library, ideally protobuf, because it has tooling out there and you don't have to send people (or agents) to do endianness or the likes in a browser js library
  • also not sure, if all is managed on one websocket, how much we have to deal with partial reads/flushes/buffering

i think we can progress with this proposal, and make serialization a pluggable implementation. but in general i'd encourage to move away from hand written serialization at least on wire-formats (persistence is a different story).

I am fine with protobuf. The only reason I didn't suggest it is that bee-js/javascript will be primary consumer if this is successful and I remember there were issues with javascript ecosystem when working with protobuf. I spoke to claude and I see that there is a new typescript compiler which makes things easier. So I am totally on board!

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.

Download chunks with websocket feat(api): Add batched chunk retrieval endpoint (e.g., POST /chunks/batch)

3 participants