From dfbef03c5b4f5e17ab6fd61fccfb5b7c82790c2e Mon Sep 17 00:00:00 2001 From: Bretton Date: Fri, 14 Aug 2026 23:43:29 -0700 Subject: [PATCH] docs(deploy): correct a runbook that recommended the harmful kill switch (18b review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A fact-check of 18597cc against the code found the runbook telling an operator to reach for the wrong lever in an emergency, because the code told the runbook the wrong thing first. PARKING SPENDS THE RETRY BUDGET. `ClaimNext` increments `attempts` on every claim; `Release` — which `park` calls — never resets it; `parkDelay` is 5s and `DefaultMaxDeliveryAttempts` is 8. So with `OUTBOUND_DISABLED` engaged and workers running, each ordering key's head delivery is re-claimed and re-parked about twelve times a minute, and its budget is gone in roughly forty seconds. Nothing poisons WHILE parked, which is the half that reads true and is why this survived: `releaseOrPoison` poisons on the FIRST retryable failure after the switch clears. An hour-long kill switch therefore ends in a burst of poisoned deliveries with single-attempt failures. `worker.go` asserted in its own comment that park "does not advance the poison budget", and the runbook inherited that claim and went further, ranking `OUTBOUND_DISABLED` above `OUTBOUND_WORKERS=0` and telling the operator to prefer it. The ranking is now reversed, with the reason: at workers=0 no worker is constructed at all, so nothing claims and nothing is spent. The comments in `park` and `parkCausal` are corrected and say so explicitly, so the next reader does not re-trust the old text. `RedrivePoisoned` resets attempts to zero and is the remedy — previously documented nowhere. The behaviour itself is NOT fixed here. `Release` is one statement serving two callers that mean opposite things — "retryable failure, back off", where incrementing is correct, and "parked by a switch", where it is not — so the fix needs a design decision and a test, and a naive one must not disarm `parkCausal`, whose safety comes from a wall clock rather than a count. FOLLOWUPS carries it as a defect with the candidate directions and a proposed test shape. The KEK blast radius named two tables and there are three. `ap_actors.rsa_key_sealed` holds every NATIVE Coves user's AP signing key under the same key — the exact population v2 exists to serve, and the one a rotation plan written from the old text would have left unable to sign. Two further corrections found while fixing it: `service_keys.private_key_pem` has not existed since migration 013 renamed it to `key_material`, and only its plc-rotation row is sealed while the service-actor row is plaintext; and `bridged_actors.signing_key` is escrowed secp256k1 repo key material, not the per-actor RSA the document claimed. Also corrected: a "none of which exists" that does — `ap_actors.rsa_key_version` is present and unused as a selector, and a false not-implemented makes someone rebuild what is already there; `OUTBOUND_DRY_RUN` does not validate the translator, since translation happens at enqueue time and the worker POSTs the stored payload verbatim; the scoped switches are inert at workers=0 because the switch struct is only constructed in that branch; Caddy does not proxy coves.social entirely to the appview, so /.well-known/webfinger returns a static-file 404 today and that is the signature to diff against; and the "2-3h to poison" derivation is about 63 minutes, fixed in the runbook and in divergence.go where it originated. Added what an operator needs and did not have: the four tidepool_outbound_* counters, where `parked` is the only positive confirmation a switch engaged and `cancelled` rises on its own from the worker's consent check; the divergence gauges' pre-sweep -1; and the psql query for `last_error_class` and `response_excerpt`, which are on no admin route. Co-Authored-By: Claude Opus 5 (1M context) --- .env.prod.example | 15 +- DEPLOY.md | 251 +++++++++++++++++++++++++++++----- FOLLOWUPS.md | 79 ++++++++++- README.md | 8 +- internal/ingest/divergence.go | 24 ++-- internal/outbound/worker.go | 28 +++- 6 files changed, 346 insertions(+), 59 deletions(-) diff --git a/.env.prod.example b/.env.prod.example index 27d8513..b034491 100644 --- a/.env.prod.example +++ b/.env.prod.example @@ -34,10 +34,19 @@ AP_USER_ORIGIN=https://coves.social # 32-byte key-encryption key sealing per-actor signing keys at rest # (AES-256-GCM). Generate with: openssl rand -hex 32 -# Losing it means losing every bridged repo's signing keys — back it up. +# Losing it means losing every repo's signing keys — back it up. # THERE IS NO ROTATION PATH. Nothing in the codebase re-seals existing -# ciphertext under a new KEK; changing this value orphans every sealed key -# (every bridged identity and the PLC escrow rotation key) with no recovery. +# ciphertext under a new KEK; changing this value orphans every sealed key, +# across THREE tables, with no recovery: +# - bridged_actors.signing_key every bridged (Lemmy-origin) identity's +# escrowed secp256k1 atproto repo key +# - service_keys.key_material the 'plc-rotation' row: the PLC escrow +# key, the only DID recovery path +# - ap_actors.rsa_key_sealed EVERY NATIVE COVES USER'S AP signing +# key (v2 / task 13). Easy to omit from a +# rotation or backup plan written before +# v2 existed — and it is the entire +# native-user population. # See DEPLOY.md "Not implemented" before you touch it. BRIDGE_KEK=CHANGE_ME diff --git a/DEPLOY.md b/DEPLOY.md index 0e9b0e3..5d3e97d 100644 --- a/DEPLOY.md +++ b/DEPLOY.md @@ -124,12 +124,38 @@ transaction, so intent past the gate is never dropped (`cmd/tidepool/main.go:695-713`). Raising `OUTBOUND_WORKERS` later drains whatever accumulated, immediately. Budget for that. -**`OUTBOUND_DISABLED` parks, it does not fail.** A blocked delivery stays -`pending` and resumes when the switch clears; it is never poisoned and never -cancelled (`internal/outbound/worker.go:235-242`, -`internal/outbound/switches.go:5-12`). Engaging a kill switch loses nothing. -`OUTBOUND_DRY_RUN` parks the same way, after translating and logging -(`worker.go:243-248`). +**`OUTBOUND_DISABLED` parks, it does not fail — but parking is not free.** A +blocked delivery stays `pending` and resumes when the switch clears; `park` +itself never poisons and never cancels (`internal/outbound/worker.go:242-244`, +`internal/outbound/switches.go:5-12`). `OUTBOUND_DRY_RUN` parks the same way, +before any signing or POST (`worker.go:245-249`). + +⚠️ **A park still consumes the delivery's retry budget, and fast.** `ClaimNext` +does `attempts = attempts + 1` on every claim +(`internal/store/outbound_deliveries.go:143`), and `park` settles through +`Release`, whose `SET` clause updates `claimed_until`, `next_attempt_at`, +`last_error_class`, `response_excerpt`, `last_status_code` and `updated_at` — +and **never resets `attempts`** (`outbound_deliveries.go:218-221`). A parked +delivery is rescheduled `parkDelay = 5 * time.Second` out (`worker.go:560`), so +with workers running, each ordering key's head delivery is re-claimed and +re-parked roughly every five seconds and its +`DefaultMaxDeliveryAttempts = 8` budget (`worker.go:78`) is exhausted in about +**40 seconds**. Nothing poisons *while* parked — `park` never calls `poison` — +but once the switch clears, the **first** retryable failure finds +`Attempts >= maxAttempts` and poisons immediately instead of retrying +(`worker.go:512-518`). An hour-long global kill switch leaves every head +delivery with hundreds of attempts and zero retries left. + +It is also a continuous write load: one `UPDATE` per parked head roughly every +five seconds, per ordering key, for as long as the switch is engaged. + +**The remedy is `POST /admin/outbound/redrive`.** `RedrivePoisoned` sets +`attempts = 0` along with `state = 'pending'` +(`internal/store/outbound_deliveries.go:613-617`), so a redrive restores a full +budget. It only matches `state = 'poisoned'` (`:619`), so it repairs the damage +after a delivery has already fallen over, rather than preventing it. **To park +everything, prefer `OUTBOUND_WORKERS=0`** — see [Rollback](#rollback). The +underlying defect is tracked in `FOLLOWUPS.md`. Scope matching, from `config.go:519-525`: @@ -142,6 +168,43 @@ Scope matching, from `config.go:519-525`: There is no allowlist form of any of these. See [Staged rollout](#5-staged-rollout) for what that means for a canary. +**Every switch in that table — and `OUTBOUND_DISABLED` and `OUTBOUND_DRY_RUN` +with them — is INERT while `OUTBOUND_WORKERS=0`.** `ConfigSwitches` is only +constructed inside `if cfg.OutboundWorkers > 0` +(`cmd/tidepool/main.go:716-736`), so with no worker there is nothing holding a +switch and nothing consulting one. Setting `OUTBOUND_DISABLED=true` while +workers are 0 changes nothing and confirms nothing; it is not a second belt. +Conversely, that is also why `OUTBOUND_WORKERS=0` costs nothing: no worker +exists to claim, park, or spend attempts. + +### Confirming a switch actually engaged + +`internal/outbound/metrics.go:10-13` publishes four counters, all under +`/admin/metrics`: + +| Metric | Bumped when | +|---|---| +| `tidepool_outbound_delivered` | a delivery reached the peer (incl. Lemmy's duplicate-ack) | +| `tidepool_outbound_poisoned` | a delivery hit a terminal failure | +| `tidepool_outbound_cancelled` | the **worker** cancelled a claimed delivery on consent state — its only call site (`worker.go:292`) | +| `tidepool_outbound_parked` | a kill-switch, dry-run, **or causal-wait** deferral | + +`tidepool_outbound_parked` is the only **positive** confirmation that a kill +switch engaged: `by_state` still reads `pending` for a parked delivery, exactly +as it does for one merely waiting its turn, so the queue view cannot tell you. +Read it as a rate, not a level — a parked head is re-parked every ~5s, so the +counter climbs continuously while a switch is held. That climb *is* the budget +being spent. + +Two caveats on it. It is shared with `parkCausal`, so a nonzero `parked` with no +switch engaged means causal waits, not an operator action. And +`tidepool_outbound_cancelled` rises **on its own** during rollout: the worker +cancels a claimed delivery whenever the actor is disabled, delivery-paused, or +opted out (`internal/outbound/worker.go:281-296`). That is the counter's only +increment site, so a climbing `cancelled` is consent doing its job and is never +evidence that somebody ran `POST /admin/outbound/cancel` — the admin cancel does +not touch this metric, and shows up only in `by_state`. + --- ## 3. The admin surface at incident time @@ -179,6 +242,12 @@ of `actor` or `community` (`follow.go:236-239`). store is wired unconditionally (`cmd/tidepool/main.go:454`), so an empty `by_state` map is the truth about the queue, not a symptom of misconfiguration. +**It is also the whole route.** `by_state` counts are *all* it returns +(`internal/ingest/follow.go:172-183`) — no rows, no ids, no reasons. There is +no admin endpoint that exposes `last_error_class` or `response_excerpt`, so +"inspect, fix, then redrive" means psql on the box; the query is in +[Step 3](#step-3--widen). Plan for that before the incident, not during it. + ```sh T="Authorization: Bearer $ADMIN_TOKEN" curl -s -H "$T" localhost:8091/admin/outbound # queue depth by state @@ -203,8 +272,23 @@ the Coves side. on the **Coves** hostname, served by the Tidepool process. Tidepool's Host router sends requests whose `Host` is `coves.social` to the persona surface, and everything under `tdpl.io` to the bridge (`internal/personas/hostrouter.go:89-115`). -Caddy currently proxies `coves.social` entirely to the AppView, so **none of -those AP paths reach Tidepool today.** +**None of those AP paths reach Tidepool today** — but not because Caddy sends +the whole hostname to the AppView. The existing `coves.social` site block +(`~/Code/coves/Caddyfile`, block opens at `:70`; routing runs `:72-117`) +already splits the hostname four ways: + +| Path | Today's handler | +|---|---| +| `/.well-known/*` | static `file_server` over `/srv` (`Caddyfile:72`) | +| `/client-metadata.json` | static `file_server` over `/srv` (`Caddyfile:79`) | +| `/img/*` | 301 to `img.coves.social` (`Caddyfile:97`) | +| everything else | catch-all `reverse_proxy appview:8080` (`Caddyfile:102`) | + +Only the catch-all reaches the AppView. That matters for the before/after diff: +`GET /.well-known/webfinger` today returns a **static-file-server 404** from +`/srv`, not an AppView response, and `GET /nodeinfo/2.0` and `/ap/*` fall to the +AppView. So the signature to look for before the change is a bare 404 on +webfinger — and after it, a Tidepool JRD. The apex needs a content-negotiated split, and Tidepool deliberately cannot do it itself: `internal/personas/instance.go:32-34` states in its own comment that @@ -405,6 +489,17 @@ Two reading rules for those keys: for "storage could not be read" (`internal/consume/metrics.go:26-30`), chosen because a `0` would claim the backlog is empty at exactly the moment nobody can tell. +- **The `tidepool_divergence_*` gauges use the same `-1` convention**, for the + same reason (`internal/ingest/divergence.go:166-175`, `divergenceUnswept`). + Because they are published at package init, they are present from process + start — reading `-1` until the startup sweep completes, and again for any + class a failed sweep never wrote. `-1` is "never measured", `0` is "measured, + nothing diverging"; do not alert on them as if both meant healthy. +- **The four `tidepool_outbound_*` counters are absent until the first delivery + worker touches them** — they are `expvar.NewInt` at package init in + `internal/outbound/metrics.go:10-13`, so the keys exist once the package is + linked, but they sit at 0 while `OUTBOUND_WORKERS=0`. A flat 0 across all four + at this step is exactly right. Check the rejections before letting anything out. A misconfigured `ADMISSION_MAX_PER_AUTHOR_PER_COMMUNITY`, a stale community mapping, or a @@ -436,10 +531,21 @@ denylist.** The reconciler will subscribe it and it will start federating on the next sweep. Whenever the follow list grows during a canary, extend `OUTBOUND_DISABLED_COMMUNITIES` in the same change. -Optionally precede this with `OUTBOUND_DRY_RUN=true` for one cycle: every -delivery is translated and logged, nothing is POSTed, and the parked deliveries -resume when you clear it. That validates the translator against real records -without touching a peer. +**`OUTBOUND_DRY_RUN=true` is a weaker instrument than it sounds, and it is not +free.** What it does: the worker claims a delivery, logs it, and parks before +signing or POSTing (`internal/outbound/worker.go:245-249`). What it does **not** +do is validate the translator. **Translation happens at ENQUEUE time**, inside +the consumer's gate transaction (`cmd/tidepool/main.go:703-710`), and the worker +POSTs the stored payload verbatim (`worker.go:309`, `:319`). By the time a +delivery is claimable its payload has already been translated and persisted — so +the moment `CONSUMER_ENABLED=true`, the translator has already run on everything, +dry run or not. Step 1 is where you check its output (read +`outbound_activities.payload` in psql), not here. + +And because dry-run parks, it carries the same budget cost as any other park: +each head delivery is re-claimed every ~5s and burns its 8 attempts in ~40 +seconds (see [§2](#2-the-v2-flag-topology)). A "one cycle" dry run is measured +in seconds, not hours, and wants a `redrive` after it. ### On announcement throttling — what actually exists @@ -450,7 +556,7 @@ Decision 19 asks for "deliberate throttling of initial actor announcements". `docker-compose.prod.yml`, because the public `plc.directory` 429s mint bursts during community backfill) gate **inbound** DID minting only. They are wired into `ingest.NewMintGate`, whose sole consumer is the materializer's minter -(`cmd/tidepool/main.go:271-276`, `:314`) — the path where an unseen *Lemmy* +(`cmd/tidepool/main.go:271-276`, `:315`) — the path where an unseen *Lemmy* author gets an atproto DID. **There is no rate limiter on the outbound side.** `internal/outbound` contains @@ -492,6 +598,27 @@ What each one means: - **`by_state.poisoned` climbing** — deliveries exhausting their retries. Inspect, fix, then `redrive` **scoped** to the affected community. + + **There is no admin inspect surface for the "why".** `GET /admin/outbound` + returns `by_state` and nothing else (`internal/ingest/follow.go:172-183`) — a + count, no rows, no reasons. The columns that carry the diagnosis, + `last_error_class` and `response_excerpt`, are written by the worker but + exposed on no route; reading them means psql on the box: + + ```sh + docker exec -i tidepool-prod-postgres psql -U tidepool -d tidepool -c " + SELECT ordering_key, last_error_class, last_status_code, attempts, + left(response_excerpt, 200) AS excerpt, updated_at + FROM outbound_deliveries + WHERE state = 'poisoned' + ORDER BY updated_at DESC LIMIT 50;" + ``` + + Check `attempts` in that output before concluding the peer rejected anything: + a delivery whose budget was spent by a held kill switch (see + [§2](#2-the-v2-flag-topology)) poisons on its first real failure with an + `attempts` far above 8 and an error class that describes one attempt, not + eight. - **echo drop counters rising steadily** — expected and healthy: our own content arriving back from Lemmy and being correctly refused. A counter at **zero** while native content is flowing is the alarming case; it means @@ -514,13 +641,27 @@ step being one `.env` edit plus `up -d tidepool`: 1. **`OUTBOUND_DISABLED_COMMUNITIES=`** — park one community. Everything else keeps flowing; the parked deliveries resume when you clear - it. -2. **`OUTBOUND_DISABLED=true`** — park everything outbound. The consumer keeps - running and keeps recording intent; nothing reaches any peer. This is the - big red button and it is **lossless**. -3. **`OUTBOUND_WORKERS=0`** — stop the workers entirely. Equivalent effect to - (2) for delivery; prefer (2), because a parked delivery carries a recorded - reason and a stopped worker does not. + it. First because it is the *narrowest*, not because it is free: it is a + park, so it spends that community's head delivery's retry budget at the same + ~5s cadence as (3). Redrive that community after clearing it. +2. **`OUTBOUND_WORKERS=0`** — stop the workers entirely. This is the big red + button for delivery. The consumer keeps running and keeps recording intent; + nothing reaches any peer. `NewWorker` is only called inside + `if cfg.OutboundWorkers > 0` (`cmd/tidepool/main.go:716`), so at 0 there is + no worker to claim anything: nothing is re-claimed, no `attempts` are spent, + and no rows are written. It costs nothing and it is genuinely lossless. +3. **`OUTBOUND_DISABLED=true`** — park everything outbound. Same *observable* + effect as (2) — nothing reaches any peer — but **it is not free, and it is + ranked below (2) for that reason.** The workers keep running, so every + ordering key's head delivery is re-claimed and re-parked every ~5s and burns + its 8-attempt budget in ~40 seconds (see [§2](#2-the-v2-flag-topology)). The + deliveries this switch exists to protect are exactly the ones left with no + retries. Reach for it only when you need the *scope* it gives you and a + whole-worker stop is too blunt — and expect to `redrive` afterwards, which + is the only thing that resets `attempts` + (`internal/store/outbound_deliveries.go:613-617`). The one thing (3) buys + over (2) is that each parked row carries a recorded `last_error_class` / + `response_excerpt`; a stopped worker records nothing. 4. **`CONSUMER_ENABLED=false`** — stop consuming. Intent stops being recorded. The consumer resumes from its stored cursor when re-enabled, so this is recoverable, but it is the only step that stops *observing*, and @@ -555,25 +696,60 @@ re-seals existing ciphertext under a new key: the binary has exactly two subcommands, `tidepool` and `tidepool migrate` (`cmd/tidepool/main.go:69-78`). -Blast radius of losing or changing it: every per-actor RSA signing key for -every bridged identity is sealed under it, plus the PLC **escrow rotation -key** (`internal/identity/keys.go:140-144`). Change the KEK and every one of -those ciphertexts becomes undecryptable — no bridged actor can sign, and the -escrow key that could recover the DIDs is itself sealed under the key you just -replaced. Approximately 950 identities. There is no recovery. +Blast radius of losing or changing it — **three** tables, not two: + +1. **`bridged_actors.signing_key`** — the escrowed **secp256k1 atproto** repo + signing key of every bridged (Lemmy-origin) identity + (`internal/db/migrations/002_create_bridged_actors.sql:14`, + `internal/identity/keys.go:86-96`). Approximately 950 of them. +2. **`service_keys.key_material`, row `plc-rotation`** — the PLC **escrow + rotation key** (`internal/identity/keys.go:140-144`), the one thing that + could recover the DIDs, itself sealed under the key you just replaced. Note + the column is `key_material`, not `private_key_pem`; migration 013 renamed + it precisely because only this row is ciphertext — the sibling + `service-actor` row is **plaintext** PKCS#8 PEM and is *not* KEK-sealed + (`internal/db/migrations/013_rename_service_key_column.sql`). +3. **`ap_actors.rsa_key_sealed`** — **every NATIVE Coves user's ActivityPub RSA + signing key**, sealed under the same KEK under its own AAD prefix + (`internal/db/migrations/017_ap_actors.sql:46`, + `internal/identity/keys.go:39-53`). Written at mint time in + `internal/personas/personas.go:185`, through the same `identity.Custodian` + handed to `personas.New` at `cmd/tidepool/main.go:521-527`. + +**(3) is the v2 one, and it is the one a rotation plan will forget**, because +it did not exist when this section was first written. A rotation built to +handle only `bridged_actors` and `service_keys` would leave every native user +unable to sign a single outbound activity — the exact population v2 exists to +serve. Change the KEK and all three sets of ciphertext become undecryptable. +There is no recovery. *Naming trap:* `LoadOrCreateRotationKey` is **not** KEK rotation. It loads or generates the did:plc escrow/recovery key — an atproto identity concept — which is itself sealed under the KEK. Do not read that symbol as evidence that rotation is implemented. -What a real rotation would require, none of which exists: a key-version -column or KEK-id alongside each sealed blob; a dual-read custodian that tries -the new KEK then the old; an online re-seal pass over `bridged_actors` and -`service_keys`; and a cutover that retires the old KEK only after the pass -completes. The ciphertext does carry a one-byte version prefix -(`internal/identity/keys.go:127`), which is a hook someone could build on, but -nothing reads it as a key selector today. +What a real rotation would require: + +- **A key-version column or KEK-id alongside each sealed blob.** *Partly + present, and this is worth knowing before anyone designs it from scratch.* + `ap_actors` **already has one**: `rsa_key_version INT NOT NULL` + (`internal/db/migrations/017_ap_actors.sql:47`), and that migration's own + comment says it is there so "rotation [is] definable without a schema change" + (`:23-24`). It is stamped from `currentRSAKeyVersion = 1` + (`internal/personas/personas.go:25`, applied at `:185`) and read back, but + **nothing uses it as a selector** — no code branches on it to choose a KEK. + So on `ap_actors` the schema work is done and only the logic is missing. + `bridged_actors.signing_key` and `service_keys.key_material` genuinely have + no version column; those two need the migration as well. +- **A dual-read custodian** that tries the new KEK then the old. +- **An online re-seal pass** over all three tables above — `ap_actors` + included, which is the one a v1-era plan omits. +- **A cutover** that retires the old KEK only after the pass completes. + +The ciphertext also carries a one-byte version prefix +(`internal/identity/keys.go:127`) — but that is the *envelope format* version, +checked for equality and rejected otherwise, not a key id. Neither it nor +`rsa_key_version` selects a key today. Per-actor **RSA** rotation is equally undefined: rotating an actor's key means republishing `publicKey` in its actor document and having every peer that @@ -585,7 +761,7 @@ somewhere that survives the loss of the server.** ### Backup and restore -**No procedure exists, and no tooling.** `docker-compose.prod.yml:46` mounts +**No procedure exists, and no tooling.** `docker-compose.prod.yml:53` mounts `./backups:/backups` into the Postgres container. Nothing writes to it. There is no cron, no `pg_dump` wrapper, no restore drill, and no documented RPO/RTO. @@ -593,8 +769,11 @@ What is at risk, in order of irreplaceability: 1. **`BRIDGE_KEK`** — lives in `/opt/tidepool/.env`, not in Postgres, and is not covered by any database backup. Losing it is unrecoverable (above). -2. **`bridged_actors` / `service_keys`** — the sealed signing keys. Losing - these loses the identities even if the KEK survives. +2. **`bridged_actors` / `service_keys` / `ap_actors`** — the sealed signing + keys, for bridged identities, the PLC escrow key, **and every native Coves + user** respectively. Losing these loses the identities even if the KEK + survives. `ap_actors` is easy to omit from a v1-era backup scope; it is the + whole native-user population. 3. **Repo blocks and commits** — the atproto repos themselves. Re-derivable from upstream only by re-bridging, which mints new DIDs; the old at-uris do not come back. diff --git a/FOLLOWUPS.md b/FOLLOWUPS.md index 0b361d6..d01be9d 100644 --- a/FOLLOWUPS.md +++ b/FOLLOWUPS.md @@ -63,6 +63,68 @@ task documents and git history rather than this list. ## Outbound delivery (task 15) +- **DEFECT, NOT DONE — a PARK spends the poison budget, so the kill switch + destroys the retries of exactly the deliveries it exists to protect.** + Found 2026-08-14 while fact-checking `DEPLOY.md`; the docs and the two park + doc-comments have been corrected to describe it, and **nothing about the + behaviour was changed.** It needs its own test-first subtask. + + *Mechanism.* `ClaimNext` does `attempts = attempts + 1` on every claim + (`internal/store/outbound_deliveries.go:143`). `park` and `parkCausal` settle + through `Release`, whose `SET` clause updates `claimed_until`, + `next_attempt_at`, `last_error_class`, `response_excerpt`, `last_status_code` + and `updated_at` — and never resets `attempts` + (`outbound_deliveries.go:218-221`). `parkDelay = 5 * time.Second` + (`internal/outbound/worker.go:560`) and `DefaultMaxDeliveryAttempts = 8` + (`worker.go:78`), so with `OUTBOUND_DISABLED=true` and workers running, each + ordering key's head delivery is re-claimed and re-parked about every five + seconds and its entire retry budget is gone in roughly **40 seconds**. + + *Why it is not caught by "park never poisons".* It isn't — `park` genuinely + never calls `poison`, which is what made the old comments read as true. The + damage lands later: `releaseOrPoison` poisons on the FIRST retryable failure + once `Attempts >= maxAttempts` (`worker.go:512-518`). So an hour-long kill + switch leaves head deliveries with hundreds of attempts and zero retries, and + the next transient 5xx or dial timeout poisons them immediately. `parkCausal` + shares the mechanism but is materially safer: `causalStatus` bounds the causal + wait by WALL CLOCK from `delivery.CreatedAt`, never by attempt count, so a + held child's outcome is still decided by elapsed time. It is exposed only for + a genuine delivery failure after the parent lands. + + *Remedy that exists today, and its limit.* `RedrivePoisoned` sets + `attempts = 0` (`outbound_deliveries.go:613-617`), but it matches + `state = 'poisoned'` only (`:619`) — it repairs after the fall, and cannot + pre-empt it. `OUTBOUND_WORKERS=0` avoids the whole problem (no worker is + constructed, `cmd/tidepool/main.go:716`) and `DEPLOY.md` §5 now ranks it above + `OUTBOUND_DISABLED` for that reason. + + *Shape of a real fix — this is the design question, not a settled plan.* + `Release` is a single statement serving two callers that mean opposite + things, and it cannot tell them apart: a **retryable failure** (where holding + the incremented `attempts` is exactly right — that is the backoff working) + from a **park by an operator switch or a causal hold** (where it is not; the + delivery never reached the wire and nothing was learned about the peer). Two + candidate directions, with the trade to be argued in the subtask: + - a **park-specific release** — a sibling statement, or a flag on `Release`, + that writes `attempts = attempts - 1` / `attempts = $n` so a park is + attempt-neutral. Cheapest and most local, but adds a second write path + through the most fencing-sensitive statement in the package, and a park + that "un-counts" must not be able to underflow or to un-count a real + attempt on a re-claim race. + - **reset on unpark** — leave the increment and clear `attempts` when a + delivery next passes the switch gate. Keeps `Release` single-purpose, but + the reset then lives on the hot path and has to distinguish "was parked" + from "was retried", which today is only knowable from `last_error_class` + (`switch_parked` / `dry_run` / the causal classes) — i.e. it would make an + error-class string load-bearing for a correctness decision. + + A RED test should pin the operator-visible fact rather than the column: with + the switch engaged, drive N claim/park cycles well past `maxAttempts`, clear + the switch, then fail the delivery once retryably and assert it is + **rescheduled, not poisoned**. Note that any fix must keep `parkCausal`'s + wall-clock poison reachable — a naive "parks never advance anything" change + must not also disarm the causal budget. + - **outbound_deliveries.ClaimNext lacks a standalone `seq` index.** The loose-scan CTE builds the head set via the `(ordering_key, seq)` partial index, but the outer `c.seq = ANY(ARRAY(...)) FOR UPDATE` re-check has no @@ -291,10 +353,19 @@ is not building them; they stay open here: - **No `BRIDGE_KEK` / per-actor RSA rotation path.** Nothing re-seals existing ciphertext under a new KEK, and the binary's only subcommand is `migrate` - (no args = serve). Changing the KEK orphans every bridged identity's signing key - *and* the PLC escrow rotation key sealed under it (~950 identities, no - recovery). Would need a key-version selector on each sealed blob, a - dual-read custodian, an online re-seal pass, and a cutover. + (no args = serve). Changing the KEK orphans sealed key material in **three** + tables, not two: `bridged_actors.signing_key` (~950 bridged identities' + escrowed secp256k1 repo keys), `service_keys.key_material` row `plc-rotation` + (the PLC escrow key, the only DID recovery path), and — added by v2, and the + one a pre-v2 plan omits — `ap_actors.rsa_key_sealed`, **every native Coves + user's AP signing key** (`internal/db/migrations/017_ap_actors.sql:46`). No + recovery for any of it. Would need a key-version selector on each sealed + blob, a dual-read custodian, an online re-seal pass over all three, and a + cutover. Partial credit on the first: `ap_actors.rsa_key_version` already + exists (`017_ap_actors.sql:47`, stamped from `currentRSAKeyVersion = 1` at + `internal/personas/personas.go:25`) and is deliberately there so rotation is + definable without a schema change — but nothing reads it as a selector, and + the other two tables have no version column at all. - **No backup or restore procedure.** `docker-compose.prod.yml` mounts `./backups` into the Postgres container and nothing writes to it. Note `BRIDGE_KEK` lives in `.env` and is not covered by any database backup at diff --git a/README.md b/README.md index 554a135..ab43d70 100644 --- a/README.md +++ b/README.md @@ -295,15 +295,15 @@ Two classes, and the difference matters at boot: | `FOLLOW_LIST_PATH` | *(optional)* | declarative follow list (see below); unset = the `/admin` API is the only subscription control | | `FOLLOW_LIST_INTERVAL` | `15m` | follow-list reconciler sweep cadence | | `DIVERGENCE_INTERVAL` | `15m` | cadence of the reconciliation sweep (task 17e) that compares atproto state against outbound state and publishes the `tidepool_divergence_*` gauges. **Not an on/off switch:** the sweep is wired unconditionally, runs once at startup before its first tick, and `0` is refused — it is read-only (it reports, never repairs), which is what makes an always-on schedule safe. `GET /admin/divergence` runs one on demand | -| `DIVERGENCE_ACCEPTANCE_STALE_AFTER` | `12h` | how long a pending delivery may sit before the sweep reports its acceptance as **stale**. The report's one crying-wolf knob — shorter and every in-flight post is a finding, longer and a queue that stopped this morning is not in tonight's report. 12h is derived from the retry schedule (~2–3h to poison) plus the causal wait budget (6h), not picked | +| `DIVERGENCE_ACCEPTANCE_STALE_AFTER` | `12h` | how long a pending delivery may sit before the sweep reports its acceptance as **stale**. The report's one crying-wolf knob — shorter and every in-flight post is a finding, longer and a queue that stopped this morning is not in tonight's report. 12h is derived from the causal wait budget (6h, the binding envelope) with the retry schedule inside it — 8 attempts of 30s doubling sum to 3810s, so **~63 minutes** to poison, not the "2–3h" this row claimed before 2026-08-14 — not picked | | `CONSUMER_ENABLED` | **off** | turns on the Jetstream consumer (task 14): native users' opt-outs, profiles, posts, comments and votes flowing outward. Default off because it writes durable outbound state, and because a deployment that has not been canaried should not start accumulating it — not because the seams behind it are stubbed. They are wired: with it on, the **real** enqueuer persists outbound intent, the acceptance engine admits postv2 and writes community-signed acceptances, and opt-out `deleteRemote` / confirmed account deletions actually purge at peers. It also gates two other things — the `OUTBOUND_WORKERS` AND, and whether `/admin/admissions*` exists at all | | `JETSTREAM_URL` | *(optional)* | the self-hosted Jetstream the consumer subscribes to (`ws://` or `wss://`); **required** when `CONSUMER_ENABLED`, and validated at boot whenever set so a typo fails fast instead of becoming a reconnect loop. May be staged ahead of the flag | | `OUTBOUND_WORKERS` | `0` (**off**) | how many delivery workers run. **There is no `OUTBOUND_ENABLED`:** delivery starts only when this is `>0` *and* `CONSUMER_ENABLED`. With the consumer on and this at `0`, outbound intent still accumulates durably and nothing is POSTed — which is the intended staging step, not a broken state. Raising it drains the accumulated backlog immediately | -| `OUTBOUND_DISABLED` | off | global delivery kill switch. A blocked delivery is **parked** — it stays `pending` and resumes when the switch clears — never poisoned, never cancelled. Engaging it loses nothing; it stops the wire | +| `OUTBOUND_DISABLED` | off | global delivery kill switch. A blocked delivery is **parked** — it stays `pending` and resumes when the switch clears — and park itself never poisons or cancels. **But it is not free:** `ClaimNext` increments `attempts` on every claim and `Release` never resets it, so with workers running each ordering key's head is re-claimed every ~5s and its 8-attempt budget is gone in ~40s; the first real failure after the switch clears then poisons instead of retrying. Only `redrive` resets `attempts`. To stop everything, **`OUTBOUND_WORKERS=0` is the cheaper switch** — no worker is constructed, so nothing is spent. Also note every switch here is **inert** while `OUTBOUND_WORKERS=0`. See `DEPLOY.md` §2 | | `OUTBOUND_DISABLED_HOSTS` | *(empty)* | comma-separated inbox **hosts** to park. Lowercased on load and compared case-insensitively — a kill switch must fail closed on case | | `OUTBOUND_DISABLED_COMMUNITIES` | *(empty)* | comma-separated community **AP ids** to park (`https://lemmy.world/c/comicstrips`), matched **exactly and case-sensitively** against the delivery's ordering key. There is no allowlist form: a one-community canary is spelled by disabling every other community | | `OUTBOUND_DISABLED_ACTORS` | *(empty)* | comma-separated actor **DIDs** to park, exact match | -| `OUTBOUND_DRY_RUN` | off | translate and log every delivery, POST nothing. Parks like the kill switches, so nothing is lost — the difference is that the translation ran and is in the log | +| `OUTBOUND_DRY_RUN` | off | log every claimed delivery and POST nothing; the worker parks **before** signing. It does **not** validate the translator — translation happens at *enqueue* time inside the consumer's gate transaction and the worker POSTs the stored payload verbatim, so the translator has already run on everything the moment `CONSUMER_ENABLED=true`. Parks like the kill switches, and carries the same `attempts` cost (above): keep a dry run to seconds, then `redrive` | | `ADMISSION_MAX_PER_AUTHOR_PER_COMMUNITY` | `50` | acceptance-engine flood cap: how many posts one native author may have accepted into one bridged community. `0` = unlimited. Tidepool signs the community's acceptance, so this bounds what it vouches for | ## The admin API @@ -325,7 +325,7 @@ through Caddy. | `GET /admin/divergence` | run one reconciliation sweep synchronously and return the report | always wired in a normal deployment | | `GET /admin/outbound` | delivery queue depth by state (`pending`/`poisoned`/`cancelled`/…) | — the store is always wired, so this answers even with the consumer off and workers at 0. An empty queue then is the truth, not a misconfiguration | | `POST /admin/outbound/redrive` | reset poisoned deliveries to pending | — | -| `POST /admin/outbound/cancel` | park an actor's or a community's pending deliveries as cancelled | — | +| `POST /admin/outbound/cancel` | **terminally** cancel an actor's or a community's pending deliveries (consent withdrawal, community removal). Not a pause: `cancelled` is terminal and `redrive` revives only `poisoned` rows, so this is not the reversible sibling of a kill-switch **park** | — | | `GET /admin/admissions` | list acceptance decisions with `status`, `decisionCode`, `evaluatedCid`; filter by `?status=`/`?community=` | **404** — these routes are registered **only** when `CONSUMER_ENABLED`. A 404 here means the consumer is off, not that the endpoint is broken | | `POST /admin/admissions/readmit` | force re-admit a rejected post | **404**, same reason | | `GET /admin/metrics` | expvar counters filtered to the `tidepool*` prefix (never Go's `cmdline`/`memstats`) | — | diff --git a/internal/ingest/divergence.go b/internal/ingest/divergence.go index 627059a..5487462 100644 --- a/internal/ingest/divergence.go +++ b/internal/ingest/divergence.go @@ -70,14 +70,22 @@ const divergenceSweepTimeout = 2 * time.Minute // or deciding whether the report is crying wolf, has to be able to find it. // // TWELVE HOURS, derived rather than picked. Two envelopes make a pending -// delivery legitimately old: the retry schedule (DefaultMaxDeliveryAttempts=8 -// steps of DefaultBackoffBase=30s doubling to a one-hour cap, so roughly two to -// three hours before a failing delivery poisons) and the causal wait -// (DefaultCausalWaitBudget=6h, during which a reply sits pending for a parent -// that has not been accepted). A window inside either one reports the system -// working. Twelve hours clears both with room, and stays well inside "noticed -// the same day" — a queue that stopped moving this morning is in the report -// before the day ends. +// delivery legitimately old, and the SECOND one dominates: +// +// - The retry schedule: DefaultMaxDeliveryAttempts=8 with +// DefaultBackoffBase=30s doubling (capped at 1h). The waits between the 8 +// attempts are 30s, 1m, 2m, 4m, 8m, 16m, 32m — the cap never binds — which +// sums to 3810s, so a continuously failing delivery poisons after roughly +// ONE HOUR, not the "two to three hours" this comment claimed until +// 2026-08-14. (Recompute this if either constant moves; it is the +// derivation an operator re-checks.) +// - The causal wait: DefaultCausalWaitBudget=6h, during which a reply sits +// pending for a parent that has not been accepted. +// +// A window inside either one reports the system working, so the binding figure +// is the 6h causal wait. Twelve hours clears it with 2x room, and stays well +// inside "noticed the same day" — a queue that stopped moving this morning is +// in the report before the day ends. const DefaultAcceptanceStaleAfter = 12 * time.Hour // Divergence classes. The class is what an operator triages on, so it names the diff --git a/internal/outbound/worker.go b/internal/outbound/worker.go index 1f753f5..60fca98 100644 --- a/internal/outbound/worker.go +++ b/internal/outbound/worker.go @@ -561,8 +561,23 @@ const parkDelay = 5 * time.Second // park holds a kill-switched or dry-run delivery: it stays pending, scheduled a // REAL delay into the future so it is not instantly re-claimable, and never -// poisons. Release does not touch the attempt counter, so a park is not a -// failure and does not itself advance the poison budget. +// poisons — park does not call poison, and no state here is terminal. +// +// IT DOES, HOWEVER, ADVANCE THE POISON BUDGET, and this comment used to claim +// the opposite. Release's SET clause does not touch `attempts` — but ClaimNext +// already did `attempts = attempts + 1` to get here (store/outbound_deliveries.go), +// and nothing ever puts it back. So a delivery parked for parkDelay is +// re-claimed ~5s later, +1 again, and a kill switch held for ~40s exhausts +// maxAttempts. The delivery does not poison while parked, but the first +// retryable failure AFTER the switch clears sees Attempts >= maxAttempts in +// releaseOrPoison and poisons instead of retrying. +// +// Only RedrivePoisoned resets attempts to 0, and only for rows already in +// 'poisoned' — i.e. the repair exists but runs after the fall, not instead of +// it. Fixing this means teaching the store to distinguish "retryable failure, +// back off" (increment is correct) from "parked by a switch" (it is not); +// see FOLLOWUPS.md, "Outbound delivery (task 15)". Documented deliberately +// rather than patched here: it needs a failing test first. func (w *Worker) park(ctx context.Context, delivery *store.OutboundDelivery, class, reason string) error { next := time.Now().Add(parkDelay) _, _, err := w.deliveries.Release(ctx, delivery.ActivityID, delivery.TargetInbox, class, reason, 0, next, *delivery.ClaimedUntil) @@ -576,8 +591,13 @@ func (w *Worker) park(ctx context.Context, delivery *store.OutboundDelivery, cla // parkCausal holds a causally-ineligible delivery WITHOUT a future delay: a held // child must become claimable the instant its bridge-origin parent is accepted // (in practice the parent, a lower-seq delivery on the same serial line, is -// delivered first, so this rarely re-fires). Like park it never poisons and does -// not advance the poison budget; the causal wait is bounded by wall clock. +// delivered first, so this rarely re-fires). Like park it never poisons — but, +// like park, it DOES advance the poison budget, because ClaimNext incremented +// attempts and Release never puts it back (see park's doc above; this comment +// previously asserted the opposite). What keeps that from mattering here is the +// wall-clock bound: causalStatus poisons on the CausalWaitBudget deadline, not +// on the attempt count, so a held child's outcome is decided by elapsed time +// even after its retry budget is spent. func (w *Worker) parkCausal(ctx context.Context, delivery *store.OutboundDelivery, class, reason string) error { _, _, err := w.deliveries.Release(ctx, delivery.ActivityID, delivery.TargetInbox, class, reason, 0, time.Now(), *delivery.ClaimedUntil) if err != nil { -- 2.51.2