Skip to content

feat: events with idempotency and fan-out, delivery reads, and CORS (steps 3.4–3.6) - #33

Open
devinder-dev wants to merge 2 commits into
feat/api-endpointsfrom
feat/api-events
Open

devinder-dev wants to merge 2 commits into
feat/api-endpointsfrom
feat/api-events

Conversation

@devinder-dev

Copy link
Copy Markdown
Owner

Stacked on #32 (which is stacked on #31). The base is feat/api-endpoints, so the diff
shows only this step. Once #31 and #32 are merged I move the base to main and rebase (same
changes, a new push), so it needs your approval once more after that.

What

Steps 3.4–3.6 of Phase 3, the last part of the core API: POST /v1/events, reading
deliveries (GET /v1/deliveries, GET /v1/deliveries/:id), and CORS for the dashboard.
With this, an event goes in and its pending deliveries can be seen in the API and the
dashboard. Sending them is Phase 4 (the worker).

POST /v1/events   Idempotency-Key: k1
 sql.begin:
   INSERT INTO events … ON CONFLICT (owner_id, idempotency_key) DO NOTHING RETURNING …
    ├─ new row  → INSERT INTO deliveries … SELECT … FROM endpoints (live, this type or "*")
    │             → 202 { id, type, created_at, deliveries }
    └─ no row   → SELECT the original event + count of its deliveries (a separate statement)
                  → 202 the same body + Idempotent-Replayed: true

Why it works this way

  • One transaction. The event and its deliveries commit together or not at all. A test makes
    the fan-out insert a delivery and then fail: neither the event nor the delivery is saved.
  • The database enforces idempotency, not our code. UNIQUE (owner_id, idempotency_key)
    with ON CONFLICT DO NOTHING: a second request with the same key waits at the unique index
    until the first commits, then inserts nothing. The original is read by a separate
    statement, because under READ COMMITTED each statement gets a fresh view and so sees the
    first request's commit. Five requests with one key at the same moment give 1 event, 1
    creator and 4 replays (held at a real lock, no sleeps).
  • The key wins. The same key with a different body returns the first answer and saves
    nothing new. Refusing it would need a new error code in the contract.
  • Retrying is safe with a key. After a 503, a network error or a 500, the producer retries
    with the same key and gets the original answer, or the event is created now. A lost answer
    after COMMIT returns the original deliveries count, even if an endpoint was added since.
    docs/api.md now says this; it never promises that an error means nothing was saved.
  • Unstorable text is a 422, not a 500. Postgres refuses \u0000 in jsonb (22P05) and a
    lone surrogate like "\ud800" (22P02). Only the event insert maps exactly these two codes
    to validation_failed with path: "payload"; anywhere else they stay a 500 (our own bug).
  • Keyset pagination. WHERE d.id < $cursor ORDER BY d.id DESC LIMIT limit + 1. UUIDv7 ids
    are ordered by time, so "older" is "smaller id"; as fast on page 100 as on page 1, and new
    rows don't shift the next page. The extra row tells us there is a next page without a count
    query; next_cursor is the id of the last row sent. It's stable paging, not a frozen
    snapshot: a row committed late can land behind a page already read, and the dashboard
    refreshes.
  • limit only accepts digits. "", " 5" and 1e2 are refused instead of quietly turned
    into numbers.
  • Every join is also on owner_id. Our foreign keys are single-column, so the database
    would accept a delivery of owner B pointing at owner A's event. The joins are the guard: a
    test plants exactly such rows and they are never shown.
  • next_attempt_at lives in one SQL fragment. The column is never NULL (while a row is
    leased it holds the lease's expiry); the API shows it only for pending, in the list and
    the detail.
  • One 404 for a malformed, missing or another owner's delivery id, so nobody can probe
    which ids exist (and a malformed id never reaches Postgres as a 500).
  • CORS is not our security. The key travels in a header, not a cookie, and curl ignores
    CORS. It only decides which websites' JavaScript may read our answers: cdn.myrelay.tech
    and localhost:4173 (both in the contract). @fastify/cors is registered at the root,
    before the /v1 auth hook, because the browser's preflight carries no key; a 401 still
    carries the CORS headers, so the dashboard can read the error. strictPreflight: false, so
    an OPTIONS without preflight headers gets 204 instead of the plugin's plain-text 400.

Changes

  • API: schemas/events.ts, repositories/events.ts, services/events.ts,
    routes/events.ts; schemas/deliveries.ts, repositories/deliveries.ts (fan-out, list,
    detail, attempts), services/deliveries.ts, routes/deliveries.ts; routes/v1.ts
    registers them; app.ts registers CORS.
  • Dependency: @fastify/cors pinned to exactly 11.3.1 (released 8 Oct; 11.4.0 is from
    2 Nov, too new for us).
  • Tests: events (fan-out to exactly the right endpoints, including a deleted one and
    another owner's; no match → 0; repeated key; another owner's key; no key; the 5-request race;
    lost answer; changed body; failed fan-out rolls back), event validation (every field, the
    key, NUL in a value, a key and nested, both surrogates, an emoji as a positive control, 413,
    415, 401), deliveries (pages of 2, 2, 1 with no gaps, filters, strict query validation),
    isolation (B can't list, filter for or read A's deliveries; planted inconsistent rows stay
    hidden), detail (the example row in docs/api.md field by field, deleted endpoint, attempt
    numbering with a tie, next_attempt_at in list and detail), CORS (preflight from both
    origins, a foreign origin, no Origin, a 401 with CORS headers, health unchanged).
  • Docs: docs/api.md (key rules, the key wins, retry advice, limit digits, malformed id
    → 404).

No migration: the tables and indexes are from migration 001.

Tested

  • bun run typecheck, bun run test:db: 288 pass, 0 fail.
  • Negative controls: removing ON CONFLICT, the NUL/surrogate mapping, owner_id on the
    event join, next_attempt_at's rule, or using the extra row as the cursor each turns the
    matching test red.
  • Snyk Code: 0 issues. bun audit: no vulnerabilities (64 packages).

After merge, checked from outside

  • POST /v1/events twice with the same Idempotency-Key and type: "live.check" → 202 twice,
    the same id and count, the replay header only on the second.
  • GET /v1/deliveries?limit=2 → the delivery, pending; numbers are numbers.
  • OPTIONS /v1/events with Origin: https://cdn.myrelay.tech and the preflight headers → 204,
    allow-origin cdn; the same with Origin: https://evil.example → no allow-origin; with no
    headers → 204, no allow-origin.

Careful: deliveries created in production now will be sent once the worker runs
(Phase 4). An event reaches every endpoint that lists its type or *, so in production we
only create endpoints that point at receivers we own, and send live.check events: then the
worker only ever sends to us.

…steps 3.4–3.6)

- POST /v1/events: the event and one delivery per matching live endpoint
  in one transaction; the same Idempotency-Key returns the first answer
  (ON CONFLICT DO NOTHING, then a separate SELECT) with
  Idempotent-Replayed: true. NUL and lone surrogates in the payload are 422.
- GET /v1/deliveries: keyset pages (limit + 1, cursor = last row sent),
  filters by status and endpoint, every join also on owner_id.
- GET /v1/deliveries/:id: event, endpoint (also when deleted) and the
  numbered attempt log; malformed, missing and foreign ids are the same 404.
- CORS for the dashboard origins, registered before the auth hook.
@AbdikhaniAbdirashid

Copy link
Copy Markdown
Collaborator

Read all 23 files. Approving. Three findings, and the first one lands on my side of the project so I
need an answer rather than a change.

1. POST /v1/deliveries/:id/replay is specified but not implemented, and the dashboard already
uses it.
docs/api.md defines it. My dashboard/src/mockStore.ts has replayDelivery(), marked
"stands in for POST /v1/deliveries/:id/replay", and it drives a real button on the delivery detail
screen. The moment I replace the mock store with the axios client, that button gets a 404. Same
question as DELETE /v1/endpoints/:id on #32: which step do these two land in? I'll keep the mock
behind them until they exist rather than ship a dead button — just tell me which, so I plan Phase 5
around it.

2. CORS allows an origin our own docs forbid. app.ts hardcodes
["https://cdn.myrelay.tech", "http://localhost:4173"], while docs/CONTEXT.md says "CORS:
https://cdn.myrelay.tech only". The risk is low — a browser won't let a page forge its Origin —
but production currently accepts an origin the contract doesn't mention. You already solved this
shape properly for SSRF_ALLOW_ORIGINS: validated in config.ts, empty by default, operators only.
I'd rather the same treatment here than two sources of truth. Either that or update
CONTEXT.md and docs/api.md to name both. This is the same drift you caught in my ADR 0005, so
I'm raising it the same way.

3. The idempotency correctness depends on READ COMMITTED, and that only lives in a comment.
ON CONFLICT (owner_id, idempotency_key) DO NOTHING followed by a separate SELECT is right, and
right for a reason I had to think about: the losing insert waits for the winner to commit, and under
READ COMMITTED the next statement gets a fresh snapshot and sees the winning row. Under REPEATABLE
READ that SELECT would use the transaction's original snapshot, find nothing, and hit
throw new Error("Conflicting event could not be found") — a 500 on a perfectly ordinary retry. The
behaviour is load-bearing and invisible from outside the file. Worth a line in an ADR or
docs/api.md, so nobody "hardens" the isolation level later and breaks retries.

Also worth stating in the contract rather than only in a comment: a repeated Idempotency-Key with a
different body returns the original answer instead of 409. Stripe and Svix return 409 there. Your
comment says body comparison needs a new contract error code, so I read it as deliberate — I just
want the contract to say so, because it's the kind of thing we'll be asked to justify.

What's good, in my own words. One sql.begin() covering the event insert and the fan-out means
there is never an event with half its deliveries — the thing that would otherwise be a dual-write
problem. The fan-out matching exact type OR '*' is one statement, no loop. Keyset pagination with
d.id < cursor and ORDER BY d.id DESC works only because the ids are UUIDv7, so id order is time
order; LIMIT limit + 1 to learn whether a next page exists is cheaper than a second count query.
next_attempt_at is hidden unless the delivery is pending, because during a lease that column holds
the lease expiry and would otherwise leak worker internals. And every response is built field by
field, so signing_secret cannot ride along by accident — the same discipline as the endpoint list.

Merging in order 31 -> 32 -> 33 since they're stacked.

@AbdikhaniAbdirashid AbdikhaniAbdirashid left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved. Need an answer on the replay route before I wire the dashboard to the real API, but it doesn't block this merge.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants