fix(dm): stop ordinary sync churn from destroying DM archives - #529
#529fix(dm): stop ordinary sync churn from destroying DM archives#529sanity merged 1 commit intomainfreenet/river:mainfrom fix/526-dm-archive-survives-relanded-dmsfreenet/river:fix/526-dm-archive-survives-relanded-dmsCopy head branch name to clipboard
Conversation
ad5b895 to
ba08dda
Compare
Blocking finding from re-review — do not merge as-isTwo independent reviewers converged on the same live defect in the current commit ( The defect
So whenever the user replies and then archives, the cutoff is receiver-clock Scenario (no clock skew needed):
Bob's message has no surface: no rail row, no unread badge ( On What the fix should beMake archive inbound-anchored — "hidden until they write again":
The user's own outbound already revives the thread through the unconditional Residual after that fix: a peer whose clock moves backward relative to their own previous message. Irreducible under timestamp gating, and far narrower than the current hole. Other findings to fold in
Verified clean by the re-reviewThe all-or-nothing-per-pair sweep claim holds. Moving to draft until the inbound anchoring lands. [AI-assisted - Claude] |
Addendum — two further residuals from the third lensBoth are in the same family (timestamp gating leaking), and both need to be folded into the inbound-anchored rework rather than fixed separately. The sweep is all-or-nothing, but the RE-LAND is notThe safety argument I wrote says "every DM in the thread satisfies The sweep removes a pair atomically, but restoration is delta-driven and unordered. A peer can re-offer a subset:
That is #526 unchanged, reached through the fixed code. Narrower window, same outcome. A second, smaller instance of the same shape: Proper close: have A future-dated DM makes a thread permanently invisible
Invisible on every surface for a year. This is a regression vs Mitigation: clamp the cutoff at Smaller items to fold in
Net assessmentThe gate is the right mechanism and closes the reported bug for the common case. But anchoring it on a timestamp — any timestamp — keeps leaking, in both directions: too low a cutoff destroys archives, too high a cutoff hides real messages. The inbound anchoring plus a future-skew clamp plus recomputing from live state closes the ones found so far. Whether that is the right stopping point, versus recording arrival identity, is worth deciding deliberately before the next revision. [AI-assisted - Claude] |
776e257 to
c8f7882
Compare
Archiving a DM writes a `(room, peer, hidden_at_ts)` entry and the rail hides the thread while the peer's newest message is at or below it. A new inbound DM is meant to revive it (#267), but the revival was implemented as a hard delete of the archive entry, fired whenever a DM crossed the merge dedupe. "Crossed the dedupe" means "was not in LOCAL state a moment ago", which is not the same question as "is new content", and the decision never consulted `hidden_at_ts` at all. Two ways that misfired: * `post_apply_cleanup` sweeps DMs whose counterparty is momentarily absent from `members` on every merge. Peers re-offer the swept DMs and they re-land carrying their original timestamps, indistinguishable there from a genuinely new message. * On the full-state path the pre-merge snapshot is empty whenever the room is in `ROOMS` but its DM state has not hydrated, so every DM read as newly landed and every archive in the room was destroyed at once. `unhide_dm_thread` also writes a session `RECENTLY_UNHIDDEN` tombstone and queues a delegate save, so the loss was persisted and a later hydration carrying the archive was suppressed. Archiving looked like it did nothing. `unhide_dm_thread_if_dm_is_newer` replaces the unconditional call at both inbound-sync sites, dropping the entry only when the DM's timestamp is strictly past the cutoff. A pair with no entry — including "the archive list has not hydrated yet" — is inert and writes no tombstone, which defuses the startup race. `fold_newly_landed_to_max_ts` reduces a merge batch to one entry per sender keeping the largest timestamp. The gate is only as good as its cutoff, so the archive clock is now INBOUND-only and recomputed from live state: * Inbound-only. `last_any_ts` also covers our own outbound DMs, whose timestamp is our LOCAL wall clock. A thread where we replied last was therefore archived against OUR clock while the gate compares a SENDER's timestamp, so a peer DM already in flight across the click landed at or below the cutoff and went invisible everywhere: no rail row, no unread badge, no notification (DMs raise none). `filter_rail_entries`, both archived-view projections and `count_unread_dms_with` all compare the inbound clock. Outbound revival now comes solely from the unconditional `unhide_dm_thread` in `do_send` / `send_structured_dm` — on the sending device only, which the docs state rather than gloss. * Recomputed from live `ROOMS`, maxed with the rendered row so a contended read falls back to it. The cutoff is deliberately NOT clamped against wall-clock time. An earlier revision clamped it at `now + MAX_DM_FUTURE_SKEW_SECS`, which broke the invariant "every existing inbound DM satisfies `ts <= cutoff`": the rail filter compares the UNCLAMPED observed clock, so Archive became a visible no-op that still showed a success toast, and the next sweep-and-re-offer destroyed the persisted archive. Clamping one side of a comparison was what created the hole. The gate bounds the INCOMING timestamp instead, where it cannot break the invariant and is safe-directional (the bounded value is never above the raw one). Residuals are documented rather than claimed closed, including the two that can still cost an archive entry: a cutoff taken inside a partial re-land window, and a DM the live read missed because `ROOMS` was contended AND the rendered row lagged. Both need an archive clicked inside a specific race; neither is the systematic churn this fixes. Also repairs the two #267 source pins, which searched for a literal their own assertions contained and so passed regardless of the production code, and pins every spot where a one-line edit re-opens the bug: the gate condition and that its branch still calls unhide, the inbound producer, the row's argument, the filter, both archived projections, the live recompute, clamp-freedom of the cutoff, and the four explicit-user-action unhides (each pinned in its own file). `common/` change is a doc comment only — no WASM change, so no migration. Closes #526
c8f7882 to
1b9c348
Compare
Problem
Archiving a DM writes a
(room, peer, hidden_at_ts)entry and the rail hides the thread while the peer's newest message is at or below it. A new inbound DM is meant to revive it (#267), but the revival was implemented as a hard delete of the archive entry, fired whenever a DM crossed the merge dedupe."Crossed the dedupe" means "was not in LOCAL state a moment ago", which is a different question from "is new content" — and the decision never consulted
hidden_at_tsat all. Two ways it misfired:post_apply_cleanupsweeps DMs whose counterparty is momentarily absent frommemberson every merge. Peers re-offer them, they re-land with their original timestamps, and the diff can't tell them from a new message.ROOMSbut its DM state hasn't hydrated, every DM in the incoming state reads as newly landed — so every archive in the room was destroyed at once.unhide_dm_threadalso writes a sessionRECENTLY_UNHIDDENtombstone and queues a delegate save, so the loss was persisted and a later hydration carrying the archive was suppressed. Archiving looked like it did nothing.Approach
ui/-only — nocommon/, delegate, or contract WASM change, so no migration.unhide_dm_thread_if_dm_is_newerreplaces the unconditional call at both inbound-sync sites, dropping the entry only when the DM's timestamp is strictly past the cutoff. A pair with no entry is inert — deliberately including "hasn't hydrated yet", so no unhide and no tombstone, which defuses the startup race.fold_newly_landed_to_max_tsreduces a merge batch to one entry per sender keeping the largest timestamp.The archive clock is inbound-only
A gate is only as good as its cutoff.
last_any_tscovers our own outbound DMs too, and an outbound DM's timestamp is our local wall clock. So a thread where we replied last was archived against our clock while the gate compares a sender's timestamp — and a peer DM already in flight across the click landed at or below the cutoff and went invisible on every surface: no rail row, no unread badge (count_unread_dms_withapplies the same filter), no notification (DMs raise none).The clock is now
last_inbound_tseverywhere the archive decides visibility:filter_rail_entries, both archived-view projections, and the unread tally. Outbound revival comes solely from the unconditionalunhide_dm_threadindo_send/send_structured_dm— on the sending device only; hydration never propagates a removal andriverctl dm senddoesn't call it, which the docs now state rather than gloss.The cutoff is also recomputed from live
ROOMSstate, maxed with the rendered row so a contended read falls back. The row can lag, and while the membership sweep is atomic per pair, the re-land is not — a peer restoring only a subset would otherwise leave the cutoff low.Why the cutoff is NOT clamped
An earlier revision clamped it at
now + MAX_DM_FUTURE_SKEW_SECSto blunt a forged-future timestamp. Two reviewers independently found that this broke the load-bearing invariant every existing inbound DM satisfiests <= cutoff: the rail filter compares the unclamped observed clock, so Archive became a visible no-op that still showed a success toast, and the next sweep-and-re-offer sawts > cutoffand destroyed the persisted archive — this bug again, narrowed to skew-violating peers. Clamping one side of a comparison was what created the hole.The forged-timestamp concern is handled where it cannot break the invariant:
should_unhide_for_inbound_dmclamps the incoming timestamp, so an implausible-future DM can never trigger an unhide.Accepted residuals
All defer messages from one peer the user already chose to archive; none destroys state.
The real remedy for a hostile peer is a block/ignore list — #461.
Testing
cargo test -p river-ui --bins— 768 pass; 772 with--features example-data,no-sync.cargo fmt --all -- --checkclean, wasm target clean. Full Playwright suite green (605 passed, 15 skipped) across Chromium, Firefox, WebKit, mobile Chrome and mobile Safari.Note the example-data build contains no DMs (populating them needs a real ECIES envelope per DM), so Playwright cannot drive the archive flow end-to-end; it covers the surfaces this PR touches against regression, and the behaviour is pinned in Rust.
Mutation-checked, each reverted independently against the committed fix — all caught:
true(pre-fix behaviour)unhide_dm_threadlast_any_ts(the outbound-anchor bug)is_self_recipientlast_any_tstoarchive_rowlast_any_tsdo_sendrouted through the gated unhideTwo mutations initially reported green and both turned out to be measurement bugs worth recording: one didn't compile (so no tests ran), and one was inert because the pin's needle matched its own explanatory comment rather than the code. Both are fixed; the clamp is now pinned structurally, because
archive_cutofftakes no injectablenowand fixture timestamps sit far below real wall-clock time, which makes any re-introduced clamp invisible to a behavioural test.Repaired two vacuous pins
The existing #267 pins searched
include_str!for literals their own assertions contained, so all three assertions in each were tautologies. They now cut atmod tests, match whitespace-stripped, assemble needles at runtime, and the delta-path pin is sliced to its own function.Review
Three full rounds, independent blind lenses each time. Every round found a real defect in my own work; all are addressed:
max(last_any_ts, now)cutoff as unnecessary and harmful.last_any_tsincludes outbound. That produced the inbound-only rework above.Also filed #530 — a pre-existing bug two reviewers surfaced: a save that runs before the delegate blob hydrates persists a truncated store, permanently losing archives and outbound-DM plaintexts. It reproduces two of this issue's symptoms by a different mechanism, so #526 alone does not fully close the user complaint.
Closes #526
[AI-assisted - Claude]