From acd9f78f2f83f97993cc70358f20c652eb6f28b8 Mon Sep 17 00:00:00 2001 From: Bretton <36870434+BrettM86@users.noreply.github.com> Date: Sat, 11 Jul 2026 01:36:32 +0000 Subject: [PATCH] Task 11: pre-internet-facing hardening — admission control, #account frame, pruners The internet-facing pass over the FOLLOWUPS backlog. Biggest diff of the loop; 17 FOLLOWUPS items closed. Admission control (new internal/ratelimit shared package — keyed token bucket, sweep throttle, 50k fail-closed cap, RemoteAddr keying): - /inbox: per-IP limit before the body read (bounds signature-verify cost), per-signer limit after verify, and a dedicated tighter per-IP cap INSIDE tombstonedSelfDelete before the outbound confirmation fetch — closing the SSRF/durable-write amplification path task 10 flagged. All inbox refusals are 503 (Lemmy drops 4xx forever, retries 5xx). - Sync surface: per-IP limit (429) + SYNC_MAX_SUBSCRIBERS connection cap (reserve-then-check, leak-free on every exit). - Refusals are observable: per-surface expvar counters + sampled Warn (a review caught that a mistuned limit would otherwise drop all traffic silently). /admin/metrics scoped to tidepool's own expvar keys. New firehose wire surface — the #account frame: - migration 011: firehose_events.kind (commit|account) + account_active/ account_status, commit columns nullable under a per-kind shape CHECK. - AppendAccountEvent shares the global commit advisory lock + in-tx pg_notify, so seq==visibility holds across both kinds and the frame always replays after the scrubs that precede it. - DeleteActor emits #account{active:false,status:deleted} LAST (verified through a real bigsky: the repo is carstore-purged, not just filtered — "deleted" is the only status token that purges, source-confirmed). Terminal state is a fixpoint (no double emit on redelivery). Protocol correctness & scrubs: - commitRecord writes record + ap_objects mapping in ONE tx via PutRecordTx/TxSideEffect; the NoOp path still commits the mapping while preserving the deterministic-rkey true-NoOp contract (no new seq/event). - DeleteActor scrubs blobs under community DIDs + own repo + the actor's vote_events (ScrubVoter, ordered aggregate locks, DELETE...RETURNING recompute so a racing vote can't leave a phantom count); a failed blob delete is retryable, not swallowed. - Media carry-forward on transient profile-refresh failure keeps the stored blob; a permanent 404/410 correctly drops it. - FollowRetrier: atomic UPDATE...RETURNING claim (no Accept clobber, no double-claim), loud at attempt exhaustion. Housekeeping: three batched pruners (ap_tombstones, undone vote_events, firehose) via a shared fail-closed retention runner; MAX_BLOB_BYTES wired to the AP client's response cap; MaxSeededCount sanity cap; service_keys.private_key_pem renamed to key_material; dead NewNoopVotes removed. delete-before-create: README was right (task 06 closed it), the FOLLOWUPS entry was stale — struck. Review: 6/7 delivered (glm watchdog-killed). No confirmed high-severity; gemini's lone HIGH (carry-forward assertion) was a false positive (typed atdata.Blob, test green) and discarded. Verification: unit suite green (incl. new prune/ratelimit packages), final clean make e2e 17/17 (232s). Co-Authored-By: Claude Fable 5 --- FOLLOWUPS.md | 176 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++----------------------------------------------- LOOP_STATE.md | 78 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-- README.md | 74 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++------------ cmd/tidepool/main.go | 74 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-------------------- internal/ap/client.go | 22 ++++++++++++++++++---- internal/ap/service_actor.go | 2 +- internal/ap/service_actor_test.go | 6 +++--- internal/config/config.go | 100 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ internal/db/migrations/011_account_events.sql | 59 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ internal/db/migrations/012_follow_retry.sql | 17 +++++++++++++++++ internal/db/migrations/013_rename_service_key_column.sql | 10 ++++++++++ internal/identity/keys.go | 4 ++-- internal/identity/keys_test.go | 2 +- internal/ingest/follow.go | 48 ++++++++++++++++++++++++++++++++++++++++++++---- internal/ingest/hardening_test.go | 340 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ internal/ingest/inbox.go | 127 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-- internal/ingest/retry.go | 184 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ internal/ingest/votes.go | 36 ++++-------------------------------- internal/materialize/actors.go | 68 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-------- internal/materialize/blobs.go | 22 +++++++++++++++++----- internal/materialize/hardening_test.go | 304 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ internal/materialize/materialize_test.go | 25 ++++++++++++++++++++++++- internal/materialize/materializer.go | 87 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++----------------------- internal/materialize/updates.go | 122 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++------------ internal/materialize/updates_test.go | 8 +++++++- internal/prune/prune.go | 68 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ internal/prune/prune_test.go | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++ internal/ratelimit/ratelimit.go | 163 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ internal/ratelimit/ratelimit_test.go | 98 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ internal/repo/blob.go | 39 +++++++++++++++++++++++++++++++++++++++ internal/repo/events.go | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ internal/repo/hardening_test.go | 209 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ internal/repo/repo.go | 57 ++++++++++++++++++++++++++++++++++++++++++++++----------- internal/repo/sync.go | 104 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-------------------- internal/repo/sync_test.go | 6 +++--- internal/store/ap_objects.go | 18 +++++++++++++++++- internal/store/communities.go | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++- internal/store/hardening_test.go | 204 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ internal/store/interfaces.go | 33 +++++++++++++++++++++++++++++++-- internal/store/models.go | 23 +++++++++++++++++------ internal/store/service_keys.go | 14 +++++++------- internal/store/service_keys_test.go | 8 ++++---- internal/store/tombstones.go | 26 ++++++++++++++++++++++++++ internal/sync/frames.go | 18 ++++++++++++++++++ internal/sync/hardening_test.go | 129 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ internal/sync/prune.go | 50 ++++---------------------------------------------- internal/sync/server.go | 94 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--------- internal/sync/subscribe.go | 43 +++++++++++++++++++++++++++++++++++-------- internal/sync/sync_test.go | 14 ++++++++++---- internal/votes/aggregator.go | 169 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ internal/votes/hardening_test.go | 153 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ internal/votes/ratelimit.go | 103 ------------------------------------------------------------------------------------------------------- internal/votes/ratelimit_test.go | 98 -------------------------------------------------------------------------------------------------- internal/votes/xrpc.go | 20 ++++---------------- tests/e2e/helpers.go | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++---- tests/e2e/lifecycle_test.go | 73 ++++++++++++++++++++++++++++++++++++++++++++++++++----------------------- 56 file(s) changed, 3716 insertion(s)(+), 546 deletion(s)(-) diff --git a/FOLLOWUPS.md b/FOLLOWUPS.md --- a/FOLLOWUPS.md +++ b/FOLLOWUPS.md @@ -22,15 +22,20 @@ indexed before the post" validation needs a retry/park-and-reprocess path, or Coves consumes the bridge firehose directly. The e2e suite's listener buffers out-of-order events (`tests/e2e/helpers.go` `pending`) and asserts presence + linkage instead of cross-repo arrival order. -- **Tombstoned (`active:false`) repo status is not observable through - bigsky.** It filters tombstoned/taken-down repos out of `listRepos`, - serves no `getRepoStatus`, and learns account state upstream only from - `#account` frames — which the bridge does not emit yet (task 11). When - the `#account` frame lands, add a relay-side e2e assertion that a - consent-revoked repo disappears from the relay's `listRepos`. Task 10's - `TestDeleteActor_ScrubsAndTombstones` PINS the current gap (the relay - still lists a tombstoned repo) with a comment pointing here — that - assertion must flip when task 11 lands. +- ~~Tombstoned (`active:false`) repo status is not observable through + bigsky (no `#account` frame emitted).~~ **Closed by task 11**: the bridge + emits `#account{active:false, status:"deleted"}` through the durable + firehose log on `Delete(Actor)` (after the scrub delete-commits, same + advisory-lock seq ordering), and `TestDeleteActor_ScrubsAndTombstones` + now asserts the frame on the bridge's own firehose AND the repo + disappearing from the relay's `listRepos`. Wire fact (source-verified + against the pinned bigsky): on `status:"deleted"` bigsky re-resolves the + DID doc (PDS-authority check), marks the account `tombstoned` (the + `listRepos` filter), and PURGES the repo's carstore data — exactly the + downstream purge consent revocation wants. bigsky's `listRepos` filter + also excludes `deactivated`/`suspended`/`takendown`, but only `deleted` + purges. nobridge (reversible) deliberately emits NO frame — the repo + stays active. - **A fresh bigsky refuses all non-admin `requestCrawl`**: the new-PDS per-day limit defaults to 0 and is checked BEFORE the trusted-domain list. Any deployment announcing to a self-hosted relay needs the @@ -106,10 +111,14 @@ id ("skip all past activities", `crates/federate/src/worker.rs`), so the Accept answering the very first Follow to a given Lemmy instance is usually skipped: the instance row is created by that same Follow, and the per-instance worker spawns after the Accept is queued. Re-sending the - Follow (fresh activity id → fresh Accept) recovers. The harness retries in - `subscribeCommunity`; production operators hit this at most once (usually) - per peer instance. Consider an automatic Follow re-send in the follow - lifecycle when a subscription stays `pending` past a threshold. + Follow (fresh activity id → fresh Accept) recovers. ~~Consider an + automatic Follow re-send when a subscription stays `pending` past a + threshold.~~ **Closed by task 11**: `ingest.FollowRetrier` re-sends the + Follow (fresh activity id) for subscriptions pending past 2m, bounded to + 5 total sends (`communities.follow_requested_at`/`follow_attempts`, + migration 012; unsubscribe resets the budget). The harness's own retry in + `subscribeCommunity` stays — the suite must not depend on the 2m + threshold. - **Author auto-upvotes do not federate (e2e).** Lemmy casts a local Like by the post author but never announces it, so live-bridged posts read one upvote lower than Lemmy's UI until a backfill re-seed. Accepted drift; @@ -213,19 +222,33 @@ - `ENVIRONMENT=production` has never been end-to-end tested (the harness runs development mode for migrations-on-start, strict validation, http scheme, private fetch). - Strict lexicon validation is dev/test-only; production logs-and-writes. - Wire a metric on validation failures and consider a strict-first rollout - (task 05 note). + ~~Wire a metric on validation failures~~ **(task 11: + `tidepool_lexicon_validation_failures` expvar counter, served on the + bearer-protected `GET /admin/metrics`)**; the strict-first rollout stays + deferred — that counter sitting at zero in production is its + precondition. ## Sync surface (task 04 notes) -- **No `#account{active:false}` frame on consent revocation** — subscribers - currently rely on the scrub delete-commits; tombstoned actors' historical - events stay replayable until retention expires. -- No connection cap / per-IP rate limit on the public sync surface - (pre-internet-facing hardening). +- ~~No `#account{active:false}` frame on consent revocation.~~ **Closed by + task 11** (see the Relay pipeline entry: durable-log `account` event + kind, migration 011, emitted by `DeleteActor`, status `deleted` — which + also became the bridge's own `getRepoStatus`/`listRepos` status token, + replacing `deactivated`). Tombstoned actors' HISTORICAL events still + replay until retention expires — deliberate: replay windows are + advertised complete, and the trailing #account frame is the purge signal. +- ~~No connection cap / per-IP rate limit on the public sync surface.~~ + **Closed by task 11**: per-client-IP token bucket over every + `com.atproto.sync.*` endpoint (`SYNC_RATE_PER_SECOND`/`SYNC_RATE_BURST`, + 429 `RateLimitExceeded`; `_health` exempt — container healthchecks must + not flap) + a concurrent `subscribeRepos` connection cap + (`SYNC_MAX_SUBSCRIBERS`, reserve-then-check, 429 + `SubscriberLimitExceeded`). - `getRepo` buffers full CARs in memory; `ExportCAR` includes unreachable historical blocks (consider reachable-set-only). -- `PruneEvents` is one unbatched DELETE per hourly sweep. +- ~~`PruneEvents` is one unbatched DELETE per hourly sweep.~~ **Closed by + task 11**: batched (1000/statement) from the oldest seq up, so a partial + sweep still leaves a contiguous retained suffix. - MST loads are full-tree (one SELECT per node) → `PutRecord` is O(repo size); needs a per-DID tree cache before big-community scale. - `SigningKeys` could become a `SignCommit` capability (keeps key plaintext @@ -238,28 +261,54 @@ (`internal/sync/server.go`). ## Ingestion (task 06 notes) -- **No per-signer/per-IP rate limit on `/inbox`** — queue-flood DoS via - many self-signed identities remains the top hardening item. -- `ap_tombstones` grows unbounded (no pruner — mirror `FIREHOSE_RETENTION` - treatment). -- A `Delete` arriving before its object was ever materialized leaves - nothing to tombstone → a later `Create` still materializes (needs dedup / - tombstone-of-unseen-ids). +- ~~No per-signer/per-IP rate limit on `/inbox`.~~ **Closed by task 11**: + two token-bucket layers (shared `internal/ratelimit`, the votes-limiter + discipline — sweep throttle, 50k fail-closed cap): per client IP before + the body is read, per verified signer after verification, plus a + DEDICATED much tighter per-IP cap on the `tombstonedSelfDelete` + confirmation branch (checked after the shape checks, BEFORE the outbound + confirmation fetch — the unauthenticated fetch+2-durable-writes + amplification task 10 flagged). All refusals are **503, never 4xx**: + Lemmy's federation crate retries server errors but drops 4xx permanently, + and rate-limited legitimate deliveries must delay, not vanish. Config + `INBOX_IP_RATE_PER_SECOND`/`INBOX_SIGNER_RATE_PER_SECOND`/ + `INBOX_TOMBSTONE_CONFIRMS_PER_MINUTE` (+ bursts). +- Every in-process per-IP limiter (inbox, the tombstone-confirmation cap, and + the `com.atproto.sync.*` bucket) keys on `RemoteAddr` and IGNORES + `X-Forwarded-For` — a deliberate non-goal (XFF is spoofable without a + trusted-proxy allowlist). A proxied/LB deployment MUST rate-limit at the + edge (see the README operations note), or every client shares the proxy's + one bucket and the tombstone-confirmation cap becomes GLOBAL. Clean future + fix if a real deployment needs it: an opt-in `TRUSTED_PROXY` config that + parses `X-Forwarded-For` only from an allowlisted hop; RemoteAddr-only stays + the safe default. +- ~~`ap_tombstones` grows unbounded.~~ **Closed by task 11**: batched + pruner (`TOMBSTONE_RETENTION`, default 30d, shared `internal/prune` + runner — fail-closed on non-positive retention). Accepted trade-off: a + pruned marker re-opens delete-before-create for that id, but redelivery + horizons are hours-to-days. +- ~~A `Delete` arriving before its object was ever materialized leaves + nothing to tombstone → a later `Create` still materializes.~~ **Stale — + closed by task 06 itself and verified in task 11**: `handleDelete` + records the `ap_tombstones` marker BEFORE `HandleDelete`, and + `materializeContent` checks it (`TestCreateAfterDeleteTombstone` pins the + whole ordering, restore included). The README's claim was correct; this + entry was the false doc. - `ClaimNext` does an O(N) row scan when one community's queue backs up behind a failing event (per-key serialization cost; revisit at scale). - A shutdown-interrupted attempt still consumes its ClaimNext attempt increment (cosmetic). -- `MAX_BLOB_BYTES` above 5 MiB is a silent no-op (clamped by the AP - client's fixed `maxResponseBytes`). +- ~~`MAX_BLOB_BYTES` above 5 MiB is a silent no-op.~~ **Closed by task + 11**: the AP client grew a dedicated media cap + (`ClientOptions.MaxMediaBytes`, wired from `MAX_BLOB_BYTES`) so raising + the blob budget does not also raise the JSON-object response cap. - Mint-gate ("retry via queue backoff") is verified at unit level only — the harness never drives minting into the rate limiter (a low `MINT_RATE_PER_MINUTE` stack variant would need its own compose profile). - The `activityID` rand-failure path is guarded but unit-untestable (Go 1.24+ makes a `crypto/rand` failure a fatal crash, not a returnable error) — permanent test gap unless the reader is injected. -- `ingest.NewNoopVotes` (`internal/ingest/votes.go`) is dead code: task 07 - wired the real `votes.NewAggregator` in main.go and nothing (prod or - test) references the noop anymore — delete candidate. +- ~~`ingest.NewNoopVotes` is dead code.~~ **Deleted in task 11.** ## Votes (task 07 notes) @@ -272,10 +321,22 @@ original id-targeted-only retraction therefore made EVERY Lemmy vote-clear a silent no-op; caught by the e2e retract-to-zero assertion and fixed in `internal/votes/aggregator.go` (id-targeted update first, known-id replay guard, then direction-agnostic live-vote fallback). -- `vote_events` grows unbounded (no pruning of superseded/undone rows). -- Actor-delete / consent revocation does **not** scrub that actor's - `vote_events` rows (inconsistent with the scrub posture elsewhere; counts - are anonymous on the wire, so exposure is low). +- ~~`vote_events` grows unbounded (no pruning of superseded/undone rows).~~ + **Closed by task 11**: batched pruner over UNDONE rows only + (`VOTE_EVENT_RETENTION`, default 90d) — live rows are the counts and are + never pruned. Accepted trade-off (documented on `PruneUndoneEvents`): an + undone row is also its activity id's dedupe record, so replay protection + now has the retention as its horizon. +- ~~Actor-delete / consent revocation does **not** scrub that actor's + `vote_events` rows.~~ **Closed by task 11**: `votes.Aggregator.ScrubVoter` + (deletes ALL of the voter's rows, recomputes every affected aggregate + under ordered row locks; seeded baselines untouched), called from both + `DeleteActor` and the reversible nobridge `SuppressActor` via the + materializer's `VoteScrubber` hook. Residual: `ScrubVoter` takes the + affected aggregate row locks in a deterministic subject order to avoid + deadlock across the multiple subjects one voter touched, but that lock + ordering has no concurrency test — deterministic deadlock tests are flaky, + so it stays asserted by construction / at unit level only. - ~~No true concurrency stress on the aggregate-row locking claim — a many-voters-one-post hammer is still missing.~~ **Closed by task 10**: `TestVoteHammer_ConcurrentVotersExactAggregates` fires ten real Lemmy @@ -286,8 +347,11 @@ per community ordering key, so this proves burst correctness end-to-end rather than true same-row lock contention (which stays unit-level). - Subject resolution happens outside the mutation tx (narrow TOCTOU with a racing Delete, documented in task 07). -- No upper sanity cap on seeded counts; comment count seeding skipped - (per-comment API calls would triple backfill egress). +- ~~No upper sanity cap on seeded counts~~ **(task 11: + `votes.MaxSeededCount` = 1,000,000 — a hostile origin's absurd baseline + is a validation error the seeder logs; the previous baseline survives)**; + comment count seeding stays skipped (per-comment API calls would triple + backfill egress). - Baseline-voter drift: a voter counted only in the seeded baseline who later flips federates a bare Dislike (no Undo), leaving the baseline upvote next to the new live downvote; a clear sends an Undo the live-vote @@ -295,12 +359,28 @@ fallback cannot act on (no live row). Counts stale until re-seed. ## Materializer (task 05 notes) -- Transient media-fetch failure on profile refresh drops existing blobs - (no carry-forward); a stale actor behind a 403-ing instance drops content - instead of serving stale. -- `commitRecord`'s PutRecord→PutMapping is not one tx (self-heals on retry; - a Delete landing in the crash window logs Warn). -- `DeleteActor` scrubs records but not blobs stored under community DIDs. +- ~~Transient media-fetch failure on profile refresh drops existing blobs + (no carry-forward).~~ **Closed by task 11**: a failed avatar/banner fetch + while the actor still ADVERTISES the image carries the stored blob + forward; a removed image still drops it. The related "stale actor behind + a 403-ing instance drops content instead of serving stale" remains open + (that is the actor DOCUMENT, not its media). +- ~~`commitRecord`'s PutRecord→PutMapping is not one tx.~~ **Closed by + task 11**: `repo.PutRecordTx` runs a `TxSideEffect` inside the commit + transaction (also on the idempotent NoOp path) and the materializer + writes the mapping through `store.PutMappingTx` there — record and + mapping land together or not at all, deterministic-rkey collisions now + roll the record back too. +- ~~`DeleteActor` scrubs records but not blobs stored under community + DIDs.~~ **Closed by task 11**: the scrub reads each record before its + delete commit and deletes the blobs it referenced (`atdata.ExtractBlobs` + — post thumbnails/images under COMMUNITY DIDs included); `DeleteActor` + additionally drops every remaining blob under the actor's own terminally + frozen DID. Accepted edge (commented on `repo.DeleteBlob`): blobs are + content-addressed per DID (`blobs` PK `(did, cid)`), so two records in one + repo embedding byte-identical media share a single blob row, and scrubbing + one record can orphan the other's image. Accepted at bridge scale; a blob + refcount / junction table is the fix if this ever bites. - ~~Test gap: `embed.images` arm + nsfw label shapes never appear on the wire (needs pictrs-backed image upload in the harness).~~ **Closed by task 10**: `TestImagePost_EmbedImagesAndNSFWLabel` uploads through @@ -315,8 +395,10 @@ single-writer v1). ## Storage / housekeeping -- `service_keys.private_key_pem` column name lies for the "plc-rotation" - row (it holds sealed ciphertext) — rename candidate (task 03 note). +- ~~`service_keys.private_key_pem` column name lies for the "plc-rotation" + row.~~ **Closed by task 11**: renamed to `key_material` (migration 013; + the per-row encoding — plaintext PEM vs sealed ciphertext — is documented + on `store.ServiceKey`). - `blocks` is append-only with no GC (load-bearing for GetRecord read consistency; revisit together with the getRepo memory item). diff --git a/LOOP_STATE.md b/LOOP_STATE.md --- a/LOOP_STATE.md +++ b/LOOP_STATE.md @@ -33,8 +33,8 @@ | # | Task | Status | Commit | Notes | |---|------|--------|--------|-------| | 10 | 09-e2e-relay | done | (see git log) | 7/7 reviewers (4 Claude emulated + codex/gemini/glm); fixes: dev requestCrawl PUBLIC-relay dial guard (codex unique catch — NewPrivateOnlyHTTPClient, inverse SSRF guard), terminal-error classification made pre-flight-only (whole-chain IsValidation was abandoning a relay on attempt 1 for transient DNS), 10s per-attempt timeout (budget arithmetic was 14min worst-case, not 2min), vacuous validation-no-retry test rewritten + 400-is-retried pin, vetEvent per-DID rev-monotonicity (restores per-repo ordering assertion suite-wide), drain() returns+clears pending (closes task-10 vacuous-pass trap), relay poll robustness + pagination cap, doc corrections (RESOLVE_ADDRESS overstatement, spec BGS_CRAWL_INSECURE_WS annotation, FOLLOWUPS 16th-failure off-by-one). KEPT DELIBERATE over 3 reviewers' objection: all wire errors incl. 4xx retried — bigsky answers the describeServer callback race with HTTP 400 (comment + test pin it). Final clean make e2e: 10/10, 96.7s | | 11 | 10-e2e-scenarios | done | (see git log) | 6/7 reviewers (glm watchdog-killed); UNANIMOUS 6/6 finding: tombstone confirm-fetch transient failure → definitive 401 permanently lost legitimate account deletions → fixed with three-way taxonomy (tombstone→202, alive/validation/404→401, transport/5xx→503 defer) + test; codex unique: confirmation fetch followed cross-authority redirects (open-redirect → forged 410) → FetchActorSameAuthority pins every hop; security: unauthenticated durable-write path flagged → encoded into task 11 rate-limit spec; also: zz-sweep replay floor + honest bounds (sentinel-only pass was vacuous), Delete(Actor) over-scrub drain, actor!=object + Announce{Delete} 401 pins, GET / route-level test, cursor 0→1 doc fixes, vote-hammer header de-overclaimed. TASK ITSELF: 7 scenarios + 2 PRODUCTION fixes (apex instance actor — Lemmy silently never delivers Delete{Person} without a Site actor row; tombstone-verified self-delete acceptance). Final clean make e2e: 17/17, 239s | -| 12 | 11-hardening | in-progress | | inbox+sync rate limits, #account frame, delete-before-create, pruners, housekeeping | -| 13 | 12-perf-scale | pending | | MST cache, getRepo streaming/reachable-set, blocks GC, ClaimNext scan | +| 12 | 11-hardening | done | (see git log) | 6/7 reviewers (glm watchdog-killed on 5k-line diff); NO high-sev confirmed (gemini's "carry-forward type assertion always fails" was a FALSE POSITIVE — GetRecord returns typed atdata.Blob, test green). Fixes: FollowRetrier atomic UPDATE...RETURNING claim (list-then-update raced Accept + burned attempts on transient failure + silent exhaustion), rate-limit refusal observability (expvar counters + sampled Warn — mistuned limit silently dropped all traffic), community-DID blob orphan now retryable (was swallowed → served forever; required delete-before-soft-delete reorder), ScrubVoter DELETE...RETURNING recompute (phantom-count lost update), carry-forward drops on permanent 404/410 vs carries on transient, DeleteActor terminal-state fixpoint (no double #account), migration-011 CHECK tightened + raw-insert test, /admin/metrics scoped expvar, internal/prune fail-closed test, proxy XFF ops note. TASK: inbox+sync admission control, #account{active:false,status:deleted} frame verified purging repo from bigsky, follow auto-retry, 3 pruners, one-tx record+mapping, blob/vote scrubs, service_keys rename. delete-before-create: README was RIGHT, FOLLOWUPS stale (task 06 already closed it). Final clean make e2e: 17/17, 232s | +| 13 | 12-perf-scale | in-progress | | MST cache, getRepo streaming/reachable-set, blocks GC, ClaimNext scan | Statuses: pending → in-progress → review → done (or blocked: ). @@ -524,3 +524,77 @@ TestDeleteActor pins "relay still lists tombstoned repo" — task 11's #account frame must FLIP that assertion. - Task-10 stretch (low MINT_RATE_PER_MINUTE compose variant) skipped as specced — needs its own compose profile (FOLLOWUPS, Ingestion). + +### From task 11 (hardening — task 12 MUST know) +- **firehose_events now carries TWO event kinds** (migration 011): `commit` + and `account` (nullable commit columns + a shape CHECK per kind). + repo.Event grew Kind/AccountActive/AccountStatus; any code iterating + events must switch on Kind (sync/subscribe.go does; e2e vetEvent skips + non-commit Jetstream kinds already). `repo.AppendAccountEvent` takes the + SAME global commit advisory lock as record commits — seq order still == + visibility order; task 12's perf work must preserve that for both kinds. +- **#account status token is "deleted", and so is the bridge's own + getRepoStatus/listRepos status** (was "deactivated"). Wire facts + (source-verified, pinned bigsky): on #account it re-resolves the DID doc + and REQUIRES the sender to be the DID's authoritative PDS; "deleted" → + tombstoned=true (listRepos filters `NOT tombstoned`) + carstore purge; + "deactivated"/"suspended"/"takendown" filter from listRepos but do NOT + purge. Emitted ONLY by DeleteActor (terminal); nobridge stays frameless + (repo active, reversible). +- **repo.PutRecordTx(… TxSideEffect)** is the new atomic seam: the hook + runs inside the commit tx (also on the NoOp re-put, with res.NoOp=true) + and an error rolls the RECORD back too. The materializer's ap_objects + mapping now rides it (store.APObjects.PutMappingTx). Hooks run under the + global advisory lock — keep them tiny; task 12's MST cache must not + change hook semantics. +- **internal/ratelimit** is the shared keyed token-bucket (extracted from + votes; sweep throttle + 50k fail-closed cap). Consumers: votes XRPC + (429), sync surface (429, _health exempt; subscribeRepos also has a + reserve-then-check connection cap), inbox (per-IP pre-body + per-signer + post-verify + a dedicated tombstone-confirmation cap INSIDE + tombstonedSelfDelete, before the confirmation fetch). INBOX REFUSALS ARE + 503, NEVER 4xx — Lemmy's federation crate retries 5xx but permanently + drops 4xx; the tombstone-cap refusal is a DEFER for the same reason. + Defaults are deliberately generous (suite = canary, ran green on + defaults); all envs in README's config table. +- **internal/prune.Run** is the shared retention loop (fail-closed on + retention<=0). Pruners: firehose (batched now, 1000/statement, oldest-up + so a partial sweep keeps the retained suffix contiguous), ap_tombstones + (30d), undone vote_events (90d — an undone row is also its activity id's + dedupe record, so replay protection now has that horizon; live rows + never pruned). +- **Blob scrub caveat**: blobs are content-addressed per (did,cid) with no + reference tracking — scrubbing a deleted actor's record deletes blob rows + other records in the same repo could share (byte-identical media). + Accepted + commented on repo.DeleteBlob; a blob-refcount would close it + if it ever matters. repo grew DeleteBlob/DeleteBlobsForDID. +- **votes.Aggregator.ScrubVoter** locks ALL affected aggregates in + deterministic subject order (ORDER BY + FOR UPDATE) before deleting — + any future multi-subject vote mutation must lock in the same order or + risk deadlock against it. +- **Follow retrier** (ingest.FollowRetrier, migration 012): every + set-to-pending stamps follow_requested_at + increments follow_attempts + (SetFollowState does it); resend consumes the attempt BEFORE sending so + a hanging peer can't get an unbounded budget; none resets. Default: + pending >2m → resend, 5 total sends, 1m sweep. +- **service_keys.private_key_pem → key_material** (migration 013); + store.ServiceKey.PrivateKeyPEM → KeyMaterial (identity/ap callers + updated). +- **ap.ClientOptions.MaxMediaBytes**: FetchMedia's outer clamp, wired from + MAX_BLOB_BYTES; the JSON-object cap (MaxResponseBytes) stays independent + at 5MiB. +- **Delete-before-create verdict**: README was RIGHT, FOLLOWUPS was stale — + task 06's handleDelete records the ap_tombstones marker before + HandleDelete and materializeContent checks it; + TestCreateAfterDeleteTombstone pins the whole ordering. No code change + needed; the false FOLLOWUPS entry is annotated. +- **Validation-failure metric**: materialize.ValidationFailures (expvar, + "tidepool_lexicon_validation_failures", counted in strict AND + log-and-write modes), served on bearer-protected GET /admin/metrics. + Strict-first production rollout still deferred; this counter is its + precondition. +- e2e: TestDeleteActor's task-09/10 pins FLIPPED — it now asserts the + #account frame on the bridge's own firehose (raw CBOR ws helper + dialBridgeFirehose/readBridgeAccountFrame in tests/e2e/helpers.go) and + the repo DISAPPEARING from the relay's listRepos (polled; bigsky + processes the frame async). diff --git a/README.md b/README.md --- a/README.md +++ b/README.md @@ -150,10 +150,12 @@ commits while the repo stays active (reversible; discovery rides the `PROFILE_REFRESH_TTL` re-fetch because Lemmy 0.19 never federates `Update{Person}` on bio edits); **`Delete(Actor)`** — account deletion scrubs the author's post/comment/profile and terminally tombstones the repo -(`getRepoStatus`/`listRepos` report `active:false`, the handle stops -resolving, content endpoints refuse — asserted on the bridge's sync surface -because the relay cannot observe tombstones until task 11's `#account` -frame); **unsubscribe** — `DELETE /admin/communities` sends `Undo{Follow}` +(`getRepoStatus`/`listRepos` report `active:false` status `deleted`, the +handle stops resolving, content endpoints refuse), the +`#account{active:false, status:"deleted"}` frame is asserted on the +bridge's own firehose, and the repo **disappears from the relay's +`listRepos`** (bigsky consumes the frame, tombstones the account, and +purges its data); **unsubscribe** — `DELETE /admin/communities` sends `Undo{Follow}` and new posts in that community produce no bridge output while a still-subscribed control keeps flowing; a **community profile update** federating as an `Announce{Update{Group}}` → `community.profile` update on @@ -206,6 +208,13 @@ | `BACKFILL_MAX_POSTS` | `100` | posts materialized per community backfill run | | `MINT_RATE_PER_MINUTE` / `MINT_BURST` | `60` / `120` | rate gate on inbound DID minting (PLC registrations are forever; unseen authors in delivered content trigger mints) | | `INGEST_WORKERS` | `4` | inbox queue worker-pool size | | `SEED_COUNTS_FROM_API` | on | seed backfilled posts' vote aggregates from the origin instance's public API (`/api/v3/post` `counts`); set `0` to disable | +| `TOMBSTONE_RETENTION` | `720h` | how long `ap_tombstones` markers (the delete-before-create guard) are kept before the hourly pruner reclaims them | +| `VOTE_EVENT_RETENTION` | `2160h` | how long **undone** (superseded/retracted) `vote_events` rows are kept; live rows are the counts and are never pruned | +| `INBOX_IP_RATE_PER_SECOND` / `INBOX_IP_RATE_BURST` | `50` / `200` | per-client-IP token bucket on `POST /inbox` (refusals are 503 — retryable for federation queues) | +| `INBOX_SIGNER_RATE_PER_SECOND` / `INBOX_SIGNER_RATE_BURST` | `20` / `100` | per-verified-signer token bucket on `POST /inbox` | +| `INBOX_TOMBSTONE_CONFIRMS_PER_MINUTE` / `INBOX_TOMBSTONE_CONFIRM_BURST` | `6` / `10` | dedicated per-IP cap on the tombstoned-self-delete confirmation branch (an unauthenticated POST that costs an outbound fetch + durable writes); over-limit deliveries defer (503) so legitimate deletions redeliver | +| `SYNC_RATE_PER_SECOND` / `SYNC_RATE_BURST` | `25` / `200` | per-client-IP token bucket over the public `com.atproto.sync.*` surface (429; `_health` exempt) | +| `SYNC_MAX_SUBSCRIBERS` | `100` | concurrent `subscribeRepos` connection cap | ## Subscribing to communities (admin API) @@ -220,6 +229,10 @@ -d '{"community":"!technology@lemmy.world"}' # state is `pending` until the community's Accept arrives at /inbox, which # flips it to `accepted` and triggers an outbox backfill automatically. +# A subscription stuck in `pending` (Lemmy usually skips the Accept for the +# very FIRST Follow to a new peer — its federation cursor starts at "now") +# is retried automatically: the bridge re-sends the Follow with a fresh +# activity id after 2 minutes, up to 5 total sends. curl localhost:8091/admin/communities \ -H "Authorization: Bearer dev-admin-token" # list @@ -229,6 +242,11 @@ -d '{"community":"!technology@lemmy.world"}' # on-demand backfill curl -X DELETE localhost:8091/admin/communities \ -H "Authorization: Bearer dev-admin-token" \ -d '{"community":"!technology@lemmy.world"}' # Undo{Follow} + +curl localhost:8091/admin/metrics \ + -H "Authorization: Bearer dev-admin-token" # expvar counters +# (includes tidepool_lexicon_validation_failures — non-zero in production, +# where validation failures log-and-write, means investigate) ``` The bridge's AP face lives next to the inbox: the service actor document at @@ -251,12 +269,17 @@ **`#nobot`** is never bridged: no DID is minted, and every post or comment they author is dropped with the reason logged. - If a **previously bridged** actor adds the marker (seen on a profile `Update` or any profile re-fetch), every record they authored is deleted - from the bridged repos and new materialization stops. This state is - **reversible**: removing the marker upstream restores bridging on the next - profile refresh. -- **`Delete(Actor)`** (account deletion upstream) scrubs all their records - and tombstones the bridged repo **terminally**; the sync surface reports - the repo `active: false`. + from the bridged repos (with the blobs those records referenced and their + `vote_events` rows) and new materialization stops. This state is + **reversible**: removing the marker upstream restores bridging — records, + media, and future votes — on the next profile refresh; no `#account` + frame is emitted and the repo stays active. +- **`Delete(Actor)`** (account deletion upstream) scrubs all their records, + the blobs those records referenced (post images live under COMMUNITY + repos), and their `vote_events` rows, then tombstones the bridged repo + **terminally**: the sync surface reports `active: false` (status + `deleted`) and an `#account{active:false, status:"deleted"}` frame goes + out on the firehose so relays purge the repo too. - Object-level `Delete`s tombstone the mapped record; a `Delete` arriving before its object was ever seen is remembered (`ap_tombstones`), so an out-of-order or re-delivered `Create` cannot resurrect deleted content. @@ -279,14 +302,41 @@ - `GET /xrpc/com.atproto.sync.subscribeRepos` (WebSocket; `?cursor=N` replays from the durable event log, then tails live; slow consumers are evicted and resume by reconnecting with their last cursor; a cursor older than - retention gets an `#info OutdatedCursor` frame first) + retention gets an `#info OutdatedCursor` frame first). The stream carries + `#commit` frames and — on `Delete(Actor)`/consent revocation — + **`#account {active:false, status:"deleted"}`** frames, appended to the + same durable log after the scrub delete-commits so relays purge the repo + instead of inferring its death (bigsky tombstones the account and drops + it from its `listRepos` on that frame). `#nobridge` suppression is + reversible and deliberately emits no `#account` frame. - `getRepo` (full CAR), `getLatestCommit`, `getRecord` (proof CAR), `listRepos` (paginated), `getRepoStatus` - `com.atproto.server.describeServer`, `/xrpc/_health` - `com.atproto.identity.resolveHandle` + `/.well-known/atproto-did` (task 03) Repos whose actor revoked consent (tombstoned) report `RepoDeactivated` / -`active: false` and their content endpoints stop serving. +`active: false` with status `deleted`, and their content endpoints stop +serving. + +The whole surface sits behind admission control (task 11): a per-client-IP +token bucket (`SYNC_RATE_PER_SECOND`/`SYNC_RATE_BURST`, 429 +`RateLimitExceeded`; `/xrpc/_health` is exempt so container healthchecks +never flap) and a concurrent-subscriber cap on `subscribeRepos` +(`SYNC_MAX_SUBSCRIBERS`, 429 `SubscriberLimitExceeded`). `POST /inbox` has +its own two-layer limiter — per client IP and per verified signer — plus a +dedicated, much tighter per-IP cap on the tombstoned-self-delete +confirmation branch; inbox refusals are **503** (retryable), because +federation queues drop 4xx permanently but redeliver on server errors. + +**Operations note (reverse proxies / load balancers):** every in-process +per-IP limiter above keys on the connection's `RemoteAddr` and deliberately +IGNORES `X-Forwarded-For` (it is spoofable without a trusted-proxy allowlist — +a deliberate non-goal; see FOLLOWUPS.md for a possible future `TRUSTED_PROXY` +config). Any deployment that terminates TLS or load-balances in front of the +bridge therefore MUST rate-limit `POST /inbox` and the `com.atproto.sync.*` +surface at the edge: otherwise every client collapses into the proxy's single +IP bucket, and in particular the tombstoned-self-delete confirmation cap +degenerates from per-IP into a GLOBAL cap. ## Vote aggregates (the AppView integration point) diff --git a/cmd/tidepool/main.go b/cmd/tidepool/main.go --- a/cmd/tidepool/main.go +++ b/cmd/tidepool/main.go @@ -23,6 +23,7 @@ "tidepool/internal/db" "tidepool/internal/identity" "tidepool/internal/ingest" "tidepool/internal/materialize" + "tidepool/internal/prune" "tidepool/internal/repo" "tidepool/internal/store" tidepoolsync "tidepool/internal/sync" @@ -109,11 +110,14 @@ } defer func() { _ = broadcaster.Close() }() go broadcaster.Run(ctx) syncServer, err := tidepoolsync.NewServer(tidepoolsync.Options{ - Repo: repoManager, - Broadcaster: broadcaster, - Logger: logger, - Hostname: cfg.BridgeHostname, - ServiceDID: cfg.BridgeServiceDID, + Repo: repoManager, + Broadcaster: broadcaster, + Logger: logger, + Hostname: cfg.BridgeHostname, + ServiceDID: cfg.BridgeServiceDID, + RatePerSecond: float64(cfg.SyncRatePerSecond), + RateBurst: cfg.SyncRateBurst, + MaxSubscribers: cfg.SyncMaxSubscribers, }) if err != nil { return err @@ -156,8 +160,11 @@ if err != nil { return err } apClient := ap.NewClient(ap.ClientOptions{ - UserAgent: cfg.UserAgent, - Signer: serviceActor.Signer(), + UserAgent: cfg.UserAgent, + Signer: serviceActor.Signer(), + // MAX_BLOB_BYTES governs media downloads only; the JSON-object cap + // keeps its own (smaller) default. + MaxMediaBytes: cfg.MaxBlobBytes, AllowPrivateAddresses: cfg.AllowPrivateAddresses, }) @@ -200,6 +207,16 @@ communities := store.NewCommunities(database) tombstones := store.NewTombstones(database) inboxEvents := store.NewInboxEvents(database) + // The vote aggregation side channel (task 07): Like/Dislike activities + // maintain bridge-side counts (never records), served over + // social.coves.bridge.getVoteAggregates. Built before the materializer + // because the materializer's actor scrub erases a deleted voter's + // vote_events rows through it. + voteAggregator, err := votes.NewAggregator(database, objects, communities, repoManager, logger) + if err != nil { + return err + } + materializer, err := materialize.New(materialize.Options{ Fetcher: apClient, Objects: objects, @@ -207,6 +224,7 @@ Actors: actors, Communities: communities, Repos: repoManager, Minter: mintGate, + Votes: voteAggregator, ServiceDID: serviceDID, ProfileRefreshTTL: cfg.ProfileRefreshTTL, MaxBlobBytes: cfg.MaxBlobBytes, @@ -217,13 +235,10 @@ if err != nil { return err } - // The vote aggregation side channel (task 07): Like/Dislike activities - // maintain bridge-side counts (never records), served over - // social.coves.bridge.getVoteAggregates. - voteAggregator, err := votes.NewAggregator(database, objects, communities, repoManager, logger) - if err != nil { - return err - } + // Task 11 retention pruners: ap_tombstones markers and undone + // vote_events rows age out like firehose events do. + go prune.Run(ctx, "ap_tombstones", cfg.TombstoneRetention, 0, tombstones.Prune, logger) + go prune.Run(ctx, "vote_events(undone)", cfg.VoteEventRetention, 0, voteAggregator.PruneUndoneEvents, logger) // Seeding imports historical scores for backfilled posts from the origin // instance's public API (AP alone cannot provide them). var seeder ingest.CountSeeder @@ -278,17 +293,36 @@ } go queue.Run(ctx) inbox, err := ingest.NewInbox(ingest.InboxOptions{ - Verifier: ap.NewVerifier(apClient), - Events: inboxEvents, - Queue: queue, - Service: serviceActor, - Fetcher: apClient, - Logger: logger, + Verifier: ap.NewVerifier(apClient), + Events: inboxEvents, + Queue: queue, + Service: serviceActor, + Fetcher: apClient, + Logger: logger, + IPRatePerSecond: float64(cfg.InboxIPRatePerSecond), + IPRateBurst: cfg.InboxIPRateBurst, + SignerRatePerSecond: float64(cfg.InboxSignerRatePerSecond), + SignerRateBurst: cfg.InboxSignerRateBurst, + TombstoneConfirmRatePerSecond: float64(cfg.InboxTombstoneConfirmsPerMinute) / 60, + TombstoneConfirmBurst: cfg.InboxTombstoneConfirmBurst, }) if err != nil { return err } inbox.Routes(router) + + // Automatic Follow re-send for subscriptions stuck in pending (the + // Lemmy first-contact Accept race; task 11). + followRetrier, err := ingest.NewFollowRetrier(ingest.FollowRetrierOptions{ + Client: apClient, + Communities: communities, + Service: serviceActor, + Logger: logger, + }) + if err != nil { + return err + } + go followRetrier.Run(ctx) admin, err := ingest.NewAdmin(ingest.AdminOptions{ Token: cfg.AdminToken, diff --git a/internal/ap/client.go b/internal/ap/client.go --- a/internal/ap/client.go +++ b/internal/ap/client.go @@ -110,8 +110,14 @@ // HTTPClient overrides the underlying client (tests). Redirects are // handled by Client itself; any CheckRedirect on this client is // replaced. HTTPClient *http.Client - // MaxResponseBytes caps response bodies. + // MaxResponseBytes caps AP object/collection response bodies. MaxResponseBytes int64 + // MaxMediaBytes caps FetchMedia response bodies (avatars, banners, post + // images) independently of MaxResponseBytes, so raising the blob budget + // (MAX_BLOB_BYTES) does not also raise the JSON-object cap. Zero falls + // back to MaxResponseBytes' effective value — the pre-task-11 behavior, + // where the object cap silently clamped media. + MaxMediaBytes int64 // PerHostRPS / PerHostBurst configure per-host rate limiting. PerHostRPS float64 PerHostBurst int @@ -139,6 +145,7 @@ httpClient *http.Client userAgent string signer *Signer maxResponseBytes int64 + maxMediaBytes int64 perHostRPS rate.Limit perHostBurst int maxAttempts int @@ -206,6 +213,7 @@ httpClient: &httpClient, userAgent: opts.UserAgent, signer: opts.Signer, maxResponseBytes: opts.MaxResponseBytes, + maxMediaBytes: opts.MaxMediaBytes, perHostRPS: rate.Limit(opts.PerHostRPS), perHostBurst: opts.PerHostBurst, maxAttempts: opts.MaxAttempts, @@ -234,6 +242,9 @@ c.userAgent = "tidepool/0.1" } if c.maxResponseBytes <= 0 { c.maxResponseBytes = DefaultMaxResponseBytes + } + if c.maxMediaBytes <= 0 { + c.maxMediaBytes = c.maxResponseBytes } if c.perHostRPS <= 0 { c.perHostRPS = DefaultPerHostRPS @@ -458,10 +469,13 @@ // It returns the body and the response Content-Type (as sent by the server, // unparsed; empty when the server sent none). Status mapping matches // FetchObject: 404/401/403 → IsNotFound, 410 → IsTombstoned, other non-2xx // → HTTPError. Content-type policy (images only) is the caller's job — the -// transport layer cannot know which lexicon slot the bytes are for. +// transport layer cannot know which lexicon slot the bytes are for. The +// outer clamp is the MEDIA cap (ClientOptions.MaxMediaBytes, wired from +// MAX_BLOB_BYTES), not the JSON-object cap, so blob budgets above 5 MiB +// actually take effect. func (c *Client) FetchMedia(ctx context.Context, iri string, maxBytes int64) (data []byte, contentType string, err error) { - if maxBytes <= 0 || maxBytes > c.maxResponseBytes { - maxBytes = c.maxResponseBytes + if maxBytes <= 0 || maxBytes > c.maxMediaBytes { + maxBytes = c.maxMediaBytes } var lastErr error for attempt := 0; attempt < c.maxAttempts; attempt++ { diff --git a/internal/ap/service_actor.go b/internal/ap/service_actor.go --- a/internal/ap/service_actor.go +++ b/internal/ap/service_actor.go @@ -102,7 +102,7 @@ default: return nil, fmt.Errorf("ap: load service key: %w", err) } - key, err := ParsePrivateKeyPEM(stored.PrivateKeyPEM) + key, err := ParsePrivateKeyPEM(stored.KeyMaterial) if err != nil { return nil, fmt.Errorf("ap: stored service key is corrupt: %w", err) } diff --git a/internal/ap/service_actor_test.go b/internal/ap/service_actor_test.go --- a/internal/ap/service_actor_test.go +++ b/internal/ap/service_actor_test.go @@ -39,7 +39,7 @@ if _, exists := f.rows[name]; exists { return nil, errors.NewConflictError("service_key", "name", name) } f.rows[name] = pem - return &store.ServiceKey{ID: 1, Name: name, PrivateKeyPEM: pem}, nil + return &store.ServiceKey{ID: 1, Name: name, KeyMaterial: pem}, nil } func (f *fakeServiceKeys) Get(_ context.Context, name string) (*store.ServiceKey, error) { @@ -49,7 +49,7 @@ pem, ok := f.rows[name] if !ok { return nil, errors.NewNotFoundError("service_key", name) } - return &store.ServiceKey{ID: 1, Name: name, PrivateKeyPEM: pem}, nil + return &store.ServiceKey{ID: 1, Name: name, KeyMaterial: pem}, nil } func TestLoadOrCreateServiceActor_GeneratesThenLoads(t *testing.T) { @@ -66,7 +66,7 @@ // The key must have been persisted as parseable PKCS#8 PEM. stored, err := keys.Get(ctx, ServiceKeyName) require.NoError(t, err) - storedKey, err := ParsePrivateKeyPEM(stored.PrivateKeyPEM) + storedKey, err := ParsePrivateKeyPEM(stored.KeyMaterial) require.NoError(t, err) assert.True(t, first.Key.Equal(storedKey)) diff --git a/internal/config/config.go b/internal/config/config.go --- a/internal/config/config.go +++ b/internal/config/config.go @@ -121,6 +121,37 @@ // from the origin instance's public API (Lemmy's `counts` field) — // history whose individual Like activities AP never delivers // (SEED_COUNTS_FROM_API, default on; set to 0/false to disable). SeedCountsFromAPI bool + // TombstoneRetention is how long ap_tombstones markers (the + // delete-before-create guard) are kept before the pruner reclaims them + // (TOMBSTONE_RETENTION, a Go duration, default 720h = 30 days — orders + // of magnitude above any real redelivery horizon). + TombstoneRetention time.Duration + // VoteEventRetention is how long undone (superseded/retracted) + // vote_events rows are kept before pruning (VOTE_EVENT_RETENTION, a Go + // duration, default 2160h = 90 days). Live rows are never pruned; see + // votes.PruneUndoneEvents for the replay-dedupe trade-off. + VoteEventRetention time.Duration + // Inbox admission control (task 11): per-client-IP and per-verified- + // signer token buckets on POST /inbox, plus the dedicated tighter cap + // on the tombstoned-self-delete confirmation branch. All are generous + // DoS backstops; see internal/ingest inbox.go for the model. + // INBOX_IP_RATE_PER_SECOND (50) / INBOX_IP_RATE_BURST (200), + // INBOX_SIGNER_RATE_PER_SECOND (20) / INBOX_SIGNER_RATE_BURST (100), + // INBOX_TOMBSTONE_CONFIRMS_PER_MINUTE (6) / + // INBOX_TOMBSTONE_CONFIRM_BURST (10). + InboxIPRatePerSecond int + InboxIPRateBurst int + InboxSignerRatePerSecond int + InboxSignerRateBurst int + InboxTombstoneConfirmsPerMinute int + InboxTombstoneConfirmBurst int + // Public sync surface admission control: per-client-IP token bucket + // over every com.atproto.sync.* endpoint (SYNC_RATE_PER_SECOND, 25 / + // SYNC_RATE_BURST, 200) and the concurrent subscribeRepos connection + // cap (SYNC_MAX_SUBSCRIBERS, 100). + SyncRatePerSecond int + SyncRateBurst int + SyncMaxSubscribers int } // Load reads configuration from the environment. logger must not be nil; @@ -290,6 +321,56 @@ if err != nil { return nil, err } + // Retention knobs for the task-11 pruners: same semantics as + // FIREHOSE_RETENTION (real defaults everywhere, must be positive). + cfg.TombstoneRetention, err = durationVar(logger, "TOMBSTONE_RETENTION", 720*time.Hour) + if err != nil { + return nil, err + } + cfg.VoteEventRetention, err = durationVar(logger, "VOTE_EVENT_RETENTION", 2160*time.Hour) + if err != nil { + return nil, err + } + + // Admission-control knobs (task 11): tuning knobs with real defaults in + // every environment, like the other rate limits. + cfg.InboxIPRatePerSecond, err = intVar(logger, "INBOX_IP_RATE_PER_SECOND", 50) + if err != nil { + return nil, err + } + cfg.InboxIPRateBurst, err = intVar(logger, "INBOX_IP_RATE_BURST", 200) + if err != nil { + return nil, err + } + cfg.InboxSignerRatePerSecond, err = intVar(logger, "INBOX_SIGNER_RATE_PER_SECOND", 20) + if err != nil { + return nil, err + } + cfg.InboxSignerRateBurst, err = intVar(logger, "INBOX_SIGNER_RATE_BURST", 100) + if err != nil { + return nil, err + } + cfg.InboxTombstoneConfirmsPerMinute, err = intVar(logger, "INBOX_TOMBSTONE_CONFIRMS_PER_MINUTE", 6) + if err != nil { + return nil, err + } + cfg.InboxTombstoneConfirmBurst, err = intVar(logger, "INBOX_TOMBSTONE_CONFIRM_BURST", 10) + if err != nil { + return nil, err + } + cfg.SyncRatePerSecond, err = intVar(logger, "SYNC_RATE_PER_SECOND", 25) + if err != nil { + return nil, err + } + cfg.SyncRateBurst, err = intVar(logger, "SYNC_RATE_BURST", 200) + if err != nil { + return nil, err + } + cfg.SyncMaxSubscribers, err = intVar(logger, "SYNC_MAX_SUBSCRIBERS", 100) + if err != nil { + return nil, err + } + defaultUserAgent := fmt.Sprintf("tidepool/0.1 (+https://%s)", cfg.BridgeHostname) cfg.UserAgent = os.Getenv("USER_AGENT") if cfg.UserAgent == "" { @@ -324,6 +405,25 @@ if len(raw) != 32 { return nil, fmt.Errorf("config: BRIDGE_KEK must decode to 32 bytes, got %d", len(raw)) } return raw, nil +} + +// durationVar returns a positive Go-duration environment variable, falling +// back to a logged default in every environment (tuning-knob semantics, +// like FIREHOSE_RETENTION). +func durationVar(logger *slog.Logger, name string, fallback time.Duration) (time.Duration, error) { + raw := os.Getenv(name) + if raw == "" { + logger.Info(name+" not set, using default", "value", fallback.String()) + return fallback, nil + } + parsed, err := time.ParseDuration(raw) + if err != nil { + return 0, fmt.Errorf("config: %s must be a Go duration (e.g. 720h): %w", name, err) + } + if parsed <= 0 { + return 0, fmt.Errorf("config: %s must be positive, got %s", name, parsed) + } + return parsed, nil } // intVar returns a positive-integer environment variable, falling back to a diff --git a/internal/db/migrations/011_account_events.sql b/internal/db/migrations/011_account_events.sql new file mode 100644 --- /dev/null +++ b/internal/db/migrations/011_account_events.sql @@ -0,0 +1,59 @@ +-- +goose Up +-- Task 11: the firehose grows a second event kind. Until now every +-- firehose_events row was a #commit; consent revocation (Delete(Actor)) +-- now appends an #account{active:false} row through the same durable log — +-- same seq space, same advisory-lock serialization, same retention — so +-- relays learn the repo is gone instead of inferring it from scrub +-- delete-commits (bigsky only learns account state from #account frames). +-- +-- Account rows carry no commit payload, so the commit columns loosen to +-- nullable with a CHECK that keeps them REQUIRED for commit rows: the old +-- invariant is not weakened, it is scoped to the kind it belongs to. +ALTER TABLE firehose_events + ADD COLUMN kind TEXT NOT NULL DEFAULT 'commit' CHECK (kind IN ('commit', 'account')), + ADD COLUMN account_active BOOLEAN, + ADD COLUMN account_status TEXT; + +ALTER TABLE firehose_events + ALTER COLUMN commit_cid DROP NOT NULL, + ALTER COLUMN rev DROP NOT NULL, + ALTER COLUMN ops DROP NOT NULL, + ALTER COLUMN car DROP NOT NULL; + +ALTER TABLE firehose_events + DROP CONSTRAINT IF EXISTS firehose_events_commit_cid_check, + DROP CONSTRAINT IF EXISTS firehose_events_rev_check; + +ALTER TABLE firehose_events + ADD CONSTRAINT firehose_events_kind_shape_check CHECK ( + (kind = 'commit' + AND commit_cid IS NOT NULL AND commit_cid <> '' + AND rev IS NOT NULL AND rev <> '' + AND ops IS NOT NULL AND car IS NOT NULL + AND account_active IS NULL AND account_status IS NULL) + OR + (kind = 'account' + AND account_active IS NOT NULL + AND commit_cid IS NULL AND rev IS NULL AND ops IS NULL AND car IS NULL + -- status is inactive-only: the model/emitter document a status + -- (e.g. 'deleted') solely for active=false frames, so an active + -- account frame must carry a NULL status. + AND (NOT account_active OR account_status IS NULL)) + ); + +-- +goose Down +DELETE FROM firehose_events WHERE kind <> 'commit'; +ALTER TABLE firehose_events + DROP CONSTRAINT IF EXISTS firehose_events_kind_shape_check; +ALTER TABLE firehose_events + ALTER COLUMN commit_cid SET NOT NULL, + ALTER COLUMN rev SET NOT NULL, + ALTER COLUMN ops SET NOT NULL, + ALTER COLUMN car SET NOT NULL; +ALTER TABLE firehose_events + ADD CONSTRAINT firehose_events_commit_cid_check CHECK (commit_cid <> ''), + ADD CONSTRAINT firehose_events_rev_check CHECK (rev <> ''); +ALTER TABLE firehose_events + DROP COLUMN IF EXISTS account_status, + DROP COLUMN IF EXISTS account_active, + DROP COLUMN IF EXISTS kind; diff --git a/internal/db/migrations/012_follow_retry.sql b/internal/db/migrations/012_follow_retry.sql new file mode 100644 --- /dev/null +++ b/internal/db/migrations/012_follow_retry.sql @@ -0,0 +1,17 @@ +-- +goose Up +-- Task 11: automatic Follow re-send. Lemmy initializes a newly-seen +-- instance's federation cursor at the current max activity id, so the +-- Accept answering the very FIRST Follow to a given Lemmy instance is +-- usually skipped (FOLLOWUPS "Lemmy first-contact Accept race"). The +-- follow retrier re-sends the Follow (fresh activity id → fresh Accept) +-- when a subscription stays pending past a threshold; these columns track +-- when the last Follow went out and how many have been sent, bounding the +-- retries. +ALTER TABLE communities + ADD COLUMN follow_requested_at TIMESTAMPTZ, + ADD COLUMN follow_attempts INTEGER NOT NULL DEFAULT 0; + +-- +goose Down +ALTER TABLE communities + DROP COLUMN IF EXISTS follow_attempts, + DROP COLUMN IF EXISTS follow_requested_at; diff --git a/internal/db/migrations/013_rename_service_key_column.sql b/internal/db/migrations/013_rename_service_key_column.sql new file mode 100644 --- /dev/null +++ b/internal/db/migrations/013_rename_service_key_column.sql @@ -0,0 +1,10 @@ +-- +goose Up +-- Task 11 housekeeping: private_key_pem lied for the "plc-rotation" row, +-- which holds AES-GCM sealed ciphertext (task 03), not PEM. key_material is +-- honest for both rows: plaintext PKCS#8 PEM for 'service-actor', sealed +-- ciphertext for 'plc-rotation' (the per-row encoding is documented on the +-- store model). +ALTER TABLE service_keys RENAME COLUMN private_key_pem TO key_material; + +-- +goose Down +ALTER TABLE service_keys RENAME COLUMN key_material TO private_key_pem; diff --git a/internal/identity/keys.go b/internal/identity/keys.go --- a/internal/identity/keys.go +++ b/internal/identity/keys.go @@ -130,7 +130,7 @@ // re-reads the winner's key. func LoadOrCreateRotationKey(ctx context.Context, keys store.ServiceKeys, custodian *Custodian) (*atcrypto.PrivateKeyK256, error) { stored, err := keys.Get(ctx, RotationKeyName) if err == nil { - return decryptRotationKey(custodian, stored.PrivateKeyPEM) + return decryptRotationKey(custodian, stored.KeyMaterial) } if !errors.IsNotFound(err) { return nil, fmt.Errorf("identity: load rotation key: %w", err) @@ -153,7 +153,7 @@ winner, getErr := keys.Get(ctx, RotationKeyName) if getErr != nil { return nil, fmt.Errorf("identity: reload rotation key after race: %w", getErr) } - return decryptRotationKey(custodian, winner.PrivateKeyPEM) + return decryptRotationKey(custodian, winner.KeyMaterial) } return nil, fmt.Errorf("identity: persist rotation key: %w", err) } diff --git a/internal/identity/keys_test.go b/internal/identity/keys_test.go --- a/internal/identity/keys_test.go +++ b/internal/identity/keys_test.go @@ -142,7 +142,7 @@ // The stored bytes must be sealed, not the raw key. stored, err := keys.Get(ctx, RotationKeyName) require.NoError(t, err) - assert.False(t, bytes.Contains(stored.PrivateKeyPEM, first.Bytes()), + assert.False(t, bytes.Contains(stored.KeyMaterial, first.Bytes()), "rotation key must be encrypted at rest") } diff --git a/internal/ingest/follow.go b/internal/ingest/follow.go --- a/internal/ingest/follow.go +++ b/internal/ingest/follow.go @@ -5,7 +5,9 @@ "context" "crypto/rand" "encoding/hex" "encoding/json" + "expvar" "fmt" + "io" "log/slog" "net/http" "strings" @@ -93,7 +95,12 @@ logger: logger, }, nil } -// Routes mounts the admin API on a chi router. +// Routes mounts the admin API on a chi router. /admin/metrics serves ONLY +// tidepool's own expvar counters (keys prefixed "tidepool_": the lexicon +// validation-failure counter and the per-surface rate-limit refusal +// counters), bearer-protected like the rest of /admin. It deliberately does +// NOT use expvar.Handler(), which would also expose Go's global "cmdline" +// and "memstats" — process internals no operator asked to publish. func (a *Admin) Routes(r chi.Router) { r.Route("/admin", func(r chi.Router) { r.Use(requireBearer(a.token, a.logger)) @@ -101,7 +108,28 @@ r.Post("/communities", a.handleSubscribe) r.Delete("/communities", a.handleUnsubscribe) r.Get("/communities", a.handleList) r.Post("/communities/backfill", a.handleBackfill) + r.Method(http.MethodGet, "/metrics", http.HandlerFunc(scopedMetrics)) }) +} + +// scopedMetrics writes the JSON expvar map filtered to tidepool's own +// counters — the same wire format as expvar.Handler(), minus Go's global +// cmdline/memstats. +func scopedMetrics(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json; charset=utf-8") + _, _ = io.WriteString(w, "{") + first := true + expvar.Do(func(kv expvar.KeyValue) { + if !strings.HasPrefix(kv.Key, "tidepool") { + return + } + if !first { + _, _ = io.WriteString(w, ",") + } + first = false + _, _ = fmt.Fprintf(w, "%q: %s", kv.Key, kv.Value) + }) + _, _ = io.WriteString(w, "}\n") } // communityRequest is the shared request body: a Lemmy-style handle @@ -333,7 +361,15 @@ // buildFollow constructs the signed Follow activity (delivery signing // happens in the client; this is the payload Lemmy validates). func (a *Admin) buildFollow(groupIRI string) (*ap.Object, error) { - id, err := a.activityID("follow") + return buildFollowActivity(a.service, groupIRI) +} + +// buildFollowActivity is the package-level Follow constructor, shared by +// the admin subscribe path and the follow retrier. Every call mints a +// FRESH activity id — Lemmy dedupes by id, so a re-sent Follow must never +// reuse one (the whole point of the retry is provoking a fresh Accept). +func buildFollowActivity(service *ap.ServiceActor, groupIRI string) (*ap.Object, error) { + id, err := mintActivityID(service, "follow") if err != nil { return nil, err } @@ -341,7 +377,7 @@ return &ap.Object{ Context: json.RawMessage(`"https://www.w3.org/ns/activitystreams"`), ID: id, Type: ap.TypeFollow, - Actor: &ap.Object{ID: a.service.ID}, + Actor: &ap.Object{ID: service.ID}, Object: &ap.Object{ID: groupIRI}, To: ap.Audience{groupIRI}, }, nil @@ -376,11 +412,15 @@ // activity id — so the first such activity would land and every later // Follow/Undo{Follow} would be silently ignored while the admin API still // reports success. The buildFollow/buildUndoFollow callers map this to a 5xx. func (a *Admin) activityID(kind string) (string, error) { + return mintActivityID(a.service, kind) +} + +func mintActivityID(service *ap.ServiceActor, kind string) (string, error) { var buf [16]byte if _, err := rand.Read(buf[:]); err != nil { return "", fmt.Errorf("ingest: mint activity id: %w", err) } - return fmt.Sprintf("%s/activities/%s/%s", a.service.BaseURL(), kind, hex.EncodeToString(buf[:])), nil + return fmt.Sprintf("%s/activities/%s/%s", service.BaseURL(), kind, hex.EncodeToString(buf[:])), nil } // writeJSON writes a JSON response body. diff --git a/internal/ingest/hardening_test.go b/internal/ingest/hardening_test.go new file mode 100644 --- /dev/null +++ b/internal/ingest/hardening_test.go @@ -0,0 +1,340 @@ +package ingest + +import ( + "context" + "fmt" + "log/slog" + "net/http" + "os" + "path/filepath" + "strings" + "sync" + "sync/atomic" + "testing" + "time" + + "github.com/go-chi/chi/v5" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "tidepool/internal/ap" + "tidepool/internal/store" +) + +// Task 11 hardening tests: inbox admission control (per-IP, per-signer, and +// the dedicated tombstone-confirmation cap) and the follow retrier. + +// limitedInbox rebuilds the harness's inbox with shrunk admission limits +// and swaps it into a fresh router (the default limits are deliberately far +// too generous for a unit test to exhaust). +func (h *harness) limitedInbox(mutate func(*InboxOptions)) { + h.t.Helper() + opts := InboxOptions{ + Verifier: ap.NewVerifier(h.client), + Events: h.events, + Queue: h.queue, + Service: h.service, + Fetcher: h.client, + } + mutate(&opts) + inbox, err := NewInbox(opts) + require.NoError(h.t, err) + h.inbox = inbox + router := chi.NewRouter() + h.inbox.Routes(router) + h.admin.Routes(router) + h.router = router +} + +// TestInboxPerIPRateLimit: past the per-IP burst, deliveries are refused +// 503 (retryable — Lemmy redelivers; a 4xx would drop them forever) BEFORE +// signature verification, and nothing is enqueued. +func TestInboxPerIPRateLimit(t *testing.T) { + h := newHarness(t) + h.limitedInbox(func(o *InboxOptions) { + o.IPRatePerSecond = 0.001 + o.IPRateBurst = 2 + }) + alice := h.newRemoteActor("https://lemmy.world/u/ipburst", person("https://lemmy.world/u/ipburst", "ipburst", nil)) + + before := InboxRateLimited.Value() + for i := 1; i <= 2; i++ { + status := h.deliver(alice, likeActivity(fmt.Sprintf("https://lemmy.world/activities/like/ip-%d", i), alice.id)) + require.Equal(t, http.StatusAccepted, status, "delivery %d within the burst", i) + } + status := h.deliver(alice, likeActivity("https://lemmy.world/activities/like/ip-3", alice.id)) + assert.Equal(t, http.StatusServiceUnavailable, status, + "past the burst the inbox must defer with 503, not drop with 4xx") + _, err := h.events.GetEvent(context.Background(), "https://lemmy.world/activities/like/ip-3") + assert.Error(t, err, "rate-limited deliveries must not be enqueued") + assert.Greater(t, InboxRateLimited.Value(), before, + "a refusal must increment the tidepool_inbox_ratelimited counter") +} + +// TestAdminMetricsScoped: /admin/metrics exposes tidepool's own counters and +// NOT Go's global cmdline/memstats that expvar.Handler() would leak. +func TestAdminMetricsScoped(t *testing.T) { + h := newHarness(t) + InboxRateLimited.Add(1) // ensure the key is present + + rec := h.adminRequest(http.MethodGet, "/admin/metrics", nil) + require.Equal(t, http.StatusOK, rec.Code) + body := rec.Body.String() + assert.Contains(t, body, "tidepool_inbox_ratelimited") + assert.NotContains(t, body, "cmdline", "the scoped metrics endpoint must not leak Go's cmdline") + assert.NotContains(t, body, "memstats", "the scoped metrics endpoint must not leak Go's memstats") +} + +// TestInboxPerSignerRateLimit: one identity is bounded even when the IP +// bucket has room; a different signer (same IP) keeps flowing. +func TestInboxPerSignerRateLimit(t *testing.T) { + h := newHarness(t) + h.limitedInbox(func(o *InboxOptions) { + o.SignerRatePerSecond = 0.001 + o.SignerRateBurst = 2 + }) + spammer := h.newRemoteActor("https://lemmy.world/u/spammer", person("https://lemmy.world/u/spammer", "spammer", nil)) + bystander := h.newRemoteActor("https://lemmy.world/u/bystander", person("https://lemmy.world/u/bystander", "bystander", nil)) + + for i := 1; i <= 2; i++ { + status := h.deliver(spammer, likeActivity(fmt.Sprintf("https://lemmy.world/activities/like/sg-%d", i), spammer.id)) + require.Equal(t, http.StatusAccepted, status) + } + status := h.deliver(spammer, likeActivity("https://lemmy.world/activities/like/sg-3", spammer.id)) + assert.Equal(t, http.StatusServiceUnavailable, status, "the signer's budget is spent") + + status = h.deliver(bystander, likeActivity("https://lemmy.world/activities/like/sg-4", bystander.id)) + assert.Equal(t, http.StatusAccepted, status, + "a different signer on the same IP must have its own bucket") +} + +// TestInboxTombstoneConfirmationRateLimited: the dedicated per-IP cap on the +// tombstonedSelfDelete branch. Past the burst, the delivery is DEFERRED +// (503, sender redelivers — a legitimate deletion must never be dropped) +// and, critically, NO confirmation fetch goes out: the cap sits in front of +// the outbound request the branch can be farmed for. +func TestInboxTombstoneConfirmationRateLimited(t *testing.T) { + h := newHarness(t) + h.limitedInbox(func(o *InboxOptions) { + o.TombstoneConfirmRatePerSecond = 0.001 + o.TombstoneConfirmBurst = 1 + }) + const ghost = "https://lemmy.world/u/ghost-limited" + var ghostHits atomic.Int64 + h.mux.HandleFunc("GET "+urlPath(h.t, ghost), func(w http.ResponseWriter, _ *http.Request) { + ghostHits.Add(1) + w.WriteHeader(http.StatusGone) + }) + key, err := ap.GenerateRSAKey() + require.NoError(t, err) + deleted := &remoteActor{id: ghost, key: key} + + // First self-delete: within the burst — confirmed and accepted. + // Each allowed delivery costs two fetches of the actor IRI: key + // resolution during Verify, then the independent confirmation. + status := h.deliver(deleted, selfDelete("https://lemmy.world/activities/delete/lim-1", ghost)) + require.Equal(t, http.StatusAccepted, status) + hitsAfterFirst := ghostHits.Load() + require.Equal(t, int64(2), hitsAfterFirst, "an admitted self-delete costs key resolution + confirmation") + + // Second: the tombstone bucket is spent. Deferred, and only the key + // resolution fetch happened — the confirmation fetch was never spent. + status = h.deliver(deleted, selfDelete("https://lemmy.world/activities/delete/lim-2", ghost)) + assert.Equal(t, http.StatusServiceUnavailable, status, + "a rate-limited tombstone confirmation must defer, not reject") + assert.Equal(t, hitsAfterFirst+1, ghostHits.Load(), + "the confirmation fetch must not go out while rate-limited") + _, err = h.events.GetEvent(context.Background(), "https://lemmy.world/activities/delete/lim-2") + assert.Error(t, err, "deferred deliveries must not be enqueued") +} + +// TestFollowRetrierResendsStalePending: a subscription stuck in pending +// past the threshold gets a fresh Follow (new activity id) and consumes one +// attempt; an exhausted budget stops the retries. +func TestFollowRetrierResendsStalePending(t *testing.T) { + h := newHarness(t) + ctx := context.Background() + + // Subscribe but never deliver the Accept: the community sits pending + // with attempts=1 (the subscribe's own Follow send). + h.technologyGroup() + webfinger, err := os.ReadFile(filepath.Join("..", "ap", "testdata", "webfinger_group.json")) + require.NoError(t, err) + h.serveJSON("/.well-known/webfinger", webfinger) + rec := h.adminRequest(http.MethodPost, "/admin/communities", + map[string]any{"community": "!technology@lemmy.world"}) + require.Equal(t, http.StatusAccepted, rec.Code, rec.Body.String()) + + community, err := h.communities.GetByAPGroupID(ctx, groupID) + require.NoError(t, err) + require.Equal(t, store.FollowStatePending, community.FollowState) + require.Equal(t, 1, community.FollowAttempts) + h.mu.Lock() + firstFollow := string(h.inboxLog[len(h.inboxLog)-1]) + deliveries := len(h.inboxLog) + h.mu.Unlock() + + retrier, err := NewFollowRetrier(FollowRetrierOptions{ + Client: h.client, + Communities: h.communities, + Service: h.service, + ResendAfter: time.Nanosecond, // everything pending is instantly stale + MaxAttempts: 3, + }) + require.NoError(t, err) + + retrier.Sweep(ctx) + h.mu.Lock() + require.Len(t, h.inboxLog, deliveries+1, "the sweep must re-send exactly one Follow") + resent := h.inboxLog[len(h.inboxLog)-1] + h.mu.Unlock() + follow, err := ap.ParseObject(resent) + require.NoError(t, err) + assert.Equal(t, ap.TypeFollow, follow.Type) + assert.Equal(t, groupID, follow.Object.ID) + prev, err := ap.ParseObject([]byte(firstFollow)) + require.NoError(t, err) + assert.NotEqual(t, prev.ID, follow.ID, + "a re-sent Follow must mint a FRESH activity id (Lemmy dedupes by id)") + + community, err = h.communities.GetByAPGroupID(ctx, groupID) + require.NoError(t, err) + assert.Equal(t, 2, community.FollowAttempts) + + // One more sweep consumes the last attempt; after that the budget is + // exhausted and sweeps stop sending. + retrier.Sweep(ctx) + retrier.Sweep(ctx) + retrier.Sweep(ctx) + h.mu.Lock() + total := len(h.inboxLog) + h.mu.Unlock() + assert.Equal(t, deliveries+2, total, "retries stop at the attempt budget") + community, err = h.communities.GetByAPGroupID(ctx, groupID) + require.NoError(t, err) + assert.Equal(t, 3, community.FollowAttempts) + assert.Equal(t, store.FollowStatePending, community.FollowState, + "an exhausted budget leaves the state pending for the operator to see") + + // An accepted community is never touched. + require.NoError(t, h.communities.SetFollowState(ctx, groupID, store.FollowStateAccepted)) + retrier.Sweep(ctx) + h.mu.Lock() + assert.Len(t, h.inboxLog, total, "accepted subscriptions are not re-followed") + h.mu.Unlock() +} + +// captureHandler is a minimal slog.Handler recording the level and message of +// every emitted record, so a test can assert a loud (Error/Warn) log fired. +type captureHandler struct { + mu sync.Mutex + records []slog.Record +} + +func (c *captureHandler) Enabled(context.Context, slog.Level) bool { return true } +func (c *captureHandler) Handle(_ context.Context, r slog.Record) error { + c.mu.Lock() + defer c.mu.Unlock() + c.records = append(c.records, r) + return nil +} +func (c *captureHandler) WithAttrs([]slog.Attr) slog.Handler { return c } +func (c *captureHandler) WithGroup(string) slog.Handler { return c } + +func (c *captureHandler) has(level slog.Level, substr string) bool { + c.mu.Lock() + defer c.mu.Unlock() + for _, r := range c.records { + if r.Level == level && strings.Contains(r.Message, substr) { + return true + } + } + return false +} + +// TestFollowRetrierAcceptMidCycleNotClobbered pins the race the atomic claim +// closes: an Accept that flips a subscription to accepted AFTER the retrier +// claimed the row (but before it sends) must NOT be downgraded back to +// pending. resend writes no follow_state at all — the claim is the only +// state-touching step, and it only matches pending rows. +func TestFollowRetrierAcceptMidCycleNotClobbered(t *testing.T) { + h := newHarness(t) + ctx := context.Background() + + h.technologyGroup() + webfinger, err := os.ReadFile(filepath.Join("..", "ap", "testdata", "webfinger_group.json")) + require.NoError(t, err) + h.serveJSON("/.well-known/webfinger", webfinger) + rec := h.adminRequest(http.MethodPost, "/admin/communities", + map[string]any{"community": "!technology@lemmy.world"}) + require.Equal(t, http.StatusAccepted, rec.Code, rec.Body.String()) + + retrier, err := NewFollowRetrier(FollowRetrierOptions{ + Client: h.client, + Communities: h.communities, + Service: h.service, + ResendAfter: time.Nanosecond, + MaxAttempts: 5, + }) + require.NoError(t, err) + + // Claim the pending row exactly like Sweep would (attempts 1 → 2), then + // simulate the Accept landing mid-cycle before the send. + claimed, err := h.communities.ClaimStalePendingFollows(ctx, time.Now(), 5) + require.NoError(t, err) + require.Len(t, claimed, 1) + require.NoError(t, h.communities.SetFollowState(ctx, groupID, store.FollowStateAccepted)) + + h.mu.Lock() + before := len(h.inboxLog) + h.mu.Unlock() + + // resend sends the (now redundant, harmless) Follow but must not touch + // state. + retrier.resend(ctx, claimed[0]) + + h.mu.Lock() + assert.Len(t, h.inboxLog, before+1, "the claimed Follow is still delivered") + h.mu.Unlock() + community, err := h.communities.GetByAPGroupID(ctx, groupID) + require.NoError(t, err) + assert.Equal(t, store.FollowStateAccepted, community.FollowState, + "a mid-cycle Accept must not be clobbered back to pending") +} + +// TestFollowRetrierExhaustionLogsLoudly: the final budgeted send emits a loud +// Error log — the subscription will stay pending forever (no later sweep can +// claim it) and only an operator re-subscribe recovers it, so the old +// misleading "will retry" must be gone. +func TestFollowRetrierExhaustionLogsLoudly(t *testing.T) { + h := newHarness(t) + ctx := context.Background() + + h.technologyGroup() + webfinger, err := os.ReadFile(filepath.Join("..", "ap", "testdata", "webfinger_group.json")) + require.NoError(t, err) + h.serveJSON("/.well-known/webfinger", webfinger) + rec := h.adminRequest(http.MethodPost, "/admin/communities", + map[string]any{"community": "!technology@lemmy.world"}) + require.Equal(t, http.StatusAccepted, rec.Code, rec.Body.String()) + + capture := &captureHandler{} + retrier, err := NewFollowRetrier(FollowRetrierOptions{ + Client: h.client, + Communities: h.communities, + Service: h.service, + ResendAfter: time.Nanosecond, + MaxAttempts: 2, // subscribe already burned attempt 1; the next send is final + Logger: slog.New(capture), + }) + require.NoError(t, err) + + retrier.Sweep(ctx) + + assert.True(t, capture.has(slog.LevelError, "final Follow sent"), + "the exhausting send must log a loud Error, not a misleading retry") + community, err := h.communities.GetByAPGroupID(ctx, groupID) + require.NoError(t, err) + assert.Equal(t, 2, community.FollowAttempts) + assert.Equal(t, store.FollowStatePending, community.FollowState) +} diff --git a/internal/ingest/inbox.go b/internal/ingest/inbox.go --- a/internal/ingest/inbox.go +++ b/internal/ingest/inbox.go @@ -9,22 +9,71 @@ import ( "context" "crypto/subtle" "encoding/json" + "expvar" "fmt" "io" "log/slog" "net/http" + "time" "github.com/go-chi/chi/v5" "tidepool/internal/ap" "tidepool/internal/errors" + "tidepool/internal/ratelimit" "tidepool/internal/store" ) +// InboxRateLimited counts inbox deliveries refused by admission control — +// the per-IP and per-signer token buckets and the tombstone-confirmation +// cap. Published to the admin /metrics surface as +// "tidepool_inbox_ratelimited" (mirrors materialize.ValidationFailures): a +// misconfigured tight limit that silently drops all traffic is otherwise +// invisible, since refusals are sampled in the logs. Every refusal +// increments it. +var InboxRateLimited = expvar.NewInt("tidepool_inbox_ratelimited") + +// refusalLogInterval throttles the (otherwise noisy) refusal Warn logs: a +// flood must be visible without drowning the log, and the expvar counter +// carries the true rate. +const refusalLogInterval = time.Second + // maxInboxBodyBytes caps inbound activity payloads. Lemmy activities are a // few KB; anything approaching a megabyte is abuse. const maxInboxBodyBytes = 1 << 20 +// Inbox admission-control defaults (task 11; config INBOX_* overrides). +// Two token-bucket layers guard the queue-flood DoS: +// +// - per client IP, checked before anything else — bounds total ingress +// per host however many self-signed identities it mints; +// - per verified signer, checked after signature verification — bounds +// one identity spraying through many addresses. +// +// Both are DoS backstops, not fairness controls, so the defaults are +// generous: a real Lemmy instance delivers sequentially per peer (its +// federation worker awaits each POST), which tops out far below these +// rates even on a LAN. +const ( + defaultInboxIPRatePerSecond = 50 + defaultInboxIPRateBurst = 200 + + defaultInboxSignerRatePerSecond = 20 + defaultInboxSignerRateBurst = 100 + + // The tombstonedSelfDelete branch gets a DEDICATED, much tighter per-IP + // cap: it is an UNAUTHENTICATED POST that costs the bridge an outbound + // confirmation fetch and — when the claimed origin answers 410, which + // any cheap attacker-run endpoint can — two durable writes. Legitimate + // origins deliver account deletions rarely (humans deleting accounts), + // so ~6/min sustained with a small burst is far above real traffic + // while starving an amplification loop. Rate-limited confirmations + // DEFER (503) rather than reject: a legitimate deletion behind the + // limiter is redelivered by the sender's queue, never dropped. + defaultTombstoneConfirmRatePerSecond = 0.1 + defaultTombstoneConfirmBurst = 10 +) + // softwareName is what nodeinfo reports; Lemmy admins allowlist by this // name. const ( @@ -58,6 +107,15 @@ // deleted-actor Delete path (Lemmy signs account deletions with a key // whose actor document it already serves as 410 Gone). Fetcher ActorFetcher Logger *slog.Logger + + // Admission-control tuning; zero values take the defaults above + // (tests shrink them). See the default constants for the model. + IPRatePerSecond float64 + IPRateBurst int + SignerRatePerSecond float64 + SignerRateBurst int + TombstoneConfirmRatePerSecond float64 + TombstoneConfirmBurst int } // Inbox is the HTTP face of ingestion: POST /inbox (+ the actor inbox @@ -69,6 +127,11 @@ queue *Queue service *ap.ServiceActor fetcher ActorFetcher logger *slog.Logger + + ipLimiter *ratelimit.Limiter + signerLimiter *ratelimit.Limiter + tombstoneLimiter *ratelimit.Limiter + refusalLog *ratelimit.Sampler } // NewInbox validates options and builds the Inbox. @@ -92,6 +155,15 @@ logger := opts.Logger if logger == nil { logger = slog.Default() } + limiterOf := func(rate float64, fallbackRate float64, burst, fallbackBurst int) *ratelimit.Limiter { + if rate <= 0 { + rate = fallbackRate + } + if burst <= 0 { + burst = fallbackBurst + } + return ratelimit.New(rate, burst) + } return &Inbox{ verifier: opts.Verifier, events: opts.Events, @@ -99,6 +171,13 @@ queue: opts.Queue, service: opts.Service, fetcher: opts.Fetcher, logger: logger, + ipLimiter: limiterOf(opts.IPRatePerSecond, defaultInboxIPRatePerSecond, + opts.IPRateBurst, defaultInboxIPRateBurst), + signerLimiter: limiterOf(opts.SignerRatePerSecond, defaultInboxSignerRatePerSecond, + opts.SignerRateBurst, defaultInboxSignerRateBurst), + tombstoneLimiter: limiterOf(opts.TombstoneConfirmRatePerSecond, defaultTombstoneConfirmRatePerSecond, + opts.TombstoneConfirmBurst, defaultTombstoneConfirmBurst), + refusalLog: ratelimit.NewSampler(refusalLogInterval), }, nil } @@ -125,6 +204,21 @@ // activity's actor to the signer, dedupe by activity id, enqueue for the // worker pool, 202. Everything heavier happens async — remote instances // time deliveries and treat slow inboxes as dead. func (ib *Inbox) handleInbox(w http.ResponseWriter, r *http.Request) { + // Admission layer 1, per client IP, before the body is even read: one + // host gets a bounded delivery rate no matter how many identities it + // signs with. Refusals are 503, NOT 429: Lemmy's federation crate + // treats server errors as retryable but drops 4xx permanently, and a + // legitimate burst must be delayed, never lost. + clientIP := ratelimit.ClientIP(r) + if !ib.ipLimiter.Allow(clientIP) { + InboxRateLimited.Add(1) + if ib.refusalLog.Allow(time.Now()) { + ib.logger.Warn("inbox delivery rate-limited by ip (sampled)", "ip", clientIP) + } + http.Error(w, "delivery rate limit exceeded; retry later", http.StatusServiceUnavailable) + return + } + body, err := io.ReadAll(http.MaxBytesReader(w, r.Body, maxInboxBodyBytes)) if err != nil { http.Error(w, "request body too large or unreadable", http.StatusRequestEntityTooLarge) @@ -134,7 +228,7 @@ actorID, err := ib.verifier.Verify(r.Context(), r, body) var activity *ap.Object if err != nil { - selfDelete, tombstonedActor, deferConfirm := ib.tombstonedSelfDelete(r.Context(), body, err) + selfDelete, tombstonedActor, deferConfirm := ib.tombstonedSelfDelete(r.Context(), body, err, clientIP) switch { case tombstonedActor != "": // A self-Delete whose actor is verifiably GONE at its own @@ -175,6 +269,19 @@ return } } + // Admission layer 2, per verified signer: one identity spraying from + // many addresses is bounded too. Keyed on the SIGNER (the key owner + // Verify authenticated — or, on the tombstone path, the confirmed-gone + // actor), which is unforgeable, unlike the activity's claimed actor. + if !ib.signerLimiter.Allow(actorID) { + InboxRateLimited.Add(1) + if ib.refusalLog.Allow(time.Now()) { + ib.logger.Warn("inbox delivery rate-limited by signer (sampled)", "signer", actorID) + } + http.Error(w, "delivery rate limit exceeded; retry later", http.StatusServiceUnavailable) + return + } + // The tombstoned-self-delete path already parsed the body; every other // path parses it here. if activity == nil { @@ -251,6 +358,10 @@ // date skew and the body digest before ever resolving the key; // - the payload is a bare self-referential Delete (actor == object) with // an id (the id gates the confirmation fetch: an unenqueueable activity // never earns an outbound request); +// - the sender's IP has budget in the dedicated tombstone-confirmation +// limiter (task 11) — otherwise the delivery DEFERS (503) without any +// outbound fetch, bounding the fetch-and-write amplification this +// branch could be farmed for; // - an INDEPENDENT fetch of that actor's own IRI — SSRF-guarded, derived // from the payload, not from any attacker-controllable keyId, and with // every redirect hop pinned to the IRI's own authority @@ -282,7 +393,7 @@ // or 404/401/403 (IsNotFound) → definitive reject; // - anything else (transport error, timeout, 5xx, context cancellation) // → defer: the failure says nothing about the actor, and permanently // dropping a legitimate deletion is unrecoverable. -func (ib *Inbox) tombstonedSelfDelete(ctx context.Context, body []byte, verifyErr error) (activity *ap.Object, actorID string, deferDelivery bool) { +func (ib *Inbox) tombstonedSelfDelete(ctx context.Context, body []byte, verifyErr error, clientIP string) (activity *ap.Object, actorID string, deferDelivery bool) { if !errors.IsTombstoned(verifyErr) { return nil, "", false } @@ -293,6 +404,18 @@ } actorIRI, objectIRI := refID(parsed.Actor), refID(parsed.Object) if actorIRI == "" || actorIRI != objectIRI { return nil, "", false + } + // The dedicated (much tighter) per-IP cap sits exactly in front of the + // costs this branch can be farmed for: the outbound confirmation fetch + // and, on a 410 answer, two durable writes. Checked only after the + // shape checks above, so an unenqueueable payload never spends a + // token, and answered as DEFER (503): a legitimate deletion behind the + // limiter is redelivered by the sender's queue, never dropped. + if !ib.tombstoneLimiter.Allow(clientIP) { + InboxRateLimited.Add(1) + ib.logger.Warn("tombstone confirmation rate-limited; deferring self-delete", + "ip", clientIP, "actor", actorIRI) + return nil, "", true } _, fetchErr := ib.fetcher.FetchActorSameAuthority(ctx, actorIRI) switch { diff --git a/internal/ingest/retry.go b/internal/ingest/retry.go new file mode 100644 --- /dev/null +++ b/internal/ingest/retry.go @@ -0,0 +1,184 @@ +package ingest + +import ( + "context" + "log/slog" + "time" + + "tidepool/internal/ap" + "tidepool/internal/errors" + "tidepool/internal/store" +) + +// Follow-retry defaults (task 11). The Lemmy first-contact Accept race: +// Lemmy initializes a newly-seen instance's federation cursor at the +// current max activity id, so the Accept answering the very FIRST Follow to +// a given Lemmy instance is usually skipped — the subscription sits in +// pending forever unless someone re-sends the Follow (fresh activity id → +// fresh Accept). The retrier automates what operators previously did by +// hand (and what the e2e harness does in subscribeCommunity). +const ( + // defaultFollowResendAfter is how long a subscription may sit pending + // before its Follow is re-sent. Accepts normally arrive within seconds; + // two minutes is far past any healthy exchange while still recovering + // the race promptly. + defaultFollowResendAfter = 2 * time.Minute + // defaultFollowRetryInterval is the sweep cadence. + defaultFollowRetryInterval = time.Minute + // defaultFollowMaxAttempts bounds TOTAL Follow sends per subscription + // (the admin subscribe's own send counts as the first). A community + // that never answers five Follows is not racing — it is refusing, + // unreachable, or misconfigured; the operator re-subscribing resets + // the budget. + defaultFollowMaxAttempts = 5 +) + +// FollowRetrierOptions configures NewFollowRetrier. Client, Communities, +// and Service are required; zero durations/counts take the defaults above. +type FollowRetrierOptions struct { + Client FollowClient + Communities store.Communities + Service *ap.ServiceActor + Logger *slog.Logger + + // ResendAfter is the pending-age threshold; Interval the sweep cadence; + // MaxAttempts the total-send budget. Tests shrink them. + ResendAfter time.Duration + Interval time.Duration + MaxAttempts int +} + +// FollowRetrier re-sends the Follow for subscriptions stuck in pending. +type FollowRetrier struct { + client FollowClient + communities store.Communities + service *ap.ServiceActor + logger *slog.Logger + + resendAfter time.Duration + interval time.Duration + maxAttempts int + now func() time.Time +} + +// NewFollowRetrier validates options and builds the retrier. +func NewFollowRetrier(opts FollowRetrierOptions) (*FollowRetrier, error) { + if opts.Client == nil { + return nil, errors.NewValidationError("client", "must not be nil") + } + if opts.Communities == nil { + return nil, errors.NewValidationError("communities", "must not be nil") + } + if opts.Service == nil { + return nil, errors.NewValidationError("service", "must not be nil") + } + logger := opts.Logger + if logger == nil { + logger = slog.Default() + } + r := &FollowRetrier{ + client: opts.Client, + communities: opts.Communities, + service: opts.Service, + logger: logger, + resendAfter: opts.ResendAfter, + interval: opts.Interval, + maxAttempts: opts.MaxAttempts, + now: time.Now, + } + if r.resendAfter <= 0 { + r.resendAfter = defaultFollowResendAfter + } + if r.interval <= 0 { + r.interval = defaultFollowRetryInterval + } + if r.maxAttempts <= 0 { + r.maxAttempts = defaultFollowMaxAttempts + } + return r, nil +} + +// Run sweeps for stale pending subscriptions until ctx is cancelled. +func (r *FollowRetrier) Run(ctx context.Context) { + ticker := time.NewTicker(r.interval) + defer ticker.Stop() + for { + select { + case <-ctx.Done(): + return + case <-ticker.C: + r.Sweep(ctx) + } + } +} + +// Sweep atomically claims every subscription pending past the threshold with +// attempt budget left and delivers a fresh Follow to each. Exported so tests +// (and operators via a future admin hook) can drive one sweep synchronously. +func (r *FollowRetrier) Sweep(ctx context.Context) { + claimed, err := r.communities.ClaimStalePendingFollows(ctx, r.now().Add(-r.resendAfter), r.maxAttempts) + if err != nil { + r.logger.Error("follow retrier: claim stale pending subscriptions", "error", err) + return + } + for _, community := range claimed { + if ctx.Err() != nil { + return + } + r.resend(ctx, community) + } +} + +// resend delivers a fresh Follow for a claimed subscription. The claim (a +// single row-locked UPDATE ... RETURNING in ClaimStalePendingFollows) has +// already consumed the attempt and re-stamped follow_requested_at, so resend +// itself never writes follow_state: an Accept that flipped this row to +// accepted between the claim and here is left as accepted (the claim never +// matched it, and even a redundant stale re-send is harmless — the bridge +// dedupes duplicate Follows). community.FollowAttempts carries the +// post-increment value, so a row at the budget is on its LAST send: no later +// sweep will ever claim it again, so exhaustion is logged loudly rather than +// leaving the misleading "will retry" on a subscription that is now stuck +// pending until an operator re-subscribes. +func (r *FollowRetrier) resend(ctx context.Context, community *store.Community) { + logger := r.logger.With("community", community.APGroupID, "attempts", community.FollowAttempts) + final := community.FollowAttempts >= r.maxAttempts + + // giveUpOrRetry logs a delivery failure loudly when the budget is spent + // (this was the last claim this row will ever get) and as a routine + // retry otherwise. + giveUpOrRetry := func(what string, args ...any) { + if final { + args = append(args, "max_attempts", r.maxAttempts) + logger.Error("follow retrier: giving up; subscription stuck pending, operator must re-subscribe: "+what, args...) + return + } + logger.Warn("follow retrier: "+what+"; will retry after threshold", args...) + } + + group, err := r.client.FetchActor(ctx, community.APGroupID) + if err != nil { + giveUpOrRetry("fetch group actor failed", "error", err) + return + } + inbox := group.SharedInboxOrInbox() + if inbox == "" { + giveUpOrRetry("group advertises no inbox") + return + } + follow, err := buildFollowActivity(r.service, community.APGroupID) + if err != nil { + logger.Error("follow retrier: build follow", "error", err) + return + } + if err := r.client.SendActivity(ctx, inbox, follow); err != nil { + giveUpOrRetry("deliver follow failed", "error", err) + return + } + if final { + logger.Error("follow retrier: final Follow sent; subscription still pending will require an operator to re-subscribe", + "activity_id", follow.ID, "max_attempts", r.maxAttempts) + return + } + logger.Info("follow re-sent for stale pending subscription", "activity_id", follow.ID) +} diff --git a/internal/ingest/votes.go b/internal/ingest/votes.go --- a/internal/ingest/votes.go +++ b/internal/ingest/votes.go @@ -2,16 +2,15 @@ package ingest import ( "context" - "log/slog" "tidepool/internal/ap" ) // VoteAggregator consumes Like/Dislike activities. Votes never become -// records (PLAN.md locked decision 7): task 07 implements this interface -// with the bridge-side aggregate store behind the -// social.coves.bridge.getVoteAggregates XRPC. Task 06 only defines the -// seam and hands activities over. +// records (PLAN.md locked decision 7): votes.Aggregator implements this +// interface with the bridge-side aggregate store behind the +// social.coves.bridge.getVoteAggregates XRPC (task 07); ingest only +// defines the seam and hands activities over. type VoteAggregator interface { // ApplyVote records one Like or Dislike. vote is the (possibly // Announce-unwrapped) activity: Type is Like or Dislike, Actor is the @@ -23,30 +22,3 @@ // RetractVote undoes a previously applied vote (Undo{Like|Dislike}); // vote is the inner activity being undone. RetractVote(ctx context.Context, vote *ap.Object, communityIRI string) error } - -// noopVotes is the task-06 placeholder implementation: it logs at debug and -// drops the vote. Task 07 replaces it with the real aggregator. -type noopVotes struct { - logger *slog.Logger -} - -// NewNoopVotes returns a VoteAggregator that discards votes (logged at -// debug). Wired until task 07 lands the aggregate store. -func NewNoopVotes(logger *slog.Logger) VoteAggregator { - if logger == nil { - logger = slog.Default() - } - return &noopVotes{logger: logger} -} - -func (v *noopVotes) ApplyVote(_ context.Context, vote *ap.Object, communityIRI string) error { - v.logger.Debug("vote dropped (aggregator lands in task 07)", - "type", vote.Type, "object", refID(vote.Object), "community", communityIRI) - return nil -} - -func (v *noopVotes) RetractVote(_ context.Context, vote *ap.Object, communityIRI string) error { - v.logger.Debug("vote retraction dropped (aggregator lands in task 07)", - "type", vote.Type, "object", refID(vote.Object), "community", communityIRI) - return nil -} diff --git a/internal/materialize/actors.go b/internal/materialize/actors.go --- a/internal/materialize/actors.go +++ b/internal/materialize/actors.go @@ -7,6 +7,7 @@ "strings" "time" "github.com/bluesky-social/indigo/atproto/atdata" + "github.com/ipfs/go-cid" "tidepool/internal/ap" "tidepool/internal/errors" @@ -201,19 +202,28 @@ // rematerializeProfile writes the actor's profile record (rkey "self"), // refreshes the mapping, and stamps profile_synced_at. Idempotent: an // unchanged profile is a repo-layer no-op. +// +// Media carry-forward (task 11): a refresh whose avatar/banner fetch FAILS +// while the actor still advertises the image keeps the previously stored +// blob instead of dropping it — a transient 5xx or timeout at the origin +// must not strip profiles bare until the next refresh. An actor that +// REMOVED its image (no icon/image in the doc) still loses the blob, as it +// should. func (m *Materializer) rematerializeProfile(ctx context.Context, stored *store.BridgedActor, doc *ap.Object) (*store.BridgedActor, error) { + collection := CollectionActorProfile + if stored.ActorType == store.ActorTypeGroup { + collection = CollectionCommunityProfile + } + + avatar := m.fetchBlobWithCarryForward(ctx, stored.DID, collection, imageURL(doc.Icon), slotAvatar, "avatar") + banner := m.fetchBlobWithCarryForward(ctx, stored.DID, collection, imageURL(doc.Image), slotBanner, "banner") + var record map[string]any - collection := CollectionActorProfile switch stored.ActorType { case store.ActorTypeGroup: - collection = CollectionCommunityProfile - record = m.buildCommunityProfile(doc, stored.CreatedAt, - m.fetchBlob(ctx, stored.DID, imageURL(doc.Icon), slotAvatar), - m.fetchBlob(ctx, stored.DID, imageURL(doc.Image), slotBanner)) + record = m.buildCommunityProfile(doc, stored.CreatedAt, avatar, banner) default: - record = m.buildActorProfile(doc, stored.CreatedAt, - m.fetchBlob(ctx, stored.DID, imageURL(doc.Icon), slotAvatar), - m.fetchBlob(ctx, stored.DID, imageURL(doc.Image), slotBanner)) + record = m.buildActorProfile(doc, stored.CreatedAt, avatar, banner) } if _, err := m.commitRecord(ctx, stored.DID, collection, ProfileRKey, record, doc, stored.DID); err != nil { return nil, err @@ -222,6 +232,48 @@ if err := m.actors.MarkProfileSynced(ctx, stored.APActorID, m.now()); err != nil { return nil, fmt.Errorf("materialize: mark profile synced for %s: %w", stored.APActorID, err) } return stored, nil +} + +// fetchBlobWithCarryForward fetches profile media like fetchBlob, but when a +// TRANSIENT fetch failure hits AND the actor still advertises an image URL, +// it falls back to the blob already stored in the existing profile record +// under field (avatar/banner) — a temporary 5xx/timeout/dial error must not +// strip the profile bare until the next refresh. A PERMANENT removal +// (IsNotFound 404 / IsTombstoned 410: the image was deleted at origin while +// its URL lingers in a stale doc) is NOT carried forward — the blob is +// dropped, because serving media the origin removed forever is wrong. A +// first-ever materialization has no existing record, so the fallback is +// naturally empty there. +func (m *Materializer) fetchBlobWithCarryForward(ctx context.Context, did, collection, url string, slot blobSlot, field string) *atdata.Blob { + if url == "" { + return nil + } + blob, fetchErr := m.fetchBlobClassified(ctx, did, url, slot) + if blob != nil { + return blob + } + if errors.IsNotFound(fetchErr) || errors.IsTombstoned(fetchErr) { + // Image gone at origin: drop it rather than carrying the stale blob + // forward forever. + m.logger.Info("media removed at origin; dropping previously stored blob", + "did", did, "field", field, "url", url) + return nil + } + existing, _, err := m.repos.GetRecord(ctx, did, collection, ProfileRKey) + if err != nil { + if !errors.IsNotFound(err) { + m.logger.Warn("media carry-forward: read existing profile failed", + "did", did, "field", field, "error", err) + } + return nil + } + prev, ok := existing[field].(atdata.Blob) + if !ok { + return nil + } + m.logger.Info("media fetch failed; carrying forward previously stored blob", + "did", did, "field", field, "url", url, "cid", cid.Cid(prev.Ref).String()) + return &prev } // EnsureCommunity makes sure the AP group behind groupRef is bridged: diff --git a/internal/materialize/blobs.go b/internal/materialize/blobs.go --- a/internal/materialize/blobs.go +++ b/internal/materialize/blobs.go @@ -37,8 +37,20 @@ // record. Media is always optional: every failure (unreachable, oversize, // wrong content type) logs a reason and returns nil so the caller omits the // image rather than dropping the record. func (m *Materializer) fetchBlob(ctx context.Context, did, url string, slot blobSlot) *atdata.Blob { + blob, _ := m.fetchBlobClassified(ctx, did, url, slot) + return blob +} + +// fetchBlobClassified is fetchBlob that also reports the ORIGIN fetch error, +// so carry-forward callers can distinguish a permanent removal (IsNotFound +// for 404/401/403, IsTombstoned for 410 — the image is gone at the source) +// from a transient failure (timeout/5xx/dial — the image is probably still +// there). Only the download error is surfaced; unusable-media outcomes +// (wrong content type, oversize, blob-store failure) return (nil, nil) — the +// origin gave no removal signal, so the image is simply omitted this pass. +func (m *Materializer) fetchBlobClassified(ctx context.Context, did, url string, slot blobSlot) (*atdata.Blob, error) { if url == "" { - return nil + return nil, nil } budget := slot.maxSize if m.maxBlob < budget { @@ -48,21 +60,21 @@ data, contentType, err := m.fetcher.FetchMedia(ctx, url, budget) if err != nil { m.logger.Warn("media fetch failed; omitting image", "slot", slot.name, "url", url, "error", err) - return nil + return nil, err } mimeType := normalizeImageMime(contentType, data) if !slotAccepts(slot, mimeType) { m.logger.Warn("media has unacceptable content type; omitting image", "slot", slot.name, "url", url, "content_type", mimeType) - return nil + return nil, nil } blob, err := m.repos.PutBlob(ctx, did, mimeType, data) if err != nil { m.logger.Warn("blob store failed; omitting image", "slot", slot.name, "url", url, "did", did, "error", err) - return nil + return nil, nil } - return blob + return blob, nil } // normalizeImageMime parses the server's Content-Type; when it is absent, diff --git a/internal/materialize/hardening_test.go b/internal/materialize/hardening_test.go new file mode 100644 --- /dev/null +++ b/internal/materialize/hardening_test.go @@ -0,0 +1,304 @@ +package materialize + +import ( + "context" + "encoding/json" + "fmt" + "net/http" + "sync/atomic" + "testing" + + "github.com/bluesky-social/indigo/atproto/atdata" + "github.com/ipfs/go-cid" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "tidepool/internal/ap" + "tidepool/internal/errors" + "tidepool/internal/repo" +) + +// Task 11 hardening tests: the #account frame + vote scrub + blob scrub on +// Delete(Actor), media carry-forward on transient refresh failure, and the +// validation-failure metric. + +// TestDeleteActor_AccountEventVoteAndBlobScrub drives the full terminal +// scrub on the captured lemmy.world fixtures: record deletes, the voter's +// vote_events scrub, blob deletion under BOTH the author's and the +// community's DID, and the trailing #account{active:false,status:deleted} +// firehose event. +func TestDeleteActor_AccountEventVoteAndBlobScrub(t *testing.T) { + h := newHarness(t) + h.serveLemmyWorldFixtures() + ctx := context.Background() + + _, err := h.m.MaterializePost(ctx, loadFixtureObject(t, "page_lemmy_world.json")) + require.NoError(t, err) + communityDID := testDIDFor("technology", "lemmy.world") + authorDID := testDIDFor("LeftLeaningFreedomFighters", "lemmy.world") + + // The post's external embed carries a thumbnail blob stored under the + // COMMUNITY's DID — the blob nothing but this scrub would ever clean up. + mapping, err := h.objects.GetByAPID(ctx, pageID) + require.NoError(t, err) + record, _, err := h.manager.GetRecord(ctx, communityDID, CollectionPost, mapping.RKey) + require.NoError(t, err) + blobs := atdata.ExtractBlobs(record) + require.NotEmpty(t, blobs, "the fixture post must embed a thumbnail blob") + thumbCID := cid.Cid(blobs[0].Ref).String() + _, _, err = h.manager.GetBlob(ctx, communityDID, thumbCID) + require.NoError(t, err, "thumbnail blob stored under the community DID before the scrub") + + require.NoError(t, h.m.DeleteActor(ctx, personID)) + + // Vote scrub hook called with the actor's AP id. + assert.Equal(t, []string{personID}, h.scrubbed.calls()) + + // The community-DID thumbnail blob is gone; the community's own profile + // media (if any) would survive because only THIS actor's records were + // walked. + _, _, err = h.manager.GetBlob(ctx, communityDID, thumbCID) + assert.True(t, errors.IsNotFound(err), "the deleted author's post thumbnail must be scrubbed from the community repo") + + // Every blob under the author's own (terminally frozen) DID is gone. + n, err := h.manager.DeleteBlobsForDID(ctx, authorDID) + require.NoError(t, err) + assert.Zero(t, n, "DeleteActor must already have emptied the author's blob rows") + + // The LAST firehose event for the author repo is the #account frame: + // active=false, status=deleted, sequenced after the scrub deletes. + events := h.firehoseEvents() + require.NotEmpty(t, events) + last := events[len(events)-1] + assert.Equal(t, repo.EventKindAccount, last.Kind) + assert.Equal(t, authorDID, last.DID) + assert.False(t, last.AccountActive) + assert.Equal(t, repo.AccountStatusDeleted, last.AccountStatus) + var accountEvents, deleteOps int + for _, ev := range events { + if ev.Kind == repo.EventKindAccount { + accountEvents++ + continue + } + for _, op := range ev.Ops { + if op.Action == repo.OpActionDelete { + deleteOps++ + } + } + } + assert.Equal(t, 1, accountEvents, "exactly one account event for one Delete(Actor)") + assert.Equal(t, 2, deleteOps, "scrub deletes: the post and the actor profile") +} + +// TestSuppressActor_ScrubsVotesToo: the reversible nobridge scrub erases +// vote_events rows with the same posture as the terminal delete — but emits +// NO #account frame (the repo stays active). +func TestSuppressActor_ScrubsVotesToo(t *testing.T) { + h := newHarness(t) + h.serveLemmyWorldFixtures() + ctx := context.Background() + + _, err := h.m.MaterializePost(ctx, loadFixtureObject(t, "page_lemmy_world.json")) + require.NoError(t, err) + + require.NoError(t, h.m.SuppressActor(ctx, personID)) + assert.Equal(t, []string{personID}, h.scrubbed.calls()) + for _, ev := range h.firehoseEvents() { + assert.NotEqual(t, repo.EventKindAccount, ev.Kind, + "nobridge is reversible: no #account frame may be emitted") + } +} + +// TestProfileRefresh_MediaCarryForward: a profile refresh whose avatar +// fetch fails transiently keeps the previously stored blob; an actor that +// REMOVED its avatar loses it. +func TestProfileRefresh_MediaCarryForward(t *testing.T) { + h := newHarness(t) + ctx := context.Background() + + const actorID = "https://lemmy.world/u/carrie" + const avatarPath = "/pictrs/image/carrie-avatar.png" + var failAvatar atomic.Bool + h.mux.HandleFunc("GET "+avatarPath, func(w http.ResponseWriter, _ *http.Request) { + if failAvatar.Load() { + w.WriteHeader(http.StatusServiceUnavailable) + return + } + w.Header().Set("Content-Type", "image/png") + _, _ = w.Write(pngBytes) + }) + // The actor document is mutable mid-test (icon removed in phase 3), so + // it is served through a swappable pointer instead of serveObject + // (ServeMux refuses duplicate patterns). + var actorDoc atomic.Value + actorDoc.Store(person(actorID, "carrie", map[string]any{ + "icon": map[string]any{"type": "Image", "url": "https://lemmy.world" + avatarPath}, + })) + h.mux.HandleFunc("GET /u/carrie", func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", ap.ContentTypeActivityJSON) + _ = json.NewEncoder(w).Encode(actorDoc.Load()) + }) + + stored, err := h.m.EnsureActor(ctx, &ap.Object{ID: actorID}) + require.NoError(t, err) + profile, _, err := h.manager.GetRecord(ctx, stored.DID, CollectionActorProfile, ProfileRKey) + require.NoError(t, err) + original, ok := profile["avatar"].(atdata.Blob) + require.True(t, ok, "first materialization stores the avatar blob") + + // Transient origin failure: the refresh must carry the old blob forward. + failAvatar.Store(true) + _, err = h.m.RefreshActor(ctx, &ap.Object{ID: actorID}) + require.NoError(t, err) + profile, _, err = h.manager.GetRecord(ctx, stored.DID, CollectionActorProfile, ProfileRKey) + require.NoError(t, err) + carried, ok := profile["avatar"].(atdata.Blob) + require.True(t, ok, "a transient media failure must not strip the avatar") + assert.Equal(t, cid.Cid(original.Ref).String(), cid.Cid(carried.Ref).String()) + + // Deliberate removal: the actor no longer advertises an icon → no + // carry-forward, the avatar drops. + actorDoc.Store(person(actorID, "carrie", nil)) + _, err = h.m.RefreshActor(ctx, &ap.Object{ID: actorID}) + require.NoError(t, err) + profile, _, err = h.manager.GetRecord(ctx, stored.DID, CollectionActorProfile, ProfileRKey) + require.NoError(t, err) + _, ok = profile["avatar"].(atdata.Blob) + assert.False(t, ok, "a removed avatar must not be carried forward") +} + +// TestProfileRefresh_PermanentRemovalDropsBlob: when the actor still +// advertises an image URL but the origin now answers 404 (the image was +// deleted at source), the stale blob is DROPPED — not carried forward +// forever like a transient failure would be. +func TestProfileRefresh_PermanentRemovalDropsBlob(t *testing.T) { + h := newHarness(t) + ctx := context.Background() + + const actorID = "https://lemmy.world/u/perm" + const avatarPath = "/pictrs/image/perm-avatar.png" + var gone atomic.Bool + h.mux.HandleFunc("GET "+avatarPath, func(w http.ResponseWriter, _ *http.Request) { + if gone.Load() { + // Permanent: image removed at origin (404 → IsNotFound). + w.WriteHeader(http.StatusNotFound) + return + } + w.Header().Set("Content-Type", "image/png") + _, _ = w.Write(pngBytes) + }) + doc := person(actorID, "perm", map[string]any{ + "icon": map[string]any{"type": "Image", "url": "https://lemmy.world" + avatarPath}, + }) + h.mux.HandleFunc("GET /u/perm", func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", ap.ContentTypeActivityJSON) + _ = json.NewEncoder(w).Encode(doc) + }) + + stored, err := h.m.EnsureActor(ctx, &ap.Object{ID: actorID}) + require.NoError(t, err) + profile, _, err := h.manager.GetRecord(ctx, stored.DID, CollectionActorProfile, ProfileRKey) + require.NoError(t, err) + _, ok := profile["avatar"].(atdata.Blob) + require.True(t, ok, "first materialization stores the avatar blob") + + // Image gone at origin, but the doc STILL advertises its URL: a permanent + // removal drops the blob (a transient 5xx would have carried it forward). + gone.Store(true) + _, err = h.m.RefreshActor(ctx, &ap.Object{ID: actorID}) + require.NoError(t, err) + profile, _, err = h.manager.GetRecord(ctx, stored.DID, CollectionActorProfile, ProfileRKey) + require.NoError(t, err) + _, ok = profile["avatar"].(atdata.Blob) + assert.False(t, ok, "a permanent 404 removal must not be carried forward") +} + +// TestDeleteActor_BlobScrubFailureIsRetryable: a failing blob delete makes +// DeleteActor return a retryable (non-skip) error so the queue redelivers it; +// a subsequent retry, with the failure cleared, completes idempotently — the +// orphan blob is scrubbed and the #account frame is emitted exactly once. +func TestDeleteActor_BlobScrubFailureIsRetryable(t *testing.T) { + h := newHarness(t) + h.serveLemmyWorldFixtures() + ctx := context.Background() + + _, err := h.m.MaterializePost(ctx, loadFixtureObject(t, "page_lemmy_world.json")) + require.NoError(t, err) + communityDID := testDIDFor("technology", "lemmy.world") + mapping, err := h.objects.GetByAPID(ctx, pageID) + require.NoError(t, err) + record, _, err := h.manager.GetRecord(ctx, communityDID, CollectionPost, mapping.RKey) + require.NoError(t, err) + blobs := atdata.ExtractBlobs(record) + require.NotEmpty(t, blobs, "the fixture post must embed a thumbnail blob") + thumbCID := cid.Cid(blobs[0].Ref).String() + + // Inject a transient failure on the community-DID thumbnail delete. + realDelete := h.m.deleteBlob + var fail atomic.Bool + fail.Store(true) + h.m.deleteBlob = func(ctx context.Context, did, c string) error { + if fail.Load() && c == thumbCID { + return fmt.Errorf("transient storage failure") + } + return realDelete(ctx, did, c) + } + + // First attempt fails, and the failure is RETRYABLE (not a skip): the + // queue must redeliver rather than drop the delete and orphan the blob. + err = h.m.DeleteActor(ctx, personID) + require.Error(t, err) + assert.False(t, IsSkip(err), "a blob scrub failure must be retryable, not a skip") + _, _, err = h.manager.GetBlob(ctx, communityDID, thumbCID) + require.NoError(t, err, "the orphan blob survives the failed scrub") + for _, ev := range h.firehoseEvents() { + assert.NotEqual(t, repo.EventKindAccount, ev.Kind, + "no #account frame until the scrub completes") + } + + // Retry with the failure cleared: DeleteActor is idempotent and completes. + fail.Store(false) + require.NoError(t, h.m.DeleteActor(ctx, personID)) + _, _, err = h.manager.GetBlob(ctx, communityDID, thumbCID) + assert.True(t, errors.IsNotFound(err), "the retry scrubs the orphaned blob") + events := h.firehoseEvents() + require.NotEmpty(t, events) + assert.Equal(t, repo.EventKindAccount, events[len(events)-1].Kind, + "the completing retry emits the #account frame") + var accountEvents int + for _, ev := range events { + if ev.Kind == repo.EventKindAccount { + accountEvents++ + } + } + assert.Equal(t, 1, accountEvents, "exactly one #account frame across the failed attempt and the retry") +} + +// TestValidationFailureMetric: the expvar counter moves in both strict and +// log-and-write modes. +func TestValidationFailureMetric(t *testing.T) { + h := newHarness(t) + + invalid := map[string]any{"$type": CollectionPost} // missing every required field + before := ValidationFailures.Value() + + // Strict (the harness default): failure is an error AND counted. + err := h.m.validateRecord(invalid) + require.Error(t, err) + assert.True(t, errors.IsValidation(err)) + assert.Equal(t, before+1, ValidationFailures.Value()) + + // Log-and-write (production): no error, still counted. + h.m.strict = false + require.NoError(t, h.m.validateRecord(invalid)) + assert.Equal(t, before+2, ValidationFailures.Value()) + + // A valid record does not move the counter. + valid := map[string]any{ + "$type": CollectionActorProfile, + "bio": "hello", + "createdAt": "2026-01-01T00:00:00.000Z", + } + require.NoError(t, h.m.validateRecord(valid)) + assert.Equal(t, before+2, ValidationFailures.Value()) +} diff --git a/internal/materialize/materialize_test.go b/internal/materialize/materialize_test.go --- a/internal/materialize/materialize_test.go +++ b/internal/materialize/materialize_test.go @@ -10,6 +10,7 @@ "net/http/httptest" "os" "path/filepath" "strings" + "sync" "testing" "github.com/bluesky-social/indigo/atproto/atcrypto" @@ -113,6 +114,26 @@ actors store.BridgedActors communities store.Communities mux *http.ServeMux fixtures *httptest.Server + scrubbed *recordingScrubber +} + +// recordingScrubber records ScrubVoter calls (the task-11 vote-scrub hook). +type recordingScrubber struct { + mu sync.Mutex + ids []string +} + +func (r *recordingScrubber) ScrubVoter(_ context.Context, voterAPID string) error { + r.mu.Lock() + defer r.mu.Unlock() + r.ids = append(r.ids, voterAPID) + return nil +} + +func (r *recordingScrubber) calls() []string { + r.mu.Lock() + defer r.mu.Unlock() + return append([]string(nil), r.ids...) } // newHarness wires the full materialization stack over the real test @@ -144,6 +165,7 @@ PerHostBurst: 100000, MaxAttempts: 1, // fixture errors are deterministic; retries just slow tests }) + scrubbed := &recordingScrubber{} m, err := New(Options{ Fetcher: client, Objects: objects, @@ -151,6 +173,7 @@ Actors: actors, Communities: communities, Repos: manager, Minter: &fakeMinter{custodian: custodian}, + Votes: scrubbed, ServiceDID: testServiceDID, StrictValidation: true, // tests always validate against the vendored lexicons }) @@ -159,7 +182,7 @@ h := &harness{ t: t, m: m, manager: manager, objects: objects, actors: actors, communities: communities, - mux: mux, fixtures: fixtures, + mux: mux, fixtures: fixtures, scrubbed: scrubbed, } // Every pictrs-style image path serves fixed bytes by extension. mux.HandleFunc("/pictrs/image/", func(w http.ResponseWriter, r *http.Request) { diff --git a/internal/materialize/materializer.go b/internal/materialize/materializer.go --- a/internal/materialize/materializer.go +++ b/internal/materialize/materializer.go @@ -18,8 +18,10 @@ package materialize import ( "context" + "database/sql" "encoding/json" stderrors "errors" + "expvar" "fmt" "log/slog" "time" @@ -35,6 +37,15 @@ "tidepool/internal/repo" "tidepool/internal/store" "tidepool/lexicons" ) + +// ValidationFailures counts records that failed lexicon validation +// (strict and log-and-write modes alike). Exported via the expvar registry +// as "tidepool_lexicon_validation_failures" — cmd/tidepool serves the +// registry on the admin surface, so a non-zero counter in production (where +// failures log-and-write) is observable without log scraping. Task 05 +// deferred a strict-first production rollout; this is the metric that +// decision waits on. +var ValidationFailures = expvar.NewInt("tidepool_lexicon_validation_failures") // Record collections the materializer produces. const ( @@ -92,6 +103,13 @@ type ActorMinter interface { MintActor(ctx context.Context, req identity.MintRequest) (*identity.Identity, error) } +// VoteScrubber erases an actor's vote_events rows — the vote counterpart of +// the record scrub (votes.Aggregator implements it). Optional: a nil +// scrubber skips vote scrubbing (tests that don't exercise votes). +type VoteScrubber interface { + ScrubVoter(ctx context.Context, voterAPID string) error +} + // Options configures New. Fetcher, Objects, Actors, Communities, Repos, // Minter, and ServiceDID are required. type Options struct { @@ -101,6 +119,9 @@ Actors store.BridgedActors Communities store.Communities Repos *repo.Manager Minter ActorMinter + // Votes scrubs a deleted actor's vote_events rows alongside the record + // scrub (optional; nil skips it). + Votes VoteScrubber // ServiceDID is the bridge's own DID: community.profile createdBy and // hostedBy (PLAN.md locked decision 6). ServiceDID string @@ -128,6 +149,7 @@ actors store.BridgedActors communities store.Communities repos *repo.Manager minter ActorMinter + votes VoteScrubber serviceDID string profileTTL time.Duration maxBlob int64 @@ -135,6 +157,10 @@ strict bool catalog *lexicon.BaseCatalog logger *slog.Logger now func() time.Time // test seam for profile TTL + // deleteBlob deletes one blob by (did, cid); defaults to repos.DeleteBlob. + // A test seam so the scrub's retryable-error path can be exercised + // without a real storage failure. + deleteBlob func(ctx context.Context, did, cid string) error } // New validates options and builds a Materializer. The vendored lexicon @@ -176,6 +202,7 @@ actors: opts.Actors, communities: opts.Communities, repos: opts.Repos, minter: opts.Minter, + votes: opts.Votes, serviceDID: opts.ServiceDID, profileTTL: opts.ProfileRefreshTTL, maxBlob: opts.MaxBlobBytes, @@ -184,6 +211,7 @@ catalog: catalog, logger: logger, now: time.Now, } + m.deleteBlob = m.repos.DeleteBlob if m.profileTTL <= 0 { m.profileTTL = defaultProfileRefreshTTL } @@ -206,17 +234,19 @@ NoOp bool } // commitRecord is the single write path for every materialized record: -// lexicon-validate, commit into the repo, upsert the ap_objects mapping. -// authorDID records who authored the record (differs from did for posts). +// lexicon-validate, then commit the record AND upsert its ap_objects +// mapping in ONE transaction (repo.PutRecordTx + PutMappingTx — task 11 +// closed the crash window where a record could land on the firehose with +// no mapping). authorDID records who authored the record (differs from did +// for posts). func (m *Materializer) commitRecord(ctx context.Context, did, collection, rkey string, record map[string]any, obj *ap.Object, authorDID string) (*Result, error) { // Don't resurrect deleted content. AP delivery is unordered, so a Create // or Update can arrive (or be re-delivered) after a Delete already // tombstoned this object's mapping. Re-materializing would un-tombstone it - // (PutMapping resets deleted_at) and re-commit the record. Task 06 clears - // the tombstone explicitly on an Undo(Delete)/restore. - // (Note: this does not cover a Delete that arrived before the object was - // ever materialized — no mapping exists to tombstone; that ordering needs - // task 06's inbox dedup. Tracked in LOOP_STATE.) + // (PutMapping resets deleted_at) and re-commit the record. The ingest + // layer clears the tombstone explicitly on an Undo(Delete)/restore, and + // its ap_tombstones marker covers the delete-before-create ordering + // (a Delete for a never-materialized object). if existing, err := m.objects.GetByAPID(ctx, obj.ID); err == nil { if existing.IsDeleted() { return nil, skip(obj.ID, "object was deleted upstream; not resurrecting") @@ -228,10 +258,6 @@ if err := m.validateRecord(record); err != nil { return nil, err } - res, err := m.repos.PutRecord(ctx, did, collection, rkey, record) - if err != nil { - return nil, fmt.Errorf("materialize: put %s/%s/%s for %s: %w", did, collection, rkey, obj.ID, err) - } mapping := store.APObjectMapping{ APID: obj.ID, @@ -242,24 +268,34 @@ DID: did, AuthorDID: authorDID, Collection: collection, RKey: rkey, - CID: res.RecordCID, } if obj.Published.OK() { published := obj.Published.Time mapping.PublishedAt = &published } - stored, err := m.objects.PutMapping(ctx, mapping) + + var stored *store.APObjectMapping + res, err := m.repos.PutRecordTx(ctx, did, collection, rkey, record, + func(ctx context.Context, tx *sql.Tx, res *repo.CommitResult) error { + mapping.CID = res.RecordCID + var mapErr error + stored, mapErr = m.objects.PutMappingTx(ctx, tx, mapping) + if mapErr != nil { + if errors.IsAlreadyExists(mapErr) { + // A different AP id already claimed this at-uri: a + // deterministic TID collision (near-impossible after the + // hash-filled-micros change; see repo.DeterministicTID). + // Loud by design — this is a bug signal, and failing here + // now rolls the record write back with it. + m.logger.Error("deterministic rkey collision: different ap_id claimed the same at-uri", + "ap_id", obj.ID, "did", did, "collection", collection, "rkey", rkey) + } + return fmt.Errorf("materialize: map %s: %w", obj.ID, mapErr) + } + return nil + }) if err != nil { - if errors.IsAlreadyExists(err) { - // A different AP id already claimed this at-uri: a deterministic - // TID collision (near-impossible after the hash-filled-micros - // change; see repo.DeterministicTID). Loud by design — this is a - // bug signal, and the record we just wrote belongs to the OTHER - // object. - m.logger.Error("deterministic rkey collision: different ap_id claimed the same at-uri", - "ap_id", obj.ID, "did", did, "collection", collection, "rkey", rkey) - } - return nil, fmt.Errorf("materialize: map %s: %w", obj.ID, err) + return nil, fmt.Errorf("materialize: put %s/%s/%s for %s: %w", did, collection, rkey, obj.ID, err) } return &Result{DID: did, ATURI: stored.ATURI, CID: res.RecordCID, NoOp: res.NoOp}, nil } @@ -278,6 +314,11 @@ if err != nil { return fmt.Errorf("materialize: encode record for validation: %w", err) } if err := lexicon.ValidateRecord(m.catalog, data, recordType, lexicon.ValidateFlags(0)); err != nil { + // Counted in BOTH modes: the metric is how a production operator + // (log-and-write mode) notices validator disagreements without log + // scraping, and strict-mode counts keep dev/prod dashboards + // comparable. + ValidationFailures.Add(1) if m.strict { return errors.NewValidationError("record", fmt.Sprintf("%s fails lexicon validation: %v", recordType, err)) diff --git a/internal/materialize/updates.go b/internal/materialize/updates.go --- a/internal/materialize/updates.go +++ b/internal/materialize/updates.go @@ -4,8 +4,12 @@ import ( "context" "fmt" + "github.com/bluesky-social/indigo/atproto/atdata" + "github.com/ipfs/go-cid" + "tidepool/internal/ap" "tidepool/internal/errors" + "tidepool/internal/repo" "tidepool/internal/store" ) @@ -73,10 +77,13 @@ return m.deleteMapping(ctx, mapping) } // SuppressActor stops bridging an actor that opted out (#nobridge/#nobot) -// after previously being bridged: it scrubs every record they authored, then -// records ConsentStateNoBridge. Unlike DeleteActor this is reversible — if -// the marker later disappears upstream, EnsureActor restores bridging. Task -// 06 calls this when it discovers a consent-marker transition out of band. +// after previously being bridged: it scrubs every record they authored +// (and the blobs those records referenced, plus their vote_events rows), +// then records ConsentStateNoBridge. Unlike DeleteActor this is reversible +// — if the marker later disappears upstream, EnsureActor restores bridging +// (re-materialization re-fetches media, so scrubbed blobs come back too). +// Task 06 calls this when it discovers a consent-marker transition out of +// band. func (m *Materializer) SuppressActor(ctx context.Context, apActorID string) error { actor, err := m.actors.GetByAPActorID(ctx, apActorID) if errors.IsNotFound(err) { @@ -87,6 +94,9 @@ return fmt.Errorf("materialize: look up actor %s: %w", apActorID, err) } n, err := m.scrubActorRecords(ctx, actor.DID) if err != nil { + return err + } + if err := m.scrubActorVotes(ctx, apActorID); err != nil { return err } if err := m.actors.SetConsentState(ctx, apActorID, store.ConsentStateNoBridge); err != nil { @@ -99,15 +109,34 @@ } // scrubActorRecords tombstones every record an actor authored — records in // their own repo (comments, profile) and posts written into community repos -// (author_did). Idempotent: already-deleted mappings are no-ops. Shared by -// DeleteActor (terminal) and the nobridge suppression paths (reversible), so -// the consent decision is separate from the scrub mechanism. +// (author_did) — and deletes the blobs those records referenced (read out +// of each record BEFORE its delete commit; post images live under the +// COMMUNITY's DID, which nothing else would ever clean up). Idempotent: +// already-deleted mappings are no-ops, and blob deletes tolerate missing +// rows. Shared by DeleteActor (terminal) and the nobridge suppression paths +// (reversible), so the consent decision is separate from the scrub +// mechanism. func (m *Materializer) scrubActorRecords(ctx context.Context, actorDID string) (int, error) { mappings, err := m.objects.ListByActorDID(ctx, actorDID) if err != nil { return 0, fmt.Errorf("materialize: list records for %s: %w", actorDID, err) } for _, mapping := range mappings { + // Delete the referenced blobs BEFORE soft-deleting the mapping, and + // fail the whole scrub (a retryable error — NOT a skip) if any blob + // delete fails. A swallowed failure orphans a blob that getBlob would + // serve forever: post images live under the COMMUNITY's DID, which + // nothing else cleans up. The ordering keeps the retry able to heal: + // while the mapping is still live, ListByActorDID re-lists it and the + // record is still readable, so the retry re-collects the same blob + // refs and re-deletes them (DeleteBlob tolerates already-missing + // rows). Soft-deleting the mapping first would drop it out of + // ListByActorDID (deleted_at IS NULL) and strand the orphan. + for _, blobCID := range m.recordBlobRefs(ctx, mapping) { + if err := m.deleteBlob(ctx, mapping.DID, blobCID); err != nil { + return 0, fmt.Errorf("materialize: scrub blob %s under %s: %w", blobCID, mapping.DID, err) + } + } if err := m.deleteMapping(ctx, mapping); err != nil { return 0, err } @@ -115,12 +144,52 @@ } return len(mappings), nil } +// recordBlobRefs reads a mapping's current record and extracts every blob +// CID it references. Best-effort by design: a record already missing from +// the repo (crash window on a previous scrub run) just means no blobs to +// collect. +func (m *Materializer) recordBlobRefs(ctx context.Context, mapping *store.APObjectMapping) []string { + if mapping.IsDeleted() { + return nil + } + record, _, err := m.repos.GetRecord(ctx, mapping.DID, mapping.Collection, mapping.RKey) + if err != nil { + if !errors.IsNotFound(err) { + m.logger.Warn("blob scrub: read record failed; skipping its blobs", + "at_uri", mapping.ATURI, "error", err) + } + return nil + } + blobs := atdata.ExtractBlobs(record) + refs := make([]string, 0, len(blobs)) + for _, blob := range blobs { + refs = append(refs, cid.Cid(blob.Ref).String()) + } + return refs +} + +// scrubActorVotes erases the actor's vote_events rows via the configured +// VoteScrubber (nil = not wired, skip). +func (m *Materializer) scrubActorVotes(ctx context.Context, apActorID string) error { + if m.votes == nil { + return nil + } + if err := m.votes.ScrubVoter(ctx, apActorID); err != nil { + return fmt.Errorf("materialize: scrub votes for %s: %w", apActorID, err) + } + return nil +} + // DeleteActor handles Delete(Actor): every record the actor authored is // tombstoned FIRST (delete commits — the repo layer releases signing keys -// for deletes even after a consent flip), then the consent state is marked -// deleted (terminal), freezing the repo. The order means a crash between -// the two steps re-runs cleanly: deletes are idempotent, and the final -// consent flip still lands. +// for deletes even after a consent flip), their vote_events rows are +// scrubbed, then the consent state is marked deleted (terminal), freezing +// the repo — and an #account{active:false, status:"deleted"} event is +// appended to the firehose so subscribers (relays above all) purge the repo +// instead of inferring its death from the delete commits. The order means a +// crash between any two steps re-runs cleanly: deletes and vote scrubs are +// idempotent, re-tombstoning is a no-op success, and the final account +// event still lands. func (m *Materializer) DeleteActor(ctx context.Context, apActorID string) error { actor, err := m.actors.GetByAPActorID(ctx, apActorID) if errors.IsNotFound(err) { @@ -131,15 +200,44 @@ if err != nil { return fmt.Errorf("materialize: look up actor %s: %w", apActorID, err) } + // Terminal fixpoint: a second Delete carrying a distinct activity id (so + // inbox dedup did not absorb it) must not re-scrub and, above all, must + // not append a SECOND #account frame. ConsentStateDeleted is terminal — + // the repo is already tombstoned and its scrub already emitted — so stop + // here. + if actor.ConsentState == store.ConsentStateDeleted { + m.logger.Debug("delete for already-deleted actor; terminal, nothing to do", + "ap_actor_id", apActorID, "did", actor.DID) + return nil + } + // Everything they authored: records in their own repo (comments, - // profile) plus posts written into community repos (author_did). + // profile) plus posts written into community repos (author_did) — and + // the blobs those records referenced (scrubActorRecords). n, err := m.scrubActorRecords(ctx, actor.DID) if err != nil { return err } + if err := m.scrubActorVotes(ctx, apActorID); err != nil { + return err + } + // The actor's own repo is now terminally frozen (getBlob refuses + // deactivated repos and nothing re-materializes into it): drop every + // remaining blob under their DID, including superseded profile media no + // live record referenced anymore. + if _, err := m.repos.DeleteBlobsForDID(ctx, actor.DID); err != nil { + return fmt.Errorf("materialize: scrub blobs for %s: %w", actor.DID, err) + } if err := m.actors.SetConsentState(ctx, apActorID, store.ConsentStateDeleted); err != nil { return fmt.Errorf("materialize: tombstone actor %s: %w", apActorID, err) + } + // AFTER the scrub commits and the consent flip, so the frame is the + // last thing a per-repo-ordered consumer sees. Emitted even when the + // actor authored nothing (an #account frame for a bare repo is still + // correct), and re-emitted on idempotent re-runs (harmless). + if _, err := m.repos.AppendAccountEvent(ctx, actor.DID, false, repo.AccountStatusDeleted); err != nil { + return fmt.Errorf("materialize: append account event for %s: %w", actor.DID, err) } m.logger.Info("actor deleted upstream; bridged records scrubbed and repo tombstoned", "ap_actor_id", apActorID, "did", actor.DID, "records", n) diff --git a/internal/materialize/updates_test.go b/internal/materialize/updates_test.go --- a/internal/materialize/updates_test.go +++ b/internal/materialize/updates_test.go @@ -131,8 +131,14 @@ c1Mapping, err := h.objects.GetByAPID(ctx, "https://lemmy.world/comment/1001") require.NoError(t, err) assert.False(t, c1Mapping.IsDeleted()) - // Idempotent: a replayed Delete(Actor) is a no-op success. + // Idempotent AND a terminal fixpoint: a replayed Delete(Actor) carrying a + // distinct activity id (so inbox dedup did not absorb it) is a no-op + // success that must NOT re-scrub or append a SECOND #account frame — + // ConsentStateDeleted short-circuits before re-emitting. + before := len(h.firehoseEvents()) require.NoError(t, h.m.DeleteActor(ctx, authorAP)) + assert.Len(t, h.firehoseEvents(), before, + "a second DeleteActor on an already-deleted actor emits no new firehose event") } // TestDeleteActorViaHandleDelete: HandleDelete recognizes actor ids and diff --git a/internal/prune/prune.go b/internal/prune/prune.go new file mode 100644 --- /dev/null +++ b/internal/prune/prune.go @@ -0,0 +1,68 @@ +// Package prune is the shared retention-sweep loop: run a batched delete +// once immediately and then on an interval, fail closed on a non-positive +// retention, log what was reclaimed. The firehose pruner (task 04) defined +// the discipline; task 11 extracted it so ap_tombstones and vote_events +// retention behave identically. +package prune + +import ( + "context" + "log/slog" + "time" +) + +// DefaultInterval is how often a sweep runs when the caller passes zero; +// retention windows are hours-to-days, so hourly sweeps keep tables tight +// without meaningful load. +const DefaultInterval = time.Hour + +// Func deletes rows older than the cutoff and reports how many it removed. +// Implementations batch internally so one sweep never holds long locks. +type Func func(ctx context.Context, cutoff time.Time) (int64, error) + +// Run enforces one retention policy until ctx is cancelled. retention <= 0 +// refuses to run (fail closed: a zero retention would compute cutoff == now +// and delete everything, every sweep); interval <= 0 selects +// DefaultInterval. +func Run(ctx context.Context, name string, retention, interval time.Duration, fn Func, logger *slog.Logger) { + if logger == nil { + logger = slog.Default() + } + if retention <= 0 { + logger.Error("pruner refusing to run", + "pruner", name, "retention", retention.String(), "reason", "retention must be positive") + return + } + if interval <= 0 { + interval = DefaultInterval + } + sweep := func() { + cutoff := time.Now().Add(-retention) + n, err := fn(ctx, cutoff) + if err != nil { + if ctx.Err() == nil { + // A batched pruner can fail mid-run after deleting some rows + // (n>0, err): log the partial count so the reclaimed work is + // not invisible. + logger.Error("retention prune failed", "pruner", name, "deleted", n, "error", err) + } + return + } + if n > 0 { + logger.Info("pruned expired rows", "pruner", name, "deleted", n, + "retention", retention.String(), "cutoff", cutoff.UTC().Format(time.RFC3339)) + } + } + + sweep() + ticker := time.NewTicker(interval) + defer ticker.Stop() + for { + select { + case <-ctx.Done(): + return + case <-ticker.C: + sweep() + } + } +} diff --git a/internal/prune/prune_test.go b/internal/prune/prune_test.go new file mode 100644 --- /dev/null +++ b/internal/prune/prune_test.go @@ -0,0 +1,50 @@ +package prune + +import ( + "context" + "io" + "log/slog" + "testing" + "time" +) + +func discardLogger() *slog.Logger { + return slog.New(slog.NewTextHandler(io.Discard, nil)) +} + +// A non-positive retention must fail closed: cutoff would be now-or-future, so +// a sweep would delete everything every tick. Run must refuse and never invoke +// the prune fn (this is the branch whose comment warns a zero retention would +// delete the whole table on each pass). +func TestRunFailsClosedOnNonPositiveRetention(t *testing.T) { + for _, retention := range []time.Duration{0, -time.Hour} { + calls := 0 + fn := func(_ context.Context, _ time.Time) (int64, error) { + calls++ + return 0, nil + } + Run(context.Background(), "test", retention, time.Hour, fn, discardLogger()) + if calls != 0 { + t.Fatalf("retention %s: prune fn ran %d time(s), want 0 (fail closed)", retention, calls) + } + } +} + +// A positive retention sweeps once synchronously before the first tick. Cancel +// the context from inside that first sweep so the loop exits immediately after +// it — no real interval elapses, so the test stays fast and non-flaky. +func TestRunSweepsOnceBeforeFirstTick(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + calls := 0 + fn := func(_ context.Context, _ time.Time) (int64, error) { + calls++ + cancel() // stop the loop right after the immediate pre-tick sweep + return 0, nil + } + // Interval is an hour, so only the synchronous sweep can run before ctx is done. + Run(ctx, "test", time.Hour, time.Hour, fn, discardLogger()) + if calls != 1 { + t.Fatalf("prune fn ran %d time(s), want exactly 1 synchronous sweep", calls) + } +} diff --git a/internal/ratelimit/ratelimit.go b/internal/ratelimit/ratelimit.go new file mode 100644 --- /dev/null +++ b/internal/ratelimit/ratelimit.go @@ -0,0 +1,163 @@ +// Package ratelimit is the bridge's shared admission-control primitive: a +// keyed token-bucket set (golang.org/x/time buckets keyed by client IP, +// signer id, or any other admission key) with idle-bucket sweeping and a +// hard fail-closed cap. It started life as the votes XRPC per-IP limiter +// (task 07) and was extracted in task 11 so the inbox and the public sync +// surface enforce the same discipline: +// +// - stale buckets are swept inline, at most once per sweep interval, so a +// flood of distinct keys cannot turn every request into an O(n) scan +// under the mutex (the limiter must never become the DoS vector); +// - the bucket map is hard-capped and FAIL-CLOSED: at the cap, requests +// from unknown keys are refused. The cap is only reachable during a +// key-rotation flood (one IPv6 /64 is effectively unlimited addresses), +// refusing unknown keys leaves established clients untouched, and +// admitting them instead would hand every rotated key a fresh full +// burst — exactly the abuse the limiter exists to stop. Idle buckets +// age out via the TTL sweep, so a genuinely new client is locked out +// only while the flood lasts. +package ratelimit + +import ( + "net" + "net/http" + "sync" + "sync/atomic" + "time" + + "golang.org/x/time/rate" +) + +// Sampler throttles noisy log lines: it admits at most one event per its +// interval, lock-free, so a refusal flood stays VISIBLE (a sampled Warn) +// without drowning the log — the paired expvar counter carries the true +// rate. Shared by every admission surface (inbox, sync) that logs refusals. +type Sampler struct { + last atomic.Int64 + interval int64 +} + +// NewSampler builds a Sampler admitting at most one event per interval. +func NewSampler(interval time.Duration) *Sampler { + return &Sampler{interval: int64(interval)} +} + +// Allow reports whether an event may be logged now, advancing the window +// when it returns true. Concurrent callers within one window: at most one +// wins. +func (s *Sampler) Allow(now time.Time) bool { + n := now.UnixNano() + prev := s.last.Load() + if n-prev < s.interval { + return false + } + return s.last.CompareAndSwap(prev, n) +} + +// Defaults for the sweeping/cap tunables. Exported so consumers' tests can +// reason about them; the rate and burst themselves have no default — every +// surface must choose its own. +const ( + // DefaultIdleTTL is how long an idle bucket survives before a sweep may + // reclaim it (a reclaimed bucket restarts full, which only ever favors + // the client). + DefaultIdleTTL = 10 * time.Minute + // DefaultSweepInterval throttles sweeps to at most one full-map scan per + // interval, however hot the surface runs. + DefaultSweepInterval = time.Minute + // DefaultSweepThreshold is the map size below which sweeps don't bother + // running: a few thousand lingering buckets cost less than the scans + // that would reclaim them. + DefaultSweepThreshold = 10_000 + // DefaultMaxBuckets hard-caps the bucket map (see the package comment + // for why the cap fails closed). + DefaultMaxBuckets = 50_000 +) + +// Limiter is a keyed token-bucket set. Construct with New; the exported +// tunables may be adjusted before first use (tests shrink them). +type Limiter struct { + perSecond rate.Limit + burst int + + // Tunables default to the package constants. They are plain fields (like + // Now) so tests can shrink them; mutate only before concurrent use. + IdleTTL time.Duration + SweepInterval time.Duration + SweepThreshold int + MaxBuckets int + // Now is the clock (test seam). + Now func() time.Time + + mu sync.Mutex + lastSweep time.Time + buckets map[string]*bucket +} + +type bucket struct { + limiter *rate.Limiter + lastSeen time.Time +} + +// New builds a Limiter allowing perSecond sustained requests per key with +// the given burst. +func New(perSecond float64, burst int) *Limiter { + return &Limiter{ + perSecond: rate.Limit(perSecond), + burst: burst, + IdleTTL: DefaultIdleTTL, + SweepInterval: DefaultSweepInterval, + SweepThreshold: DefaultSweepThreshold, + MaxBuckets: DefaultMaxBuckets, + Now: time.Now, + buckets: map[string]*bucket{}, + } +} + +// Allow reports whether one request from key fits in its bucket. Unknown +// keys are refused outright while the bucket map sits at MaxBuckets (fail +// closed; see the package comment). +func (l *Limiter) Allow(key string) bool { + l.mu.Lock() + defer l.mu.Unlock() + now := l.Now() + + if len(l.buckets) >= l.SweepThreshold && now.Sub(l.lastSweep) >= l.SweepInterval { + l.lastSweep = now + for k, b := range l.buckets { + if now.Sub(b.lastSeen) > l.IdleTTL { + delete(l.buckets, k) + } + } + } + + b, ok := l.buckets[key] + if !ok { + if len(l.buckets) >= l.MaxBuckets { + return false + } + b = &bucket{limiter: rate.NewLimiter(l.perSecond, l.burst)} + l.buckets[key] = b + } + b.lastSeen = now + return b.limiter.AllowN(now, 1) +} + +// Size reports the current bucket count (tests). +func (l *Limiter) Size() int { + l.mu.Lock() + defer l.mu.Unlock() + return len(l.buckets) +} + +// ClientIP extracts the connection's remote IP from a request. Deliberately +// not X-Forwarded-For: the bridge cannot know which proxies to trust, and a +// spoofable header would let one client exhaust every bucket. Deployments +// behind a load balancer rate-limit the real client at the edge. +func ClientIP(r *http.Request) string { + host, _, err := net.SplitHostPort(r.RemoteAddr) + if err != nil { + return r.RemoteAddr + } + return host +} diff --git a/internal/ratelimit/ratelimit_test.go b/internal/ratelimit/ratelimit_test.go new file mode 100644 --- /dev/null +++ b/internal/ratelimit/ratelimit_test.go @@ -0,0 +1,98 @@ +package ratelimit + +import ( + "fmt" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// testClock is a hand-cranked clock for driving the limiter's TTL, sweep, +// and refill paths deterministically. +type testClock struct{ t time.Time } + +func newTestClock() *testClock { + return &testClock{t: time.Date(2026, 7, 8, 12, 0, 0, 0, time.UTC)} +} + +func (c *testClock) now() time.Time { return c.t } +func (c *testClock) advance(d time.Duration) { c.t = c.t.Add(d) } + +func TestLimiterIndependentBuckets(t *testing.T) { + limiter := New(0.001, 1) + + require.True(t, limiter.Allow("192.0.2.1")) + require.False(t, limiter.Allow("192.0.2.1"), "the first key's burst is spent") + assert.True(t, limiter.Allow("192.0.2.2"), "a second key must have its own bucket") +} + +func TestLimiterHardCap(t *testing.T) { + limiter := New(0.001, 1) + limiter.MaxBuckets = 3 + + // Many distinct keys: the map must never grow past the cap. + for i := 0; i < 50; i++ { + limiter.Allow(fmt.Sprintf("2001:db8::%x", i)) + require.LessOrEqual(t, limiter.Size(), 3, "after key %d", i) + } + assert.Equal(t, 3, limiter.Size()) + + // At the cap: unknown keys are refused, established ones still tracked. + assert.False(t, limiter.Allow("198.51.100.9"), "unknown key at cap is refused") + assert.False(t, limiter.Allow("2001:db8::0"), + "a known key still hits its own (spent) bucket, not the cap") +} + +func TestLimiterCapRecoversAfterSweep(t *testing.T) { + clock := newTestClock() + limiter := New(0.001, 1) + limiter.Now = clock.now + limiter.MaxBuckets = 2 + limiter.SweepThreshold = 1 + + require.True(t, limiter.Allow("192.0.2.1")) + require.True(t, limiter.Allow("192.0.2.2")) + require.False(t, limiter.Allow("192.0.2.3"), "the map is at its cap") + + // Once the residents idle past the TTL and the sweep throttle window + // passes, the next request sweeps them out and the new key fits. + clock.advance(DefaultIdleTTL + DefaultSweepInterval) + assert.True(t, limiter.Allow("192.0.2.3")) + assert.Equal(t, 1, limiter.Size()) +} + +func TestLimiterSweepThrottled(t *testing.T) { + clock := newTestClock() + limiter := New(0.001, 1) + limiter.Now = clock.now + limiter.SweepThreshold = 1 + limiter.IdleTTL = time.Second + + limiter.Allow("192.0.2.1") + limiter.Allow("192.0.2.2") // crosses the threshold: sweeps, stamps lastSweep + + // Both buckets idle past the (tiny) TTL, but the throttle window has + // not elapsed — the next Allow must not sweep them. + clock.advance(2 * time.Second) + limiter.Allow("192.0.2.3") + assert.Equal(t, 3, limiter.Size(), "no sweep inside the throttle window") + + // Past the window, the sweep runs and reclaims every idle bucket. + clock.advance(DefaultSweepInterval) + limiter.Allow("192.0.2.4") + assert.Equal(t, 1, limiter.Size(), "idle buckets reclaimed once the window passes") +} + +func TestLimiterRefillFollowsClock(t *testing.T) { + clock := newTestClock() + limiter := New(1, 1) + limiter.Now = clock.now + + require.True(t, limiter.Allow("192.0.2.1")) + require.False(t, limiter.Allow("192.0.2.1"), "burst spent, no time has passed") + + clock.advance(1500 * time.Millisecond) + assert.True(t, limiter.Allow("192.0.2.1"), "one token refills after a second") +} diff --git a/internal/repo/blob.go b/internal/repo/blob.go --- a/internal/repo/blob.go +++ b/internal/repo/blob.go @@ -57,6 +57,45 @@ Size: int64(len(data)), }, nil } +// DeleteBlob removes one stored blob. Missing rows are a no-op success +// (scrubs are idempotent). Task 11's Delete(Actor)/nobridge scrub uses this +// for blobs the deleted actor's records referenced — including post images +// stored under COMMUNITY DIDs, which record deletion alone never touches. +// Caveat, accepted: blobs are content-addressed, so if two different records +// in the same repo embedded byte-identical media, scrubbing one deletes the +// blob out from under the other (a missing image, not missing content) — +// tracking cross-record blob references is not worth that edge at bridge +// scale. +func (m *Manager) DeleteBlob(ctx context.Context, did, cidStr string) error { + if _, err := cid.Parse(cidStr); err != nil { + return errors.NewValidationError("cid", err.Error()) + } + if _, err := m.db.ExecContext(ctx, + `DELETE FROM blobs WHERE did = $1 AND cid = $2`, did, cidStr); err != nil { + return fmt.Errorf("repo: delete blob %s for %s: %w", cidStr, did, err) + } + return nil +} + +// DeleteBlobsForDID removes every blob stored under a DID — the terminal +// Delete(Actor) path for the actor's own (now frozen) repo, where getBlob +// already refuses to serve and nothing can reference the bytes again. It +// returns how many rows were deleted. +func (m *Manager) DeleteBlobsForDID(ctx context.Context, did string) (int64, error) { + if _, err := syntax.ParseDID(did); err != nil { + return 0, errors.NewValidationError("did", err.Error()) + } + res, err := m.db.ExecContext(ctx, `DELETE FROM blobs WHERE did = $1`, did) + if err != nil { + return 0, fmt.Errorf("repo: delete blobs for %s: %w", did, err) + } + n, err := res.RowsAffected() + if err != nil { + return 0, fmt.Errorf("repo: delete blobs for %s: rows affected: %w", did, err) + } + return n, nil +} + // GetBlob reads a stored blob by (did, cid). A missing blob is an error // satisfying errors.IsNotFound. com.atproto.sync.getBlob serves this. func (m *Manager) GetBlob(ctx context.Context, did, cidStr string) (data []byte, mimeType string, err error) { diff --git a/internal/repo/events.go b/internal/repo/events.go --- a/internal/repo/events.go +++ b/internal/repo/events.go @@ -9,11 +9,14 @@ "fmt" "strconv" indigorepo "github.com/bluesky-social/indigo/atproto/repo" + "github.com/bluesky-social/indigo/atproto/syntax" blockformat "github.com/ipfs/go-block-format" "github.com/ipfs/go-cid" car "github.com/ipld/go-car" carutil "github.com/ipld/go-car/util" + + "tidepool/internal/errors" ) // OpAction is the kind of record mutation an Op describes. The values @@ -112,6 +115,60 @@ if _, err := tx.ExecContext(ctx, `SELECT pg_notify($1, $2)`, FirehoseNotifyChannel, strconv.FormatInt(seq, 10)); err != nil { return 0, fmt.Errorf("repo: notify firehose event %d: %w", seq, err) } + return seq, nil +} + +// AppendAccountEvent appends an #account row to the firehose log: the +// account-state signal (active=false + status) subscribers need to purge a +// repo instead of inferring its death from scrub delete-commits. It takes +// the same global commit advisory lock as record commits, so account rows +// share the "seq order == visibility order" guarantee and can never +// interleave incorrectly with the scrub commits that precede them. status +// uses the com.atproto account-status vocabulary (AccountStatusDeleted for +// the terminal consent flip); it is stored only when active is false. +func (m *Manager) AppendAccountEvent(ctx context.Context, did string, active bool, status string) (int64, error) { + if _, err := syntax.ParseDID(did); err != nil { + return 0, errors.NewValidationError("did", err.Error()) + } + + lock := m.lockFor(did) + lock.Lock() + defer lock.Unlock() + + tx, err := m.db.BeginTx(ctx, nil) + if err != nil { + return 0, fmt.Errorf("repo: begin tx: %w", err) + } + defer func() { _ = tx.Rollback() }() + + if _, err := tx.ExecContext(ctx, + `SELECT pg_advisory_xact_lock($1)`, commitAdvisoryLockKey); err != nil { + return 0, fmt.Errorf("repo: take commit advisory lock: %w", err) + } + + var statusValue any + if !active && status != "" { + statusValue = status + } + var seq int64 + // clock_timestamp() for the same reason as commit events: stamp + // visibility time, not transaction start (see appendFirehoseEvent). + if err := tx.QueryRowContext(ctx, ` + INSERT INTO firehose_events (kind, did, account_active, account_status, created_at) + VALUES ('account', $1, $2, $3, clock_timestamp()) + RETURNING seq`, + did, active, statusValue).Scan(&seq); err != nil { + return 0, fmt.Errorf("repo: append account event for %s: %w", did, err) + } + if _, err := tx.ExecContext(ctx, + `SELECT pg_notify($1, $2)`, FirehoseNotifyChannel, strconv.FormatInt(seq, 10)); err != nil { + return 0, fmt.Errorf("repo: notify account event %d: %w", seq, err) + } + if err := tx.Commit(); err != nil { + return 0, fmt.Errorf("repo: commit account event for %s: %w", did, err) + } + m.logger.Info("account event appended to firehose", + "did", did, "active", active, "status", status, "seq", seq) return seq, nil } diff --git a/internal/repo/hardening_test.go b/internal/repo/hardening_test.go new file mode 100644 --- /dev/null +++ b/internal/repo/hardening_test.go @@ -0,0 +1,209 @@ +package repo + +import ( + "context" + "database/sql" + stderrors "errors" + "fmt" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "tidepool/internal/errors" +) + +// Task 11 hardening tests: account events on the durable log, the +// transactional PutRecord side effect, blob deletion, and batched pruning. + +func TestAppendAccountEvent(t *testing.T) { + manager, _, _ := testManager(t) + ctx := t.Context() + + // A commit first, so ordering (account AFTER the scrub commits) is + // observable. + commit, err := manager.PutRecord(ctx, testDID, testCollection, testRKey(1), testRecord("doomed")) + require.NoError(t, err) + + seq, err := manager.AppendAccountEvent(ctx, testDID, false, AccountStatusDeleted) + require.NoError(t, err) + assert.Greater(t, seq, commit.Seq, "account event sequenced after the commit") + + events, err := manager.ListEvents(ctx, 0, 10) + require.NoError(t, err) + require.Len(t, events, 2) + + assert.Equal(t, EventKindCommit, events[0].Kind) + assert.Equal(t, commit.Seq, events[0].Seq) + + account := events[1] + assert.Equal(t, EventKindAccount, account.Kind) + assert.Equal(t, seq, account.Seq) + assert.Equal(t, testDID, account.DID) + assert.False(t, account.AccountActive) + assert.Equal(t, AccountStatusDeleted, account.AccountStatus) + assert.Empty(t, account.CommitCID, "account rows carry no commit payload") + assert.Empty(t, account.CAR) + assert.Empty(t, account.Ops) + + // SeqBounds counts account events like any other retained event. + oldest, newest, err := manager.SeqBounds(ctx) + require.NoError(t, err) + assert.Equal(t, commit.Seq, oldest) + assert.Equal(t, seq, newest) + + // Malformed DID fails closed. + _, err = manager.AppendAccountEvent(ctx, "not-a-did", false, AccountStatusDeleted) + require.Error(t, err) + assert.True(t, errors.IsValidation(err)) +} + +// TestFirehoseEventKindShapeCheck pins the kind-shape CHECK (migration 011) +// at the DB level with raw inserts: an account row must carry account_active, +// a commit row must not carry account fields, and an ACTIVE account frame +// must not carry a status (status is inactive-only in the model/emitter). +func TestFirehoseEventKindShapeCheck(t *testing.T) { + _, database, _ := testManager(t) + + // (a) kind='account' with NULL account_active is rejected. + _, err := database.Exec(` + INSERT INTO firehose_events (kind, did, account_active, account_status, created_at) + VALUES ('account', $1, NULL, NULL, NOW())`, testDID) + require.Error(t, err, "account row must carry a non-NULL account_active") + + // (b) kind='commit' carrying account fields is rejected (even with an + // otherwise well-formed commit payload). + _, err = database.Exec(` + INSERT INTO firehose_events (kind, did, commit_cid, rev, ops, car, account_active, created_at) + VALUES ('commit', $1, 'bafyreib2rxk3rybk3aobmv5cjuql3bm2twh4jo5uxgf5kpqrsqxi3jgxte', + 'rev1', '[]'::jsonb, ''::bytea, false, NOW())`, testDID) + require.Error(t, err, "commit row must not carry account fields") + + // (c) kind='account' active=true with a non-NULL status is rejected. + _, err = database.Exec(` + INSERT INTO firehose_events (kind, did, account_active, account_status, created_at) + VALUES ('account', $1, true, 'deleted', NOW())`, testDID) + require.Error(t, err, "an active account frame must not carry a status") + + // Control: the well-formed shapes the emitter actually writes are + // accepted — an inactive+deleted frame and an active+NULL-status frame. + _, err = database.Exec(` + INSERT INTO firehose_events (kind, did, account_active, account_status, created_at) + VALUES ('account', $1, false, 'deleted', NOW())`, testDID) + require.NoError(t, err, "inactive account frame with a status is valid") + _, err = database.Exec(` + INSERT INTO firehose_events (kind, did, account_active, account_status, created_at) + VALUES ('account', $1, true, NULL, NOW())`, testDID) + require.NoError(t, err, "active account frame with NULL status is valid") +} + +// TestPutRecordTxSideEffectAtomicity: the side effect and the record write +// land together or not at all, and the hook also runs on the NoOp re-put. +func TestPutRecordTxSideEffectAtomicity(t *testing.T) { + manager, database, _ := testManager(t) + ctx := t.Context() + rkey := testRKey(2) + + // A failing side effect rolls the record write back — no record, no + // repo, no firehose event. + sentinel := stderrors.New("side effect refused") + _, err := manager.PutRecordTx(ctx, testDID, testCollection, rkey, testRecord("phantom"), + func(context.Context, *sql.Tx, *CommitResult) error { return sentinel }) + require.ErrorIs(t, err, sentinel) + _, _, err = manager.GetRecord(ctx, testDID, testCollection, rkey) + assert.True(t, errors.IsNotFound(err), "record must not survive a failed side effect") + var eventCount int + require.NoError(t, database.QueryRow(`SELECT COUNT(*) FROM firehose_events`).Scan(&eventCount)) + assert.Zero(t, eventCount, "no firehose event for a rolled-back commit") + + // A successful side effect observes the commit result and can write in + // the same transaction. + var hookRes *CommitResult + res, err := manager.PutRecordTx(ctx, testDID, testCollection, rkey, testRecord("real"), + func(_ context.Context, tx *sql.Tx, r *CommitResult) error { + hookRes = r + // Prove tx is live and the firehose row is already visible in it. + var n int + return tx.QueryRowContext(ctx, + `SELECT COUNT(*) FROM firehose_events WHERE seq = $1`, r.Seq).Scan(&n) + }) + require.NoError(t, err) + require.NotNil(t, hookRes) + assert.Equal(t, res.RecordCID, hookRes.RecordCID) + assert.False(t, hookRes.NoOp) + + // The idempotent re-put still runs the hook (bookkeeping refresh), with + // NoOp set and no new firehose event. + hookRes = nil + res2, err := manager.PutRecordTx(ctx, testDID, testCollection, rkey, testRecord("real"), + func(_ context.Context, _ *sql.Tx, r *CommitResult) error { + hookRes = r + return nil + }) + require.NoError(t, err) + require.NotNil(t, hookRes, "the side effect must run on the NoOp path too") + assert.True(t, hookRes.NoOp) + assert.Equal(t, res.RecordCID, res2.RecordCID) + require.NoError(t, database.QueryRow(`SELECT COUNT(*) FROM firehose_events`).Scan(&eventCount)) + assert.Equal(t, 1, eventCount, "a NoOp re-put emits no new event") +} + +func TestDeleteBlobAndDeleteBlobsForDID(t *testing.T) { + manager, _, _ := testManager(t) + ctx := t.Context() + + blob1, err := manager.PutBlob(ctx, testDID, "image/png", []byte("png-bytes-1")) + require.NoError(t, err) + blob2, err := manager.PutBlob(ctx, testDID, "image/png", []byte("png-bytes-2")) + require.NoError(t, err) + const otherDID = "did:plc:yk4dd2qkboz2yv6tpubpc6co" + other, err := manager.PutBlob(ctx, otherDID, "image/png", []byte("png-bytes-3")) + require.NoError(t, err) + + cid1 := blob1.Ref.String() + require.NoError(t, manager.DeleteBlob(ctx, testDID, cid1)) + _, _, err = manager.GetBlob(ctx, testDID, cid1) + assert.True(t, errors.IsNotFound(err)) + // Idempotent: deleting again is a no-op success. + require.NoError(t, manager.DeleteBlob(ctx, testDID, cid1)) + + n, err := manager.DeleteBlobsForDID(ctx, testDID) + require.NoError(t, err) + assert.Equal(t, int64(1), n, "only the remaining blob under the DID") + _, _, err = manager.GetBlob(ctx, testDID, blob2.Ref.String()) + assert.True(t, errors.IsNotFound(err)) + + // The other DID's blob is untouched. + data, _, err := manager.GetBlob(ctx, otherDID, other.Ref.String()) + require.NoError(t, err) + assert.Equal(t, []byte("png-bytes-3"), data) +} + +// TestPruneEventsBatched drives PruneEvents across multiple DELETE batches +// (rows inserted raw — committing 2500 real records would dominate the +// suite's runtime) and checks the retained suffix stays contiguous. +func TestPruneEventsBatched(t *testing.T) { + manager, database, _ := testManager(t) + ctx := t.Context() + + const expired = 2500 // > 2 × pruneEventsBatchSize + _, err := database.Exec(fmt.Sprintf(` + INSERT INTO firehose_events (did, commit_cid, rev, ops, car, created_at) + SELECT '%s', 'bafyreib2rxk3rybk3aobmv5cjuql3bm2twh4jo5uxgf5kpqrsqxi3jgxte', + 'rev' || n, '[]'::jsonb, ''::bytea, NOW() - INTERVAL '2 days' + FROM generate_series(1, %d) AS n`, testDID, expired)) + require.NoError(t, err) + // One fresh commit that must survive. + fresh, err := manager.PutRecord(ctx, testDID, testCollection, testRKey(3), testRecord("fresh")) + require.NoError(t, err) + + n, err := manager.PruneEvents(ctx, time.Now().Add(-24*time.Hour)) + require.NoError(t, err) + assert.Equal(t, int64(expired), n) + + events, err := manager.ListEvents(ctx, 0, 10) + require.NoError(t, err) + require.Len(t, events, 1) + assert.Equal(t, fresh.Seq, events[0].Seq) +} diff --git a/internal/repo/repo.go b/internal/repo/repo.go --- a/internal/repo/repo.go +++ b/internal/repo/repo.go @@ -131,12 +131,27 @@ // existed, so no new commit or firehose event was produced. NoOp bool } +// TxSideEffect runs INSIDE the commit transaction, after the record write +// and its firehose event but before COMMIT — the seam that makes "record + +// its bookkeeping row" atomic (the materializer's ap_objects mapping rides +// here). An error rolls the whole commit back, record included. The hook +// also runs on the idempotent NoOp path (res.NoOp == true, Seq == 0), so +// bookkeeping refreshes even when no new commit is produced. Keep hooks +// fast: the global commit advisory lock is held while they run. +type TxSideEffect func(ctx context.Context, tx *sql.Tx, res *CommitResult) error + // PutRecord creates or updates a record and commits the change. The first // write to a DID creates its repo (genesis commit). Re-putting an identical // record is an idempotent no-op: no new commit or firehose event is // emitted, and the result carries NoOp with the existing CID, head, and rev // (deterministic rkeys make re-ingestion hit this path on purpose). func (m *Manager) PutRecord(ctx context.Context, did, collection, rkey string, record map[string]any) (*CommitResult, error) { + return m.PutRecordTx(ctx, did, collection, rkey, record, nil) +} + +// PutRecordTx is PutRecord with a side effect executed inside the commit +// transaction (see TxSideEffect). A nil sideEffect is exactly PutRecord. +func (m *Manager) PutRecordTx(ctx context.Context, did, collection, rkey string, record map[string]any, sideEffect TxSideEffect) (*CommitResult, error) { if err := validateRecord(record); err != nil { return nil, err } @@ -148,14 +163,14 @@ c, err := cidForBlock(recordBytes) if err != nil { return nil, err } - return m.commitWrite(ctx, did, collection, rkey, &c, recordBytes) + return m.commitWrite(ctx, did, collection, rkey, &c, recordBytes, sideEffect) } // DeleteRecord removes a record and commits the change. A missing record — // or a repo that does not exist yet — is an error satisfying // errors.IsNotFound. The result's RecordCID is empty. func (m *Manager) DeleteRecord(ctx context.Context, did, collection, rkey string) (*CommitResult, error) { - return m.commitWrite(ctx, did, collection, rkey, nil, nil) + return m.commitWrite(ctx, did, collection, rkey, nil, nil, nil) } // GetRecord reads the current version of a record. Missing repo or record @@ -238,7 +253,7 @@ // and bytes, DeleteRecord passes nil. It serializes on the per-DID mutex // and the global commit advisory lock, applies the mutation to the MST, // signs a new commit, and persists blocks, head, and the firehose event in // one transaction. -func (m *Manager) commitWrite(ctx context.Context, did, collection, rkey string, newCID *cid.Cid, recordBytes []byte) (*CommitResult, error) { +func (m *Manager) commitWrite(ctx context.Context, did, collection, rkey string, newCID *cid.Cid, recordBytes []byte, sideEffect TxSideEffect) (*CommitResult, error) { path, parsedDID, err := validatePath(did, collection, rkey) if err != nil { return nil, err @@ -315,12 +330,23 @@ } if newCID != nil && op.Prev != nil && op.Prev.Equals(*newCID) { // Identical re-put: idempotent no-op, keep the existing commit. // op.Prev != nil implies the repo exists, so state is non-nil here. - return &CommitResult{ + res := &CommitResult{ RecordCID: newCID.String(), CommitCID: state.headCID, Rev: prevRev, NoOp: true, - }, nil + } + if sideEffect != nil { + // The side effect still runs (bookkeeping refresh) and must still + // be durable, so the — otherwise write-free — transaction commits. + if err := sideEffect(ctx, tx, res); err != nil { + return nil, err + } + if err := tx.Commit(); err != nil { + return nil, fmt.Errorf("repo: commit no-op side effect for %s: %w", did, err) + } + } + return res, nil } // New blocks this commit introduces: MST diff nodes + the record block @@ -404,12 +430,6 @@ if err != nil { return nil, err } - if err := tx.Commit(); err != nil { - return nil, fmt.Errorf("repo: commit tx for %s: %w", did, err) - } - - m.logger.Debug("repo commit", - "did", did, "rev", rev.String(), "commit", commitCID.String(), "path", path, "seq", seq) res := &CommitResult{ CommitCID: commitCID.String(), Rev: rev.String(), @@ -418,6 +438,21 @@ } if newCID != nil { res.RecordCID = newCID.String() } + if sideEffect != nil { + // Inside the transaction: a failing side effect rolls the record + // write back too — record and bookkeeping land together or not at + // all. + if err := sideEffect(ctx, tx, res); err != nil { + return nil, err + } + } + + if err := tx.Commit(); err != nil { + return nil, fmt.Errorf("repo: commit tx for %s: %w", did, err) + } + + m.logger.Debug("repo commit", + "did", did, "rev", rev.String(), "commit", commitCID.String(), "path", path, "seq", seq) return res, nil } diff --git a/internal/repo/sync.go b/internal/repo/sync.go --- a/internal/repo/sync.go +++ b/internal/repo/sync.go @@ -35,13 +35,34 @@ // order, exactly matching seq order. internal/sync's broadcaster LISTENs on // this channel to wake subscriber outboxes without polling. const FirehoseNotifyChannel = "tidepool_firehose" +// Event kinds (firehose_events.kind). Commit rows carry the #commit frame +// payload; account rows carry an #account frame (active/status) and no +// commit columns. +const ( + EventKindCommit = "commit" + EventKindAccount = "account" +) + +// Account status tokens for #account frames (the com.atproto.sync.defs +// account-status vocabulary the bridge emits). Deleted is what Delete(Actor) +// / consent revocation uses: bigsky marks the repo tombstoned — dropping it +// from listRepos — and purges its carstore data, which is exactly the +// downstream purge the consent flip wants. +const ( + AccountStatusDeleted = "deleted" +) + // Event is one row of the durable subscribeRepos backlog, as read back for -// serving. Field semantics match the #commit frame of -// com.atproto.sync.subscribeRepos (see migration 006 for the storage notes). +// serving. For Kind == EventKindCommit the field semantics match the +// #commit frame of com.atproto.sync.subscribeRepos (see migration 006 for +// the storage notes); for EventKindAccount only Seq, DID, CreatedAt, +// AccountActive, and AccountStatus are meaningful. type Event struct { // Seq is the firehose cursor (bigserial; gapless in visibility order, // though individual integers may be skipped by rolled-back writes). Seq int64 + // Kind discriminates commit vs account rows (EventKind*). + Kind string // DID is the repo the commit belongs to (the frame's `repo` field). DID string // CommitCID is the signed commit block's CID. @@ -61,6 +82,10 @@ // block first, then MST diff nodes and record blocks). CAR []byte // CreatedAt is when the commit landed (the frame's `time`). CreatedAt time.Time + // AccountActive / AccountStatus carry the #account frame payload for + // Kind == EventKindAccount rows. + AccountActive bool + AccountStatus string } // ListEvents returns up to limit firehose events with seq > sinceSeq, in seq @@ -72,7 +97,8 @@ if limit <= 0 { return nil, errors.NewValidationError("limit", "must be positive") } rows, err := m.db.QueryContext(ctx, ` - SELECT seq, did, commit_cid, prev_data_cid, since_rev, rev, ops, car, created_at + SELECT seq, kind, did, commit_cid, prev_data_cid, since_rev, rev, ops, car, created_at, + account_active, account_status FROM firehose_events WHERE seq > $1 ORDER BY seq @@ -86,12 +112,21 @@ var out []*Event for rows.Next() { var ev Event var opsJSON []byte - if err := rows.Scan(&ev.Seq, &ev.DID, &ev.CommitCID, &ev.PrevDataCID, - &ev.SinceRev, &ev.Rev, &opsJSON, &ev.CAR, &ev.CreatedAt); err != nil { + var commitCID, rev, accountStatus sql.NullString + var accountActive sql.NullBool + if err := rows.Scan(&ev.Seq, &ev.Kind, &ev.DID, &commitCID, &ev.PrevDataCID, + &ev.SinceRev, &rev, &opsJSON, &ev.CAR, &ev.CreatedAt, + &accountActive, &accountStatus); err != nil { return nil, fmt.Errorf("repo: scan firehose event: %w", err) } - if err := json.Unmarshal(opsJSON, &ev.Ops); err != nil { - return nil, fmt.Errorf("repo: decode ops for seq %d: %w", ev.Seq, err) + ev.CommitCID = commitCID.String + ev.Rev = rev.String + ev.AccountActive = accountActive.Bool + ev.AccountStatus = accountStatus.String + if len(opsJSON) > 0 { + if err := json.Unmarshal(opsJSON, &ev.Ops); err != nil { + return nil, fmt.Errorf("repo: decode ops for seq %d: %w", ev.Seq, err) + } } out = append(out, &ev) } @@ -152,26 +187,52 @@ } return lastValue + 1, lastValue, nil } +// pruneEventsBatchSize bounds one DELETE statement inside PruneEvents. One +// unbatched DELETE over a large expired prefix holds row locks (and bloats +// one WAL transaction) for the whole sweep; batching keeps each statement +// short so commits — which contend on the same table — never stall behind +// retention. +const pruneEventsBatchSize = 1000 + // PruneEvents deletes firehose events older than the cutoff. It prunes a // strict seq prefix — everything up to the newest expired seq — rather than // filtering on created_at row-by-row: commit-transaction start times are not // perfectly ordered with seq assignment (CURRENT_TIMESTAMP is the tx start, // the advisory lock is taken after BeginTx), and retention must never punch // holes in the retained suffix, because subscribeRepos replay treats -// [oldest, newest] as complete. It returns the number of events deleted. +// [oldest, newest] as complete. The DELETE runs in batches (each its own +// implicit transaction) from the oldest seq up, so a partial sweep still +// leaves a contiguous retained suffix. It returns the number of events +// deleted. func (m *Manager) PruneEvents(ctx context.Context, cutoff time.Time) (int64, error) { - res, err := m.db.ExecContext(ctx, ` - DELETE FROM firehose_events WHERE seq <= ( - SELECT COALESCE(MAX(seq), 0) FROM firehose_events WHERE created_at < $1 - )`, cutoff) - if err != nil { - return 0, fmt.Errorf("repo: prune firehose events before %s: %w", cutoff.Format(time.RFC3339), err) + // The boundary is computed once: everything at or below it is expired. + var boundary int64 + if err := m.db.QueryRowContext(ctx, ` + SELECT COALESCE(MAX(seq), 0) FROM firehose_events WHERE created_at < $1`, + cutoff).Scan(&boundary); err != nil { + return 0, fmt.Errorf("repo: find prune boundary before %s: %w", cutoff.Format(time.RFC3339), err) + } + if boundary == 0 { + return 0, nil } - n, err := res.RowsAffected() - if err != nil { - return 0, fmt.Errorf("repo: prune firehose events: rows affected: %w", err) + var total int64 + for { + res, err := m.db.ExecContext(ctx, ` + DELETE FROM firehose_events WHERE seq IN ( + SELECT seq FROM firehose_events WHERE seq <= $1 ORDER BY seq LIMIT $2 + )`, boundary, pruneEventsBatchSize) + if err != nil { + return total, fmt.Errorf("repo: prune firehose events before %s: %w", cutoff.Format(time.RFC3339), err) + } + n, err := res.RowsAffected() + if err != nil { + return total, fmt.Errorf("repo: prune firehose events: rows affected: %w", err) + } + total += n + if n < pruneEventsBatchSize { + return total, nil + } } - return n, nil } // GetRecordProof builds the com.atproto.sync.getRecord response: a CARv1 @@ -306,7 +367,10 @@ HeadCID string Rev string Active bool // Status is the lexicon's account-status token when inactive - // ("deactivated"); empty when active. + // ("deleted" — the bridged actor was tombstoned by Delete(Actor)/ + // consent revocation, which is terminal); empty when active. The same + // token rides the #account frame, so the HTTP surface and the firehose + // agree. Status string } @@ -369,7 +433,7 @@ return nil, fmt.Errorf("repo: scan repo info: %w", err) } info.Active = consent != string(store.ConsentStateDeleted) if !info.Active { - info.Status = "deactivated" + info.Status = AccountStatusDeleted } return &info, nil } diff --git a/internal/repo/sync_test.go b/internal/repo/sync_test.go --- a/internal/repo/sync_test.go +++ b/internal/repo/sync_test.go @@ -246,7 +246,7 @@ testRecord(fmt.Sprintf("repo %d", i))) require.NoError(t, err) } - // Tombstone the middle DID's actor: its repo must report deactivated. + // Tombstone the middle DID's actor: its repo must report inactive (status "deleted"). actors := store.NewBridgedActors(database) _, err := actors.UpsertActor(ctx, store.BridgedActor{ APActorID: "https://lemmy.example/u/tombstoned", @@ -272,12 +272,12 @@ assert.Empty(t, page1[0].Status) assert.NotEmpty(t, page1[0].HeadCID) assert.NotEmpty(t, page1[0].Rev) assert.False(t, page1[1].Active, "tombstoned actor's repo must be inactive") - assert.Equal(t, "deactivated", page1[1].Status) + assert.Equal(t, "deleted", page1[1].Status) info, err := manager.GetRepoInfo(ctx, dids[1]) require.NoError(t, err) assert.False(t, info.Active) - assert.Equal(t, "deactivated", info.Status) + assert.Equal(t, "deleted", info.Status) head, rev, err := manager.Head(ctx, dids[0]) require.NoError(t, err) diff --git a/internal/store/ap_objects.go b/internal/store/ap_objects.go --- a/internal/store/ap_objects.go +++ b/internal/store/ap_objects.go @@ -26,6 +26,22 @@ id, ap_id, ap_type, origin_instance, origin, did, author_did, collection, rkey, at_uri, cid, ap_published_at, indexed_at, deleted_at` func (r *postgresAPObjects) PutMapping(ctx context.Context, mapping APObjectMapping) (*APObjectMapping, error) { + return r.putMapping(ctx, r.db, mapping) +} + +func (r *postgresAPObjects) PutMappingTx(ctx context.Context, tx *sql.Tx, mapping APObjectMapping) (*APObjectMapping, error) { + if tx == nil { + return nil, errors.NewValidationError("tx", "must not be nil") + } + return r.putMapping(ctx, tx, mapping) +} + +// queryRower is the slice of *sql.DB / *sql.Tx putMapping needs. +type queryRower interface { + QueryRowContext(ctx context.Context, query string, args ...any) *sql.Row +} + +func (r *postgresAPObjects) putMapping(ctx context.Context, q queryRower, mapping APObjectMapping) (*APObjectMapping, error) { if err := validateMapping(&mapping); err != nil { return nil, err } @@ -49,7 +65,7 @@ indexed_at = CURRENT_TIMESTAMP, deleted_at = NULL RETURNING` + apObjectColumns - row := r.db.QueryRowContext(ctx, query, + row := q.QueryRowContext(ctx, query, mapping.APID, mapping.APType, mapping.OriginInstance, string(mapping.Origin), mapping.DID, nullIfEmpty(mapping.AuthorDID), mapping.Collection, mapping.RKey, mapping.ATURI, mapping.CID, mapping.PublishedAt, diff --git a/internal/store/communities.go b/internal/store/communities.go --- a/internal/store/communities.go +++ b/internal/store/communities.go @@ -23,7 +23,8 @@ } const communityColumns = ` id, ap_group_id, did, preferred_username, instance, - follow_state, followed_at, last_backfill_at, created_at` + follow_state, followed_at, last_backfill_at, created_at, + follow_requested_at, follow_attempts` func (r *postgresCommunities) UpsertCommunity(ctx context.Context, community Community) (*Community, error) { if err := validateCommunity(&community); err != nil { @@ -110,6 +111,13 @@ // followed_at stamps only on the transition INTO accepted — AP happily // redelivers Accept, and a re-accept must not re-stamp the original // time. none clears it; pending leaves it alone. + // + // The follow-retry bookkeeping (task 11) rides the same statement: + // every set-to-pending means "a Follow was just sent" (admin subscribe + // and the retrier both send before recording), so it stamps + // follow_requested_at and increments follow_attempts; none resets both + // (a fresh subscription starts a fresh budget); accepted leaves them + // for post-mortems. query := ` UPDATE communities SET followed_at = CASE @@ -117,6 +125,16 @@ WHEN $2 = 'accepted' AND follow_state <> 'accepted' THEN CURRENT_TIMESTAMP WHEN $2 = 'none' THEN NULL ELSE followed_at END, + follow_requested_at = CASE + WHEN $2 = 'pending' THEN CURRENT_TIMESTAMP + WHEN $2 = 'none' THEN NULL + ELSE follow_requested_at + END, + follow_attempts = CASE + WHEN $2 = 'pending' THEN follow_attempts + 1 + WHEN $2 = 'none' THEN 0 + ELSE follow_attempts + END, follow_state = $2 WHERE ap_group_id = $1` @@ -178,6 +196,54 @@ } return communities, nil } +func (r *postgresCommunities) ClaimStalePendingFollows(ctx context.Context, requestedBefore time.Time, maxAttempts int) ([]*Community, error) { + if maxAttempts <= 0 { + return nil, errors.NewValidationError("max_attempts", "must be positive") + } + // One atomic conditional claim: the UPDATE consumes an attempt and + // re-stamps follow_requested_at on exactly the rows it matches, and + // RETURNING hands back only those (post-increment) rows for the retrier + // to send. Because the claim IS the match, a concurrent sweep and an + // Accept can never be clobbered: + // - two overlapping sweeps: the second UPDATE re-evaluates its WHERE + // against the first sweep's just-written follow_requested_at (row is + // row-locked until the first commits), which is no longer < the + // cutoff, so it claims nothing — no double-send; + // - an Accept between sweeps flips follow_state to 'accepted', which + // the WHERE excludes, so the claim leaves it untouched (no downgrade + // back to pending). + // NULL follow_requested_at (legacy rows from before migration 012) is + // treated as stale: a pending row with no recorded send time has, by + // definition, waited longer than any threshold. + query := ` + UPDATE communities + SET follow_attempts = follow_attempts + 1, + follow_requested_at = CURRENT_TIMESTAMP + WHERE follow_state = 'pending' + AND follow_attempts < $2 + AND (follow_requested_at IS NULL OR follow_requested_at < $1) + RETURNING` + communityColumns + + rows, err := r.db.QueryContext(ctx, query, requestedBefore, maxAttempts) + if err != nil { + return nil, fmt.Errorf("claim stale pending follows: %w", err) + } + defer func() { _ = rows.Close() }() + + var communities []*Community + for rows.Next() { + community, err := scanCommunity(rows) + if err != nil { + return nil, fmt.Errorf("claim stale pending follows: scan: %w", err) + } + communities = append(communities, community) + } + if err := rows.Err(); err != nil { + return nil, fmt.Errorf("claim stale pending follows: rows: %w", err) + } + return communities, nil +} + func validateCommunity(community *Community) error { if community.APGroupID == "" { return errors.NewValidationError("ap_group_id", "must not be empty") @@ -208,6 +274,7 @@ &community.ID, &community.APGroupID, &community.DID, &community.PreferredUsername, &community.Instance, &followState, &community.FollowedAt, &community.LastBackfillAt, &community.CreatedAt, + &community.FollowRequestedAt, &community.FollowAttempts, ) if err != nil { return nil, err diff --git a/internal/store/hardening_test.go b/internal/store/hardening_test.go new file mode 100644 --- /dev/null +++ b/internal/store/hardening_test.go @@ -0,0 +1,204 @@ +package store + +import ( + "context" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "tidepool/internal/errors" + "tidepool/internal/testutil" +) + +// Task 11 hardening tests: tombstone pruning, follow-retry bookkeeping, and +// the transactional mapping write. + +func TestTombstonesPrune(t *testing.T) { + database := testutil.DB(t) + testutil.Truncate(t, database, "ap_tombstones") + tombstones := NewTombstones(database) + ctx := context.Background() + + for _, id := range []string{"https://l.test/post/old1", "https://l.test/post/old2", "https://l.test/post/fresh"} { + require.NoError(t, tombstones.Record(ctx, id)) + } + _, err := database.Exec( + `UPDATE ap_tombstones SET deleted_at = NOW() - INTERVAL '40 days' WHERE ap_id LIKE '%old%'`) + require.NoError(t, err) + + n, err := tombstones.Prune(ctx, time.Now().Add(-30*24*time.Hour)) + require.NoError(t, err) + assert.Equal(t, int64(2), n) + + gone, err := tombstones.Exists(ctx, "https://l.test/post/old1") + require.NoError(t, err) + assert.False(t, gone, "aged marker pruned") + kept, err := tombstones.Exists(ctx, "https://l.test/post/fresh") + require.NoError(t, err) + assert.True(t, kept, "fresh marker survives") + + // Nothing left to prune: zero, no error. + n, err = tombstones.Prune(ctx, time.Now().Add(-30*24*time.Hour)) + require.NoError(t, err) + assert.Zero(t, n) +} + +func TestCommunitiesFollowRetryBookkeeping(t *testing.T) { + database := testutil.DB(t) + testutil.Truncate(t, database, "communities") + communities := NewCommunities(database) + ctx := context.Background() + + const groupIRI = "https://lemmy.test/c/retry" + _, err := communities.UpsertCommunity(ctx, Community{ + APGroupID: groupIRI, + DID: "did:plc:ewvi7nxzyoun6zhxrhs64oiz", + PreferredUsername: "retry", + Instance: "lemmy.test", + }) + require.NoError(t, err) + + // Every set-to-pending records one Follow send: stamp + increment. + require.NoError(t, communities.SetFollowState(ctx, groupIRI, FollowStatePending)) + stored, err := communities.GetByAPGroupID(ctx, groupIRI) + require.NoError(t, err) + require.NotNil(t, stored.FollowRequestedAt, "pending must stamp follow_requested_at") + assert.Equal(t, 1, stored.FollowAttempts) + + require.NoError(t, communities.SetFollowState(ctx, groupIRI, FollowStatePending)) + stored, err = communities.GetByAPGroupID(ctx, groupIRI) + require.NoError(t, err) + assert.Equal(t, 2, stored.FollowAttempts, "a re-send consumes an attempt") + + // Accept preserves the bookkeeping (post-mortems); none resets it. + require.NoError(t, communities.SetFollowState(ctx, groupIRI, FollowStateAccepted)) + stored, err = communities.GetByAPGroupID(ctx, groupIRI) + require.NoError(t, err) + assert.Equal(t, 2, stored.FollowAttempts) + require.NotNil(t, stored.FollowRequestedAt) + + require.NoError(t, communities.SetFollowState(ctx, groupIRI, FollowStateNone)) + stored, err = communities.GetByAPGroupID(ctx, groupIRI) + require.NoError(t, err) + assert.Zero(t, stored.FollowAttempts, "unsubscribe resets the budget") + assert.Nil(t, stored.FollowRequestedAt) +} + +func TestClaimStalePendingFollows(t *testing.T) { + database := testutil.DB(t) + testutil.Truncate(t, database, "communities") + communities := NewCommunities(database) + ctx := context.Background() + + seed := func(slug, did string) string { + iri := "https://lemmy.test/c/" + slug + _, err := communities.UpsertCommunity(ctx, Community{ + APGroupID: iri, DID: did, PreferredUsername: slug, Instance: "lemmy.test", + }) + require.NoError(t, err) + require.NoError(t, communities.SetFollowState(ctx, iri, FollowStatePending)) + return iri + } + stale := seed("stale", "did:plc:ewvi7nxzyoun6zhxrhs64oiz") + fresh := seed("fresh", "did:plc:yk4dd2qkboz2yv6tpubpc6co") + exhausted := seed("exhausted", "did:plc:44ybard66vv44zksje25o7dz") + + // Age the stale and exhausted rows past any threshold; burn the + // exhausted row's budget. + _, err := database.Exec(` + UPDATE communities SET follow_requested_at = NOW() - INTERVAL '1 hour' + WHERE ap_group_id IN ($1, $2)`, stale, exhausted) + require.NoError(t, err) + _, err = database.Exec( + `UPDATE communities SET follow_attempts = 5 WHERE ap_group_id = $1`, exhausted) + require.NoError(t, err) + + // The claim returns only the stale, under-budget pending row — and as a + // side effect consumes its attempt and re-stamps follow_requested_at. + got, err := communities.ClaimStalePendingFollows(ctx, time.Now().Add(-time.Minute), 5) + require.NoError(t, err) + require.Len(t, got, 1, "only the stale, under-budget pending row qualifies") + assert.Equal(t, stale, got[0].APGroupID) + assert.Equal(t, 2, got[0].FollowAttempts, "the claim returns the post-increment attempt count") + _ = fresh + + // The claim is not idempotent-read: immediately re-claiming finds nothing + // (the row it just claimed was re-stamped fresh — no double-send). + got, err = communities.ClaimStalePendingFollows(ctx, time.Now().Add(-time.Minute), 5) + require.NoError(t, err) + assert.Empty(t, got, "a just-claimed row is fresh again and must not be re-claimed") + + // A NULL follow_requested_at (legacy pending row) counts as stale and is + // claimed. + _, err = database.Exec( + `UPDATE communities SET follow_requested_at = NULL WHERE ap_group_id = $1`, fresh) + require.NoError(t, err) + got, err = communities.ClaimStalePendingFollows(ctx, time.Now().Add(-time.Minute), 5) + require.NoError(t, err) + require.Len(t, got, 1) + assert.Equal(t, fresh, got[0].APGroupID) + + // An Accept landing between sweeps (accepted state) is never claimed, so + // the claim can never downgrade it back to pending. + require.NoError(t, communities.SetFollowState(ctx, stale, FollowStateAccepted)) + _, err = database.Exec(` + UPDATE communities SET follow_requested_at = NOW() - INTERVAL '1 hour' + WHERE ap_group_id = $1`, stale) + require.NoError(t, err) + got, err = communities.ClaimStalePendingFollows(ctx, time.Now().Add(-time.Minute), 5) + require.NoError(t, err) + for _, c := range got { + assert.NotEqual(t, stale, c.APGroupID, "an accepted row must never be claimed") + } + accepted, err := communities.GetByAPGroupID(ctx, stale) + require.NoError(t, err) + assert.Equal(t, FollowStateAccepted, accepted.FollowState, "claim must not clobber accepted back to pending") + + _, err = communities.ClaimStalePendingFollows(ctx, time.Now(), 0) + require.Error(t, err) + assert.True(t, errors.IsValidation(err)) +} + +func TestPutMappingTxAtomicity(t *testing.T) { + database := testutil.DB(t) + testutil.Truncate(t, database, "ap_objects") + objects := NewAPObjects(database) + ctx := context.Background() + + mapping := APObjectMapping{ + APID: "https://lemmy.test/post/tx1", + APType: "Page", + OriginInstance: "lemmy.test", + DID: "did:plc:ewvi7nxzyoun6zhxrhs64oiz", + Collection: "social.coves.community.post", + RKey: "3jzfcijpj2z2a", + CID: "bafyreib2rxk3rybk3aobmv5cjuql3bm2twh4jo5uxgf5kpqrsqxi3jgxte", + } + + // A rolled-back transaction leaves no mapping behind. + tx, err := database.BeginTx(ctx, nil) + require.NoError(t, err) + _, err = objects.PutMappingTx(ctx, tx, mapping) + require.NoError(t, err) + require.NoError(t, tx.Rollback()) + _, err = objects.GetByAPID(ctx, mapping.APID) + assert.True(t, errors.IsNotFound(err), "rollback must discard the mapping") + + // A committed one persists. + tx, err = database.BeginTx(ctx, nil) + require.NoError(t, err) + stored, err := objects.PutMappingTx(ctx, tx, mapping) + require.NoError(t, err) + require.NoError(t, tx.Commit()) + assert.NotEmpty(t, stored.ATURI) + got, err := objects.GetByAPID(ctx, mapping.APID) + require.NoError(t, err) + assert.Equal(t, stored.ATURI, got.ATURI) + + // Nil tx is refused. + _, err = objects.PutMappingTx(ctx, nil, mapping) + require.Error(t, err) + assert.True(t, errors.IsValidation(err)) +} diff --git a/internal/store/interfaces.go b/internal/store/interfaces.go --- a/internal/store/interfaces.go +++ b/internal/store/interfaces.go @@ -10,6 +10,7 @@ package store import ( "context" + "database/sql" "time" ) @@ -25,6 +26,11 @@ // explicitly). Re-putting an existing APID updates APType, Origin, DID, // Collection, RKey, ATURI, CID, and PublishedAt, refreshes IndexedAt, // and clears any soft delete (re-materialization revives the mapping). PutMapping(ctx context.Context, mapping APObjectMapping) (*APObjectMapping, error) + + // PutMappingTx is PutMapping executed on an existing transaction — the + // seam repo.TxSideEffect uses so a record commit and its mapping land + // atomically (task 11 closed the PutRecord→PutMapping crash window). + PutMappingTx(ctx context.Context, tx *sql.Tx, mapping APObjectMapping) (*APObjectMapping, error) // GetByAPID returns the mapping for an AP object id, including // soft-deleted rows (callers can check IsDeleted to detect tombstones). @@ -135,6 +141,20 @@ // ListByFollowState returns all communities in the given follow state, // ordered by creation time. ListByFollowState(ctx context.Context, state FollowState) ([]*Community, error) + + // ClaimStalePendingFollows atomically claims communities stuck in pending + // whose last Follow went out before requestedBefore (rows with a NULL + // follow_requested_at — legacy pending rows — are included) and that have + // consumed fewer than maxAttempts Follow sends. Claiming is a single + // UPDATE ... RETURNING that increments follow_attempts and re-stamps + // follow_requested_at on exactly the matched rows, row-locked, so + // overlapping sweeps never double-claim and an Accept that flipped a row + // to accepted between sweeps is never clobbered back to pending (the + // WHERE only matches follow_state='pending'). The returned rows carry the + // post-increment follow_attempts and are the ones the retrier must send a + // fresh Follow to. The follow retrier's work query (Lemmy first-contact + // Accept race). + ClaimStalePendingFollows(ctx context.Context, requestedBefore time.Time, maxAttempts int) ([]*Community, error) } // ServiceKeys persists the bridge's own long-lived keys (today: the service @@ -144,8 +164,9 @@ // signatures already in flight. type ServiceKeys interface { // Create inserts a new named key and returns the stored row. An existing // name returns an error satisfying errors.IsAlreadyExists — callers that - // lose a bootstrap race must Get the winner's key instead. - Create(ctx context.Context, name string, privateKeyPEM []byte) (*ServiceKey, error) + // lose a bootstrap race must Get the winner's key instead. keyMaterial's + // encoding is the caller's contract (see ServiceKey.KeyMaterial). + Create(ctx context.Context, name string, keyMaterial []byte) (*ServiceKey, error) // Get returns the key for a purpose name. A missing key is an error // satisfying errors.IsNotFound. @@ -231,4 +252,12 @@ // Remove clears the marker (Undo{Delete}/restore). Removing a missing // marker is a no-op success. Remove(ctx context.Context, apID string) error + + // Prune deletes markers recorded before the cutoff, in batches, and + // returns how many were deleted. Retention trade-off, accepted: a + // pruned marker re-opens the create-after-delete window for THAT id, + // but out-of-order re-deliveries happen minutes apart, not months — + // TOMBSTONE_RETENTION's default (30 days) is orders of magnitude above + // any observed redelivery horizon. + Prune(ctx context.Context, cutoff time.Time) (int64, error) } diff --git a/internal/store/models.go b/internal/store/models.go --- a/internal/store/models.go +++ b/internal/store/models.go @@ -131,16 +131,27 @@ FollowState FollowState FollowedAt *time.Time LastBackfillAt *time.Time CreatedAt time.Time + // FollowRequestedAt is when the most recent Follow activity went out + // (stamped on every transition to pending — subscribe and automatic + // re-send alike); FollowAttempts counts those sends since the last + // unsubscribe. Together they bound the follow retrier (task 11, the + // Lemmy first-contact Accept race). + FollowRequestedAt *time.Time + FollowAttempts int } // ServiceKey is one of the bridge's own long-lived keys, keyed by purpose -// name (e.g. "service-actor" for the AP-side RSA signing key, stored as -// PKCS#8 PEM). +// name. KeyMaterial's encoding is per-row: plaintext PKCS#8 PEM for +// "service-actor" (the AP-side RSA signing key — the bridge's own service +// credential, not user key material), AES-GCM sealed ciphertext for +// "plc-rotation" (the escrow rotation key, sealed under BRIDGE_KEK by +// identity.Custodian). The column was renamed from private_key_pem in +// migration 013 because "PEM" lied for the sealed row. type ServiceKey struct { - ID int64 - Name string - PrivateKeyPEM []byte - CreatedAt time.Time + ID int64 + Name string + KeyMaterial []byte + CreatedAt time.Time } // InboxEvent is a received AP activity: the dedupe record AND the durable diff --git a/internal/store/service_keys.go b/internal/store/service_keys.go --- a/internal/store/service_keys.go +++ b/internal/store/service_keys.go @@ -18,22 +18,22 @@ func NewServiceKeys(db *sql.DB) ServiceKeys { return &postgresServiceKeys{db: db} } -const serviceKeyColumns = ` id, name, private_key_pem, created_at` +const serviceKeyColumns = ` id, name, key_material, created_at` -func (r *postgresServiceKeys) Create(ctx context.Context, name string, privateKeyPEM []byte) (*ServiceKey, error) { +func (r *postgresServiceKeys) Create(ctx context.Context, name string, keyMaterial []byte) (*ServiceKey, error) { if name == "" { return nil, errors.NewValidationError("name", "must not be empty") } - if len(privateKeyPEM) == 0 { - return nil, errors.NewValidationError("private_key_pem", "must not be empty") + if len(keyMaterial) == 0 { + return nil, errors.NewValidationError("key_material", "must not be empty") } query := ` - INSERT INTO service_keys (name, private_key_pem) + INSERT INTO service_keys (name, key_material) VALUES ($1, $2) RETURNING` + serviceKeyColumns - key, err := scanServiceKey(r.db.QueryRowContext(ctx, query, name, privateKeyPEM)) + key, err := scanServiceKey(r.db.QueryRowContext(ctx, query, name, keyMaterial)) if err != nil { // Keys are create-once by design: a concurrent bootstrap losing the // insert race must re-Get the winner's key, never overwrite it. @@ -59,7 +59,7 @@ } func scanServiceKey(row rowScanner) (*ServiceKey, error) { var key ServiceKey - if err := row.Scan(&key.ID, &key.Name, &key.PrivateKeyPEM, &key.CreatedAt); err != nil { + if err := row.Scan(&key.ID, &key.Name, &key.KeyMaterial, &key.CreatedAt); err != nil { return nil, err } return &key, nil diff --git a/internal/store/service_keys_test.go b/internal/store/service_keys_test.go --- a/internal/store/service_keys_test.go +++ b/internal/store/service_keys_test.go @@ -19,13 +19,13 @@ created, err := repo.Create(ctx, "service-actor", pemBytes) require.NoError(t, err) assert.NotZero(t, created.ID) assert.Equal(t, "service-actor", created.Name) - assert.Equal(t, pemBytes, created.PrivateKeyPEM) + assert.Equal(t, pemBytes, created.KeyMaterial) assert.False(t, created.CreatedAt.IsZero()) got, err := repo.Get(ctx, "service-actor") require.NoError(t, err) assert.Equal(t, created.ID, got.ID) - assert.Equal(t, pemBytes, got.PrivateKeyPEM) + assert.Equal(t, pemBytes, got.KeyMaterial) } func TestServiceKeys_CreateExistingNameConflicts(t *testing.T) { @@ -45,7 +45,7 @@ assert.True(t, errors.IsAlreadyExists(err), "second create must satisfy IsAlreadyExists, got %v", err) got, err := repo.Get(ctx, "service-actor") require.NoError(t, err) - assert.Equal(t, original, got.PrivateKeyPEM, "losing create must not clobber the stored key") + assert.Equal(t, original, got.KeyMaterial, "losing create must not clobber the stored key") } func TestServiceKeys_GetMissingIsNotFound(t *testing.T) { @@ -78,5 +78,5 @@ require.NoError(t, err) got, err := repo.Get(ctx, "some-future-key") require.NoError(t, err) - assert.Equal(t, []byte("key-b"), got.PrivateKeyPEM) + assert.Equal(t, []byte("key-b"), got.KeyMaterial) } diff --git a/internal/store/tombstones.go b/internal/store/tombstones.go --- a/internal/store/tombstones.go +++ b/internal/store/tombstones.go @@ -4,6 +4,7 @@ import ( "context" "database/sql" "fmt" + "time" "tidepool/internal/errors" ) @@ -53,3 +54,28 @@ return fmt.Errorf("remove tombstone %q: %w", apID, err) } return nil } + +// tombstonePruneBatchSize bounds one DELETE inside Prune (same reasoning as +// the firehose pruner: short statements never stall the write path). +const tombstonePruneBatchSize = 1000 + +func (r *postgresTombstones) Prune(ctx context.Context, cutoff time.Time) (int64, error) { + var total int64 + for { + res, err := r.db.ExecContext(ctx, ` + DELETE FROM ap_tombstones WHERE ap_id IN ( + SELECT ap_id FROM ap_tombstones WHERE deleted_at < $1 LIMIT $2 + )`, cutoff, tombstonePruneBatchSize) + if err != nil { + return total, fmt.Errorf("prune tombstones before %s: %w", cutoff.Format(time.RFC3339), err) + } + n, err := res.RowsAffected() + if err != nil { + return total, fmt.Errorf("prune tombstones: rows affected: %w", err) + } + total += n + if n < tombstonePruneBatchSize { + return total, nil + } + } +} diff --git a/internal/sync/frames.go b/internal/sync/frames.go --- a/internal/sync/frames.go +++ b/internal/sync/frames.go @@ -78,6 +78,24 @@ }, }, nil } +// accountFrame converts a stored account event (repo.EventKindAccount) into +// the subscribeRepos #account message: the signal relays use to update +// account state (bigsky tombstones the repo and purges its data on +// status "deleted") instead of inferring it from scrub delete-commits. +func accountFrame(ev *repo.Event) *events.XRPCStreamEvent { + account := &comatproto.SyncSubscribeRepos_Account{ + Seq: ev.Seq, + Did: ev.DID, + Active: ev.AccountActive, + Time: ev.CreatedAt.UTC().Format(frameTimeFormat), + } + if !ev.AccountActive && ev.AccountStatus != "" { + status := ev.AccountStatus + account.Status = &status + } + return &events.XRPCStreamEvent{RepoAccount: account} +} + // infoFrame builds a #info message (e.g. OutdatedCursor). func infoFrame(name, message string) *events.XRPCStreamEvent { info := &comatproto.SyncSubscribeRepos_Info{Name: name} diff --git a/internal/sync/hardening_test.go b/internal/sync/hardening_test.go new file mode 100644 --- /dev/null +++ b/internal/sync/hardening_test.go @@ -0,0 +1,129 @@ +package sync + +import ( + "context" + "encoding/json" + "fmt" + "net/http" + "testing" + "time" + + "github.com/gorilla/websocket" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "tidepool/internal/repo" +) + +// Task 11 hardening tests: the #account frame on the wire, the +// subscribeRepos connection cap, and the per-IP rate limit on the public +// surface. + +// TestSubscribeRepos_AccountFrame: an account event appended to the durable +// log is served as a real #account frame — replayed from a cursor AND +// live-tailed — with the commit that preceded it arriving first (per-repo +// order: scrub commits, then the purge signal). +func TestSubscribeRepos_AccountFrame(t *testing.T) { + h := newHarness(t) + + commit := putRecord(t, h, "acct1", "doomed record") + seq, err := h.manager.AppendAccountEvent(context.Background(), testDID, false, repo.AccountStatusDeleted) + require.NoError(t, err) + + // Replay path: cursor 0 serves the commit, then the account frame. + conn := h.dial(t, "0") + evt := readFrame(t, conn, 5*time.Second) + require.NotNil(t, evt.RepoCommit, "the scrub commit precedes the account frame") + assert.Equal(t, commit.Seq, evt.RepoCommit.Seq) + + evt = readFrame(t, conn, 5*time.Second) + require.NotNil(t, evt.RepoAccount, "expected an #account frame") + assert.Equal(t, seq, evt.RepoAccount.Seq) + assert.Equal(t, testDID, evt.RepoAccount.Did) + assert.False(t, evt.RepoAccount.Active) + require.NotNil(t, evt.RepoAccount.Status) + assert.Equal(t, repo.AccountStatusDeleted, *evt.RepoAccount.Status) + assert.NotEmpty(t, evt.RepoAccount.Time) + + // Live-tail path: a second subscriber sees a fresh account event pushed + // through the broadcaster like any commit. + live := h.dial(t, "") + waitForSubscribers(t, h, 2) + liveSeq, err := h.manager.AppendAccountEvent(context.Background(), testDID, false, repo.AccountStatusDeleted) + require.NoError(t, err) + evt = readFrame(t, live, 5*time.Second) + require.NotNil(t, evt.RepoAccount) + assert.Equal(t, liveSeq, evt.RepoAccount.Seq) +} + +// TestSubscribeRepos_ConnectionCap: the cap refuses the N+1th concurrent +// subscriber with 429 and frees the slot when a connection closes. +func TestSubscribeRepos_ConnectionCap(t *testing.T) { + h := newHarnessWithPoll(t, 500*time.Millisecond, func(o *Options) { o.MaxSubscribers = 2 }) + + first := h.dial(t, "") + _ = h.dial(t, "") + waitForSubscribers(t, h, 2) + + _, resp, err := websocket.DefaultDialer.Dial(h.wsURL(""), nil) + require.Error(t, err, "the third concurrent subscriber must be refused") + require.NotNil(t, resp) + defer func() { _ = resp.Body.Close() }() + assert.Equal(t, http.StatusTooManyRequests, resp.StatusCode) + var body struct { + Error string `json:"error"` + } + require.NoError(t, json.NewDecoder(resp.Body).Decode(&body)) + assert.Equal(t, "SubscriberLimitExceeded", body.Error) + + // Closing one connection frees its slot. + require.NoError(t, first.Close()) + require.Eventually(t, func() bool { + conn, resp, err := websocket.DefaultDialer.Dial(h.wsURL(""), nil) + if resp != nil && resp.Body != nil { + _ = resp.Body.Close() + } + if err != nil { + return false + } + _ = conn.Close() + return true + }, 5*time.Second, 50*time.Millisecond, "a freed slot must admit a new subscriber") +} + +// TestSyncSurface_PerIPRateLimit: past the burst, HTTP reads answer 429 +// RateLimitExceeded; _health stays exempt (container healthchecks must +// never flap on a limiter). +func TestSyncSurface_PerIPRateLimit(t *testing.T) { + h := newHarnessWithPoll(t, 500*time.Millisecond, func(o *Options) { + o.RatePerSecond = 0.001 // effectively no refill inside the test + o.RateBurst = 3 + }) + + url := h.http.URL + "/xrpc/com.atproto.sync.listRepos" + status := func(u string) int { + resp, err := http.Get(u) + require.NoError(t, err) + defer func() { _ = resp.Body.Close() }() + return resp.StatusCode + } + + for i := 0; i < 3; i++ { + assert.Equal(t, http.StatusOK, status(url), "request %d within the burst", i+1) + } + resp, err := http.Get(url) + require.NoError(t, err) + defer func() { _ = resp.Body.Close() }() + assert.Equal(t, http.StatusTooManyRequests, resp.StatusCode) + var body struct { + Error string `json:"error"` + } + require.NoError(t, json.NewDecoder(resp.Body).Decode(&body)) + assert.Equal(t, "RateLimitExceeded", body.Error) + + // The limiter guards the whole surface, not just listRepos… + assert.Equal(t, http.StatusTooManyRequests, + status(h.http.URL+fmt.Sprintf("/xrpc/com.atproto.sync.getRepoStatus?did=%s", testDID))) + // …except the healthcheck probe. + assert.Equal(t, http.StatusOK, status(h.http.URL+"/xrpc/_health")) +} diff --git a/internal/sync/prune.go b/internal/sync/prune.go --- a/internal/sync/prune.go +++ b/internal/sync/prune.go @@ -5,59 +5,17 @@ "context" "log/slog" "time" + "tidepool/internal/prune" "tidepool/internal/repo" ) -// defaultPruneInterval is how often the retention job runs; retention -// windows are hours-to-days, so hourly sweeps keep the backlog tight without -// meaningful load. -const defaultPruneInterval = time.Hour - // RunPruner enforces FIREHOSE_RETENTION: it deletes firehose events older // than the retention window, once immediately and then every interval, until // ctx is canceled. Consumers whose cursor falls off the retained window get // an OutdatedCursor #info frame on reconnect (see streamEvents) — that is // the documented contract of a bounded replay window, matching relay -// rollback behavior. interval <= 0 selects the default. +// rollback behavior. interval <= 0 selects the default; retention <= 0 +// refuses to run (fail closed — see internal/prune). func RunPruner(ctx context.Context, mgr *repo.Manager, retention, interval time.Duration, logger *slog.Logger) { - if logger == nil { - logger = slog.Default() - } - if retention <= 0 { - // Fail closed: a zero retention would compute cutoff == now and - // delete the entire retained backlog every sweep. Config validates - // this too, but this function must not rely on one caller. - logger.Error("firehose pruner refusing to run", "retention", retention.String(), - "reason", "retention must be positive") - return - } - if interval <= 0 { - interval = defaultPruneInterval - } - prune := func() { - cutoff := time.Now().Add(-retention) - n, err := mgr.PruneEvents(ctx, cutoff) - if err != nil { - if ctx.Err() == nil { - logger.Error("firehose retention prune failed", "error", err) - } - return - } - if n > 0 { - logger.Info("pruned firehose events", - "deleted", n, "retention", retention.String(), "cutoff", cutoff.UTC().Format(time.RFC3339)) - } - } - - prune() - ticker := time.NewTicker(interval) - defer ticker.Stop() - for { - select { - case <-ctx.Done(): - return - case <-ticker.C: - prune() - } - } + prune.Run(ctx, "firehose_events", retention, interval, mgr.PruneEvents, logger) } diff --git a/internal/sync/server.go b/internal/sync/server.go --- a/internal/sync/server.go +++ b/internal/sync/server.go @@ -2,9 +2,11 @@ package sync import ( "encoding/json" + "expvar" "log/slog" "net/http" "strconv" + "sync/atomic" "time" comatproto "github.com/bluesky-social/indigo/api/atproto" @@ -12,9 +14,18 @@ "github.com/go-chi/chi/v5" "github.com/gorilla/websocket" "tidepool/internal/errors" + "tidepool/internal/ratelimit" "tidepool/internal/repo" ) +// SyncRateLimited counts public sync-surface requests refused by admission +// control — the per-client-IP 429s on the read/dial surface and the +// subscribeRepos connection-cap refusals. Published to the admin /metrics +// surface as "tidepool_sync_ratelimited" (mirrors +// materialize.ValidationFailures): a misconfigured tight limit silently +// dropping all sync traffic is otherwise invisible. +var SyncRateLimited = expvar.NewInt("tidepool_sync_ratelimited") + // healthVersion is reported by /xrpc/_health, mirroring what the reference // PDS exposes (crawlers and Jetstream use it as a liveness probe). const healthVersion = "tidepool 0.1" @@ -34,6 +45,20 @@ // listRepos limit bounds per com.atproto.sync.listRepos. defaultListReposLimit = 500 maxListReposLimit = 1000 + + // Admission-control defaults for the public sync surface (task 11, + // pre-internet-facing hardening). Sized as DoS backstops, not fairness + // controls: a relay indexing the bridge for the first time bursts + // getRepo/getRecord per new DID, so the burst absorbs a whole first + // crawl and the sustained rate stays far above anything a legitimate + // consumer needs (relays hold ONE WebSocket, not request storms). + defaultSyncRatePerSecond = 25 + defaultSyncRateBurst = 200 + // defaultMaxSubscribers caps concurrent subscribeRepos connections. A + // bridge has a handful of legitimate firehose consumers (relays, + // Jetstream, debug taps); each one costs a goroutine pair and a DB + // cursor, so the cap bounds that multiplication. + defaultMaxSubscribers = 100 ) // Server implements the sync XRPC surface over a repo.Manager. Zero-valued @@ -50,6 +75,15 @@ writeTimeout time.Duration pingInterval time.Duration replayBatch int + // limiter is the per-client-IP admission limiter for the whole public + // surface (HTTP reads and WebSocket dials alike); subscribers counts + // live subscribeRepos connections against maxSubscribers. + limiter *ratelimit.Limiter + maxSubscribers int64 + subscribers atomic.Int64 + // refusalLog samples the (otherwise noisy) rate-limit refusal Warn logs. + refusalLog *ratelimit.Sampler + // onUpgrade, when set, runs on every accepted subscribeRepos socket // before streaming starts. Test seam: the slow-consumer eviction test // shrinks the kernel send buffer so the write deadline is reachable @@ -72,6 +106,14 @@ WriteTimeout time.Duration PingInterval time.Duration ReplayBatch int + + // RatePerSecond / RateBurst tune the per-client-IP token bucket guarding + // every public sync endpoint (config SYNC_RATE_PER_SECOND / + // SYNC_RATE_BURST); zero values take generous defaults. MaxSubscribers + // caps concurrent subscribeRepos connections (SYNC_MAX_SUBSCRIBERS). + RatePerSecond float64 + RateBurst int + MaxSubscribers int } // NewServer validates options and builds the server. @@ -112,22 +154,56 @@ } if s.replayBatch <= 0 { s.replayBatch = defaultReplayBatch } + perSecond := opts.RatePerSecond + if perSecond <= 0 { + perSecond = defaultSyncRatePerSecond + } + burst := opts.RateBurst + if burst <= 0 { + burst = defaultSyncRateBurst + } + s.limiter = ratelimit.New(perSecond, burst) + s.refusalLog = ratelimit.NewSampler(time.Second) + s.maxSubscribers = int64(opts.MaxSubscribers) + if s.maxSubscribers <= 0 { + s.maxSubscribers = defaultMaxSubscribers + } return s, nil } // Routes mounts the sync surface on a chi router. resolveHandle and // /.well-known/atproto-did stay in cmd/tidepool (task 03 wired them; they -// belong to the identity package). +// belong to the identity package). Every endpoint except _health sits +// behind the per-IP admission limiter (_health is the container healthcheck +// probe — a rate-limited healthcheck would flap the whole stack, and it +// does no per-request work worth guarding). func (s *Server) Routes(r chi.Router) { - r.Get("/xrpc/com.atproto.sync.subscribeRepos", s.handleSubscribeRepos) - r.Get("/xrpc/com.atproto.sync.getRepo", s.handleGetRepo) - r.Get("/xrpc/com.atproto.sync.getLatestCommit", s.handleGetLatestCommit) - r.Get("/xrpc/com.atproto.sync.getRecord", s.handleGetRecord) - r.Get("/xrpc/com.atproto.sync.getBlob", s.handleGetBlob) - r.Get("/xrpc/com.atproto.sync.listRepos", s.handleListRepos) - r.Get("/xrpc/com.atproto.sync.getRepoStatus", s.handleGetRepoStatus) - r.Get("/xrpc/com.atproto.server.describeServer", s.handleDescribeServer) + r.Get("/xrpc/com.atproto.sync.subscribeRepos", s.limited(s.handleSubscribeRepos)) + r.Get("/xrpc/com.atproto.sync.getRepo", s.limited(s.handleGetRepo)) + r.Get("/xrpc/com.atproto.sync.getLatestCommit", s.limited(s.handleGetLatestCommit)) + r.Get("/xrpc/com.atproto.sync.getRecord", s.limited(s.handleGetRecord)) + r.Get("/xrpc/com.atproto.sync.getBlob", s.limited(s.handleGetBlob)) + r.Get("/xrpc/com.atproto.sync.listRepos", s.limited(s.handleListRepos)) + r.Get("/xrpc/com.atproto.sync.getRepoStatus", s.limited(s.handleGetRepoStatus)) + r.Get("/xrpc/com.atproto.server.describeServer", s.limited(s.handleDescribeServer)) r.Get("/xrpc/_health", s.handleHealth) +} + +// limited wraps a handler with the per-client-IP token bucket. Refusals are +// 429 RateLimitExceeded (the XRPC convention; sync consumers are pollers and +// reconnecting WebSocket clients, both of which handle 429 by backing off). +func (s *Server) limited(next http.HandlerFunc) http.HandlerFunc { + return func(w http.ResponseWriter, r *http.Request) { + if !s.limiter.Allow(ratelimit.ClientIP(r)) { + SyncRateLimited.Add(1) + if s.refusalLog.Allow(time.Now()) { + s.logger.Warn("sync request rate-limited (sampled)", "remote", r.RemoteAddr, "path", r.URL.Path) + } + writeXRPCError(w, http.StatusTooManyRequests, "RateLimitExceeded", "rate limit exceeded") + return + } + next(w, r) + } } // loadActiveRepo resolves the did query parameter to an active repo, diff --git a/internal/sync/subscribe.go b/internal/sync/subscribe.go --- a/internal/sync/subscribe.go +++ b/internal/sync/subscribe.go @@ -9,6 +9,8 @@ "time" "github.com/bluesky-social/indigo/events" "github.com/gorilla/websocket" + + "tidepool/internal/repo" ) // upgrader intentionally skips origin checks: subscribeRepos is a public @@ -58,6 +60,21 @@ return } cursor = &parsed } + + // Connection cap: reserve a slot before upgrading (reserve-then-check, + // so concurrent dials cannot all observe "one below the cap" and land + // together). Each live connection costs goroutines and a DB cursor; + // unbounded subscribers is the cheapest way to sink the bridge. + if s.subscribers.Add(1) > s.maxSubscribers { + s.subscribers.Add(-1) + SyncRateLimited.Add(1) + s.logger.Warn("subscribeRepos refused: connection cap reached", + "remote", r.RemoteAddr, "max", s.maxSubscribers) + writeXRPCError(w, http.StatusTooManyRequests, "SubscriberLimitExceeded", + "too many concurrent subscribers") + return + } + defer s.subscribers.Add(-1) conn, err := upgrader.Upgrade(w, r, nil) if err != nil { @@ -134,13 +151,14 @@ } // streamEvents is the outbox loop described on handleSubscribeRepos. // -// KNOWN GAP (deliberate, tracked for task 06): the stream carries only -// #commit frames and does not consult consent state, so a tombstoned actor's -// already-committed events remain replayable for the retention window even -// though the HTTP read surface refuses the repo. The atproto-native fix is -// emitting an #account{active:false} frame when consent flips (plus delete -// commits from the record scrub) so downstream consumers purge — that emit -// path must land with task 06's consent flows. +// The stream carries #commit and #account frames. Delete(Actor)/consent +// revocation appends an #account{active:false, status:"deleted"} event to +// the durable log (repo.AppendAccountEvent) after the scrub delete-commits, +// so consumers purge the repo instead of inferring its death. A tombstoned +// actor's HISTORICAL commit events stay replayable until retention expires — +// deliberate: replay windows are advertised as complete, and the #account +// frame (which replays with them, after them in seq order) is the purge +// signal. func (s *Server) streamEvents(ctx context.Context, conn *websocket.Conn, cursor *int64, logger *slog.Logger) { // Subscribe BEFORE reading the seq bounds: an event landing between the // bounds read and the first wait leaves a token in the wake channel, so @@ -227,7 +245,16 @@ } } } for _, ev := range batch { - frame, err := commitFrame(ev) + var frame *events.XRPCStreamEvent + var err error + switch ev.Kind { + case repo.EventKindAccount: + frame = accountFrame(ev) + default: + // EventKindCommit — and legacy rows written before the kind + // column existed, which default to it. + frame, err = commitFrame(ev) + } if err != nil { // A stored event that cannot be framed is data corruption; // dropping it silently would desync consumers, so terminate. diff --git a/internal/sync/sync_test.go b/internal/sync/sync_test.go --- a/internal/sync/sync_test.go +++ b/internal/sync/sync_test.go @@ -61,7 +61,9 @@ } // newHarnessWithPoll lets a test pick the broadcaster poll fallback — an // effectively-infinite interval makes the LISTEN/NOTIFY path load-bearing. -func newHarnessWithPoll(t *testing.T, pollInterval time.Duration) *harness { +// Optional mutators adjust the server Options before construction (the +// admission-control tests shrink limits with them). +func newHarnessWithPoll(t *testing.T, pollInterval time.Duration, mutate ...func(*Options)) *harness { t.Helper() database := testutil.DB(t) testutil.Truncate(t, database, "blocks", "repo_state", "firehose_events", "bridged_actors", "blobs") @@ -80,14 +82,18 @@ cancel() _ = broadcaster.Close() }) - server, err := NewServer(Options{ + opts := Options{ Repo: manager, Broadcaster: broadcaster, Hostname: testHostname, WriteTimeout: 2 * time.Second, PingInterval: time.Second, ReplayBatch: 10, - }) + } + for _, m := range mutate { + m(&opts) + } + server, err := NewServer(opts) require.NoError(t, err) router := chi.NewRouter() @@ -737,7 +743,7 @@ resp := getJSON(t, base+"/xrpc/com.atproto.sync.getRepoStatus?did="+testDID, &status) assert.Equal(t, http.StatusOK, resp.StatusCode) assert.False(t, status.Active) require.NotNil(t, status.Status) - assert.Equal(t, "deactivated", *status.Status) + assert.Equal(t, "deleted", *status.Status) } // TestGetBlob pins the com.atproto.sync.getBlob surface task 05 added: a diff --git a/internal/votes/aggregator.go b/internal/votes/aggregator.go --- a/internal/votes/aggregator.go +++ b/internal/votes/aggregator.go @@ -20,7 +20,9 @@ "database/sql" stderrors "errors" "fmt" "log/slog" + "sort" "strings" + "time" "tidepool/internal/ap" "tidepool/internal/errors" @@ -33,6 +35,15 @@ const ( directionUp = "up" directionDown = "down" ) + +// MaxSeededCount is the upper sanity cap on one seeded baseline value +// (task 11). Seeds come from a REMOTE instance's public API — a hostile or +// broken origin must not be able to inject absurd baselines into served +// scores. The largest scores on the biggest Lemmy instances are low five +// figures; one million is comfortably above anything real while making a +// deliberately poisoned 2^31 baseline a validation error the seeder logs +// and drops (the previous baseline survives). +const MaxSeededCount = 1_000_000 // RecordReader is the slice of *repo.Manager the aggregator uses to read a // bridged comment's stored record: the record's reply.root strongRef names @@ -341,6 +352,10 @@ } if upvotes < 0 || downvotes < 0 { return errors.NewValidationError("counts", "must not be negative") } + if upvotes > MaxSeededCount || downvotes > MaxSeededCount { + return errors.NewValidationError("counts", + fmt.Sprintf("exceed the seeded-count sanity cap of %d (up=%d down=%d)", MaxSeededCount, upvotes, downvotes)) + } mapping, err := a.subjectMapping(ctx, subjectAPID) if err != nil { @@ -368,6 +383,160 @@ } // Fold any live events that landed before the seed into the totals. return recomputeAggregate(ctx, tx, subjectAPID) }) +} + +// ScrubVoter erases every vote_events row a voter ever produced — the vote +// counterpart of the record scrub on Delete(Actor)/consent revocation +// (task 11; the rows tie an identifiable AP actor id to their voting +// history, which must not outlive their account). Live votes are removed +// from served counts too: every affected aggregate is locked (in +// deterministic subject order, so concurrent per-subject writers cannot +// deadlock against the scrub) and recomputed in the same transaction. +// Seeded baselines are untouched — they were never per-voter data. A voter +// with no rows is a no-op success. +func (a *Aggregator) ScrubVoter(ctx context.Context, voterAPID string) error { + if voterAPID == "" { + return errors.NewValidationError("voter_ap_id", "must not be empty") + } + return a.inTx(ctx, func(tx *sql.Tx) error { + // Lock the aggregates the voter is KNOWN to have voted on first, + // ordered, exactly like the single-subject paths do (lock → mutate → + // recompute). + locked := map[string]bool{} + rows, err := tx.QueryContext(ctx, ` + SELECT subject_ap_id + FROM vote_aggregates + WHERE subject_ap_id IN ( + SELECT DISTINCT subject_ap_id FROM vote_events WHERE voter_ap_id = $1 + ) + ORDER BY subject_ap_id + FOR UPDATE`, voterAPID) + if err != nil { + return fmt.Errorf("lock aggregates for voter %q: %w", voterAPID, err) + } + for rows.Next() { + var subject string + if err := rows.Scan(&subject); err != nil { + _ = rows.Close() + return fmt.Errorf("scan scrub subject for %q: %w", voterAPID, err) + } + locked[subject] = true + } + if err := rows.Err(); err != nil { + _ = rows.Close() + return fmt.Errorf("iterate scrub subjects for %q: %w", voterAPID, err) + } + _ = rows.Close() + + // DELETE ... RETURNING is the authoritative recompute set: it names + // EVERY subject a row was actually removed from. That is a superset of + // the pre-DELETE snapshot when a concurrent AddVote committed a vote on + // a not-previously-voted subject between the lock above and this DELETE + // — a subject that would otherwise be deleted-but-never-recomputed + // (phantom count). Recomputing only the snapshot is exactly the + // lost-update bug. + var deleted int64 + affected := map[string]bool{} + delRows, err := tx.QueryContext(ctx, + `DELETE FROM vote_events WHERE voter_ap_id = $1 RETURNING subject_ap_id`, voterAPID) + if err != nil { + return fmt.Errorf("scrub vote events for %q: %w", voterAPID, err) + } + for delRows.Next() { + var subject string + if err := delRows.Scan(&subject); err != nil { + _ = delRows.Close() + return fmt.Errorf("scan deleted scrub subject for %q: %w", voterAPID, err) + } + deleted++ + affected[subject] = true + } + if err := delRows.Err(); err != nil { + _ = delRows.Close() + return fmt.Errorf("iterate deleted scrub subjects for %q: %w", voterAPID, err) + } + _ = delRows.Close() + + // Newly appearing subjects (in the delete set but not the snapshot): + // lock them before recompute, in subject order, preserving the + // deterministic ORDER BY subject_ap_id lock discipline (task 07) so we + // do not introduce a deadlock. + var newSubjects []string + for subject := range affected { + if !locked[subject] { + newSubjects = append(newSubjects, subject) + } + } + sort.Strings(newSubjects) + for _, subject := range newSubjects { + var got string + err := tx.QueryRowContext(ctx, ` + SELECT subject_ap_id FROM vote_aggregates + WHERE subject_ap_id = $1 FOR UPDATE`, subject).Scan(&got) + if stderrors.Is(err, sql.ErrNoRows) { + // No aggregate row for this subject (the concurrent writer's + // row was itself removed): nothing to lock or recompute. + continue + } + if err != nil { + return fmt.Errorf("lock newly appearing aggregate %q for voter %q: %w", subject, voterAPID, err) + } + locked[subject] = true + } + + // Recompute every locked aggregate (snapshot ∪ newly-appearing rows = + // exactly the subjects with an aggregate row that lost one of this + // voter's votes), in deterministic subject order. + recomputeSet := make([]string, 0, len(locked)) + for subject := range locked { + recomputeSet = append(recomputeSet, subject) + } + sort.Strings(recomputeSet) + for _, subject := range recomputeSet { + if err := recomputeAggregate(ctx, tx, subject); err != nil { + return err + } + } + if deleted > 0 { + a.logger.Info("vote events scrubbed for deleted voter", + "voter", voterAPID, "events", deleted, "subjects", len(recomputeSet)) + } + return nil + }) +} + +// pruneVoteEventsBatchSize bounds one DELETE inside PruneUndoneEvents. +const pruneVoteEventsBatchSize = 1000 + +// PruneUndoneEvents deletes undone (superseded or retracted) vote_events +// rows older than the cutoff, in batches, returning how many were removed. +// Live rows are NEVER pruned — they are the counts. Retention trade-off, +// accepted and mirrored from ap_tombstones: an undone row is also the +// dedupe record for its activity id, so pruning re-opens replay for +// activities older than the retention window — but real federation queues +// redeliver over hours-to-days, not months, and VOTE_EVENT_RETENTION's +// default (90 days) dwarfs that horizon. +func (a *Aggregator) PruneUndoneEvents(ctx context.Context, cutoff time.Time) (int64, error) { + var total int64 + for { + result, err := a.db.ExecContext(ctx, ` + DELETE FROM vote_events WHERE id IN ( + SELECT id FROM vote_events + WHERE undone AND created_at < $1 + LIMIT $2 + )`, cutoff, pruneVoteEventsBatchSize) + if err != nil { + return total, fmt.Errorf("votes: prune undone events before %s: %w", cutoff.Format(time.RFC3339), err) + } + n, err := result.RowsAffected() + if err != nil { + return total, fmt.Errorf("votes: prune undone events: rows affected: %w", err) + } + total += n + if n < pruneVoteEventsBatchSize { + return total, nil + } + } } // subjectMapping resolves a voted-on AP id to its materialized mapping. It diff --git a/internal/votes/hardening_test.go b/internal/votes/hardening_test.go new file mode 100644 --- /dev/null +++ b/internal/votes/hardening_test.go @@ -0,0 +1,153 @@ +package votes + +import ( + "context" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "tidepool/internal/errors" +) + +// Task 11 hardening tests: the seeded-count sanity cap, the actor-delete +// vote scrub, and the undone-event pruner. + +// TestSeedAggregatesSanityCap: a hostile origin API must not be able to +// inject absurd baselines — values over MaxSeededCount are a validation +// error and the previous baseline survives. +func TestSeedAggregatesSanityCap(t *testing.T) { + database := testDB(t) + agg, objects := testAggregator(t, database) + bridgeSubject(t, objects, subjectPost, "3jzfcijpj2z2a") + ctx := context.Background() + + require.NoError(t, agg.SeedAggregates(ctx, subjectPost, 7, 2)) + + err := agg.SeedAggregates(ctx, subjectPost, MaxSeededCount+1, 0) + require.Error(t, err) + assert.True(t, errors.IsValidation(err), "an absurd upvote baseline is a validation error") + err = agg.SeedAggregates(ctx, subjectPost, 0, MaxSeededCount+1) + require.Error(t, err) + assert.True(t, errors.IsValidation(err), "an absurd downvote baseline is a validation error") + + // The previous good baseline survives the refused re-seed. + up, down, found := counts(t, database, subjectPost) + require.True(t, found) + assert.Equal(t, 7, up) + assert.Equal(t, 2, down) + + // Exactly at the cap is allowed (the cap is a bound, not off-by-one). + require.NoError(t, agg.SeedAggregates(ctx, subjectPost, MaxSeededCount, 0)) +} + +// TestScrubVoterErasesRowsAndRecounts: the actor-delete scrub removes every +// vote_events row a voter produced — live AND undone — and recomputes every +// affected aggregate, leaving other voters and seeded baselines untouched. +func TestScrubVoterErasesRowsAndRecounts(t *testing.T) { + database := testDB(t) + agg, objects := testAggregator(t, database) + bridgeSubject(t, objects, subjectPost, "3jzfcijpj2z2a") + bridgeSubject(t, objects, subjectComment, "3jzfcijpj3aaa") + ctx := context.Background() + + // A seeded baseline plus live votes from two voters on two subjects; + // alice also has an undone (superseded) row from a flip. + require.NoError(t, agg.SeedAggregates(ctx, subjectPost, 10, 1)) + require.NoError(t, agg.ApplyVote(ctx, like(activityID(t, 1), voterAlice, subjectPost), "")) + require.NoError(t, agg.ApplyVote(ctx, dislike(activityID(t, 2), voterAlice, subjectPost), "")) // flip: supersedes 1 + require.NoError(t, agg.ApplyVote(ctx, like(activityID(t, 3), voterBob, subjectPost), "")) + require.NoError(t, agg.ApplyVote(ctx, like(activityID(t, 4), voterAlice, subjectComment), "")) + + require.NoError(t, agg.ScrubVoter(ctx, voterAlice)) + + // Every alice row is GONE (not just undone — the scrub is a privacy + // erase, not a retraction). + var aliceRows int + require.NoError(t, database.QueryRow( + `SELECT COUNT(*) FROM vote_events WHERE voter_ap_id = $1`, voterAlice).Scan(&aliceRows)) + assert.Zero(t, aliceRows, "scrub must delete all of the voter's rows") + + // Post: baseline 10/1 + bob's live like; alice's live dislike removed. + up, down, _ := counts(t, database, subjectPost) + assert.Equal(t, 11, up) + assert.Equal(t, 1, down) + // Comment: alice was the only voter → back to zero. + up, down, _ = counts(t, database, subjectComment) + assert.Zero(t, up) + assert.Zero(t, down) + + // Idempotent: scrubbing again (and scrubbing a never-seen voter) is a + // no-op success. + require.NoError(t, agg.ScrubVoter(ctx, voterAlice)) + require.NoError(t, agg.ScrubVoter(ctx, "https://lemmy.world/u/never-voted")) +} + +// TestScrubVoter_RecomputesEveryDeletedSubject pins the DELETE ... RETURNING +// recompute set (task 11 lost-update fix): every subject the voter had a row +// on is recomputed, even when the served aggregate has drifted to a phantom +// value. A single-transaction unit test cannot reproduce the true concurrent +// race (a vote landing on a not-previously-voted subject between the snapshot +// lock and the DELETE), so it pins the load-bearing property directly — a +// deleted subject is ALWAYS in the recompute set, never left phantom. +func TestScrubVoter_RecomputesEveryDeletedSubject(t *testing.T) { + database := testDB(t) + agg, objects := testAggregator(t, database) + bridgeSubject(t, objects, subjectPost, "3jzfcijpj2z2a") + ctx := context.Background() + + // Alice's live upvote creates the post aggregate at up=1. + require.NoError(t, agg.ApplyVote(ctx, like(activityID(t, 1), voterAlice, subjectPost), "")) + + // Force the served upvotes to a phantom value that no longer matches the + // live rows — exactly the drift a lost update would leave behind. + _, err := database.Exec( + `UPDATE vote_aggregates SET upvotes = 99 WHERE subject_ap_id = $1`, subjectPost) + require.NoError(t, err) + + // Scrubbing alice deletes her row on the post; the recompute set is + // driven by DELETE ... RETURNING, so the subject is recomputed back to the + // truth (no live votes, no seeded baseline) rather than keeping the + // phantom. + require.NoError(t, agg.ScrubVoter(ctx, voterAlice)) + up, down, _ := counts(t, database, subjectPost) + assert.Zero(t, up, "the deleted subject is recomputed away from its phantom count") + assert.Zero(t, down) +} + +// TestPruneUndoneEventsKeepsLiveRows: only undone rows older than the +// cutoff are reclaimed; live votes (the counts!) and fresh undone rows +// survive, and served aggregates do not change. +func TestPruneUndoneEventsKeepsLiveRows(t *testing.T) { + database := testDB(t) + agg, objects := testAggregator(t, database) + bridgeSubject(t, objects, subjectPost, "3jzfcijpj2z2a") + ctx := context.Background() + + require.NoError(t, agg.ApplyVote(ctx, like(activityID(t, 1), voterAlice, subjectPost), "")) + require.NoError(t, agg.ApplyVote(ctx, dislike(activityID(t, 2), voterAlice, subjectPost), "")) // supersedes 1 → undone + require.NoError(t, agg.ApplyVote(ctx, like(activityID(t, 3), voterBob, subjectPost), "")) + require.NoError(t, agg.RetractVote(ctx, like(activityID(t, 3), voterBob, subjectPost), "")) // undone + + // Age alice's undone row past the cutoff; bob's undone row stays fresh. + _, err := database.Exec( + `UPDATE vote_events SET created_at = NOW() - INTERVAL '100 days' WHERE activity_id = $1`, + activityID(t, 1)) + require.NoError(t, err) + + n, err := agg.PruneUndoneEvents(ctx, time.Now().Add(-90*24*time.Hour)) + require.NoError(t, err) + assert.Equal(t, int64(1), n, "exactly the aged undone row is pruned") + + var live, undone int + require.NoError(t, database.QueryRow( + `SELECT COUNT(*) FILTER (WHERE NOT undone), COUNT(*) FILTER (WHERE undone) FROM vote_events`). + Scan(&live, &undone)) + assert.Equal(t, 1, live, "alice's live dislike survives") + assert.Equal(t, 1, undone, "bob's fresh undone row survives") + + up, down, _ := counts(t, database, subjectPost) + assert.Zero(t, up) + assert.Equal(t, 1, down, "served counts unchanged by pruning") +} diff --git a/internal/votes/ratelimit.go b/internal/votes/ratelimit.go deleted file mode 100644 --- a/internal/votes/ratelimit.go +++ /dev/null @@ -1,103 +0,0 @@ -package votes - -import ( - "sync" - "time" - - "golang.org/x/time/rate" -) - -const ( - // limiterIdleTTL is how long an idle per-IP bucket survives before a - // sweep may reclaim it (a reclaimed bucket restarts full, which only - // ever favors the client). - limiterIdleTTL = 10 * time.Minute - // limiterSweepInterval throttles sweeps to at most one full-map scan - // per interval, however hot the endpoint runs — without it a flood of - // distinct IPs would provoke an O(n) scan under the mutex on every - // request, turning the limiter itself into the DoS vector. - limiterSweepInterval = time.Minute - // limiterSweepThreshold is the map size below which sweeps don't - // bother running: a few thousand lingering buckets cost less than the - // scans that would reclaim them. - limiterSweepThreshold = 10_000 - // limiterMaxBuckets hard-caps the bucket map. At the cap, requests - // from IPs without a bucket are refused (fail closed): the cap is only - // reachable during an address-rotation flood (one IPv6 /64 is - // effectively unlimited addresses, none idle long enough for the TTL - // sweep), refusing unknown IPs leaves established clients — the - // AppView's long-lived poller — untouched, and admitting them instead - // would hand every rotated address a fresh full burst, which is - // exactly the abuse the limiter exists to stop. Idle buckets age out - // via the TTL sweep, so a genuinely new client is locked out only - // while the flood lasts. - limiterMaxBuckets = 50_000 -) - -// ipLimiter is a per-client-IP token bucket set (the same golang.org/x/time -// machinery as ingest's mint gate, keyed by IP). Stale buckets are swept -// inline — at most once per sweepInterval — and the map is hard-capped, so -// an address-rotating scraper can balloon neither memory nor scan time. -type ipLimiter struct { - perSecond rate.Limit - burst int - - // Tunables default to the package constants; they are fields (like - // now) so tests can shrink them. - idleTTL time.Duration - sweepInterval time.Duration - sweepThreshold int - maxBuckets int - now func() time.Time - - mu sync.Mutex - lastSweep time.Time - buckets map[string]*ipBucket -} - -type ipBucket struct { - limiter *rate.Limiter - lastSeen time.Time -} - -func newIPLimiter(perSecond float64, burst int) *ipLimiter { - return &ipLimiter{ - perSecond: rate.Limit(perSecond), - burst: burst, - idleTTL: limiterIdleTTL, - sweepInterval: limiterSweepInterval, - sweepThreshold: limiterSweepThreshold, - maxBuckets: limiterMaxBuckets, - now: time.Now, - buckets: map[string]*ipBucket{}, - } -} - -// allow reports whether one request from ip fits in its bucket. Unknown IPs -// are refused outright while the bucket map sits at maxBuckets (see the -// constant for why fail-closed is the right shape there). -func (l *ipLimiter) allow(ip string) bool { - l.mu.Lock() - defer l.mu.Unlock() - now := l.now() - - if len(l.buckets) >= l.sweepThreshold && now.Sub(l.lastSweep) >= l.sweepInterval { - l.lastSweep = now - for key, bucket := range l.buckets { - if now.Sub(bucket.lastSeen) > l.idleTTL { - delete(l.buckets, key) - } - } - } - - bucket, ok := l.buckets[ip] - if !ok { - if len(l.buckets) >= l.maxBuckets { - return false - } - bucket = &ipBucket{limiter: rate.NewLimiter(l.perSecond, l.burst)} - l.buckets[ip] = bucket - } - bucket.lastSeen = now - return bucket.limiter.AllowN(now, 1) -} diff --git a/internal/votes/ratelimit_test.go b/internal/votes/ratelimit_test.go deleted file mode 100644 --- a/internal/votes/ratelimit_test.go +++ /dev/null @@ -1,98 +0,0 @@ -package votes - -import ( - "fmt" - "testing" - "time" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -// testClock is a hand-cranked clock for driving the limiter's TTL, sweep, -// and refill paths deterministically. -type testClock struct{ t time.Time } - -func newTestClock() *testClock { - return &testClock{t: time.Date(2026, 7, 8, 12, 0, 0, 0, time.UTC)} -} - -func (c *testClock) now() time.Time { return c.t } -func (c *testClock) advance(d time.Duration) { c.t = c.t.Add(d) } - -func TestIPLimiterIndependentBuckets(t *testing.T) { - limiter := newIPLimiter(0.001, 1) - - require.True(t, limiter.allow("192.0.2.1")) - require.False(t, limiter.allow("192.0.2.1"), "the first IP's burst is spent") - assert.True(t, limiter.allow("192.0.2.2"), "a second IP must have its own bucket") -} - -func TestIPLimiterHardCap(t *testing.T) { - limiter := newIPLimiter(0.001, 1) - limiter.maxBuckets = 3 - - // Many distinct IPs: the map must never grow past the cap. - for i := 0; i < 50; i++ { - limiter.allow(fmt.Sprintf("2001:db8::%x", i)) - require.LessOrEqual(t, len(limiter.buckets), 3, "after IP %d", i) - } - assert.Len(t, limiter.buckets, 3) - - // At the cap: unknown IPs are refused, established ones still tracked. - assert.False(t, limiter.allow("198.51.100.9"), "unknown IP at cap is refused") - assert.False(t, limiter.allow("2001:db8::0"), - "a known IP still hits its own (spent) bucket, not the cap") -} - -func TestIPLimiterCapRecoversAfterSweep(t *testing.T) { - clock := newTestClock() - limiter := newIPLimiter(0.001, 1) - limiter.now = clock.now - limiter.maxBuckets = 2 - limiter.sweepThreshold = 1 - - require.True(t, limiter.allow("192.0.2.1")) - require.True(t, limiter.allow("192.0.2.2")) - require.False(t, limiter.allow("192.0.2.3"), "the map is at its cap") - - // Once the residents idle past the TTL and the sweep throttle window - // passes, the next request sweeps them out and the new IP fits. - clock.advance(limiterIdleTTL + limiterSweepInterval) - assert.True(t, limiter.allow("192.0.2.3")) - assert.Len(t, limiter.buckets, 1) -} - -func TestIPLimiterSweepThrottled(t *testing.T) { - clock := newTestClock() - limiter := newIPLimiter(0.001, 1) - limiter.now = clock.now - limiter.sweepThreshold = 1 - limiter.idleTTL = time.Second - - limiter.allow("192.0.2.1") - limiter.allow("192.0.2.2") // crosses the threshold: sweeps, stamps lastSweep - - // Both buckets idle past the (tiny) TTL, but the throttle window has - // not elapsed — the next allow must not sweep them. - clock.advance(2 * time.Second) - limiter.allow("192.0.2.3") - assert.Len(t, limiter.buckets, 3, "no sweep inside the throttle window") - - // Past the window, the sweep runs and reclaims every idle bucket. - clock.advance(limiterSweepInterval) - limiter.allow("192.0.2.4") - assert.Len(t, limiter.buckets, 1, "idle buckets reclaimed once the window passes") -} - -func TestIPLimiterRefillFollowsClock(t *testing.T) { - clock := newTestClock() - limiter := newIPLimiter(1, 1) - limiter.now = clock.now - - require.True(t, limiter.allow("192.0.2.1")) - require.False(t, limiter.allow("192.0.2.1"), "burst spent, no time has passed") - - clock.advance(1500 * time.Millisecond) - assert.True(t, limiter.allow("192.0.2.1"), "one token refills after a second") -} diff --git a/internal/votes/xrpc.go b/internal/votes/xrpc.go --- a/internal/votes/xrpc.go +++ b/internal/votes/xrpc.go @@ -5,7 +5,6 @@ "database/sql" "encoding/json" "fmt" "log/slog" - "net" "net/http" "strings" "time" @@ -15,6 +14,7 @@ "github.com/go-chi/chi/v5" "github.com/lib/pq" "tidepool/internal/errors" + "tidepool/internal/ratelimit" ) // NSIDGetVoteAggregates is the sanctioned side-channel query @@ -53,7 +53,7 @@ // AppView uses to display scores for bridged content (PLAN.md decision 7). // Public, cacheable, rate limited by client IP. type XRPC struct { db *sql.DB - limiter *ipLimiter + limiter *ratelimit.Limiter logger *slog.Logger } @@ -76,7 +76,7 @@ burst = defaultRateBurst } return &XRPC{ db: opts.DB, - limiter: newIPLimiter(perSecond, burst), + limiter: ratelimit.New(perSecond, burst), logger: logger, }, nil } @@ -101,7 +101,7 @@ // Well-formed but unknown uris are omitted from the response, never an // error — the AppView batches optimistically over content that may predate // vote ingestion. Malformed at-uris are InvalidRequest. func (x *XRPC) handleGetVoteAggregates(w http.ResponseWriter, r *http.Request) { - if !x.limiter.allow(clientIP(r)) { + if !x.limiter.Allow(ratelimit.ClientIP(r)) { x.writeXRPCError(w, http.StatusTooManyRequests, "RateLimitExceeded", "rate limit exceeded") return } @@ -192,18 +192,6 @@ uris = append(uris, value) } } return uris, nil -} - -// clientIP extracts the connection's remote IP. Deliberately not -// X-Forwarded-For: the bridge cannot know which proxies to trust, and a -// spoofable header would let one client exhaust every bucket. Deployments -// behind a load balancer rate-limit the real client at the edge. -func clientIP(r *http.Request) string { - host, _, err := net.SplitHostPort(r.RemoteAddr) - if err != nil { - return r.RemoteAddr - } - return host } func (x *XRPC) writeJSON(w http.ResponseWriter, status int, body any) { diff --git a/tests/e2e/helpers.go b/tests/e2e/helpers.go --- a/tests/e2e/helpers.go +++ b/tests/e2e/helpers.go @@ -34,8 +34,10 @@ "sync" "testing" "time" + comatproto "github.com/bluesky-social/indigo/api/atproto" "github.com/bluesky-social/indigo/atproto/atdata" "github.com/bluesky-social/indigo/atproto/lexicon" + indigoevents "github.com/bluesky-social/indigo/events" "github.com/gorilla/websocket" "tidepool/lexicons" @@ -837,10 +839,12 @@ // ── Bridge sync-surface client ───────────────────────────────────────────── // // The bridge's OWN com.atproto.sync.* + identity endpoints (tidepool :8092). // Repo lifecycle state (active/deactivated) must be asserted here rather -// than through the relay: bigsky serves no getRepoStatus, FILTERS -// tombstoned repos out of its listRepos, and learns account state upstream -// only from #account frames — which the bridge does not emit until task 11 -// (FOLLOWUPS.md "Relay pipeline"). +// than through the relay: bigsky serves no getRepoStatus and FILTERS +// tombstoned repos out of its listRepos. Since task 11 the bridge emits an +// #account{active:false, status:"deleted"} frame on Delete(Actor), so the +// relay-side signal IS observable — as the repo DISAPPEARING from the +// relay's listRepos (asserted in TestDeleteActor) — while fine-grained +// status still reads from the bridge. // xrpcResult is the decoded outcome of one bridge XRPC GET: the HTTP status // plus either the success body or the standard XRPC error shape. @@ -898,6 +902,51 @@ if err := json.Unmarshal(res.body, &out); err != nil { return repoStatus{}, res, fmt.Errorf("getRepoStatus(%s): decode: %w (%s)", did, err, truncate(res.body, 200)) } return out, res, nil +} + +// dialBridgeFirehose opens a raw WebSocket on the BRIDGE's own +// subscribeRepos (live tail — no cursor), for asserting frames Jetstream's +// JSON view abstracts away. The caller reads with readBridgeAccountFrame. +func (h *harness) dialBridgeFirehose(t *testing.T) *websocket.Conn { + t.Helper() + wsURL := "ws" + strings.TrimPrefix(tidepoolURL(), "http") + "/xrpc/com.atproto.sync.subscribeRepos" + conn, resp, err := websocket.DefaultDialer.Dial(wsURL, nil) + if resp != nil && resp.Body != nil { + defer func() { _ = resp.Body.Close() }() + } + if err != nil { + t.Fatalf("dial bridge firehose %s: %v", wsURL, err) + } + t.Cleanup(func() { _ = conn.Close() }) + return conn +} + +// readBridgeAccountFrame consumes the bridge's CBOR event stream until an +// #account frame for did arrives (skipping #commit and #info frames), or +// fails the test at the deadline. Uses indigo's frame decoder — the exact +// bytes a relay consumes. +func readBridgeAccountFrame(t *testing.T, conn *websocket.Conn, did string, timeout time.Duration) *comatproto.SyncSubscribeRepos_Account { + t.Helper() + deadline := time.Now().Add(timeout) + for { + if err := conn.SetReadDeadline(deadline); err != nil { + t.Fatalf("bridge firehose: set read deadline: %v", err) + } + msgType, payload, err := conn.ReadMessage() + if err != nil { + t.Fatalf("bridge firehose: no #account frame for %s within %s: %v", did, timeout, err) + } + if msgType != websocket.BinaryMessage { + continue + } + var evt indigoevents.XRPCStreamEvent + if err := evt.Deserialize(bytes.NewReader(payload)); err != nil { + t.Fatalf("bridge firehose: undecodable frame: %v", err) + } + if evt.RepoAccount != nil && evt.RepoAccount.Did == did { + return evt.RepoAccount + } + } } // bridgeListRepos walks the bridge's own listRepos (which, unlike the diff --git a/tests/e2e/lifecycle_test.go b/tests/e2e/lifecycle_test.go --- a/tests/e2e/lifecycle_test.go +++ b/tests/e2e/lifecycle_test.go @@ -4,11 +4,13 @@ package e2e // Task 10 lifecycle scenarios: consent (#nobridge), Delete(Actor), // unsubscribe, community profile update. All assert through the relay-fed -// Jetstream like the rest of the suite; repo lifecycle STATE (active / -// deactivated) is asserted against the bridge's own sync surface because -// bigsky cannot show it (no getRepoStatus, tombstoned repos filtered from -// listRepos, account state learned only from #account frames the bridge -// does not emit until task 11 — FOLLOWUPS.md "Relay pipeline"). +// Jetstream like the rest of the suite; fine-grained repo lifecycle STATE +// is asserted against the bridge's own sync surface (bigsky serves no +// getRepoStatus and filters tombstoned repos from listRepos). Since task 11 +// the bridge emits #account{active:false, status:"deleted"} on +// Delete(Actor), so TestDeleteActor additionally asserts the frame on the +// bridge's OWN firehose and the repo's DISAPPEARANCE from the relay's +// listRepos (bigsky tombstones the repo and purges its data on that frame). // // Negative-assertion discipline: every "nothing bridged" claim is bounded // by a positive control. Where the control post shares the suppressed @@ -173,9 +175,10 @@ // - the Delete arrives signed by an actor its origin already serves as // 410 Gone, so the inbox accepts it on independently-confirmed origin // tombstone evidence (inbox.go tombstonedSelfDelete). // -// NOTE: there is no #account frame yet (task 11), so the RELAY cannot -// observe the tombstone — bigsky keeps listing the repo. That gap is pinned -// here (and should flip when task 11 lands). +// Task 11 added the #account{active:false, status:"deleted"} firehose frame +// (emitted after the scrub delete-commits): this scenario asserts the frame +// on the bridge's own subscribeRepos AND its relay-side consequence — bigsky +// consumes it, marks the repo tombstoned, and stops listing it. func TestDeleteActor_ScrubsAndTombstones(t *testing.T) { h := newHarness(t) community, sub := setupSubscribedCommunity(t, h, "del") @@ -219,6 +222,10 @@ if did, res := h.bridgeResolveHandle(t, handle); res.status != 200 || did != authorDID { t.Fatalf("pre-delete: handle %s does not resolve to %s (status %d, did %q)", handle, authorDID, res.status, did) } + // Subscribe to the bridge's OWN firehose (live tail) BEFORE the delete + // so the #account frame cannot be missed. + fhConn := h.dialBridgeFirehose(t) + user.deleteAccount(t) // Scrub delete-commits for all three records, on their observed rkeys. @@ -235,6 +242,18 @@ return e.Commit.Collection == colActorProfile && e.Commit.Operation == opDelete && e.Did == authorDID && e.Commit.RKey == rkeySelf }) + // The #account frame on the bridge's own firehose: active:false, + // status "deleted", emitted AFTER the scrub commits (the purge signal a + // per-repo-ordered consumer sees last). This is the exact CBOR frame a + // relay consumes. + account := readBridgeAccountFrame(t, fhConn, authorDID, eventTimeout) + if account.Active { + t.Errorf("#account frame for %s reports active:true, want false", authorDID) + } + if account.Status == nil || *account.Status != "deleted" { + t.Errorf("#account frame status = %v, want \"deleted\"", account.Status) + } + // Over-scrub guard: the scrub must delete EXACTLY the three records // awaited above. A bounded drain asserts no OTHER delete op appears on // either affected repo — above all the community.profile rkey `self` @@ -258,8 +277,8 @@ deadline := time.Now().Add(eventTimeout) for { status, res, err := h.bridgeGetRepoStatus(authorDID) if err == nil && res.status == 200 && !status.Active { - if status.Status != "deactivated" { - t.Errorf("getRepoStatus(%s).status = %q, want deactivated", authorDID, status.Status) + if status.Status != "deleted" { + t.Errorf("getRepoStatus(%s).status = %q, want deleted", authorDID, status.Status) } break } @@ -274,8 +293,8 @@ repos := h.bridgeListRepos(t) entry, ok := repos[authorDID] if !ok { t.Errorf("bridge listRepos no longer lists tombstoned repo %s (it must, with active:false)", authorDID) - } else if entry.Active || entry.Status != "deactivated" { - t.Errorf("bridge listRepos entry for %s = %+v, want active:false status:deactivated", authorDID, entry) + } else if entry.Active || entry.Status != "deleted" { + t.Errorf("bridge listRepos entry for %s = %+v, want active:false status:deleted", authorDID, entry) } // The handle stops resolving: tombstoned identity is frozen. @@ -295,17 +314,25 @@ t.Errorf("%s(%s) = status %d err %q, want 400 RepoDeactivated", ep, authorDID, res.status, res.errCode) } } - // RELAY LIMITATION (task 09 finding, pinned deliberately): bigsky only - // learns account state from #account frames, which the bridge does not - // emit until task 11 — so the relay STILL lists the tombstoned repo. - // When task 11 adds the frame, this assertion must flip to "disappears - // from the relay's listRepos" (FOLLOWUPS.md "Relay pipeline"). - relayRepos, err := h.relayListRepos() - if err != nil { - t.Fatalf("relay listRepos: %v", err) - } - if _, ok := relayRepos[authorDID]; !ok { - t.Errorf("relay listRepos no longer lists %s — has task 11's #account frame landed? Flip this assertion (see comment)", authorDID) + // RELAY-SIDE CONSEQUENCE of the #account frame (flipped from the task-09 + // pin): bigsky consumes it, re-resolves the DID doc to confirm the + // bridge is authoritative, marks the account tombstoned, and drops the + // repo from its listRepos (source-verified against the pinned bigsky: + // status "deleted" → tombstoned=true + carstore purge; listRepos filters + // NOT tombstoned). Polled: the relay processes the frame asynchronously. + relayDeadline := time.Now().Add(eventTimeout) + for { + relayRepos, err := h.relayListRepos() + if err != nil { + t.Fatalf("relay listRepos: %v", err) + } + if _, ok := relayRepos[authorDID]; !ok { + break + } + if time.Now().After(relayDeadline) { + t.Fatalf("relay still lists tombstoned repo %s after the #account frame — did the relay reject it?", authorDID) + } + time.Sleep(2 * time.Second) } } -- tangled.sh