Skip to content

fix(common): stop the messages/DM prune-resend loop with retention horizons - #485

Merged
sanity merged 13 commits into
mainfrom
fix/messages-prune-resend-loop
Jul 25, 2026
Merged

fix(common): stop the messages/DM prune-resend loop with retention horizons#485
sanity merged 13 commits into
mainfrom
fix/messages-prune-resend-loop

Conversation

@sanity

@sanity sanity commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Problem

This is a CORRECTNESS fix, not a bandwidth fix. Deltas are 0.9% of the Freenet Official room's wire bytes; the other 99.15% is full-state sends. This PR operates only on deltas, so it does not address the bandwidth incident that prompted it, and it should not be merged in the belief that it will. The remedy for that lives in freenet-core's full-state fan-out, not in the room contract.

What it does fix is real and worth fixing on its own terms: a gossip exchange that never terminates. See "Why this is still worth merging" below.

Scope: this fixes the CAP-DRIVEN prune/resend loop only. It does NOT fix the other channels that share the same shape — see "Not fixed here".

ComposableState convergence assumes merge is a join: state grows toward a least upper bound, so delta eventually returns None and fan-out stops. Two collections in ChatRoomStateV1 break that by removing entries in apply_delta:

  • MessagesV1 drains the oldest messages over max_recent_messages (message.rs:230-233).
  • DirectMessagesV1 capped each ordered (sender, recipient) pair at MAX_DM_MESSAGES_PER_PAIR, first-come-wins.

delta was a pure "everything you don't have" set-difference and Summary carried no prune horizon, so a peer whose retained window differed from its neighbour's offered exactly the entries the neighbour discards. The receiver applied them, pruned straight back, and its summary never changed — so the sender, which cannot see the rejection, computed the identical payload again on the next fan-out. delta never returned None, so freenet-core's "empty delta → skip" path never fired.

How long it persists depends on whether the peers agree on max_recent_messages. An earlier draft of this description said the loop runs "forever" unconditionally. That is wrong and has been corrected:

  • Equal caps — each direction loops (the sender cannot see the rejection), but a bidirectional exchange heals the pair: the lagging peer learns the newer messages and its stale window prunes itself away. An 8-peer simulation converged in 16 sends. So under equal caps this is bounded waste, not a permanent loop.
  • Unequal caps — permanent non-healing requires the two peers to DISAGREE on max_recent_messages. max_recent_messages is per-room configuration read from parent_state, so a config update that has reached one peer and not another produces exactly this.

The regression tests drive only the direction that persists, which is why they are one-directional loop detectors rather than convergence tests.

Measured impact — small, and measured rather than inferred

Telemetry from broadcast_payload_mix on v0.2.106 (1,319 peers, 291 emitting, 7,621 rollup windows, segmented on service.version):

full-state delta
fleet-wide, share of wire bytes 95.58% 4.42%
Freenet Official room 99.15% 0.9%

The room figure is isolated to 204 windows where the room is provably the only contract with attributed full-state bytes. Full-state sends are ~254 KB each — matching the state size, and explaining the "~370 KB payload" reports — against ~881 B per delta send. The room accounts for 23.2 GB, 46.9% of all attributed full-state bytes network-wide.

This PR operates only on deltas. The cap-driven loop it fixes, and the ban-purge and BansV1 channels it does not, all live inside that 0.9%. Fixing it will not produce a visible traffic reduction.

And the outbound leg deliberately gets no horizon benefit at all. outbound_summary neutralises both horizons on the send path (see the fix note below), so an outgoing update is the plain id-set difference against last_synced_state. Because update_room_state_inner does not call sync_info.state_updated, that baseline stays stale after an inbound full state, and the next tick re-offers current-minus-baseline unfiltered. This matches main, so it is NOT a regression and it cannot loop — the baseline only ever grows toward the state — but it means the horizon's saving applies to the RECEIVE path only. The already-small delta share above should be read with that in mind rather than as an upper bound the horizon fully captures.

Withdrawn claims. Two earlier drafts of this description overstated the impact and neither figure should be repeated:

  1. A network-wide share-of-broadcast-work figure derived from a state_size-weighted proxy. state_size is the post-apply FULL state, logged whether a delta or a full state actually crossed the wire, so it structurally overweights any large-state contract and cannot be attributed to this bug at all.
  2. A "14.3% of sends / 16.3% of delta bytes" steady-state waste rate. That is the waste rate within deltas, so it must be scaled by the delta share: 16.3% of 0.9% is not a bandwidth win. Those numbers are retained below only as evidence the loop is real, never as an impact claim.

Why this is still worth merging

The defect is a non-terminating gossip exchange, and it is empirically reproduced against the live deployed contract: a 14,989-byte non-empty delta applied with the state hash IDENTICAL before and after, 5 rounds out of 5. The receiver accepts the payload, prunes it straight back, its summary is unchanged, and the sender recomputes the same bytes indefinitely. Within deltas the steady-state waste at the room's real cap of 50 measures 14.3% of sends and 16.3% of delta bytes — cited here as confirmation that the loop is live and sustained, not as a bandwidth argument.

A merge operation that never reaches a fixpoint is a correctness bug in a CRDT regardless of how many bytes it currently moves, and it is the kind that gets worse silently as caps and room sizes change. The fix is small, the mechanism is proven, and the test coverage is unusually strong (16 mutation-verified properties plus 14 scenario tests).

Open contradiction, tracked separately

83.1% of this room's full-state bytes are attributed to the is_delta_efficient gate refusing — yet the room measures 33,804 B summary against 251,887 B state (13.4%, well under the 50% threshold), and no constructible state trips it. That is unresolved and is being chased in freenet-core, not here. Recorded so the trail is not lost.

Approach

Publish the receiver's retention horizon in its summary, and have the sender filter to entries the receiver would actually keep.

  • MessagesV1::Summary becomes { message_ids: BTreeSet<MessageId>, horizon }. RetentionHorizon is Open (below cap), OldestRetained(key) (at cap), or Closed (max_recent_messages == 0 — reachable via a full-state PUT, since verify does not reject a zero cap the way apply_delta does).

  • DirectMessagesSummary gains sorted pair_horizons, one per at-capacity pair.

  • The DM per-pair cap becomes newest-N instead of first-come-wins. First-come was order-dependent, so two peers at the cap could hold different sets forever; it also silently dropped every later DM once a pair filled up, which was a user-visible bug on its own.

    User-visible behaviour change, intended but worth calling out: the old rule kept the FIRST 100 messages per ordered pair; the new one keeps the NEWEST 100. Any pair already sitting at 100 will therefore start evicting its OLDEST DMs on the first merge after migration. Users of such a pair will see old DMs disappear. This is the correct behaviour — a chat that silently discards everything you send after the hundredth message is worse — but it is a visible change, not a silent no-op. (Not currently reached in the Freenet Official room: its busiest pair holds 25.)

Why it terminates. The horizon is the minimum held key, published only at capacity, so it never over-states — a sender is never told to withhold something the receiver would have kept. Under-stating costs one round. Applying any offered entry pushes the peer over capacity, so the prune drops at least the horizon entry itself and the horizon strictly increases; a peer below capacity discards nothing and its id set only grows. Each exchange either grows a bounded set or strictly advances a bounded key.

max_recent_messages is read from parent_state, so this is per-room config, not a constant, and peers may legitimately differ.

Summaries stay deterministic (BTreeSet / sorted Vec, never HashMap/HashSet) per .claude/rules/contract-summary-determinism.md.

UI: merge can no longer take a sentinel parent

MessagesV1::summarize now reads parent_state, retiring the invariant room_synchronizer relied on to pass a cheap ChatRoomStateV1::default(). Under the sentinel it reads the DEFAULT cap (100) instead of the room's, advertises an open horizon, and re-opens the loop.

Replaced with merge_incoming_state, which unrolls merge so summarize gets the room's own state while apply_delta keeps the sentinel (the macro ignores its outer parent_state there) — so no clone is reintroduced and the #246 saving stands.

The old equivalence pin merge_with_default_sentinel_parent_matches_merge_with_self_clone_parent is replaced by merge_uses_room_state_as_parent_so_horizon_is_correct, which asserts on the delta, not the state: the final state is identical either way, so a state-equality assertion structurally cannot catch this regression.

That replacement covers the summarize leg ONLY. It drives merge_incoming_state on both sides of its assertion, and merge_incoming_state always passes the sentinel to apply_delta, so it structurally cannot detect the sentinel becoming unsafe. The apply_delta leg is pinned separately by apply_delta_ignores_its_outer_parent_state_so_the_sentinel_is_safe, which calls apply_delta directly — once with ChatRoomStateV1::default(), once with the room's own state — over a room with non-default max_members, max_recent_messages and a real ban, the exact parent_state fields the per-field apply_deltas read.

Outbound path: why the summary is the ENTIRE attack surface

compute_update_data computes an outgoing update as state.delta(baseline, params, baseline_summary), i.e. it feeds the device's OWN last_synced_state in as if it were a receiver. Two things make that safe once the horizons are neutralised, and both were verified field by field:

1. All NINE delta impls declare _parent_state UNUSED. Checked signature by signature across configuration, bans, members, member_info, secrets, recent_messages, direct_messages, upgrade, version. This matters because compute_update_data passes the BASELINE as parent_state — a delta that read it would be consuming a stale/self quantity that outbound_summary structurally could not fix, since outbound_summary only touches the summary. Nothing does, so the summary is provably the whole surface.

2. Only two of the nine summary fields are horizon-shaped. "Horizon-shaped" means the field makes the SENDER withhold something it holds and the receiver lacks. recent_messages.horizon and direct_messages.pair_horizons are the only two; every other field is a pure have-statement (an id set, a version, a signature map), which is safe — indeed required — to feed from the sender's own baseline, because that is exactly what makes the delta "what changed since I last synced". upgrade and version are inert on this path for independent reasons: strip_upgrade_pointer forces upgrade == None outbound, and StateVersion::delta returns None unconditionally.

outbound_summary is written as an exhaustive destructure of ChatRoomStateV1Summary rather than a struct-update spread, so adding a summary field is a compile error at exactly the site that must decide keep-or-clear. That is deliberate: MembersV1 and BansV1 are deferred below as having the same defect, so the follow-up that adds MembersSummary.horizon is planned — and a spread would have compiled clean while silently reintroducing this bug on a new field. Verified by simulating that follow-up: error[E0027]: pattern does not mention field.

Reachability of the outbound bug — narrower than first stated

An earlier revision of this description said a clock-skewed device would simply never send anything. That is corrected. The UI send path applies the composed message through room_data.room_state.apply_delta before any sync tick, and MessagesV1::apply_delta sorts by (time, id) and drains from the FRONT — so on a device already AT capacity, a message sorting below the local window is dropped LOCALLY at compose time and never reaches compute_update_data.

The reachable trigger needs current BELOW cap while baseline was AT cap publishing a real horizon: either max_recent_messages was raised so back-filled older messages are retained locally but still sort below the stale horizon, or post_apply_cleanup's ban/member sweep shrank the local set while last_synced_state holds the pre-sweep snapshot. The regression test models the first, and asserts current is within its own cap so the premise stays reachable.

The invariant it protects: the #[composable] macro takes _parent_state and shadows it with a per-field self.clone(). Its own comment calls that "ugly", which is a description and not a stability promise. If a freenet-scaffold bump ever forwarded the argument, both UI ingestion paths would merge every room against a DEFAULT state — members empty, so the author-must-be-a-member retain drops EVERY message. Silent, total, local message loss. The test also asserts that the sentinel-parent MessagesV1::apply_delta drops every message, so the equality above cannot pass vacuously.

Backward compatibility

The #[serde(default)] asymmetry is inert today but hazardous. DirectMessagesSummary::pair_horizons carries #[serde(default)]; MessagesSummary::horizon does not. That asymmetry cannot bite on the cross-generation path — ChatRoomStateV1Summary decodes all its fields together, so an old-shape summary fails on recent_messages long before the DM leniency is reached. What the default DOES buy is a silent failure mode: if pair_horizons ever goes missing for any reason it decodes as empty, which disables the DM horizon filter and quietly re-opens the resend loop with no error anywhere, whereas the messages side fails loudly. Recorded deliberately: the leniency is retained for forward-compatibility with a future summary that legitimately omits the field, and the silent-degradation risk is the price.

The summary shape change is safe because it re-keys the contract. Peers on the old WASM derive a different contract key and never exchange summaries with peers on the new one; within a key every peer runs identical code fetched by that key, so there is no mixed-shape window. State encoding is unchanged, so river#292 auto-migration re-PUTs existing state forward untouched. This is the deliberate reasoning — a rollout-compatible summary shape was not needed and would have added a legacy branch for no benefit.

Not fixed here (deliberate)

MembersV1 (remove_excess_members) and BansV1 (max_user_bans) have the same defect by construction. Excluded because:

  1. Their retention key depends on receiver-side state the sender cannot see — invite-chain depth against the receiver's member graph, enforcing-vs-inert against the receiver's member_info. A horizon is not expressible; they need a different design.
  2. They are downstream of this fix: post_apply_cleanup prunes members by recent-message authorship, so member churn follows message churn.
  3. Payloads are ~100 bytes against ~1.6 KB per message, and both caps (200 members / 10 bans) are rarely reached in the affected room.

Filed as a follow-up issue rather than stretched into this PR.

KNOWN OPEN GAP: receiver-side rejection is invisible to the sender (#490)

This PR does not fix this, and no retention horizon can. Tracked as #490; independently identified by the protocol reviewer as finding S2.

When a receiver rejects a payload in post_apply_cleanup's ban/membership sweep rather than in the cap prune, the rejection leaves no trace in its summary, so the sender can never learn it was refused. Reproduced by ban_sweep_reopening_the_horizon_still_loops_known_gap: R at 100/100 bans X, the sweep drops X's 40 messages, R falls to 60 and its horizon REGRESSES OldestRetained -> Open — so it invites MORE, not less. Two channels loop together, messages AND members, and every round is a byte-identical no-op for R.

A horizon governs only what the sender withholds; it cannot express "I refused this". Closing it needs the ban, or some rejection signal, to reach the sender — a protocol change, not a summary change.

That test deliberately asserts the broken behaviour, so the suite stays green and the defect is a CI-enforced named fact rather than a red test people learn to skip. If the gap is ever closed it fails and tells the reader to restore the terminating assertion.

KNOWN OPEN GAP: DM recipient-purges, same family (#490)

A third instance of the same mechanism, recorded on #490 rather than fixed here. A recipient purges a DM by publishing an AuthorizedRecipientPurges envelope, and apply_delta step 3 drops any held message whose token is tombstoned — but the summary publishes only purge_versions, not the tombstoned PurgeToken set (deliberately: the tokens are BLAKE3-derived so they cannot be signature-ground, and publishing the set would leak which messages were purged).

So a sender that has not seen the envelope re-offers the purged DM forever: it is absent from the receiver's signature set, and pair_horizons does not filter it because it was removed for being purged, not for being old. No horizon can cover it — a horizon is an ordering statement, and purge membership is deliberately unpublished. Bounded in practice, and a bidirectional exchange heals it (the envelope travels in advanced_purges); it is one-directional gossip that persists. Not covered by a test.

Testing

Scenario tests — common/tests/retention_loop_test.rs (14)

Asymmetric loop detectors for messages and DMs, horizon advance under displacement, back-fill for peers below capacity, idempotence, zero-cap, DM newest-N retention, and a CBOR round trip through the exact encode/decode summarize_state/get_state_delta perform. Added since the first draft:

Property tests — common/tests/retention_proptest.rs (16, new)

Generalises the scenario tests over randomly-generated divergent windows, on a fixed-seed runner so failures reproduce:

  • Delta termination — after a peer merges what a neighbour offered, the neighbour has nothing left to offer. For MessagesV1, DirectMessagesV1, and the whole ChatRoomStateV1 through merge_incoming_state plus the CBOR round trip. This is the property the bug violated.
  • Retention invariant — the merged result is EXACTLY the newest min(|union|, cap) by (time, id). Deliberately an exact set equality: a "fix" that raises or removes the cap fails it, and so does a horizon that over-states.
  • Commutativity, associativity, idempotence (self-merge, and re-merge producing neither state change nor traffic); the DM per-pair cap; summary byte-determinism under state-Vec reordering; max_recent_messages varied independently per peer, including 0.

Generators are biased at the failing shape — peers AT capacity with overlapping-but-different windows, ties in time broken by id, boundary entries, empty states — and the_generator_reaches_at_capacity_peers_with_divergent_windows asserts that bias holds rather than trusting it.

Mutation verification — 12 mutations, all 12 caught

# Mutation Caught by
M1 messages delta drops the horizon filter (the pre-fix set-difference) merging_a_peer_leaves_it_with_nothing_further_to_offer, re_merging_the_same_peer_changes_neither_state_nor_traffic, a_zero_cap_peer_retains_nothing_and_is_offered_nothing, whole_room_state_gossip_terminates_across_the_cbor_round_trip
M2 horizon never Open (published below capacity) a_merge_retains_exactly_the_newest_cap_entries_of_the_union, absorbing_two_peers_is_commutative, absorbing_two_peers_is_associative
M3 zero cap yields Open instead of Closed a_zero_cap_peer_..., merging_a_peer_leaves_it_..., re_merging_the_same_peer_...
M4 horizon by index rather than min (order-dependent) message_summary_bytes_are_independent_of_state_vec_order
M5 messages stop pruning (the WRONG "fix": raise/remove the cap) a_merge_retains_exactly_..., a_zero_cap_peer_..., absorbing_two_peers_is_commutative, absorbing_two_peers_is_associative, the_generator_reaches_...
M6 MessagesSummary.message_ids BTreeSet -> HashSet message_summary_bytes_..., whole_room_state_gossip_...
M7 DM delta drops the pair-horizon filter merging_a_dm_peer_leaves_it_..., whole_room_state_gossip_...
M8 DM per-pair trim keeps the OLDEST instead of the newest a_dm_merge_retains_exactly_..., absorbing_two_dm_peers_is_commutative, ..._is_associative, merging_a_dm_peer_..., whole_room_state_gossip_...
M9 DM summary publishes no pair horizons merging_a_dm_peer_leaves_it_..., whole_room_state_gossip_...
M10 pair_horizons returned unsorted (HashMap order leaks out) dm_summary_bytes_are_independent_of_state_vec_order
M11 DM per-pair cap back to first-come-wins (the pre-fix behaviour) a_dm_merge_retains_exactly_..., absorbing_two_dm_peers_is_commutative, ..._is_associative, merging_a_dm_peer_..., whole_room_state_gossip_...
M12 DM per-pair cap removed entirely a_dm_merge_retains_exactly_..., absorbing_two_dm_peers_is_commutative, ..._is_associative

Two results worth calling out:

  • M2 is caught only by the exact-equality retention invariant and the algebraic laws — never by delta termination. An over-stating horizon suppresses more, so the delta empties sooner. Had the retention property been written as a subset check rather than exact set equality, M2 would have walked straight through.
  • M10 is caught only by the new property, not by the existing direct_messages_summary_serialization_is_order_independent, which constructs the summary by hand and so never exercises pair_horizons()'s internal HashMap.

Mutation testing changed the tests, twice more than the first draft recorded. Two originally-written bidirectional merge fixpoint tests turned out to be unable to fail and were rewritten as one-directional loop detectors. Then, on the property suite: a uniform-subset generator reached the 100-message DM cap in 5 of 512 cases, making every DM property near-decorative — fixed with a capacity-biased generator, with the bias now asserted. And dm_summary_bytes_are_independent_of_state_vec_order originally loaded one pair, but pair_horizons only emits an entry per at-capacity pair and a one-element Vec has no order to get wrong; it now loads both pairs, which is the only reason M10 is caught.

CI coverage gap — closed UPSTREAM, not here

An earlier revision of this description claimed this PR closed a CI gap. That claim is corrected: origin/main already closed it.

The finding was real when made — build.yml enumerated river-core test targets one at a time, cargo test -p river-core --lib does not build integration targets, and the enumeration covered only 7 of 14, so convergence_tests, direct_messages_test, deputy_ban_test, private_room_test, import_merge_test, memberid_test, stdlib_compat_test and this PR's own retention_loop_test never ran. But #481 (9a988954) independently added cargo test -p river-core --tests, and this branch's merge-base predated it.

After rebasing, build.yml here is byte-identical to main — the rebase would otherwise have left a duplicate --tests step running the slowest job twice. The retention suite is CI-gated, just not by this PR.

Migration

The room contract re-keys. The chat delegate deliberately does NOT.

An earlier revision of this branch re-keyed both, registering a V29 entry in each registry. The delegate half has been reverted, because that re-key was pure collateral:

  • Summary, merge, summarize and apply_delta appear in delegate source only inside comments — the delegate never summarises or merges room state.
  • The one contract-state type it does use, ChatRoomStateV1Delta, is unchanged: both type Delta declarations are identical to origin/main, and DirectMessagesDelta is byte-identical. This PR changed the SUMMARY types, not the DELTA types.
  • The delegate WASM had moved for codegen reasons alone, with no behavioural difference.

A V29 delegate entry would nonetheless push every user through a legacy-delegate migration — the single highest data-loss-risk step in a River release (the March 2026 incident in wasm-safety.md; and old delegate WASM built against stdlib <0.1.34 cannot execute on the current runtime). Taking that risk for zero benefit is a bad trade, so ui/public/contracts/chat_delegate.wasm is restored to origin/main's bytes, the V29 delegate entry is dropped, and the legacy_set_fingerprint pin reverts to b2824be852437587. The delegate key is unchanged from main, so nobody migrates.

Both --ci origin/main HEAD gates confirm the split:

check-migration.sh            → "Committed WASM unchanged — no migration needed."
check-room-contract-migration → "Old hash found in common/legacy_room_contracts.toml"

Read that green delegate gate accurately — it does not validate this revert's premise. scripts/check-migration.sh compares COMMITTED bytes at base vs head and never rebuilds anything. So it confirms the committed delegate is unchanged from main, which is the property users care about (no re-key, no migration), but it is structurally incapable of noticing that the committed delegate WASM is no longer this branch's build product. That gap is real and deliberate: a fresh --locked build of the delegate on this branch yields f3848929…, not the committed 2f8c5f1d…. The justification for shipping main's bytes is the source-level argument above (the delegate never summarises or merges, and ChatRoomStateV1Delta is unchanged), not the green check.

Consequence for whoever publishes: scripts/sync-wasm.sh copies BOTH WASMs unconditionally. This branch ran sync-wasm for the room contract and then hand-reverted the delegate half, so re-running cargo make sync-wasm here would silently re-key the delegate again with no V29 delegate entry present. The ordinary publish path is safe (include_bytes! reads the committed file; build-chat-delegate writes only to target/), and a committed re-copy would trip the gate — but do not run sync-wasm on this branch.

Final committed hashes (these are the bytes that will publish):

artifact BLAKE3 re-keys?
room_contract.wasm f8cca7600a63dac16de1974e08211e3eb6e530713a8cfe78caed3a66372a3e50 YES — V29 registered
chat_delegate.wasm 2f8c5f1d5c517e57208538fb2a7ec819e882eafa29bc43047eb6c025b37eba8e NO — identical to main

No V30 entry. A migration entry records the hash you migrate FROM. V29 pins origin/main's hashes (room 5ae5930a..., delegate 2f8c5f1d...) — the generation live rooms actually sit on — so the new hash moving cannot invalidate it. The intermediate WASM this branch carried before the version bump (2a782ca8... / c281c2c7...) was never published, so it is replaced rather than registered; registering an unpublished intermediate would pollute the registry with a generation no room ever used.

verify() is untouched, asserted byte-identical to origin/main in both message.rs (29 lines) and direct_messages.rs (115 lines). This is load-bearing: the forward-migration PUT is gated on validate_state = verify alone, so tightening it would make existing state REJECTED and lose rooms.

river-core 0.1.18 (version bump)

common/ changed but [workspace.package] version was still 0.1.17, which is already on crates.io. cli/Cargo.toml declared river-core = { version = "0.1.17", path = "../common" }, so a published riverctl 0.2.4 would resolve river-core from crates.io — the OLD summary shape, no RetentionHorizon, no pair_horizons — while embedding the NEW room-contract WASM.

Independently of any runtime failure, publishing a crate whose resolved dependency differs from the source it was built against is wrong, so the bump is hygiene rather than a bet on a failure mode. That a stale river-core would actually fail at runtime is reasoned, not proven: the two summary types gained non-Option fields on the decode path, and the v0.2.11 incident is the precedent. An end-to-end demonstration was deliberately skipped as it could not change the decision.

Measured, not assumed: building the key-defining WASMs with --locked at 0.1.17 reproduced the committed bytes exactly, and at 0.1.18 moved both (river-core's version feeds -C metadata for both crates). cargo make sync-wasm then reproduced the 0.1.18 hashes a third time.

riverctl's river-core requirement is pinned exactly

cli/Cargo.toml now reads river-core = { version = "=0.1.18", ... }, not "0.1.18" (which is ^0.1.18). cargo install re-resolves dependencies by default — it does not use the packaged lockfile without --locked — so under a caret range a fresh cargo install riverctl@0.2.4 performed after river-core 0.1.19 ships would link 0.1.19 against the 0.1.18-era bundled WASM. That is the same v0.2.11-class skew this version bump exists to prevent, merely deferred by one release.

Known gap in the check-wasm-sync gate

check-wasm-sync compares cli/Cargo.toml's version against crates.io and nothing else. A version bump alone fully satisfies it while the underlying river-core requirement stays stale — which is exactly the case that would have shipped this break. A follow-up should make it also compare the river-core requirement against [workspace.package] version.

No room recreation, gkapi change, secret rotation, or invite reissue — all key off the unchanged owner VK. Clients auto-migrate on refresh (river#292).

Do not publish from this branch

Publishing is sequenced separately by the River master agent; this re-keys the live room.

[AI-assisted - Claude]

sanity added a commit that referenced this pull request Jul 25, 2026
…horizon

Test-validity review findings on #485.

**The DM leg of `outbound_summary` was completely unpinned.** The outbound
horizon fix landed with a test asserting only on `delta.recent_messages`, so
deleting `summary.direct_messages.pair_horizons.clear()` left every test in
the repo green while a clock-skewed device's DM to an at-capacity pair was
silently dropped before the wire — the exact failure class just fixed for
messages, one line away. Added
`outbound_update_is_not_filtered_by_the_senders_own_dm_pair_horizon`;
mutation-verified (deleting the line makes it fail at the `.expect`, i.e.
`compute_update_data` returns `None` outright).

**Four client-side per-pair cap guards still enforced first-come-wins**, each
justified in-comment by "the contract silently drops overflow" — which this
branch changes to newest-N. Left in place, the user still could not send at
the cap, now for a purely client-side reason, and riverctl 0.2.4 would ship
that block against the new contract. Removed from all six sites (the review
named four; `dm_thread_modal` had a second in-write-lock re-check and its
error arm), along with the now-unreachable `SendDmOutcome::CapHit` and
`ApplyOutcome::CapHit` variants and the "This thread is full" copy.

Pinned by `dm_send_has_no_client_side_pair_cap_guard`. **That pin was vacuous
when first written** — scoped to `execute_send` while the guard actually lived
in `deliver_dm` a few hundred lines away, so it passed under mutation. Now
scoped to the whole non-test body, and mutation-verified failing.

**`merging_a_dm_peer_with_itself_is_the_identity` was structurally vacuous.**
A self-merge yields `delta == None` and `apply_delta` early-returns before
`trim_pairs_to_cap`, so it could not fail for any mutation of the trim or the
pair horizon. Labelled honestly (its messages-side twin already was), and the
real property it was standing in for is now added:
`re_merging_the_same_dm_peer_changes_neither_state_nor_traffic`, whose first
merge carries a genuine non-empty delta.

**`apply_delta_silently_drops_per_pair_overflow` misdescribed the code.** It
asserted only `len == MAX`, so it stayed green across first-come-wins ->
newest-N while its name and failure message asserted the opposite of the new
behaviour. Renamed to
`apply_delta_evicts_the_oldest_to_admit_a_newer_message_at_the_pair_cap` and
now asserts on the SET — the newcomer present, the displaced oldest gone.

**Proptest fixture keys are seeded.** `build_fixture` used
`SigningKey::generate(&mut OsRng)`, so `MessageId` (a hash of the signature)
and `MemberId` were random per run — meaning the `(time, id)` tiebreak that
`MSG_PER_TIMESTAMP = 2` exists to exercise, and `DmPairHorizon` ordering, were
not reproducible, and the module doc's "a failure reproduces by re-running"
was false with `failure_persistence: None`. Now fixed seeds, matching
`summary_determinism_test.rs`.

**CI workflow reverted to main's.** `origin/main` already added
`cargo test -p river-core --tests` in #481; this branch's merge-base predated
it, so the gap was closed upstream, not here. The rebase produced a duplicate
`--tests` step (~72s twice); removed, leaving build.yml byte-identical to main.

Room-contract WASM rebuilt on the rebased source and reproduces
`f8cca760...` — main's `content.rs` change since the merge-base is
doc-comment-only, so codegen is unaffected. Delegate stays at main's
`2f8c5f1d...`; both `--ci origin/main HEAD` gates still report
delegate-unchanged / room-contract-entry-exists.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018Y5evTaWMyBARkuw8JgAVh
@sanity
sanity force-pushed the fix/messages-prune-resend-loop branch from 8f5c4db to 947cb10 Compare July 25, 2026 19:27
sanity and others added 10 commits July 25, 2026 14:31
…rizons

## Problem

`ComposableState` convergence assumes `merge` is a join: state grows toward a
least upper bound, so `delta` eventually returns `None` and fan-out stops. Two
collections in `ChatRoomStateV1` break that by REMOVING entries in `apply_delta`:

- `MessagesV1` drains the oldest messages over `max_recent_messages`.
- `DirectMessagesV1` capped each ordered `(sender, recipient)` pair at
  `MAX_DM_MESSAGES_PER_PAIR`, first-come-wins.

`delta` was a pure "everything you don't have" set-difference and `Summary`
carried no prune horizon, so a peer whose retained window differed from its
neighbour's offered exactly the entries the neighbour discards. The receiver
applied them, pruned straight back, and its summary never changed — so the
sender, which cannot see the rejection, computed the identical payload again on
every fan-out. `delta` never returned `None`, so freenet-core's "empty delta ->
skip" path never fired.

The loop is asymmetric and does not self-heal: in a quiet room the lagging peer
receives nothing, so its stale window never ages out.

Measured on 2026-07-25: the Freenet Official room reached 63.7% of all
byte-weighted broadcast work network-wide (1.2% a day earlier); median per-peer
rate 72 KB/s -> 398 KB/s in 24h; 59 of 497 peers above 10 GB/hour.

## Approach

Publish the receiver's retention horizon in its summary, and have the sender
filter to entries the receiver would actually keep.

- `MessagesV1::Summary` becomes `{ message_ids: BTreeSet<MessageId>, horizon }`,
  where `RetentionHorizon` is `Open` (below cap), `OldestRetained(key)` (at cap)
  or `Closed` (`max_recent_messages == 0`, reachable via a full-state PUT since
  `verify` does not reject a zero cap).
- `DirectMessagesSummary` gains sorted `pair_horizons`, one per at-capacity pair.
- The DM per-pair cap becomes newest-N instead of first-come-wins. First-come was
  order-dependent (so two peers at the cap could hold different sets forever) and
  also silently dropped every later DM once a pair filled up.

Horizons are the MINIMUM held key, published only at capacity, so they never
over-state: a sender is never told to withhold something the receiver would have
kept. Under-stating only costs a round, and terminates — applying any offered
entry pushes the peer over capacity, so the prune drops at least the horizon
entry itself and the horizon strictly increases.

`max_recent_messages` is read from `parent_state`, so this is per-room config,
not a constant, and peers may legitimately differ.

Summaries stay deterministic (`BTreeSet` / sorted `Vec`, no `HashMap`/`HashSet`)
per .claude/rules/contract-summary-determinism.md.

### UI: `merge` no longer takes a sentinel parent

`MessagesV1::summarize` now READS `parent_state`, which retires the invariant
`room_synchronizer` relied on to pass a cheap `ChatRoomStateV1::default()`. Under
the sentinel it would read the DEFAULT cap instead of the room's and re-open the
loop. Replaced with `merge_incoming_state`, which unrolls `merge` so `summarize`
gets the room's own state while `apply_delta` keeps the sentinel (the macro
ignores it there), so no clone is reintroduced. The old equivalence pin is
replaced by one that asserts on the DELTA — the final state is identical either
way, so a state-equality assertion cannot catch this.

### Backward compatibility

The summary shape change is safe because it re-keys the contract: peers on the
old WASM derive a different contract key and never exchange summaries with peers
on the new one. Within a key, every peer runs identical code fetched by that key.
State encoding is unchanged, so river#292 auto-migration re-PUTs existing state
forward untouched.

## Not fixed here

`MembersV1` (`remove_excess_members`) and `BansV1` (`max_user_bans`) have the
same defect by construction. They are excluded deliberately: their retention key
depends on receiver-side state the sender cannot see (invite-chain depth against
the receiver's member graph; enforcing-vs-inert against the receiver's
`member_info`), so they need a different design rather than a horizon. Both are
also downstream of this fix — `post_apply_cleanup` prunes members by recent-message
authorship, so member churn follows message churn — and their payloads are ~100
bytes against ~1.6 KB per message. Filed separately.

## Testing

`common/tests/retention_loop_test.rs` (12 tests): loop detectors driving only the
asymmetric direction that persists, horizon advance under displacement,
back-fill for peers below capacity, idempotence, zero-cap, and a CBOR round trip
through the exact encode/decode the contract does.

Every test was mutation-verified — 8 mutations, each reverting one piece of the
fix, all caught. Two tests were rewritten after mutation testing showed they
could not fail: a bidirectional exchange heals the pair on its own, so it pins
convergence, not the loop.

## Migration

Room-contract and chat-delegate WASMs both re-keyed; outgoing hashes registered
as V29 in `common/legacy_room_contracts.toml` and `legacy_delegates.toml` BEFORE
the rebuild. `check-room-contract-migration` and `check-migration` both pass.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HP4Xk5kf2FujRiFq7qye38
The pin keys the per-user "migration done" localStorage flag and is derived
from `LEGACY_DELEGATES`'s exact contents and order, so it must move whenever a
genuine new legacy entry is registered — which V29 (the pre-retention-horizon
delegate generation) is.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HP4Xk5kf2FujRiFq7qye38
…tract

The room-contract WASM changed in this branch, so `check-wasm-sync` fails
while `cli/Cargo.toml` still matches the published 0.2.3 on crates.io.
riverctl embeds the WASM to derive the contract key, so shipping new WASM
in the UI without a corresponding riverctl release makes the two target
different contracts (the Feb 2026 incident).

0.2.4 is not yet on crates.io (max published is 0.2.3).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018Y5evTaWMyBARkuw8JgAVh
Generalises the hand-written regression tests in `retention_loop_test.rs`
over randomly-generated divergent retained windows.

16 properties across `MessagesV1`, `DirectMessagesV1` and the whole
`ChatRoomStateV1`:

* Delta termination (the property the bug violated): after a peer merges
  what a neighbour offered, the neighbour must have nothing left to offer.
  Also end-to-end through `merge_incoming_state` and the CBOR round trip
  the contract performs.
* Retention invariant, as an EXACT equality against the newest
  `min(|union|, cap)` by `(time, id)` — so a "fix" that raises or removes
  the cap fails it, and a horizon that over-states fails it too.
* Commutativity, associativity, idempotence (self-merge, and re-merge
  producing neither state change nor traffic).
* The DM per-pair cap, which this branch changed from first-come-wins to
  newest-N.
* Summary byte-determinism under state-Vec reordering.
* `max_recent_messages` varied independently per peer, including 0.

The generators are deliberately biased at the failing shape: peers AT
capacity whose retained windows overlap but differ, ties in `time` broken
by id, boundary entries, and empty states.
`the_generator_reaches_at_capacity_peers_with_divergent_windows` asserts
that bias holds rather than trusting it — the first draft used a uniform
subset generator that reached the 100-message DM cap in 5 of 512 cases,
which would have made every DM property decorative.

Runs on a fixed-seed `TestRunner` so failures reproduce without a
persisted regressions file. proptest is added with `default-features =
false` (no `fork`/`timeout`) to keep Cargo.lock, which pins the
byte-reproducible WASM builds, minimal.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018Y5evTaWMyBARkuw8JgAVh
…op gap

Three coverage gaps from the protocol review, plus the CI wiring that was
letting the whole retention suite go unrun.

**Restore the apply_delta-leg sentinel pin (ui/room_synchronizer.rs).**
This branch deleted `merge_with_default_sentinel_parent_matches_merge_with_
self_clone_parent`. Its replacement drives `merge_incoming_state` on BOTH
sides of its assertion, and `merge_incoming_state` always passes the sentinel
to `apply_delta` — so it structurally cannot detect the sentinel becoming
unsafe; it pins the `summarize` leg only.

`apply_delta_ignores_its_outer_parent_state_so_the_sentinel_is_safe` calls
`apply_delta` directly, once with `ChatRoomStateV1::default()` and once with
the room's own state, over a room with non-default `max_members`,
`max_recent_messages` and a real ban — the exact `parent_state` fields the
per-field `apply_delta`s read. If a freenet-scaffold bump ever forwarded
`_parent_state`, both UI ingestion paths would merge against a DEFAULT state
(members empty, so the author-must-be-a-member retain drops EVERY message):
silent, total, local message loss.

It also proves it is not vacuous — `MessagesV1::apply_delta` under the
sentinel is asserted to drop every message, so the two parents are genuinely
distinguishable and the equality above is pinning the macro's behaviour
rather than an accident.

**Asymmetric caps, both directions.** Every existing test gave both peers one
cap. `max_recent_messages` is per-room config read from `parent_state`, so a
config update that reached one peer and not the other leaves peers at
different caps — which the PR body calls legitimate and nothing covered.
50-cap and 200-cap peers now exchange in both directions.

**Ban-sweep horizon regression: a REAL FINDING, pinned as it behaves.**
`ban_sweep_reopening_the_horizon_still_loops_known_gap` drives the FULL
`ChatRoomStateV1` merge so `post_apply_cleanup` runs: R at 100/100 bans X,
the sweep drops X's 40 messages, R falls to 60 and its horizon regresses
`OldestRetained` -> `Open`. Written first as `..._does_not_loop`, it FAILED.
The loop is real and the retention horizon cannot close it: an OPEN horizon
withholds nothing, and the messages R rejects are dropped by the membership
sweep, which leaves no trace in R's summary, so S recomputes the identical
payload forever. Two coupled channels loop, messages AND members. Root cause
is that R's ban never reaches S — a protocol gap, not a summary one.

Rather than weaken the assertion or leave the suite red, the test is INVERTED
into a characterisation test asserting the loop is present and each round is a
no-op for R. When the gap is closed it fails and forces the polarity back.

**CI.** `build.yml` enumerates river-core test targets one at a time and
`--lib` does not build integration targets, so `retention_loop_test` (this
branch's own regression suite), `retention_proptest` and
`summary_determinism_test` had ZERO CI coverage. Added a step running all
three.

Also caps proptest shrinking at 30s wall-clock: a failing DM property shrank
for over ten minutes unbounded, which on CI reads as a hung job.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018Y5evTaWMyBARkuw8JgAVh
… in CI

**river-core 0.1.18.** `common/` changed on this branch, but the workspace
version was still 0.1.17 — which is already on crates.io. `cli/Cargo.toml`
declares `river-core = { version = "0.1.17", path = "../common" }`, so a
published riverctl 0.2.4 would resolve river-core FROM crates.io: the OLD
summary shape, with no `RetentionHorizon` and no `pair_horizons`, against the
NEW embedded room-contract WASM. That is the v0.2.11 wire-format failure class.
Independently of any runtime failure, publishing a crate whose resolved
dependency differs from the source it was built against is wrong.

Measured, not assumed: building the key-defining WASMs with `--locked` at
0.1.17 reproduced the committed bytes EXACTLY, and at 0.1.18 moved both
(river-core's version feeds `-C metadata` for both crates):

  room_contract  2a782ca8... -> f8cca760...
  chat_delegate  c281c2c7... -> f3848929...

`cargo make sync-wasm` then reproduced those two hashes a third time, so the
`--locked` + pinned-toolchain reproducibility guarantee holds.

**No new migration entry.** A migration entry records the hash you migrate
FROM. V29 pins `origin/main`'s hashes (room `5ae5930a...`, delegate
`2f8c5f1d...`) — the generation live rooms actually sit on — so the new hash
moving cannot invalidate it. The intermediate `2a782ca8...` / `c281c2c7...`
WASM was never published to the network, so it is replaced rather than
registered; registering an unpublished intermediate would pollute the registry
with a generation no room ever used.

**`verify()` is untouched** — asserted byte-identical to `origin/main` in both
`message.rs` (29 lines) and `direct_messages.rs` (115 lines). The forward
migration PUT is gated on `validate_state` = `verify` alone, so tightening it
would make old state REJECTED and lose rooms.

**CI: run every river-core test target.** The previous step enumerated three
targets by name. The enumeration this job already used had drifted to cover
only 7 of 14: `convergence_tests` (the CRDT convergence suite),
`direct_messages_test`, `deputy_ban_test`, `private_room_test`,
`import_merge_test`, `memberid_test` and `stdlib_compat_test` had NO CI
coverage at all. `cargo test -p river-core --tests` builds and runs all of
them plus the lib target, so a new file under `common/tests/` is covered the
moment it is added, with no workflow edit. All 14 pass in ~72s (~64s of which
is `retention_proptest`). The enumerated steps above it are kept: a named
failing step attributes a breakage at a glance, and they cost ~0.1s each.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018Y5evTaWMyBARkuw8JgAVh
The test asserts a KNOWN OPEN defect rather than correct behaviour, so it
needs to say where the analysis lives instead of re-deriving it inline. The
panic message now also tells whoever closes the gap to close the issue.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018Y5evTaWMyBARkuw8JgAVh
…n horizon

Plus: revert the collateral delegate re-key, and pin river-core exactly.

**The bug.** `compute_update_data` built its baseline summary with
`baseline.summarize(baseline, params)`. `baseline` is `last_synced_state` —
this device's own snapshot, NOT a receiver. A retention horizon is a
RECEIVER-published quantity ("do not offer me what I would discard"), so
using the sender's own filtered the device's outgoing update against its own
retention window.

`message.time` is the browser wall clock, so a device whose clock is behind by
more than the room's retention window composes messages sorting at or below
its own baseline's oldest retained key. Those were filtered out of the delta
and never reached the wire. `compute_update_data` then returned `None`, which
in `process_rooms` still calls `sync_info.state_updated(..)` and advances the
baseline — so it was never retried. The message sits in the sender's own UI
forever while no other peer ever receives it: silent, local, permanent loss.

Scoped honestly: pre-PR the message went out and the contract's cap-prune
often discarded it anyway, so the network-visible outcome was frequently the
same. The regression is that it is now dropped BEFORE the wire — losing the
case where canonical state is below cap and would have KEPT it — and that the
failure is now silent and purely local.

Fixed by `outbound_summary`, which neutralises both horizons (messages and DM
pair horizons) on the outbound path, restoring the pure id-set difference and
leaving retention to the contract, the only party that knows canonical state
and the room's real cap. The horizon still does its job on the receive path.

Pinned by `outbound_update_is_not_filtered_by_the_senders_own_horizon`, which
also asserts the fix does NOT turn the delta into a full resend.
Mutation-verified: restoring `baseline.summarize(..)` makes it fail with
`compute_update_data` returning `None` outright — the silent-loss path itself.

**Delegate re-key reverted.** The delegate re-key in this branch was pure
collateral: verified independently that `Summary`, `merge`, `summarize` and
`apply_delta` appear in delegate source only inside COMMENTS, and that the one
contract-state type it does use, `ChatRoomStateV1Delta`, is unchanged — both
`type Delta` declarations are identical to origin/main and `DirectMessagesDelta`
is byte-identical. The WASM had moved for codegen reasons alone.

Yet the V29 delegate entry would force EVERY user through a legacy-delegate
migration, the highest data-loss-risk step in a River release (the March 2026
incident; plus old delegate WASM built against stdlib <0.1.34 cannot execute on
the new runtime). That risk for zero benefit.

So `ui/public/contracts/chat_delegate.wasm` is restored to origin/main's bytes
(`2f8c5f1d...`), the V29 entry is dropped from `legacy_delegates.toml`, and the
`legacy_set_fingerprint` pin reverts to `b2824be852437587`. The delegate key is
now unchanged from main, so no user migrates. `check-migration.sh` lists
"revert the WASM change" as a valid resolution and no CI job rebuilds-and-
compares the delegate.

**riverctl's river-core dependency pinned to `=0.1.18`.** It was `^0.1.18`, and
`cargo install` re-resolves by default (it does not use the packaged lock
without `--locked`). Once river-core 0.1.19 ships with another shape change, a
fresh `cargo install riverctl@0.2.4` would link 0.1.19 against the 0.1.18-era
bundled WASM — the same v0.2.11-class skew the version bump exists to prevent,
just deferred a release.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018Y5evTaWMyBARkuw8JgAVh
…horizon

Test-validity review findings on #485.

**The DM leg of `outbound_summary` was completely unpinned.** The outbound
horizon fix landed with a test asserting only on `delta.recent_messages`, so
deleting `summary.direct_messages.pair_horizons.clear()` left every test in
the repo green while a clock-skewed device's DM to an at-capacity pair was
silently dropped before the wire — the exact failure class just fixed for
messages, one line away. Added
`outbound_update_is_not_filtered_by_the_senders_own_dm_pair_horizon`;
mutation-verified (deleting the line makes it fail at the `.expect`, i.e.
`compute_update_data` returns `None` outright).

**Four client-side per-pair cap guards still enforced first-come-wins**, each
justified in-comment by "the contract silently drops overflow" — which this
branch changes to newest-N. Left in place, the user still could not send at
the cap, now for a purely client-side reason, and riverctl 0.2.4 would ship
that block against the new contract. Removed from all six sites (the review
named four; `dm_thread_modal` had a second in-write-lock re-check and its
error arm), along with the now-unreachable `SendDmOutcome::CapHit` and
`ApplyOutcome::CapHit` variants and the "This thread is full" copy.

Pinned by `dm_send_has_no_client_side_pair_cap_guard`. **That pin was vacuous
when first written** — scoped to `execute_send` while the guard actually lived
in `deliver_dm` a few hundred lines away, so it passed under mutation. Now
scoped to the whole non-test body, and mutation-verified failing.

**`merging_a_dm_peer_with_itself_is_the_identity` was structurally vacuous.**
A self-merge yields `delta == None` and `apply_delta` early-returns before
`trim_pairs_to_cap`, so it could not fail for any mutation of the trim or the
pair horizon. Labelled honestly (its messages-side twin already was), and the
real property it was standing in for is now added:
`re_merging_the_same_dm_peer_changes_neither_state_nor_traffic`, whose first
merge carries a genuine non-empty delta.

**`apply_delta_silently_drops_per_pair_overflow` misdescribed the code.** It
asserted only `len == MAX`, so it stayed green across first-come-wins ->
newest-N while its name and failure message asserted the opposite of the new
behaviour. Renamed to
`apply_delta_evicts_the_oldest_to_admit_a_newer_message_at_the_pair_cap` and
now asserts on the SET — the newcomer present, the displaced oldest gone.

**Proptest fixture keys are seeded.** `build_fixture` used
`SigningKey::generate(&mut OsRng)`, so `MessageId` (a hash of the signature)
and `MemberId` were random per run — meaning the `(time, id)` tiebreak that
`MSG_PER_TIMESTAMP = 2` exists to exercise, and `DmPairHorizon` ordering, were
not reproducible, and the module doc's "a failure reproduces by re-running"
was false with `failure_persistence: None`. Now fixed seeds, matching
`summary_determinism_test.rs`.

**CI workflow reverted to main's.** `origin/main` already added
`cargo test -p river-core --tests` in #481; this branch's merge-base predated
it, so the gap was closed upstream, not here. The rebase produced a duplicate
`--tests` step (~72s twice); removed, leaving build.yml byte-identical to main.

Room-contract WASM rebuilt on the rebased source and reproduces
`f8cca760...` — main's `content.rs` change since the merge-base is
doc-comment-only, so codegen is unaffected. Delegate stays at main's
`2f8c5f1d...`; both `--ci origin/main HEAD` gates still report
delegate-unchanged / room-contract-entry-exists.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018Y5evTaWMyBARkuw8JgAVh
Strengthens the two `outbound_summary` pins against the failure mode that
matters most for that fix: neutralising the retention horizon must not widen
an update into a resend of already-synced state.

Both tests previously carried ONE new entry and asserted `len == 1`. A count
against a single new entry is weak evidence — it happens to discriminate here,
but it reads as a 1-vs-N check rather than a set comparison. Now each test
composes several new entries and asserts an EXACT set equality against the
ids/signatures the baseline lacks, so it fails in both directions: too few
(something still filters the send path) and too many (the id-set difference
stopped applying).

Mutation-verified in the resend direction, which is new coverage: making
`outbound_summary` additionally clear `message_ids` / `message_signatures`
now fails both tests with the exact-set assertion. Previously that mutation
was caught only incidentally by the count.

The structural reason the fix is safe is unchanged and worth stating:
`outbound_summary` clears ONLY the horizons and keeps `message_ids`,
`message_signatures` and `purge_versions`, so `delta` still performs the
id/signature-set difference. Removing an additional filter can at most widen
the payload back to that set difference — which is bounded by what the
baseline lacks, and is exactly the pre-horizon behaviour. A full resend would
require the id sets to be cleared too, which is what the new mutation
simulates.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018Y5evTaWMyBARkuw8JgAVh
@sanity
sanity force-pushed the fix/messages-prune-resend-loop branch from 947cb10 to 2934929 Compare July 25, 2026 19:35
sanity and others added 3 commits July 25, 2026 14:50
…ame #485

Delta re-review should-fixes.

**S1 — the two UI guard-removal sites had no pin at all.** Only the CLI
removal was pinned, and my first attempt at THAT pin was vacuous (scoped to
`execute_send` while the guard lived in `deliver_dm`). The same gap class was
left wide open on the two files where nearly all users are: a contributor
re-adding a `pair_message_count(..) >= MAX_DM_MESSAGES_PER_PAIR` early return
to `dm_thread_modal::do_send` would keep every test green while the UI
silently reverted to blocking at 100, making the newest-N contract change
invisible to the population that matters.

Added the same `include_str!` + cut-at-`mod tests` scrape to both files, with
three deliberate differences from the CLI pin:

* Scoped to the WHOLE non-test body, never one function — scope-by-function is
  exactly how the CLI pin went vacuous.
* Cut point VERIFIED per file rather than assumed: each has exactly one
  `mod tests` and it is the real module. That is a property of these files,
  not a guarantee of the technique, and the comment says so.
* Keyed on BOTH `pair_message_count(` and `MAX_DM_MESSAGES_PER_PAIR`. Neither
  has a legitimate non-test use left in either file, so this ALSO catches a
  hand-rolled `.filter(..).count() >= MAX_DM_MESSAGES_PER_PAIR` — the variant
  the review noted a symbol-only pin would miss. The CLI pin cannot do this;
  it uses the constant legitimately for outbound-cache pruning.

Both mutation-verified, including the review's exact regression scenario
(guard re-added to `do_send` with the "This thread is full" copy) and the
hand-rolled-count variant. Residual limitation documented in-comment: a guard
inlining the literal 100 still slips past.

**S2** — `dm_thread_modal.rs` still imported `MAX_DM_MESSAGES_PER_PAIR` after
the guard removal; only `MAX_DM_CIPHERTEXT_BYTES` survives. It warned on every
river-ui build including the publish build, and with `clippy.yml` disabled and
no `-D warnings` anywhere it would have sat there indefinitely.

**N3** — `common/legacy_room_contracts.toml` shipped a literal `#TBD` in an
append-only registry description. Filled in as #485.

Verified the N3 registry edit does NOT move the room-contract WASM: a fresh
`--locked` build into a scratch target dir reproduces `f8cca760...` (fifth
independent reproduction). Built to scratch and hashed WITHOUT copying —
`sync-wasm` copies both WASMs unconditionally and would have silently re-keyed
the hand-reverted delegate. Committed WASMs are untouched.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018Y5evTaWMyBARkuw8JgAVh
…rt the whole delta

Round-2 re-review findings 2 and 3.

**Finding 3 — `outbound_summary` is now an exhaustive destructure.** It was a
hand-maintained mirror of "which summary fields are horizon-shaped", with no
structural link to the horizon definitions. This PR explicitly defers
`MembersV1` and `BansV1` as having the same non-monotonic defect, so the
follow-up that adds `MembersSummary.horizon` is planned — and nothing would
have forced it to extend this function. A struct-update spread compiles clean
and silently reintroduces the exact bug just fixed, on a new field: a device
withholding its own member records from every outgoing update, baseline
advancing anyway, never retried.

Binding all nine fields by name makes adding a summary field a COMPILE ERROR at
exactly the site that must make the keep-or-clear decision. Verified by
simulating the follow-up (adding a tenth composable field):
`error[E0027]: pattern does not mention field ...`, reported at the destructure.

**Finding 2 — the exact-set-equality assertions were PER-FIELD.** Both tests
read one field of the decoded delta, so over-clearing an UNRELATED field in
`outbound_summary` — e.g. `bans` or `members`, making every sync tick re-send
the full list forever — passed both. Now each test asserts the whole delta is
EXACTLY the one field under test, cross-wise included.

The first version of that assertion was VACUOUS: `create_test_room()` leaves
`members` and `bans` empty, so clearing an empty ban summary yields no ban
delta and the assertion passed under mutation. Added `add_members_and_bans` so
the fixture has something to bite on. Both over-clear mutations are now caught,
in both tests:
  left: ["bans", "recent_messages"]  right: ["recent_messages"]

**Also verified for the PR body:** all NINE `delta` impls declare
`_parent_state` unused (checked signature by signature across the nine
`common/src/room_state/*.rs` files). That is what makes the summary provably the
entire outbound attack surface — `compute_update_data` passes the BASELINE as
`parent_state`, so a `delta` that read it would be consuming a stale/self
quantity that `outbound_summary` structurally could not fix, since it only
touches the summary.

Committed WASMs untouched; `sync-wasm` deliberately not run.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018Y5evTaWMyBARkuw8JgAVh
…ible state

Round-2 finding 4. The narrative in the fix commit — that a clock-skewed
device's message "sits in the sender's own UI forever while no other peer ever
receives it" — is WRONG, and worse, the test modelled a state that cannot exist.

Verified both halves of the correction directly:

* The UI send path applies the composed message through
  `room_data.room_state.apply_delta` before any sync tick
  (`conversation.rs:1751` and siblings).
* `MessagesV1::apply_delta` sorts by `(time, id)` and drains from the FRONT
  (`message.rs:337-340`).

So on a device already AT capacity, a message sorting below the local window is
dropped LOCALLY at compose time and never reaches `compute_update_data`. The
old test held 6 messages against a cap of 3 — `apply_delta` can never produce
that, so the test demonstrated a real bug from an unreachable premise, which is
the kind of thing a future reader falsifies in five minutes and then distrusts
the whole file for.

The reachable trigger needs `current` BELOW cap while `baseline` was AT cap
publishing a real horizon. Two routes: `max_recent_messages` raised so
back-filled older messages are retained locally but still sort below the stale
horizon; or `post_apply_cleanup`'s ban/member sweep shrinking the local set
while `last_synced_state` holds the pre-sweep snapshot.

The test now models the first: baseline at cap 3 with a real `OldestRetained`,
`current` with the cap raised to 10 holding a back-filled message below that
horizon, plus an explicit assertion that `current` is within its own cap so the
premise stays reachable. The configuration bump rides along in the delta, so
the whole-delta assertion expects `["configuration", "recent_messages"]` — which
is more honest than the previous single-field expectation anyway.

Re-verified the realistic premise costs no discrimination: reverting the fix
still fails it, and the full-resend mutation still fails the exact-set
assertion.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018Y5evTaWMyBARkuw8JgAVh
@sanity
sanity marked this pull request as ready for review July 25, 2026 20:20
@sanity
sanity merged commit c02462f into main Jul 25, 2026
7 checks passed
sanity added a commit that referenced this pull request Jul 25, 2026
Room-contract re-key (V29): 5ae5930a… -> f8cca760…
New Official room contract key: 4XwkMCQ4g1goJUzqVwdGgqEu2xgKHj7nge1GEXDDv4Yq
Chat delegate unchanged (2f8c5f1d…), no delegate migration entry.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0157PLorh6qju9s4C4dUDWJR
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.

1 participant