fix(session): seal first insert on missing or digest-mismatched catalog - #219
seonghobae wants to merge 47 commits into
Conversation
Store participant and published-release identity for SessionState::Created with exact replay and fail-closed rebinding. Command-replay persistence stays outside this first slice.
Assert the Database error message and source, and fail the replay SELECT after ON CONFLICT by redirecting search_path so classify runs instead of the insert.
Linux llvm-cov leaves the isolated query_one ? tail uncovered unless the Err arm is an explicit match. Keep the search_path redirect test.
Linux branch coverage missed the later AND operands of exact-replay classification. Rebind each stored field independently, and prove a domain-legal u64::MAX creation time fails closed as ValueOutOfRange.
The replay SELECT failure constructed Database evidence without checking its safe display text or source, leaving those two production lines uncovered on Linux.
SHOW transaction_isolation can fail after the caller transaction is already aborted. Persist must surface that as a typed database error instead of leaving the probe Result uncovered.
Satisfy clippy::manual_let_else in the library test that instantiates AssessmentSessionPersistenceError::Database.
Name instrument_version_ref in the public persist contract and assert the committed version column. Keep TRACEABILITY, changelog, and as-built schema at Active PR #61 rather than promoting the slice to protected-main truth. Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
Restore persisted created-session identity from PostgreSQL without asking whether the original release still accepts new sessions, so later suspend or retire cannot rewrite provenance. Missing rows return none; later stored states and malformed lookup references fail closed. Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
Name the opened successor so TRACEABILITY, as-built schema, and ERD point at the persist-and-load head instead of the persist-only #106 slice. Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
Add the headline reconstitution case: create while published, then suspend or retire so AssessmentSession::new fails, then restore the original Created identity and Activate. Cover numeric-like participant, release, and version references. Point AS_BUILT_SCHEMA at Active PR #109 instead of predecessor #61. Fail closed on every later stored state and on load against a missing table. Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
Store accepted session commands in assessment_session_command and project the current lifecycle state. Load reconstitutes created identity without re-checking publication eligibility, then replays commands so Pause/Resume still work after process restart. Exact command replay is idempotent; sequence reuse and evidence rebinding fail closed. Later stored states without command history still fail closed. Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
Point TRACEABILITY, as-built schema, and ERD at the successor that stores assessment_session_command and replays Activate after restart. Keep #109 named as the persist-and-load predecessor. Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
A worker that only remembers Activate must not rewind a later Pause/Resume projection. Count stored commands after exact replay and fail closed when the in-memory history is shorter, so load still reconstitutes the paused session after the rejected persist. Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
Point traceability, as-built schema, and ERD at the successor that rejects a shorter command history instead of rewinding Pause/Resume. Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
Rejecting a shorter command history is not enough under READ COMMITTED. Lock the created-session row with SELECT … FOR UPDATE before inserting or counting commands so a concurrent Activate-only persist cannot count a prefix and then rewind a later Pause/Resume projection. Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
Point TRACEABILITY, as-built schema, and ERD at the successor that locks assessment_session before command insert or count. Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
New sessions must call AssessmentSession::new through created_session_for_start / start_created_assessment_session so a draft, suspended, or retired release cannot insert a row. Keep the #146 header-row lock and command-history persist. Reconstitution remains load, not start. Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
Name the start-boundary landing vehicle so TRACEABILITY, ERD, UML, and as-built schema point at this head instead of the #146 lock predecessor. Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
In-memory AssessmentSession::new is not enough: a stale Published object could insert after another transaction persisted Suspend or Retire. Lock instrument_release with SELECT FOR UPDATE in the same start transaction, add start_created_assessment_session_from_stored_release, and fail closed on missing, unpublished, locale-mismatched, or digest-mismatched stored evidence. Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
Point TRACEABILITY, as-built schema, ERD, and UML at the successor that locks instrument_release before a new session insert. Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
Keep the #180 stored-publication FOR UPDATE lock, then return the original created session when a buyer retries the exact start after persist Suspend or Retire. A new session_ref or rebound participant still fails closed. Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
Seal persist_assessment_session so a from_persisted_created first insert cannot mint a session after stored Suspend or Retire. Exact persist of an already stored created identity still replays. Prove Retire start replay and locale/created_at identity mismatch. Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
Reject reconstituted persist when the stored instrument_release row is missing or digest/version/locale-mismatched, while keeping exact replay of an already stored Created row after later Suspend or Retire. Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: do not merge. This head closes the #209 NotFound hole and binds digest/version/locale on first insert, but persist still peeks assessment_session without a lock and returns a publication error without classifying an exact stored Created row.
Under READ COMMITTED, a concurrent exact retry can miss the in-flight first insert, wait on instrument_release, then fail after that insert commits and ops suspends the catalog. QA-REL-05 claims that retry stays legal.
Prefer #218 (deb38ae) over this head, #209, and #205. #218 always locks the catalog row and classifies an exact stored Created row when the first-insert seal fails. Do not merge #219, #209, #205, #198, #180, #188, #164, #153, or #154 in parallel. Do not open a fourth stored-start PR. Do not add HTTP on this slice.
This review is not independent last-push approval.
Sent by Cursor Automation: Fix Issues
| &[&session_ref], | ||
| )?; | ||
| if existing.is_none() { | ||
| require_published_release_for_first_insert(transaction, session)?; | ||
| } | ||
| let session_state = session.state().persist_name(); | ||
| let inserted = transaction.execute( |
There was a problem hiding this comment.
Same unlocked peek as #205. After existing.is_none(), require_published_release_for_first_insert can fail while the exact session_ref is already committed by the transaction that just released the catalog lock. Persist must classify that stored Created row as Duplicate before failing closed. That path is on #218; keep this head unmerged.
There was a problem hiding this comment.
Verdict: do not merge. HEAD c7b5eda still peeks assessment_session without a lock, then calls require_published_release_for_first_insert only when that peek misses. Under READ COMMITTED, a concurrent exact retry can miss the row, take the catalog lock after the first insert commits and after a later Suspend/Retire, and return a publication error for a session the buyer already has.
#218 (deb38ae) always locks instrument_release and classifies an exact stored Created row as Duplicate before fail-closed. Prefer that head for stored-publication start plus first-insert seal plus seal-fail persist replay. Keep #219, #209, #205, #198, and #180 Draft. Do not add HTTP onto this branch.
Sent by Cursor Automation: Fix Issues
| &[&session_ref], | ||
| )?; | ||
| if existing.is_none() { | ||
| require_published_release_for_first_insert(transaction, session)?; |
There was a problem hiding this comment.
Unlocked peek is unchanged. After existing.is_none(), this seal can fail while the exact session_ref is already committed. Persist must SELECT … FOR UPDATE that Created row and return Duplicate before InstrumentReleaseUnavailable / InvalidStartRelease. That path is on #218; keep this head unmerged.
There was a problem hiding this comment.
Review — first-insert catalog seal
Do not merge this head for persist landing. Prefer #218 deb38ae.
The missing/digest/Retire first-insert seal is real, and exact persist of an already-visible Created row still returns Duplicate when the unlocked peek sees the row. The remaining hole is the peek itself:
SELECT 1 FROM assessment_session WHERE session_ref = $1 -- no lock
if none: require_published_release_for_first_insert
then INSERT … ON CONFLICT DO NOTHING
Under READ COMMITTED, a concurrent exact retry can peek no row while the first insert still holds the release lock, then take that lock after the insert commits and after ops Suspends or Retires the catalog, and fail closed even though the buyer already has the session. start_* already replays that miss. This persist path does not. QA-REL-05 claims the retry stays legal.
#218 removes the peek, always locks instrument_release, and classifies an exact stored Created row when that lock finds a missing or unpublished release.
Keep Draft. Do not add HTTP. Do not open a third persist-seal PR. Qualifying approval stays with an independent reviewer.
Sent by Cursor Automation: Fix Issues
| let session_ref = session.session_ref(); | ||
| let participant_ref = session.participant_ref(); | ||
| let created_at_unix_ms = postgres_bigint(session.created_at_unix_ms())?; | ||
| let existing = transaction.query_opt( |
There was a problem hiding this comment.
Unlocked peek is the hole #218 closes. Under READ COMMITTED, a concurrent exact retry can see no row here, wait on the later instrument_release lock, then observe Suspend/Retire after the first insert committed, and return UnpublishedStart without classifying the stored Created row.
Prefer #218 (deb38ae): always lock the release, and on seal-fail classify an exact stored Created row before returning the seal error. Keep this PR Draft. Do not merge in parallel with #218.
|
Closing this Draft landing vehicle without merge. Current review has three unresolved exact-head findings on the same unlocked |
2 participants
Why
PR #209 rejects a reconstituted first insert after stored Suspend, but
persist_assessment_sessionstill treated a missing catalog row as success and did not compare digest/version/locale on a still-published row. TRACEABILITY already claimed those first inserts fail closed.A reconstitutor must not mint a session after catalog delete or with a stale digest. A purchaser who already started must still retry the exact stored row.
What this PR does
FOR UPDATEstart lock, exact start replay after later persist Suspend or Retire, and Suspend persist reject.instrument_releaseon the firstpersist_assessment_sessioninsert.Duplicateafter that later persist.Out of scope
POST /v1/sessions/ OpenAPI / RFC 9457 (feat(api): create and reload in-process sessions over HTTP #149, feat(session): map POST /v1/sessions and lock command persist #161)Test plan
cargo test --lib postgres_assessment_sessioncargo test --test session_start --test documentation_architecture_contract --test traceability_active_pr_contractcargo clippy --all-targets -- -D warningscargo test --test postgres_assessment_session_persistence persist_rejects_first_insert_when_stored_release_is_missing persist_rejects_first_insert_when_digest_mismatches_published_row persist_rejects_reconstituted_first_insert_after_stored_release_is_suspended(CI PostgreSQL)This is the successor to #209 for stored-publication start, exact start replay, and persist first-insert seal after later persist Suspend or Retire, catalog delete, or digest mismatch. Prefer this head over #209, #205, #198, #180, #188, #164, #153, and #154 for the start-from-store path. Do not merge until exact-head checks and independent last-push approval are satisfied.