diff --git a/FOLLOWUPS.md b/FOLLOWUPS.md --- a/FOLLOWUPS.md +++ b/FOLLOWUPS.md @@ -537,20 +537,59 @@ ## Deferred by 17e (reconciliation scoped to detect-only) 17e reports divergence and never repairs it (decision 19). These are the repairs -and the comparisons it deliberately did not build. +and the comparisons it deliberately did not build. Leg 1 has since been built +and is kept here, marked CLOSED, for the design record rather than as work +outstanding. -- **The re-cast race, leg 1 — `upsert` clobbers a delivered vote.** - `internal/consume/votes.go` hardcodes `pending` on the vote write and - `internal/store/outbound_votes.go` `Upsert` sets +- **CLOSED — the re-cast race, leg 1: the upsert no longer clobbers a delivered + vote.** The write path hardcoded `pending` + (`internal/consume/votes.go`) over an `ON CONFLICT` that took `delivered_state = EXCLUDED.delivered_state`, so re-casting a DELIVERED vote - resets the row to pending while Lemmy still holds the OLD vote in the OLD - direction: we subtract nothing and keep our stale vote. Transient normally, - PERMANENT if that delivery poisons. THE FIX, and it already has a model in - the tree: make the upsert refuse to write `pending` over `delivered` exactly - the way `SetDeliveredState` now refuses to write over `undone` (17d), so a - re-cast leaves a row that still owes an Undo. Vote-accounting change with its - own RED test — 17e's report is its regression oracle, which is why the report - ships first. + reset the row to pending while Lemmy still held the OLD vote in the OLD + direction — the vote was then invisible to the reseed (which subtracts only + `delivered`) and to the erasure purge (which enumerates only standing votes), + permanently if the new delivery poisoned. The chosen design is the model 17d + already set: a guard in the STATEMENT rather than in Go, as a `CASE` inside + 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 + 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. + + *The rejected candidate, recorded so it is not re-proposed.* Freezing + `direction` alongside `delivered_state` — keeping every fact about the + delivered vote together — is wrong in the same way the "what the peer holds" + vs "what the user wants" column pair below is wrong, and for a sharper + reason: `consume.applyVoteWrite` builds the OUTGOING intent from the row the + upsert RETURNS, so a frozen direction would federate the flip in the + direction the user just abandoned, leaving the peer counting the vote they + changed away from. The row states the newest intent and the older delivery + together, deliberately. + + *Two residual limits, both accepted.* + - **Direction incoherence inside the re-cast window.** `delivered_state` now + survives the flip while `direction` is already the new one, so a reseed + landing in the window subtracts the wrong side: the direction the peer + 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 + `TestReseedDuringARecastWindowMisreadsBothDirections` + (`internal/votes/reseed_recast_cost_test.go`). + - **Delete then re-cast the same rkey before the Undo settles.** The late Undo + callback resolves the OLD activity id, misses (`GetByActivityID` → NotFound + → no-op), and the row keeps `delivered` for a vote the peer no longer holds + until the next flip delivery or undo. Narrow and known, and chosen over the + pre-fix behaviour where that same sequence left the vote invisible to both + 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. - **The re-cast race, leg 2 — the settlement silently forgets the old vote. Recorded nowhere before now.** `internal/outbound/worker.go` `voteCallback` diff --git a/internal/outbound/purge.go b/internal/outbound/purge.go --- a/internal/outbound/purge.go +++ b/internal/outbound/purge.go @@ -214,6 +214,23 @@ VoteATURI: vote.VoteATURI, SubjectAPID: vote.SubjectAPID, // Read back from state, never guessed: an Undo{Like} withdrawing a // Dislike would move the peer's count the wrong way. + // + // AND THE MISMATCH IS NOW REACHABLE, where it used to be + // hypothetical. Since the upsert began keeping `delivered` through + // a flip, a vote that was flipped but whose flip never delivered is + // STANDING — ListStandingForActor returns it — so this Undo goes + // out carrying the NEW direction and an InnerActivityID the peer + // never saw, to retract the OLD vote they actually hold. + // + // What is expected to save it is that the translator spells the + // inner object out in full — {type, id, actor, object} with actor + // and object as real ids (outbound/translator.go) — so a peer + // matching the retraction on (actor, object) drops the right vote + // whatever the wrapped type and id say. WHETHER LEMMY MATCHES ON + // THAT PAIR IS NOT ESTABLISHED IN THIS TREE: there is no + // outbound-vote e2e, so nothing here has ever watched a real + // instance answer this request. Treat it as an assumption carried + // by the erasure path, not as a verified guarantee. Direction: vote.Direction, ID: consume.ActivityID(p.userOrigin, vote.VoteATURI, consume.OperationUndo, bumped.ActivitySeq), InnerActivityID: vote.CurrentActivityID, diff --git a/internal/store/divergence.go b/internal/store/divergence.go --- a/internal/store/divergence.go +++ b/internal/store/divergence.go @@ -203,16 +203,21 @@ // RecastDivergence is a vote a peer HOLDS that our own state does not claim. // // It is produced by the re-cast race 17b recorded and deferred: re-casting a -// delivered vote re-upserts the SAME row back to pending under a new activity -// id, while the peer still holds the old vote in the old direction. Transient -// while the new delivery is in flight — and PERMANENT the moment it poisons. +// delivered vote rewrites the SAME row to the new direction under a NEW +// activity id, while the peer still holds the old vote in the old direction. +// Transient while the new delivery is in flight — and PERMANENT the moment it +// poisons. // // IT CANNOT BE READ FROM THE VOTE ROW, which is what makes it a reconciliation -// item rather than a query. worker.voteCallback resolves its row through -// GetByActivityID and returns nil on NotFound, so when a delivery that was -// already in flight lands AFTER a re-cast, its id no longer matches -// current_activity_id and the settlement silently no-ops. The row is precisely -// the evidence the bug erases. outbound_activities is append-only and its +// item rather than a query. The row keeps `delivered` through the flip (the +// upsert guard defends that state), so it still says a vote of ours stands +// here — but current_activity_id has moved to the new activity, so it can no +// longer say WHICH one, and which one is the entire content of this finding. +// worker.voteCallback resolves its row through GetByActivityID and returns nil +// on NotFound, so when a delivery that was already in flight lands AFTER a +// re-cast, its id no longer matches current_activity_id and the settlement +// silently no-ops — nothing writes the old activity back into the row, ever. +// outbound_activities is append-only and its // parent_at_uri carries the subject at-uri from both vote enqueue sites, so the // activity/delivery history is the durable record of what each peer was // actually told. @@ -557,14 +562,14 @@ // holds nothing now. // a LATER DELIVERED VOTE followed it — without this, every successful vote // FLIP is a finding, forever. A flip is an in-place upsert // (consume.applyVoteWrite): current_activity_id moves to the new -// activity, delivered_state resets to pending, and NO Undo is enqueued, -// because Lemmy holds one vote per (person, object) and REPLACES it on a -// bare opposite vote. So once the new vote delivers, the old delivered -// activity satisfies neither exclusion above — the ledger names the new -// id and no Undo will ever join it — and the append-only history keeps it -// forever. A later delivered Like/Dislike for the same pair supersedes an -// earlier one EXACTLY as a delivered Undo does, and that is the only -// reason this is correct rather than merely convenient. +// activity, and NO Undo is enqueued, because Lemmy holds one vote per +// (person, object) and REPLACES it on a bare opposite vote. So once the +// new vote delivers, the old delivered activity satisfies neither +// exclusion above — the ledger names the new id and no Undo will ever +// join it — and the append-only history keeps it forever. A later +// delivered Like/Dislike for the same pair supersedes an earlier one +// EXACTLY as a delivered Undo does, and that is the only reason this is +// correct rather than merely convenient. // // Both time exclusions compare by TIME rather than by id on purpose: an Undo // names the activity it withdraws in its payload, but a re-cast mints new diff --git a/internal/store/interfaces.go b/internal/store/interfaces.go --- a/internal/store/interfaces.go +++ b/internal/store/interfaces.go @@ -498,6 +498,17 @@ // (ActorDID, SubjectATURI) pair that already has one returns an error // satisfying errors.IsAlreadyExists: one actor holds at most one live // vote per subject, and silently clobbering the old row would strand its // Undo. + // + // ONE STATE TRANSITION IS REFUSED: `pending` over a stored `delivered` + // keeps `delivered`. A re-cast replaces a vote the peer still holds rather + // than withdrawing it, and the caller states `pending` on every write + // because it records intent and cannot know what the wire said — so + // letting it land would erase the only record that a delivery happened. + // Every other column still updates and ActivitySeq still bumps, and the + // RETURNED row reflects the KEPT state: callers build their outgoing + // intent from what comes back, so the struct and the stored row cannot + // disagree. No other transition is defended — `undone` (the purge's + // retraction) and `delivered` both write straight through. Upsert(ctx context.Context, vote OutboundVote) (*OutboundVote, error) // UpsertTx is Upsert on an existing transaction. A nil tx is an error diff --git a/internal/store/models.go b/internal/store/models.go --- a/internal/store/models.go +++ b/internal/store/models.go @@ -229,7 +229,21 @@ const ( // DeliveredStatePending means the intent is recorded but unconfirmed. DeliveredStatePending DeliveredState = "pending" - // DeliveredStateDelivered means a peer accepted the Like/Dislike. + // DeliveredStateDelivered means a peer accepted A VOTE from this actor for + // this subject — NOT necessarily the activity this row currently names. + // + // After a re-cast the row carries two facts from different moments: + // `direction` and `current_activity_id` describe the NEWEST intent, while + // this state describes a delivery that already happened. The upsert keeps + // `delivered` through a flip on purpose (outbound_votes.go), because the + // alternative erases the only record that any delivery occurred. So the + // question this column answers is exactly "does the peer hold a vote of + // ours here", and no more than that. + // + // WHICH activity the peer accepted is therefore not readable from this row + // after a flip. Only the append-only delivery ledger still knows, which is + // why RecastDivergence is reconciled out of outbound_activities joined to + // outbound_deliveries rather than queried from here (divergence.go). 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 diff --git a/internal/store/outbound_votes.go b/internal/store/outbound_votes.go --- a/internal/store/outbound_votes.go +++ b/internal/store/outbound_votes.go @@ -40,6 +40,12 @@ // records INTENT, and only task 15 may claim delivery — on success, from // the wire. Defaulting the zero value the other way would silently mark a // vote as delivered that no peer ever saw, and its Undo would then look // unnecessary. + // + // This decides only what the caller ASKED FOR, not what the row ends up + // holding: on a re-cast the ON CONFLICT below may keep a stored + // `delivered` over the `pending` defaulted here. The two rules do not + // disagree — this one refuses to INVENT a delivery nobody witnessed, that + // one refuses to DISCARD one that was. if vote.DeliveredState == "" { vote.DeliveredState = DeliveredStatePending } @@ -73,6 +79,26 @@ // // It lives in SQL, not Go: the consumer's state read is non-transactional, // so a read-then-decide guard races the delivery worker's settlement. The // CASE evaluates under the row lock ON CONFLICT already holds. + // + // FREEZING `direction` ALONGSIDE IT WAS REJECTED. It looks like the + // consistent move — keep every fact about the delivered vote together — + // but consume.applyVoteWrite builds the OUTGOING intent from the row this + // statement RETURNS, so a frozen direction would federate the flip in the + // direction the user just abandoned, and the peer would keep counting the + // vote they changed away from. It is also the rejected "what the peer + // holds" vs "what the user wants" column pair collapsed into one column, + // carrying the same defect: two facts in one place with no way to tell + // which a reader meant. The row therefore states the newest intent and the + // older delivery TOGETHER, on purpose (store.DeliveredStateDelivered). + // + // THE ACCEPTED COST, so it is not rediscovered as a fresh bug: delete a + // vote and re-cast the SAME rkey before the Undo settles, and the late + // callback resolves the OLD activity id, misses (GetByActivityID → + // NotFound → no-op), and this row keeps `delivered` for a vote the peer no + // longer holds — until the next flip delivers or an undo lands. Narrow and + // known, and chosen over the pre-fix behaviour, where the same sequence + // left the vote invisible to BOTH the erasure purge and the reseed rather + // than merely stale to one of them. query := ` INSERT INTO outbound_votes ( vote_at_uri, actor_did, subject_at_uri, subject_ap_id, community_did, diff --git a/internal/votes/aggregator.go b/internal/votes/aggregator.go --- a/internal/votes/aggregator.go +++ b/internal/votes/aggregator.go @@ -478,9 +478,11 @@ // the whole run inside the freshness window — so for a quiet community "heals // on the next re-seed" can mean "heals when an admin forces a backfill", and // may mean never. // -// Three residual races span the origin API fetch and this transaction. All are -// transient and self-healing on the next re-seed, with the caveat above (the -// pre-fix over-count race was PERMANENT and compounding): +// Three residual races span the origin API fetch and this transaction. The +// first two are transient and self-healing on the next re-seed, with the caveat +// above (the pre-fix over-count race was PERMANENT and compounding); the third +// heals on the FLIP'S DELIVERY rather than on a re-seed, so re-seeding inside +// its window reproduces it rather than converging it: // - under-count by one: a vote federates AFTER the fetch but is live here, so // it is net-subtracted from the baseline yet not present in the fetched // total; @@ -488,15 +490,21 @@ // - over-count by one (the mirror): a vote already IN the fetched total whose // federated activity arrives AFTER this seed tx — the net-of-live // subtraction cannot yet see it as a live row, so the baseline keeps it AND // the later live event adds it again, until the next re-seed reconciles; -// - over-count by one, outbound side: a native user RE-CASTS a vote they had -// already delivered. consume's applyVoteWrite re-upserts the row and -// OutboundVotes.Upsert resets delivered_state to 'pending', while Lemmy -// still holds the OLD vote in the OLD direction — so this seed subtracts -// nothing for it and the stale vote stays in the served tally. Transient -// (the redelivery flips the row back to 'delivered') but PERMANENT if that -// delivery poisons. Fixing it needs a second column pair modelling "what -// the peer holds" against "what the user wants", which is task 17e's, not -// this one's. +// - direction incoherence during a re-cast window, outbound side: a native +// user RE-CASTS a vote they had already delivered. The CLOBBER this bullet +// used to describe is closed — OutboundVotes.Upsert now keeps 'delivered' +// through a flip, so the row stays in the `ours` term instead of dropping +// out of it entirely, and the vote is no longer invisible to this seed. +// What remains is narrower and is a DIFFERENT error, not the same one: +// 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 +// wants" column pair once proposed here was REJECTED with reasons; they +// are recorded in FOLLOWUPS.md so it is not re-proposed. // // Subjects not present in ap_objects are dropped and logged at debug, like // ApplyVote. @@ -540,8 +548,9 @@ // would read on its own READ COMMITTED snapshot (inTx takes the default // isolation), so the two counts could come from different moments — // creating exactly the torn read the aggregate lock exists to prevent. // - // ours.* is the votes LEMMY CURRENTLY HOLDS for our personas, and the - // predicate is the POSITIVE EQUALITY delivered_state = 'delivered': + // ours.* is the votes LEMMY CURRENTLY HOLDS for our personas — exactly + // per ROW, only APPROXIMATELY per DIRECTION — and the predicate is the + // POSITIVE EQUALITY delivered_state = 'delivered': // // pending (first try, retrying, poisoned) → the peer does not hold it // delivered → it does: subtract @@ -551,6 +560,28 @@ // 'delivered' precisely because the // peer has not yet processed the // withdrawal // row gone (Undo delivered) → nothing to subtract + // + // THE DIRECTION IS THE APPROXIMATE HALF, and only inside a re-cast + // window. The row's `direction` tracks the newest INTENT while + // `delivered_state` describes a delivery that already happened, so + // after a flip the two come from different moments (see + // store.DeliveredStateDelivered). This subtracts the flip's direction + // from an origin total that still contains the old one: the direction + // the peer really holds is left in the baseline, and the direction it + // does not hold is subtracted from a total that never contained it. + // Per row the term is RIGHT — the vote is counted among `ours`, which + // 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. // // A negation ("NOT undone", "<> 'pending'") reads identically TODAY only // because nothing writes 'undone'. If a policy ever does, it will mean