diff --git a/FOLLOWUPS.md b/FOLLOWUPS.md --- a/FOLLOWUPS.md +++ b/FOLLOWUPS.md @@ -537,20 +537,97 @@ ## 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 `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. + + *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 — 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 + 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. + +- **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` diff --git a/internal/consume/votes_test.go b/internal/consume/votes_test.go --- a/internal/consume/votes_test.go +++ b/internal/consume/votes_test.go @@ -20,13 +20,25 @@ // outbound_votes exists, and it is why the row is written before the intent // and outlives the record it describes. func voteFrame(did, rev, rkey, subjectATURI, direction string) []byte { + return voteWriteFrame(did, rev, rkey, subjectATURI, direction, "create") +} + +// voteRecastFrame is a vote FLIPPED in place. An up→down change is an edit of +// the existing record, not a delete plus a create, so it arrives as an update +// commit on the same rkey — and therefore the same at-uri the outbound_votes +// row is keyed by. +func voteRecastFrame(did, rev, rkey, subjectATURI, direction string) []byte { + return voteWriteFrame(did, rev, rkey, subjectATURI, direction, "update") +} + +func voteWriteFrame(did, rev, rkey, subjectATURI, direction, operation string) []byte { return []byte(fmt.Sprintf( - `{"did":%q,"time_us":9500,"kind":"commit","commit":{"rev":%q,"operation":"create",`+ + `{"did":%q,"time_us":9500,"kind":"commit","commit":{"rev":%q,"operation":%q,`+ `"collection":"social.coves.feed.vote","rkey":%q,`+ `"cid":"bafyreievgu2ty7qbiaaom5zhmkznsnajuzideek3lo7e65dwqlrvrxnmo4",`+ `"record":{"$type":"social.coves.feed.vote","subject":{"uri":%q,"cid":%q},`+ `"direction":%q,"createdAt":"2026-08-13T10:00:00.000Z"}}}`, - did, rev, rkey, subjectATURI, acceptRootCID, direction)) + did, rev, operation, rkey, subjectATURI, acceptRootCID, direction)) } func voteDeleteFrame(did, rev, rkey string) []byte { @@ -359,3 +371,67 @@ intent, ok := calls[1].Intent.(VoteIntent) require.True(t, ok) assert.Equal(t, "undo", intent.Op) } + +// --------------------------------------------------------------------------- +// I6 — the re-cast guard +// --------------------------------------------------------------------------- + +func TestVoteRecast_DeliveredStateSurvivesTheFlip(t *testing.T) { + database := dispatchTestDB(t) + seedBridgedCommunity(t, database) + seedThreadRoot(t, database) + fixture := newDispatchFixture(t, database) + votes := store.NewOutboundVotes(database) + ctx := context.Background() + + const rkey = "3lzvote000012" + voteATURI := voteATURIFor(dispatchNativeDID, rkey) + + require.NoError(t, fixture.handle(t, + voteFrame(dispatchNativeDID, dispatchRev, rkey, acceptRootATURI, "up"))) + + // The Like reached the peer, and the delivery worker settled it — the same + // call task 15's settlement callback makes, from the wire, on success. + require.NoError(t, votes.SetDeliveredState(ctx, voteATURI, store.DeliveredStateDelivered), + "the vote must be genuinely delivered before the flip, or this test proves nothing") + + // The user flips up→down by EDITING the vote record, so the commit is an + // update on the same rkey — the same at-uri the row above is keyed by. + require.NoError(t, fixture.handle(t, + voteRecastFrame(dispatchNativeDID, dispatchRevHigher, rkey, acceptRootATURI, "down"))) + + recast, err := votes.GetByATURI(ctx, voteATURI) + require.NoError(t, err) + require.NotNil(t, recast) + assert.Equal(t, "down", recast.Direction, "the flip itself is recorded") + + assert.Equal(t, store.DeliveredStateDelivered, recast.DeliveredState, + "the peer STILL HOLDS a vote from this actor — the flip is a replacement, not a "+ + "retraction, and nothing has come back off the wire to say otherwise. Resetting "+ + "to pending erases the only record that a delivery ever happened, and the fact "+ + "is unrecoverable: no event re-fires it") + + // The guard is about the ledger, not the wire. The flip must still go out. + calls := fixture.enqueuer.Calls() + require.Len(t, calls, 2, "one Like, then the Dislike that replaces it") + intent, ok := calls[1].Intent.(VoteIntent) + require.True(t, ok, "want VoteIntent, got %T", calls[1].Intent) + assert.Equal(t, "down", intent.Direction, + "suppressing the outgoing flip would leave the peer counting the vote the user "+ + "just changed away from") + assert.Equal(t, ActivityID(acceptUserOrigin, voteATURI, "create", 1), intent.ActivityID(), + "under a BUMPED seq: an id colliding with the delivered Like's would be swallowed "+ + "as a duplicate by any peer that already has it") + assert.Equal(t, recast.CurrentActivityID, intent.ActivityID(), + "and the row carries that same id, because the Undo has to embed it") + + // The operator-visible consequence, and the reason the ledger fact matters: + // this list is what the erasure purge enumerates. + standing, err := votes.ListStandingForActor(ctx, dispatchNativeDID) + require.NoError(t, err) + require.Len(t, standing, 1, + "a flipped vote must still be reachable by the purge — dropping out of this list "+ + "strands it un-retractable on the peer, so erasing the actor would leave their "+ + "vote standing on someone else's instance forever") + assert.Equal(t, voteATURI, standing[0].VoteATURI) +} 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. @@ -541,8 +546,9 @@ // The row appears below only as an EXCLUSION, and the direction matters: it // 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: @@ -557,14 +563,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 @@ -534,7 +545,13 @@ ListStandingForActor(ctx context.Context, actorDID string) ([]OutboundVote, error) // 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 --- a/internal/store/models.go +++ b/internal/store/models.go @@ -229,11 +229,27 @@ 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 - // 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.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 } @@ -51,6 +57,48 @@ // ON CONFLICT names the PRIMARY KEY only. The (actor_did, subject_at_uri) // constraint is deliberately NOT an upsert target: a different vote record // for a pair that already holds one must FAIL, because overwriting the row // would strand the Undo still owed for the first vote. + // + // The CASE on delivered_state defends ONE transition: `delivered` must not + // be overwritten by `pending`. A re-cast REPLACES a vote the peer still + // holds — it does not withdraw it — and the consumer states `pending` on + // every write because it records intent and cannot know what the wire said. + // Letting that land would erase the only record that a delivery ever + // happened, unrecoverably: no event re-fires it, the vote drops out of the + // standing list the erasure purge enumerates, and it is left un-retractable + // on someone else's instance. + // + // Only that transition, because this is the INTENT writer and every other + // caller here is restating the row on purpose: the purge retracts THROUGH + // this upsert with `undone` (outbound.Purger.undoLiveVotes), in the same + // statement that bumps the seq for the Undo it enqueues, so a guard that + // defended `delivered` against everything would silently drop it. The + // asymmetry with SetDeliveredState — where undone IS terminal — is + // deliberate: that method fields late settlements, stale facts about an old + // message, while this one fields new writes by a live human. + // + // 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, @@ -63,7 +111,12 @@ subject_ap_id = EXCLUDED.subject_ap_id, community_did = EXCLUDED.community_did, direction = EXCLUDED.direction, current_activity_id = EXCLUDED.current_activity_id, - delivered_state = EXCLUDED.delivered_state, + delivered_state = CASE + WHEN outbound_votes.delivered_state = '` + string(DeliveredStateDelivered) + `' + AND EXCLUDED.delivered_state = '` + string(DeliveredStatePending) + `' + THEN outbound_votes.delivered_state + ELSE EXCLUDED.delivered_state + END, activity_seq = outbound_votes.activity_seq + 1, updated_at = now() RETURNING` + outboundVoteColumns diff --git a/internal/store/outbound_votes_test.go b/internal/store/outbound_votes_test.go new file mode 100644 --- /dev/null +++ b/internal/store/outbound_votes_test.go @@ -0,0 +1,201 @@ +package store + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// THE UPSERT IS THE INTENT WRITER. SetDeliveredState IS THE SETTLEMENT WRITER. +// They are the two writers of one column, and they do NOT obey the same rule. +// +// A re-cast — the user flipping up→down — reaches this table as an ordinary +// upsert carrying `pending`, because the consumer records intent and never +// claims delivery. But a flip is a REPLACEMENT, not a retraction: the peer is +// still holding a vote from this actor, and nothing has come back off the wire +// to say otherwise. Letting `pending` win erases the only record that a +// delivery ever happened, and the loss is permanent — no event re-fires it, so +// the vote silently drops out of ListStandingForActor and the erasure purge can +// no longer retract it. +// +// The guard is therefore narrow, and its narrowness is the whole design: +// +// incoming pending over stored delivered -> KEEP delivered (the re-cast) +// incoming pending over stored undone -> take pending (a NEW intent) +// +// The second line is the one that rules out the tempting shortcut. "Keep the +// stored value whenever the incoming write says pending" satisfies the re-cast +// and breaks the purge: `undone` is a decision about an IDENTITY, and an upsert +// arriving over it is a live human casting a new vote, not a stale fact about +// an old message catching up. Terminality guards LATE SETTLEMENTS. It has no +// business refusing new intents, which is why SetDeliveredState refuses +// delivered-over-undone and this path does not. The asymmetry is deliberate. + +// --------------------------------------------------------------------------- +// B1 — the guard itself +// --------------------------------------------------------------------------- + +// recastActivityID is the id the FLIPPED vote goes out under: a re-cast is a new +// activity, so it must not reuse the delivered Like's id. +var recastActivityID = "https://coves.social/ap/activity/" + repeatHex('b') + +func TestOutboundVotes_UpsertKeepsDeliveredThroughARecast(t *testing.T) { + database := outboundTestDB(t) + repo := NewOutboundVotes(database) + ctx := context.Background() + + _, err := repo.Upsert(ctx, testOutboundVote()) + require.NoError(t, err) + require.NoError(t, repo.SetDeliveredState(ctx, testVoteATURI, DeliveredStateDelivered), + "the peer accepted the Like — without this the test proves nothing") + + // The consumer's re-cast write, verbatim: applyVoteWrite always states + // pending, because it records intent and cannot know what the wire said. + recast := testOutboundVote() + recast.Direction = "down" + recast.CurrentActivityID = recastActivityID + recast.DeliveredState = DeliveredStatePending + + returned, err := repo.Upsert(ctx, recast) + require.NoError(t, err) + require.NotNil(t, returned) + + // Pinned on the RETURNING value, not just on a re-read: applyVoteWrite builds + // the outgoing intent straight from this struct and never reloads the row, so + // a guard that fixed only the stored value would still hand the caller a lie. + assert.Equal(t, DeliveredStateDelivered, returned.DeliveredState, + "a flip REPLACES a vote the peer still holds; it does not withdraw it. Resetting to "+ + "pending discards the only evidence a delivery ever happened, and nothing "+ + "re-establishes it — the vote then vanishes from the standing list the erasure "+ + "purge enumerates, un-retractable on someone else's instance") + + assert.Equal(t, "down", returned.Direction, + "while everything the re-cast actually carries still lands: a guard that froze the "+ + "whole row would leave the peer counting the vote the user changed away from") + assert.Equal(t, recastActivityID, returned.CurrentActivityID, + "including the new id — the Undo embeds whatever this column holds") + assert.Equal(t, 1, returned.ActivitySeq, + "and the seq still bumps: an id colliding with the delivered Like's would be "+ + "swallowed as a duplicate by any peer that already has it") + + got, err := repo.GetByATURI(ctx, testVoteATURI) + require.NoError(t, err) + require.NotNil(t, got) + assert.Equal(t, DeliveredStateDelivered, got.DeliveredState, + "and the row agrees with what the upsert returned") + assert.Equal(t, recastActivityID, got.CurrentActivityID) + assert.Equal(t, 1, got.ActivitySeq) +} + +// --------------------------------------------------------------------------- +// B2 — everything the guard must NOT change +// --------------------------------------------------------------------------- + +// 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. +func TestOutboundVotes_UpsertDeliveredStateMatrix(t *testing.T) { + tests := []struct { + name string + // seed is the state the row is in before the write under test. Empty + // means NO PRIOR ROW — the plain INSERT branch, which has no stored + // value to defend and so cannot reach the guard at all. + seed DeliveredState + write DeliveredState + want DeliveredState + wantSeq int + why string + }{ + { + name: "a first cast asks for pending and gets pending", + seed: "", write: DeliveredStatePending, want: DeliveredStatePending, wantSeq: 0, + why: "the ordinary first vote: the consumer records intent, and only the " + + "settlement callback may ever claim more than that", + }, + { + name: "pending over pending stays pending, and still bumps", + seed: DeliveredStatePending, write: DeliveredStatePending, want: DeliveredStatePending, wantSeq: 1, + why: "a re-cast of a vote that never reached the peer has nothing to protect — " + + "but it is still a second activity, so the seq moves", + }, + { + name: "delivered over delivered applies", + seed: DeliveredStateDelivered, write: DeliveredStateDelivered, want: DeliveredStateDelivered, wantSeq: 1, + why: "applyVoteDelete re-upserts the row it just read back, verbatim, to bump the " + + "seq for the Undo — so a delivered row writes its own state straight through", + }, + { + name: "undone over delivered applies", + seed: DeliveredStateDelivered, write: DeliveredStateUndone, want: DeliveredStateUndone, wantSeq: 1, + why: "THE PURGE WRITES `undone` THROUGH THIS UPSERT (outbound.Purger.undoLiveVotes), " + + "in the one statement that also bumps the seq for the Undo it enqueues. A " + + "guard that defended `delivered` against everything would silently drop the " + + "retraction of a withdrawn actor's vote", + }, + { + name: "pending over undone applies", + seed: DeliveredStateUndone, write: DeliveredStatePending, want: DeliveredStatePending, wantSeq: 1, + why: "no production path reaches this today, and it is allowed anyway: this is the " + + "INTENT writer, and terminality guards late settlements — stale facts about " + + "an old message — not a new vote cast by a live human. Refusing it here is " + + "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, + why: "the same rationale, and the deliberate asymmetry: SetDeliveredState REFUSES " + + "this exact transition, because a settlement landing after a retraction is a " + + "stale fact. An upsert carrying it is a caller restating the whole row, and " + + "the intent path does not second-guess that", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + database := outboundTestDB(t) + repo := NewOutboundVotes(database) + ctx := context.Background() + + if tc.seed != "" { + _, err := repo.Upsert(ctx, testOutboundVote()) + require.NoError(t, err, "seed the row") + if tc.seed != DeliveredStatePending { + require.NoError(t, repo.SetDeliveredState(ctx, testVoteATURI, tc.seed), + "seed the row into %s", tc.seed) + } + } + + vote := testOutboundVote() + vote.DeliveredState = tc.write + returned, err := repo.Upsert(ctx, vote) + require.NoError(t, err) + require.NotNil(t, returned) + + assert.Equal(t, tc.want, returned.DeliveredState, tc.why) + assert.Equal(t, tc.wantSeq, returned.ActivitySeq, + "the seq bump is independent of the delivered_state decision") + + got, err := repo.GetByATURI(ctx, testVoteATURI) + require.NoError(t, err) + require.NotNil(t, got) + assert.Equal(t, tc.want, got.DeliveredState, + "the stored row must agree with what the upsert returned") + }) + } +} 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,26 @@ // - 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 +// 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. // // Subjects not present in ap_objects are dropped and logged at debug, like // ApplyVote. @@ -540,8 +553,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 @@ -552,13 +566,43 @@ // peer has not yet processed the // withdrawal // row gone (Undo delivered) → nothing to subtract // - // 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. + // 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 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'") 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 diff --git a/internal/votes/divergence_recast_test.go b/internal/votes/divergence_recast_test.go --- a/internal/votes/divergence_recast_test.go +++ b/internal/votes/divergence_recast_test.go @@ -16,12 +16,13 @@ // TASK 17e — THE RE-CAST DIVERGENCE: THE PEER HOLDS A VOTE WE DO NOT CLAIM. // // 17b found this and deferred it here. Re-casting a delivered vote re-upserts -// the SAME outbound_votes row back to 'pending' under a new activity id, while -// the peer goes on holding the old vote in the old direction. Transient while -// the new delivery is in flight; PERMANENT the moment it poisons — nothing -// re-drives a poisoned delivery on its own, and the reseed subtracts only -// 'delivered' rows, so the community's score keeps counting a vote we have -// stopped accounting for and will never correct. +// the SAME outbound_votes row under a NEW activity id, while the peer goes on +// holding the old vote in the old direction. The row keeps its delivered state +// — the re-cast guard sees to that — but it no longer NAMES the activity the +// peer accepted, and the ledger's claim is that PAIR, not the flag alone. +// Transient while the new delivery is in flight; PERMANENT the moment it +// poisons — nothing re-drives a poisoned delivery on its own, so the row goes +// on standing for a vote in a direction the peer never received. // // THE FIXTURE IS THE ENTIRE TEST, and it must be DRIVEN, not assembled. This is // 17b's own blind spot by name: "the fixture nobody writes is the one where the @@ -30,8 +31,8 @@ // real path — // // consume.Dispatcher.HandleEvent (vote commit) → outbound_votes + a Dislike // outbound.Worker.DeliverNext → delivered, ledger flipped -// consume.Dispatcher.HandleEvent (the re-cast) → SAME row reset to pending, -// new activity id, a Like +// consume.Dispatcher.HandleEvent (the re-cast) → SAME row, NEW activity id, +// delivered_state KEPT, a Like // outbound.Worker.DeliverNext (failing sender) → that Like POISONS // // — because a hand-inserted row would be some steady state a fixture author @@ -41,7 +42,8 @@ // // AND IT MUST NOT BE READ FROM THE VOTE ROW. worker.voteCallback resolves via // GetByActivityID and returns nil on NotFound, so a delivery already in flight // when the re-cast lands settles into silence: its id no longer matches -// current_activity_id and the callback no-ops. The row is what the bug erases. +// current_activity_id and the callback no-ops. The row survives the re-cast; +// what does not survive is its link to the activity the peer accepted. // outbound_activities is append-only and carries the subject in parent_at_uri // from both vote enqueue sites, so the activity/delivery history is the only // durable record of what the peer was actually told. @@ -77,6 +79,29 @@ "precondition: a vote really was delivered to the peer") return id } +// recastRowFacts reads the two columns of the live vote row whose meaning this +// cycle changed: the activity the ledger currently names, and the direction it +// currently claims. +func recastRowFacts(t *testing.T, w *recastWorld) (currentActivityID, direction string) { + t.Helper() + require.NoError(t, w.db.QueryRow(` + SELECT current_activity_id, direction + FROM outbound_votes WHERE vote_at_uri = $1`, + "at://"+tpNativeDID+"/social.coves.feed.vote/"+tpVoteRKey). + Scan(¤tActivityID, &direction)) + return currentActivityID, direction +} + +// poisonedVoteActivity is the activity id of the delivery that failed for good. +// One row, because the callers assert there is exactly one. +func poisonedVoteActivity(t *testing.T, w *recastWorld) string { + t.Helper() + var id string + require.NoError(t, w.db.QueryRow( + `SELECT activity_id FROM outbound_deliveries WHERE state = 'poisoned'`).Scan(&id)) + return id +} + // sweep runs the real reconciler over this world and returns its report. func sweep(t *testing.T, w *recastWorld) ingest.DivergenceReport { t.Helper() @@ -116,13 +141,32 @@ require.Equal(t, string(store.DeliveredStateDelivered), w.state(t)) held := deliveredVoteActivity(t, w) // --- STEP 2: the user changes their mind. The SAME record is rewritten, so - // the row resets to pending under a new activity id while the peer's - // copy of the old vote is untouched. + // the row moves to a new activity id while the peer's copy of the old + // vote is untouched. w.sender.fail(fmt.Errorf("lemmy is unreachable")) w.castVote(t, "3lztprev00002", directionUp) - require.Equal(t, string(store.DeliveredStatePending), w.state(t), - "precondition: the re-cast reset the row — this is the step that erases our record "+ - "of what the peer holds") + require.Equal(t, string(store.DeliveredStateDelivered), w.state(t), + "precondition: the re-cast KEEPS the delivered state. A flip REPLACES a vote the peer "+ + "still holds rather than withdrawing it, so the record that a delivery happened "+ + "survives it — what erases our accounting of the OLD activity is the id below "+ + "moving, which is exactly why the class still fires") + + // The two residuals, pinned here so they are recorded rather than + // rediscovered: they are what the flip still moves, and the class is built + // on the first of them. + currentID, direction := recastRowFacts(t, w) + require.NotEqual(t, held, currentID, + "precondition: current_activity_id has MOVED off the activity the peer accepted. The "+ + "ledger's claim is the PAIR (id, delivered) — the divergence query's first "+ + "exclusion matches on both — so a delivered flag pointing at a different "+ + "activity accounts for nothing about the old one") + require.Equal(t, directionUp, direction, + "and the row already reads the NEW direction while the peer demonstrably holds the "+ + "OLD one (a Dislike, above). KNOWN AND ACCEPTED for the poison window: this "+ + "column answers 'does the peer hold a vote of ours', not 'which way did it go'. "+ + "The reseed subtracts by direction, so while this window is open it subtracts an "+ + "up-vote the peer never received — which is the condition the divergence below "+ + "exists to surface, not one the row itself can express") // --- STEP 3: and the new vote never lands. w.deliver(t) @@ -132,16 +176,20 @@ `SELECT COUNT(*) FROM outbound_deliveries WHERE state = 'poisoned'`).Scan(&poisoned)) require.Equal(t, 1, poisoned, "precondition: the re-cast's delivery POISONED, which is what makes this permanent "+ "rather than a moment in flight") + require.Equal(t, currentID, poisonedVoteActivity(t, w), + "and the id the row moved to is the POISONED one: our accounting now names a vote "+ + "nobody received, and nothing re-drives a poisoned delivery to correct it") // --- THEN: the store names the pair, citing what the peer is holding. found, err := store.NewDivergences(w.db).RecastDivergences(ctx) require.NoError(t, err) require.Len(t, found, 1, "exactly one divergence: the peer is counting a Dislike this bridge no longer claims. "+ - "Our vote row says 'pending' — it was reset by the re-cast — so nothing in the "+ - "ledger records that a vote of ours stands on that instance, and the reseed "+ - "subtracts only 'delivered' rows. Read from the vote row this condition is "+ - "invisible by construction; only the append-only activity history still knows") + "Our vote row says 'delivered' — but under the NEW id and the NEW direction, so "+ + "the OLD delivered activity matches no exclusion, and the reseed subtracts a "+ + "vote in a direction the peer never received. Read from the vote row this "+ + "condition is invisible by construction: the row is self-consistent and looks "+ + "settled. Only the append-only activity history still knows what was sent") assert.Equal(t, tpNativeDID, found[0].ActorDID) assert.Equal(t, w.subject, found[0].SubjectATURI, "the pair (actor, subject) IS the identity of a vote — only one may be live at a time — "+ @@ -232,7 +280,8 @@ // TestRecastDivergence_ASuccessfulRecastIsNotADivergence is the ORDINARY vote // flip, and it is most of what this table does. // // A flip is an in-place upsert: current_activity_id moves to the new id, -// delivered_state resets to pending, and NO Undo is enqueued — Lemmy takes a +// delivered_state is KEPT — the flip replaces a vote the peer still holds +// rather than withdrawing it — and NO Undo is enqueued: Lemmy takes a // bare opposite vote as a replacement (17b measured this; a flip is not an Undo // followed by a vote). So once the new vote delivers, the OLD delivered activity // matches NEITHER exclusion: the ledger names the new id, and no Undo exists to diff --git a/internal/votes/reseed_recast_cost_test.go b/internal/votes/reseed_recast_cost_test.go new file mode 100644 --- /dev/null +++ b/internal/votes/reseed_recast_cost_test.go @@ -0,0 +1,136 @@ +package votes + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "tidepool/internal/store" +) + +// THE ACCEPTED COST OF THE RE-CAST GUARD, WRITTEN DOWN. +// +// The guard (FOLLOWUPS 17e leg 1) stops a re-cast from resetting delivered_state +// to 'pending'. It buys the thing that matters: a flipped vote stays visible to +// the erasure purge and to this seed, instead of vanishing from both the moment +// the user changes their mind. What it costs is that ONE row now carries two +// facts from different moments — `delivered`, which is true of the OLD activity +// the peer accepted, and `direction`, which is already the NEW one. +// +// The seed subtracts BY DIRECTION (SeedAggregates' `ours` term), so for as long +// as the new delivery has not landed, it gets the subtraction wrong in both +// directions at once: +// +// the up the peer IS holding → not subtracted (the row no longer says 'up') +// the down the peer is NOT → subtracted (the row says 'down' now) +// +// Neither error is silent-by-design — the second one drives the baseline +// negative, and the clamp signal is exactly the thing 17b built to make that +// observable. But the window is real, and the alternative was strictly worse: +// before the guard the row read 'pending', so it was not `ours` at all, the +// purge could not enumerate it, and nothing anywhere recorded that a vote of +// ours stood on that instance. A wrong-by-one subtraction that FIRES A COUNTER +// beats a vote nobody can see. +// +// This is characterization: it passes on the day it is written. Its job is to +// make the drift a recorded number rather than something an operator rediscovers +// from a score that will not add up. + +// flipDeliveredVote drives ONE outbound row through the two-step history this +// file is about — delivered as an up-vote, then re-cast down — through the real +// store, so the GUARD is what leaves delivered_state where it is. A hand-written +// row would be whatever steady state a fixture author picked; the whole +// condition here is a row whose history has two steps that disagree. +func flipDeliveredVote(t *testing.T, agg *Aggregator, subjectAPID, subjectATURI string) *store.OutboundVote { + t.Helper() + ctx := context.Background() + votes := store.NewOutboundVotes(agg.db) + + const did = "did:plc:recastcostpersona" + const voteATURI = "at://" + did + "/social.coves.feed.vote/3lzrecastcost1" + row := store.OutboundVote{ + VoteATURI: voteATURI, + ActorDID: did, + SubjectATURI: subjectATURI, + SubjectAPID: subjectAPID, + CommunityDID: rsCommunityDID, + Direction: directionUp, + CurrentActivityID: "https://coves.social/ap/activity/recast-cost-0", + DeliveredState: store.DeliveredStatePending, + } + + _, err := votes.Upsert(ctx, row) + require.NoError(t, err) + require.NoError(t, votes.SetDeliveredState(ctx, voteATURI, store.DeliveredStateDelivered), + "the peer accepted the UP-vote — this is the fact the guard exists to preserve") + + // The flip. Same record, opposite direction, a new activity id, and the + // 'pending' the consumer states on every cast because it records intent. + row.Direction = directionDown + row.CurrentActivityID = "https://coves.social/ap/activity/recast-cost-1" + row.DeliveredState = store.DeliveredStatePending + flipped, err := votes.Upsert(ctx, row) + require.NoError(t, err) + return flipped +} + +// TestReseedDuringARecastWindowMisreadsBothDirections pins the arithmetic of the +// window, exactly as it is. +func TestReseedDuringARecastWindowMisreadsBothDirections(t *testing.T) { + agg, logs, objects := clampWorld(t) + ctx := context.Background() + subjectATURI := bridgeSubject(t, objects, subjectPost, "3lzrecastcost1") + + flipped := flipDeliveredVote(t, agg, subjectPost, subjectATURI) + require.Equal(t, store.DeliveredStateDelivered, flipped.DeliveredState, + "precondition: the guard KEPT the delivered state through the flip — without it this "+ + "row would read 'pending', drop out of the `ours` term entirely, and the whole "+ + "window below would be invisible instead of merely wrong") + require.Equal(t, directionDown, flipped.Direction, + "precondition: while the direction is ALREADY the new one — the two facts this row now "+ + "carries come from different moments") + + // The origin's totals still contain the up-vote the peer is holding, and + // nothing of the down that has not been delivered. No inbound vote_events + // exist: the echo of our own vote has not come back either. + oursBefore := SeedOursSubtracted.Value() + clampedBefore := SeedBaselineClamped.Value() + require.NoError(t, agg.SeedAggregates(ctx, subjectPost, 1, 0)) + + seededUp, seededDown := seededCounts(t, agg.db, subjectPost) + assert.Equal(t, 1, seededUp, + "1 origin − 0 live − 0 ours: the up the peer IS holding was NOT subtracted, because "+ + "the row no longer says 'up'. It therefore stays in the baseline, and when the "+ + "echo of that vote arrives as a live event it will be counted a second time") + assert.Equal(t, 0, seededDown, + "0 origin − 0 live − 1 ours = −1, floored by GREATEST(0, …): a down the peer has NOT "+ + "accepted was subtracted from a total that never contained it. The clamp is what "+ + "stops that becoming a negative served score") + + up, down, found := counts(t, agg.db, subjectPost) + require.True(t, found) + assert.Equal(t, 1, up) + assert.Equal(t, 0, down) + + // The two errors happen to cancel in the SERVED number here, which is + // precisely why the counters below are the assertion that matters: reading + // 1/0 off vote_aggregates, this subject looks perfectly healthy. + assert.Equal(t, oursBefore+1, SeedOursSubtracted.Value(), + "the flipped row IS counted among `ours` — this is the guard's payoff, and the one "+ + "number that distinguishes this state from the pre-guard one, where the row read "+ + "'pending' and was subtracted from nothing") + assert.Equal(t, clampedBefore+1, SeedBaselineClamped.Value(), + "and the mis-subtraction is SIGNALLED rather than swallowed: the down baseline went "+ + "negative, which is the condition 17b's clamp counter exists to surface. An "+ + "operator seeing this on one subject sees a flip mid-flight; seeing it on many "+ + "sees an origin discarding votes") + + line := logs.String() + assert.Contains(t, line, "direction=down", "the breach is down-only") + assert.Contains(t, line, "deficit_down=-1", "by exactly the one vote that was flipped away") + assert.Contains(t, line, "ours_down=1", + "and the line names it as OURS — the difference between 'we mis-subtracted our own "+ + "in-flight flip' and 'the origin lost somebody else's votes'") +}