chadtmiller.com/knotless

Switch repository

Loading…

What changes

An #account event saying an account was deleted drops what knotless-serve holds of it only when an answer says it is gone: its DID document is 404 or 410 (a PLC tombstone), or the host that document names answers getRepoStatus with RepoNotFound, or with active: false and status: deleted. Active, or inactive for any other reason (deactivated, suspended, taken down, logged), it is kept. Anything else, a network error, a timeout, a 5xx, no #atproto_pds service, an answer without active, is asked again: 30 s, doubling to a 10-minute cap, six asks in all, then the account is kept.

accounts::gone returns Asked::{Gone, Active, Unknown}; accounts::settle does the asking again and purges only on Gone. accounts is public so the tests can reach them. docs/design/serve.md (Accounts that go) says which answers count.

Why

deleted() returned true for any failed lookup: identity::resolve failing for any reason, or getRepoStatus failing or saying anything but active: true. A PLC outage, a host restarting or a timeout while a deletion event came in dropped an account's index rows, mirrors and sessions for good, as did a host answering deactivated to one. grain's port of this code had the same flaw and fixed it this way (crates/grain-serve/src/accounts.rs), which this follows.

The document is read with identity::document, not resolve, so the check sees the identity as PLC has it now rather than a cached one, and can tell a 404/410 from an outage.

How it was checked

crates/knotless-serve/tests/accounts.rs, against a stand-in that is PLC and every account's host: each answer above maps to the Asked it should (active, deactivated, suspended, taken down; deleted, RepoNotFound, tombstone, unregistered; PLC 503, host 502, host unreachable, no active, no host); settle keeps one active or deactivated with its mirrors, purges one whose PLC fails once and then is deleted, and gives up on and keeps one PLC never answers. The existing deletion flow in tests/forge.rs (an account still active kept; one deleted at the reference PDS, RepoNotFound, dropped) still holds. Built and tested on the runner.

Shortcomings

A retry lives in a spawned task: a restart during the half hour forgets it, and the account is kept, the safe way to fail. A host that answers active: false with a status of its own that is not deleted keeps the account, as one that can be undone.

Changeset: none

This makes an #account deletion event drop an account only when an answer says it is gone: the DID document is 404 or 410, or the host answers RepoNotFound, or active: false with status: deleted. Any other answer is Unknown and is asked again with backoff, then the account is kept. I found nothing that must change.

• http_status: identity::document wraps error_for_status() in anyhow context. That keeps the reqwest::Error in the error chain, so http_status finds the 404/410 as intended.
• Tests: they cover each answer and the retry paths, and settle is exercised for kept, purged and gave-up.
• Docs: docs/design/serve.md is updated with the table of answers.
• Checks: test, check, docs and e2e were still queued when I looked, so the 100% coverage gate is unconfirmed. Watch for the Asked::Unknown and gone/settle log branches.
• did:web: a 404 from a did:web host counts as gone, so a misconfigured host that briefly returns 404 would drop the account. The docs table states this, so I take it as intended.

This round adds only telemetry. The tracing log lines in accounts::gone and accounts::settle now go through a new telemetry::account_asked helper. The deletion logic and the tests are unchanged from the version I approved. The new helper maps cleanly onto the logging fields, and the log level at each call site matches the one it replaces.

The test, check, docs and e2e checks were still queued when I looked, so the build and the 100% coverage gate are unconfirmed on this commit. Check that the new account_asked helper and its call sites are covered, since it now holds the branches that were bare tracing calls.

I found nothing that must change.

All seven checks pass at df1146135ba7, test's 100% coverage gate among them, so account_asked and every branch of gone and settle are covered. On did:web: a 404 or 410 for the DID document is gone whichever way it is read, as with PLC. A did:web host that answers 404 for its own did.json has no resolvable identity at that moment, and nothing short of that 404 drops the account. Kept as it is.