Sync wire diet: binary envelope frames + permessage-deflate - #1057
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
📝 WalkthroughWalkthroughThe sync protocol adds ADE1 binary envelopes, binary chunking, per-message deflate configuration, and wire-aware sizing. The sync host and iOS service negotiate the capability and support binary frame parsing, decompression, chunk assembly, and legacy text fallback. ChangesBinary sync transport
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/ios/ADE/Services/SyncService.swift (1)
17095-17105: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReject a nested
envelope_chunkafter reassembly.
syncPreprocessIncomingData(reassembled)can return anotherenvelope_chunk.handleIncoming(nested)then re-enters this samecase, reassembles again, and recurses. No depth limit exists.A host can drive this cheaply. One chunk frame with
total: 1whose body is another chunk frame advances one level per frame. Each level holds its reassembledDataalive across anawait, so memory grows with depth and never unwinds until the innermost frame resolves.The TypeScript host already treats this shape as a protocol error.
apps/ade-cli/src/services/sync/syncHostService.tsline 3234 throws"Nested envelope_chunk frames are not allowed.". Apply the same rule here so both sides reject the same wire shape.As per path instructions: "iOS Swift app — check for memory management, Swift conventions, and proper SwiftUI patterns".
🛡️ Proposed guard against nested chunk frames
if let reassembled { // The reassembled envelope can be tens of megabytes — decode it off // the main actor like any first-class frame. let nested = try await Task.detached(priority: .userInitiated) { try syncPreprocessIncomingData(reassembled) }.value guard isCurrentConnectionGeneration(generation) else { return } if let nested { + guard nested.type != "envelope_chunk" else { + throw NSError( + domain: "ADE", + code: 10, + userInfo: [NSLocalizedDescriptionKey: "Nested envelope_chunk frames are not allowed."] + ) + } try await handleIncoming(nested) } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/ios/ADE/Services/SyncService.swift` around lines 17095 - 17105, Reject nested envelope_chunk frames in the reassembly branch of handleIncoming: after syncPreprocessIncomingData(reassembled), detect when the resulting nested frame is itself an envelope_chunk and throw the protocol error "Nested envelope_chunk frames are not allowed." before recursively calling handleIncoming(nested). Preserve normal handling for non-chunk nested frames and keep the existing connection-generation check.Source: Path instructions
🧹 Nitpick comments (2)
apps/ios/ADE/Services/SyncService.swift (1)
16993-16996: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPreserve diagnostic context when a binary frame fails to decode.
For a binary frame,
textis set to""at line 16967. If the detached decode throws,handleIncomingFailure(error, text: text, task: task)receives an empty string. The failure report therefore carries no frame context at all.Binary frames are the hardest frames to diagnose from a device log, because the payload is compressed and no readable text exists. Pass a short description instead, for example the frame byte count and the decoded header type.
♻️ Proposed diagnostic context for binary frames
} catch { if self.socket === task { - self.handleIncomingFailure(error, text: text, task: task) + let failureContext = binaryFrame.map { "<ADE1 binary frame, \($0.count) bytes>" } ?? text + self.handleIncomingFailure(error, text: failureContext, task: task) } break }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/ios/ADE/Services/SyncService.swift` around lines 16993 - 16996, Update the binary-frame decode failure path in the surrounding sync handling to construct a concise diagnostic description containing the frame byte count and decoded header type, then pass it instead of the empty text value to handleIncomingFailure(error:text:task:). Preserve the existing behavior for non-binary frames and the self.socket === task guard.apps/ade-cli/src/services/sync/syncProtocol.ts (1)
66-71: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winBound per-connection zlib memory for permessage-deflate.
SYNC_PER_MESSAGE_DEFLATE_OPTIONSretains the default 320 KiB per-connection zlib context pair and only limits concurrently running jobs viaconcurrencyLimit. Keep context takeover and add boundedzlibDeflateOptions/zlibInflateOptionsso context retention per peer does not grow independently of the pool size.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/ade-cli/src/services/sync/syncProtocol.ts` around lines 66 - 71, Update SYNC_PER_MESSAGE_DEFLATE_OPTIONS to retain context takeover while adding bounded zlibDeflateOptions and zlibInflateOptions settings, limiting per-connection zlib context memory independently of concurrencyLimit. Use the supported zlib memory configuration fields and preserve the existing threshold and job concurrency behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/ade-cli/src/services/sync/syncProtocol.ts`:
- Around line 590-593: Set binaryType to "buffer" on every WebSocketServer used
to send sync binary envelopes, including the instances created by
syncHostService.ts, sharedSyncListener.ts, and related tests. Ensure all sync
server construction paths preserve binary frames as Buffer values so
parseSyncEnvelopeFrame can use isSyncBinaryFrame without converting them to
text.
- Around line 618-630: Update parseSyncBinaryEnvelope’s compression === "none"
branch to validate that type is "envelope_chunk" before constructing the raw
envelope; reject other uncompressed binary frame types with the protocol error
“Uncompressed binary sync envelopes must be envelope chunks.”
In `@apps/ios/ADE/Services/SyncService.swift`:
- Line 1838: Replace both Data-to-String conversions in the sync preprocessing
path and receive loop with the failable String(bytes:encoding:) initializer,
including the conversion near syncPreprocessIncoming and the corresponding
receive-loop conversion. Propagate the resulting optional failure through the
existing error-handling flow so invalid UTF-8 produces an explicit decode
failure rather than continuing to JSON parsing.
---
Outside diff comments:
In `@apps/ios/ADE/Services/SyncService.swift`:
- Around line 17095-17105: Reject nested envelope_chunk frames in the reassembly
branch of handleIncoming: after syncPreprocessIncomingData(reassembled), detect
when the resulting nested frame is itself an envelope_chunk and throw the
protocol error "Nested envelope_chunk frames are not allowed." before
recursively calling handleIncoming(nested). Preserve normal handling for
non-chunk nested frames and keep the existing connection-generation check.
---
Nitpick comments:
In `@apps/ade-cli/src/services/sync/syncProtocol.ts`:
- Around line 66-71: Update SYNC_PER_MESSAGE_DEFLATE_OPTIONS to retain context
takeover while adding bounded zlibDeflateOptions and zlibInflateOptions
settings, limiting per-connection zlib context memory independently of
concurrencyLimit. Use the supported zlib memory configuration fields and
preserve the existing threshold and job concurrency behavior.
In `@apps/ios/ADE/Services/SyncService.swift`:
- Around line 16993-16996: Update the binary-frame decode failure path in the
surrounding sync handling to construct a concise diagnostic description
containing the frame byte count and decoded header type, then pass it instead of
the empty text value to handleIncomingFailure(error:text:task:). Preserve the
existing behavior for non-binary frames and the self.socket === task guard.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3618c693-7c49-4c15-8782-073275da2c7b
📒 Files selected for processing (8)
apps/ade-cli/src/services/sync/sharedSyncListener.tsapps/ade-cli/src/services/sync/syncBinaryFrame.test.tsapps/ade-cli/src/services/sync/syncBinaryFrame.tsapps/ade-cli/src/services/sync/syncHostService.tsapps/ade-cli/src/services/sync/syncProtocol.test.tsapps/ade-cli/src/services/sync/syncProtocol.tsapps/desktop/src/shared/types/sync.tsapps/ios/ADE/Services/SyncService.swift
| export function parseSyncEnvelopeFrame(raw: unknown): ParsedSyncEnvelope { | ||
| if (isSyncBinaryFrame(raw)) return parseSyncBinaryEnvelope(raw); | ||
| return parseSyncEnvelope(wsDataToText(raw)); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check binaryType configuration and raw-message shapes on every sync WebSocket path.
set -euo pipefail
# Test: locate binaryType settings and WebSocketServer construction options.
rg -n -C4 'binaryType' --type=ts
rg -n -C6 'new WebSocketServer\(' --type=ts
# Test: inspect wsDataToText to see which raw shapes it already normalizes.
ast-grep run --pattern 'function wsDataToText($_) { $$$ }' --lang typescript
rg -n -C10 'wsDataToText' --type=tsRepository: arul28/ADE
Length of output: 148
🏁 Script executed:
#!/bin/bash
set -u
printf '--- files ---\n'
git ls-files | rg 'apps/ade-cli/src/services/sync|syncProtocol|syncBinary|adapter|webSocket|WebSocket' || true
printf '\n--- target location context ---\n'
if [ -f apps/ade-cli/src/services/sync/syncProtocol.ts ]; then
wc -l apps/ade-cli/src/services/sync/syncProtocol.ts
sed -n '560,610p' apps/ade-cli/src/services/sync/syncProtocol.ts
fi
printf '\n--- search binaryType / WebSocketServer / wsDataToText ---\n'
rg -n -C4 'binaryType|WebSocketServer|wsDataToText|parseSyncEnvelopeFrame|isSyncBinaryFrame|parseSyncBinaryEnvelope' . || trueRepository: arul28/ADE
Length of output: 50367
🏁 Script executed:
#!/bin/bash
set -u
printf '%s\n' '--- syncProtocol wsDataToText ---'
rg -n -C12 'function wsDataToText|const wsDataToText' apps/ade-cli/src/services/sync/syncProtocol.ts
printf '%s\n' '--- parseSyncEnvelopeFrame and parseSyncBinaryEnvelope ---'
sed -n '540,645p' apps/ade-cli/src/services/sync/syncProtocol.ts
sed -n '645,730p' apps/ade-cli/src/services/sync/syncProtocol.ts
printf '%s\n' '--- syncHostService message handler ---'
sed -n '3185,3240p' apps/ade-cli/src/services/sync/syncHostService.ts
printf '%s\n' '--- sharedSyncListener WebSocketServer construction ---'
sed -n '555,575p' apps/ade-cli/src/services/sync/sharedSyncListener.ts
rg -n -C4 'new WebSocketServer\([' apps/ade-cli/src/services/sync apps/desktop/src/main/services/sync --iglob '*.ts' || true
printf '%s\n' '--- binaryType occurrences in TypeScript only ---'
rg -n /binaryType/ --glob '*.ts' . || true
printf '%s\n' '--- deterministic Buffer / non-Buffer detection behavior ---'
node - <<'JS'
const data = [
Buffer.from("ADE1xxxx"),
new ArrayBuffer(10),
new Uint8Array([0x41, 0x44, 0x45, 0x31, 0x78, 0x78, 0x78, 0x78]),
[Buffer.from("ADE1xxxx")],
];
for (const raw of data) {
const isBuffer = Buffer.isBuffer(raw);
const text = Buffer.isBuffer(raw)
? raw.toString("utf8")
: ((raw instanceof ArrayBuffer || raw instanceof Uint8Array) ? Buffer.from(raw).toString("utf8") : String(raw));
console.log(JSON.stringify({
type: Array.isArray(raw) ? "Buffer[]" : Array.from(raw).slice(0,4).filter(x => (x >= 32 && x <=126) || x === 9 || x === 10 || (x >= 13 && x <=13 || x >= 128)).join(""),
isBuffer,
text,
jsonParse: (() => { try { JSON.parse(text); return "ok"; } catch (e) { return e.message; } })()
}));
}
JSRepository: arul28/ADE
Length of output: 12356
🏁 Script executed:
#!/bin/bash
set -u
printf '%s\n' '--- syncHostService WebSocketServer construction ---'
sed -n '2680,2715p' apps/ade-cli/src/services/sync/syncHostService.ts
printf '%s\n' '--- WebSocketServer construction options in sync TypeScript ---'
rg -n -C5 'new WebSocketServer\(' apps/ade-cli/src/services/sync apps/desktop/src/main/services/sync --glob '*.ts' || true
printf '%s\n' '--- relay binary/frame handling references ---'
rg -n `binaryType|ArrayBuffer|Uint8Array|Buffer\.concat|Buffer\.from|send\(data|send\([^,]+,\s*\{\s*binary\s*:` apps/ade-cli/src/services/sync apps/desktop/src/main/services/sync apps/tunnel-relay --glob '*.ts' --glob '*.mjs' || true
printf '%s\n' '--- behavioral probe for wsDataToText non-Buffer binary raw shapes ---'
node - <<'JS'
const BufferLike = class { constructor(buf) { this.buf = buf; } toString() { return this.buf.toString('utf8'); } };
const nonBufferBinary = new Uint8Array(Buffer.from("EDE1bad envelope", "utf8"));
const result = (() => {
if (typeof nonBufferBinary === "string") return "string";
if (Buffer.isBuffer(nonBufferBinary)) return nonBufferBinary.toString("utf8");
if (Array.isArray(nonBufferBinary)) return Buffer.concat(nonBufferBinary).toString("utf8");
return String(nonBufferBinary);
})();
console.log(JSON.stringify({
rawType: nonBufferBinary.constructor.name,
outputType: typeof result,
output: result.slice(0, 40),
jsonValid: true,
}));
JSRepository: arul28/ADE
Length of output: 47410
Set binaryType: "buffer" on every sync WebSocketServer that sends binary envelopes.
syncHostService.ts and sharedSyncListener.ts create WebSocketServer instances without binaryType, and tests also omit it. Since isSyncBinaryFrame only accepts Buffer, any ArrayBuffer-typed binary frame becomes UTF-8 text via wsDataToText, fails parseSyncEnvelope, and closes the sync connection with Invalid sync envelope JSON.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/ade-cli/src/services/sync/syncProtocol.ts` around lines 590 - 593, Set
binaryType to "buffer" on every WebSocketServer used to send sync binary
envelopes, including the instances created by syncHostService.ts,
sharedSyncListener.ts, and related tests. Ensure all sync server construction
paths preserve binary frames as Buffer values so parseSyncEnvelopeFrame can use
isSyncBinaryFrame without converting them to text.
| maxUncompressedBytes: Int = maxUncompressedSyncEnvelopeBytes | ||
| ) throws -> SyncPreprocessedEnvelope? { | ||
| guard SyncBinaryFrame.isBinaryFrame(data) else { | ||
| return try syncPreprocessIncoming(String(decoding: data, as: UTF8.self), maxUncompressedBytes: maxUncompressedBytes) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use the failable String(bytes:encoding:) initializer for the two Data to String conversions.
SwiftLint reports optional_data_string_conversion on both lines. String(decoding:as:) never fails. It substitutes U+FFFD for invalid bytes, so a corrupt or mis-routed binary frame becomes a mangled string and then surfaces as a confusing JSON parse error instead of an explicit decode failure.
Line 1838 handles a binary frame without the ADE1 magic. Line 16969 handles the same case in the receive loop. Both are exactly the paths where a non-text frame can arrive.
🐛 Proposed fix for both conversions
Line 1838:
guard SyncBinaryFrame.isBinaryFrame(data) else {
- return try syncPreprocessIncoming(String(decoding: data, as: UTF8.self), maxUncompressedBytes: maxUncompressedBytes)
+ guard let text = String(bytes: data, encoding: .utf8) else {
+ throw NSError(
+ domain: "ADE",
+ code: 10,
+ userInfo: [NSLocalizedDescriptionKey: "Sync frame is neither a binary envelope nor valid UTF-8 text."]
+ )
+ }
+ return try syncPreprocessIncoming(text, maxUncompressedBytes: maxUncompressedBytes)
}Line 16969:
} else {
- text = String(decoding: data, as: UTF8.self)
+ text = String(bytes: data, encoding: .utf8) ?? ""
}Also applies to: 16969-16969
🧰 Tools
🪛 SwiftLint (0.65.0)
[Warning] 1838-1838: Prefer failable String(bytes:encoding:) initializer when converting Data to String
(optional_data_string_conversion)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/ios/ADE/Services/SyncService.swift` at line 1838, Replace both
Data-to-String conversions in the sync preprocessing path and receive loop with
the failable String(bytes:encoding:) initializer, including the conversion near
syncPreprocessIncoming and the corresponding receive-loop conversion. Propagate
the resulting optional failure through the existing error-handling flow so
invalid UTF-8 produces an explicit decode failure rather than continuing to JSON
parsing.
Source: Linters/SAST tools
6b29e0c to
4988d1e
Compare
4988d1e to
c47aadb
Compare
A compressed envelope was base64'd into a JSON text frame — a flat +33% re-inflation of exactly the bytes compression had just removed. Compressed envelopes now travel in a binary container instead, gated on a new `binaryEnvelopes` hello capability so a peer that does not declare it keeps the base64 wire byte for byte. The container is a magic prefix, a uint32 header length, the envelope minus its payload as JSON, then the compressed bytes raw. The magic is load-bearing rather than decoration: `wsDataToText` has always decoded Buffer frames as utf8, and transports do deliver text that way, so sniffing the first four bytes is what keeps a text frame arriving as data on the text path. Oversized envelopes chunk as binary too. This matters more than it looks: a text chunk base64s an envelope whose payload is already base64, and budgets its slice down to 3/4 to pay for the expansion, so the tax compounds exactly on the largest frames. One real db_version group in this machine's database is 11.4 MiB and cannot be split — the pump must ack rows sharing a db_version together — so that path is not hypothetical. Measured through `encodeSyncEnvelopeFrames` on 26.2 MiB of this machine's real CRR rows, at the 720 KiB frame budget iOS negotiates: catch-up (250-row batches) 6.51 MiB -> 4.03 MiB 38.1% smaller live broadcast (4-row) 8.47 MiB -> 5.60 MiB 33.9% smaller Per envelope in isolation the saving is the flat 25% base64 tax; the rest is the chunk path no longer paying it twice. Also enables permessage-deflate on both sync WebSocket servers, and skips the application codec when a peer negotiated it. The transport's dictionary persists across frames, so it compresses changeset traffic better than per-envelope compression can, and stacking the two is worse than either alone — measured as bytes written to a real socket, live-broadcast sized: 7.34 MiB app-level only, 5.31 MiB both, 4.14 MiB transport only. iOS cannot negotiate the extension (`URLSessionWebSocketTask` has no support), which is precisely why it gets the binary container instead. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
c47aadb to
c1867ce
Compare
Tier 1C of the t3code competitor research, items 1 and the transport half of item 2. Builds on #1056's projection layer rather than duplicating it — this changes how bytes are framed, not what is in them.
What changed
Compressed envelopes travel as binary frames.
encodeSyncEnvelopecompressed the payload and then base64'd it into a JSON text frame — a flat +33% re-inflation of exactly the bytes it had just removed. Compressed envelopes now use a binary container (magicADE1, uint32 header length, envelope-minus-payload as JSON, compressed bytes raw), gated on a newbinaryEnvelopeshello capability. A peer that does not declare it keeps the base64 wire byte for byte.The magic prefix is load-bearing, not decoration:
wsDataToTexthas always decoded Buffer frames as utf8 and transports do deliver text that way, so sniffing four bytes is what keeps a text frame arriving as data on the text path.Oversized envelopes chunk as binary too, which matters more than it looks — a text chunk base64s an envelope whose payload is already base64 and budgets its slice down to 3/4 to pay for the expansion, so the tax compounds precisely on the largest frames.
permessage-deflate on both sync WebSocket servers, with the application codec skipped when a peer negotiates it. The transport's dictionary persists across frames, so it compresses changeset traffic better than per-envelope compression can.
Measurements
All on 26.2 MiB of this machine's real
crsql_changesrows, batched through the production batcher (selectChangesetBatchChunk) and encoded through the production encoder at the 720 KiB frame budget iOS negotiates.Per envelope in isolation the saving is the flat 25% base64 tax; the remainder is the chunk path no longer paying it twice. That path is not hypothetical — one real
db_versiongroup in this database is 11.4 MiB and cannot be split, because the pump must ack rows sharing adb_versiontogether.permessage-deflate, measured as bytes actually written to a real socket, live-broadcast sized:
Stacking the two is worse than either alone, which is why the application codec is skipped when the extension is negotiated.
A correction to the research report
The report's headline for this item — "today 2.08x, shared-window deflate 7.64x, 3.68x smaller than today" — is not reproducible, and the gap is a baseline error rather than a measurement error.
deflate@512— which current iOS does — is at 3.2x–4.9x today, not 2.08x.The measured decomposition, live-broadcast sized: raw 26.65 MiB → today 7.33 MiB → binary frames 5.51 MiB → streaming context takeover 4.52 MiB. Binary framing is the reliable, stateless win; context takeover adds 12–18% on top of it and only on small envelopes.
This also changes the design for the rest of item 2. permessage-deflate already is context takeover, done by the transport, and it measured better than the hand-rolled equivalent (4.14 vs 4.52 MiB) because it compresses raw JSON rather than already-deflated bytes and carries no per-envelope header. So for every peer that can negotiate it — browsers, the desktop renderer, Node sync peers — item 2 is complete with no per-peer stream state, no ordering coupling, and no reset story. A bespoke app-level stream is now only worth considering for iOS, which cannot negotiate the extension.
Compatibility
binaryEnvelopesis declared only by iOS in this change; an older build never declares it and receives the identical base64 wire.Verification
syncHostServicetests.syncBinaryFrame.test.ts: container round-trip, empty body, truncated and lying header lengths, header cap, non-object header, JSON-text-delivered-as-binary taking the text path, payload equality between the binary and base64 wires, uncompressed payloads staying JSON text, gzip and deflate, binary chunking under the frame budget, out-of-order reassembly, and chunk-header validation.xcodebuildBUILD SUCCEEDED against the current scheme.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Greptile Summary
The PR adds capability-gated binary framing for compressed sync envelopes and enables permessage-deflate on the sync WebSocket servers.
ADE1binary container and raw binary chunking/reassembly.Confidence Score: 5/5
The PR appears safe to merge.
No blocking failure remains.
Important Files Changed
Sequence Diagram
sequenceDiagram participant Client participant Host Client->>Host: hello(capabilities) alt binaryEnvelopes and application compression Host->>Client: ADE1 header + compressed binary body opt frame exceeds negotiated budget Host->>Client: ADE1 binary envelope_chunk frames Client->>Client: Reassemble binary envelope end else permessage-deflate negotiated Host->>Client: JSON text frame compressed by WebSocket transport else legacy peer Host->>Client: JSON text with base64-compressed payload endReviews (4): Last reviewed commit: "Send compressed sync envelopes as binary..." | Re-trigger Greptile
Context used:
ade codeTUI