fix(universaldb): chunk postgres commit requests that exceed the nats max_payload

[MasterPtato]

No description provided.

[MasterPtato]

Stack for rivet-dev/actors

• #5704 fix(universaldb): run postgres transaction reads concurrentl …
• #5705 test(gasoline): reproduce slow workflow pulls on postgres
• #5702 fix(universaldb): chunk postgres commit requests that exceed … 👈
• #5703 fix(universaldb): clear postgres key ranges one statement pe …
• #5706 perf(universaldb): time each drain batch statement to locali …
• #5695 fix(universaldb): report the last error when transaction ret …
• main

Get stack: forklift get 5702
Push local edits: forklift submit
Merge when ready: forklift merge 5702

_{change zwmywrmu}

[the-company-company]

✅ No issues found

_{Reviewed commit 7f4ec42.}

[claude]

Review

Solid fix for a real problem (a >1MiB follower commit would get its NATS connection closed outright). The chunking design is careful: a separate commit_chunk subject so pre-upgrade leaders never see undecodable messages, a monotonic attempt counter so a stale resend's pieces can't get mixed into a newer one, strict sequential-index reassembly that aborts (rather than risking silent corruption) on any gap, and idle eviction so a dead follower's partial request doesn't pin memory forever. The vbare v1→v2 migration follows the repo's field-by-field conversion convention correctly, and vec![Ok] for the version-less CommitRequestChunk matches the documented pattern for a type with no real predecessor. Test coverage is good: unit tests cover gap-abandonment, stale-attempt rejection, and idle eviction; the integration test verifies an oversized commit both succeeds via chunking and fails fast (with the fleet-negotiation guard) without ever hitting the NATS server's payload limit.

A few things worth a look:

Performance/reliability: In submit_nats (commit.rs:157), the single REQUEST_TIMEOUT (5s) wraps the entire send_chunks call — every intermediate publish plus the final request — not just the round-trip. For a commit split into many chunks (small max_payload, or just a large payload under network latency), the whole send could routinely blow past 5s before the last chunk is even sent, causing "timed out; resending" on every attempt and burning all MAX_SUBMIT_ATTEMPTS without ever completing, even though the data itself is fine. The per-piece publishes and the final round-trip probably deserve their own timeout budgets rather than sharing one request-sized window.

Minor efficiency: PendingRequest.bytes (chunks.rs) starts as the first piece and grows via extend_from_slice per subsequent chunk with no capacity hint, so a large reassembled request pays for several reallocations. Since count and the first piece's length are known up front, Vec::with_capacity could avoid most of that.

Design assumption worth flagging: the reassembly logic leans on "NATS delivers one publisher's messages on a subject in order" (stated in the chunks.rs doc comment). If that's ever violated (e.g. under certain clustered NATS routing/reconnect scenarios), the code fails safe — any out-of-order index aborts the attempt rather than reassembling incorrectly — but it would show up as repeated failed attempts rather than a clean error, which is worth being aware of operationally.

Nothing here blocks the change; the fail-safe design means the concerns above are latency/availability risks under adverse conditions, not correctness bugs.