Optimistic-lock TOCTOU in applyRemoteUpdate - stale remote write can clobber a newer local edit #96

New issue

Closed

opened 2026-07-15 19:51:30 +00:00 by claude-bot · 1 comment

claude-bot commented

2026-07-15 19:51:30 +00:00

Contributor

Severity: Medium · Confidence: Medium · Effort: S · Category: code

Evidence

api/src/services/remoteContribution.ts:125-153 - updated_at read via the pool outside the txn (:125); conflict decision (:135-137) and UPDATE (:149-152) happen later in a separate client txn with no re-check.

Problem
The optimistic-lock read happens outside the transaction that performs the write, with no re-check. Between read and write, a concurrent local edit or another delivery-worker job can bump updated_at; the stale remote update then overwrites the newer local edit - defeating the lock. Also inherently sensitive to inter-instance clock skew (host clock vs remote published).

Impact
Silent loss of a local edit under concurrent federated writes. Narrow window, requires an active remote editor.

Fix
Do the check inside the txn with SELECT . FOR UPDATE, or make the write conditional: UPDATE . WHERE id=$ AND updated_at <= $publishedAt and treat 0 rows as a conflict.

Acceptance criteria

A remote update whose basis is older than the current row is queued as a conflict, not applied.
Test simulates concurrent local-edit-then-remote-apply and asserts no lost update.

Filed from the 2026-07-15 codebase audit. Full report: docs/.internal/report-2026-07-15.md (gitignored).

**Severity:** Medium · **Confidence:** Medium · **Effort:** S · Category: code **Evidence** - `api/src/services/remoteContribution.ts:125-153` - `updated_at` read via the pool outside the txn (:125); conflict decision (:135-137) and UPDATE (:149-152) happen later in a separate client txn with no re-check. **Problem** The optimistic-lock read happens outside the transaction that performs the write, with no re-check. Between read and write, a concurrent local edit or another delivery-worker job can bump `updated_at`; the stale remote update then overwrites the newer local edit - defeating the lock. Also inherently sensitive to inter-instance clock skew (host clock vs remote `published`). **Impact** Silent loss of a local edit under concurrent federated writes. Narrow window, requires an active remote editor. **Fix** Do the check inside the txn with `SELECT . FOR UPDATE`, or make the write conditional: `UPDATE . WHERE id=$ AND updated_at <= $publishedAt` and treat 0 rows as a conflict. **Acceptance criteria** - [ ] A remote update whose basis is older than the current row is queued as a conflict, not applied. - [ ] Test simulates concurrent local-edit-then-remote-apply and asserts no lost update. --- _Filed from the 2026-07-15 codebase audit. Full report: `docs/.internal/report-2026-07-15.md` (gitignored)._

claude-bot added this to the v7.2.0 - Reliability, data integrity & docs milestone

2026-07-15 19:51:30 +00:00

claude-bot added the

bug

label

2026-07-15 19:51:30 +00:00

rbrooks referenced this issue from a commit

2026-07-17 02:28:29 +00:00

fix(v7.2): wave 1 — data integrity (#98, #93, #94, #96)

claude-bot commented

2026-07-17 02:32:08 +00:00

Author

Contributor

Fixed in 763aa0b (v7.2.0 wave 1).

Took the SELECT … FOR UPDATE option. The lock basis is now read inside the write transaction and the row is locked until commit:

await client.query('BEGIN');
const { rows: entryRows } = await client.query(
  `SELECT id, updated_at FROM entries
   WHERE id = $1 AND project_id = $2 AND deleted_at IS NULL
   FOR UPDATE`, [targetEntryId, ctx.projectId]);
// … conflict decision + UPDATE now happen under that lock

The pre-txn pool read is gone entirely (it was also a redundant second query). This serialises the two writers both ways: a concurrent local edit blocks until the remote write commits, and if the local edit committed first, we re-read its updated_at under the lock and correctly queue a conflict.

Acceptance criteria:

A remote update whose basis is older than the current row is queued as a conflict, not applied.
Test simulates concurrent local-edit-then-remote-apply and asserts no lost update — api/src/test/integration/remoteUpdateLock.test.ts.

The race test is deterministic rather than timing-based: it takes the row lock in the test's own transaction, calls applyRemoteUpdate (which then blocks inside its txn on FOR UPDATE), commits a newer local edit, and asserts the result is conflict and the entry still reads local edit. It fails against the old code — which read updated_at on the pool before the txn, saw the pre-edit value, decided "no conflict", waited on the lock, and then overwrote the local edit — so it genuinely pins the regression. Two companion tests cover the ordinary apply and plain-stale-conflict paths.

On the clock-skew sensitivity you noted: still inherent (the comparison is host updated_at vs remote published), and unchanged by this fix. #102 widens the AP signature freshness window for related reasons; worth revisiting the lock's skew tolerance separately if federated editing gets real use.

CI run 191 green (241/241).

Fixed in `763aa0b` (v7.2.0 wave 1). Took the `SELECT … FOR UPDATE` option. The lock basis is now read **inside** the write transaction and the row is locked until commit: ```ts await client.query('BEGIN'); const { rows: entryRows } = await client.query( `SELECT id, updated_at FROM entries WHERE id = $1 AND project_id = $2 AND deleted_at IS NULL FOR UPDATE`, [targetEntryId, ctx.projectId]); // … conflict decision + UPDATE now happen under that lock ``` The pre-txn pool read is gone entirely (it was also a redundant second query). This serialises the two writers both ways: a concurrent local edit blocks until the remote write commits, and if the local edit committed first, we re-read *its* `updated_at` under the lock and correctly queue a conflict. **Acceptance criteria:** - [x] A remote update whose basis is older than the current row is queued as a conflict, not applied. - [x] Test simulates concurrent local-edit-then-remote-apply and asserts no lost update — `api/src/test/integration/remoteUpdateLock.test.ts`. The race test is deterministic rather than timing-based: it takes the row lock in the test's own transaction, calls `applyRemoteUpdate` (which then blocks inside its txn on `FOR UPDATE`), commits a newer local edit, and asserts the result is `conflict` and the entry still reads `local edit`. **It fails against the old code** — which read `updated_at` on the pool before the txn, saw the pre-edit value, decided "no conflict", waited on the lock, and then overwrote the local edit — so it genuinely pins the regression. Two companion tests cover the ordinary apply and plain-stale-conflict paths. On the clock-skew sensitivity you noted: still inherent (the comparison is host `updated_at` vs remote `published`), and unchanged by this fix. #102 widens the AP signature freshness window for related reasons; worth revisiting the lock's skew tolerance separately if federated editing gets real use. CI run 191 green (241/241).

claude-bot closed this issue

2026-07-17 02:32:15 +00:00