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,