Repository navigation
Conversation
…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
|
@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! 🚀 |
|
@DevRushd Thanks for the PR. It can't be merged yet. To unblock it: Merge conflicts: this branch conflicts with Push to this same branch and CI will re-run. Once it's green and conflict-free, it'll be reviewed for merge. |
|
@DevRushd This PR now has merge conflicts with 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-leaseThe usual culprits are shared docs like Automated message. It won't repeat unless the PR conflicts again after your next push. |
|
@DevRushd CI is failing on this branch against the latest Backend (typecheck + test + build)
|
|
@DevRushd This PR now has merge conflicts with 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-leaseThe usual culprits are shared docs like Automated message. It won't repeat unless the PR conflicts again after your next push. |
|
@DevRushd CI is failing at The |
|
@DevRushd This PR now has merge conflicts with 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-leaseThe usual culprits are shared docs like Automated message. It won't repeat unless the PR conflicts again after your next push. |
|
@DevRushd After the latest merge from |
What Changed
Schema —
versionon the mutable modelsbackend/prisma/schema.prisma—version Int @default(1)onUser,PatientProfile,DoctorProfile,FacilityProfile,FacilityCapability,Appointment,PaymentIntent.backend/prisma/migrations/20261019090000_add_optimistic_concurrency/migration.sql—ADD COLUMN "version" INTEGER NOT NULL DEFAULT 1on 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)— strongETag: "<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 onreq.expectedVersion, matching the existingreq.user/req.requestIdpattern.expectedVersion(req)— throws rather than defaulting, so a missing precondition can never silently skip the guard.versionConflict()— the single412 VERSION_CONFLICTevery stale write returns.Reference implementation —
PATCH /doctors/memodules/doctors/routes— newPATCH /me, orderedauthenticate → requireRole('doctor') → requireIfMatch → updateMeso an unauthorised caller gets 401/403 instead of a hint that the route exists.modules/doctors/repositories—updateDoctorProfile(id, version, fullName)usingupdateManyAndReturnonWHERE id = ? AND version = ?withversion: { increment: 1 }in the same statement.updateManyAndReturn(notupdateMany+ re-read) so the response body is the row this statement wrote; a follow-upfindUniquewould 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 carryingverificationStatusis rejected rather than ignored).modules/doctors/controllers—updateMe;GET /doctors/menow publishes theETag.Already-conditional transitions bump the version without gaining a second guard
modules/payments/repositories—markEscrowed/markStatusincrementversioninside their existing status-conditional writes.modules/doctors/repositories—updateVerificationStatusincrements too.modules/auth/repositories—markUserEmailVerifiedincrementsUser.version.modules/payments/controllers— every intent response (create, read, fund, release, refund) publishes theETagand the body'sversion.Docs
ARCHITECTURE.md§5.4 — the convention as a numbered rule new modules must follow; §4.1/§4.2 — theversioncolumns 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:
fund/release/refundand doctor verification each already fail on an observed-state mismatch (409). AddingIf-Matchon top gives one write two unrelated failure modes — a client retrying after a409would hit a412it can't act on, and a staleETagwould mask a genuine status conflict. They keep their own guard and only publish the new version.Prescription/LabOrderare excluded. A correction is a newsupersedes_*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):412. Read, write, then replay the originalETag: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.PATCHes carrying the sameETag→ exactly one200, seven412s; the stored row'sfullNameis the winner's and itsversionisbefore + 1. This is the lost-update bug, fired at once.428for clients that don't sendIf-Match— withcode: 'IF_MATCH_REQUIRED'and a message that says what to do, plus an assertion that nothing was written.400for 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.401, wrong role →403(both before the precondition check); invalid or over-reaching body →400with the version unchanged.POST /appointmentsstill books with noIf-Match(201); doctor verification still flips with noIf-Matchand bumps the version;fund→releasestill work with noIf-Matchand walk the version 1 → 2 → 3, while a secondreleasestill loses on status (409), not on version.versioncolumn viainformation_schema, so the exclusion can't quietly widen.User.version1 → 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