Repository navigation
feat(swift): add the VSR protocol layer with request and reply codecs - #4355
RustToMetal wants to merge 3 commits into
Conversation
|
Thanks for the PR. It is labeled Slash commands (own line, regular comment) move it around the queue:
See CONTRIBUTING.md for details. |
|
/ready |
c43ff2d to
53abd50
Compare
|
/skill team-review-slim |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary: The Swift VSR protocol layer matches the Rust wire formats, but its batch decoder adds an unbounded batch length before it checks the sum, so a hostile poll reply aborts the client, and the default poll path skips the frame-count tiling check that the Rust decoder enforces. The remaining notes cover trapping reply-header accessors, missing error context in the user validators, and three unused header offsets.
Counts: critical 1, warning 1, nit 2, simplification 1
This review was generated by Claude Code 2.1.284 on deepseek-flash[1m]. Review the output before you act on it.
github-actions
Bot
added
S-waiting-on-author
53abd50 to
a8a267a
Compare
The consensus frame every request and reply travels in: the 256-byte header with its command, operation, request id, session fence, and checksum stamp, the session state that hands out request ids the way the Rust SDK does, and the eviction and result-section rules that turn a reply into a typed error or a body. On top of it, one request encoder per command code, one reply decoder per reply shape, and the message batch format shared by send bodies and poll replies, checksummed with XXH3. Every encoder, decoder, and header is compared against bytes produced by the Rust crates through the golden fixture. Every length read from the wire is bounded before it becomes an Int, so a hostile frame cannot trap a 32-bit platform. Relates to apache#4001
c68c1d7 to
22c0926
Compare
|
purge is getting removed from the server and every sdk, and i'll open the removal pr today. so you can drop |
|
@RustToMetal could you please fix CI? it now fails after rebase |
…w error codes
Master added a 32-byte bind_secret field (for session binding) to
LoginRegisterRequest and LoginRegisterWithPatRequest, and introduced
BindSessionRequest (command 15). Three new server error codes arrived
in the same wave: partition_history_changed (3014), session_mismatch
(14004), request_id_exhausted (14005). The eviction window min was
pinned to 0.11.1 (equal to current).
- Requests.loginRegister and loginRegisterWithPersonalAccessToken
accept bindSecret: [UInt8] (default zeros) and write the 32 bytes
after the ClientVersionInfo prefix, matching the Rust wire shape.
- Requests.bindSession encodes the new re-auth request.
- CommandCode gains bindSession = 15 / "session.bind".
- IggyErrorCode gains the three new codes with their snake_case names.
- golden.json is rebased on the master fixture (protocol_version_min
11265, updated login + eviction vectors, bind_session vector) while
keeping the Swift-only purge_stream and purge_topic vectors.
- WireVectorTests passes the 0x5A test bind_secret so the encoded
bytes match the reference fixture; adds the bindSession case.
- The golden-vectors generator is updated to compile with the new
Rust structs and emit the same vectors CI checks against.
Relates to apache#4001
c7630a6 to
27ca929
Compare
This branch has not been deployed
2 participants
The protocol layer: the consensus frame every request and reply travels in, the session state that hands out request ids, one request encoder per command code, one reply decoder per reply shape, and the message batch format that send bodies and poll replies share. Nothing here opens a socket; the TCP and TLS client that drives this layer is the next PR.
Fourth of nine stacked PRs for the Swift SDK; builds on #4190, #4306, and #4335.
Which issue does this PR address?
Relates to #4001
Rationale
The client, producer, and consumer are thin over this layer, and the wire rules are the part most likely to diverge from the server. Landing them on their own lets every header, encoder, and decoder be reviewed next to the golden vector the Rust crates produced for it, before any networking is involved.
What changed?
VSRFrameportscore/binary_protocol/src/consensus/header.rs: the 256-byte header offsets, the command and eviction discriminants,encodeRequest(which stamps the request checksum for the operations the server's client table deduplicates and leaves it zero for partition and non-replicated operations, like the Rust SDK), anddecodeReply, which applies the fixed order of frame discriminant, declared size, pre-commit denial status, then the committed result section. Eviction reasons map to the same errors aseviction_reason_to_errorincore/common.ConsensusSessionportscore/sdk/src/session.rs: an ephemeral client id, the session fence, and monotonic request ids for replicated operations.ConsensusOperationandCommandCodemirror the Rust enums, with the metadata, partition, and result-framed bands.Requestshas one encoder per command, with the username, password, and name bounds applied before encoding so a length prefix can never desync.Responseshas one decoder per reply shape, mapping codec failures toinvalidCommandlike the Rust client; the stats decoder applies the crate's rule for the optional open-files tail (absent on older servers, a partial tail fails).Batchportsbatch.rs: the 256-byte batch header, the 48-byte message frame, and the XXH3 checksums over frames and batch.Every length read from the wire is bounded before it is converted or added (frame size, result-section count, prefixed blocks, batch length, message payload and header lengths), so a hostile frame cannot trap or overflow on any platform, and the raw header accessors answer nil for a short buffer instead of indexing past it. The frames of a batch must tile
message_counton the layout-only poll path as well as the verifying one, as in the RustLayoutOnlydecode. The byte codec gains the helpers this layer needs:readPrefixedBytes,skip,rest,writeZeros,overwrite, and the checksum error cases with theirIggyErrormapping.Tests:
WireVectorTestscompares every request encoder and consensus header against the Rust bytes ingolden.json,ResponseVectorTestsdoes the same for every reply decoder, andProtocolTestscovers the session rules, result-section handling, denial ordering, duplicate sends, and eviction mapping.Local Execution
swift build -Xswiftc -warnings-as-errors,swift test(106 tests),swift format lint --strict, hawkeye license checkprek run --from-ref upstream/master --to-ref HEADpassedAI Usage
core/binary_protocol,core/sdk/src/vsr.rs, andcore/sdk/src/session.rsas the reference, reviewed and directed by me.iggy-server(35 end-to-end tests) plus the 9 BDD scenarios.