From cc491eed80a8b72dbe54b5fc06394687653bc7db Mon Sep 17 00:00:00 2001 From: Artur Shiriev Date: Sat, 17 Oct 2026 11:04:44 +0300 Subject: [PATCH] docs: merge the upsert ADRs, drop replaced and non-ADR records, renumber ADRs that a later decision replaces are now deleted once their reasoning moves into the new record. --- AGENTS.md | 6 ++++++ docs/adr/0005-idempotency-scoped-per-chat.md | 8 +++++++ .../0005-upsert-via-duplicate-key-recovery.md | 21 ------------------- docs/adr/0006-idempotency-scoped-per-chat.md | 14 ------------- docs/adr/0006-read-marker-integrity.md | 9 ++++++++ docs/adr/0007-read-marker-integrity.md | 16 -------------- ...=> 0007-repoint-last-message-on-delete.md} | 0 ...id.md => 0008-auth-carries-an-actor-id.md} | 0 ...009-coverage-exclusions-structural-only.md | 15 ------------- ...y-jti.md => 0009-logout-revokes-by-jti.md} | 2 +- ...-ping-off.md => 0010-pool-pre-ping-off.md} | 0 .../0011-idempotent-creates-are-one-upsert.md | 20 ++++++++++++++++++ docs/adr/0012-message-send-is-one-upsert.md | 19 ----------------- .../0013-direct-chat-create-is-one-upsert.md | 15 ------------- docs/adr/README.md | 21 +++++++------------ tests/use_cases/test_unread_counts.py | 5 +++++ 16 files changed, 56 insertions(+), 115 deletions(-) create mode 100644 docs/adr/0005-idempotency-scoped-per-chat.md delete mode 100644 docs/adr/0005-upsert-via-duplicate-key-recovery.md delete mode 100644 docs/adr/0006-idempotency-scoped-per-chat.md create mode 100644 docs/adr/0006-read-marker-integrity.md delete mode 100644 docs/adr/0007-read-marker-integrity.md rename docs/adr/{0008-repoint-last-message-on-delete.md => 0007-repoint-last-message-on-delete.md} (100%) rename docs/adr/{0010-auth-carries-an-actor-id.md => 0008-auth-carries-an-actor-id.md} (100%) delete mode 100644 docs/adr/0009-coverage-exclusions-structural-only.md rename docs/adr/{0011-logout-revokes-by-jti.md => 0009-logout-revokes-by-jti.md} (93%) rename docs/adr/{0012-pool-pre-ping-off.md => 0010-pool-pre-ping-off.md} (100%) create mode 100644 docs/adr/0011-idempotent-creates-are-one-upsert.md delete mode 100644 docs/adr/0012-message-send-is-one-upsert.md delete mode 100644 docs/adr/0013-direct-chat-create-is-one-upsert.md diff --git a/AGENTS.md b/AGENTS.md index 9c0f919..ceb9346 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -187,6 +187,12 @@ env vars (see `docker-compose.yml`). `api_bootstrapper_config` builds the `ForeignKey`, a `return` from inside a transaction block, discarded work that is not dead code (`AuthenticateUserUseCase`'s hash-anyway). Alembic's own `# ###` autogenerate markers stay — they are regenerated on every migration. +- **Coverage.** The suite runs at `--cov-fail-under=100` and no `# pragma: no cover` + appears anywhere. The only exemptions are structural: `[tool.coverage.run] omit` + for files the default run never imports, and `exclude_also` for + `if typing.TYPE_CHECKING:`. A branch that looks untestable gets a test that + produces its condition for real, such as the concurrent create tests' + second connection holding an uncommitted insert. - `ruff` is configured with `select = ["ALL"]` and a line length of 120 — expect strict lint. Type-check with `ty`. diff --git a/docs/adr/0005-idempotency-scoped-per-chat.md b/docs/adr/0005-idempotency-scoped-per-chat.md new file mode 100644 index 0000000..01fa079 --- /dev/null +++ b/docs/adr/0005-idempotency-scoped-per-chat.md @@ -0,0 +1,8 @@ +# Idempotency is scoped per chat + +`messages` carries `UniqueConstraint("chat_id", "idempotency_key")`, and the +send's `ON CONFLICT` names that constraint. Idempotency belongs to the operation +"send this message to this chat", so a key reused in another chat is a different +operation, not a retry. A global constraint on `idempotency_key` shipped first. +A client that reused a key across chats got the other chat's message back, and +the message it meant to send was never written. diff --git a/docs/adr/0005-upsert-via-duplicate-key-recovery.md b/docs/adr/0005-upsert-via-duplicate-key-recovery.md deleted file mode 100644 index c54a6a3..0000000 --- a/docs/adr/0005-upsert-via-duplicate-key-recovery.md +++ /dev/null @@ -1,21 +0,0 @@ ---- -superseded_by: 0013-direct-chat-create-is-one-upsert ---- - -# Upsert by recovering from DuplicateKeyError - -`CreateChatUseCase` reads first to see whether the direct chat already exists -and then inserts, but the read is only an optimisation: -the correctness guarantee is the `except DuplicateKeyError` branch, which rolls -back and re-reads the row the winner committed. The pre-check alone does not -make the insert safe. At READ COMMITTED two concurrent "open a DM with Bob" -requests both miss the read, both insert, and the loser violates -`uq_chats_direct_key`, which surfaces as a `409` to a user who should simply -have received the existing chat. `@postgres_retry` does not rescue it either, -because `db-retry` retries serialization and connection failures, not integrity -violations, and `SELECT ... FOR UPDATE` has nothing to take, since the race is -between two inserts of a row that does not yet exist. The recovery path is -exercised by a repository subclass, `_RacingChatsRepository`, whose `create` -raises and whose lookup misses once, simulating the database condition at a seam -the tests already own. Message sends used this pattern too until -[0012](0012-message-send-is-one-upsert.md) replaced it with a single statement. diff --git a/docs/adr/0006-idempotency-scoped-per-chat.md b/docs/adr/0006-idempotency-scoped-per-chat.md deleted file mode 100644 index 1a02bec..0000000 --- a/docs/adr/0006-idempotency-scoped-per-chat.md +++ /dev/null @@ -1,14 +0,0 @@ -# Idempotency is scoped per chat - -`messages` carries `UniqueConstraint("chat_id", "idempotency_key")`, and the -send's `ON CONFLICT` names that constraint. Idempotency is a property of an operation, and the -operation is "send this message to this chat", so two chats are two operations -and a key reused across them is not a retry of anything. A global constraint on -`idempotency_key` shipped first and made a reachable state look unreachable: -cross-chat key reuse under concurrency raised `DuplicateKeyError` from the -insert, the chat-scoped re-read missed, and control reached a guard whose only -justification was "the unique constraint guarantees a match here". Worse, a -client reusing a key across chats received the other chat's message while its -intended message was never written, a silent wrong-row return. The surfaces -had to agree, and aligning the constraint with the lookup was both the cheaper -direction and the one that matches the domain. diff --git a/docs/adr/0006-read-marker-integrity.md b/docs/adr/0006-read-marker-integrity.md new file mode 100644 index 0000000..61daf05 --- /dev/null +++ b/docs/adr/0006-read-marker-integrity.md @@ -0,0 +1,9 @@ +# Read-marker integrity + +`MarkReadUseCase` rejects a `last_read_message_id` that does not name a message +in that chat with a `400`, and moving the marker to a lower id is a silent +no-op. Accepting any id would let a client set its marker arbitrarily high and +permanently zero its own unread counts, and storing self-inflicted corruption is +worse than refusing the request. A lower id usually comes from a replayed or +reordered request, which is not a client mistake worth reporting, so it gets the +current marker back instead of an error. diff --git a/docs/adr/0007-read-marker-integrity.md b/docs/adr/0007-read-marker-integrity.md deleted file mode 100644 index 5948bce..0000000 --- a/docs/adr/0007-read-marker-integrity.md +++ /dev/null @@ -1,16 +0,0 @@ -# Read-marker integrity - -`MarkReadUseCase` verifies membership, verifies that `last_read_message_id` -names a message in that chat, then advances the marker in one UPDATE that sets -it to `GREATEST(COALESCE(last_read_message_id, 0), :requested)`. Computing the -maximum in Python from a prior read would let two concurrent `POST /read/` calls -interleave and the lower id win, the regression the monotonic rule exists to -prevent; `GREATEST` inside the UPDATE makes it atomic without a lock. Accepting -any id would let a client set its marker arbitrarily high and permanently zero -its own unread counts, and persisting self-inflicted corruption is worse than -refusing the request. Unread counts the messages above the marker whose -`user_id IS DISTINCT FROM` the member's; `IS DISTINCT FROM` rather than `!=` is -load-bearing, because system messages carry `user_id IS NULL` and `NULL != 1` -evaluates to NULL, silently dropping every one of them. Advancing to a lower id -is a silent no-op, since a replayed request is not a client mistake worth -reporting. diff --git a/docs/adr/0008-repoint-last-message-on-delete.md b/docs/adr/0007-repoint-last-message-on-delete.md similarity index 100% rename from docs/adr/0008-repoint-last-message-on-delete.md rename to docs/adr/0007-repoint-last-message-on-delete.md diff --git a/docs/adr/0010-auth-carries-an-actor-id.md b/docs/adr/0008-auth-carries-an-actor-id.md similarity index 100% rename from docs/adr/0010-auth-carries-an-actor-id.md rename to docs/adr/0008-auth-carries-an-actor-id.md diff --git a/docs/adr/0009-coverage-exclusions-structural-only.md b/docs/adr/0009-coverage-exclusions-structural-only.md deleted file mode 100644 index edd2e47..0000000 --- a/docs/adr/0009-coverage-exclusions-structural-only.md +++ /dev/null @@ -1,15 +0,0 @@ -# Coverage exclusions are structural only - -The suite runs at `--cov-fail-under=100`, and exemptions are structural only: -`[tool.coverage.run] omit` for files the default pytest run never imports at -all, each carrying an inline reason in `pyproject.toml` where the path does not -explain itself, plus `exclude_also` for `if typing.TYPE_CHECKING:` blocks. No -`# pragma: no cover` appears in `app/`, `tests/` or `migrations/`. "Awkward to -reach" is not a warrant: where a branch looks untestable, the answer is to -produce the database condition for real, as the concurrent create tests do with -a second connection holding an uncommitted insert. Pragmas on unreachable-in-production guards were argued -for twice and refused, because an excluded branch is one nobody notices when it -stops being unreachable, and the coverage number then asserts something untrue -about what the tests exercise; tests written only to move the number were -removed for the same reason. `filterwarnings = ["error"]` enforces the companion -property, so a new warning fails a test rather than scrolling past. diff --git a/docs/adr/0011-logout-revokes-by-jti.md b/docs/adr/0009-logout-revokes-by-jti.md similarity index 93% rename from docs/adr/0011-logout-revokes-by-jti.md rename to docs/adr/0009-logout-revokes-by-jti.md index 6283ea1..220e5d9 100644 --- a/docs/adr/0011-logout-revokes-by-jti.md +++ b/docs/adr/0009-logout-revokes-by-jti.md @@ -8,7 +8,7 @@ that user, and short-lived tokens with refresh because they replace the whole auth model. The cost is one primary-key lookup per authenticated request, on a session the middleware closes before the handler opens its own, so a request still holds one pooled connection at a time. That lookup is the read -[ADR-0010](0010-auth-carries-an-actor-id.md) kept out of authentication; the +[ADR-0008](0008-auth-carries-an-actor-id.md) kept out of authentication; the user row is still never loaded. A token without a `jti`, which includes every token issued before this change, gets a 401. Each logout prunes expired rows, so the table holds at most a token lifetime of logouts. diff --git a/docs/adr/0012-pool-pre-ping-off.md b/docs/adr/0010-pool-pre-ping-off.md similarity index 100% rename from docs/adr/0012-pool-pre-ping-off.md rename to docs/adr/0010-pool-pre-ping-off.md diff --git a/docs/adr/0011-idempotent-creates-are-one-upsert.md b/docs/adr/0011-idempotent-creates-are-one-upsert.md new file mode 100644 index 0000000..94e4b7d --- /dev/null +++ b/docs/adr/0011-idempotent-creates-are-one-upsert.md @@ -0,0 +1,20 @@ +# Idempotent creates are one INSERT ... ON CONFLICT DO UPDATE + +Message sends (`MessagesRepository.get_or_create`, conflicting on +`uk_messages_chat_id_idempotency_key`) and direct chats +(`ChatsRepository.get_or_create_direct`, conflicting on `direct_key`) are each +written by one `INSERT ... ON CONFLICT ... DO UPDATE SET = EXCLUDED. +RETURNING ..., xmax = 0`. The no-op update returns the existing row on a replay, +and a request racing an uncommitted insert of the same key waits on its lock and +then gets the winner's row. `xmax = 0` is true only for a row version the +statement inserted, so it decides 201 or 200 and whether the follow-up writes +run. A read before the insert cannot make it safe: at READ COMMITTED both +requests miss the read, `postgres_retry` does not retry the loser's integrity +error, and `SELECT ... FOR UPDATE` has no row to lock yet. Re-reading after a +`DuplicateKeyError` worked but cost a round trip and ran outside the retried +transaction; `ON CONFLICT DO NOTHING` returns no row to the loser. A replay +writes a row version and consumes a sequence value, so a retried send leaves a +gap that [ADR-0001](0001-sequence-ids-not-snowflakes.md)'s gap detection answers +with one empty fetch, and a DM replay takes three statements where a pre-check +took two. Postgres does not document `xmax` in `RETURNING`; the concurrent +tests in `test_create_message.py` and `test_create_chat.py` would catch a change. diff --git a/docs/adr/0012-message-send-is-one-upsert.md b/docs/adr/0012-message-send-is-one-upsert.md deleted file mode 100644 index 7a761b9..0000000 --- a/docs/adr/0012-message-send-is-one-upsert.md +++ /dev/null @@ -1,19 +0,0 @@ -# A message send is one INSERT ... ON CONFLICT DO UPDATE - -`MessagesRepository.get_or_create` writes a send as one `INSERT ... ON CONFLICT -ON CONSTRAINT uk_messages_chat_id_idempotency_key DO UPDATE SET idempotency_key -= EXCLUDED.idempotency_key RETURNING ..., xmax = 0`. The no-op update makes a -replay return the existing row from the same statement, and a request racing an -uncommitted insert of the same key waits on that row's lock, then gets the -winner's row. `xmax = 0` is true only for a row version the statement inserted, -so the use case can answer 201 or 200 and skip the `last_message_id` update on a -replay. The read-then-insert with `DuplicateKeyError` recovery from -[0005](0005-upsert-via-duplicate-key-recovery.md) cost two round trips per new -message and re-read the winner outside the retried transaction. `ON CONFLICT DO -NOTHING` returns no row to the loser, and advanced-alchemy's `upsert` reads -first. The cost: every replay writes a row version and consumes a sequence -value, so a retried send leaves a gap in message ids that -[0001](0001-sequence-ids-not-snowflakes.md)'s client gap detection answers with -one empty catch-up fetch. Postgres does not document `xmax` in `RETURNING`; the -replay and concurrent tests in `tests/use_cases/test_create_message.py` would -catch a change. diff --git a/docs/adr/0013-direct-chat-create-is-one-upsert.md b/docs/adr/0013-direct-chat-create-is-one-upsert.md deleted file mode 100644 index 4594a34..0000000 --- a/docs/adr/0013-direct-chat-create-is-one-upsert.md +++ /dev/null @@ -1,15 +0,0 @@ -# A direct chat is created with one upsert - -`ChatsRepository.get_or_create_direct` uses the statement shape from -[0012](0012-message-send-is-one-upsert.md): `INSERT ... ON CONFLICT (direct_key) -DO UPDATE SET direct_key = EXCLUDED.direct_key RETURNING ..., xmax = 0`. A -concurrent request for the same pair waits on the row lock and gets the -winner's chat. Members are inserted only when the chat is new, and -`fetch_with_members` loads the result inside the same transaction. This replaces -the `DuplicateKeyError` recovery in -[0005](0005-upsert-via-duplicate-key-recovery.md), whose re-read ran outside the -retried transaction. The cost lands on replay, the common path: opening an -existing DM now takes three statements (upsert, SELECT, selectin) where the -pre-check took two. Keeping replay at two meant inserting or selecting members -by hand and attaching them to the viewonly relationship, which was rejected as -more machinery than one statement is worth. diff --git a/docs/adr/README.md b/docs/adr/README.md index 7378e89..de2b3b4 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -2,7 +2,7 @@ One file per decision taken, especially the options **rejected**, so reviews do not re-litigate them. The directory listing is the index: there is no generated -listing and no `summary` frontmatter. Nothing validates the set mechanically; +listing and no frontmatter. Nothing validates the set mechanically; the standard below is held up by review. ## Numbering @@ -14,20 +14,13 @@ sequence stays contiguous, and every citation moves in the same change: `tests/test_adr_citations.py` fails on a `docs/adr/` path that no longer resolves. -## Status lives in the frontmatter, or nowhere +## Every record is current -An ADR with no frontmatter is **accepted**. A decision that is re-argued and -replaced is superseded, not deleted, so the earlier reasoning stays readable. A -record that no longer passes the admission test below is deleted outright; git -history keeps it. - -When a later ADR supersedes an earlier one, add to the earlier file: - -```yaml ---- -superseded_by: 0010-its-slug ---- -``` +Every file here is an accepted decision, so ADRs carry no status frontmatter. +When a later decision replaces an earlier one, the reasoning that still holds, +usually why the rejected options fail, moves into the new record and the old +file is deleted. A record that no longer passes the admission test below is +deleted too. Git history keeps both. ## The admission test diff --git a/tests/use_cases/test_unread_counts.py b/tests/use_cases/test_unread_counts.py index 05f8590..7d2a24d 100644 --- a/tests/use_cases/test_unread_counts.py +++ b/tests/use_cases/test_unread_counts.py @@ -47,6 +47,10 @@ async def test_system_messages_count_as_unread( direct_chat: tables.ChatsTable, alice: Actor, ) -> None: + """INVARIANT: system messages (user_id IS NULL) count as unread. + + Broken by comparing user_id with != instead of IS DISTINCT FROM, since NULL != 1 is NULL. + """ await messages_repository.create( tables.MessagesTable(chat_id=direct_chat.id, user_id=None, idempotency_key=uuid.uuid4(), text="Bob joined") ) @@ -175,6 +179,7 @@ async def test_marking_read_is_monotonic( bob: Actor, send: SendFixture, ) -> None: + """INVARIANT: marking read with a lower id leaves the marker where it is and raises nothing.""" first, _ = await send(bob, direct_chat.id, "one") second, _ = await send(bob, direct_chat.id, "two") await mark_read_use_case(