diff --git a/internal/atproto/pds/applywrites.go b/internal/atproto/pds/applywrites.go index 13d6ef7..5344afc 100644 --- a/internal/atproto/pds/applywrites.go +++ b/internal/atproto/pds/applywrites.go @@ -1,6 +1,11 @@ package pds -import "context" +import ( + "context" + "fmt" + + "github.com/bluesky-social/indigo/atproto/syntax" +) // Batch commits and commit revisions. // @@ -82,7 +87,7 @@ type RecordCommit struct { CommitCID string } -// LatestCommit is a repo's current head, as com.atproto.repo.getLatestCommit +// LatestCommit is a repo's current head, as com.atproto.sync.getLatestCommit // reports it. It is what a batch passes as swapCommit so a concurrent writer's // commit is a detected conflict rather than a silent clobber. type LatestCommit struct { @@ -120,22 +125,236 @@ type CommitClient interface { // Ensure the concrete client implements the commit-aware surface too. var _ CommitClient = (*client)(nil) +// applyWritesUnion is the lexicon namespace the batch's union discriminants are +// drawn from. A batch entry without a `$type` is not an applyWrites entry: the +// PDS has no other way to tell a create from a delete. +const applyWritesUnion = "com.atproto.repo.applyWrites#" + +// commitResponse is the `commit` object every commit-aware write returns. +type commitResponse struct { + CID string `json:"cid"` + Rev string `json:"rev"` +} + // ApplyWrites applies a batch of writes as one repo commit. func (c *client) ApplyWrites(ctx context.Context, writes []Write, swapCommit string) (*ApplyWritesResult, error) { - return nil, nil + if len(writes) == 0 { + // An empty batch would be a commit that says nothing, and the PDS would + // happily make one. Refusing locally keeps a caller whose state-shaping + // concluded "there is nothing to do" from advancing the repo's revision + // — and from stamping a watermark off a commit that changed no record. + return nil, fmt.Errorf("applyWrites: %w: a batch must carry at least one write", ErrBadRequest) + } + + entries := make([]map[string]any, 0, len(writes)) + for i, write := range writes { + entry := map[string]any{ + "$type": applyWritesUnion + string(write.Op), + "collection": write.Collection, + "rkey": write.RKey, + } + + switch write.Op { + case WriteOpCreate, WriteOpUpdate: + if write.Record == nil { + return nil, fmt.Errorf("applyWrites: %w: writes[%d] is a %s with no record body", + ErrBadRequest, i, write.Op) + } + entry["value"] = write.Record + case WriteOpDelete: + // No `value`. A delete carrying a record body is a malformed union + // member, and the PDS is entitled to refuse the whole batch for it. + default: + return nil, fmt.Errorf("applyWrites: %w: writes[%d] has unknown operation %q", + ErrBadRequest, i, write.Op) + } + + entries = append(entries, entry) + } + + payload := map[string]any{ + "repo": c.did, + "writes": entries, + // The records are Coves lexicons the PDS has never been taught, so + // validate:true is a refusal. It must be the BOOLEAN false rather than + // absent — the lexicon's default is not false. + "validate": false, + } + + // An empty swapCommit means "no guard", not "guard against the empty + // string": sending it would have every unguarded batch refused. + if swapCommit != "" { + payload["swapCommit"] = swapCommit + } + + var response struct { + Commit *commitResponse `json:"commit"` + Results []struct { + Type string `json:"$type"` + URI string `json:"uri"` + CID string `json:"cid"` + } `json:"results"` + } + + if err := c.apiClient.Post(ctx, syntax.NSID("com.atproto.repo.applyWrites"), payload, &response); err != nil { + return nil, wrapAPIError(err, "applyWrites") + } + + if response.Commit == nil || response.Commit.Rev == "" { + // The commit rev IS the watermark this method exists to obtain. A + // success without one leaves the caller with a committed batch it cannot + // order, and silently reporting an empty rev would have it stamp a clock + // value no commit ever had. + return nil, fmt.Errorf("applyWrites: PDS returned success without a commit rev (%d writes)", len(writes)) + } + + result := &ApplyWritesResult{ + CommitRev: response.Commit.Rev, + CommitCID: response.Commit.CID, + Results: make([]WriteResult, len(response.Results)), + } + for i, entry := range response.Results { + result.Results[i] = WriteResult{ + Op: writeOpOfResult(entry.Type), + URI: entry.URI, + CID: entry.CID, + } + } + + return result, nil +} + +// writeOpOfResult maps a result's union discriminant back onto the operation +// that produced it. An unrecognised discriminant yields the empty op rather +// than a guess: the results are positional, so the caller can still match them +// to what it submitted. +func writeOpOfResult(unionType string) WriteOp { + switch unionType { + case applyWritesUnion + "createResult": + return WriteOpCreate + case applyWritesUnion + "updateResult": + return WriteOpUpdate + case applyWritesUnion + "deleteResult": + return WriteOpDelete + default: + return "" + } } // PutRecordWithCommit creates or updates a record and reports the commit. +// +// EVERY PUT THROUGH THIS METHOD IS GUARDED, and that is the difference from +// Client.PutRecord. A non-empty swapRecord is the record CID the caller expects +// to be replacing; an EMPTY one sends `swapRecord: null`, which the PDS reads as +// "there must be no record here yet". The state-shaped writers pre-read before +// they write, and an unguarded put would let a concurrent writer's record be +// clobbered in exactly the window the pre-read opened. Callers wanting the +// lenient, unguarded write still have PutRecord. func (c *client) PutRecordWithCommit(ctx context.Context, collection, rkey string, record any, swapRecord string) (*RecordCommit, error) { - return nil, nil + payload := map[string]any{ + "repo": c.did, + "collection": collection, + "rkey": rkey, + "record": record, + // Coves lexicons; see ApplyWrites. + "validate": false, + } + + if swapRecord != "" { + payload["swapRecord"] = swapRecord + } else { + // Explicit JSON null, NOT an absent key. Absent means "overwrite + // whatever is there"; null means "expect nothing to be there", which is + // what makes a concurrent create a detected ErrSwapConflict rather than + // a silent overwrite. + payload["swapRecord"] = nil + } + + var response struct { + URI string `json:"uri"` + CID string `json:"cid"` + Commit *commitResponse `json:"commit"` + } + + if err := c.apiClient.Post(ctx, syntax.NSID("com.atproto.repo.putRecord"), payload, &response); err != nil { + return nil, wrapAPIError(err, "putRecord") + } + + return recordCommit("putRecord", collection, response.URI, response.CID, response.Commit) } // CreateRecordWithCommit creates a record and reports the commit. func (c *client) CreateRecordWithCommit(ctx context.Context, collection, rkey string, record any) (*RecordCommit, error) { - return nil, nil + payload := map[string]any{ + "repo": c.did, + "collection": collection, + "record": record, + "validate": false, + } + + // An empty rkey lets the PDS generate a TID, matching CreateRecord. + if rkey != "" { + payload["rkey"] = rkey + } + + var response struct { + URI string `json:"uri"` + CID string `json:"cid"` + Commit *commitResponse `json:"commit"` + } + + if err := c.apiClient.Post(ctx, syntax.NSID("com.atproto.repo.createRecord"), payload, &response); err != nil { + return nil, wrapAPIError(err, "createRecord") + } + + return recordCommit("createRecord", collection, response.URI, response.CID, response.Commit) +} + +// recordCommit validates a single-record write's response and shapes it. +// +// A 200 carrying no uri/cid, or no commit rev, is a malformed body from the PDS +// or something in front of it. It is reported rather than returned as a +// zero-valued success, because the two things missing here are precisely the two +// the caller is about to persist: the record it will reference, and the +// revision it will order by. +func recordCommit(operation, collection, uri, cid string, commit *commitResponse) (*RecordCommit, error) { + if uri == "" || cid == "" { + return nil, fmt.Errorf("%s: PDS returned success without uri/cid (collection %s)", operation, collection) + } + if commit == nil || commit.Rev == "" { + return nil, fmt.Errorf("%s: PDS returned success without a commit rev (collection %s)", operation, collection) + } + + return &RecordCommit{ + URI: uri, + CID: cid, + CommitRev: commit.Rev, + CommitCID: commit.CID, + }, nil } // GetLatestCommit returns the repo's current head. +// +// The method is com.atproto.SYNC.getLatestCommit. There is no +// com.atproto.repo.getLatestCommit — a PDS asked for one answers "No service +// configured for com.atproto.repo.getLatestCommit", because its XRPC router +// falls through to proxying a method it has never heard of. The head of a repo +// is sync-namespace data (it is the commit the firehose carries), and it is +// what a batch's swapCommit guard is read from. func (c *client) GetLatestCommit(ctx context.Context) (*LatestCommit, error) { - return nil, nil + var response commitResponse + + if err := c.apiClient.Get(ctx, syntax.NSID("com.atproto.sync.getLatestCommit"), + map[string]any{"did": c.did}, &response); err != nil { + return nil, wrapAPIError(err, "getLatestCommit") + } + + if response.CID == "" || response.Rev == "" { + // The CID is what a batch is guarded by. An empty one would be sent as + // "no guard" by ApplyWrites, silently turning an optimistic batch into + // an unconditional one. + return nil, fmt.Errorf("getLatestCommit: PDS returned success without a commit cid/rev (repo %s)", c.did) + } + + return &LatestCommit{CID: response.CID, Rev: response.Rev}, nil } diff --git a/internal/atproto/pds/applywrites_test.go b/internal/atproto/pds/applywrites_test.go index 10200d9..19106de 100644 --- a/internal/atproto/pds/applywrites_test.go +++ b/internal/atproto/pds/applywrites_test.go @@ -331,8 +331,13 @@ func TestClient_GetLatestCommit(t *testing.T) { // has to be read immediately before the batch is shaped — the pre-read and // the commit it is consistent with are the same observation. c, closeServer := newCommitClient(t, func(w http.ResponseWriter, r *http.Request) { - if r.URL.Path != "/xrpc/com.atproto.repo.getLatestCommit" { - t.Errorf("path = %q, want /xrpc/com.atproto.repo.getLatestCommit", r.URL.Path) + // THE SYNC NAMESPACE, NOT THE REPO ONE. getLatestCommit is + // com.atproto.sync.getLatestCommit — there is no repo-namespace + // spelling, and a PDS answers that one with "No service configured" + // rather than a 404, so the mistake reads as a deployment problem + // instead of a wrong method name. + if r.URL.Path != "/xrpc/com.atproto.sync.getLatestCommit" { + t.Errorf("path = %q, want /xrpc/com.atproto.sync.getLatestCommit", r.URL.Path) } if got := r.URL.Query().Get("did"); got != applyWritesDID { t.Errorf("did = %q, want %q", got, applyWritesDID) diff --git a/internal/atproto/pds/client.go b/internal/atproto/pds/client.go index 8d24364..11037c8 100644 --- a/internal/atproto/pds/client.go +++ b/internal/atproto/pds/client.go @@ -98,6 +98,44 @@ func wrapAPIError(err error, operation string) error { // Check if it's an APIError from atclient var apiErr *atclient.APIError if errors.As(err, &apiErr) { + // THE ERROR NAME IS CONSULTED ON TOP OF THE STATUS, for one case. A lost + // swap comes back from a live PDS as HTTP 400 with "error": + // "InvalidSwap", not the 409 the lexicon documents, so the status alone + // maps it onto ErrBadRequest — indistinguishable from a malformed + // record. It is the one 400 that must be re-read and retried rather than + // reported. + // + // A 409 InvalidSwap — what the lexicon says, and what some + // implementation may yet send — is BOTH sentinels at once, so callers + // written against either one behave correctly whichever status arrives. + if apiErr.Name == "InvalidSwap" { + switch apiErr.StatusCode { + case 400: + return fmt.Errorf("%s: %w: %s", operation, ErrSwapConflict, apiErr.Message) + case 409: + return fmt.Errorf("%s: %w: %w: %s", operation, ErrConflict, ErrSwapConflict, apiErr.Message) + } + } + + // "No such record" is the other name that outranks its status. The + // reference PDS answers getRecord for a missing record with HTTP 400 and + // "error": "RecordNotFound" (internal/core/users/profile_backfill.go + // documents the same observation), so the status alone calls an absent + // record a malformed request. A writer that shapes create-vs-update from + // a pre-read cannot tell those apart, and every caller already testing + // errors.Is(err, ErrNotFound) after a GetRecord is silently never true. + if apiErr.StatusCode == 400 && (apiErr.Name == "RecordNotFound" || apiErr.Name == "NotFound") { + return fmt.Errorf("%s: %w: %s", operation, ErrNotFound, apiErr.Message) + } + + // A 5xx is its own class, not the generic wrap. applyWrites answers a + // delete of a missing record — and a create of an existing one — with a + // 500, and a state-shaped writer meeting that has to know its pre-read + // went stale so it can re-shape the batch. + if apiErr.StatusCode >= 500 { + return fmt.Errorf("%s: %w: %s", operation, ErrServerError, apiErr.Message) + } + switch apiErr.StatusCode { case 400: return fmt.Errorf("%s: %w: %s", operation, ErrBadRequest, apiErr.Message) diff --git a/internal/core/comments/comment_service.go b/internal/core/comments/comment_service.go index da2e41c..6a5b445 100644 --- a/internal/core/comments/comment_service.go +++ b/internal/core/comments/comment_service.go @@ -913,7 +913,14 @@ func (s *commentService) UpdateComment(ctx context.Context, session *oauth.Clien if pds.IsAuthError(err) { return nil, fmt.Errorf("%w: %w", ErrNotAuthorized, err) } - if errors.Is(err, pds.ErrConflict) { + // ErrSwapConflict is the branch that actually fires. A PDS answers a + // stale swapRecord with HTTP 400 and "error": "InvalidSwap", not the 409 + // the lexicon documents — verified against a live PDS — so the + // ErrConflict test alone never matched and every concurrent edit + // surfaced as a generic failure instead of ErrConcurrentModification. + // Both are kept: 409 remains legal, and an implementation that sends it + // must not regress to the generic branch. + if errors.Is(err, pds.ErrSwapConflict) || errors.Is(err, pds.ErrConflict) { return nil, fmt.Errorf("%w: %w", ErrConcurrentModification, err) } return nil, fmt.Errorf("failed to update comment: %w", err) diff --git a/internal/core/posts/admit.go b/internal/core/posts/admit.go index 9c84492..e143953 100644 --- a/internal/core/posts/admit.go +++ b/internal/core/posts/admit.go @@ -469,6 +469,27 @@ type admissionDeps struct { // A non-nil error means the decision could NOT be made — a lookup failed — and // is distinct from a refusal, which is a decision. func admitPost(ctx context.Context, deps admissionDeps, req AdmissionRequest) (AdmissionDecision, error) { + decision, err := evaluateAdmissionPolicy(ctx, deps, req) + if err != nil || !decision.Admitted() { + return decision, err + } + return reserveSubmission(ctx, deps, req, decision.Community) +} + +// evaluateAdmissionPolicy is admitPost's steps 0-4: everything that decides +// whether this author may post to this community AT ALL, and nothing that +// reserves a slot for the attempt. +// +// THE SPLIT EXISTS FOR THE ACCEPTANCE ENGINE. §5.6's engine decides about a post +// that ALREADY EXISTS — often one it has decided about before, arriving again +// from an overlapping feed or a redrive — so it needs the policy and must not +// have the reservation. Running the ledger insert there would charge an author's +// quota for a firehose redelivery and then refuse the redecision as a duplicate +// of the submission it is redeciding. +// +// On an admission it returns the resolved community and NOTHING else, so a +// caller that goes on to reserve does so explicitly. +func evaluateAdmissionPolicy(ctx context.Context, deps admissionDeps, req AdmissionRequest) (AdmissionDecision, error) { // 0. The actor class must be one this decision knows. It gates everything // below — including the trusted skip of visibility, ban and authorization — // so an unknown value must fail CLOSED before any lookup runs. Falling @@ -550,6 +571,17 @@ func admitPost(ctx context.Context, deps admissionDeps, req AdmissionRequest) (A } } + return AdmissionDecision{Community: community}, nil +} + +// reserveSubmission is admitPost's steps 5-6: the dedupe insert and the +// rolling-window quota, both of which exist only for a NEW submission. +// +// They are one function rather than two because step 6 counts the row step 5 +// inserted — the reservation is on the ledger before the quota is measured, so +// the limit is reached when the count EXCEEDS it — and every path out that is +// not an admission has to hand the slot back. +func reserveSubmission(ctx context.Context, deps admissionDeps, req AdmissionRequest, community *communities.Community) (AdmissionDecision, error) { // 5. Dedupe, for every actor class. The INSERT is the check: a unique // violation means an identical submission is already on the ledger for this // window. It runs ahead of the quota so that a client retrying after a lost diff --git a/internal/core/posts/community_writer.go b/internal/core/posts/community_writer.go index 8f341f9..fbb4ecc 100644 --- a/internal/core/posts/community_writer.go +++ b/internal/core/posts/community_writer.go @@ -2,6 +2,9 @@ package posts import ( "context" + "errors" + "fmt" + "time" "Coves/internal/atproto/pds" ) @@ -177,18 +180,429 @@ func NewCommunityRecordWriter(repos CommunityRepoFactory, now Clock) CommunityRe return &communityRecordWriter{repos: repos, now: now} } +// swapRetryLimit is how many times a writer re-reads and re-shapes after losing +// an optimistic guard before it gives up and lets the caller try again later. +// +// It is bounded because a lost swap means somebody ELSE is writing this same +// record, and the only writers that can be are the three acceptance writers of +// §3.2 — all of which converge on the same rkey. Two retries is enough to +// absorb a real race; an unbounded loop against a community whose repo is +// genuinely churning would spin a queue worker against a PDS instead of +// deferring the subject and moving on. +const swapRetryLimit = 2 + +// standingRecord is what a pre-read found in the community's repo, reduced to +// the four things a writer shapes its commit from. +// +// It is read out of the decoded record rather than parsed into a typed struct +// because the writer only ever compares these fields; a full decode would give +// it the chance to fail on a record some other build wrote. +type standingRecord struct { + // CID is the RECORD's own CID — the swapRecord guard, not the subject. + CID string + + // SubjectCID is the content CID the record's strongRef pins. For an + // acceptance this is the whole point: an acceptance pinning the CID we want + // is one we must not rewrite. + SubjectCID string + + // CreatedAt is carried forward onto every update. An acceptance's createdAt + // means "when this community accepted this post", so restamping it on a + // re-acceptance or a repin would rewrite history every time a bridge + // refreshed its vote counts — and would give two writers racing to the same + // outcome two different record CIDs. + CreatedAt string + + // Code and Reason are the removal's decision, and empty on an acceptance. + Code string + Reason string +} + +// acceptanceMode says what an ABSENT acceptance record means to the caller. +type acceptanceMode int + +const ( + // acceptanceMayCreate: absence is the ordinary case — this is the first + // acceptance of the subject — and the writer creates one. + acceptanceMayCreate acceptanceMode = iota + + // acceptanceMustExist: a repin moves a STANDING acceptance onto new content + // and re-decides nothing, so it has no authority to create one. An absent + // record means the AppView's row and the community's repo disagree, and + // silently minting an acceptance nobody decided is the one thing a path that + // skips admission must never do. + acceptanceMustExist +) + func (w *communityRecordWriter) WriteAcceptance(ctx context.Context, cmd CommunityWriteCommand) (CommunityWriteResult, error) { - return CommunityWriteResult{}, nil + return w.pinAcceptance(ctx, cmd, acceptanceMayCreate) +} + +func (w *communityRecordWriter) RepinAcceptance(ctx context.Context, cmd CommunityWriteCommand) (CommunityWriteResult, error) { + return w.pinAcceptance(ctx, cmd, acceptanceMustExist) +} + +// pinAcceptance makes an acceptance of cmd.PostCID stand at the subject's rkey. +// +// THE PRE-READ DECIDES EVERYTHING. Whether there is work to do at all, what the +// put is guarded against, and what createdAt it carries are all read out of the +// repo rather than assumed, because the same record has three independent +// writers (§3.2) and every one of them retries. +// +// EVERY PUT IS GUARDED, including the first. A create is sent with an empty +// swapRecord, which pds.PutRecordWithCommit spells on the wire as "there must be +// no record here yet" — so a writer that lost the race between its pre-read and +// its put is told, rather than silently clobbering the winner's record. +func (w *communityRecordWriter) pinAcceptance(ctx context.Context, cmd CommunityWriteCommand, mode acceptanceMode) (CommunityWriteResult, error) { + if err := validateWriteCommand(cmd); err != nil { + return CommunityWriteResult{}, err + } + + repo, err := w.openRepo(ctx, cmd.CommunityDID) + if err != nil { + return CommunityWriteResult{}, err + } + + rkey := SubjectRkey(cmd.PostURI) + uri := recordURI(repo.DID(), AcceptanceCollection, rkey) + + for attempt := 0; ; attempt++ { + standing, err := readStandingRecord(ctx, repo, AcceptanceCollection, rkey) + if err != nil { + return CommunityWriteResult{}, err + } + + if standing == nil && mode == acceptanceMustExist { + return CommunityWriteResult{}, fmt.Errorf( + "repinning the acceptance of %s in %s: %w: no acceptance record stands at %s, so there is nothing to repin", + cmd.PostURI, cmd.CommunityDID, pds.ErrNotFound, uri) + } + + // ALREADY DONE. Re-putting an identical record would mint a fresh record + // CID, emit a commit that decided nothing, and invalidate every + // reference to the acceptance it just rewrote — on every retry, forever. + if standing != nil && standing.SubjectCID == cmd.PostCID { + return CommunityWriteResult{URI: uri, RKey: rkey, CID: standing.CID, Skipped: true}, nil + } + + swapRecord, createdAt := "", w.stamp() + if standing != nil { + swapRecord = standing.CID + if standing.CreatedAt != "" { + createdAt = standing.CreatedAt + } + } + + commit, err := repo.PutRecordWithCommit(ctx, AcceptanceCollection, rkey, + acceptanceRecord(cmd.PostURI, cmd.PostCID, createdAt), swapRecord) + if err == nil { + return CommunityWriteResult{URI: commit.URI, RKey: rkey, CID: commit.CID, Rev: commit.CommitRev}, nil + } + + if !errors.Is(err, pds.ErrSwapConflict) || attempt >= swapRetryLimit { + return CommunityWriteResult{}, fmt.Errorf("writing the acceptance of %s in %s: %w", + cmd.PostURI, cmd.CommunityDID, err) + } + // Lost the race. Loop: re-read what the winner actually wrote, and + // either discover the work is done or aim at the new record. + } } func (w *communityRecordWriter) WriteRemoval(ctx context.Context, cmd CommunityRemovalCommand) (CommunityWriteResult, error) { - return CommunityWriteResult{}, nil + if err := validateRemovalCommand(cmd); err != nil { + return CommunityWriteResult{}, err + } + + return w.commitPair(ctx, pairCommit{ + communityDID: cmd.CommunityDID, + postURI: cmd.PostURI, + standCollection: RemovalCollection, + clearCollection: AcceptanceCollection, + record: func(createdAt string) map[string]any { + return removalRecord(cmd, createdAt) + }, + // A removal is URI-SCOPED: the pinned CID is audit metadata recording + // the version present when the post was removed, and the removal applies + // to the post across later edits (§5.5). So a standing removal carrying + // the same decision is already the answer, and rewriting it to pin a + // newer CID would churn the record's CID on every re-fire while changing + // nothing anyone reads. The DECISION is what has to match. + unchanged: func(standing *standingRecord) bool { + return standing.Code == string(cmd.Code) && standing.Reason == cmd.Reason + }, + }) } func (w *communityRecordWriter) RestoreAcceptance(ctx context.Context, cmd CommunityWriteCommand) (CommunityWriteResult, error) { - return CommunityWriteResult{}, nil + if err := validateWriteCommand(cmd); err != nil { + return CommunityWriteResult{}, err + } + + return w.commitPair(ctx, pairCommit{ + communityDID: cmd.CommunityDID, + postURI: cmd.PostURI, + standCollection: AcceptanceCollection, + clearCollection: RemovalCollection, + record: func(createdAt string) map[string]any { + return acceptanceRecord(cmd.PostURI, cmd.PostCID, createdAt) + }, + unchanged: func(standing *standingRecord) bool { + return standing.SubjectCID == cmd.PostCID + }, + }) } -func (w *communityRecordWriter) RepinAcceptance(ctx context.Context, cmd CommunityWriteCommand) (CommunityWriteResult, error) { - return CommunityWriteResult{}, nil +// pairCommit is the shape both moderation commits have: one record is made to +// stand and its opposite is cleared, TOGETHER, so the firehose never carries a +// half-completed moderation action (§3.3). +// +// A removal and a restore are the same commit with the two collections swapped, +// which is exactly what §5.5 means by "there is no distinct restore operation on +// the wire" — consumers see ordinary events winning the §5.2 tuple CAS. +type pairCommit struct { + communityDID string + postURI string + + // standCollection holds the record this commit makes stand. + standCollection string + + // clearCollection holds the record this commit deletes, IF one is there. + // The delete is emitted only on presence: the PDS answers a delete of a + // missing record with a 500 and refuses the whole batch with it. + clearCollection string + + // record builds the body to write, stamped with the given createdAt. + record func(createdAt string) map[string]any + + // unchanged reports whether the standing record already says what this + // commit would say, so an identical re-fire writes nothing. + unchanged func(standing *standingRecord) bool +} + +// commitPair reads the subject's two records, shapes one commit from what it +// found, and applies it. +// +// THE SHAPE IS NOT OPTIONAL. applyWrites has no upsert and no tolerant delete: +// a create of an existing record and a delete of a missing one are both a 500 +// that takes the whole batch down with them. So presence chooses delete-or-not +// and create-or-update, and a pre-read that went stale under a concurrent writer +// is met the same way a lost swap is — by reading again and re-shaping. +func (w *communityRecordWriter) commitPair(ctx context.Context, spec pairCommit) (CommunityWriteResult, error) { + repo, err := w.openRepo(ctx, spec.communityDID) + if err != nil { + return CommunityWriteResult{}, err + } + + rkey := SubjectRkey(spec.postURI) + uri := recordURI(repo.DID(), spec.standCollection, rkey) + + for attempt := 0; ; attempt++ { + // THE HEAD IS READ FIRST, before the records the batch is shaped from. + // A swapCommit read afterwards could be NEWER than the state that shaped + // the batch, which would guard the commit against a revision that + // already contains the change the shape assumed was absent. Read first, + // and any interleaved write makes the guard stale — a detected conflict + // rather than a silent clobber. + head, err := repo.GetLatestCommit(ctx) + if err != nil { + return CommunityWriteResult{}, fmt.Errorf("reading the head of %s: %w", spec.communityDID, err) + } + + standing, err := readStandingRecord(ctx, repo, spec.standCollection, rkey) + if err != nil { + return CommunityWriteResult{}, err + } + toClear, err := readStandingRecord(ctx, repo, spec.clearCollection, rkey) + if err != nil { + return CommunityWriteResult{}, err + } + + // Nothing to clear and the standing record already says it: a re-fire of + // a commit that has already landed writes nothing, for the same reason + // an identical acceptance is not re-put. + if toClear == nil && standing != nil && spec.unchanged(standing) { + return CommunityWriteResult{URI: uri, RKey: rkey, CID: standing.CID, Skipped: true}, nil + } + + writes := make([]pds.Write, 0, 2) + if toClear != nil { + writes = append(writes, pds.Write{ + Op: pds.WriteOpDelete, + Collection: spec.clearCollection, + RKey: rkey, + }) + } + + op, createdAt := pds.WriteOpCreate, w.stamp() + if standing != nil { + op = pds.WriteOpUpdate + if standing.CreatedAt != "" { + createdAt = standing.CreatedAt + } + } + writes = append(writes, pds.Write{ + Op: op, + Collection: spec.standCollection, + RKey: rkey, + Record: spec.record(createdAt), + }) + + result, err := repo.ApplyWrites(ctx, writes, head.CID) + if err == nil { + return CommunityWriteResult{ + URI: uri, + RKey: rkey, + CID: standCIDOf(result, len(writes)-1), + Rev: result.CommitRev, + }, nil + } + + // A lost swapCommit and a 500 are the same fact from two directions: the + // state this batch was shaped from is not the state the PDS is in. Both + // are answered by reading again, never by resending the same shape. + staleShape := errors.Is(err, pds.ErrSwapConflict) || errors.Is(err, pds.ErrServerError) + if !staleShape || attempt >= swapRetryLimit { + return CommunityWriteResult{}, fmt.Errorf("committing %s for %s in %s: %w", + spec.standCollection, spec.postURI, spec.communityDID, err) + } + } +} + +// openRepo opens the community's repo and proves it is the one that was asked +// for. +// +// The DID check is not paranoia about the factory: the repo's DID is the +// AUTHORITY half of every record URI this writer produces, so a factory that +// handed back the wrong session would have one community vouching for a post +// with another community's key, and the resulting acceptance would look +// perfectly valid to every consumer on the network. +func (w *communityRecordWriter) openRepo(ctx context.Context, communityDID string) (CommunityRepo, error) { + repo, err := w.repos(ctx, communityDID) + if err != nil { + return nil, fmt.Errorf("opening the repo of community %s: %w", communityDID, err) + } + if repo == nil { + return nil, fmt.Errorf("opening the repo of community %s: the factory returned no client", communityDID) + } + if repo.DID() != communityDID { + return nil, fmt.Errorf("opening the repo of community %s: the factory returned a session on %s instead", + communityDID, repo.DID()) + } + return repo, nil +} + +// stamp is the createdAt a newly written record carries. +func (w *communityRecordWriter) stamp() string { + return w.now().UTC().Format(time.RFC3339) +} + +// readStandingRecord returns what stands at a rkey, or nil when nothing does. +// +// An absent record is a VALUE here rather than an error, because absence is +// half of what the shape is chosen from: it is the difference between a create +// and an update, and between a batch that carries a delete and one that must +// not. +func readStandingRecord(ctx context.Context, repo CommunityRepo, collection, rkey string) (*standingRecord, error) { + response, err := repo.GetRecord(ctx, collection, rkey) + if err != nil { + if errors.Is(err, pds.ErrNotFound) { + return nil, nil + } + return nil, fmt.Errorf("reading %s/%s from %s: %w", collection, rkey, repo.DID(), err) + } + if response == nil { + return nil, nil + } + + standing := &standingRecord{CID: response.CID} + if subject, ok := response.Value["subject"].(map[string]any); ok { + standing.SubjectCID, _ = subject["cid"].(string) + } + standing.CreatedAt, _ = response.Value["createdAt"].(string) + standing.Code, _ = response.Value["code"].(string) + standing.Reason, _ = response.Value["reason"].(string) + return standing, nil +} + +// standCIDOf picks the written record's CID out of a batch's results. +// +// Results are POSITIONAL — the lexicon returns one per submitted write, in +// order — so the record this commit made stand is the last one. A result that +// carries no CID (the lexicon's #updateResult may omit it) leaves the field +// empty rather than borrowing a neighbour's. +func standCIDOf(result *pds.ApplyWritesResult, index int) string { + if result == nil || index < 0 || index >= len(result.Results) { + return "" + } + return result.Results[index].CID +} + +// recordURI is the AT-URI of a record in a repo. The authority is the repo's +// own DID, which for these two collections is the COMMUNITY — an acceptance in +// the author's repo would be an author vouching for themselves. +func recordURI(repoDID, collection, rkey string) string { + return "at://" + repoDID + "/" + collection + "/" + rkey +} + +// acceptanceRecord is a social.coves.community.acceptance body. The community +// is implicit in the repo it lands in, which is why it is not a field. +func acceptanceRecord(postURI, postCID, createdAt string) map[string]any { + return map[string]any{ + "$type": AcceptanceCollection, + "subject": map[string]any{"uri": postURI, "cid": postCID}, + "createdAt": createdAt, + } +} + +// removalRecord is a social.coves.community.removal body. +func removalRecord(cmd CommunityRemovalCommand, createdAt string) map[string]any { + record := map[string]any{ + "$type": RemovalCollection, + "subject": map[string]any{"uri": cmd.PostURI, "cid": cmd.PostCID}, + "code": string(cmd.Code), + "createdAt": createdAt, + } + // reason is optional in the lexicon, and an empty string is not the same + // thing as an absent one: a client rendering #removedPost would show an + // explanation that says nothing. + if cmd.Reason != "" { + record["reason"] = cmd.Reason + } + return record +} + +// validateWriteCommand refuses an acceptance that would pin nothing. +// +// A strongRef without a CID is the one thing an acceptance may not be: the +// pinned CID IS the guarantee, and an acceptance naming only a URI would render +// whatever the author put there most recently. +func validateWriteCommand(cmd CommunityWriteCommand) error { + switch { + case cmd.CommunityDID == "": + return fmt.Errorf("acceptance write: %w", NewValidationError("communityDID", "is required")) + case cmd.PostURI == "": + return fmt.Errorf("acceptance write: %w", NewValidationError("postURI", "is required")) + case cmd.PostCID == "": + return fmt.Errorf("acceptance write: %w", NewValidationError("postCID", + "is required — an acceptance's subject is a strongRef, and one without a CID pins nothing")) + } + return nil +} + +// validateRemovalCommand refuses a removal with no reason code. `code` is +// required by the lexicon and is what a client renders in #removedPost and what +// the author is told. +func validateRemovalCommand(cmd CommunityRemovalCommand) error { + switch { + case cmd.CommunityDID == "": + return fmt.Errorf("removal write: %w", NewValidationError("communityDID", "is required")) + case cmd.PostURI == "": + return fmt.Errorf("removal write: %w", NewValidationError("postURI", "is required")) + case cmd.PostCID == "": + return fmt.Errorf("removal write: %w", NewValidationError("postCID", + "is required — it records the version present at removal time")) + case cmd.Code == "": + return fmt.Errorf("removal write: %w", NewValidationError("code", "is required")) + } + return nil } diff --git a/internal/core/posts/engine.go b/internal/core/posts/engine.go index a39b7e9..f504558 100644 --- a/internal/core/posts/engine.go +++ b/internal/core/posts/engine.go @@ -1,6 +1,12 @@ package posts -import "context" +import ( + "context" + "errors" + "fmt" + + "Coves/internal/atproto/pds" +) // The acceptance engine: the single decision point of // docs/PRD_AUTHOR_OWNED_POSTS.md §5.6. @@ -127,5 +133,202 @@ func NewAcceptanceEngine( // authority on what the repo says, and this write is the AppView catching up // with itself. func (e *AcceptanceEngine) ProcessAdmission(ctx context.Context, communityDID, postURI string) (EngineOutcome, error) { - return "", nil + // THE ROW IS READ FIRST, and the status half of the routing decision comes + // from it rather than from whoever queued the subject. A queue entry is a + // hint that something happened; the row is what the AppView believes, and + // only one of those can be trusted to say a post has already been removed. + row, err := e.admissions.Get(ctx, communityDID, postURI) + if err != nil { + return EngineDeferred, fmt.Errorf("reading the admission of %s in %s: %w", postURI, communityDID, err) + } + if row == nil { + return EngineDeferred, fmt.Errorf("reading the admission of %s in %s: %w", postURI, communityDID, ErrNotFound) + } + + switch row.Status { + case AdmissionStatusPending, AdmissionStatusPendingReacceptance: + // The two states a decision is owed for. + default: + // A DEFENSIVE SKIP, and `removed` is why it has to be here rather than + // in the queue. Removal is terminal against everything except a + // moderator restore at a strictly greater watermark (§5.5), so an engine + // that re-ran policy on a settled row would launder a removed post + // straight back into the feeds it was removed from — and the queue hands + // it the same subject constantly: redrives, overlapping feeds, a notify + // racing the firehose. + return EngineDeferred, nil + } + + // An acceptance's subject is a strongRef, and a strongRef without a CID pins + // nothing. A row with no evaluated CID is one whose content this AppView has + // not decoded yet, so there is nothing to judge and nothing to pin. + if row.EvaluatedCID == nil || *row.EvaluatedCID == "" { + return EngineDeferred, nil + } + evaluatedCID := *row.EvaluatedCID + + decision, err := e.decider.DecideAdmission(ctx, communityDID, postURI) + if err != nil { + // UNDECIDED, which is neither an admission nor a refusal. A community + // lookup or a ban lookup that could not be reached must never become a + // verdict: recording one would turn a Postgres blip into a permanent + // decision about someone's post, and set redrivable=false on it. + return EngineDeferred, fmt.Errorf("deciding %s for %s: %w", postURI, communityDID, err) + } + if !decision.Admitted() && decision.Code == "" { + // The same state arriving as a value rather than an error. Admitted() is + // false for both a refusal and an undecided answer, so the code is what + // tells them apart — and an answer with neither is one nothing may be + // written from. + return EngineDeferred, fmt.Errorf("deciding %s for %s: the policy returned no verdict: %w", + postURI, communityDID, decision.Cause) + } + + if decision.Admitted() { + return e.accept(ctx, communityDID, postURI, evaluatedCID) + } + + if row.Status == AdmissionStatusPending { + return e.reject(ctx, communityDID, postURI, evaluatedCID, decision.Code) + } + + // pending_reacceptance + refused. §5.5 is explicit that a failed + // re-acceptance is a REMOVAL: an acceptance is standing in the community's + // repo and was published to the firehose, so an AppView-local rejection + // would leave it in place and federated peers would keep rendering a post + // this AppView had hidden. + return e.remove(ctx, communityDID, postURI, evaluatedCID, decision.Code) +} + +// accept makes the community's acceptance stand and then stamps the row. +// +// THE ORDER IS THE POINT. The repo write happens first and the row is stamped +// only after it commits, so the AppView can never claim an acceptance the PDS +// never took. +func (e *AcceptanceEngine) accept(ctx context.Context, communityDID, postURI, evaluatedCID string) (EngineOutcome, error) { + written, err := e.write(ctx, communityDID, func() (CommunityWriteResult, error) { + return e.writer.WriteAcceptance(ctx, CommunityWriteCommand{ + CommunityDID: communityDID, + PostURI: postURI, + // The CID the AppView has INDEXED. Pinning anything else is an + // acceptance of content nobody evaluated. + PostCID: evaluatedCID, + }) + }) + if err != nil { + return EngineDeferred, err + } + + // A SKIPPED WRITE STAMPS NOTHING. The repo already held this exact + // acceptance, so nothing committed and there is no revision to report — + // getRecord does not reveal the revision an existing record was written at. + // The repository refuses an empty rev as a fabricated watermark, and + // inventing one to get past that would write a clock value no commit had. + if written.Skipped { + return EngineAccepted, nil + } + + if _, err := e.admissions.ApplyAcceptance(ctx, ApplyAcceptanceCommand{ + CommunityDID: communityDID, + PostURI: postURI, + AcceptanceURI: written.URI, + AcceptanceRkey: written.RKey, + PinnedCID: evaluatedCID, + // The rev the write actually COMMITTED in — the §5.2 watermark. Its + // OpRank is deliberately left zero: the repository derives the rank from + // the operation itself, because the rank IS the operation's kind. + Watermark: CommunityWatermark{Rev: written.Rev}, + }); err != nil { + // The record IS in the community's repo and the row disagrees. The + // firehose will reconcile it, but a silent divergence is how a post + // stays invisible for hours with nothing to search for. + return EngineAccepted, fmt.Errorf("stamping the acceptance of %s in %s: %w", postURI, communityDID, err) + } + + return EngineAccepted, nil +} + +// reject records the AppView's own refusal and writes NO community record. +// +// §3.3: a submission refused before it was ever accepted must not bloat the +// community's repository — spam would otherwise be permanently archived in the +// repo of the community that refused it. +func (e *AcceptanceEngine) reject(ctx context.Context, communityDID, postURI, evaluatedCID string, code DecisionCode) (EngineOutcome, error) { + if _, err := e.admissions.RecordRejection(ctx, RecordRejectionCommand{ + CommunityDID: communityDID, + PostURI: postURI, + DecisionCode: string(code), + // The CID the verdict JUDGED. The repository lands the rejection only on + // a pending row still holding it, so an author who edited between the + // read and the write gets fresh content judged fresh rather than + // condemned by a verdict about something else. + JudgedCID: evaluatedCID, + // A policy refusal is terminal. Leaving this true would have the + // dead-letter redrive pass retry a decision that will never change. + Redrivable: false, + }); err != nil { + return EngineDeferred, fmt.Errorf("recording the rejection of %s in %s: %w", postURI, communityDID, err) + } + + return EngineRejected, nil +} + +// remove withdraws a standing acceptance with a removal commit. +func (e *AcceptanceEngine) remove(ctx context.Context, communityDID, postURI, evaluatedCID string, code DecisionCode) (EngineOutcome, error) { + written, err := e.write(ctx, communityDID, func() (CommunityWriteResult, error) { + return e.writer.WriteRemoval(ctx, CommunityRemovalCommand{ + CommunityDID: communityDID, + PostURI: postURI, + // Audit metadata: the version present at removal time. The removal + // itself is URI-scoped and survives later edits (§5.5). + PostCID: evaluatedCID, + Code: code, + }) + }) + if err != nil { + return EngineDeferred, err + } + + if written.Skipped { + return EngineRemoved, nil + } + + if _, err := e.admissions.ApplyRemoval(ctx, ApplyRemovalCommand{ + CommunityDID: communityDID, + PostURI: postURI, + DecisionCode: string(code), + Watermark: CommunityWatermark{Rev: written.Rev}, + }); err != nil { + return EngineRemoved, fmt.Errorf("stamping the removal of %s in %s: %w", postURI, communityDID, err) + } + + return EngineRemoved, nil +} + +// write runs one community-repo write, renewing the community's credentials +// once if they turn out to be stale. +// +// ONE FORCED RENEWAL, NEVER MORE. A community's PDS access token expires on a +// schedule that has nothing to do with moderation, so a single retry is the +// difference between a decision that lands and a decision that has to be +// redriven. Retrying persistently would instead have every pass in the queue +// spin against the PDS of a community whose credentials are genuinely gone. +// +// AND NEVER A VERDICT. Whatever comes back, a write failure is returned as an +// error and the caller defers: "I could not write the acceptance" and "this post +// is not acceptable" are opposite facts, and an engine that recorded the second +// when it meant the first would answer the author with a permanent refusal that +// nothing would ever retry. +func (e *AcceptanceEngine) write(ctx context.Context, communityDID string, attempt func() (CommunityWriteResult, error)) (CommunityWriteResult, error) { + result, err := attempt() + if err == nil || !errors.Is(err, pds.ErrUnauthorized) { + return result, err + } + + if refreshErr := e.credentials.RefreshCommunityCredentials(ctx, communityDID); refreshErr != nil { + return CommunityWriteResult{}, fmt.Errorf("renewing the credentials of %s after %w: %w", + communityDID, err, refreshErr) + } + + return attempt() } diff --git a/internal/core/posts/record_diff.go b/internal/core/posts/record_diff.go index eb9de7f..6036714 100644 --- a/internal/core/posts/record_diff.go +++ b/internal/core/posts/record_diff.go @@ -1,5 +1,12 @@ package posts +import "reflect" + +// bridgedStatsField is the ONE field §5.5 lets through unexamined. It is named +// once, here, so that widening the exception is a visible edit to this file +// rather than a condition that grew a second disjunct. +const bridgedStatsField = "bridgedStats" + // RecordDiffClass says what kind of change an author made to a post record — // the classification the bridgedStats exception of §5.5 turns on. type RecordDiffClass string @@ -40,5 +47,48 @@ const ( // bridge post; in the unsafe direction it is edited content rendering under an // acceptance granted to different content. func classifyRecordDiff(oldRecord, newRecord map[string]any) RecordDiffClass { - return "" + // A version that is not there proves nothing about the version that is. The + // engine reaches this with whatever it managed to decode, and "I could not + // read the old record" must not read as "the change was only stats". + if oldRecord == nil || newRecord == nil { + return RecordDiffPolicyRelevant + } + + // EVERYTHING EXCEPT bridgedStats, COMPARED WHOLE. Comparing the remainder as + // one value rather than field by field is what makes the exception fail + // closed against fields this build has never heard of: an unknown key is + // part of the remainder, so it is compared like any other, and a bridge + // running a newer lexicon cannot smuggle one past. + oldRest := withoutBridgedStats(oldRecord) + newRest := withoutBridgedStats(newRecord) + if !reflect.DeepEqual(oldRest, newRest) { + return RecordDiffPolicyRelevant + } + + // The remainder is identical, so bridgedStats is the only thing left that + // can differ. Presence is part of the comparison: a bridge that started or + // stopped reporting counts changed its stats, not its content. + oldStats, oldHasStats := oldRecord[bridgedStatsField] + newStats, newHasStats := newRecord[bridgedStatsField] + if oldHasStats == newHasStats && reflect.DeepEqual(oldStats, newStats) { + return RecordDiffNone + } + return RecordDiffBridgedStatsOnly +} + +// withoutBridgedStats copies a record with the excepted field removed. +// +// The copy is not an optimisation to skip: the caller owns these maps — they +// are decoded event payloads that the engine goes on to index — and a +// classifier that deleted a field from them would silently strip bridged vote +// counts out of everything downstream of the decision. +func withoutBridgedStats(record map[string]any) map[string]any { + rest := make(map[string]any, len(record)) + for field, value := range record { + if field == bridgedStatsField { + continue + } + rest[field] = value + } + return rest } diff --git a/internal/core/posts/rkey.go b/internal/core/posts/rkey.go index 141091a..5b672e5 100644 --- a/internal/core/posts/rkey.go +++ b/internal/core/posts/rkey.go @@ -1,5 +1,23 @@ package posts +import ( + "crypto/sha256" + "encoding/base32" + "strings" +) + +// subjectRkeyEncoding is RFC 4648 base32 with the padding removed. +// +// Padding is dropped rather than trimmed afterwards because '=' is not in the +// atProto record-key charset, and the lowercasing that follows is what keeps +// the key inside it: the standard alphabet is uppercase, and an uppercase rkey +// is a different key to a PDS that treats record keys as opaque bytes. +// +// base32-Hex (the other encoding in the same package) draws from a DIFFERENT +// alphabet and would produce a different, equally plausible-looking key for +// every subject in the network. It is not interchangeable here. +var subjectRkeyEncoding = base32.StdEncoding.WithPadding(base32.NoPadding) + // SubjectRkey is the record key a community's records about one post use. // // It is the unpadded lowercase base32 encoding of the SHA-256 digest of the @@ -26,5 +44,6 @@ package posts // The row's bytes are the identity the AppView indexes under, so a writer that // normalized would key its records to a URI the reader never looks up. func SubjectRkey(postURI string) string { - return "" + digest := sha256.Sum256([]byte(postURI)) + return strings.ToLower(subjectRkeyEncoding.EncodeToString(digest[:])) } diff --git a/internal/core/posts/service_writeforward_test.go b/internal/core/posts/service_writeforward_test.go index b30fa03..cac7d11 100644 --- a/internal/core/posts/service_writeforward_test.go +++ b/internal/core/posts/service_writeforward_test.go @@ -238,28 +238,14 @@ func TestService_DeleteRemovesTheRecordFromTheCommunityRepo(t *testing.T) { assert.True(t, testkit.IsNotFound(getRecordErr(ctx, community, postCollection, rkey)), "the post record is still in the community's repo after its author deleted it") - // KNOWN DEFECT, pinned as it behaves rather than as it is meant to. - // - // DeletePost intends a repeated delete to be idempotent: it checks the - // record fetch for pds.ErrNotFound and returns nil, commented "Post already - // deleted or never existed - idempotent success" (service.go step 7). That - // branch is unreachable against this PDS. com.atproto.repo.getRecord answers - // a missing record with HTTP 400 and "Could not locate record", and - // pds/client.go maps 400 to ErrBadRequest — so the not-found check misses, - // and the delete a client retries after a lost response comes back as an - // opaque failure the handler renders as a 500. - // - // Asserting the intent here would fail the suite over a production bug this - // task is not fixing; asserting nothing would let the bug become invisible. - // So the assertion is the current truth, and it is written to FAIL LOUDLY - // the moment the classification is fixed — at which point this block becomes - // assert.NoError and the comment goes away. - err := f.service.DeletePost(ctx, sessionFor(t, f.author, f.pds.URL()), - posts.DeletePostRequest{URI: resp.URI}) - require.Errorf(t, err, "the idempotent-delete defect appears to be FIXED: "+ - "replace this block with assert.NoError and delete the KNOWN DEFECT comment above it") - assert.Contains(t, err.Error(), "Could not locate record", - "the repeated delete failed for a different reason than the known not-found misclassification") + // The idempotent-delete path is real now: the PDS answers a missing record + // with HTTP 400 named RecordNotFound, and the client's name-before-status + // mapping turns that into pds.ErrNotFound, so DeletePost's not-found branch + // is reachable. Previously pinned as a known defect (p3 from the + // test-refactor loop); fixed by task 4's PDS error mapping. + assert.NoError(t, f.service.DeletePost(ctx, sessionFor(t, f.author, f.pds.URL()), + posts.DeletePostRequest{URI: resp.URI}), + "a repeated delete is idempotent — the retried delete after a lost response succeeds") } func TestService_DeleteRefusesEveryoneButTheAuthor(t *testing.T) {