rbrooks/Quest-Board

Watch

Fork 0

fix(api): conditional writes so concurrent edits stop clobbering each other (#403) #450

Merged

claude-bot merged 1 commit from fix/403-conditional-writes into main

2026-08-31 01:36:18 +00:00

claude-bot commented

2026-08-30 00:38:39 +00:00

Contributor

Closes #403 (HIGH).

The defect

Every mutable row was written last-write-wins with no version check of any kind. Two writers moments apart — two people, or a person and a background job — meant one edit simply vanished. No error, no merge, nothing in a log.

The scenario the issue is built around: a player opens a session on a laptop (which loads the private note once at mount), types notes from their phone all evening via the Discord /note command — which correctly appends server-side — then clicks Save on the laptop out of habit. PUT /my-note is a full replacement, so the evening's notes are replaced with what the page loaded hours earlier, and the response is a cheerful 200.

A deliberate departure from the proposed fix

The issue suggests making the note editor append or merge instead of replace. I implemented a conditional write instead.

Appending is wrong for a full-text editor: a GM correcting a typo would get their entire note duplicated rather than fixed. That trades silent loss for silent duplication, which is not an improvement — it is the same class of bug wearing a different hat. Refusing the stale write loses nothing in either direction, and the UI never discards what the user typed: only an explicit Reload replaces the draft, and only when they ask for it.

All four acceptance criteria are still met.

What ships

expected_updated_at on three write paths — the note upsert, PATCH /sessions/{id} (summary/transcript/title), and the beat-notes PATCH. A mismatch is a 409 carrying a message written for a human; the frontend shows it inline next to the thing that conflicted, with a Reload control.

Session.updated_at is now exposed on SessionResponse — it has existed on the model since #107 and was never sent to anyone, so no client could compare-and-swap even if it wanted to.

The token is optional by design. The bot's append path is already correct and needs none, older clients keep working, and requiring it would be a breaking API change for a guarantee that is purely additive. The protection is real because the clients that do full replacements are exactly the ones that send it.

Two things the tests forced out

The guard did not work at first. Session.updated_at relies on onupdate=func.now(), and Postgres now() is transaction start time — so two writes inside one transaction get identical timestamps and the token stops distinguishing them. This codebase already hit that exact trap in migration d5e6f7a8b0c1, which switched created_at to clock_timestamp() for the same reason. The write paths now stamp explicitly from the wall clock, which is what upsert_note always did.

SessionListItem now carries updated_at too. Without it, an editor working from a session list had to make a second round trip just to learn the version it was editing — and could save unconditionally in the window before that trip returned. Adding it to the list schema removes both the fetch and the gap.

Verification

Mutation-checked in two independent places:

Guard disabled: 5 tests fail, including the issue's own laptop-versus-phone scenario.
Only the explicit updated_at stamp reverted, guard intact: the stale-summary test fails on its own.

Also asserted directly: a naive datetime from a client is read as UTC rather than raising (a concurrency guard that 500s is worse than none), and a one-microsecond difference is still a conflict.

The 409 branch keys on e.status, which I checked against the real ApiError class rather than trusting the test's mock — it does carry the HTTP status, so the conflict UI is reachable in production and not just in tests.

1,450 backend tests pass (was 1,439). 452 frontend (was 449). Lint clean at CI's pinned ruff 0.4.4; eslint clean on every touched file.

Not done here

workbenchTools.jsx's AI-tool beat-notes append/replace actions still write unconditionally. They are a deliberate "apply this generated text" action rather than a stale-editor save, and they have no natural reload UX, so they take the documented optional-token path. Worth revisiting if it ever bites, but expanding scope to it now would be speculative.

Noted while in there and not fixed, as it is unrelated to this issue: CampaignPlanning.jsx reads beat_notes off session list items, which have never carried that field — so the beat-notes editor appears to open empty regardless of what is stored. Happy to file it separately.

🤖 Generated with Claude Code

Closes #403 (HIGH). ## The defect Every mutable row was written last-write-wins with no version check of any kind. Two writers moments apart — two people, or a person and a background job — meant one edit simply vanished. No error, no merge, nothing in a log. The scenario the issue is built around: a player opens a session on a laptop (which loads the private note once at mount), types notes from their phone all evening via the Discord `/note` command — which correctly appends server-side — then clicks Save on the laptop out of habit. `PUT /my-note` is a **full replacement**, so the evening's notes are replaced with what the page loaded hours earlier, and the response is a cheerful 200. ## A deliberate departure from the proposed fix The issue suggests making the note editor **append or merge** instead of replace. I implemented a **conditional write** instead. Appending is wrong for a full-text editor: a GM correcting a typo would get their entire note duplicated rather than fixed. That trades silent loss for silent duplication, which is not an improvement — it is the same class of bug wearing a different hat. Refusing the stale write loses nothing in either direction, and the UI never discards what the user typed: only an explicit **Reload** replaces the draft, and only when they ask for it. All four acceptance criteria are still met. ## What ships `expected_updated_at` on three write paths — the note upsert, `PATCH /sessions/{id}` (summary/transcript/title), and the beat-notes PATCH. A mismatch is a 409 carrying a message written for a human; the frontend shows it inline next to the thing that conflicted, with a Reload control. `Session.updated_at` is now exposed on `SessionResponse` — it has existed on the model since #107 and was never sent to anyone, so no client could compare-and-swap even if it wanted to. **The token is optional by design.** The bot's append path is already correct and needs none, older clients keep working, and requiring it would be a breaking API change for a guarantee that is purely additive. The protection is real because the clients that do full replacements are exactly the ones that send it. ## Two things the tests forced out **The guard did not work at first.** `Session.updated_at` relies on `onupdate=func.now()`, and Postgres `now()` is *transaction* start time — so two writes inside one transaction get identical timestamps and the token stops distinguishing them. This codebase already hit that exact trap in migration `d5e6f7a8b0c1`, which switched `created_at` to `clock_timestamp()` for the same reason. The write paths now stamp explicitly from the wall clock, which is what `upsert_note` always did. **`SessionListItem` now carries `updated_at` too.** Without it, an editor working from a session list had to make a second round trip just to learn the version it was editing — and could save unconditionally in the window before that trip returned. Adding it to the list schema removes both the fetch and the gap. ## Verification Mutation-checked in two independent places: - Guard disabled: **5 tests fail**, including the issue's own laptop-versus-phone scenario. - Only the explicit `updated_at` stamp reverted, guard intact: the stale-summary test fails on its own. Also asserted directly: a naive datetime from a client is read as UTC rather than raising (a concurrency guard that 500s is worse than none), and a one-microsecond difference is still a conflict. The 409 branch keys on `e.status`, which I checked against the real `ApiError` class rather than trusting the test's mock — it does carry the HTTP status, so the conflict UI is reachable in production and not just in tests. **1,450 backend tests pass** (was 1,439). **452 frontend** (was 449). Lint clean at CI's pinned ruff 0.4.4; eslint clean on every touched file. ## Not done here `workbenchTools.jsx`'s AI-tool beat-notes append/replace actions still write unconditionally. They are a deliberate "apply this generated text" action rather than a stale-editor save, and they have no natural reload UX, so they take the documented optional-token path. Worth revisiting if it ever bites, but expanding scope to it now would be speculative. Noted while in there and **not** fixed, as it is unrelated to this issue: `CampaignPlanning.jsx` reads `beat_notes` off session **list** items, which have never carried that field — so the beat-notes editor appears to open empty regardless of what is stored. Happy to file it separately. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

claude-bot added 1 commit

2026-08-30 00:38:40 +00:00

fix(api): conditional writes so concurrent edits stop clobbering each other (#403 )

CI / Bot/backend version sync (pull_request) Successful in 47s

Details

CI / Backend lint (ruff) (pull_request) Successful in 53s

Details

CI / Summarisation accuracy eval harness (stub provider) (pull_request) Successful in 1m26s

Details

CI / Frontend tests, audit, and build (pull_request) Successful in 1m53s

Details

CI / Bot tests and audit (pull_request) Successful in 1m57s

Details

CI / Docker image build (pull_request) Successful in 4m7s

Details

CI / Backend migration, tests, and audit (pull_request) Successful in 8m26s

Details

23b7cfc62e

Notes, summaries and beat notes were written last-write-wins with no
version check, so two writers moments apart meant one edit vanished with no
error and no trace. The sharpest case: a laptop loads a private note at
page mount, the Discord /note command appends to it all evening, and a Save
clicked out of habit at the end of the night replaces the lot.

Writes now carry the version they were based on, and a mismatch is a 409
the client turns into "reload before saving".

Deliberately NOT the append the issue suggested. Appending is wrong for a
full-text editor — correcting a typo would duplicate the whole note instead
of fixing it, trading silent loss for silent duplication. Refusing the
write loses nothing either way, and the UI never discards what was typed:
only an explicit Reload replaces the draft.

expected_updated_at is optional throughout. The bot's append path is
already correct and needs no token, older clients keep working, and
requiring it would be a breaking change for a purely additive guarantee.
The clients that do full replacements are the ones that send it.

Two things the tests forced out. Session.updated_at relies on
onupdate=func.now(), and Postgres now() is transaction start time, so two
writes in one transaction tie and the token stops distinguishing them —
the trap migration d5e6f7a8b0c1 already hit with created_at. The write
paths now stamp explicitly, as upsert_note always did. And SessionListItem
carries updated_at so an editor working from a list holds the token
immediately, with no window in which its first save goes through unguarded.

Mutation-checked: disabling the guard fails 5 tests including the issue's
own laptop-versus-phone scenario; reverting only the explicit stamp fails
the summary test alone. 1,450 backend and 452 frontend tests pass.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

claude-bot merged commit 508cb640ab into main

2026-08-31 01:36:18 +00:00

claude-bot deleted branch fix/403-conditional-writes

2026-08-31 01:36:19 +00:00

claude-bot referenced this pull request from a commit

2026-08-31 01:36:21 +00:00

Merge PR #450: conditional writes so concurrent edits stop clobbering each other (#403)

claude-bot referenced this pull request

2026-08-31 01:37:23 +00:00

[Frontend] "Append to beat notes" silently replaces them — the session list never carried beat_notes #454