fix(result): fail closed on cyclic session supersession - #157
fix(result): fail closed on cyclic session supersession#157cursor[bot] wants to merge 19 commits into
Conversation
A published result must survive process restart without calling the scoring engine again. Reconstruct stored provenance and copied observations so operators can serve the same snapshot, exact persist replay stays Duplicate, and gapped or noncanonical evidence fails closed. Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
Reject gapped observation_order on reload and return the unique non-superseded published tip for the session a participant is viewing. Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
A persist-created A↔B supersession cycle made the current-tip query return no rows. Treating that as absent looked like "no score yet" and would let a restarted worker invent a result. Fail closed when the session has snapshots but no unique tip. 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.
Review of 106a89b (cyclic session supersession)
The latest fix is the right fail-closed rule for this slice. After restart, call load_current_result_snapshot_for_session with the session the participant is viewing. None now means the session has no stored result rows. A supersession cycle (or any graph that leaves zero tips while rows exist) returns InconsistentEvidence, so a worker must not treat that as "score now".
The unit matrix (0,false) → Absent, (0,true) → fail closed, (1,true) → load, plus the PostgreSQL A↔B persist-then-load test, matches the buyer-visible bug from #137.
Do not merge this head yet. It is Draft, last-push CI is still queued, and an independent reviewer who did not author 106a89b still has to review the exact head. Prefer #157 over #137/aef54c9 and #119. Do not open another overlapping result-reload PR.
Leave on a later slice (not this PR):
- Persist still accepts an A↔B pair (
persist_okin the new test). Load now fails closed; persist-time cycle rejection and a physical unique-current-tip constraint are not in this head. - No product HTTP/worker caller. Result-serving transport stays Target.
- Do not fold response-snapshot reload (#151), item-delivery reload (#110), or session HTTP (#149) into this branch.
Independent reviewer: review 106a89b after Runtime CI, coverage, and required review workflows finish on this exact SHA. If those stay green and the persist-time residual is accepted as follow-up, this is the result-reload landing vehicle.
Sent by Cursor Automation: Fix Issues
Protected-main #81 requires claim_deadline_at on processing integration_consumption rows. Recovery fixtures that omit the column fail constraint integration_consumption_claim_deadline_shape.
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
The isolated EXISTS ? after an empty tip list was unexecuted on Linux coverage. Tip and snapshot counts now come from one query, and a blank stored construct still fails closed through observation reconstruction.
Append-only observation triggers blocked the reconstruct fixture. DISABLE TRIGGER ALL is fixture-only, matching other corrupt-row loads.
Immutability triggers and the construct-ref check both blocked the blank-construct row. Fixture-only DROP CONSTRAINT plus DISABLE TRIGGER ALL lets load reconstruct fail closed.
COUNT(*) cannot be negative at runtime, so the isolated try_from map_err arm stayed unexecuted. stored_nonnegative_count now owns that fail-closed conversion and the lib test takes the negative arm.
Linux llvm-cov left load_current_result_snapshot_for_session as an unexecuted instantiation because only integration tests called it. The lib test takes the missing-relation Database arm on that copy.
Linux llvm-cov left the isolated query_one ? on the unique-session tip path uncovered. Classify that lookup through a match and instantiate the Database arm from the library.
Linux llvm-cov still left one production line after the isolated query_one ? was moved. Classify the count query through a match and load a stored unique tip from a library test so the Ok arms execute in the same instantiation as the Database miss.
|
Hourly loop: Linux line coverage on |
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
Blocking exact-head review: the new read-only reload paths inherit the persistence-only READ COMMITTED restriction. persist_result_snapshot needs READ COMMITTED because its ON CONFLICT DO NOTHING replay classifier relies on a fresh command snapshot after a concurrent unique-key winner; load_result_snapshot and load_current_result_snapshot_for_session do not. Rejecting REPEATABLE READ / SERIALIZABLE for pure reads unnecessarily breaks callers using stronger isolation and is inconsistent with the analogous audit reload contract. Remove the write-specific isolation precondition from both load paths (or narrowly prove a read-specific necessity), and replace the rejection test with success/absence/corruption coverage at stronger isolation. Keep persistence itself fail-closed at READ COMMITTED. The cycle/two-tip fail-closed semantics otherwise look sound on this head.
Persist still requires READ COMMITTED for ON CONFLICT replay classification. load_result_snapshot and load_current_result_snapshot_for_session are read-only and now succeed under REPEATABLE READ and SERIALIZABLE, matching the audit reload contract. Absence, unique-tip success, and cyclic-tip corruption stay fail-closed at those levels.
|
Hourly loop: addressed CHANGES_REQUESTED on load-path isolation. Persist still fail-closed at READ COMMITTED. |
|
Hourly loop: exact head remains |
Keep session-result reload as Active PR #157. Persist stays READ COMMITTED; read-only reload still succeeds under stronger isolation.
|
Hourly loop: merged protected main after #244 ( |
|
@opencode-agent Please re-review exact unchanged head |
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
OpenCode cannot approve yet because required coverage evidence did not pass.
Review outcome
1. HIGH .github/workflows/opencode-review.yml:1 - Coverage evidence did not prove required test/docstring evidence
-
Problem: The required coverage-evidence job result was
failure, so OpenCode cannot establish approval sufficiency for this head. -
Root cause: Automated approval is only valid when the same-head coverage-evidence job proves supported repository test suites passed and configured docstring gates passed or were advisory, or reports not applicable because no supported source files or package manifests exist. Missing, failed, skipped, unavailable, or unsupported-tooling test evidence is a blocker.
-
Fix: Install or configure the repository test/docstring evidence tooling when source files or package manifests exist, rerun the current-head coverage-evidence job, and approve only after it reports
successwith required evidence or explicit no-source not-applicable evidence. -
Regression test: Keep the approval branch checking
needs.coverage-evidence.result == successbefore posting APPROVE, and publish REQUEST_CHANGES when coverage-evidence blocker states such as cancelled, skipped, failed, unsupported-tooling, or below-100 evidence are present. -
Result: REQUEST_CHANGES
-
Reason: coverage-evidence result was
failure, so required test/docstring evidence was not proven for current head821b6a48ddd79a26a4ae885535dc04825b99d436. -
Head SHA:
821b6a48ddd79a26a4ae885535dc04825b99d436 -
Workflow run: 32045151574
-
Workflow attempt: 1
Coverage evidence
Coverage Decision
- Result: FAIL
- Test evidence: not proven passing
- Docstring evidence: not proven passing when configured
- Failure count: 1
Changed-File Evidence Map
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Changed file (4 files)"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Changed file (4 files)"]
R1 --> V1["required checks"]
Evidence --> S2["Docs (2 files)"]
S2 --> I2["operator or user guidance"]
I2 --> R2["Review risk: Docs (2 files)"]
R2 --> V2["docs review"]
Evidence --> S3["Test (3 files)"]
S3 --> I3["regression suite"]
I3 --> R3["Review risk: Test (3 files)"]
R3 --> V3["targeted test run"]
OpenCode Review Overview
Pull request overviewOpenCode cannot approve yet because required coverage evidence did not pass. Review outcome1. HIGH .github/workflows/opencode-review.yml:1 - Coverage evidence did not prove required test/docstring evidence
Coverage evidenceCoverage Decision
Changed-File Evidence Mapflowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Changed file (4 files)"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Changed file (4 files)"]
R1 --> V1["required checks"]
Evidence --> S2["Docs (2 files)"]
S2 --> I2["operator or user guidance"]
I2 --> R2["Review risk: Docs (2 files)"]
R2 --> V2["docs review"]
Evidence --> S3["Test (3 files)"]
S3 --> I3["regression suite"]
I3 --> R3["Review risk: Test (3 files)"]
R3 --> V3["targeted test run"]
|
2 participants
Why
PR #137 loads the unique non-superseded result for the session a participant is viewing. If two snapshots each claim to supersede the other, that query returns no rows. Treating the empty tip set as "no score yet" lets a restarted worker invent a result.
This successor keeps #137's contiguous
observation_orderand current-tip load, and fails closed when the session has snapshots but no unique tip.What this PR does
0tips + existing session rows asInconsistentEvidence(cycle / all-superseded).Noneonly when the session has no result rows.Out of scope
fast-mlsirm)Test plan
cargo test --lib reference_guard_testscargo clippy --all-targets -- -D warningscargo test --test documentation_architecture_contract --test traceability_active_pr_contract --test result_snapshot_reload_contractcargo test --test postgres_result_snapshot_persistence(needsTEST_DATABASE_URL)Operator next action
Prefer this head over #137 and #119. After exact-head Runtime CI, coverage, and an independent last-push review, merge this slice. Then call
load_current_result_snapshot_for_sessionwith the session the participant is viewing so a restarted worker returns the stored tip or fails closed on a corrupt supersession graph.Do not merge until exact-head checks and independent last-push approval are satisfied. Never self-approve.