Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 12 additions & 22 deletions docs/adr/0001-sequence-ids-not-snowflakes.md
Original file line number Diff line number Diff line change
@@ -1,24 +1,14 @@
# Sequence ids, not snowflakes

`messages.id` is a plain BigInt identity column. Postgres assigns values
monotonically, which supplies the total ordering that cursor pagination
(`before_id`), catch-up (`after_id`), and client-side gap detection all read off
the primary key index.

## Rejected: snowflake ids

64-bit time-sortable ids assembled from timestamp, machine id, and sequence.
Their advantage is coordination-free generation across independent writers while
staying k-sortable. This service has one writer, Postgres, so the coordination
problem it solves does not exist here. Adopting them would add an id-generation
component and a machine-id assignment concern to demonstrate nothing the repo is
about.

Time-based ids would also be strictly worse for gap detection: clock skew makes
"is my next id contiguous with the last one I saw" unanswerable, whereas a
sequence makes it a comparison.

## Revisit trigger

Message writes originating from more than one process without passing through
Postgres, or a partitioned/sharded messages table.
`messages.id` is a plain BigInt identity column, so Postgres assigns values
monotonically and supplies the total ordering that cursor pagination
(`before_id`), catch-up (`after_id`) and client-side gap detection all read off
the primary key index. Snowflake ids, the usual choice for a chat backend, were
rejected: what they buy is coordination-free generation across independent
writers, and this service has exactly one writer, Postgres, so there is no
coordination problem to solve and an id-generation component plus a machine-id
assignment concern would be added to demonstrate nothing. Time-based ids would
also be strictly worse for gap detection, because clock skew makes "is my next
id contiguous with the last one I saw" unanswerable where a sequence makes it a
comparison. Message writes that reach storage without passing through Postgres,
or a partitioned `messages` table, would reopen this.
46 changes: 14 additions & 32 deletions docs/adr/0002-cookie-auth-not-bearer.md
Original file line number Diff line number Diff line change
@@ -1,34 +1,16 @@
# Cookie auth, not a bearer header

**Decision:** The JWT travels in a cookie (`JWTCookieAuth[Actor]`), not in
an `Authorization: Bearer` header.

## Context

Every endpoint shipped today is REST, where a bearer header is the more
conventional choice and keeps the token out of the browser's ambient
credential store. The realtime follow-on adds a server-sent-events stream.

## Decision & rationale

A browser `EventSource` cannot set request headers — there is no API for it.
An SSE endpoint authenticated by a bearer header would therefore need a
second authentication mechanism (a token in the query string, or a
short-lived ticket exchanged before connecting), which means two code paths
to keep in agreement and a token that lands in access logs.

A cookie is sent automatically on the `EventSource` request, so the stream
authenticates identically to every other endpoint with no second path. That
this repository exists to demonstrate a realistic composition is what settles
it: carrying two auth mechanisms to avoid a cookie would be the less
realistic shape.

The cost is accepted deliberately: cookie auth needs CSRF consideration on
state-changing endpoints, and `jwt_cookie_secure` must be `True` behind
HTTPS — see
[`0003-explicit-cookie-secure-flag.md`](0003-explicit-cookie-secure-flag.md).

## Revisit trigger

The SSE endpoint being dropped from scope, or a non-browser client becoming
the primary consumer.
The JWT travels in a cookie (`JWTCookieAuth[Actor]`), not in an
`Authorization: Bearer` header, although every endpoint shipped today is REST,
where a bearer header is conventional and keeps the token out of the browser's
ambient credential store. The planned server-sent-events stream settles it: a
browser `EventSource` cannot set request headers, so a bearer-authenticated
stream needs a second mechanism, a query-string token or a pre-connect ticket,
which is two auth paths to keep in agreement and a token that lands in access
logs. Two costs are accepted deliberately: state-changing endpoints need CSRF
consideration, and `jwt_cookie_secure` is an explicit setting defaulting to
`False` rather than derived from `service_environment`, because a security
property inferred from an unrelated string is one nobody audits, and because
`True` by default would break the local HTTP development this application is
demonstrated with. The anonymous surface is four anchored `exclude` prefixes:
`^/docs`, `^/health`, `^/static` and `^/metrics`.
18 changes: 18 additions & 0 deletions docs/adr/0003-domain-error-vocabulary.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
# Three domain exceptions, not one

`app/exceptions.py` defines `ChatAppError` with three subclasses, each mapped to
a status code by a handler registered in `build_app`: `PermissionDeniedError` to
`403` for authorization only, `ValidationError` to `400` for request shape ("a
direct chat must have exactly two distinct members"), and `ConflictError` to
`409` for state conflict, which today means editing a deleted message. The
initial design raised `PermissionDeniedError` for all three, and it is wrong
twice over: a body with three members in a direct chat is not a permissions
problem, and the author of a deleted message is authorized, so either inside a
"Permission denied" envelope tells the client to find credentials it already
has. Login failure is the mirror of that mistake, which is why `login` and
`GET /api/auth/me/` raise Litestar's `NotAuthorizedException`: a bad credential
is an identification failure, with no actor yet to authorize.
`advanced-alchemy`'s `NotFoundError`, `DuplicateKeyError` and `ForeignKeyError`
map to `404`, `409` and `400` with constant detail strings rather than
`str(exc)`, because an integrity error can carry bound parameters from another
row.
39 changes: 0 additions & 39 deletions docs/adr/0003-explicit-cookie-secure-flag.md

This file was deleted.

36 changes: 0 additions & 36 deletions docs/adr/0004-anonymous-doc-and-metrics-paths.md

This file was deleted.

16 changes: 16 additions & 0 deletions docs/adr/0004-mutation-requires-membership.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
# Mutation requires membership, not just authorship

`fetch_message_for_author` (`app/use_cases/message_authorization.py`) is the one
definition of the check order shared by `EditMessageUseCase` and
`DeleteMessageUseCase`: load the message (`404` if absent), verify the actor is
a member of its chat (`403`), then verify the actor is the author (`403`).
Authorship alone shipped first and left every authenticated user able to `PATCH`
or `DELETE` an arbitrary message id in a chat they had no visibility into, and
to distinguish "does not exist" from "exists, not mine" for it; authorship
happens to block the ordinary case, which is why the first round of tests passed
identically with and without the membership check. The rule is pinned by a test
that constructs the one state where the two disagree, deleting the author's
`chat_members` row. A non-member still learns whether a message id exists,
because the message must be loaded before its chat is known, and that residual
is accepted deliberately, mirroring `FetchChatUseCase` returning `403` rather
than pretending the chat does not exist.
44 changes: 0 additions & 44 deletions docs/adr/0005-domain-error-vocabulary.md

This file was deleted.

16 changes: 16 additions & 0 deletions docs/adr/0005-upsert-via-duplicate-key-recovery.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
# Upsert by recovering from DuplicateKeyError

`CreateChatUseCase` and `CreateMessageUseCase` both read first to see whether
the row already exists and then insert, 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. Both recovery paths are
exercised by repository subclasses (`_RacingChatsRepository`,
`_RacingMessagesRepository`) whose `create` raises and whose lookup misses once,
simulating the database condition at a seam the tests already own.
15 changes: 15 additions & 0 deletions docs/adr/0006-idempotency-scoped-per-chat.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
# Idempotency is scoped per chat

`messages` carries `UniqueConstraint("chat_id", "idempotency_key")`, and the
pre-check lookup, the constraint and the `DuplicateKeyError` recovery re-read
all filter on that same pair. 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 three
surfaces must agree, and aligning the constraint with the lookup was both the
cheaper direction and the one that matches the domain.
36 changes: 0 additions & 36 deletions docs/adr/0006-mutation-requires-membership.md

This file was deleted.

16 changes: 16 additions & 0 deletions docs/adr/0007-read-marker-integrity.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
# 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.
39 changes: 0 additions & 39 deletions docs/adr/0007-upsert-via-duplicate-key-recovery.md

This file was deleted.

Loading
Loading