Skip to content

feat(api): optimistic concurrency (version + If-Match) for mutable records (#49) - #161

Closed
DevRushd wants to merge 10 commits into
DocForum:mainfrom
DevRushd:feat/optimistic-concurrency-version-if-match-for
Closed

DevRushd wants to merge 10 commits into
DocForum:mainfrom
DevRushd:feat/optimistic-concurrency-version-if-match-for

Conversation

@DevRushd

Copy link
Copy Markdown
Contributor

What Changed

Schema — version on the mutable models

  • backend/prisma/schema.prisma — version Int @default(1) on User, PatientProfile, DoctorProfile, FacilityProfile, FacilityCapability, Appointment, PaymentIntent.
  • backend/prisma/migrations/20261019090000_add_optimistic_concurrency/migration.sql — ADD COLUMN "version" INTEGER NOT NULL DEFAULT 1 on those seven tables, with a comment recording what is deliberately excluded and why.

Middleware

  • backend/src/middleware/optimistic-concurrency.ts (new) — four shared pieces so every module answers identically:
    • setVersionEtag(res, version) — strong ETag: "<int>" on an individual-resource read.
    • requireIfMatch — the PATCH/PUT guard. Missing header → 428 (IF_MATCH_REQUIRED); anything that isn't one strong "<int>" (weak validator, validator list, unquoted, garbage) → 400 (INVALID_IF_MATCH). The version lands on req.expectedVersion, matching the existing req.user / req.requestId pattern.
    • expectedVersion(req) — throws rather than defaulting, so a missing precondition can never silently skip the guard.
    • versionConflict() — the single 412 VERSION_CONFLICT every stale write returns.

Reference implementation — PATCH /doctors/me

  • modules/doctors/routes — new PATCH /me, ordered authenticate → requireRole('doctor') → requireIfMatch → updateMe so an unauthorised caller gets 401/403 instead of a hint that the route exists.
  • modules/doctors/repositories — updateDoctorProfile(id, version, fullName) using updateManyAndReturn on WHERE id = ? AND version = ? with version: { increment: 1 } in the same statement. updateManyAndReturn (not updateMany + re-read) so the response body is the row this statement wrote; a follow-up findUnique would open a second window for someone else's write to land in.
  • modules/doctors/services — updateMyDoctorProfile; the caller's version is only used to find the profile id, the guard itself lives in the repository statement.
  • modules/doctors/validation — updateDoctorProfileSchema (.strict(), so a body carrying verificationStatus is rejected rather than ignored).
  • modules/doctors/controllers — updateMe; GET /doctors/me now publishes the ETag.

Already-conditional transitions bump the version without gaining a second guard

  • modules/payments/repositories — markEscrowed / markStatus increment version inside their existing status-conditional writes.
  • modules/doctors/repositories — updateVerificationStatus increments too.
  • modules/auth/repositories — markUserEmailVerified increments User.version.
  • modules/payments/controllers — every intent response (create, read, fund, release, refund) publishes the ETag and the body's version.

Docs

  • ARCHITECTURE.md §5.4 — the convention as a numbered rule new modules must follow; §4.1/§4.2 — the version columns annotated in the data-model mirror, including the rationale for what is not versioned; §6.1 item 6 — the lost-update risk now marked resolved, with the remaining gap stated.
  • ARCHITECTURE_ESSENTIALS.md — hard rule 7 rewritten (a compare-then-write is now called out as forbidden, not just discouraged) and a pointer added to the data-model map.
  • docs/api/README.md — an "Optimistic concurrency" section: the status table, what clients must send, what to do on 412, and which routes are explicitly not gated.
  • ROADMAP.md — Phase 8 checklist item with status, plus a changelog entry.

Why

Booking (§5.1) is the only write in this backend protected by a conditional update. Every other mutable record is last-write-wins: a doctor renames their profile in one tab while an admin action lands in another, or two tabs edit the same appointment — both writes succeed, the second silently destroys the first, and neither actor is ever told. Silent data loss on a clinical record is a correctness bug and a patient-safety one, and it gets more likely as Phases 2–5 add mutable state.

The fix has to be a single conditional statement, not a read-compare-write. Read-compare-write looks equivalent and isn't: two callers both read version 4, both pass a check made before either write, and the second write still wins. The WHERE ... AND version = ? has to be the write itself so the database, not the application, arbitrates.

Two guardrails are deliberate rather than incidental:

  • Already-conditional transitions are not re-gated. Booking, payment fund/release/refund and doctor verification each already fail on an observed-state mismatch (409). Adding If-Match on top gives one write two unrelated failure modes — a client retrying after a 409 would hit a 412 it can't act on, and a stale ETag would mask a genuine status conflict. They keep their own guard and only publish the new version.
  • Append-only Prescription/LabOrder are excluded. A correction is a new supersedes_* row (hard rule 1), so there is no update whose result could be lost. Giving them a version would imply an edit path the architecture forbids.

How it Was Tested

backend/tests/integration/optimistic-concurrency.test.mts (new, 12 cases, real embedded Postgres on its own port, escrow SDK faked):

  • Stale write → 412. Read, write, then replay the original ETag: 412 VERSION_CONFLICT, the first writer's value is still stored, and the version advanced exactly once — not twice. Re-reading and retrying the loser's intent then succeeds, proving the rejection is recoverable rather than a dead end.
  • Concurrent proof. 8 simultaneous PATCHes carrying the same ETag → exactly one 200, seven 412s; the stored row's fullName is the winner's and its version is before + 1. This is the lost-update bug, fired at once.
  • 428 for clients that don't send If-Match — with code: 'IF_MATCH_REQUIRED' and a message that says what to do, plus an assertion that nothing was written.
  • 400 for weak validators — W/"2", "2", "3", 2, abc, "-1", "0" all rejected, version unchanged. Picking one member of a list is how silent overwrites come back.
  • Ordering: anonymous → 401, wrong role → 403 (both before the precondition check); invalid or over-reaching body → 400 with the version unchanged.
  • No double-guard: POST /appointments still books with no If-Match (201); doctor verification still flips with no If-Match and bumps the version; fund → release still work with no If-Match and walk the version 1 → 2 → 3, while a second release still loses on status (409), not on version.
  • Schema shape: asserts the exact set of tables carrying a version column via information_schema, so the exclusion can't quietly widen.
  • System writes: email verification moves User.version 1 → 2.

Local: npm run typecheck, npm test (93 unit), npm run test:integration (169 across 9 files), npm run build — all green.

Tracking Reference

Closes #49

…cords (DocForum#49)

Booking was the only write protected by a conditional update. Every other
mutable record was last-write-wins: a doctor editing their profile in one
tab and an admin in another, or two tabs on one appointment, both wrote and
the second silently erased the first — with no signal to either actor.

- `version` int (default 1) on the seven mutable models: User,
  PatientProfile, DoctorProfile, FacilityProfile, FacilityCapability,
  Appointment, PaymentIntent. Append-only Prescription/LabOrder and the
  immutable tables are excluded by design.
- `backend/src/middleware/optimistic-concurrency.ts`: `setVersionEtag`,
  `requireIfMatch`, `expectedVersion`, `versionConflict`. Strong `"<int>"`
  validators only — a weak validator or a list is refused with 400 rather
  than guessed at. Missing precondition is 428, stale version 412.
- `PATCH /doctors/me` is the reference implementation. The write is a single
  `updateManyAndReturn` on `WHERE id = ? AND version = ?` with the increment
  in the same statement, so the response is the row that statement wrote.
- Transitions that are already conditional (booking's slot claim, payment
  fund/release/refund, doctor verification) keep their own status guard
  rather than gaining a second, unrelated one, but do bump and republish the
  version.
- Convention in ARCHITECTURE.md §5.4 + hard rule 7; columns in §4.1/§4.2.
- Integration proof in tests/integration/optimistic-concurrency.test.mts:
  eight simultaneous writes on one version resolve to exactly one 200 and
  seven 412s, and the stored row is the winner's.

Closes DocForum#49
@drips-wave

drips-wave Bot commented Oct 19, 2026

Copy link
Copy Markdown

@DevRushd Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits.

You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀

Learn more about application limits

@afeezorobsco-cyber

Copy link
Copy Markdown
Contributor

@DevRushd Thanks for the PR. It can't be merged yet. To unblock it:

Merge conflicts: this branch conflicts with main. Please rebase onto the latest main (git fetch upstream && git rebase upstream/main), resolve the conflicts (often ROADMAP.md, ARCHITECTURE*.md or prisma/schema.prisma), and force-push.

Push to this same branch and CI will re-run. Once it's green and conflict-free, it'll be reviewed for merge.

@github-actions github-actions Bot added the needs-rebase Has merge conflicts with main; the author needs to rebase label Oct 20, 2026
@github-actions github-actions Bot removed the needs-rebase Has merge conflicts with main; the author needs to rebase label Oct 21, 2026
@github-actions github-actions Bot added the needs-rebase Has merge conflicts with main; the author needs to rebase label Oct 21, 2026
@github-actions

Copy link
Copy Markdown

@DevRushd This PR now has merge conflicts with main, most likely because #156 (feat(patients): account deletion with clinical-record retention: anonymise identity, keep the audit trail) was just merged, so it can't be merged until it's rebased:

git fetch upstream            # upstream = https://github.com/DocForum/docforum-core.git
git rebase upstream/main
# resolve the conflicts, re-run the tests, then:
git push --force-with-lease

The usual culprits are shared docs like ROADMAP.md and ARCHITECTURE*.md. If the conflict is in the ROADMAP.md changelog, move your entry into a new file under changelog.d/ (see changelog.d/README.md) rather than resolving it in place, so it can't conflict again. CI re-runs when you push, and the needs-rebase label is removed automatically once the conflicts are resolved.

Automated message. It won't repeat unless the PR conflicts again after your next push.

@github-actions github-actions Bot removed the needs-rebase Has merge conflicts with main; the author needs to rebase label Oct 21, 2026
@afeezorobsco-cyber

Copy link
Copy Markdown
Contributor

@DevRushd CI is failing on this branch against the latest main, so it can't be merged yet. The errors from the latest run:

Backend (typecheck + test + build)

src/modules/doctors/controllers/index.ts(18,24): error TS2304: Cannot find name 'updateDoctorProfileSchema'.
src/modules/doctors/controllers/index.ts(19,40): error TS2551: Property 'updateMyDoctorProfile' does not exist on type 'typeof import("/home/runner/work/docforum-core/docforum-core/backend/src/modules/doctors/services/index")'. Did you mean 'getMyDoctorProfile
src/modules/doctors/routes/index.ts(3,10): error TS6133: 'requireIfMatch' is declared but its value is never read.
src/modules/doctors/services/index.ts(3,10): error TS6133: 'versionConflict' is declared but its value is never read.
src/modules/payments/controllers/index.ts(54,30): error TS2339: Property 'version' does not exist on type '{ id: string; prescriptionId: string | null; labOrderId: string | null; status: PaymentIntentStatus; patientProfileId: string; facilityId: string; amount
src/modules/payments/controllers/index.ts(55,40): error TS2741: Property 'version' is missing in type '{ id: string; prescriptionId: string | null; labOrderId: string | null; status: PaymentIntentStatus; patientProfileId: string; facilityId: string; amountStro
tests/integration/optimistic-concurrency.test.mts(300,21): error TS2339: Property 'version' does not exist on type '{ id: string; prescriptionId: string | null; labOrderId: string | null; status: PaymentIntentStatus; patientProfileId: string; facilityId: strin
tests/integration/optimistic-concurrency.test.mts(316,91): error TS2339: Property 'version' does not exist on type '{ id: string; prescriptionId: string | null; labOrderId: string | null; status: PaymentIntentStatus; patientProfileId: string; facilityId: strin

doctors/controllers uses updateDoctorProfileSchema and updateMyDoctorProfile, which aren't defined or exported any more. They were probably lost while resolving the rebase.

@github-actions github-actions Bot added the needs-rebase Has merge conflicts with main; the author needs to rebase label Oct 21, 2026
@github-actions

Copy link
Copy Markdown

@DevRushd This PR now has merge conflicts with main, most likely because #160 (feat(payments): multi-asset intents and patient-signed prepare) was just merged, so it can't be merged until it's rebased:

git fetch upstream            # upstream = https://github.com/DocForum/docforum-core.git
git rebase upstream/main
# resolve the conflicts, re-run the tests, then:
git push --force-with-lease

The usual culprits are shared docs like ROADMAP.md and ARCHITECTURE*.md. If the conflict is in the ROADMAP.md changelog, move your entry into a new file under changelog.d/ (see changelog.d/README.md) rather than resolving it in place, so it can't conflict again. CI re-runs when you push, and the needs-rebase label is removed automatically once the conflicts are resolved.

Automated message. It won't repeat unless the PR conflicts again after your next push.

@github-actions github-actions Bot removed the needs-rebase Has merge conflicts with main; the author needs to rebase label Oct 26, 2026
@afeezorobsco-cyber

Copy link
Copy Markdown
Contributor

@DevRushd CI is failing at npm run typecheck:

src/modules/doctors/controllers/index.ts(18,24): error TS2304: Cannot find name 'updateDoctorProfileSchema'.
src/modules/doctors/controllers/index.ts(19,40): error TS2551: Property 'updateMyDoctorProfile' does not exist on type '...doctors/services/index'.
src/modules/doctors/routes/index.ts(3,10): error TS6133: 'requireIfMatch' is declared but its value is never read.
src/modules/doctors/services/index.ts(3,10): error TS6133: 'versionConflict' is declared but its value is never read.
src/modules/payments/controllers/index.ts(66,30): error TS2339: Property 'version' does not exist on type '{ id: string; prescriptionId: ...; status: PaymentIntentStatus; ... }'.
tests/integration/optimistic-concurrency.test.mts(68,9): error TS2353: ... 'orderType' does not exist in type '...PaymentIntentUncheckedCreateInput'
tests/integration/optimistic-concurrency.test.mts(147,46): error TS2353: ... 'facilityId' does not exist in type '...WalletLinkCreateInput...'

The version column(s) and the doctor-profile update path aren't on the branch anymore, and the test fixtures use fields that main's schema doesn't have. Please reconcile with current main, run npm run typecheck in backend/, and push.

@github-actions github-actions Bot added the needs-rebase Has merge conflicts with main; the author needs to rebase label Oct 27, 2026
@github-actions

Copy link
Copy Markdown

@DevRushd This PR now has merge conflicts with main, most likely because #164 (Implement cancellation policy (issue #56), waitlist with time-boxed offers (issue #55), and reschedule (issue #54)) was just merged, so it can't be merged until it's rebased:

git fetch upstream            # upstream = https://github.com/DocForum/docforum-core.git
git rebase upstream/main
# resolve the conflicts, re-run the tests, then:
git push --force-with-lease

The usual culprits are shared docs like ROADMAP.md and ARCHITECTURE*.md. If the conflict is in the ROADMAP.md changelog, move your entry into a new file under changelog.d/ (see changelog.d/README.md) rather than resolving it in place, so it can't conflict again. CI re-runs when you push, and the needs-rebase label is removed automatically once the conflicts are resolved.

Automated message. It won't repeat unless the PR conflicts again after your next push.

@github-actions github-actions Bot removed the needs-rebase Has merge conflicts with main; the author needs to rebase label Oct 27, 2026
@afeezorobsco-cyber

Copy link
Copy Markdown
Contributor

@DevRushd After the latest merge from main, CI still fails at npm run typecheck with the same errors as in my earlier comment. The merge didn't bring back the missing pieces. The doctor-profile update path (updateDoctorProfileSchema, updateMyDoctorProfile) and the version column on PaymentIntent are still missing, and tests/integration/optimistic-concurrency.test.mts still uses orderType/facilityId fields that main's schema doesn't have. Please fix those, run npm run typecheck in backend/, and push.

@DevRushd

Copy link
Copy Markdown
Contributor Author

Implementation consolidated into PR #162, which now covers both issue #48 and issue #49. Optimistic concurrency tests, migration, and generated Prisma client are included there; PR #162 references Closes #49.

@DevRushd DevRushd closed this Oct 28, 2026
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.

[Phase 8] Optimistic concurrency (version + If-Match) for mutable records

2 participants