fix(server): reject a refused op instead of logging, deduping and acking it (C19) - #367
Merged
Conversation
…ing it `Hub::ingest_records` appended to the store before `Document::apply`, discarded the bool, and entered the room's dedup set and retained log unconditionally. An op `apply` refuses permanently was therefore durable, deduped against the author's corrected resend of the same `OpId` forever, fanned out to every peer, replayed on each reload, and acked `Accepted` while landing nowhere. `apply`'s `false` covered three unrelated cases, only one permanent. Split it: `Op::is_admissible` names the refusal set — a stamp not naming its author, a stamp off a mintable position, a transaction size no group can have — as a pure function of the op, so every replica refuses the same set and the room converges on its absence. `apply` and the snapshot decode both gate on it, so the rule has one home. The ingest seams drop an inadmissible record before persisting; `handle_ops` refuses the batch recoverably (`OpsRejected` / `MalformedOp`) so the author keeps its ops. A merely waiting op is admissible and is logged, fanned out and acked exactly as before.
…ed and acked The mutation sweep found the gap: treating a held transaction member as inadmissible passed the suite, and would strand every group forever.
Go and Python enumerate the wire codes, and the protocol round-trip lists are the codec's spec — all three would otherwise silently omit the new reason.
- the buffered-op case now proves the op was held, not applied: the create releases it in a second phase - the whole-batch rejection uses distinct sequences, so it names the admissible op's own frontier rather than collapsing onto the refused one's - the branch tail gains a positive control and a dedup check - assert the `Accepted` frontier, which is the value the whole rule turns on - `replay::canonical_tail` applies the same filter, so its index-to-sequence mirroring holds against bytes this node did not write
KANBAN C19 to Done with the reachability correction (C3's in-process-only conclusion does not carry to #366's stamp predicates, which are wire-reachable); C26 filed for the SDK apply seams. DECISIONS + ARCHITECTURE gain the boundary.
The restore path bypasses ingest_branch and installs the stored tail verbatim, so it holds the rule on its own — the bytes are supplied by whoever hands the store over. Also documents the drop on the two public ingest entry points.
The order is load-bearing: dedup-first would let an inadmissible record's id into the batch set and swallow an admissible resend arriving beside it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hub::ingest_recordsappended to the store beforeDocument::apply, discarded the bool, and didroom.seen.insert+room.log.pushunconditionally.handle_opscomputesthroughas a max over the submitted batch, so the session repliedAccepted { through }and the author's outbox dropped the op.An op the replica refuses was therefore durably logged, permanently deduped (a corrected resend of the same
OpIdswallowed forever), fanned out to every peer, replayed on every room reload, and acked as landed while landing nowhere.Reach — the board understated it
Tx.countrefusal (the codec bounds it); does not carry to the stamp predicates fix(core): mint above the replica's own id space, and bound the stored clock both ways (C17, C18) #366 added.codec.rsreferences none ofstamp_names_its_author/stamp_occupies_a_mintable_position/reservation_end(grep count 0);decode_opsbounds neitherstamp.offset, norstamp.lamport, norstamp.clientvsid.client.doc.rs:2172is the state-decode path (a snapshot's buffered ops), not the wire op path. The wire refusal is inapply, afterdecode_opssucceeded.a_refused_op_survives_a_codec_round_trip.What
apply'sfalsemeantThree unrelated situations, only one permanent:
Only the third may be dropped.
Op::is_admissiblenames it: a public pure predicate over the op alone.applyand the snapshot decode both gate on it, so the rule has one home.Why rejecting is safe
Every condition reads the op and nothing else. Two replicas at unrelated states reach the same verdict, so refusing converges the room on the op's absence rather than splitting it. Dropping state a peer might still apply would diverge; dropping state no peer will ever apply does not.
The gates
handle_ops→OpsRejected/ newErrorCode::MalformedOp(tag 9; mirrored through wasm, FFI, Go, Python, and both codec round-trip lists). Recoverable, not a disconnect — the frame was well-formed.Hub::ingest_records+ingest_branchfilter independently —apply_replicatereachesHub::ingestfrom a peer'sReplicateframe without crossinghandle_ops.install_room) filters too: it bypassesingest_branchand installs a stored tail verbatim, and the bytes come from whoever hands the store over.replay::canonical_tailtakes the same filter — it numbers sequences from the tail the hub commits, so against bytes this node did not write an unfiltered tail slides every later op's sequence by one.Untouched
A buffered op and an incomplete transaction member are logged, fanned out and acked exactly as before. Refusing a member for not applying yet would strand its whole group forever.
Not done — filed as C26
The
bool→enum refactor acrossclient.rs/ FFI / wasm / JS / Python / Go. The public predicate makes "never" answerable at any seam that cares, andclient.rsadvancinglast_seen_seqpast a refused op is correct (no replica will hold it). What remains is the SDK apply paths reducing the result to a count.Gates
cargo test --workspace --no-fail-fast: 0 failed.-p crdtsync-server --test refused_op --test replay --test persistenceand-p crdtsync-core --test state_doc --test transaction --test protocol --test lamport_ceilingclean, exit 0.Acceptedfrontier, replay tail mismatch, timeless-comment violations, and — from the final pass — the unfiltered branch-tail restore path).--no-fail-fast, isolatedgit archivecopy), 9 mutations, none zero; unmutated tree fails 0:ingest_recordsfilteris_admissibleignores the author checkis_admissibleignores the position checkis_admissibleignores the tx-count checkreplay::canonical_tailfilteris_admissiblerefuses a held transaction memberThree of these survived an earlier sweep — the held transaction member, the replay tail, and the branch restore — and each is now killed by the test written in response. The sweep audited the suite, not only the source.