Skip to content

fix(server): reject a refused op instead of logging, deduping and acking it (C19) - #367

Merged
vieiralucas merged 10 commits into
mainfrom
server/c19-refused-op-ack
Jul 28, 2026
Merged

fix(server): reject a refused op instead of logging, deduping and acking it (C19)#367
vieiralucas merged 10 commits into
mainfrom
server/c19-refused-op-ack

Conversation

@vieiralucas

@vieiralucas vieiralucas commented Jul 28, 2026

Copy link
Copy Markdown
Member

Hub::ingest_records appended to the store before Document::apply, discarded the bool, and did room.seen.insert + room.log.push unconditionally. handle_ops computes through as a max over the submitted batch, so the session replied Accepted { 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 OpId swallowed forever), fanned out to every peer, replayed on every room reload, and acked as landed while landing nowhere.

Reach — the board understated it

What apply's false meant

Three unrelated situations, only one permanent:

  • duplicate — already applied or already held
  • waiting — target not yet reachable, or atomic group incomplete; a later arrival commits it
  • refused forever — stamp not naming its author, stamp off a mintable position, transaction size no group can have

Only the third may be dropped. Op::is_admissible names it: a public pure predicate over the op alone. apply and 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_opsOpsRejected / new ErrorCode::MalformedOp (tag 9; mirrored through wasm, FFI, Go, Python, and both codec round-trip lists). Recoverable, not a disconnect — the frame was well-formed.
  • The whole batch, not the offending op: the ack frontier is a max over the batch, so acking the admissible half would acknowledge the refused op's sequence with it.
  • Hub::ingest_records + ingest_branch filter independently — apply_replicate reaches Hub::ingest from a peer's Replicate frame without crossing handle_ops.
  • The branch-tail restore path (install_room) filters too: it bypasses ingest_branch and installs a stored tail verbatim, and the bytes come from whoever hands the store over.
  • The branch tail matters on its own: a tail folds into a document only at materialization, so an inadmissible op there sits durable and undetected until the fold.
  • replay::canonical_tail takes 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 across client.rs / FFI / wasm / JS / Python / Go. The public predicate makes "never" answerable at any seam that cares, and client.rs advancing last_seen_seq past 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.
  • Miri: -p crdtsync-server --test refused_op --test replay --test persistence and -p crdtsync-core --test state_doc --test transaction --test protocol --test lamport_ceiling clean, exit 0.
  • Local cold review: three passes, findings addressed (vacuous buffered-op assertion, colliding op ids in the batch test, missing branch positive control, un-asserted Accepted frontier, replay tail mismatch, timeless-comment violations, and — from the final pass — the unfiltered branch-tail restore path).
  • Mutation sweep (--no-fail-fast, isolated git archive copy), 9 mutations, none zero; unmutated tree fails 0:
mutation tests failed
no ingress gate 4
no ingest_records filter 3
no branch-tail filter 1
gate filters the op instead of refusing the batch 4
is_admissible ignores the author check 4
is_admissible ignores the position check 11
is_admissible ignores the tx-count check 2
no branch-restore filter 1
no replay::canonical_tail filter 1
is_admissible refuses a held transaction member 1

Three 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.

…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.
@vieiralucas
vieiralucas requested a review from Copilot July 28, 2026 20:11

This comment was marked as outdated.

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.
@vieiralucas
vieiralucas requested a review from Copilot July 28, 2026 20:23

This comment was marked as outdated.

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.
@vieiralucas
vieiralucas requested a review from Copilot July 28, 2026 20:32

This comment was marked as outdated.

@vieiralucas
vieiralucas merged commit 4e7d1f3 into main Jul 28, 2026
18 checks passed
@vieiralucas
vieiralucas deleted the server/c19-refused-op-ack branch July 28, 2026 21:49
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.

2 participants