docs: add SessionRecord store plan and persistence ADR - #584
Conversation
Database side of the MainEHR launch sequence (issue informedica#516). ADR-0006 records the linked decisions: GenPRES gains a relational database it owns, one engine for dev and prod (default Postgres), Dapper with hand-written SQL over an ORM, an append-only event log plus a current-state projection, and fixed-order row locking to decide the launch races. Status is Proposed pending the production engine choice and a V8 design amendment. The implementation plan covers the private-store schema, the SessionStore port, the Rule 14 anonymous cap, the Rule 41 sweep, the Rule 46 audit writer, and the migration and test infrastructure, in 14 PR/commit sized steps. Refs informedica#516
Greptile SummaryThis documentation PR proposes GenPRES-owned relational persistence for sessions and future clinical data, using an append-only event log, a current-state projection, Dapper/DbUp, and durable lock rows for launch concurrency.
Confidence Score: 4/5The plan should not be approved unchanged because concurrent anonymous and authenticated opens for one browser can bypass the intended lock serialization. The previous first-open, anonymous-cap, and configuration findings are addressed, and the clarified expiry wording supports the author’s explanation. However, the new anonymous global lock is used instead of the browser lock even though anonymous sessions may carry a browser identity and remain subject to Rule 8’s browser limit, leaving that mixed-session race unresolved. Files Needing Attention: docs/implementation-plans/516-sessionrecord-store.md Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart LR
Launch[Session open request] --> Kind{Anonymous?}
Kind -->|No| UserLock[Lock user key]
UserLock --> BrowserLock[Lock browser key]
Kind -->|Yes| GlobalLock[Lock anonymous global key]
GlobalLock --> Cap[Check anonymous cap]
BrowserLock --> Replace[Close conflicting sessions]
Cap --> Replace
Replace --> Insert[Insert current session]
Insert --> Event[Append session event]
Event --> Audit[Write audit entry]
Reviews (2): Last reviewed commit: "docs(server): clarify key-row locking de..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| 3. Take a row lock in a fixed order. Lock the user's open row, then the browser's open row, both | ||
| `SELECT ... FOR UPDATE` on Postgres or `WITH (UPDLOCK, HOLDLOCK)` on SQL Server, then do the close | ||
| and the insert. A fixed order, user before browser, always, makes a deadlock impossible. The | ||
| partial unique indexes stay, but as a backstop invariant, not as the thing that decides the race. |
There was a problem hiding this comment.
When two first launches run concurrently, neither finds an existing user or browser row to lock, so both can proceed to insert. The partial unique index then becomes the actual race arbiter, despite the plan treating it only as a backstop because its behavior differs between database engines. This can make one launch fail instead of allowing both operations to complete with the later one superseding the earlier session. Define a stable lock target for absent rows, or fully specify and test the unique-conflict retry path on every supported engine.
There was a problem hiding this comment.
Agreed, the fixed-order lock had nothing to lock on a first open, which left the partial unique index as the actual race decider in exactly the case Decision 2 calls engine-dependent. Fixed: the lock target is now a durable row in a new session_key_lock table, one row per (scope, lock_key) for ('user', user_id), ('browser', browser_id), ('anon', 'global'). The open ensures the rows exist then SELECT ... FOR UPDATEs them user-before-browser, so two first launches for the same user serialize even with no session row yet. Plain row locks, so a transaction-mode pooler does not break them either.
Changed: plan "concurrency mechanism" approach 3 (line 95), "chosen approach" locking paragraph (line 126), schema (new session_key_lock table, line 217), first race walkthrough (line 254); ADR Decision 5 (renamed, line 82).
| 8. SQL adapter, the open. `openSessionClosingOthers` as the one transaction: the nonce spend first, | ||
| then the ordered locks, the ordered closes, the insert, the audit row, the Rule 14 count. A nonce | ||
| that is already spent short-circuits to the replay answer. Concurrency tests: many threads racing | ||
| an open for the same user, the same browser, and the same nonce, asserting the ext 8b outcome and | ||
| a single spent nonce. |
There was a problem hiding this comment.
Rule 14 uses a global count of open anonymous sessions, but this transaction defines no lock or other serialization point for that count. Concurrent anonymous opens have no non-null user row to lock, may have no browser row, and are excluded from both partial unique indexes. Several transactions can therefore observe available capacity and insert enough sessions to exceed the cap. Add atomic enforcement for the global cap and cover racing anonymous opens in the concurrency tests.
There was a problem hiding this comment.
Correct. Anonymous sessions are in neither partial index and count(*) under READ COMMITTED does not serialize. Fixed: anonymous opens take the single ('anon', 'global') row in session_key_lock before counting and inserting, so every anonymous open sees a count that already includes every one committing before it. The cap cannot be overshot. Added a dedicated race test for many anonymous opens against the cap.
Changed: plan scope line (line 33), new anonymous-cap paragraph in the race walkthrough (line 264), step 8 concurrency tests (line 365); ADR Decision 5.
| Config: `GENPRES_DB_CONNECTION`, or the discrete `GENPRES_DB_HOST` / `_PORT` / `_NAME` / `_USER` / | ||
| `_PASSWORD`. In production (`GENPRES_PROD=1`) the server refuses to start without it, the same | ||
| fail-closed rule as `GENPRES_PASSWORD`. Demo and a bare `dotnet run` fall back to the in-memory port. | ||
| Demo then loses its sessions on restart, which is a divergence from Rule 32. Document it, or run a | ||
| throwaway Postgres container in the demo compose file. | ||
|
|
||
| Wiring: `SessionStore` goes on `AppEnv`, and the composition root picks the SQL or in-memory | ||
| implementation from whether `GENPRES_DB_CONNECTION` is set. This meets the `launchSession` and |
There was a problem hiding this comment.
The plan presents the discrete GENPRES_DB_HOST, PORT, NAME, USER, and PASSWORD settings as an alternative to GENPRES_DB_CONNECTION, but the composition root selects SQL storage only when GENPRES_DB_CONNECTION is set. An operator following the discrete configuration path would therefore get volatile in-memory storage outside production, while production would reject the configuration. Either remove the unsupported alternative or specify connection construction and make adapter selection and validation accept both forms.
There was a problem hiding this comment.
Fixed by dropping the discrete-vars path. GENPRES_DB_CONNECTION is now the single switch: set wires the SQL adapter, unset wires in-memory, and the production guard checks the same one thing. If a deployment wants GENPRES_DB_HOST / _PORT / ... later, the composition root builds the string from them and the one switch still applies.
Changed: plan config paragraph (line 149), steps 6 and 14; ADR Decision 6 (line 96) and Consequences (line 106).
| browser_id text null, -- see open decision 3 | ||
| launch_nonce text null, | ||
| opened_at bigint not null, | ||
| expires_at bigint null, -- absolute lifetime (Rule 30) |
There was a problem hiding this comment.
The schema labels expires_at as Rule 30's absolute lifetime, but Rule 30 defines the idle TTL. Launched-session absolute lifetime belongs to Rule 10, while anonymous absolute lifetime belongs to Rule 14. This misleading attribution could cause implementation work to conflate idle and absolute expiry. Correct the rule reference and state which policy value populates this column for each session type.
There was a problem hiding this comment.
Rule 30 is the configuration rule that sizes both the idle gap and the absolute cap ("span the gaps between a clinician's actions, and no longer than a shift whatever happens in it"). Rule 10 enumerates the endings and cites Rule 30 for absolute lifetime; Rule 9 is the idle-clock refresh. The model already keeps the two apart as separate end marks (Idle vs Expired) and the two columns mirror that, so there is no conflation. I have expanded the schema comments to name which SessionPolicy value populates each column and which rules govern it.
Changed: plan schema comments on expires_at and last_seen (line 198). No ADR change (it does not reference these columns). This one is a wording clarification, not a design fix.
Update ADR-0006 and the informedica#516 implementation plan to define race handling with fixed-order locks on durable `session_key_lock` rows instead of locking open-session rows. The docs now explicitly cover first-open serialization, anonymous-cap enforcement under `('anon','global')`, and why this works with transaction-mode poolers. They also simplify config guidance to a single `GENPRES_DB_CONNECTION` switch (required in prod, in-memory fallback otherwise) and align schema, rollout, and concurrency test expectations with that model.
| `('anon', 'global')` instead, then counts the open anonymous sessions and refuses above the cap | ||
| (Rule 14) inside the same transaction. `READ COMMITTED` with a bounded retry on a serialization or | ||
| unique-violation error. The nonce spend is idempotent (`INSERT ... ON CONFLICT (kind, nonce) |
There was a problem hiding this comment.
An anonymous session can carry a browser_id, and Rule 8 still applies the browser limit to anonymous sessions. However, this design makes an anonymous open lock ('anon', 'global') instead of the browser key. An anonymous open and an authenticated open for the same browser can therefore run concurrently under different locks, leaving the partial unique index to decide the race and potentially failing one launch instead of performing the required ordered replacement. Anonymous opens that have a browser must also use the browser-key locking protocol.
Address findings and the plan/ADR gaps raised in review.
- concurrency: lock a durable session_key_lock row, not the open session, so a first open with no row yet still serializes; anonymous opens serialize on a single ('anon','global') row for the Rule 14 cap
- config: GENPRES_DB_CONNECTION is the single switch; drop the unsupported discrete-vars path
- schema: name which SessionPolicy value fills each expiry column; widen audit_entry so the reader can query it
- correct step 1's claim that the ADR resolves the open decisions
- record the expand/contract migration constraint and the browser_id
limitation in the ADR; add audit retention and multi-server clock
skew as open decisions
|
@halcwb Question that need to be addressed to refine this further:
|
1 participant
Database side of the MainEHR launch sequence (issue #516).
ADR-0006 records the linked decisions: GenPRES gains a relational database it owns, one engine for dev and prod (default Postgres), Dapper with hand-written SQL over an ORM, an append-only event log plus a current-state projection, and fixed-order row locking to decide the launch races. Status is Proposed pending the production engine choice and a V8 design amendment.
The implementation plan covers the private-store schema, the SessionStore port, the Rule 14 anonymous cap, the Rule 41 sweep, the Rule 46 audit writer, and the migration and test infrastructure, in 14 PR/commit sized steps.
Refs #516
Claude was used in the formulation of the plan with an additional adversarial review to refine it further.
Firstly, I'm not a db guy at all, this is more of an initial stab of the plan rather than being written in stone. One of the big things is getting the sign-off on which database we will use and make sure we use the same for both dev/test/prod.
Things to be mindful of are using type providers for db access can get painful, I've seen this layer get quite tangled with type providers being really clunky and slow, this may have changed between the last time I used them though.
Technologies for the db layer are suggestions, if anyone has a better suggestion then lets detail this here and refine the design/plan.