Repository navigation
Fix: end-to-end optimistic concurrency for water corrections (codex review of #76) - #77
Merged
Merged
Conversation
…eview of #76) codex P1: the EF Version token only guarded the update handler's own read->save window — with no client-supplied base version, a stale form silently overwrote an intervening correction, and the parallel-update test was timing-flaky (codex's run had both PUTs return 204). The contract is now end-to-end: Version rides on every list row, PUT requires the base version, a mismatch is a deterministic 409, and the EF token backstops the residual in-request window. Race test rewritten to send the same base version from both writers — deterministic, no timing dependence — plus a sequential stale-form 409 test. Also from the review: - P2: update now enforces the same flock lifecycle gate as recording — an archived flock's water history is read-only (test). - P2: meter readings validated to 3 decimal places and a sane range; extra precision could round into Quantity != MeterEnd - MeterStart after numeric(18,3) persistence (test). - P2: cancelling/finishing an edit fully restores capture defaults (flock/date/source/unit) — it previously left the form aimed at the historical record's flock, possibly archived and absent from the picker. - P3: water source accepts named values only — numeric enum strings like "1" are refused (test). Declined as established app-wide conventions (noted on the PR): the idempotency-key rotate-after-refresh contract, and farm_id/house_id/ created_by columns (houses phantom until Phase 2; creator attribution arrives with the audit-log slice) — both identical to FeedUsage, reviewed twice there. 187 tests green. Live-verified: correct-with-base 204, stale-base 409, numeric enum 400, version increments on the row.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #76 (merged before codex's review landed — owner asked for a new PR). Full review table below.
The P1
The EF
Versiontoken only protected the update handler's internal read→save window. A user's actual read→edit→save cycle had no protection: load record, someone else corrects it, save — the second save re-read the fresh row and silently overwrote the first correction. The parallel-update test only passed by timing luck (codex's run had both PUTs return 204 — flaky).Fix — end-to-end optimistic contract:
versionon every list row → PUT requires the base version → mismatch is a deterministic 409 ("changed since you loaded it") → EF token backstops the residual in-request race. The SPA threads the loaded version through edit mode and shows a human conflict message. Race test now sends the same base version from both writers: deterministically one 204, one 409, no timing dependence.Review table (codex, 8 findings)
Quantity ≠ End − Startpossible after numeric(18,3) roundingresetFormrestores all capture defaults"1") accepted as sourcefarm_id/house_id/created_by(spec §12.5)187 tests green. Live-verified: correct-with-base 204 → stale-base 409 → row at version 1; numeric enum 400.