From e68e3fdcff984d8d5368db831505e1bbf0404c6f Mon Sep 17 00:00:00 2001 From: Bretton Date: Fri, 14 Aug 2026 10:55:57 -0700 Subject: [PATCH] fix(moderation): CAS the removal an edit inspected, pin the restore's current CID MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two TOCTOU-shaped gaps left by the terminality fix, both cases where closing the obvious hole left the adjacent one open. TERM-1: standingRemoval read the removal's code and discarded its CID, and Restore's removal-delete carried no precondition — so if a moderator replaced an admission-revoked removal with their own between the read and the commit, the edit deleted whichever removal was current, wrote the acceptance and ran the outbound side effect. The reversal the terminality branch exists to prevent, reachable again through a smaller window. The inspected removal's CID is now threaded into ExpectPrevCID; a mismatch commits nothing (side effect included) and re-runs the decision against the moderator's record. The vanished-removal direction no longer records a terminal ledger row for a removal that no longer stands, and the edit stays recoverable by redrive. PIN-1: restorePin read outbound_objects.LastCID — the last version FEDERATED — which the terminality fix guarantees is stale exactly when it matters, because an edit against a standing removal writes nothing outbound. A restore then signed the community's name to the pre-removal version: the one version we know the moderators did NOT reinstate, and one that may no longer resolve in the author's PDS. The admissions ledger's last evaluated CID is now the primary source, with LastCID as the documented fallback for a post whose latest decision IS what it federated (the common case) and for a deployment with no ledger wired. Both bite-proven. Test-side, both found by running the tests rather than reading the reports: the race fixture had discriminated Restore from AcceptSubject by the ABSENCE of a precondition, so the fix it drove made its own hook unreachable; and the ingest harness never wired the ModerationLedger that main.go wires, leaving restorePin's primary source dead under test while the fallback quietly passed. Co-Authored-By: Claude Opus 5 (1M context) --- internal/accept/admissions.go | 22 ++ internal/accept/engine.go | 106 ++++++--- internal/accept/outer_acceptance_test.go | 5 +- internal/accept/terminality_race_test.go | 233 ++++++++++++++++++++ internal/acceptrec/acceptrec.go | 17 +- internal/ingest/ingest_test.go | 9 +- internal/ingest/moderation_terminal_test.go | 181 +++++++++++++++ internal/materialize/acceptance.go | 39 +++- internal/materialize/materializer.go | 5 + 9 files changed, 574 insertions(+), 43 deletions(-) create mode 100644 internal/accept/terminality_race_test.go diff --git a/internal/accept/admissions.go b/internal/accept/admissions.go index 7600661..74640cd 100644 --- a/internal/accept/admissions.go +++ b/internal/accept/admissions.go @@ -154,6 +154,28 @@ func (a *Admissions) markModeration(ctx context.Context, op, communityDID, postU return nil } +// LastEvaluatedCID is the CID of the most recent version of a post this engine +// decided on. It satisfies materialize.ModerationLedger. +// +// It is the ONLY record of the current version for a post whose latest decision +// wrote nothing outward — an edit refused against a standing moderator removal +// writes no acceptance and enqueues nothing, so outbound_objects keeps naming +// the version that was removed. A missing row answers "" (no error): a post the +// engine never decided on has no evaluated version, and the caller falls back. +func (a *Admissions) LastEvaluatedCID(ctx context.Context, communityDID, postURI string) (string, error) { + var cid sql.NullString + err := a.db.QueryRowContext(ctx, + `SELECT evaluated_cid FROM admissions WHERE community_did = $1 AND post_uri = $2`, + communityDID, postURI).Scan(&cid) + if stderrors.Is(err, sql.ErrNoRows) { + return "", nil + } + if err != nil { + return "", fmt.Errorf("accept: read evaluated cid %s/%s: %w", communityDID, postURI, err) + } + return cid.String, nil +} + // Get returns the admission for a (community, post), or an error satisfying // errors.IsNotFound when the engine has never decided on it. func (a *Admissions) Get(ctx context.Context, communityDID, postURI string) (*Admission, error) { diff --git a/internal/accept/engine.go b/internal/accept/engine.go index b17459e..640a729 100644 --- a/internal/accept/engine.go +++ b/internal/accept/engine.go @@ -91,11 +91,20 @@ const ( // that records "removed / moderator-removed" answers 200 accepted/enqueued. var ErrModeratorRemovalStands = stderrors.New("accept: a moderator removal stands") -// errRemovalVanished reports that the removal AcceptSubject refused against was -// gone by the time its code was read — the moderators restored the post inside -// the window. It is RETRYABLE and self-healing: the retry's AcceptSubject finds -// no removal and admits the edit normally. -var errRemovalVanished = stderrors.New("accept: the standing removal vanished before its code could be read") +// errRemovalChanged reports that the removal an edit was deciding about is no +// longer the record it inspected — it vanished, or a different one replaced it. +// The decision is re-run against whatever stands now: an edit may only reverse +// the exact removal it read, and it may only record a terminal decision about +// one that is still there. +// +// It never escapes accept(): the retry loop consumes it. +var errRemovalChanged = stderrors.New("accept: the standing removal changed mid-decision") + +// maxRemovalDecisionAttempts bounds that loop. Each pass is one repo read plus +// one commit attempt against a record a moderator is concurrently rewriting; +// three is generous for a human-paced race and cheap to spend, and running out +// surfaces as a retryable error rather than a guess. +const maxRemovalDecisionAttempts = 3 // RemovalCodeAdmissionRevoked is the removal `code` written when a post that WAS // accepted fails RE-admission (an edit made it titleless or over the cap). @@ -537,15 +546,29 @@ func (e *Engine) accept(ctx context.Context, did, communityDID, postURI string, }) } - _, err = acceptrec.AcceptSubject(ctx, e.repos, communityDID, postURI, commit.CID, - publishedAtOf(commit.Record), sideEffect) - if stderrors.Is(err, acceptrec.ErrRemovalStands) { - return e.editAgainstRemoval(ctx, did, communityDID, postURI, commit, sideEffect) - } - if err != nil { - return fmt.Errorf("accept: admit %s into %s: %w", postURI, communityDID, err) + // The decision spans two operations — read the standing removal, then act on + // it — and a moderator can write between them. Every such change re-runs the + // whole decision against the state that is actually there; nothing is + // decided from a record that has since moved. + for attempt := 0; ; attempt++ { + _, err = acceptrec.AcceptSubject(ctx, e.repos, communityDID, postURI, commit.CID, + publishedAtOf(commit.Record), sideEffect) + if stderrors.Is(err, acceptrec.ErrRemovalStands) { + err = e.editAgainstRemoval(ctx, did, communityDID, postURI, commit, sideEffect) + if stderrors.Is(err, errRemovalChanged) && attempt+1 < maxRemovalDecisionAttempts { + continue + } + if stderrors.Is(err, errRemovalChanged) { + return fmt.Errorf("accept: %s in %s: the standing removal kept changing across %d attempts", + postURI, communityDID, maxRemovalDecisionAttempts) + } + return err + } + if err != nil { + return fmt.Errorf("accept: admit %s into %s: %w", postURI, communityDID, err) + } + return nil } - return nil } // editAgainstRemoval decides what an edit may do when a removal already stands @@ -570,14 +593,14 @@ func (e *Engine) accept(ctx context.Context, did, communityDID, postURI string, func (e *Engine) editAgainstRemoval(ctx context.Context, did, communityDID, postURI string, commit *consume.CommitEvent, sideEffect repo.TxSideEffect) error { - code, err := e.standingRemovalCode(ctx, communityDID, postURI) + code, removalCID, err := e.standingRemoval(ctx, communityDID, postURI) if err != nil { return err } if code != RemovalCodeAdmissionRevoked { e.logger.Info("edit against a standing moderator removal: acceptance refused, nothing enqueued", "community_did", communityDID, "post", postURI, "removal_code", code) - if rerr := e.admissions.Record(ctx, Admission{ + terminal := Admission{ AuthorDID: did, CommunityDID: communityDID, PostURI: postURI, @@ -585,7 +608,20 @@ func (e *Engine) editAgainstRemoval(ctx context.Context, did, communityDID, post DecisionCode: DecisionModeratorRemoved, EvaluatedCID: commit.CID, EvaluatedSnapshot: e.evaluatedSnapshot(commit), - }); rerr != nil { + } + // Re-verify before recording: the ledger is what an operator reads to + // answer "why did my edit do nothing", and a moderator-removed row for a + // removal that has since been lifted answers with a removal nobody can + // find. The re-read is what turns "a removal stood when we looked" into + // "one stands now"; if it has moved, the whole decision re-runs. + current, currentCID, verr := e.standingRemoval(ctx, communityDID, postURI) + if verr != nil { + return verr + } + if currentCID != removalCID || current != code { + return errRemovalChanged + } + if rerr := e.admissions.Record(ctx, terminal); rerr != nil { return rerr } // The decision is complete; the sentinel only tells the CALLER what was @@ -594,22 +630,32 @@ func (e *Engine) editAgainstRemoval(ctx context.Context, did, communityDID, post // this very call refused to write. return ErrModeratorRemovalStands } + // CAS on the removal we actually inspected: between the read above and this + // commit a moderator may have replaced our admission-revoked removal with + // their own, and deleting whichever removal happens to be current is the + // reversal this whole branch exists to prevent — reachable again through a + // smaller window. A mismatch commits nothing (the side effect included) and + // re-runs the decision against their record. if _, rerr := acceptrec.Restore(ctx, e.repos, communityDID, postURI, commit.CID, - publishedAtOf(commit.Record), sideEffect); rerr != nil { + removalCID, publishedAtOf(commit.Record), sideEffect); rerr != nil { + if stderrors.Is(rerr, repo.ErrPreconditionFailed) { + return errRemovalChanged + } return fmt.Errorf("accept: restore %s into %s: %w", postURI, communityDID, rerr) } return nil } -// standingRemovalCode reads the `code` off the removal AcceptSubject refused -// against. The two ways this read can fail are different events and are -// reported differently: +// standingRemoval reads the removal AcceptSubject refused against: its `code`, +// which decides whose decision it is, and its CID, which is the token every +// later step is checked against. The two ways this read can fail are different +// events and are reported differently: // // - NOT FOUND is the benign race: the moderators restored the post between -// the refusal and this read. errRemovalVanished says so, and one retry -// resolves it — the retry's AcceptSubject finds no removal and admits the -// edit. Reporting it as "not ours, terminal" would strand a post whose -// removal no longer exists. +// the refusal and this read. errRemovalChanged says so and the decision +// re-runs — the next AcceptSubject finds no removal and admits the edit. +// Reporting it as "not ours, terminal" would strand a post whose removal no +// longer exists. // - ANYTHING ELSE is an infrastructure failure (the repo store is down, a // timeout). It propagates as itself, so the retry budget is spent on a // message about a broken read rather than about a "standing removal" that @@ -618,18 +664,18 @@ func (e *Engine) editAgainstRemoval(ctx context.Context, did, communityDID, post // A record whose `code` is absent or not a string yields "", which the caller // treats as a moderator's — terminal. That direction is deliberate: refusing an // edit is recoverable, and pushing a removed post back at its community is not. -func (e *Engine) standingRemovalCode(ctx context.Context, communityDID, postURI string) (string, error) { +func (e *Engine) standingRemoval(ctx context.Context, communityDID, postURI string) (code, cid string, err error) { rkey := acceptrec.SubjectRKey(postURI) - record, _, err := e.repos.GetRecord(ctx, communityDID, acceptrec.CollectionRemoval, rkey) + record, cid, err := e.repos.GetRecord(ctx, communityDID, acceptrec.CollectionRemoval, rkey) switch { case errors.IsNotFound(err): - return "", fmt.Errorf("%w: %s in %s", errRemovalVanished, postURI, communityDID) + return "", "", fmt.Errorf("%w: %s in %s", errRemovalChanged, postURI, communityDID) case err != nil: - return "", fmt.Errorf("accept: read standing removal %s/%s/%s: %w", + return "", "", fmt.Errorf("accept: read standing removal %s/%s/%s: %w", communityDID, acceptrec.CollectionRemoval, rkey, err) } - code, _ := record["code"].(string) - return code, nil + code, _ = record["code"].(string) + return code, cid, nil } // removeAccepted withdraws a post that WAS accepted and now fails re-admission: diff --git a/internal/accept/outer_acceptance_test.go b/internal/accept/outer_acceptance_test.go index 30a7d86..37dd331 100644 --- a/internal/accept/outer_acceptance_test.go +++ b/internal/accept/outer_acceptance_test.go @@ -262,7 +262,10 @@ func realEnqueuer(t *testing.T, conn *sql.DB) *outbound.Enqueuer { return enq } -func wireEngine(t *testing.T, conn *sql.DB, repos *repo.Manager, enqueuer consume.OutboundEnqueuer) *Engine { +// wireEngine takes the RepoManager INTERFACE, not *repo.Manager, so a test can +// interpose on the two operations the terminality decision spans (the removal +// read and the commit that acts on it) without a second wiring helper. +func wireEngine(t *testing.T, conn *sql.DB, repos acceptrec.RepoManager, enqueuer consume.OutboundEnqueuer) *Engine { t.Helper() engine, err := NewEngine(Options{ Repos: repos, diff --git a/internal/accept/terminality_race_test.go b/internal/accept/terminality_race_test.go new file mode 100644 index 0000000..27bca3e --- /dev/null +++ b/internal/accept/terminality_race_test.go @@ -0,0 +1,233 @@ +package accept + +import ( + "context" + "sync" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "tidepool/internal/acceptrec" + "tidepool/internal/repo" +) + +// TERMINALITY IS DECIDED ACROSS TWO OPERATIONS, AND THE GAP IS THE BUG. +// +// The edit path reads the standing removal's `code` and then acts on it: +// admission-revoked auto-restores (delete the removal, write an acceptance, run +// the outbound side effect), anything else is terminal. Between those two +// operations the removal can CHANGE — and acceptrec.Restore deletes whichever +// removal is current, not the one that was inspected. +// +// So the moderation reversal we closed at the frame above is still reachable +// through a narrower window: read admission-revoked, moderator replaces it with +// their own removal, restore deletes theirs. The window is small; the loss is +// the same, and it is silent — the firehose shows an acceptance, and the +// moderators' record is simply gone. +// +// The property is one sentence: AN EDIT MAY ONLY REVERSE THE EXACT REMOVAL IT +// INSPECTED. acceptrec already does compare-and-set with ExpectPrevCID on the +// acceptance for the same reason; these tests pin the behaviour, not that +// mechanism. + +// racingRepos interposes on the two operations the decision spans. It wraps a +// real *repo.Manager, so every commit and read is the production path — only the +// interleaving is arranged. +type racingRepos struct { + acceptrec.RepoManager + + mu sync.Mutex + // afterRemovalRead fires once, after the Nth read of a removal record, and + // receives the manager so it can mutate the repo. N counts from 1. + afterRemovalRead func() + readsBeforeFire int + removalReads int + fired bool + + // beforeRemovalDelete fires once, immediately before a commit that deletes a + // removal — i.e. exactly when the moderator's write would land between the + // read and the restore. + beforeRemovalDelete func() + deleteFired bool +} + +func (r *racingRepos) GetRecord(ctx context.Context, did, collection, rkey string) (map[string]any, string, error) { + record, cid, err := r.RepoManager.GetRecord(ctx, did, collection, rkey) + if collection != acceptrec.CollectionRemoval { + return record, cid, err + } + r.mu.Lock() + r.removalReads++ + fire := r.afterRemovalRead != nil && !r.fired && r.removalReads >= r.readsBeforeFire + if fire { + r.fired = true + } + hook := r.afterRemovalRead + r.mu.Unlock() + if fire { + hook() + } + return record, cid, err +} + +func (r *racingRepos) ApplyOpsTx(ctx context.Context, did string, ops []repo.RecordOp, sideEffect repo.TxSideEffect) (*repo.CommitResult, error) { + // WHICH removal-delete is the restore's? Not "the one without a + // precondition" — that was true only until Restore grew one, and a fixture + // whose discriminator the fix invalidates stops firing silently. + // + // The stable difference is what each precondition MEANS: + // + // AcceptSubject: ExpectPrevCID = "" — "expect NO removal to exist" + // Restore: ExpectPrevCID = — "expect THIS removal to exist" + // + // An implementation cannot swap those without inverting what the operations + // do, so this discriminator survives any correct version of the fix — + // including the one that has no precondition at all, which is the build this + // test has to go red against. + deletesRemoval := false + for _, op := range ops { + if op.Action != repo.OpActionDelete || op.Collection != acceptrec.CollectionRemoval { + continue + } + if op.ExpectPrevCID == nil || *op.ExpectPrevCID != "" { + deletesRemoval = true + } + } + r.mu.Lock() + fire := deletesRemoval && r.beforeRemovalDelete != nil && !r.deleteFired + if fire { + r.deleteFired = true + } + hook := r.beforeRemovalDelete + r.mu.Unlock() + if fire { + hook() + } + return r.RepoManager.ApplyOpsTx(ctx, did, ops, sideEffect) +} + +// TestEditCannotDeleteARemovalItNeverInspected is the forward race, and the one +// that loses the moderator's decision. +// +// The engine reads admission-revoked — OUR removal, correctly reversible — and +// while it is deciding, a moderator removes the post for their own reasons. The +// restore then deletes the record it never read. +func TestEditCannotDeleteARemovalItNeverInspected(t *testing.T) { + conn := acceptanceDB(t) + ctx := context.Background() + repos := newRepos(t, conn) + seedBridgedCommunity(t, conn) + + racing := &racingRepos{RepoManager: repos} + enqueuer := realEnqueuer(t, conn) + engine := wireEngine(t, conn, racing, enqueuer) + dispatcher := wireDispatcher(t, conn, engine, enqueuer) + + // A post is accepted, then an edit fails admission: OUR removal stands. + admittedCreate(t, dispatcher) + require.NoError(t, dispatcher.HandleEvent(ctx, + postEvent("update", acPostRKey, acRevUpdate, acPostCID2, acPostTimeUS+1, + pv2Record(func(r map[string]any) { r["title"] = "" })))) + removal, ok := removalStandsAt(t, repos, acCommunityDID, acPostURI) + require.True(t, ok, "precondition: an admission-revoked removal stands") + require.Equal(t, RemovalCodeAdmissionRevoked, removal["code"]) + + // THE RACE: a moderator replaces it with their own removal in the window + // between the engine reading the code and the restore committing. + racing.beforeRemovalDelete = func() { + _, err := acceptrec.Remove(ctx, repos, acCommunityDID, acPostURI, acPostCID, + "moderator-discretion", "removed by a human", time.Now(), nil) + require.NoError(t, err, "the moderator's removal lands mid-decision") + } + + activitiesBefore := activityKindCount(t, conn, "Update") + createsBefore := activityKindCount(t, conn, "Create") + + // The corrective edit: admission passes now. + editErr := dispatcher.HandleEvent(ctx, + postEvent("update", acPostRKey, acRevDelete, acPostCID2, acPostTimeUS+2, pv2Record())) + require.True(t, racing.deleteFired, "the fixture must have raced the commit") + // Logged, not asserted: the reversal below happens with the edit reporting + // SUCCESS, which is why nothing upstream notices. + t.Logf("the edit reported: %v", editErr) + + surviving, ok := removalStandsAt(t, repos, acCommunityDID, acPostURI) + assert.True(t, ok, + "the MODERATOR's removal must survive: the edit inspected a different record, and "+ + "deleting whichever removal happens to be current is the reversal we already "+ + "closed once — reachable again through a smaller window") + if ok { + assert.Equal(t, "moderator-discretion", surviving["code"], + "and it must still be theirs, not replaced by our restore's idea of the state") + } + + _, accepted := acceptanceSubjectCID(t, repos, acCommunityDID, acPostURI) + assert.False(t, accepted, + "no acceptance may be written against a removal we never read: on the firehose that "+ + "is the post coming back, published by the community that removed it") + + assert.Equal(t, activitiesBefore, activityKindCount(t, conn, "Update"), + "and nothing may be enqueued — the side effect rides the restore commit, so a "+ + "restore that should not have happened pushes the post back at the community") + assert.Equal(t, createsBefore, activityKindCount(t, conn, "Create")) +} + +// TestVanishedRemovalIsNotRecordedAsTerminal is the reverse race. +// +// The engine reads a moderator-discretion removal and decides the edit is +// terminal — but the moderators lift the removal before that decision is +// recorded. The ledger then carries a moderator-removed row for a post that is +// not removed, which is the surface an operator reads when the author asks why +// their edit did nothing. +// +// The edit must stay recoverable: whatever the engine does with the decision, a +// redrive must admit the post, because by then nothing stands against it. +func TestVanishedRemovalIsNotRecordedAsTerminal(t *testing.T) { + conn := acceptanceDB(t) + ctx := context.Background() + repos := newRepos(t, conn) + seedBridgedCommunity(t, conn) + + racing := &racingRepos{RepoManager: repos} + enqueuer := realEnqueuer(t, conn) + engine := wireEngine(t, conn, racing, enqueuer) + dispatcher := wireDispatcher(t, conn, engine, enqueuer) + + admittedCreate(t, dispatcher) + _, err := acceptrec.Remove(ctx, repos, acCommunityDID, acPostURI, acPostCID, + "moderator-discretion", "removed by a human", time.Now(), nil) + require.NoError(t, err, "precondition: a moderator removal stands") + + // THE RACE: the moderators restore the post after the engine has read their + // removal and before it records the decision. The second removal read is the + // engine's own (the first belongs to acceptrec's removal guard). + racing.readsBeforeFire = 2 + racing.afterRemovalRead = func() { + _, rerr := repos.ApplyOps(ctx, acCommunityDID, []repo.RecordOp{ + {Action: repo.OpActionDelete, Collection: acceptrec.CollectionRemoval, + RKey: acceptrec.SubjectRKey(acPostURI)}, + }) + require.NoError(t, rerr, "the moderators lift the removal mid-decision") + } + + _ = dispatcher.HandleEvent(ctx, + postEvent("update", acPostRKey, acRevUpdate, acPostCID2, acPostTimeUS+1, pv2Record())) + require.True(t, racing.fired, "the fixture must have raced the decision") + + status, code := admissionOf(t, conn, acCommunityDID, acPostURI) + assert.NotEqual(t, StatusRemoved, status, + "the ledger must not record a terminal moderator removal that no longer stands: it "+ + "is what an operator reads to answer 'why did my edit do nothing', and here the "+ + "answer would be a removal nobody can find (code=%q)", code) + + // And the edit is recoverable: a redrive finds no removal and admits it. + require.NoError(t, dispatcher.HandleEvent(ctx, + postEvent("update", acPostRKey, acRevDelete, acPostCID2, acPostTimeUS+2, pv2Record()))) + cid, accepted := acceptanceSubjectCID(t, repos, acCommunityDID, acPostURI) + assert.True(t, accepted, + "a redrive after the removal was lifted must admit the edit — otherwise a post the "+ + "moderators reinstated stays invisible until its author edits again") + assert.Equal(t, acPostCID2, cid, "pinning the version the redrive evaluated") +} diff --git a/internal/acceptrec/acceptrec.go b/internal/acceptrec/acceptrec.go index 5c35355..f4a01dc 100644 --- a/internal/acceptrec/acceptrec.go +++ b/internal/acceptrec/acceptrec.go @@ -253,7 +253,20 @@ func Remove(ctx context.Context, repos RepoManager, communityDID, subjectURI, su // AcceptSubject, it does NOT trip the removal guard: deleting the removal is the // point. createdAt is derived from publishedAt (a fresh acceptance stands for the // current version), so a redelivery re-puts byte-identical bytes. -func Restore(ctx context.Context, repos RepoManager, communityDID, subjectURI, subjectCID string, publishedAt time.Time, sideEffect repo.TxSideEffect) (*repo.CommitResult, error) { +// expectRemovalCID is a REQUIRED compare-and-set token: the CID of the removal +// the caller inspected before deciding to reverse it. The delete only applies +// if that exact record is still standing, so an edit can never delete a removal +// it never read — a moderator replacing our admission-revoked removal with +// their own in the decision window would otherwise have theirs deleted, the +// acceptance written over it, and the side effect (the outbound enqueue) fired, +// pushing the post back at the community that just removed it. On a mismatch +// the commit returns repo.ErrPreconditionFailed and NOTHING runs, side effect +// included. +func Restore(ctx context.Context, repos RepoManager, communityDID, subjectURI, subjectCID, expectRemovalCID string, publishedAt time.Time, sideEffect repo.TxSideEffect) (*repo.CommitResult, error) { + if expectRemovalCID == "" { + return nil, errors.NewValidationError("expect_removal_cid", + "a restore must name the removal it inspected") + } rkey := SubjectRKey(subjectURI) acceptance := map[string]any{ "$type": CollectionAcceptance, @@ -264,7 +277,7 @@ func Restore(ctx context.Context, repos RepoManager, communityDID, subjectURI, s // so the firehose never shows a window where the post is neither removed nor // accepted. res, err := repos.ApplyOpsTx(ctx, communityDID, []repo.RecordOp{ - {Action: repo.OpActionDelete, Collection: CollectionRemoval, RKey: rkey}, + {Action: repo.OpActionDelete, Collection: CollectionRemoval, RKey: rkey, ExpectPrevCID: &expectRemovalCID}, {Action: repo.OpActionUpdate, Collection: CollectionAcceptance, RKey: rkey, Record: acceptance}, }, sideEffect) if err != nil { diff --git a/internal/ingest/ingest_test.go b/internal/ingest/ingest_test.go index 25ddbf3..5f64f7b 100644 --- a/internal/ingest/ingest_test.go +++ b/internal/ingest/ingest_test.go @@ -22,6 +22,7 @@ import ( "github.com/go-chi/chi/v5" "github.com/stretchr/testify/require" + "tidepool/internal/accept" "tidepool/internal/ap" "tidepool/internal/echo" "tidepool/internal/identity" @@ -328,7 +329,13 @@ func newHarness(t *testing.T) *harness { // so the origin dispatch, the tombstone refusal and the empty-CID // refusal are all dead code in every ingest test — passing by never // running. - OutboundObjects: store.NewOutboundObjects(database), + OutboundObjects: store.NewOutboundObjects(database), + // Same rule, same reason (main.go passes *accept.Admissions here): with + // a nil ledger restorePin silently takes its LastCID FALLBACK, so the + // primary source is never exercised and a restore pins whatever was last + // federated — which is the pre-removal version exactly when an author + // edited while removed. + Ledger: accept.NewAdmissions(database), ServiceDID: testServiceDID, StrictValidation: true, }) diff --git a/internal/ingest/moderation_terminal_test.go b/internal/ingest/moderation_terminal_test.go index edc352c..e82e79a 100644 --- a/internal/ingest/moderation_terminal_test.go +++ b/internal/ingest/moderation_terminal_test.go @@ -13,6 +13,7 @@ import ( "tidepool/internal/accept" "tidepool/internal/consume" + "tidepool/internal/echo" "tidepool/internal/errors" "tidepool/internal/materialize" "tidepool/internal/outbound" @@ -446,3 +447,183 @@ func TestAnnouncedRestoreOfANativePostLiftsTheRemovalCleanly(t *testing.T) { "and the restore is DECIDED, not left retrying: %s", event.Error) assert.Nil(t, event.FailedAt, "nor poisoned") } + +// TestRestoredAcceptancePinsTheEditedVersion is PIN-1. +// +// RestorePost's own doc says the fresh acceptance pins "the post's CURRENT +// version, not the one that was removed: the author may have edited it while it +// was out". Terminality made that false in the common case — an edit against a +// standing moderator removal is refused and writes NOTHING to outbound_objects, +// so LastCID keeps naming the pre-removal version, which is exactly the one that +// was removed. +// +// The consequence is not internal. The community SIGNS an acceptance whose +// strongRef names a CID that may no longer resolve in the author's PDS, and the +// "self-heals on the author's next edit" argument rests on an edit that may +// never come — the author has no reason to edit again, because from their side +// the post is back. +// +// The assertion is on the OUTCOME, not the source: an authority-pinned +// getRecord and the terminal admission's EvaluatedCID both satisfy it, and which +// one is right is GREEN's call. +func TestRestoredAcceptancePinsTheEditedVersion(t *testing.T) { + h := newHarness(t) + ctx := context.Background() + world := newModerationWorld(t, h) + + // A moderator removes the post. + reason := "removed pending an edit" + h.announceDeleteWithSummary(world.groupA, + "https://lemmy.world/activities/announce/delete/mt-pin", mtPostAPID, &reason) + require.True(t, removalStandsFor(t, h, world), "precondition: the removal stands") + + // The author edits it WHILE REMOVED. The edit is refused (terminal) and + // writes nothing outbound — which is precisely why LastCID goes stale. + require.NoError(t, world.dispatcher.HandleEvent(ctx, + mtPostEvent(t, "update", mtEditRev, mtEditCID, mtEditTime))) + require.True(t, removalStandsFor(t, h, world), + "precondition: the edit did not reverse the removal (17c-1's terminality)") + + // The moderators reconsider and restore it. + h.announceUndoDelete(world.groupA, + "https://lemmy.world/activities/announce/undo/mt-pin", + "https://lemmy.world/activities/announce/delete/mt-pin/delete", + mtPostAPID, &reason) + + acceptance, _, err := h.manager.GetRecord(ctx, + world.communityADID, materialize.CollectionAcceptance, world.digestRKey) + require.NoError(t, err, "the restore must re-accept the post") + subject, ok := acceptance["subject"].(map[string]any) + require.True(t, ok, "the acceptance carries a strongRef, got %#v", acceptance["subject"]) + + assert.Equal(t, mtEditCID, subject["cid"], + "the acceptance must pin the CURRENT record: the author edited while the post was "+ + "out, and pinning the pre-removal CID signs the community's name to a version "+ + "that may no longer resolve in the author's PDS — the one version we know the "+ + "moderators did NOT reinstate") + assert.NotEqual(t, mtPostCID, subject["cid"], + "and specifically not the version that was removed") +} + +// removalStandsFor reports whether community A currently holds a removal for the +// fixture's post. +func removalStandsFor(t *testing.T, h *harness, world moderationWorld) bool { + t.Helper() + _, _, err := h.manager.GetRecord(context.Background(), + world.communityADID, materialize.CollectionRemoval, world.digestRKey) + if errors.IsNotFound(err) { + return false + } + require.NoError(t, err) + return true +} + +// TestSummarylessCrossCommunityDeleteIsRefused bounds the one attribution the +// announced-delete path takes on trust. +// +// moderateAnnouncedDelete's summary-less branch asks whether the INNER Delete's +// actor is the post's author — an unverified claim, since only the announcing +// community's signature is checked. The bound is that authorization runs FIRST: +// whatever the inner actor claims, the announcer must own the target's mapping, +// so at most a community can withdraw a post from ITSELF. +// +// The inner actor here is a LEMMY moderator, not our persona: a summary-less +// delete attributed to one of OUR personas never reaches this branch at all (see +// the sibling test below), so attributing it that way would pin the echo guard +// while claiming to pin authorization. +func TestSummarylessCrossCommunityDeleteIsRefused(t *testing.T) { + h := newHarness(t) + ctx := context.Background() + world := newModerationWorld(t, h) + + h.announceDeleteBy(world.groupB, + "https://lemmy.world/activities/announce/delete/mt-summaryless", + mtPostAPID, modActorID, nil) + + _, _, err := h.manager.GetRecord(ctx, + world.communityADID, materialize.CollectionRemoval, world.digestRKey) + assert.True(t, errors.IsNotFound(err), + "a community that does not own the mapping decides nothing about it, whoever the "+ + "inner activity claims to be (err=%v)", err) + _, _, err = h.manager.GetRecord(ctx, + world.communityADID, materialize.CollectionAcceptance, world.digestRKey) + assert.NoError(t, err, "A's acceptance is untouched") + + mapping, err := h.objects.GetByAPID(ctx, mtPostAPID) + require.NoError(t, err) + assert.False(t, mapping.IsDeleted(), + "and the post is not deleted either: the summary-less branch's other outcome is the "+ + "author's own delete, which would destroy the record") +} + +// TestSummarylessDeleteAttributedToOurPersonaIsDroppedAsAnEcho records where the +// unverified attribution actually lands for NATIVE content. +// +// A native post's author IS one of our personas, so a truthful summary-less +// self-delete announced back by the community is indistinguishable from our own +// Delete coming home — and the echo classifier takes it first, by the inner +// ACTOR, before any authorization runs. +// +// That is the M1 behaviour working as designed, and it means the attribution +// this branch trusts is unreachable for native posts from either direction: a +// forged persona attribution is dropped as an echo, and a foreign attribution is +// bounded by the ownership conjunct above. +func TestSummarylessDeleteAttributedToOurPersonaIsDroppedAsAnEcho(t *testing.T) { + h := newHarness(t) + ctx := context.Background() + world := newModerationWorld(t, h) + before := dropSnapshot() + + // Community A — the OWNER, so ownership cannot be what refuses this. + h.announceDeleteBy(world.groupA, + "https://lemmy.world/activities/announce/delete/mt-persona-attributed", + mtPostAPID, mtUserOrigin+"/ap/actor/"+mtAuthorDID, nil) + + _, _, err := h.manager.GetRecord(ctx, + world.communityADID, materialize.CollectionAcceptance, world.digestRKey) + assert.NoError(t, err, "the post stays accepted") + mapping, err := h.objects.GetByAPID(ctx, mtPostAPID) + require.NoError(t, err) + assert.False(t, mapping.IsDeleted(), "and its record is not destroyed") + + assert.Equal(t, before[echo.ClassLocalActor]+1, echo.Drops(echo.ClassLocalActor), + "it is dropped as an ECHO, by the inner actor — which is what makes the summary-less "+ + "branch's unverified author attribution unreachable for native posts") +} + +// TestRestoreWithNoInterveningEditPinsTheFederatedVersion covers the FALLBACK +// half of restorePin's pair. +// +// PIN-1 arose because a primary/fallback pair had only its fallback exercised; +// wiring the ledger fixes that and creates the mirror risk — every restore now +// takes the primary, and LastCID's correctness stops being tested at all. +// +// The case that must still work is the ordinary one: a moderator removes a post +// and reinstates it with NO author edit in between. The ledger holds no decision +// for that post beyond its acceptance, so the pin comes from outbound state — +// and there it is right, because nothing has changed since it was federated. +func TestRestoreWithNoInterveningEditPinsTheFederatedVersion(t *testing.T) { + h := newHarness(t) + ctx := context.Background() + world := newModerationWorld(t, h) + + reason := "removed and reinstated, no edit in between" + h.announceDeleteWithSummary(world.groupA, + "https://lemmy.world/activities/announce/delete/mt-nofallback", mtPostAPID, &reason) + require.True(t, removalStandsFor(t, h, world), "precondition: the removal stands") + + h.announceUndoDelete(world.groupA, + "https://lemmy.world/activities/announce/undo/mt-nofallback", + "https://lemmy.world/activities/announce/delete/mt-nofallback/delete", + mtPostAPID, &reason) + + acceptance, _, err := h.manager.GetRecord(ctx, + world.communityADID, materialize.CollectionAcceptance, world.digestRKey) + require.NoError(t, err, "the restore re-accepts the post") + subject, ok := acceptance["subject"].(map[string]any) + require.True(t, ok) + assert.Equal(t, mtPostCID, subject["cid"], + "with no edit to supersede it, the version we federated IS the current one — the "+ + "fallback has to be right for the common case, or fixing the stale pin just moves "+ + "the staleness into whichever branch nobody exercises") +} diff --git a/internal/materialize/acceptance.go b/internal/materialize/acceptance.go index 5622eb6..62293e4 100644 --- a/internal/materialize/acceptance.go +++ b/internal/materialize/acceptance.go @@ -99,6 +99,11 @@ func (m *Materializer) removalStands(ctx context.Context, communityDID, rkey str } } +// removalCodeModeratorDiscretion is the removal lexicon's catch-all: Lemmy +// sends no machine-readable code, so anything narrower would be the bridge +// asserting a reason the moderator never gave. +const removalCodeModeratorDiscretion = "moderator-discretion" + // RemovePost records a community's moderator removal of a post: the acceptance // is deleted and a removal written IN ONE COMMIT, at the same digest rkey. // @@ -112,11 +117,6 @@ func (m *Materializer) removalStands(ctx context.Context, communityDID, rkey str // deleting the author's record would let one community destroy content for // every other, and tombstoning the mapping would block the post's later edits // and votes from ever materializing again. -// removalCodeModeratorDiscretion is the removal lexicon's catch-all: Lemmy -// sends no machine-readable code, so anything narrower would be the bridge -// asserting a reason the moderator never gave. -const removalCodeModeratorDiscretion = "moderator-discretion" - func (m *Materializer) RemovePost(ctx context.Context, mapping *store.APObjectMapping, reason string) error { communityDID, postURI, rkey, err := m.moderationTarget(ctx, mapping) if err != nil { @@ -207,7 +207,7 @@ func (m *Materializer) RestorePost(ctx context.Context, mapping *store.APObjectM return nil } - currentCID, ok, err := m.restorePin(ctx, mapping) + currentCID, ok, err := m.restorePin(ctx, communityDID, mapping) if err != nil { return err } @@ -281,7 +281,7 @@ func (m *Materializer) recordModeration(ctx context.Context, mapping *store.APOb // outbound row is the sharpest case: the author deleted the post while it was // removed, so restoring would publish an acceptance for a record that no longer // exists — the community asserting it admitted something deleted. -func (m *Materializer) restorePin(ctx context.Context, mapping *store.APObjectMapping) (string, bool, error) { +func (m *Materializer) restorePin(ctx context.Context, communityDID string, mapping *store.APObjectMapping) (string, bool, error) { if mapping.Origin != store.OriginBridge { _, cid, err := m.repos.GetRecord(ctx, mapping.DID, mapping.Collection, mapping.RKey) if err != nil { @@ -314,12 +314,33 @@ func (m *Materializer) restorePin(ctx context.Context, mapping *store.APObjectMa "ap_id", mapping.APID, "at_uri", mapping.ATURI) return "", false, nil } - if state.LastCID == "" { + + // The pin is the version the acceptance ENGINE last decided on, not the one + // the bridge last FEDERATED, and after task 17c-1 those routinely differ: + // an edit against a standing removal is terminal, so it writes no acceptance + // and enqueues nothing — outbound_objects keeps naming the pre-removal + // version, which is the one version we know the moderators did NOT + // reinstate, and it may no longer resolve in the author's PDS at all. + // + // outbound state remains the fallback, for a post whose latest decision IS + // what it federated (the common case) and for a deployment with no ledger + // wired. + pin := state.LastCID + if m.ledger != nil { + evaluated, err := m.ledger.LastEvaluatedCID(ctx, communityDID, mapping.ATURI) + if err != nil { + return "", false, fmt.Errorf("materialize: read evaluated cid for %s: %w", mapping.ATURI, err) + } + if evaluated != "" { + pin = evaluated + } + } + if pin == "" { m.logger.Warn("restore target has no recorded CID to pin; leaving the removal in place", "ap_id", mapping.APID, "at_uri", mapping.ATURI) return "", false, nil } - return state.LastCID, true, nil + return pin, true, nil } // moderationTarget resolves the community, subject uri and digest rkey a diff --git a/internal/materialize/materializer.go b/internal/materialize/materializer.go index dda65ec..8da73e9 100644 --- a/internal/materialize/materializer.go +++ b/internal/materialize/materializer.go @@ -152,6 +152,11 @@ type ModerationLedger interface { // RecordRestore marks the post accepted again, pinning the CID the fresh // acceptance was written against. RecordRestore(ctx context.Context, communityDID, postURI, authorDID, cid string) error + // LastEvaluatedCID is the CID of the most recent version of the post the + // acceptance engine DECIDED on — including a decision that wrote nothing + // outward, which is exactly the case a restore has to pin. "" means the + // ledger knows of no decision for this (community, post). + LastEvaluatedCID(ctx context.Context, communityDID, postURI string) (string, error) } // Options configures New. Fetcher, Objects, Actors, Communities, Repos, -- 2.51.2