fix(api): answer 401 on expired sessions; consolidate 13 error mappers
Every handler package carried its own near-identical writeError/
handleServiceError pair. Because each mapped a different subset of errors,
whatever a package forgot fell through to a 500 — most damagingly a dead
OAuth session on post, comment, and vote, which left clients with no signal
to re-authenticate and users stuck on "an internal error occurred" until
they signed out by hand.
The fix spans three layers, because the handler switch was only the visible
half: services were discarding the PDS cause before it ever reached the
boundary, and the expired-session path never produced a PDS error at all.
Changes:
- Add pds.ErrSessionExpired and IsReauthRequired. An unresumable OAuth
session fails at client construction, before any PDS request, so it had no
status to map; factory.go now tags it. Tagged narrowly, on
ErrSessionNotFound only — a store outage or cancelled request must not
read as expired, or a brief database blip would 401 every user in flight.
- Stop services swallowing the cause: 17 sites in votes, comments, and
communities now return fmt.Errorf("%w: %w", DomainSentinel, err) so the
domain sentinel and the PDS cause both stay matchable.
- Add internal/api/xrpc.Mapper, a rule table replacing all 13 switches.
Precedence is re-auth, then domain rules, then shared rules (typed
coreerrors, remaining PDS, request lifecycle), then a fallback. Re-auth
jumps the queue so a domain sentinel mapping to 403 cannot mask a 401.
- Strip pds sentinels from post delete: posts live in the community's repo,
so a rejected community credential must not tell the caller to sign in.
- Replace all 50 err == sentinel comparisons with errors.Is.
- Classify identity resolver errors in ResolveHandleToDID, so an unknown
handle is 404 and a resolver outage is 500 rather than both being 404.
- Delete the duplicate writers: handlers.WriteError, writeJSONError,
writeUpdateProfileError, and three inline switches in update_profile.go.
- Fix along the way: comments 409 on concurrent edit (was 500), community
429/413 (was 500), update_profile 400 (was 500), and routes/user.go
reporting a resolver outage as 404 ProfileNotFound.
- Add 30 tests: mapper precedence, per-package code contracts, session
classification, and handle resolution.
Error codes are a client contract and are preserved, with one deliberate
exception: a wrapped communities.ErrHandleTaken now answers NameTaken
instead of AlreadyExists. The old `err ==` never matched because the service
wraps it, so the create path always returned AlreadyExists; errors.Is now
matches and yields the code the switch plainly intended. Clients keying on
AlreadyExists for a handle collision need updating.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
fix(api): answer 401 on expired sessions; consolidate 13 error mappers
Every handler package carried its own near-identical writeError/
handleServiceError pair. Because each mapped a different subset of errors,
whatever a package forgot fell through to a 500 — most damagingly a dead
OAuth session on post, comment, and vote, which left clients with no signal
to re-authenticate and users stuck on "an internal error occurred" until
they signed out by hand.
The fix spans three layers, because the handler switch was only the visible
half: services were discarding the PDS cause before it ever reached the
boundary, and the expired-session path never produced a PDS error at all.
Changes:
- Add pds.ErrSessionExpired and IsReauthRequired. An unresumable OAuth
session fails at client construction, before any PDS request, so it had no
status to map; factory.go now tags it. Tagged narrowly, on
ErrSessionNotFound only — a store outage or cancelled request must not
read as expired, or a brief database blip would 401 every user in flight.
- Stop services swallowing the cause: 17 sites in votes, comments, and
communities now return fmt.Errorf("%w: %w", DomainSentinel, err) so the
domain sentinel and the PDS cause both stay matchable.
- Add internal/api/xrpc.Mapper, a rule table replacing all 13 switches.
Precedence is re-auth, then domain rules, then shared rules (typed
coreerrors, remaining PDS, request lifecycle), then a fallback. Re-auth
jumps the queue so a domain sentinel mapping to 403 cannot mask a 401.
- Strip pds sentinels from post delete: posts live in the community's repo,
so a rejected community credential must not tell the caller to sign in.
- Replace all 50 err == sentinel comparisons with errors.Is.
- Classify identity resolver errors in ResolveHandleToDID, so an unknown
handle is 404 and a resolver outage is 500 rather than both being 404.
- Delete the duplicate writers: handlers.WriteError, writeJSONError,
writeUpdateProfileError, and three inline switches in update_profile.go.
- Fix along the way: comments 409 on concurrent edit (was 500), community
429/413 (was 500), update_profile 400 (was 500), and routes/user.go
reporting a resolver outage as 404 ProfileNotFound.
- Add 30 tests: mapper precedence, per-package code contracts, session
classification, and handle resolution.
Error codes are a client contract and are preserved, with one deliberate
exception: a wrapped communities.ErrHandleTaken now answers NameTaken
instead of AlreadyExists. The old `err ==` never matched because the service
wraps it, so the create path always returned AlreadyExists; errors.Is now
matches and yields the code the switch plainly intended. Clients keying on
AlreadyExists for a handle collision need updating.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
refactor(server): bound HTTP and pool, extract internal/config, split cmd/server
Closes the p1 resilience gap in the backlog
(2026-07-22-no-db-pool-limits-no-http-timeouts).
The HTTP server was constructed with only Addr and Handler, so all four
net/http timeouts were zero — which means "no deadline". A client that
opened a connection and dribbled request headers held a goroutine and a
file descriptor indefinitely; enough of them exhaust the process without a
single complete request ever arriving. Separately, sql.Open was called with
no pool configuration at all, and database/sql defaults MaxOpenConns to
unlimited, so a traffic spike could open connections until PostgreSQL's
max_connections was exhausted — locking out psql and the cmd/ maintenance
tools along with the AppView.
Both are now bounded and env-tunable. Query time is bounded server-side via
statement_timeout injected into the DSN rather than per-query context
deadlines: lib/pq does send a CancelRequest on context cancellation, but
that path needs a second connection, races the query finishing, and does
nothing if the client dies outright. Schema migrations deliberately run on
a separate connection with statement_timeout stripped, since a CREATE INDEX
killed halfway is worse than a slow one.
main() was 1090 lines with 30 inline os.Getenv calls and no config struct.
Configuration now lives in internal/config, which applies defaults, rejects
malformed values, and enforces the requirements that differ between dev and
production — reporting every problem at once so a misconfigured deployment
is fixed in one pass instead of one restart per mistake. main() is now ~240
lines and returns an error rather than calling log.Fatal, which is what
makes the deferred cleanup reachable: os.Exit skips defers, so a fatal call
partway through startup abandoned the database pool and discarded the
buffered OpenTelemetry spans describing the failure.
Changes:
- Set ReadHeaderTimeout, ReadTimeout, WriteTimeout, IdleTimeout (cmd/server/httpserver.go)
- Configure the pool: MaxOpenConns, MaxIdleConns, ConnMaxLifetime, ConnMaxIdleTime (cmd/server/database.go)
- Inject statement_timeout into the app DSN; strip it for migrations (internal/config/dsn.go)
- Add internal/config with Load/Validate and per-subsystem config structs
- Split cmd/server into main, wiring, routes, consumers, database, httpserver, jobs, health, pds
- Embed goose migrations (internal/db/migrations/embed.go), removing the
working-directory dependency and the Dockerfile's migrations COPY
- Drain background work concurrently with the listener rather than after it,
so a slow in-flight request cannot consume the whole shutdown budget and
leave Jetstream cursors unflushed; drain on the listener-failure path too,
and report shutdown failures through the exit code
- Bound each background job cycle and recover per cycle rather than per
goroutine, so neither a panic nor a hang can silently kill the job
permanently; run a cycle at startup instead of waiting out the first tick
- Resolve the OAuth session store once at boot and fail loudly if absent,
rather than turning the cleanup job into a silent hourly no-op
- Add a context timeout to the instance PDS login, which used
http.DefaultClient and could hang the boot forever
- Document the new HTTP_* and DB_* variables in the env examples
Production now fails closed on OAUTH_SEAL_SECRET (base64, 32 bytes),
CURSOR_SECRET (rejects the documented CHANGE_ME placeholder), JETSTREAM_FEEDS,
APPVIEW_PUBLIC_URL and PDS_URL (must not be loopback), a non-DID INSTANCE_DID,
SKIP_DID_WEB_VERIFICATION, and a zero value for any HTTP timeout. The current
docker-compose.prod.yml and .env.prod.example satisfy all of these; a live
.env.prod with a short CURSOR_SECRET or a malformed OAUTH_SEAL_SECRET will
refuse to boot.
Verified: make test-all green across all three stages, make fmt-check clean,
go vet clean, race detector clean on the changed packages.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
fix(api): answer 401 on expired sessions; consolidate 13 error mappers
Every handler package carried its own near-identical writeError/
handleServiceError pair. Because each mapped a different subset of errors,
whatever a package forgot fell through to a 500 — most damagingly a dead
OAuth session on post, comment, and vote, which left clients with no signal
to re-authenticate and users stuck on "an internal error occurred" until
they signed out by hand.
The fix spans three layers, because the handler switch was only the visible
half: services were discarding the PDS cause before it ever reached the
boundary, and the expired-session path never produced a PDS error at all.
Changes:
- Add pds.ErrSessionExpired and IsReauthRequired. An unresumable OAuth
session fails at client construction, before any PDS request, so it had no
status to map; factory.go now tags it. Tagged narrowly, on
ErrSessionNotFound only — a store outage or cancelled request must not
read as expired, or a brief database blip would 401 every user in flight.
- Stop services swallowing the cause: 17 sites in votes, comments, and
communities now return fmt.Errorf("%w: %w", DomainSentinel, err) so the
domain sentinel and the PDS cause both stay matchable.
- Add internal/api/xrpc.Mapper, a rule table replacing all 13 switches.
Precedence is re-auth, then domain rules, then shared rules (typed
coreerrors, remaining PDS, request lifecycle), then a fallback. Re-auth
jumps the queue so a domain sentinel mapping to 403 cannot mask a 401.
- Strip pds sentinels from post delete: posts live in the community's repo,
so a rejected community credential must not tell the caller to sign in.
- Replace all 50 err == sentinel comparisons with errors.Is.
- Classify identity resolver errors in ResolveHandleToDID, so an unknown
handle is 404 and a resolver outage is 500 rather than both being 404.
- Delete the duplicate writers: handlers.WriteError, writeJSONError,
writeUpdateProfileError, and three inline switches in update_profile.go.
- Fix along the way: comments 409 on concurrent edit (was 500), community
429/413 (was 500), update_profile 400 (was 500), and routes/user.go
reporting a resolver outage as 404 ProfileNotFound.
- Add 30 tests: mapper precedence, per-package code contracts, session
classification, and handle resolution.
Error codes are a client contract and are preserved, with one deliberate
exception: a wrapped communities.ErrHandleTaken now answers NameTaken
instead of AlreadyExists. The old `err ==` never matched because the service
wraps it, so the create path always returned AlreadyExists; errors.Is now
matches and yields the code the switch plainly intended. Clients keying on
AlreadyExists for a handle collision need updating.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
fix(api): answer 401 on expired sessions; consolidate 13 error mappers
Every handler package carried its own near-identical writeError/
handleServiceError pair. Because each mapped a different subset of errors,
whatever a package forgot fell through to a 500 — most damagingly a dead
OAuth session on post, comment, and vote, which left clients with no signal
to re-authenticate and users stuck on "an internal error occurred" until
they signed out by hand.
The fix spans three layers, because the handler switch was only the visible
half: services were discarding the PDS cause before it ever reached the
boundary, and the expired-session path never produced a PDS error at all.
Changes:
- Add pds.ErrSessionExpired and IsReauthRequired. An unresumable OAuth
session fails at client construction, before any PDS request, so it had no
status to map; factory.go now tags it. Tagged narrowly, on
ErrSessionNotFound only — a store outage or cancelled request must not
read as expired, or a brief database blip would 401 every user in flight.
- Stop services swallowing the cause: 17 sites in votes, comments, and
communities now return fmt.Errorf("%w: %w", DomainSentinel, err) so the
domain sentinel and the PDS cause both stay matchable.
- Add internal/api/xrpc.Mapper, a rule table replacing all 13 switches.
Precedence is re-auth, then domain rules, then shared rules (typed
coreerrors, remaining PDS, request lifecycle), then a fallback. Re-auth
jumps the queue so a domain sentinel mapping to 403 cannot mask a 401.
- Strip pds sentinels from post delete: posts live in the community's repo,
so a rejected community credential must not tell the caller to sign in.
- Replace all 50 err == sentinel comparisons with errors.Is.
- Classify identity resolver errors in ResolveHandleToDID, so an unknown
handle is 404 and a resolver outage is 500 rather than both being 404.
- Delete the duplicate writers: handlers.WriteError, writeJSONError,
writeUpdateProfileError, and three inline switches in update_profile.go.
- Fix along the way: comments 409 on concurrent edit (was 500), community
429/413 (was 500), update_profile 400 (was 500), and routes/user.go
reporting a resolver outage as 404 ProfileNotFound.
- Add 30 tests: mapper precedence, per-package code contracts, session
classification, and handle resolution.
Error codes are a client contract and are preserved, with one deliberate
exception: a wrapped communities.ErrHandleTaken now answers NameTaken
instead of AlreadyExists. The old `err ==` never matched because the service
wraps it, so the create path always returned AlreadyExists; errors.Is now
matches and yields the code the switch plainly intended. Clients keying on
AlreadyExists for a handle collision need updating.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
fix(oauth,identity): require explicit PLC directory; refresh stale tests
An empty PLCURL silently meant the production plc.directory, so a config
that merely omitted the field resolved identities against production. The
same hazard sat in identity.DefaultConfig(), which hardcoded production and
was used unmodified by ~30 integration test sites - so test runs issued real
lookups upstream for DIDs that only exist on the local PLC.
Nothing here could ever WRITE to a directory: DID registration is a PDS
operation and the dev PDS is pinned to the local PLC container, so no
production records were at risk. The silent default was still a footgun, and
an empty field should never be the way you select production.
Changes:
- NewOAuthClient errors when PLCURL is empty instead of falling through to
indigo's default directory; the directory override is now unconditional
and the startup warning became a structured log line
- identity.DefaultConfig honors PLC_DIRECTORY_URL, which the Makefile
already exports from .env.dev, pointing every test resolver at the local
PLC without touching ~30 call sites
- OAuth test helpers name their directory explicitly; unit tests use an
unroutable address so an unintended resolution fails loudly rather than
quietly reaching production
Also refreshes four tests that pinned superseded behavior:
- TestValidateActorProfile: actor.profile has had no required fields since
81ecb15, so "missing required field" is no longer a failure mode; now
asserts the constraints the schema does enforce
- TestGetCommunityFeed_BlobURLTransformation: expects embed.external#view
per the postView union, since a thumb rewritten to a URL no longer fits
the record schema
- TestOAuth_SessionFixationAttackPrevention: 62f9066 replaced raw-400
dead-ends with a first-party 302, so "not 302" asserted the mechanism
rather than the property; now asserts the redirect is first-party and
carries no session material
- TestUserCreationAndRetrieval/Resolve Handle to DID: resolves the real
bretton.dev, so it gets its own production-pinned READ-ONLY resolver
make test-all passes: 30 packages across unit, integration and E2E.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
bretton.dev