diff --git a/internal/core/posts/service_delete_compensation_test.go b/internal/core/posts/service_delete_compensation_test.go new file mode 100644 index 0000000..e3eb1c5 --- /dev/null +++ b/internal/core/posts/service_delete_compensation_test.go @@ -0,0 +1,278 @@ +//go:build integration + +package posts_test + +import ( + "context" + "testing" + "time" + + "Coves/internal/core/posts" + "Coves/internal/core/users" + "Coves/internal/db/postgres" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// A DELETE MUST COMPENSATE ITSELF, exactly as a create settles itself (§4.2 +// steps 4 and 5, §5.3). +// +// The create path does not wait for the firehose: it writes the postv2 into the +// author's repo, seeds the admission row, and — for a community THIS AppView +// hosts — writes the community's acceptance and stamps the row, all before it +// answers the author. settleSubmission is that compensation. +// +// The delete path has no such thing. It deletes the postv2 from the author's +// repo and returns, leaving the local index row, the community's acceptance +// record and the admission row to be cleaned up by the firehose consumer's +// tombstoneAuthorPost/withdrawAcceptance when the delete event comes back +// around. +// +// THE EVENT IS NOT GUARANTEED TO COME BACK. An author's PDS only reaches this +// AppView if it is on one of the configured jetstream feeds; a self-hosted PDS, +// a feed reconfiguration, or a consumer that is simply down for the afternoon +// all produce the same outcome, and it is silent: +// +// - the post keeps being served from the index, after its author deleted it; +// - the community's repo keeps a signed acceptance record citing a record +// nobody can fetch — the CAR that its whole portability argument rests on, +// permanently asserting content the author withdrew; +// - the admission row still says `accepted`, so getStatus tells the author +// their deleted post is live in the community. +// +// This test runs the delete with NO consumer anywhere near it. Everything it +// asserts is the AppView's own account of a post it hosts both halves of, and +// every one of them is reachable synchronously — the same credentials that +// wrote the acceptance a moment ago can withdraw it. +// +// The firehose copy of this deletion is NOT made redundant by any of this: it +// still arrives on every other AppView, and it still arrives here on redelivery. +// Both paths are idempotent by construction (DeleteAcceptance reports "nothing +// to withdraw" as a skip; the admission CAS refuses a rev that does not win), so +// doing the work twice is a no-op — while doing it zero times is the bug above. +func TestService_DeleteWithdrawsTheAcceptanceWithoutWaitingForTheFirehose(t *testing.T) { + t.Parallel() + + f := newPostFixture(t) + ctx := context.Background() + + // ── GIVEN ──────────────────────────────────────────────────────────────── + // An accepted post in a community this AppView hosts. The create fast path + // does all of this for real: a postv2 in the author's repo, an acceptance + // record in the community's, and an `accepted` admission row. + const title = "a post its author will withdraw" + const content = "the body of a post that will be deleted" + created := f.createPost(t, f.author.DID, title, content) + require.Equalf(t, posts.PostStatusAccepted, created.Status, + "fixture: this test is about withdrawing an acceptance, so there has to be one — the fast path "+ + "must have settled %s synchronously", created.URI) + + // And the firehose consumer indexed the CREATE, back when it was running. + // That is why the post is being served at all, and it is what makes the + // missing delete-side compensation observable: the row is here, the events + // that would retract it are not. + f.indexTheCreate(t, created, title, content) + + acceptanceRkey := posts.SubjectRkey(created.URI) + community := f.communityAccount(t) + + before := f.admissionOf(t, created.URI) + require.Equal(t, posts.AdmissionStatusAccepted, before.Status, "fixture: the row must be accepted") + require.NotNil(t, before.AcceptanceURI, "fixture: an accepted row names its acceptance record") + require.NotNil(t, before.LastCommunityEvent, + "fixture: the acceptance write stamped a §5.2 watermark, which the withdrawal has to advance past") + require.Equalf(t, []string{acceptanceRkey}, listRecordKeys(t, community, posts.AcceptanceCollection), + "fixture: exactly one acceptance must stand in %s before the author deletes the post", f.community.DID) + require.NotNilf(t, f.getPost(t, created.URI).Post, + "fixture: post.get must SERVE this post before the delete, or (d) below proves nothing — a URI "+ + "that was never served would answer notFound whether or not the delete compensated") + + communityHeadBefore := repoHead(t, community) + + // ── WHEN ───────────────────────────────────────────────────────────────── + // The author deletes their post. No consumer is running: this call is the + // only thing that happens. + require.NoError(t, f.service.DeletePost(ctx, sessionFor(t, f.author, f.pds.URL()), + posts.DeletePostRequest{URI: created.URI})) + + // The half that already works: the author's record is gone from their repo. + // Asserted so that a failure below cannot be read as "the delete did not + // happen" — it did, and what follows is what it left behind. + require.Emptyf(t, listRecordKeys(t, f.author, posts.PostV2Collection), + "the postv2 record is still standing in %s — the delete itself failed, so nothing after this "+ + "assertion is meaningful", f.author.DID) + + // ── THEN ───────────────────────────────────────────────────────────────── + // Four facts, each its own subtest so that ONE run names every half of the + // compensation that is missing rather than stopping at the first. + + t.Run("(a) the local index row is soft-deleted", func(t *testing.T) { + row := f.rawIndexedRow(t, created.URI) + require.NotNilf(t, row, "the indexed row of %s vanished entirely; a delete is a SOFT delete — "+ + "the row stays so that comments, votes and the removal path can still resolve the subject", + created.URI) + + assert.NotNilf(t, row.DeletedAt, + "LOCAL HALF MISSING: deleteAuthorPost deleted the record from the author's repo and returned "+ + "without soft-deleting the index row, so this AppView is still serving a post its author "+ + "withdrew. The firehose consumer's tombstoneAuthorPost is the only thing that clears it "+ + "today, and it never runs for an author PDS that is not on a configured jetstream feed") + }) + + t.Run("(b) the community's acceptance is withdrawn from its repo", func(t *testing.T) { + assert.Emptyf(t, listRecordKeys(t, community, posts.AcceptanceCollection), + "REMOTE HALF MISSING: the acceptance at %s still stands in %s, signed by the community and "+ + "pointing at a postv2 record that no longer exists. This AppView holds the community's "+ + "credentials — it wrote that acceptance moments ago — so withdrawing it is a call it can "+ + "make, not something to leave to a firehose event that may never arrive", + acceptanceRkey, f.community.DID) + + // AND NOTHING WAS PUBLISHED IN ITS PLACE. A removal record here would be + // the community declaring, permanently and portably, that it moderated + // this post — a public accusation about an author who simply deleted + // their own words. That is why §5.3 gives the withdrawal its own writer + // instead of spelling it as a removal with a special code. + assert.Emptyf(t, listRecordKeys(t, community, posts.RemovalCollection), + "the compensation published a removal record in %s: the AUTHOR deleted this post, and a "+ + "removal record says the COMMUNITY took it down", f.community.DID) + }) + + t.Run("(c) the admission row is pending again, stamped with the withdrawal's rev", func(t *testing.T) { + after := f.admissionOf(t, created.URI) + + assert.Equalf(t, posts.AdmissionStatusPending, after.Status, + "STAMP MISSING: the admission of %s still reads %q, so getStatus answers the author that "+ + "their deleted post is live in the community. ApplyAcceptanceDelete moves an accepted (or "+ + "pending_reacceptance) row back to pending — NOT to removed, which would record a "+ + "moderation decision nobody made", created.URI, after.Status) + + assert.Nilf(t, after.AcceptanceURI, + "the row still names acceptance %v, which is the guard the firehose sweep itself keys off "+ + "(authorpost.go withdrawAcceptance tests the AcceptanceURI, not the status) — a row that "+ + "keeps it is a row that claims a record standing in the community's repo", + derefOrNil(after.AcceptanceURI)) + assert.Nil(t, after.AcceptedCID, + "the row still pins a CID for an acceptance that no longer exists") + + // THE WATERMARK IS THE WITHDRAWAL'S OWN REV, and it is what makes the + // firehose copy of this same deletion a no-op instead of a second + // decision. Anchored to the community repo's head because the withdrawal + // is the last thing committed there. + communityHeadAfter := repoHead(t, community) + require.NotEqualf(t, communityHeadBefore, communityHeadAfter, + "the community's repo never committed anything: no withdrawal was written, so there is no "+ + "rev for the row to be stamped with (see (b))") + + require.NotNil(t, after.LastCommunityEvent, "the row carries no community watermark at all") + assert.Equalf(t, communityHeadAfter, after.LastCommunityEvent.Rev, + "the row is stamped with rev %q but the withdrawal committed in %q: the stamp must carry the "+ + "rev DeleteAcceptance reported, or the firehose copy of this deletion wins the §5.2 CAS "+ + "and re-applies a decision that has already been made", + after.LastCommunityEvent.Rev, communityHeadAfter) + assert.Greaterf(t, after.LastCommunityEvent.Rev, before.LastCommunityEvent.Rev, + "the watermark did not advance past the acceptance's own rev %q", before.LastCommunityEvent.Rev) + assert.Equalf(t, posts.CommunityOpDelete, after.LastCommunityEvent.OpRank, + "a withdrawal is a DELETE within its commit, and the op rank is what orders it against a put "+ + "sharing the same rev") + }) + + t.Run("(d) post.get answers notFound — not the post, and not a tombstone", func(t *testing.T) { + result := f.getPost(t, created.URI) + + assert.Nilf(t, result.Post, + "STILL SERVED: post.get returned the full view of %s after its author deleted it. This is the "+ + "user-visible shape of the missing compensation — every permalink, feed hydration and "+ + "cold load keeps rendering a withdrawn post", created.URI) + + // A TOMBSTONE IS A DIFFERENT CLAIM. #removedPost tells the reader the + // community took this post down and carries the moderator's code; an + // author deleting their own post is a plain disappearance, and dressing + // it as a removal attributes a takedown to a community that never made + // one. removedMarkers gets this right only because it skips a + // soft-deleted row — which is another way of saying (a) is load-bearing + // here, and that a compensation which stamped the row `removed` instead + // of `pending` would fail this assertion. + assert.Nilf(t, result.Removed, + "post.get answered a #removedPost tombstone for %s: the AUTHOR deleted this post, so it is "+ + "gone, not moderated — a tombstone here advertises a community takedown that never "+ + "happened", created.URI) + + assert.NotNilf(t, result.NotFound, + "post.get must answer #notFoundPost for a post its author deleted; got %#v", result) + }) +} + +// --------------------------------------------------------------------------- +// Fixtures +// --------------------------------------------------------------------------- + +// indexTheCreate is the firehose consumer's half of the CREATE, performed by +// hand: the postv2 commit reached this AppView, was indexed, and the post has +// been served from that row ever since. +// +// It is done directly against the repository rather than by running a consumer +// because this test is about what happens when there is NO consumer. The create +// being indexed and the delete not being indexed is not a contradiction — it is +// the ordinary shape of the bug: the author's PDS was on a feed when they +// posted, and by the time they deleted it was not (a feed reconfiguration, a +// migrated repo, a consumer down for an afternoon). The row is the only reason +// the post is visible at all, which is what makes the missing compensation +// observable from outside. +func (f *postFixture) indexTheCreate(t *testing.T, created *posts.CreatePostResponse, title, content string) { + t.Helper() + ctx := context.Background() + + // The posts row carries a foreign key to users, so the author has to be + // indexed before their post can be. + _, err := postgres.NewUserRepository(f.db).Create(ctx, &users.User{ + DID: f.author.DID, + Handle: f.author.Handle, + PDSURL: f.pds.URL(), + }) + require.NoErrorf(t, err, "indexing the author %s", f.author.DID) + + require.NoErrorf(t, postgres.NewPostRepository(f.db).Create(ctx, &posts.Post{ + URI: created.URI, + CID: created.CID, + RKey: rkeyOf(t, created.URI), + AuthorDID: f.author.DID, + CommunityDID: f.community.DID, + Title: stringPtr(title), + Content: stringPtr(content), + CreatedAt: time.Now().UTC(), + }), "indexing the created post %s", created.URI) +} + +// rawIndexedRow reads the post row the way the removal path does — ungated, so +// a soft-deleted row is still visible to the test that is asserting it IS +// soft-deleted. +func (f *postFixture) rawIndexedRow(t *testing.T, uri string) *posts.Post { + t.Helper() + + row, err := postgres.NewPostRepository(f.db).GetRawIndexedRow(context.Background(), uri) + require.NoErrorf(t, err, "reading the indexed row of %s", uri) + return row +} + +// getPost is post.get for one URI, read anonymously — the shape every permalink +// and cold load goes through. +func (f *postFixture) getPost(t *testing.T, uri string) *posts.PostResult { + t.Helper() + + results, err := f.service.GetPosts(context.Background(), posts.GetPostsRequest{URIs: []string{uri}}) + require.NoErrorf(t, err, "post.get for %s", uri) + require.Lenf(t, results, 1, "post.get must answer one result per requested URI") + return results[0] +} + +func stringPtr(s string) *string { return &s } + +// derefOrNil renders an optional string for a failure message without panicking +// on the nil the assertion is hoping for. +func derefOrNil(s *string) any { + if s == nil { + return nil + } + return *s +} -- 2.51.2 From 08806c50488f27ec22205aa7aeaf47341db12aa8 Mon Sep 17 00:00:00 2001 From: Bretton Date: Sun, 9 Aug 2026 20:47:37 -0700 Subject: [PATCH 2/6] feat(posts): deleteAuthorPost compensates directly instead of waiting for the firehose MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An author deleting their own postv2 now, in the same call: soft-deletes the index row, withdraws the community's acceptance from every community that admitted the post (forks included), and stamps each admission row back to pending at the withdrawal's rev. Mirrors the create path's settleSubmission and the consumer's tombstoneAuthorPost/withdrawAcceptance — the delete path was the one half that never got it, so a post whose author's PDS is off the firehose stayed served, its acceptance dangling and getStatus still 'accepted'. Departs from the consumer in one way on purpose: a compensation failure is SURFACED, not swallowed, because the client is the retry loop and every step is idempotent. ErrCommunityNotHosted is the sole graceful skip. The ErrNotFound retry branch now runs the full compensation too, so a crash between the repo delete and the compensation converges on retry. Compensation is default-on: the withdrawer is derived from the mandatory community service, so no wiring omission can silently restore the bug. The new AcceptanceWithdrawer is a one-method interface — the service must never write a verdict. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01QDvRJ45k6E5KrBARDHUtiM --- cmd/server/wiring.go | 5 + internal/core/posts/postv2.go | 33 ++++++ internal/core/posts/service.go | 179 ++++++++++++++++++++++++++++++++- 3 files changed, 216 insertions(+), 1 deletion(-) diff --git a/cmd/server/wiring.go b/cmd/server/wiring.go index c671fbc..25fda02 100644 --- a/cmd/server/wiring.go +++ b/cmd/server/wiring.go @@ -389,6 +389,11 @@ func (a *application) buildServices(ctx context.Context) error { posts.WithAuthorRepoFactory( posts.NewAuthorRepoFactory(a.oauthClient.ClientApp, aggregators.DefaultSessionID)), posts.WithSyncAcceptance(a.admissionRepo, acceptanceEngine), + // The SAME writer the engine accepts through, so both ends of an + // acceptance's life — the write the fast path makes and the withdrawal + // the author's delete makes (§5.3) — share one writer's clock and swap + // retry budget. buildAcceptanceEngine above is what populated it. + posts.WithAcceptanceWithdrawal(a.communityWriter), posts.WithAdmissionPolicy(posts.AdmissionPolicy{ Ledger: postgresRepo.NewSubmissionLedger(a.db), Bans: a.communityService, diff --git a/internal/core/posts/postv2.go b/internal/core/posts/postv2.go index 0300a83..a187121 100644 --- a/internal/core/posts/postv2.go +++ b/internal/core/posts/postv2.go @@ -271,3 +271,36 @@ func WithSyncAcceptance(admissions AdmissionRepository, acceptor SubmissionAccep s.acceptor = acceptor } } + +// AcceptanceWithdrawer withdraws a community's acceptance of a post whose +// AUTHOR has deleted it — the delete path's half of §5.3. +// +// Narrowed to the one method, exactly as the firehose consumer's +// AcceptanceDeleter is and for the same reason: the post service must never +// write an acceptance, a removal or a repin. Those are the ENGINE's verdicts, +// reached against a community's own policy, and a service holding the full +// CommunityRecordWriter is one edit away from making one. Satisfied by +// CommunityRecordWriter. +type AcceptanceWithdrawer interface { + // DeleteAcceptance removes the community's acceptance record and writes + // nothing in its place. It reports "nothing stood" as a skip rather than an + // error, which is what makes running it twice — here and again when the + // firehose copy of the same deletion arrives — a no-op. + DeleteAcceptance(ctx context.Context, cmd CommunityAcceptanceDeleteCommand) (CommunityWriteResult, error) +} + +// WithAcceptanceWithdrawal replaces the writer the delete path withdraws +// acceptances through. +// +// It is deliberately NOT folded into WithSyncAcceptance, whose "both or +// neither" pairing asserts a different invariant: a row to settle and an engine +// to settle it. Production passes the SAME CommunityRecordWriter the acceptance +// engine writes through, so both ends of a subject's life share one writer's +// clock and retry budget; a test passes one that fails on demand. +// +// Omitting it does not disable the compensation — see NewPostService, which +// derives the production writer from the community service when no option +// supplies one. +func WithAcceptanceWithdrawal(withdrawer AcceptanceWithdrawer) PostServiceOption { + return func(s *postService) { s.acceptanceWithdrawal = withdrawer } +} diff --git a/internal/core/posts/service.go b/internal/core/posts/service.go index 3e5127f..473b4df 100644 --- a/internal/core/posts/service.go +++ b/internal/core/posts/service.go @@ -41,6 +41,11 @@ type postService struct { authorRepos AuthorRepoFactory admissions AdmissionRepository acceptor SubmissionAcceptor + + // acceptanceWithdrawal is the delete path's mirror of acceptor: the writer + // that retracts, from a hosted community's repo, the acceptance the fast + // path put there. See compensateAuthorDelete. + acceptanceWithdrawal AcceptanceWithdrawer } // PostServiceOption configures optional postService dependencies. Options keep the @@ -79,6 +84,22 @@ func NewPostService( for _, opt := range opts { opt(s) } + + // THE DELETE COMPENSATION IS DEFAULT-ON. There is no wiring in which an + // AppView should hold a community's credentials and still decline to + // withdraw its acceptance of a post the author has deleted (§5.3), so + // leaving it to an option would mean one omitted line silently restores the + // bug: a signed acceptance left citing a record nobody can fetch, with + // nothing anywhere to notice. + // + // The writer is a pure derivation of communityService, which is mandatory + // here, and the factory's hosting test is CREDENTIAL PRESENCE — so for every + // community this instance does not host it answers ErrCommunityNotHosted and + // the delete path skips, which is the common case. + if s.acceptanceWithdrawal == nil && communityService != nil { + s.acceptanceWithdrawal = NewCommunityRecordWriter(NewCommunityRepoFactory(communityService), time.Now) + } + // The admission policy is mandatory, and a missing or partial one panics // here rather than defaulting to no-ops: a post service whose ban check and // quota silently do not exist is a wiring bug, not a configuration. @@ -1754,13 +1775,169 @@ func (s *postService) deleteAuthorPost(ctx context.Context, session *oauth.Clien if errors.Is(err, pds.ErrNotFound) { // Already deleted or never existed — the retried delete after a lost // response succeeds. + // + // AND IT STILL COMPENSATES. This is the shape a RETRY arrives in: + // the first attempt removed the record and then failed part way + // through the compensation below, so the second finds nothing left + // to delete. Returning here would make the retry the client is + // offered the one path that can never finish the work. log.Printf("[POST-DELETE] Post not found in the author's repo (already deleted?): %s", uri) - return nil + return s.compensateAuthorDelete(ctx, uri) } return fmt.Errorf("failed to delete post from PDS: %w", err) } log.Printf("[POST-DELETE] Successfully deleted post: uri=%s, author=%s", uri, userDID) + + // The AppView's own half of the deletion, performed now rather than left to + // a firehose event that may never arrive. See compensateAuthorDelete. + return s.compensateAuthorDelete(ctx, uri) +} + +// compensateAuthorDelete retracts everything the create wrote about a post +// whose author has just deleted it (§5.3), without waiting for the firehose +// copy of the deletion to come back. +// +// IT IS THE MIRROR OF settleSubmission. A create writes the postv2 into the +// author's repo, seeds the admission row and — for a community this AppView +// hosts — writes the acceptance into the community's repo and stamps the row, +// all before it answers the author. A delete undoes exactly those, and it is +// reachable with exactly the same credentials: the acceptance being withdrawn +// here is one this instance signed moments or months ago. +// +// THE FIREHOSE IS NOT A GUARANTEE, which is why this exists at all. The +// consumer's tombstoneAuthorPost only runs for an author whose PDS is on a +// configured jetstream feed, and a feed reconfiguration, a migrated repo or a +// consumer down for an afternoon all end the same silent way: this AppView +// keeps serving a post its author withdrew, the community's repo keeps a +// signed acceptance citing a record nobody can fetch, and getStatus keeps +// telling the author their deleted post is live in the community. +// +// The firehose copy is NOT made redundant by any of this — it still reaches +// every other AppView, and it still reaches this one on redelivery. Both paths +// are idempotent by construction (a withdrawal of nothing is a skip, and the +// §5.2 CAS refuses a rev that does not win), so doing the work twice is a +// no-op while doing it zero times is the failure above. +// +// A FAILURE IS RETURNED, and that is the one place this deliberately departs +// from the consumer, which logs and swallows. The consumer must: an error +// there dead-letters an event whose local half already committed, and the rev +// gate would refuse the redrive, so the retry could never reach the sweep +// again. Here the CLIENT is the retry loop — the author's record is already +// gone, every step below is idempotent, and surfacing the failure is what gets +// the remaining work done. Reporting success over a half-finished compensation +// reproduces the exact silence this path exists to end. +func (s *postService) compensateAuthorDelete(ctx context.Context, uri string) error { + // THE LOCAL TRUTH LANDS FIRST, in the consumer's order and for its reason: + // the author asked for their post to be gone, and a community PDS that + // cannot be reached must not keep this AppView serving it. + // + // The consumer gates its tombstone on the commit rev; this path needs no + // gate. SoftDelete is monotonic — NULL → NOW() under `WHERE deleted_at IS + // NULL` — and the create it could race is protected by the indexer's own + // ON CONFLICT DO NOTHING, so applying it here and again on the firehose + // copy leaves the same row either way. + if err := s.repo.SoftDelete(ctx, uri); err != nil { + return fmt.Errorf("soft-deleting the indexed row of %s: %w", uri, err) + } + + // Without the admissions store there is nothing that says WHICH community + // accepted this post — a deletion carries no record to read it from — and + // without a withdrawer there are no credentials to withdraw it with. That + // combination is the pre-flip wiring, where the firehose owns every + // community-repo write, and the tombstone above is all this path can do. + if s.admissions == nil || s.acceptanceWithdrawal == nil { + return nil + } + + admissionsByURI, err := s.admissions.GetByPostURIs(ctx, []string{uri}) + if err != nil { + // SURFACED, never swallowed. A read that failed says nothing about + // whether an acceptance stands, and treating "I could not look" as + // "there is nothing there" would reintroduce the silent version of this + // bug through the back door. + return fmt.Errorf("resolving the admissions of %s: %w", uri, err) + } + + // EVERY community that admitted this post, not just the one the record + // names. A post can also carry a decision from a community that FORKED it + // (the case removedMarkers reads the same map for), and every acceptance of + // it now cites a record that no longer exists. The ones this AppView does + // not host answer ErrCommunityNotHosted and cost a lookup. + for _, admission := range admissionsByURI[uri] { + if err := s.withdrawAcceptanceOf(ctx, admission); err != nil { + return err + } + } + return nil +} + +// withdrawAcceptanceOf removes one community's acceptance record and stamps the +// admission row that pointed at it. +// +// The two are one unit: the record is what the community publishes, the row is +// what this AppView answers getStatus from, and a withdrawal that did only the +// first would leave the author told their deleted post is still live. +func (s *postService) withdrawAcceptanceOf(ctx context.Context, admission *Admission) error { + // THE GUARD IS THE ACCEPTANCE URI, NOT THE STATUS — the same test the + // firehose sweep makes. `accepted` and `pending_reacceptance` both have a + // live acceptance record standing in the community's repo (the second + // merely pins content the author has since edited), and both must be + // withdrawn when the subject itself is deleted. A row holding no URI has + // nothing standing to withdraw. + if admission == nil || admission.AcceptanceURI == nil { + return nil + } + + withdrawn, err := s.acceptanceWithdrawal.DeleteAcceptance(ctx, CommunityAcceptanceDeleteCommand{ + CommunityDID: admission.CommunityDID, + PostURI: admission.PostURI, + }) + switch { + case errors.Is(err, ErrCommunityNotHosted): + // Not this instance's community, and no retry changes that: the + // acceptance lives in the community's repo and needs its keys. The + // community's own AppView performs this cleanup when the deletion + // reaches it over the firehose, so the author's delete succeeds here + // rather than failing forever on work this instance cannot do. + log.Printf("[POST-DELETE] %s is hosted elsewhere; its acceptance of %s is not ours to withdraw", + admission.CommunityDID, admission.PostURI) + return nil + case err != nil: + return fmt.Errorf("withdrawing the acceptance of %s in %s: %w", + admission.PostURI, admission.CommunityDID, err) + } + + // A SKIPPED WITHDRAWAL STILL STAMPS, on the catch-up rev the writer reports + // (the repo HEAD it read before its pre-read). Nothing stood, so the row's + // claim to an acceptance is precisely what needs clearing — this is how a + // row stranded by an earlier pass that committed the delete and then failed + // this stamp is caught up. The engine's accept path stamps a skip for the + // same reason. + if withdrawn.Rev == "" { + // Defensive only: the writer contract reports a rev on every path, + // committed or skipped. An empty one must not be stamped — the + // repository refuses it as a fabricated watermark, correctly. + return nil + } + + if _, err := s.admissions.ApplyAcceptanceDelete(ctx, CommunityDeleteCommand{ + CommunityDID: admission.CommunityDID, + PostURI: admission.PostURI, + // The rev the withdrawal COMMITTED in — the §5.2 watermark that makes + // the firehose copy of this same deletion a no-op instead of a second + // decision. OpRank is left zero deliberately: the repository derives + // the rank from the operation, because the rank IS the operation's kind. + Watermark: CommunityWatermark{Rev: withdrawn.Rev}, + }); err != nil { + // The record is OUT of the community's repo and the row still names it. + // The firehose copy would reconcile it eventually, but "eventually" is + // the assumption this whole path refuses to make. + return fmt.Errorf("stamping the withdrawal of %s in %s: %w", + admission.PostURI, admission.CommunityDID, err) + } + + log.Printf("[POST-DELETE] Withdrew the acceptance of %s in %s", admission.PostURI, admission.CommunityDID) return nil } -- 2.51.2 From 9ca951acff4d55f0868e775e08bcdfaf76929c6d Mon Sep 17 00:00:00 2001 From: Bretton Date: Sun, 9 Aug 2026 20:58:20 -0700 Subject: [PATCH 3/6] test(posts): T0 battery pinning every branch of the delete compensation Eleven unit tests over deleteAuthorPost's compensation, each proven to bite by mutation: the ErrNotFound retry runs the full compensation (the original bug), the guard is the AcceptanceURI not the status (pending_reacceptance still withdraws), ErrCommunityNotHosted is the sole graceful skip, an admission-read failure and each of soft-delete / withdrawal / stamp failures all surface, the stamp carries the withdrawal's own rev, and the legacy community.post path never touches the withdrawer. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01QDvRJ45k6E5KrBARDHUtiM --- .../service_delete_compensation_unit_test.go | 486 ++++++++++++++++++ 1 file changed, 486 insertions(+) create mode 100644 internal/core/posts/service_delete_compensation_unit_test.go diff --git a/internal/core/posts/service_delete_compensation_unit_test.go b/internal/core/posts/service_delete_compensation_unit_test.go new file mode 100644 index 0000000..8037fa5 --- /dev/null +++ b/internal/core/posts/service_delete_compensation_unit_test.go @@ -0,0 +1,486 @@ +package posts + +import ( + "context" + "errors" + "testing" + + "Coves/internal/atproto/pds" + "Coves/internal/core/communities" + + "github.com/bluesky-social/indigo/atproto/auth/oauth" + "github.com/bluesky-social/indigo/atproto/syntax" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// The BRANCHES of the delete-path compensation (§5.3), pinned one at a time. +// +// service_delete_compensation_test.go proves the whole thing end to end against +// a real PDS and a real admissions table, and that is the test that says the +// feature works. It can only walk the happy path, though: it cannot make the +// community's host answer ErrCommunityNotHosted, cannot make the stamp fail +// after the record has already left the repo, and cannot arrange for the +// author's record to be gone before the delete arrives. Every one of those is +// a branch that decides whether an author's deletion finishes or silently +// half-finishes, and each is one `if` away from being wrong. +// +// So they are pinned here, in-process, with fakes — the tier that can construct +// a failure at an exact step (docs/TEST_ARCHITECTURE.md: behavioural breadth +// belongs at T0, not T2). +// +// WHAT THESE ASSERT IS AN ORDER AND A SET OF CALLS, deliberately. There is no +// database and no repo here, so "the row is soft-deleted" is not observable; +// what IS observable — and what the production bug was — is whether the service +// ISSUES the commands at all, and with what. Every assertion below is about a +// command that was or was not sent. + +// --------------------------------------------------------------------------- +// Fakes +// +// Each one extends an existing package fake with the single capability this +// file needs: the ability to fail on demand, and to remember what it was asked +// to do. The originals return nil and record nothing, which is exactly right +// for their own tests and useless for these. +// --------------------------------------------------------------------------- + +// recordingWithdrawer is the AcceptanceWithdrawer seam, observed. It is the +// whole point of the fake set: the community-repo write is the step no unit +// test can perform and the one the compensation exists to make. +type recordingWithdrawer struct { + cmds []CommunityAcceptanceDeleteCommand + result CommunityWriteResult + err error +} + +func (w *recordingWithdrawer) DeleteAcceptance(_ context.Context, cmd CommunityAcceptanceDeleteCommand) (CommunityWriteResult, error) { + w.cmds = append(w.cmds, cmd) + return w.result, w.err +} + +// tombstoningRepo is mockRepository with a SoftDelete that remembers and can +// fail. +type tombstoningRepo struct { + mockRepository + + softDeleted []string + softDeleteErr error +} + +func (r *tombstoningRepo) SoftDelete(_ context.Context, uri string) error { + r.softDeleted = append(r.softDeleted, uri) + return r.softDeleteErr +} + +// stampingAdmissions is fakeAdmissions with an ApplyAcceptanceDelete that keeps +// the command — the watermark it carries is the assertion in behaviour 1 — and +// can fail. +type stampingAdmissions struct { + *fakeAdmissions + + stamps []CommunityDeleteCommand + stampErr error +} + +func (a *stampingAdmissions) ApplyAcceptanceDelete(_ context.Context, cmd CommunityDeleteCommand) (AdmissionResult, error) { + a.stamps = append(a.stamps, cmd) + return AdmissionResult{}, a.stampErr +} + +// deletingAuthorRepo is memAuthorRepo with a DeleteRecord that can answer +// pds.ErrNotFound — the shape a retry after a half-finished delete arrives in. +type deletingAuthorRepo struct { + *memAuthorRepo + + deleted []string + deleteErr error +} + +func (r *deletingAuthorRepo) DeleteRecord(_ context.Context, collection, rkey string) error { + r.deleted = append(r.deleted, collection+"/"+rkey) + return r.deleteErr +} + +// absentCommunities answers every community lookup with "not indexed". +// +// The interface is EMBEDDED rather than implemented: communities.Service has +// twenty methods and this file needs one, and an embedded nil interface panics +// on any other — which is itself the assertion that the delete path reaches for +// nothing else. +type absentCommunities struct{ communities.Service } + +func (absentCommunities) GetByDID(_ context.Context, did string) (*communities.Community, error) { + return nil, communities.ErrCommunityNotFound +} + +// --------------------------------------------------------------------------- +// Harness +// --------------------------------------------------------------------------- + +const ( + compensationAuthorDID = "did:plc:deletecompensationauthorxx" + compensationCommunityDID = "did:plc:deletecompensationcommunity" + compensationRkey = "3lzdeletecompensation" + compensationAcceptance = "at://" + compensationCommunityDID + "/social.coves.community.acceptance/aaaa" + + // withdrawalRev is what the withdrawer reports having committed in. It is + // the value behaviour 1 traces all the way through to the admission stamp. + withdrawalRev = "3lzwithdrawalrev0" +) + +// deleteHarness is the post service over the four fakes, plus the author's +// session and the URI of the post they are deleting. +type deleteHarness struct { + service Service + repo *tombstoningRepo + admissions *stampingAdmissions + withdrawer *recordingWithdrawer + authorRepo *deletingAuthorRepo + + uri string + session *oauth.ClientSessionData +} + +// newDeleteHarness builds the DEFAULT world: one accepted admission holding a +// standing acceptance URI, an author repo that deletes cleanly, and a +// withdrawer that withdraws successfully. Every test below changes exactly one +// thing about it, so the thing it changes is the thing it is testing. +func newDeleteHarness(t *testing.T) *deleteHarness { + t.Helper() + + uri := "at://" + compensationAuthorDID + "/" + PostV2Collection + "/" + compensationRkey + acceptance := compensationAcceptance + + h := &deleteHarness{ + uri: uri, + repo: &tombstoningRepo{}, + admissions: &stampingAdmissions{fakeAdmissions: &fakeAdmissions{ + rec: &engineRecorder{}, + byPostURIs: map[string][]*Admission{ + uri: {{ + CommunityDID: compensationCommunityDID, + PostURI: uri, + Status: AdmissionStatusAccepted, + AcceptanceURI: &acceptance, + }}, + }, + }}, + withdrawer: &recordingWithdrawer{result: CommunityWriteResult{Rev: withdrawalRev}}, + authorRepo: &deletingAuthorRepo{memAuthorRepo: newMemAuthorRepo(compensationAuthorDID)}, + session: sessionForDIDString(t, compensationAuthorDID), + } + + h.service = NewPostService( + h.repo, nil, nil, nil, nil, nil, "https://pds.invalid", + WithAuthorRepoFactory(func(context.Context, string, *oauth.ClientSessionData) (AuthorRepo, error) { + return h.authorRepo, nil + }), + // The acceptor half is unused by the delete path; the admissions half is + // what says WHICH community accepted the post being deleted. + WithSyncAcceptance(h.admissions, nil), + WithAcceptanceWithdrawal(h.withdrawer), + WithAdmissionPolicy(NewAllowAllAdmissionPolicyForTests()), + ) + return h +} + +// withCommunityService rebuilds the service over a community service, for the +// legacy-collection route. Everything else is unchanged, so a call that reached +// the new withdrawer would still be recorded. +func (h *deleteHarness) withCommunityService(communityService communities.Service) { + h.service = NewPostService( + h.repo, communityService, nil, nil, nil, nil, "https://pds.invalid", + WithAuthorRepoFactory(func(context.Context, string, *oauth.ClientSessionData) (AuthorRepo, error) { + return h.authorRepo, nil + }), + WithSyncAcceptance(h.admissions, nil), + WithAcceptanceWithdrawal(h.withdrawer), + WithAdmissionPolicy(NewAllowAllAdmissionPolicyForTests()), + ) +} + +// delete runs the deletion under test. +func (h *deleteHarness) delete(uri string) error { + return h.service.DeletePost(context.Background(), h.session, DeletePostRequest{URI: uri}) +} + +func sessionForDIDString(t *testing.T, did string) *oauth.ClientSessionData { + t.Helper() + + parsed, err := syntax.ParseDID(did) + require.NoErrorf(t, err, "the test's own author DID %q is malformed", did) + return &oauth.ClientSessionData{AccountDID: parsed, SessionID: "delete-compensation-unit-test"} +} + +// --------------------------------------------------------------------------- +// 1. The happy path issues both halves +// --------------------------------------------------------------------------- + +func TestDeletePost_WithdrawsTheAcceptanceAndStampsItAtTheWithdrawalsRev(t *testing.T) { + h := newDeleteHarness(t) + + require.NoError(t, h.delete(h.uri)) + + // THE LOCAL HALF. Without it this AppView keeps serving a post its author + // withdrew, whatever the community's repo says. + assert.Equal(t, []string{h.uri}, h.repo.softDeleted, + "the index row was never soft-deleted, so post.get keeps serving the deleted post") + + // THE REMOTE HALF, aimed at the community the ADMISSION names — a deletion + // carries no record to read the community from, so the row is the only + // thing that knows whose acceptance this is. + require.Lenf(t, h.withdrawer.cmds, 1, + "expected exactly one withdrawal for one standing acceptance, got %d", len(h.withdrawer.cmds)) + assert.Equal(t, CommunityAcceptanceDeleteCommand{ + CommunityDID: compensationCommunityDID, + PostURI: h.uri, + }, h.withdrawer.cmds[0], + "the withdrawal must name the community and post the admission row names") + + // AND THE STAMP CARRIES THE WITHDRAWAL'S OWN REV. This is the §5.2 watermark + // that makes the firehose copy of this same deletion a no-op rather than a + // second decision: a stamp at any other rev either loses the CAS (and leaves + // the row claiming an acceptance that is gone) or outranks an event it + // should not. + require.Lenf(t, h.admissions.stamps, 1, + "the admission row was never stamped, so getStatus still answers `accepted` for a deleted post") + assert.Equal(t, CommunityDeleteCommand{ + CommunityDID: compensationCommunityDID, + PostURI: h.uri, + Watermark: CommunityWatermark{Rev: withdrawalRev}, + }, h.admissions.stamps[0], + "the stamp must carry the rev the WITHDRAWAL committed in") +} + +// --------------------------------------------------------------------------- +// 2. The guard is the acceptance URI, not the status +// --------------------------------------------------------------------------- + +func TestDeletePost_WithdrawsNothingWhenNoAcceptanceStands(t *testing.T) { + h := newDeleteHarness(t) + + // A row that was never accepted — pending, no acceptance record anywhere. + // Reaching the community's host for it would be a PDS round trip per delete + // for the majority of posts, since most posts a community sees it never + // accepted. + h.admissions.byPostURIs[h.uri] = []*Admission{{ + CommunityDID: compensationCommunityDID, + PostURI: h.uri, + Status: AdmissionStatusPending, + AcceptanceURI: nil, + }} + + require.NoError(t, h.delete(h.uri)) + + assert.Emptyf(t, h.withdrawer.cmds, + "a row holding no acceptance URI has nothing standing to withdraw; the service asked the "+ + "community's host to delete a record that was never written: %+v", h.withdrawer.cmds) + assert.Empty(t, h.admissions.stamps, + "nothing was withdrawn, so there is no withdrawal rev to stamp — a stamp here would advance the "+ + "§5.2 watermark past a community event that never happened") + + // THE LOCAL HALF STILL RUNS. It is not conditional on any community: the + // author deleted their post, so this AppView stops serving it regardless of + // what any community's repo does or does not hold. + assert.Equal(t, []string{h.uri}, h.repo.softDeleted, + "the local tombstone must not be gated on there being an acceptance to withdraw") +} + +func TestDeletePost_WithdrawsAnAcceptanceThatIsAwaitingReacceptance(t *testing.T) { + h := newDeleteHarness(t) + + // pending_reacceptance means the author EDITED the post after it was + // accepted: the acceptance record is still standing in the community's repo, + // it merely pins content that has since changed. The subject is being + // deleted, so that record has to go — and a guard written against the STATUS + // rather than the URI would leave it dangling, which is the exact bug the + // firehose sweep's own comment calls out (authorpost.go withdrawAcceptance). + acceptance := compensationAcceptance + h.admissions.byPostURIs[h.uri] = []*Admission{{ + CommunityDID: compensationCommunityDID, + PostURI: h.uri, + Status: AdmissionStatusPendingReacceptance, + AcceptanceURI: &acceptance, + }} + + require.NoError(t, h.delete(h.uri)) + + require.Lenf(t, h.withdrawer.cmds, 1, + "a pending_reacceptance row HAS a live acceptance record standing in the community's repo, and "+ + "deleting the subject must withdraw it; the guard is the acceptance URI, not the status") + assert.Equal(t, compensationCommunityDID, h.withdrawer.cmds[0].CommunityDID) + assert.Len(t, h.admissions.stamps, 1, "the withdrawal committed, so the row must be stamped") +} + +// --------------------------------------------------------------------------- +// 3. What the admission lookup answers +// --------------------------------------------------------------------------- + +func TestDeletePost_FailsWhenItCannotTellWhichCommunityAcceptedThePost(t *testing.T) { + h := newDeleteHarness(t) + h.admissions.byPostURIsErr = errors.New("the admissions table is unreachable") + + err := h.delete(h.uri) + + require.Errorf(t, err, "a lookup that FAILED says nothing about whether an acceptance stands, and "+ + "treating `I could not look` as `there is nothing there` reintroduces the silent version of this "+ + "bug: the author is told their post is gone while the community's repo still attests to it") + assert.Empty(t, h.withdrawer.cmds, + "nothing is known about which community to ask, so nothing may be withdrawn") +} + +func TestDeletePost_SucceedsForAPostNoCommunityEverDecidedAbout(t *testing.T) { + h := newDeleteHarness(t) + + // No admission row at all. This is an ordinary state, not a corrupt one: a + // post written while the community was unreachable, or one whose row has + // been reaped. There is nothing to withdraw and nothing to stamp, and the + // author's deletion must still succeed. + delete(h.admissions.byPostURIs, h.uri) + + require.NoError(t, h.delete(h.uri), + "a post with no admission row has no acceptance anywhere; its deletion has nothing to compensate "+ + "beyond the local tombstone, and must not fail") + assert.Empty(t, h.withdrawer.cmds) + assert.Empty(t, h.admissions.stamps) + assert.Equal(t, []string{h.uri}, h.repo.softDeleted, + "the local tombstone runs for every deleted post, admission row or not") +} + +// --------------------------------------------------------------------------- +// 4. A community hosted elsewhere +// --------------------------------------------------------------------------- + +func TestDeletePost_SucceedsWhenTheAcceptanceBelongsToACommunityHostedElsewhere(t *testing.T) { + h := newDeleteHarness(t) + h.withdrawer.err = ErrCommunityNotHosted + + require.NoError(t, h.delete(h.uri), + "the acceptance lives in the community's repo and needs its keys, which this instance does not "+ + "hold — and no retry ever will. The community's own AppView performs this cleanup when the "+ + "deletion reaches it over the firehose, so failing the author's delete here would fail it "+ + "forever on work this instance cannot do") + + assert.Equal(t, []string{h.uri}, h.repo.softDeleted, + "not hosting the community is no reason to keep serving the post locally") + assert.Len(t, h.withdrawer.cmds, 1, "the skip is discovered BY asking; the attempt still happens") + + // NOT STAMPED. The row belongs to a community whose repo this instance + // cannot read or write, so it has no rev to stamp and no standing to claim + // the acceptance was withdrawn — that community's own AppView will say so. + assert.Empty(t, h.admissions.stamps, + "a withdrawal that did not happen must not be recorded as one; stamping here would advance the "+ + "§5.2 watermark past a community event that was never written, and the real deletion arriving "+ + "later over the firehose would lose the CAS") +} + +// --------------------------------------------------------------------------- +// 5. The retry, arriving after the record is already gone +// --------------------------------------------------------------------------- + +func TestDeletePost_CompensatesOnRetryWhenTheRecordIsAlreadyGone(t *testing.T) { + h := newDeleteHarness(t) + + // THE SHAPE A RETRY ARRIVES IN. The first attempt removed the record from + // the author's repo and then failed part way through the compensation — a + // crash, a lost connection to the community's host, a restart. The client + // retries, and the PDS answers "no such record". + // + // Before the fix this branch returned a bare nil, which made the retry the + // client is explicitly offered the ONE path that could never finish the + // work: the post stays served and the acceptance stays standing, no matter + // how many times the author presses delete. + h.authorRepo.deleteErr = pds.ErrNotFound + + require.NoError(t, h.delete(h.uri), + "a record that is already gone is the outcome the caller asked for, so the delete succeeds") + + assert.Equal(t, []string{h.uri}, h.repo.softDeleted, + "the retry must still tombstone the index row — the first attempt may be exactly why it is "+ + "still standing") + require.Lenf(t, h.withdrawer.cmds, 1, + "the retry must still withdraw the acceptance: the record is gone from the author's repo, and "+ + "the community's attestation to it is precisely the thing left to clean up") + assert.Len(t, h.admissions.stamps, 1, + "the retry must still stamp the row back to pending") +} + +// --------------------------------------------------------------------------- +// 6. Every failure after the record is gone is surfaced +// +// The consumer logs and swallows these, and must: an error there dead-letters +// an event whose local half already committed, and the rev gate refuses the +// redrive, so the retry could never reach the sweep again. HERE THE CLIENT IS +// THE RETRY LOOP — every step is idempotent, and reporting success over a +// half-finished compensation is the exact silence this path exists to end. +// --------------------------------------------------------------------------- + +func TestDeletePost_SurfacesAFailureToTombstoneTheIndexRow(t *testing.T) { + h := newDeleteHarness(t) + h.repo.softDeleteErr = errors.New("the database is unreachable") + + err := h.delete(h.uri) + + require.Error(t, err, + "the record is out of the author's repo and this AppView is still serving the post: answering "+ + "success would leave the author looking at a post they were told was deleted, with nothing "+ + "anywhere scheduled to notice") + assert.Empty(t, h.withdrawer.cmds, + "the local truth lands FIRST, in the consumer's order — a compensation that raced ahead to the "+ + "remote half would withdraw the community's acceptance of a post this AppView still serves") +} + +func TestDeletePost_SurfacesAFailureToWithdrawTheAcceptance(t *testing.T) { + h := newDeleteHarness(t) + h.withdrawer.err = errors.New("the community's PDS is unreachable") + + err := h.delete(h.uri) + + require.Error(t, err, + "the community's repo still holds a signed acceptance citing a record nobody can fetch, and the "+ + "client is the only retry loop this path has") + assert.Empty(t, h.admissions.stamps, + "a withdrawal that failed must not be stamped as done — the row's claim to an acceptance is "+ + "TRUE while the record still stands") +} + +func TestDeletePost_SurfacesAFailureToStampTheWithdrawal(t *testing.T) { + h := newDeleteHarness(t) + h.admissions.stampErr = errors.New("the admissions table is unreachable") + + err := h.delete(h.uri) + + require.Error(t, err, + "the acceptance record is OUT of the community's repo and the admission row still names it: "+ + "getStatus answers `accepted` over a post that no longer exists anywhere. The firehose copy "+ + "would reconcile it eventually, and `eventually` is the assumption this path refuses to make") + assert.Len(t, h.withdrawer.cmds, 1, "the withdrawal itself succeeded; it is the stamp that failed") +} + +// --------------------------------------------------------------------------- +// 7. The deprecated collection is not this path's business +// --------------------------------------------------------------------------- + +func TestDeletePost_DoesNotWithdrawAcceptancesForALegacyCommunityRepoPost(t *testing.T) { + h := newDeleteHarness(t) + h.withCommunityService(absentCommunities{}) + + // A pre-flip social.coves.community.post lives in the COMMUNITY's repo and + // has no acceptance record at all — the acceptance/admission machinery came + // in with the ownership flip. Routing one of these into the compensation + // would send a withdrawal for a record that was never written, against a + // subject rkey derived from a URI the acceptance scheme does not cover. + legacyURI := "at://" + compensationCommunityDID + "/" + LegacyPostCollection + "/" + compensationRkey + + err := h.delete(legacyURI) + + assert.ErrorIsf(t, err, ErrCommunityNotFound, + "the legacy delete must route to deleteCommunityPost, which resolves the community first; got: %v", err) + assert.Emptyf(t, h.withdrawer.cmds, + "a legacy community-repo post has no acceptance to withdraw, and the author-post compensation "+ + "must never see it: %+v", h.withdrawer.cmds) + assert.Empty(t, h.admissions.stamps, + "nothing about a legacy post's deletion stamps an admission row") + assert.Empty(t, h.repo.softDeleted, + "the legacy path's own tombstone is the firehose consumer's, unchanged by this work") +} -- 2.51.2 From 1fef52dcc2b720e1844387d841176f125bf87324 Mon Sep 17 00:00:00 2001 From: Bretton Date: Sun, 9 Aug 2026 21:21:22 -0700 Subject: [PATCH 4/6] docs(posts): correct two review-flagged comment inaccuracies MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The consumer-vs-service error-disposition comment claimed the rev gate stops a redrive from reaching the sweep; in fact the consumer reconsiders the sweep on every redelivery whose row is already tombstoned — so its swallow is a genuine choice, which is what makes the service's surface-instead the real departure. The wiring comment claimed the shared writer means a shared clock/retry budget; the writer is stateless per call, so it is shared configuration, not budget. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01QDvRJ45k6E5KrBARDHUtiM --- cmd/server/wiring.go | 6 ++++-- internal/core/posts/service.go | 18 +++++++++++------- 2 files changed, 15 insertions(+), 9 deletions(-) diff --git a/cmd/server/wiring.go b/cmd/server/wiring.go index 25fda02..a9eb429 100644 --- a/cmd/server/wiring.go +++ b/cmd/server/wiring.go @@ -391,8 +391,10 @@ func (a *application) buildServices(ctx context.Context) error { posts.WithSyncAcceptance(a.admissionRepo, acceptanceEngine), // The SAME writer the engine accepts through, so both ends of an // acceptance's life — the write the fast path makes and the withdrawal - // the author's delete makes (§5.3) — share one writer's clock and swap - // retry budget. buildAcceptanceEngine above is what populated it. + // the author's delete makes (§5.3) — go through one configured writer + // (same repo factory and clock func). The writer is stateless per call, + // so this is shared configuration, not shared runtime budget. + // buildAcceptanceEngine above is what populated it. posts.WithAcceptanceWithdrawal(a.communityWriter), posts.WithAdmissionPolicy(posts.AdmissionPolicy{ Ledger: postgresRepo.NewSubmissionLedger(a.db), diff --git a/internal/core/posts/service.go b/internal/core/posts/service.go index 473b4df..c6562f3 100644 --- a/internal/core/posts/service.go +++ b/internal/core/posts/service.go @@ -1820,13 +1820,17 @@ func (s *postService) deleteAuthorPost(ctx context.Context, session *oauth.Clien // no-op while doing it zero times is the failure above. // // A FAILURE IS RETURNED, and that is the one place this deliberately departs -// from the consumer, which logs and swallows. The consumer must: an error -// there dead-letters an event whose local half already committed, and the rev -// gate would refuse the redrive, so the retry could never reach the sweep -// again. Here the CLIENT is the retry loop — the author's record is already -// gone, every step below is idempotent, and surfacing the failure is what gets -// the remaining work done. Reporting success over a half-finished compensation -// reproduces the exact silence this path exists to end. +// from the consumer, which logs and swallows. The consumer can afford to: its +// sweep is reconsidered on every redelivery whose row is already tombstoned +// (authorpost.go withdrawAcceptance is gated on the POST's state, not the +// event's), so a transient withdrawal failure there gets another firehose +// attempt for free, and returning an error would only dead-letter an event +// whose local half already committed. Here the firehose is exactly the thing +// that may never arrive — the CLIENT is the only retry loop, the author's +// record is already gone, and every step below is idempotent — so surfacing the +// failure is what gets the remaining work done. Reporting success over a +// half-finished compensation reproduces the exact silence this path exists to +// end. func (s *postService) compensateAuthorDelete(ctx context.Context, uri string) error { // THE LOCAL TRUTH LANDS FIRST, in the consumer's order and for its reason: // the author asked for their post to be gone, and a community PDS that -- 2.51.2 From dcb2280c37d6de2aa204bd06db5cc4ce2176b0de Mon Sep 17 00:00:00 2001 From: Bretton Date: Sun, 9 Aug 2026 21:24:33 -0700 Subject: [PATCH 5/6] =?UTF-8?q?test(posts):=20RED=20=E2=80=94=20three=20re?= =?UTF-8?q?view=20findings=20on=20the=20delete=20compensation?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Failing tests for: the multi-community loop must not abort on the first failing community (one sick PDS starves the others' cleanup); a community credential failure must not surface as the author's own session dying (the XRPC mapper would answer 401 over a community-side outage); and a COMMITTED withdrawal reporting no rev must surface rather than silently succeed with the row unstamped. Plus two regression guards (a hosted-elsewhere first community still continues the loop; a genuinely-skipped empty-rev withdrawal stays a graceful no-op). Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01QDvRJ45k6E5KrBARDHUtiM --- .../service_delete_compensation_unit_test.go | 271 ++++++++++++++++++ 1 file changed, 271 insertions(+) diff --git a/internal/core/posts/service_delete_compensation_unit_test.go b/internal/core/posts/service_delete_compensation_unit_test.go index 8037fa5..b4131f2 100644 --- a/internal/core/posts/service_delete_compensation_unit_test.go +++ b/internal/core/posts/service_delete_compensation_unit_test.go @@ -3,6 +3,7 @@ package posts import ( "context" "errors" + "fmt" "testing" "Coves/internal/atproto/pds" @@ -44,20 +45,56 @@ import ( // for their own tests and useless for these. // --------------------------------------------------------------------------- +// withdrawalOutcome is what one community's host answers. +type withdrawalOutcome struct { + result CommunityWriteResult + err error +} + // recordingWithdrawer is the AcceptanceWithdrawer seam, observed. It is the // whole point of the fake set: the community-repo write is the step no unit // test can perform and the one the compensation exists to make. +// +// It answers PER COMMUNITY when byCommunity holds an entry, and falls back to +// the flat result/err otherwise. The per-community layer exists because the +// interesting failures are asymmetric: one community's host being down while +// another's is healthy is precisely the case a loop that aborts on the first +// error gets wrong, and a single shared answer cannot express it. type recordingWithdrawer struct { cmds []CommunityAcceptanceDeleteCommand result CommunityWriteResult err error + + byCommunity map[string]withdrawalOutcome } func (w *recordingWithdrawer) DeleteAcceptance(_ context.Context, cmd CommunityAcceptanceDeleteCommand) (CommunityWriteResult, error) { w.cmds = append(w.cmds, cmd) + if outcome, ok := w.byCommunity[cmd.CommunityDID]; ok { + return outcome.result, outcome.err + } return w.result, w.err } +// answer scripts one community's host. +func (w *recordingWithdrawer) answer(communityDID string, result CommunityWriteResult, err error) { + if w.byCommunity == nil { + w.byCommunity = map[string]withdrawalOutcome{} + } + w.byCommunity[communityDID] = withdrawalOutcome{result: result, err: err} +} + +// callsFor counts the withdrawals aimed at one community. +func (w *recordingWithdrawer) callsFor(communityDID string) int { + n := 0 + for _, cmd := range w.cmds { + if cmd.CommunityDID == communityDID { + n++ + } + } + return n +} + // tombstoningRepo is mockRepository with a SoftDelete that remembers and can // fail. type tombstoningRepo struct { @@ -87,6 +124,17 @@ func (a *stampingAdmissions) ApplyAcceptanceDelete(_ context.Context, cmd Commun return AdmissionResult{}, a.stampErr } +// stampsFor returns the stamps aimed at one community. +func (a *stampingAdmissions) stampsFor(communityDID string) []CommunityDeleteCommand { + var out []CommunityDeleteCommand + for _, cmd := range a.stamps { + if cmd.CommunityDID == communityDID { + out = append(out, cmd) + } + } + return out +} + // deletingAuthorRepo is memAuthorRepo with a DeleteRecord that can answer // pds.ErrNotFound — the shape a retry after a half-finished delete arrives in. type deletingAuthorRepo struct { @@ -204,6 +252,28 @@ func (h *deleteHarness) delete(uri string) error { return h.service.DeletePost(context.Background(), h.session, DeletePostRequest{URI: uri}) } +// acceptedIn replaces the post's admissions with one accepted row per +// community, IN THE ORDER GIVEN — the order the compensation will walk them, so +// a test can put the sick community first and ask what happened to the healthy +// one behind it. +// +// A post carries more than one admission whenever a community FORKED it: the +// fork's acceptance is a real record in a real repo, standing on its own, and +// every one of them now cites a postv2 that no longer exists. +func (h *deleteHarness) acceptedIn(communityDIDs ...string) { + rows := make([]*Admission, 0, len(communityDIDs)) + for _, did := range communityDIDs { + acceptance := "at://" + did + "/" + AcceptanceCollection + "/" + SubjectRkey(h.uri) + rows = append(rows, &Admission{ + CommunityDID: did, + PostURI: h.uri, + Status: AdmissionStatusAccepted, + AcceptanceURI: &acceptance, + }) + } + h.admissions.byPostURIs[h.uri] = rows +} + func sessionForDIDString(t *testing.T, did string) *oauth.ClientSessionData { t.Helper() @@ -484,3 +554,204 @@ func TestDeletePost_DoesNotWithdrawAcceptancesForALegacyCommunityRepoPost(t *tes assert.Empty(t, h.repo.softDeleted, "the legacy path's own tombstone is the firehose consumer's, unchanged by this work") } + +// --------------------------------------------------------------------------- +// 8. ONE SICK COMMUNITY MUST NOT STARVE THE OTHERS +// +// A post can hold an admission from more than one community: the one it was +// written into, and any that FORKED it. Each has its own acceptance record in +// its own repo, on its own host, and those hosts fail independently. +// +// The compensation walks them in a loop, and a loop that returns on the first +// error makes the set only as available as its least-available member. The +// author is told their delete failed, which is true; what is NOT true is that +// the work was attempted — every community behind the failing one is skipped, +// and it is skipped again on every retry, because the retry re-enters the same +// loop and stops in the same place. A single community whose host is gone for +// good therefore strands the acceptance of every other community, permanently, +// with no signal that anything but the first is even involved. +// +// Processing all of them and joining the failures converges instead: each pass +// completes the ones it can, and the retry has strictly less to do. +// --------------------------------------------------------------------------- + +const ( + compensationCommunityA = "did:plc:deletecompensationcommunitya" + compensationCommunityB = "did:plc:deletecompensationcommunityb" +) + +func TestDeletePost_WithdrawsFromEveryCommunityEvenWhenOneOfThemFails(t *testing.T) { + h := newDeleteHarness(t) + h.acceptedIn(compensationCommunityA, compensationCommunityB) + + // A's host is down — a real failure, not the ErrCommunityNotHosted skip. + // B's is healthy, and B is behind A in the walk. + hostDown := errors.New("community A's PDS is unreachable") + h.withdrawer.answer(compensationCommunityA, CommunityWriteResult{}, hostDown) + h.withdrawer.answer(compensationCommunityB, CommunityWriteResult{Rev: withdrawalRev}, nil) + + err := h.delete(h.uri) + + // THE FAILURE STILL SURFACES. The compensation did not finish, and the + // client is the retry loop. + require.Error(t, err, "A's acceptance is still standing, so the delete has not finished") + + // AND B WAS STILL PROCESSED. This is the finding: today the loop returns on + // A and B is never reached, so B's community keeps a signed acceptance of a + // post that no longer exists — and no retry ever gets to it, because every + // retry stops on A first. + assert.Equalf(t, 1, h.withdrawer.callsFor(compensationCommunityB), + "community B was never asked to withdraw: the loop aborted on A. One unreachable host must not "+ + "strand every community behind it — each pass must attempt them all and join what failed, so "+ + "a retry has strictly less left to do rather than stopping in the same place forever") + assert.Lenf(t, h.admissions.stampsFor(compensationCommunityB), 1, + "B's withdrawal committed, so B's admission row must be stamped back to pending; leaving it "+ + "accepted tells the author their deleted post is still live in B") + + // A's row is NOT stamped: nothing was withdrawn there, so there is no + // withdrawal rev and no withdrawal to record. + assert.Empty(t, h.admissions.stampsFor(compensationCommunityA), + "A's withdrawal failed, so A's row must keep claiming the acceptance that is genuinely still there") + + // AND THE OPERATOR CAN SEE WHICH COMMUNITY FAILED. A joined error that + // named neither would leave "one of this post's communities failed" as the + // entire diagnosis. + assert.ErrorContainsf(t, err, compensationCommunityA, + "the error must name the community whose withdrawal failed; got: %v", err) + assert.ErrorIsf(t, err, hostDown, + "a generic (non-auth) failure must stay in the error chain — only the pds AUTH sentinels are "+ + "severed, and for a specific reason that does not apply here; got: %v", err) +} + +func TestDeletePost_ContinuesPastACommunityHostedElsewhereToOneWeDoHost(t *testing.T) { + h := newDeleteHarness(t) + h.acceptedIn(compensationCommunityA, compensationCommunityB) + + // A is somebody else's community — a permanent, expected skip — and it is + // FIRST. B is ours. + h.withdrawer.answer(compensationCommunityA, CommunityWriteResult{}, ErrCommunityNotHosted) + h.withdrawer.answer(compensationCommunityB, CommunityWriteResult{Rev: withdrawalRev}, nil) + + require.NoError(t, h.delete(h.uri), + "a community hosted elsewhere is not a failure — its own AppView performs the cleanup when the "+ + "deletion reaches it over the firehose — so it must not fail the author's delete") + + assert.Equal(t, 1, h.withdrawer.callsFor(compensationCommunityA)) + assert.Empty(t, h.admissions.stampsFor(compensationCommunityA), + "a withdrawal this instance cannot perform must not be recorded as one") + + assert.Equalf(t, 1, h.withdrawer.callsFor(compensationCommunityB), + "the skip must not end the walk: B's acceptance is ours to withdraw and is the whole reason "+ + "this instance is doing any of this") + assert.Len(t, h.admissions.stampsFor(compensationCommunityB), 1) +} + +// --------------------------------------------------------------------------- +// 9. THE COMMUNITY'S CREDENTIALS ARE NOT THE AUTHOR'S SESSION +// +// The acceptance being withdrawn lives in the COMMUNITY's repo and goes out on +// the community's stored service token. When that token is rejected, the pds +// package answers ErrUnauthorized/ErrForbidden — the same sentinels the author's +// own session produces when IT dies. +// +// Letting them travel up the chain means the XRPC boundary reads a community's +// expired token as the caller's session being dead and answers 401 "sign in +// again": a user with a perfectly healthy session is told to re-authenticate +// over a server-side credential problem they cannot fix, they do, and it fails +// identically. It also hides a real outage from 5xx alerting, because a 401 is +// a client error by every dashboard's reckoning. +// +// The legacy delete path already severs this (communityCredentialFailure, %v +// not %w). The compensation is the same class of failure and needs the same cut. +// --------------------------------------------------------------------------- + +func TestDeletePost_DoesNotBlameTheAuthorsSessionForTheCommunitysCredentials(t *testing.T) { + for _, sentinel := range []error{pds.ErrUnauthorized, pds.ErrForbidden} { + t.Run(sentinel.Error(), func(t *testing.T) { + h := newDeleteHarness(t) + + // Exactly the shape pds.wrapAPIError produces for a 401/403 from the + // community's host. + h.withdrawer.err = fmt.Errorf("applyWrites: %w: token has expired", sentinel) + + err := h.delete(h.uri) + + // IT STILL FAILS. The acceptance is standing and the compensation did + // not finish; swallowing it would be the silence this whole path exists + // to end. + require.Error(t, err, "the withdrawal failed, so the delete has not finished") + + // BUT NOT AS AN AUTH FAILURE. This is the finding: the boundary maps + // these sentinels to a 401 aimed at the CALLER. + assert.Falsef(t, errors.Is(err, sentinel), + "the community's rejected credentials reached the API boundary carrying %v, which the "+ + "mapper reads as the AUTHOR's session being dead — answering 401 `sign in again` to a "+ + "user whose session is fine, over a token only the server can fix. Sever it with %%v "+ + "the way communityCredentialFailure does, so it lands as an unclassified 500 and "+ + "shows up in outage alerting; got: %v", sentinel, err) + assert.Falsef(t, pds.IsAuthError(err), + "pds.IsAuthError is the predicate the boundary actually consults; got: %v", err) + + // The diagnosis survives the cut — the operator still gets the cause + // and the community it came from, just not as a typed auth error. + assert.ErrorContains(t, err, compensationCommunityDID, + "severing the sentinel must not sever the operator's ability to tell which community's "+ + "credentials were rejected") + assert.ErrorContains(t, err, "token has expired", + "the underlying message must survive; a bare `credentials rejected` names no cause") + }) + } +} + +// --------------------------------------------------------------------------- +// 10. AN EMPTY REV MEANS TWO DIFFERENT THINGS +// +// The stamp is guarded on a non-empty rev, because the admission repository +// refuses a fabricated watermark — correctly. But the guard currently answers +// the same way to two states that are not remotely the same: +// +// - a SKIP with no rev: nothing stood in the community's repo, so there is +// nothing to record. Returning nil is right. +// - a COMMITTED withdrawal with no rev: the acceptance record is GONE and the +// writer failed to report where. The row still names it, and returning nil +// reports success over exactly the half-finished state — record gone, row +// stranded, nothing scheduled to notice — that this whole change exists to +// make impossible. +// +// The second is a writer-contract violation (CommunityWriteResult documents a +// rev on every path), and a contract violation that produces silence is the +// worst available outcome. It has to surface. +// --------------------------------------------------------------------------- + +func TestDeletePost_SucceedsWhenTheSkippedWithdrawalHasNoRevToStamp(t *testing.T) { + h := newDeleteHarness(t) + h.withdrawer.result = CommunityWriteResult{Skipped: true, Rev: ""} + + require.NoError(t, h.delete(h.uri), + "nothing stood in the community's repo, so there is nothing to withdraw and nothing to record; "+ + "this is the ordinary redelivery case and must not fail the author's delete") + assert.Empty(t, h.admissions.stamps, + "there is no rev, and a stamp without one would be a fabricated watermark the repository "+ + "correctly refuses") +} + +func TestDeletePost_FailsWhenACommittedWithdrawalReportsNoRev(t *testing.T) { + h := newDeleteHarness(t) + + // Skipped false: the writer says it COMMITTED the deletion. And no rev, + // which its own contract forbids. + h.withdrawer.result = CommunityWriteResult{Skipped: false, Rev: ""} + + err := h.delete(h.uri) + + require.Error(t, err, + "the acceptance record is OUT of the community's repo and the admission row still names it, "+ + "because there was no rev to stamp with. Answering success here strands the row silently — "+ + "getStatus keeps saying `accepted` for a post whose acceptance no longer exists, and nothing "+ + "anywhere is scheduled to reconcile it. A writer that committed without reporting a rev has "+ + "broken its contract, and a broken contract must not be absorbed into a success") + + assert.Empty(t, h.admissions.stamps, + "an empty rev must never be stamped — the repository refuses it as a fabricated watermark, and "+ + "surfacing the writer's failure is the answer, not laundering it into a bad write") +} -- 2.51.2 From 32eeb52ca775d4b748243d0d00d2a23d8949027c Mon Sep 17 00:00:00 2001 From: Bretton Date: Sun, 9 Aug 2026 21:31:00 -0700 Subject: [PATCH 6/6] fix(posts): harden the delete compensation per review (loop, credentials, empty-rev) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three findings from the multi-model review: - The multi-community loop returned on the first failing community, so one unreachable community PDS starved cleanup of every other community that admitted the post — each client retry re-failed on the same one while the healthy communities' acceptances dangled. Now every admission is attempted and the failures are errors.Join'd, so a healthy community is always cleaned up on the same pass and the retry converges the rest regardless of order. - A community credential failure (the community's stored token, not the author's session) surfaced through %w carrying pds.ErrUnauthorized/Forbidden, so the XRPC mapper would tell the AUTHOR to sign in again over a community-side outage — and hide that outage from 5xx alerting. Now severed via communityCredentialFailure, exactly as the legacy deleteCommunityPost path already does. - A committed withdrawal that reported no rev silently returned success with the admission row unstamped — the exact silent strand this change exists to kill. Now split: a genuine skip with no rev stays a graceful no-op; a committed result with no rev surfaces. Also corrects the loop comment to stop overstating forks as a live reality (§10.2's fork/import flow is not built; a post URI carries at most one standing acceptance today). Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01QDvRJ45k6E5KrBARDHUtiM --- internal/core/posts/service.go | 62 ++++++++++++++++++++++++++++------ 1 file changed, 51 insertions(+), 11 deletions(-) diff --git a/internal/core/posts/service.go b/internal/core/posts/service.go index c6562f3..fa0feda 100644 --- a/internal/core/posts/service.go +++ b/internal/core/posts/service.go @@ -1863,17 +1863,32 @@ func (s *postService) compensateAuthorDelete(ctx context.Context, uri string) er return fmt.Errorf("resolving the admissions of %s: %w", uri, err) } - // EVERY community that admitted this post, not just the one the record - // names. A post can also carry a decision from a community that FORKED it - // (the case removedMarkers reads the same map for), and every acceptance of - // it now cites a record that no longer exists. The ones this AppView does - // not host answer ErrCommunityNotHosted and cost a lookup. + // ONE ROW TODAY, and this does not pretend otherwise. The only thing that + // would give a post URI a second STANDING acceptance is §10.2's fork/import + // flow, which is deliberately not built — both consumer paths refuse a + // cross-community acceptance until it is — so at most one accepted row per + // post exists right now. + // + // It is walked as a set anyway, because the data model already permits the + // rows and the fork flow is exactly what would populate them: a + // compensation shaped around a single community would, on the day that flow + // lands, silently leave every other acceptance standing. + // + // EVERY MEMBER IS ATTEMPTED AND THE FAILURES ARE JOINED. Returning on the + // first would make the set only as available as its least-available member: + // every community behind a dead host is skipped on this pass and skipped + // again on every retry, because the retry re-enters the same loop and stops + // in the same place — so one host that never comes back would strand the + // rest permanently. Attempting them all means each pass finishes what it + // can and the retry has strictly less left to do. The ones this AppView + // does not host answer ErrCommunityNotHosted and cost a lookup. + var failures []error for _, admission := range admissionsByURI[uri] { if err := s.withdrawAcceptanceOf(ctx, admission); err != nil { - return err + failures = append(failures, err) } } - return nil + return errors.Join(failures...) } // withdrawAcceptanceOf removes one community's acceptance record and stamps the @@ -1907,6 +1922,17 @@ func (s *postService) withdrawAcceptanceOf(ctx context.Context, admission *Admis log.Printf("[POST-DELETE] %s is hosted elsewhere; its acceptance of %s is not ours to withdraw", admission.CommunityDID, admission.PostURI) return nil + case pds.IsAuthError(err): + // THE COMMUNITY'S TOKEN, NOT THE AUTHOR'S SESSION. This withdrawal goes + // out on the community's stored service credentials, and their rejection + // produces the very same pds sentinels a dead OAuth session does. Letting + // them travel up the chain has the API boundary read a community-side + // credential outage as the CALLER's session being dead: the author is + // told to sign in again over something only the server can fix, and the + // outage is filed as a 401 client error where no 5xx alert looks. Severed + // exactly as the legacy delete path severs it. + return communityCredentialFailure( + fmt.Sprintf("withdrawing the acceptance of %s", admission.PostURI), admission.CommunityDID, err) case err != nil: return fmt.Errorf("withdrawing the acceptance of %s in %s: %w", admission.PostURI, admission.CommunityDID, err) @@ -1918,11 +1944,25 @@ func (s *postService) withdrawAcceptanceOf(ctx context.Context, admission *Admis // row stranded by an earlier pass that committed the delete and then failed // this stamp is caught up. The engine's accept path stamps a skip for the // same reason. + // + // AN EMPTY REV MEANS TWO DIFFERENT THINGS, and only one of them is fine. if withdrawn.Rev == "" { - // Defensive only: the writer contract reports a rev on every path, - // committed or skipped. An empty one must not be stamped — the - // repository refuses it as a fabricated watermark, correctly. - return nil + if withdrawn.Skipped { + // Nothing stood in the community's repo and there is no head rev to + // catch up from, so there is nothing to record. The ordinary + // redelivery case, and not a failure. + return nil + } + // A COMMITTED withdrawal that reports no rev is the writer breaking its + // own contract (CommunityWriteResult documents a rev on every path), and + // it leaves exactly the half-finished state this path exists to make + // impossible: the acceptance record is GONE, the row still names it, and + // there is no watermark to stamp it with. Stamping anyway would write a + // fabricated clock value the repository correctly refuses, and returning + // nil would report success over a silently stranded row — so the + // contract violation is surfaced instead. + return fmt.Errorf("withdrawing the acceptance of %s in %s: %w: the withdrawal committed but reported no rev", + admission.PostURI, admission.CommunityDID, ErrInvalidWatermark) } if _, err := s.admissions.ApplyAcceptanceDelete(ctx, CommunityDeleteCommand{