diff --git a/FOLLOWUPS.md b/FOLLOWUPS.md index 338888e..cae9463 100644 --- a/FOLLOWUPS.md +++ b/FOLLOWUPS.md @@ -553,8 +553,10 @@ outstanding. the `ON CONFLICT` `SET` (`internal/store/outbound_votes.go`), defending exactly one transition — `pending` may not overwrite a stored `delivered`. Every other column still updates, `activity_seq` still bumps, `RETURNING` - reflects the kept state, and the purge's `undone` and the callback's - `delivered` still write straight through. It is SQL and not Go because the + reflects the kept state, and the purge's `undone` and `applyVoteDelete`'s + re-stated `delivered` still write straight through (the settlement callback + writes via `SetDeliveredState` and never touches the guard). It is SQL and + not Go because the consumer's state read is non-transactional and would race the delivery worker's settlement; the `CASE` evaluates under the row lock `ON CONFLICT` already holds. @@ -576,11 +578,15 @@ outstanding. holds stays in the baseline, the direction it does not hold is subtracted from a total that never contained it. One directional error was traded for a different one — but the new one is SIGNALLED, where the old one was - silent: `SeedOursSubtracted` counts the row, and the negative baseline - trips `SeedBaselineClamped` and the clamp `Warn` naming direction, - `deficit_*` and `ours_*`. The two errors can cancel in the served number, - so those counters are the only place the window shows. Heals when the flip - delivers or the vote is undone; the arithmetic is pinned by + silent — but signalled ONLY when the mis-subtraction breaches the zero + floor: `SeedOursSubtracted` counts the row (and every healthy vote), and + the negative baseline trips `SeedBaselineClamped` and the sampled clamp + `Warn`; unrelated votes in the same direction can absorb the error with no + distinguishing signal, and the two errors can cancel in the served number. + Heals when the flip delivers or the vote is undone — and if the flip's + delivery POISONS, neither ever comes: the misread then recurs on every + re-seed until an undo, with the standing divergence reported by + `RecastDivergence`. The clamping arithmetic is pinned by `TestReseedDuringARecastWindowMisreadsBothDirections` (`internal/votes/reseed_recast_cost_test.go`). - **Delete then re-cast the same rkey before the Undo settles.** The late Undo @@ -591,6 +597,38 @@ outstanding. the purge and the reseed rather than merely stale to one of them. This is leg 2's mechanism, below, reached from the opposite direction. +- **Found by the guard's multi-model review — three PRE-EXISTING gaps, none + introduced or widened by the fix, all verified unchanged against the + pre-guard code:** + - *Subject mutation on a live vote is unaccounted.* An update commit + re-pointing the same vote rkey at a DIFFERENT subject overwrites + `subject_at_uri` in place; the peer keeps the old subject's vote while the + row describes the new one (pre-guard it went `pending` and the vote was + simply invisible). Coves never re-points vote records, and the 17e report + still names the stranded activity (its exclusion joins on subject + id). + Candidate fix: refuse a subject change for an existing live vote in + `applyVoteWrite`, with a same-rkey-different-subject regression test. + - *An ordinary delete after a poisoned re-cast sends an Undo naming the + never-delivered activity* (`applyVoteDelete` embeds + `stored.CurrentActivityID` — the NEW id). Identical pre-guard; it rides + the same unverified assumption the purge path documents (Lemmy matching + the inner object on `(actor, object)`), without the documentation. Covered + by the same verification below. + - *A purge racing a stale queued vote event can resurrect `undone`.* A vote + commit that passes `mayFederate` before a concurrent purge commits can + upsert `pending` over `undone` and enqueue outward work past the purge's + cancellation snapshot — the terminality invariant rests on an upstream + gate outside the state writer (same class as 17c-3's recorded ban race). + Unreachable in practice at current scale; before scale, the candidate fix + is making `undone` terminal in the upsert too (a purged actor never + legitimately votes again), as its own test-first subtask — it reverses a + recorded matrix decision, so it needs its own RED tests, not a quiet edit. + - *Shared verification for the first two:* an outbound-vote e2e against a + real Lemmy asserting an Undo whose inner id/direction mismatch the held + vote still retracts it — the `(actor, object)` assumption is now + load-bearing for the purge path. The production canary doubles as this + check at current scale. + - **The re-cast race, leg 2 — the settlement silently forgets the old vote. Recorded nowhere before now.** `internal/outbound/worker.go` `voteCallback` resolves via `GetByActivityID(activity.ActivityID)` and returns `nil` on diff --git a/internal/store/divergence.go b/internal/store/divergence.go index ca7ca29..fa729a2 100644 --- a/internal/store/divergence.go +++ b/internal/store/divergence.go @@ -546,8 +546,9 @@ func (r *postgresDivergences) UndeliveredAcceptanceCounts(ctx context.Context, s // can suppress a finding, never create one. If our ledger still names this // exact activity as the live vote AND still calls it delivered, then we // account for what the peer holds and there is nothing to reconcile. When -// the row has been reset, retracted or deleted — every shape this bug takes -// — the row cannot answer, and the history stands on its own. +// the row has moved to a new activity id, been retracted, or been deleted — +// every shape this bug takes — the row cannot answer, and the history stands +// on its own. // // THREE INDEPENDENT EXCLUSIONS, because they answer different questions and // each is the whole defence against a different way of ruining this report: diff --git a/internal/store/interfaces.go b/internal/store/interfaces.go index ec08a21..80631c0 100644 --- a/internal/store/interfaces.go +++ b/internal/store/interfaces.go @@ -545,7 +545,13 @@ type OutboundVotes interface { // SetDeliveredState transitions the delivery state. An unknown state is // an error satisfying errors.IsValidation; a missing vote is an error - // satisfying errors.IsNotFound. + // satisfying errors.IsNotFound. `undone` is TERMINAL here: any other + // write over an undone row is refused and reports SUCCESS (a decided + // no-op — failing it would leave a settlement retrying a write that can + // never apply), and re-setting `undone` stays allowed so the write is + // idempotent. This is the settlement writer's guard against late facts + // about old messages; the intent-writer's one refused transition lives + // on Upsert, deliberately different (see its doc). SetDeliveredState(ctx context.Context, voteATURI string, state DeliveredState) error // Delete removes the vote state once its Undo is delivered. Deleting a diff --git a/internal/store/models.go b/internal/store/models.go index 40b90dd..822f90d 100644 --- a/internal/store/models.go +++ b/internal/store/models.go @@ -247,7 +247,9 @@ const ( DeliveredStateDelivered DeliveredState = "delivered" // DeliveredStateUndone means the vote is NO LONGER LIVE on the peer as far as // this bridge is concerned, so nothing may count it: the reseed subtracts - // only `delivered`, and the destructive tier enumerates only `delivered`. + // only `delivered`, and the destructive tier's standing list never includes + // `undone` (it enumerates `delivered` plus held-for-settlement pending rows + // — ListStandingForActor). // // IT IS NO LONGER RESERVED, and its meaning is narrower than the obvious // reading. Task 15's worker still DELETES the row on a successful Undo, so diff --git a/internal/store/outbound_votes_test.go b/internal/store/outbound_votes_test.go index 7c2fd23..a60b307 100644 --- a/internal/store/outbound_votes_test.go +++ b/internal/store/outbound_votes_test.go @@ -93,8 +93,9 @@ func TestOutboundVotes_UpsertKeepsDeliveredThroughARecast(t *testing.T) { // B2 — everything the guard must NOT change // --------------------------------------------------------------------------- -// TestOutboundVotes_UpsertDeliveredStateMatrix pins the five transitions that -// already work, so the fix above cannot be bought by freezing the column. +// TestOutboundVotes_UpsertDeliveredStateMatrix pins the six conflict +// transitions that already work — plus the plain INSERT branch — so the fix +// above cannot be bought by freezing the column. // // Each of these is a live production path, named in its own case. They pass // before the guard exists and must pass after it. @@ -145,6 +146,16 @@ func TestOutboundVotes_UpsertDeliveredStateMatrix(t *testing.T) { "the shortcut that would break the purge case above, since both arrive as " + "`pending`-shaped writes over a non-pending row", }, + { + name: "undone over pending applies", + seed: DeliveredStatePending, write: DeliveredStateUndone, want: DeliveredStateUndone, wantSeq: 1, + why: "a REAL production path, not a hypothetical: the purge retracts votes whose " + + "delivery is HELD FOR SETTLEMENT — the peer accepted the POST, only our " + + "bookkeeping lagged — and those rows still read `pending` " + + "(ListStandingForActor's second term). A guard shaped 'only a delivered row " + + "may take undone' would pass every other case here and break that purge at " + + "the store level", + }, { name: "delivered over undone applies", seed: DeliveredStateUndone, write: DeliveredStateDelivered, want: DeliveredStateDelivered, wantSeq: 1, diff --git a/internal/votes/aggregator.go b/internal/votes/aggregator.go index ee45051..2e8ba33 100644 --- a/internal/votes/aggregator.go +++ b/internal/votes/aggregator.go @@ -499,10 +499,15 @@ func (a *Aggregator) RetractVote(ctx context.Context, vote *ap.Object, community // the row's direction is already the NEW one while the peer still holds // the OLD, so the subtraction lands on the wrong side of the tally — the // direction the peer holds is not subtracted, and the direction it does -// not hold is. It heals when the flip delivers or the vote is undone, and -// unlike its predecessor it is SIGNALLED while it lasts (see the ours.* -// binding in SeedAggregates for which counters, and why the served number -// alone will not show it). The "what the peer holds" vs "what the user +// not hold is. It heals when the flip delivers or the vote is undone — and +// if the flip's delivery POISONS, neither event ever comes: the wrong-side +// subtraction then recurs on every re-seed until an undo, and the standing +// divergence is RecastDivergence's finding. It is signalled while it lasts +// ONLY when the mis-subtraction breaches the zero floor (see the ours.* +// binding in SeedAggregates: SeedBaselineClamped fires on the breach, and +// unrelated votes in the same direction can absorb the error silently — +// the counters witness the clamping shape, not every window). The "what +// the peer holds" vs "what the user // wants" column pair once proposed here was REJECTED with reasons; they // are recorded in FOLLOWUPS.md so it is not re-proposed. // @@ -573,23 +578,31 @@ func (a *Aggregator) SeedAggregates(ctx context.Context, subjectAPID string, upv // is what the upsert guard bought — and per direction it is wrong // until the flip delivers or the vote is undone. // - // It is SIGNALLED rather than swallowed, and that is the whole reason - // this is tolerable: SeedOursSubtracted counts the row, the wrong-side - // subtraction drives that direction's baseline negative, and the - // GREATEST(0, …) floor trips SeedBaselineClamped plus the clamp Warn - // naming direction, deficit_* and ours_*. The two directional errors - // can CANCEL in the served number, so vote_aggregates alone shows a - // healthy subject and the counters are the only place the window is - // visible. The arithmetic is pinned exactly as it stands by - // TestReseedDuringARecastWindowMisreadsBothDirections. + // It is signalled ONLY when the wrong-side subtraction breaches the + // zero floor: SeedOursSubtracted counts the row (it also counts every + // healthy delivered vote, so it identifies routine work, not this + // window), and GREATEST(0, …) trips SeedBaselineClamped plus the + // sampled clamp Warn naming direction, deficit_* and ours_* — but + // unrelated votes in the subtracted direction can keep the raw + // baseline non-negative, in which case the misread is absorbed with + // NO distinguishing signal. The two directional errors can also + // CANCEL in the served number, so vote_aggregates alone shows a + // healthy subject either way. The clamping shape — the one that does + // signal — is pinned exactly as it stands by + // TestReseedDuringARecastWindowMisreadsBothDirections; the silent + // shape has no witness here, and a standing one is RecastDivergence's + // to report. // - // A negation ("NOT undone", "<> 'pending'") reads identically TODAY only - // because nothing writes 'undone'. If a policy ever does, it will mean - // the peer ACCEPTED the withdrawal — not live — and every negation - // silently inverts while this equality stays correct. A poisoned Undo - // leaves a delivered row subtracting forever, which is the same hazard - // decision 16 cites for banning queue-history arithmetic: "delivered - // Likes minus delivered Undos" gets that row permanently wrong. + // A negation ("NOT undone", "<> 'pending'") would be WRONG TODAY, not + // merely future-hostile: 'undone' has a live writer — 17d's purge + // (outbound.Purger.undoLiveVotes) retracts a withdrawn actor's votes + // through the upsert — and it records OUR decision to stop counting + // at purge time, not the peer's acceptance (store.DeliveredStateUndone). + // A negation would resume subtracting a withdrawn actor's votes; this + // equality stays correct. A poisoned Undo leaves a delivered row + // subtracting forever, which is the same hazard decision 16 cites for + // banning queue-history arithmetic: "delivered Likes minus delivered + // Undos" gets that row permanently wrong. // // The read takes no row locks. The seed holds the aggregate lock and // reads outbound_votes lock-free; the delivery worker locks