fix(session): seal persist first-insert against unpublished reconstitution - #205
cursor[bot] wants to merge 45 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>
…ution First insert now locks stored publication state. Exact replay of an already stored Created row still succeeds after later suspend. Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
![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. The first-insert seal is the right product cut, and it is stricter than #209, but persist still peeks assessment_session without a lock and returns UnpublishedStart without classifying an exact stored row.
start_created_assessment_session / start_created_assessment_session_from_stored_release already replay an exact stored start after persist Suspend or Retire. persist_assessment_session does not. Under READ COMMITTED, a concurrent exact retry can peek no row while the first insert still holds instrument_release, then take the lock after that insert commits and after ops suspends, and fail closed even though the buyer already has the session. QA-REL-05 claims that retry stays legal.
Prefer this slice over #198, #180, #188, #164, #153, #154, #146, #138, and #209. Do not merge those in parallel. #209 is not a substitute: its first-insert helper treats NotFound as insertable and does not bind digest/version/locale on a published row.
Stay draft until persist classifies an exact stored Created row when the first-insert seal fails. Do not add HTTP POST /v1/sessions on this persist-seal slice. Independent last-push approval is still required; this review is not that approval.
Sent by Cursor Automation: Fix Issues
| let existing = transaction.query_opt( | ||
| "SELECT 1 FROM assessment_session WHERE session_ref = $1", | ||
| &[&session_ref], | ||
| )?; | ||
| if existing.is_none() { | ||
| require_published_release_for_first_insert(transaction, session)?; | ||
| } |
There was a problem hiding this comment.
This unlocked SELECT 1 is the buyer-visible hole. If another transaction has inserted the same session_ref but not committed, the peek sees no row. This transaction then waits on load_published_instrument_release (FOR UPDATE). After the first insert commits, a concurrent Suspend/Retire can take the release lock next. This caller then returns UnpublishedStart and never reaches ON CONFLICT / classify_existing_session.
start_* already calls replay_started_session_after_publication_block on that miss. Persist must do the same: when the first-insert seal returns UnpublishedStart or InvalidStartRelease, lock the session row and classify an exact Created identity as Duplicate before failing closed. Otherwise a purchaser who already started cannot retry the exact persist after ops suspends the catalog.
Name the first-insert seal successor after the pull request number is assigned. Keep #205 as the unlocked-peek predecessor. Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
There was a problem hiding this comment.
Verdict: do not merge this head. The first-insert seal is the right product cut and is stricter than #209, but persist still peeks assessment_session without a lock and returns UnpublishedStart without classifying an exact stored row.
Landing vehicle is now #218 (deb38ae): persist always locks instrument_release, and when that seal fails it classifies an exact stored Created row as Duplicate before failing closed. Prefer #218 over #205, #209, #198, #180, #188, #164, #153, #154, #146, and #138. Do not merge those in parallel. Do not add HTTP POST /v1/sessions on the persist-seal slice.
Independent last-push approval is still required; this review is not that approval.
Sent by Cursor Automation: Fix Issues
| let existing = transaction.query_opt( | ||
| "SELECT 1 FROM assessment_session WHERE session_ref = $1", | ||
| &[&session_ref], | ||
| )?; | ||
| if existing.is_none() { | ||
| require_published_release_for_first_insert(transaction, session)?; | ||
| } |
There was a problem hiding this comment.
This unlocked SELECT 1 is the buyer-visible hole. A concurrent exact retry can peek no row, wait on the release lock, then return UnpublishedStart after the first insert commits and ops suspends the catalog.
Fixed on #218: persist always takes the publication lock, then classifies an exact stored Created row when the seal returns UnpublishedStart or InvalidStartRelease. Keep this head unmerged.
2 participants
Superseded by repaired persist line
Close this predecessor without merge. Its current review identifies a buyer-visible retry defect that is repaired on the #218 line and carried forward verbatim by #232.
Fresh evidence immediately before closure:
bd9ea70b252fe83f1ab0584236b97c7f95127aaadeb38ae2718b10d6eb636f17df1965b8382f2ca0(closed predecessor)c7b45145c9b693951fae498e0779d58cbe170f86main:a7637351be8f0f90c12651d3bcafd959bc52ac81assessment_sessionpeek: an exact retry can observe no row, wait for the publication lock, then fail asUnpublishedStartafter the original start commits and the release is suspended/retired.Duplicatebefore failing closed.deb38ae…is its merge base), and feat(api): start and reload sessions over persist-backed HTTP #232 explicitly keeps the fix(session): replay exact persist after first-insert seal miss #218 persist/load/command-history/start-lock, first-insert seal, and seal-fail exact persist replay while adding persist-backed session HTTP.Do not mark #205's review threads resolved: the defect remains on this exact head and is the reason this predecessor is closed. #232 remains Draft and must satisfy its own exact-head CI/security/review gates before merge.