Add the update place endpoint, with optimistic concurrency #50

Merged

rob merged 3 commits from feat/update-place-endpoint into main

2026-08-05 03:29:44 +00:00

rob commented

2026-08-05 03:06:08 +00:00

Owner

Closes task 69. PUT /api/places/{id} edits a place's name, description, latitude and longitude, gated by GroupMayEditPlaces via the existing RequireGroupCapabilityForPlace filter (ADR-0045) — Editor/Owner may update, a Viewer (a member with insufficient role) gets 403, a non-member gets 404 indistinguishable from a nonexistent id (ADR-0041). group_id cannot be changed here, including by over-posting: UpdatePlaceRequest has no such property and PlaceRepository.UpdateAsync's SET clause has no way to write it.

Concurrency decision (ADR-0048): unlike task 76's group rename, this endpoint checks optimistic concurrency. A shared group can have several Editors, and a coordinate collision (two people dragging the same pin) leaves no visible diff for the loser to notice, unlike an overwritten group name. The token is Postgres's own xmin system column, read back as an opaque long — no schema change, no migration. PlaceResponse gains Version; UpdatePlaceRequest requires it back. A stale version, or a place deleted out from under the write, is refused with 409 rather than silently overwritten; the ADR argues the trade-offs and alternatives (dedicated version column, If-Match header) in full.

Took ADR number 0048, not 0047 — docs/adr/README.md's own "next free" rule: the in-flight feat/delete-group-endpoint branch already claims 0047 locally, ahead of being pushed as a PR.

Tests: Owner/Editor 200 (updated_at and Version advance), Viewer 403, non-member 404, nonexistent id 404, the two 404s byte-identical, group_id unchanged after an over-post attempt, coordinate/name validation 400, unauthenticated 401, stale-version 409 with the row left as the winning edit left it — all against a real Postgres via WebApplicationFactory + Testcontainers, plus repository-level concurrency tests.

Closes task 69. `PUT /api/places/{id}` edits a place's name, description, latitude and longitude, gated by `GroupMayEditPlaces` via the existing `RequireGroupCapabilityForPlace` filter (ADR-0045) — Editor/Owner may update, a Viewer (a member with insufficient role) gets 403, a non-member gets 404 indistinguishable from a nonexistent id (ADR-0041). `group_id` cannot be changed here, including by over-posting: `UpdatePlaceRequest` has no such property and `PlaceRepository.UpdateAsync`'s `SET` clause has no way to write it. **Concurrency decision (ADR-0048): unlike task 76's group rename, this endpoint checks optimistic concurrency.** A shared group can have several Editors, and a coordinate collision (two people dragging the same pin) leaves no visible diff for the loser to notice, unlike an overwritten group name. The token is Postgres's own `xmin` system column, read back as an opaque `long` — no schema change, no migration. `PlaceResponse` gains `Version`; `UpdatePlaceRequest` requires it back. A stale version, or a place deleted out from under the write, is refused with 409 rather than silently overwritten; the ADR argues the trade-offs and alternatives (dedicated version column, `If-Match` header) in full. Took ADR number **0048**, not 0047 — `docs/adr/README.md`'s own "next free" rule: the in-flight `feat/delete-group-endpoint` branch already claims 0047 locally, ahead of being pushed as a PR. Tests: Owner/Editor 200 (`updated_at` and `Version` advance), Viewer 403, non-member 404, nonexistent id 404, the two 404s byte-identical, `group_id` unchanged after an over-post attempt, coordinate/name validation 400, unauthenticated 401, stale-version 409 with the row left as the winning edit left it — all against a real Postgres via `WebApplicationFactory` + Testcontainers, plus repository-level concurrency tests.

rob added 1 commit

2026-08-05 03:06:08 +00:00

Add the update place endpoint, with optimistic concurrency

CI / build (pull_request) Successful in 2m11s

Details

b9df42eddd

rob reviewed

2026-08-05 03:13:21 +00:00

rob left a comment

Verdict: mergeable

Checked the headline decision hardest, since that's where the risk actually is.

Race safety is real, not decorative. I added a Task.WhenAll test firing two UpdateAsync calls at the same row with the same version and ran it against a real container — exactly one succeeds, the other gets null. The concurrency control lives entirely in the atomic UPDATE … WHERE id = @Id AND xmin::text::bigint = @ExpectedVersion; the second GetByIdAsync in PutUpdatePlace only runs after that write has already lost, purely to pick 404 vs 409 for the response — it plays no part in deciding who wins. Not a check-then-act race.

Worth adding to the suite: PlaceRepositoryTests only proves the stale-version case sequentially (write, then write-again-with-old-token). Nothing in the PR itself exercises genuine DB-level contention, which is the one claim this whole ADR rests on. Suggest lifting something like the Task.WhenAll test I used into PlaceRepositoryTests before this ships, so the proof lives in the repo rather than in a reviewer's scratch worktree.

xmin round-trips correctly — xmin::text::bigint consistently on read, write and compare; no narrowing, no type mismatch.

Freeze/wraparound: traced through it, and the ADR's one-line treatment undersells its own case. VACUUM FREEZE rewrites a row's xmin to the reserved marker 2, which no real transaction id ever takes. Because the WHERE clause compares against the row's current value, a stale token can only ever fail to match after a real intervening change (safe 409) — it can't retroactively become 2, since a caller only ever reads 2 from a row that is already frozen, not from one that later gets frozen out from under them. So freezing can produce spurious 409s on a row that's gone untouched for tens of millions of write transactions (real but distant at this app's scale, and self-recovering — reload and retry), never a silent "unchanged" false positive. True 32-bit wraparound collision is additionally guarded by Postgres's own anti-wraparound autovacuum forcing freezes well before that point. Net: no reachable data-loss path. ADR-0048's "the value is guaranteed unique enough to detect a change… never a global ordering or a count of edits" gestures at this without actually walking through freeze, which is exactly the sharp edge a reader would come to this ADR wanting reassurance on. Worth a paragraph making the freeze-can't-cause-false-negatives argument explicit, rather than leaving it implied. Not blocking — the mechanism is sound — but the record should show the reasoning, not just the conclusion.

Everything else checked out: Viewer 403 / non-member 404 byte-identical (own test, passing), group_id immutable including the over-post route (no property to bind, SET clause has no column for it), coordinate/name validation matches create, updated_at advances, entities stay off the wire (PlaceResponse/UpdatePlaceRequest only, explicit mapping in the handler). The list-places rebase only added the xmin column to the existing SELECT/ORDER BY — didn't touch the ordering itself, and the full suite (ListPlacesEndpointTests included) passes clean against the pinned SDK.

Verdict: mergeable Checked the headline decision hardest, since that's where the risk actually is. **Race safety is real, not decorative.** I added a `Task.WhenAll` test firing two `UpdateAsync` calls at the same row with the same version and ran it against a real container — exactly one succeeds, the other gets `null`. The concurrency control lives entirely in the atomic `UPDATE … WHERE id = @Id AND xmin::text::bigint = @ExpectedVersion`; the second `GetByIdAsync` in `PutUpdatePlace` only runs after that write has already lost, purely to pick 404 vs 409 for the response — it plays no part in deciding who wins. Not a check-then-act race. **Worth adding to the suite**: `PlaceRepositoryTests` only proves the stale-version case sequentially (write, then write-again-with-old-token). Nothing in the PR itself exercises genuine DB-level contention, which is the one claim this whole ADR rests on. Suggest lifting something like the `Task.WhenAll` test I used into `PlaceRepositoryTests` before this ships, so the proof lives in the repo rather than in a reviewer's scratch worktree. **`xmin` round-trips correctly** — `xmin::text::bigint` consistently on read, write and compare; no narrowing, no type mismatch. **Freeze/wraparound: traced through it, and the ADR's one-line treatment undersells its own case.** `VACUUM FREEZE` rewrites a row's `xmin` to the reserved marker `2`, which no real transaction id ever takes. Because the WHERE clause compares against the row's *current* value, a stale token can only ever fail to match after a real intervening change (safe 409) — it can't retroactively become `2`, since a caller only ever reads `2` from a row that is *already* frozen, not from one that later gets frozen out from under them. So freezing can produce spurious 409s on a row that's gone untouched for tens of millions of write transactions (real but distant at this app's scale, and self-recovering — reload and retry), never a silent "unchanged" false positive. True 32-bit wraparound collision is additionally guarded by Postgres's own anti-wraparound autovacuum forcing freezes well before that point. Net: no reachable data-loss path. ADR-0048's "the value is guaranteed unique enough to detect a change… never a global ordering or a count of edits" gestures at this without actually walking through freeze, which is exactly the sharp edge a reader would come to this ADR wanting reassurance on. Worth a paragraph making the freeze-can't-cause-false-negatives argument explicit, rather than leaving it implied. Not blocking — the mechanism is sound — but the record should show the reasoning, not just the conclusion. Everything else checked out: Viewer 403 / non-member 404 byte-identical (own test, passing), `group_id` immutable including the over-post route (no property to bind, `SET` clause has no column for it), coordinate/name validation matches create, `updated_at` advances, entities stay off the wire (`PlaceResponse`/`UpdatePlaceRequest` only, explicit mapping in the handler). The list-places rebase only added the `xmin` column to the existing `SELECT`/`ORDER BY` — didn't touch the ordering itself, and the full suite (`ListPlacesEndpointTests` included) passes clean against the pinned SDK.

rob added 1 commit

2026-08-05 03:18:47 +00:00

Pin the update-place concurrency race, and argue xmin freeze safety

CI / build (pull_request) Successful in 2m19s

Details

d11b644a47

rob reviewed

2026-08-05 03:23:31 +00:00

rob left a comment

Verdict: mergeable

Re-reviewed the delta only (b9df42e → d11b644): PlaceRepositoryTests.cs and ADR-0048.

The race test is genuinely concurrent, not accidentally serialised. The shared NpgsqlDataSource used here (AddPlaceMarkDatabase) takes Npgsql's default Maximum Pool Size of 100 — only the health-probe pool is capped, at 5, and that's a separate keyed registration the repository never touches. 20 writers comfortably fit without queueing for a connection.

Confirmed directly rather than assumed: reverted UpdateAsync locally to a check-then-act shape (a separate SELECT of xmin, an if on the caller's expected version, an artificial 20ms delay, then an UPDATE … WHERE id = @Id with no version guard) and reran the new test — it reddened correctly, all 20 writers succeeding. Restored the real implementation and reran; green. Ran the real test 8 more times standalone plus once more in the full suite (9 runs total) with no flake.

ADR-0048's expanded xmin section is accurate and does not overclaim. Walked the mechanics independently before reading the write-up: MVCC leaves a dead tuple's xmin alone and always mints a fresh, live one on write; VACUUM FREEZE is the only thing that rewrites a live tuple's xmin in place, to the reserved constant 2, which no ordinary transaction is ever assigned; any subsequent real edit immediately un-freezes the row by writing a new tuple with a genuine transaction id. That combination is what makes a false match (silent data loss) require actual 32-bit wraparound, which PostgreSQL's own anti-wraparound autovacuum and last-resort write refusal exist to prevent — a false conflict (spurious 409) is the only failure shape actually reachable, and it's the safe direction. The write-up reaches the same conclusion by the same route, correctly separates the two failure shapes, and no longer asserts the vaguer "guaranteed unique enough" line the previous revision leaned on. The named revisit condition — autovacuum disabled/broken past its own wraparound safeguard, or transaction volume growing enough that tens of millions of transactions could elapse inside one edit-collision window rather than months or years — is the right one; it's also, correctly, framed as "this would already be an incident on its own terms" rather than a plausible silent failure at this application's scale.

Nothing else in the diff changed outside these two files.

Verdict: mergeable Re-reviewed the delta only (`b9df42e` → `d11b644`): `PlaceRepositoryTests.cs` and ADR-0048. **The race test is genuinely concurrent, not accidentally serialised.** The shared `NpgsqlDataSource` used here (`AddPlaceMarkDatabase`) takes Npgsql's default `Maximum Pool Size` of 100 — only the health-probe pool is capped, at 5, and that's a separate keyed registration the repository never touches. 20 writers comfortably fit without queueing for a connection. Confirmed directly rather than assumed: reverted `UpdateAsync` locally to a check-then-act shape (a separate `SELECT` of `xmin`, an `if` on the caller's expected version, an artificial 20ms delay, then an `UPDATE … WHERE id = @Id` with no version guard) and reran the new test — it reddened correctly, all 20 writers succeeding. Restored the real implementation and reran; green. Ran the real test 8 more times standalone plus once more in the full suite (9 runs total) with no flake. **ADR-0048's expanded `xmin` section is accurate and does not overclaim.** Walked the mechanics independently before reading the write-up: MVCC leaves a dead tuple's `xmin` alone and always mints a fresh, live one on write; `VACUUM FREEZE` is the only thing that rewrites a live tuple's `xmin` in place, to the reserved constant `2`, which no ordinary transaction is ever assigned; any subsequent real edit immediately un-freezes the row by writing a new tuple with a genuine transaction id. That combination is what makes a false match (silent data loss) require actual 32-bit wraparound, which PostgreSQL's own anti-wraparound autovacuum and last-resort write refusal exist to prevent — a false conflict (spurious 409) is the only failure shape actually reachable, and it's the safe direction. The write-up reaches the same conclusion by the same route, correctly separates the two failure shapes, and no longer asserts the vaguer "guaranteed unique enough" line the previous revision leaned on. The named revisit condition — autovacuum disabled/broken past its own wraparound safeguard, or transaction volume growing enough that tens of millions of transactions could elapse inside one edit-collision window rather than months or years — is the right one; it's also, correctly, framed as "this would already be an incident on its own terms" rather than a plausible silent failure at this application's scale. Nothing else in the diff changed outside these two files.

rob added 1 commit

2026-08-05 03:25:23 +00:00

Merge remote-tracking branch 'origin/main' into feat/update-place-endpoint

CI / build (pull_request) Successful in 2m13s

Details

dc38529cb2