Skip to content

Implement State Audit Protocol V1 - #283

Open
VirtualAdam wants to merge 7 commits into
mainfrom
feature/281-state-audit-v1
Open

VirtualAdam wants to merge 7 commits into
mainfrom
feature/281-state-audit-v1

Conversation

@VirtualAdam

@VirtualAdam VirtualAdam commented Sep 15, 2026

Copy link
Copy Markdown
Collaborator

Current source: ab84f5a - canonical migration portability repair

Exact head: ab84f5aa72649fad2e323fac95d3e8755ebd6e96. This preserves all four review corrections in 58c379d and every committed SQL blob. CI run35142090852 exposed a checksum error in newly added migration50: the local checksum described pre-Git-normalization bytes. The manifest now binds to the unchanged committed LF SQL. Two clean-index-export regressions (autocrlf true and false) reproduce the former failure and pass with the correction; all12 migration-tool cases pass.

The optional CI-workflow addition could not be published with the current token permissions and was omitted, not bypassed. .github/workflows/ci.yml is unchanged. New migration regressions were run locally; the existing hosted migration gate still runs normally. No credentials/scopes were changed. All six hosted jobs now pass at this exact head: https://github.com/All-The-Vibes/ecorp/actions/runs/35143839226. A separate automated reviewer found no blockers in the exact three-file portability follow-up; all seven review threads are resolved. Human review is still requested from @shyamsridhar123 and no human approval is claimed. Historical failures and evidence below remain retained. #293 and its tests are untouched; migrations0047-0049 remain reserved for that contribution.


Current foundation review closeout: 58c379d

Exact source: 58c379deb0f248d3ed6fc0adbb45c4ea96333c4c, additive to foundation 32a7772d13f59561fa205b37a6ea5575283af3aa and its preserved #290 integration.

  • Acquire the publication audit gate before native resource locks, including renewal/replay paths.
  • Preserve coverage identity with additive migration 0050_state_audit_coverage_identity_guard.sql; legitimate fingerprint refresh remains supported.
  • Record attributable monotonic-budget refusals inside the audited action with stable replay receipts.
  • Durably disable publication for ancestry rewrites detected during or after the GitHub file loop.

Migration coordination: #293 already owns versions 0047-0049, which are untouched. The checker now accepts positive, unique, increasing version gaps while retaining manifest/file/version/SHA384 checks. Existing migration SQL is unchanged. SQLx 0.8.6 foundation upgrade 46 -> 50, idempotence and missing-version rejection are covered. Combined Base migration ordering remains the responsibility of #293's separate qualification; this work did not modify or run that PR.

Evidence: all four behavioral regressions first failed against original production code and passed after correction. The combined 80-case PostgreSQL control lane passed; the final five issue283_ PostgreSQL cases and ten Node migration-tool cases passed. All-target workspace Clippy, formatting, immutable migration checks, protocol compatibility, web build/lint and Phoenix trace passed. The full local workspace attempt still encountered unchanged runner-fixture failures; it is not represented as all-green. Earlier CI success below belongs to 32a7772, not this new commit. Fresh hosted CI for 58c379d is pending at publication.

A separate read-only automated reviewer found no blocking issues in this corrective diff. That is not a human approval; review is requested from @shyamsridhar123. The contributor explicitly authorized landing #283, subject to normal repository permissions and gates. Prefer a merge commit to preserve stacked ancestry; no bypass or auto-merge is enabled. #293 remains untouched and draft. #281/#287/#292 acceptance is not closed by this foundation.


Earlier integration and correction history

Current deadlock-fix integration: 32a7772

This branch now incorporates the separate #223 repair from PR #290 at exact original commit 35b76f162e3fbf7f80ae2819ac639fab906de8de, preserving its author and history. New head: 32a7772d13f59561fa205b37a6ea5575283af3aa. The existing run-before-agent lock, destination revalidation and ten regression cases are reused without a competing implementation. The only merge conflict was the EVALS append; both sections are retained. The combined foundation passed 25 real-migration store cases (10 steering plus 15 audit), scoped all-target Clippy, formatting and migration integrity. Complete hosted CI at this exact new head now passes all six jobs, including the full integration job and its Codex and downstream scenarios: https://github.com/All-The-Vibes/ecorp/actions/runs/35122721186. Earlier failures below remain historical records, not the current result. No main merge, issue closure, factory recovery or live-testnet action occurred.


Earlier audit review-correction record

Summary

  • Add deterministic CBOR, domain-separated BLAKE3 history hashes, Ed25519 checkpoints, bounded offline verification, and signing-key history.
  • Transactionally audit covered mission governance mutations, including contract/budget revisions and legacy Factory source upgrades with stable accepted/refused replay receipts.
  • Add durable GitHub publication scheduling, workflow assurance gates, stale-checkpoint prevention, conflict claims, restore-divergence shutdown, and explicit ancestry-checked reconciliation.
  • Include a compiler-bound Solidity reference contract exercised in REVM with the frozen checkpoint vector.
  • Document architecture, security boundaries, operations, compatibility, and acceptance evidence.

Refs #281. The umbrella stays open for separately tracked Base scope and acceptance under #287 / #293; merging this foundation must not automatically close it. Tracking: All-The-Vibes Project #5.

Review closeout and exact source

Correction input: c19cdfdbc5893c4655f6d58bd00f57668879d4a4.
Current foundation head: 6b72eebcf2bd5c990a52b715f45f1876b3240df4.
Exact corrective diff.

372bf58 corrects manifest hashes 42-46 to committed LF SQL, without editing SQL. Missing retained Corp witnesses now use the existing store disable/reconciliation mechanism before publication can proceed. Two actual publisher-loop/PostgreSQL regressions cover missing and other-Corp witnesses, persistent status across reconnection, no repeated retry, and a blocked workflow gate. The offline verifier is accurately described as bounded, not streamed.

After these fixes unblocked the earlier migration failure, hosted Linux CI exposed a pre-existing source/artifact mismatch: the Solidity artifact was compiled from CRLF source, while Git stored LF source. 6b72eeb enforces LF checkout bytes and regenerates compiler metadata with the already installed exact solc-js 0.8.30+commit.73712a01.Emscripten.clang. The contract ABI and executable creation bytecode excluding the metadata trailer are unchanged. The original source-hash/IPFS-binding assertions remain intact. No contract was deployed.

Corrective write scope: crates/crony-server/src/state_audit.rs, db/migrations/manifest.json, .gitattributes, contracts/StateAuditAnchor.compiled.json, and docs/STATE_AUDIT_ETH_V2_CONTRACT.md. Work used an isolated branch/worktree; original development worktrees remain untouched. This reuses the native ECorp publisher and store, not a replacement agent harness or alternate factory verifier.

Local validation and evidence boundary

Local toolchain: Rust 1.97.1, Node 22.14.0, repository-pinned pnpm 11.19.0. These are operator-local results, not a native factory acceptance or independent reviewer approval.

At 372bf58:

  • node tools/check_migrations.mjs: passed, 46 migrations.
  • node tools/check_state_audit_compatibility.mjs: passed.
  • cargo fmt --check: passed.
  • cargo clippy --locked --workspace --all-targets -- -D warnings: passed.
  • cargo test --locked -p crony-server -p crony-store issue281_ -- --ignored --test-threads=1: passed against owned disposable PostgreSQL. Both new publisher regressions were first reproduced failing before the fix.
  • pnpm build:web and pnpm lint:web: passed.
  • cargo test --locked --workspace -- --test-threads=1: failed at the two unchanged Git 2.55 runner fixtures tracked in Native GitHub setup: empty core.hooksPath is incompatible with Git 2.55 #289 (211 runner tests passed, 2 failed, 1 ignored).
  • cargo test --locked --workspace --exclude crony-runner -- --test-threads=1: 347 passed, 339 ignored. This is not a full-workspace pass.

At 6b72eeb, after the compiler-binding correction:

  • cargo test --locked -p crony-audit --test ethereum_local_chain: passed. Canonical LF source first reproduced the exact hosted failure before artifact regeneration.
  • node tools/check_state_audit_compatibility.mjs and cargo fmt --check: passed.
  • Fresh Git index exports with core.autocrlf=true and false retain exact committed source/artifact bytes.

Full local logs are retained in the isolated worktree's ignored output/issue283-*.log; GitHub review replies identify each correction. Hosted results are available in this PR's current checks; earlier hosted failure 104851880429 is retained as the compiler-binding reproduction, not concealed as a pass. No latest all-green hosted claim is made here.

Delivery boundary and next gate

This foundation provides signed audit evidence, GitHub publication, and executable local-contract compatibility. It does not submit public-chain transactions or establish provider independence, production signing authority, legal notarization, or regulatory compliance. Base runtime and live Sepolia acceptance remain separate in #287 / #293.

Independent review of the corrected head remains required before the foundation merge. No self-approval, auto-merge, deployment, native budget reset, or issue closure has been performed. After the foundation lands, deliberately reconcile #293 against the actual merge result rather than assuming stack retargeting preserves ancestry.

Previous hosted closeout status at 6b72eeb

Final same-head result at 6b72eeb: quality, all three runner platforms, and Windows desktop passed. Integration failed on both the initial attempt and the single bounded rerun with the existing #223 steering/run-status PostgreSQL deadlock. The rerun log shows INSERT runner_commands waiting on the runs FK while the other transaction updates agents: https://github.com/All-The-Vibes/ecorp/actions/runs/35114891686/job/104860939451 . No further reruns, test suppression, lock-order repair, or merge-policy bypass were performed. Independent maintainer review and disposition of that known failure remain outstanding; no reviewer has been guessed or approval fabricated. This PR is not merged.

Adds transactional mission governance auditing, signed checkpoints, offline verification, durable publication assurance, restore reconciliation, and executable Ethereum anchor compatibility for issue #281.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 47c873d0-3389-46da-9dd4-027e4d876378
Copilot AI balanced review requested due to automatic review settings September 15, 2026 05:03

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The migration hashes fail validation, missing retained witnesses do not durably disable publication, and the verifier is not streamed as claimed.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Introduces State Audit Protocol V1 for transactional governance auditing, signed checkpoints, offline verification, GitHub publication, and future Ethereum anchoring.

Changes:

  • Adds the crony-audit protocol crate and verification tooling.
  • Integrates audited mission mutations, persistence, publication, and APIs.
  • Adds migrations, Solidity compatibility artifacts, tests, and documentation.
File summaries
File Description
tools/check_state_audit_compatibility.mjs Validates the frozen protocol vector.
package.json Adds audit validation commands.
docs/state-audit-v1-vector.json Defines the V1 compatibility vector.
docs/STATE_AUDIT_V1.md Documents V1 operation and guarantees.
docs/STATE_AUDIT_ETH_V2_CONTRACT.md Specifies Ethereum anchor compatibility.
docs/SECURITY.md Documents audit security boundaries.
docs/EVALS.md Records acceptance-test scope.
docs/ARCHITECTURE.md Describes audit architecture.
db/migrations/manifest.json Registers audit migrations; hashes need correction.
db/migrations/0042_state_audit.sql Creates core audit storage.
db/migrations/0043_state_audit_anchors.sql Adds destinations and receipts.
db/migrations/0044_state_audit_local_results.sql Adds private replay results.
db/migrations/0045_state_audit_identity_guards.sql Protects identities and coverage.
db/migrations/0046_state_audit_assurance.sql Adds scheduling and assurance state.
crates/crony-store/src/state_audit_tests.rs Exercises transactional audit behavior.
crates/crony-store/src/publication.rs Applies workflow assurance gates.
crates/crony-store/src/lib.rs Audits Factory source upgrades.
crates/crony-store/src/contract_revision.rs Audits contract revisions.
crates/crony-store/src/budget_revision.rs Audits budget revisions.
crates/crony-store/Cargo.toml Adds audit dependencies.
crates/crony-server/src/state_audit.rs Adds audit service and API.
crates/crony-server/src/main.rs Starts and routes the audit service.
crates/crony-server/src/factory_connection_tests.rs Updates server fixtures.
crates/crony-server/Cargo.toml Adds the audit crate dependency.
crates/crony-cli/src/main.rs Adds audit request and verification commands.
crates/crony-cli/Cargo.toml Adds the audit crate dependency.
crates/crony-audit/tests/publication.rs Tests additive publication behavior.
crates/crony-audit/tests/protocol.rs Tests canonical protocol behavior.
crates/crony-audit/tests/history.rs Tests history replay and tampering.
crates/crony-audit/tests/ethereum_local_chain.rs Tests the contract through REVM.
crates/crony-audit/src/wire.rs Implements strict JSON parsing.
crates/crony-audit/src/publication.rs Implements GitHub publication.
crates/crony-audit/src/lib.rs Defines canonical hashing and checkpoints.
crates/crony-audit/src/history.rs Implements history verification.
crates/crony-audit/src/archive.rs Implements archive verification.
crates/crony-audit/src/anchor.rs Models anchor progression.
crates/crony-audit/examples/checkpoint_vector.rs Generates the compatibility vector.
crates/crony-audit/Cargo.toml Defines the new crate.
contracts/StateAuditAnchor.sol Implements the anchor contract.
contracts/StateAuditAnchor.compiled.json Provides the compiler-bound artifact.
Cargo.toml Adds the audit workspace member.
Review details
  • Files reviewed: 42/43 changed files
  • Comments generated: 3
  • Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread db/migrations/manifest.json Outdated
Comment thread crates/crony-cli/src/main.rs
Comment thread crates/crony-server/src/state_audit.rs Outdated
Refs #281. Address PR #283 review: disable a destination durably when no retained Corp witness exists, cover the actual publisher loop with PostgreSQL regressions, and correct migration 42-46 hashes to committed LF bytes without changing SQL.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 9ff77cc2-649e-4aef-934d-5aa8b4a54d62
Refs #281. Reproduce the Linux source-hash failure, regenerate metadata with the existing pinned solc-js 0.8.30 compiler, and enforce LF source/artifact checkout bytes. Contract ABI and executable bytecode excluding compiler metadata are unchanged; do not weaken the metadata-binding test.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 9ff77cc2-649e-4aef-934d-5aa8b4a54d62
VirtualAdam added a commit that referenced this pull request Sep 16, 2026
Refs #281, #287. Preserve both published histories; carry PR #283 missing-witness regressions into PR #293 without duplicating the Base implementation.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 9ff77cc2-649e-4aef-934d-5aa8b4a54d62

# Conflicts:
#	db/migrations/manifest.json
@VirtualAdam VirtualAdam moved this to In Review in ECorp Build Sep 16, 2026
@VirtualAdam

Copy link
Copy Markdown
Collaborator Author

Current head 6b72eeb now passes hosted quality, all three runner-platform jobs and Windows desktop. Integration failed in the Codex steering scenario with a PostgreSQL deadlock: control_message insertion waits on runs while another transaction updates agents (job104857681576). The failure is retained in Actions and local logs. Requested one bounded rerun of only that same-head integration job; this is not an all-green claim or an approval. No runtime retry suppression or database reset was added to hide the deadlock.

@VirtualAdam

Copy link
Copy Markdown
Collaborator Author

The integration deadlock matches existing open #223 (agents/run lock inversion), which explicitly tracks this separately from audit foundation work. Added this exact CI occurrence there; retained the original failure before the single diagnostic rerun. No independent reviewer is assigned yet, so the PR is left unmerged. All three original review findings have commit-bound replies.

@VirtualAdam

Copy link
Copy Markdown
Collaborator Author

Final same-head result at 6b72eeb: quality, all three runner platforms, and Windows desktop passed. Integration failed on both the initial attempt and the single bounded rerun with the existing #223 steering/run-status PostgreSQL deadlock. The rerun log shows INSERT runner_commands waiting on the runs FK while the other transaction updates agents: https://github.com/All-The-Vibes/ecorp/actions/runs/35114891686/job/104860939451 . No further reruns, test suppression, lock-order repair, or merge-policy bypass were performed. Independent maintainer review and disposition of that known failure remain outstanding; no reviewer has been guessed or approval fabricated. This PR is not merged.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Unresolved lock-order, coverage-integrity, refusal-recording, and divergence-handling defects affect critical audit guarantees.

Review details
  • Files reviewed: 43/44 changed files
  • Comments generated: 4
  • Review effort level: Balanced

Comment thread crates/crony-store/src/publication.rs Outdated
Comment thread db/migrations/0042_state_audit.sql
Comment thread crates/crony-store/src/budget_revision.rs
Comment thread crates/crony-store/src/state_audit.rs
Integrate PR #290 at 35b76f1 without rewriting the original authored commit. Retain both steering and state-audit evaluation documentation. Refs #223, #283.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 9ff77cc2-649e-4aef-934d-5aa8b4a54d62
@VirtualAdam

Copy link
Copy Markdown
Collaborator Author

Integrated the existing #290 fix for #223 at head32a7772d13f59561fa205b37a6ea5575283af3aa. All 25 explicitly enabled real PostgreSQL steering/audit cases pass on the combination, with the status transaction observed waiting at its run lock rather than the agent update. No lock weakening, retry-until-green or test suppression was introduced. The complete hosted integration job is now the next gate; earlier failures remain retained.

@VirtualAdam

Copy link
Copy Markdown
Collaborator Author

The fresh CI run for repaired head32a7772d13f59561fa205b37a6ea5575283af3aa is now fully green: https://github.com/All-The-Vibes/ecorp/actions/runs/35122721186. All six jobs pass, including complete integration rather than only the formerly failing Codex steering step. Reused #290 unchanged; no retries, skipped tests, lock weakening or main merge. This resolves the observed #223 engineering blocker on this PR; independent review and main landing remain separate.

Correct publication lock ordering, retain auditable budget refusals, disable diverged destinations, and preserve coverage identity with additive migration 0050. Support reserved migration-version gaps without changing historical SQL or checksums.

Refs #281. Preserves migration versions 0047-0049 for the stacked Base contribution.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: c29beaab-8454-4802-8cdf-7e86b42cc8a2
Preserve every SQL blob, correct migration 0050 manifest metadata, and add canonical clean-checkout regressions for both autocrlf modes. The existing CI workflow is unchanged.

Refs #281. Original failed CI evidence is retained; no Base changes.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: c29beaab-8454-4802-8cdf-7e86b42cc8a2
@VirtualAdam

Copy link
Copy Markdown
Collaborator Author

@shyamsridhar123 Foundation landing request: the contributor has explicitly requested merging #283 to unblock stacked #293. The four newest review findings have commit-bound fixes/replies at58c379d, with a follow-up canonical-migration checksum repair at current headab84f5aa72649fad2e323fac95d3e8755ebd6e96. The corrective diff received a separate automated review with no blockers; this is not a human approval. Fresh exact-head CI is pending. Please provide independent maintainer review and, if accepted and gates pass, the authorized landing. A merge commit is preferred to preserve #293 ancestry; retain the foundation branch until that PR is retargeted. No main-policy bypass, automatic issue closure, deployment, or #293 modification is requested.

@VirtualAdam

Copy link
Copy Markdown
Collaborator Author

@shyamsridhar123 Foundation #283 is ready for authorized maintainer landing at exact head ab84f5aa72649fad2e323fac95d3e8755ebd6e96: all six hosted CI jobs pass (https://github.com/All-The-Vibes/ecorp/actions/runs/35143839226), all seven review threads are resolved, and the separate automated corrective reviews found no blockers. Human approval is not being claimed.

I attempted the contributor-authorized normal merge with this head SHA pinned and merge_method=merge. GitHub refused with HTTP 405: Repository rule violations found — Cannot update this protected ref. No admin override, permission change, or alternate write path was attempted.

Please review and merge this exact head using an account authorized by the existing main rules. Prefer a merge commit and retain feature/281-state-audit-v1 for the stacked #293 work. #293 remains owned by the contributor and was not modified by this landing effort. This foundation must not close umbrella #281 or the separate Base acceptance work.

@shyamsridhar123 shyamsridhar123 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed commit ab84f5aa72649fad2e323fac95d3e8755ebd6e96 against ECorp's product, architecture, security, and eval contract.

Changes required: reproduced database deadlock.

  • [P2] crates/crony-store/src/state_audit.rs:286: Reconciliation locks the destination row before calling lock_ledger, while configure_audit_destination locks the same Corp ledger before the destination (lines 412 and 445). Concurrent valid management calls can deadlock. Independently reproduced with both real store methods on PostgreSQL 17: configuration completed, while reconciliation failed with 'deadlock detected'. Acquire the ledger before the destination consistently and retain a regression that requires both concurrent calls to complete. This can interrupt explicit recovery from a disabled publication destination; there is no transaction retry here.

Validation: Reviewed the protocol, canonical serialization, signing/key history, bounded archive verification, native audited governance paths, authorization, publication/reconciliation/scheduling, migrations, CLI changes, compiler binding and compatibility tooling. Independent exact-head checks passed: 11 crony-audit protocol/history/publication/local-EVM tests; 19 state-audit PostgreSQL store tests plus the publication-governance lock gate test; 2 real publisher-loop PostgreSQL server tests; all 12 migration-tool regressions; migration integrity (47 migrations, latest version 50), Node cross-language compatibility and cargo fmt --check. All six refreshed hosted checks pass. A separate temporary regression against unchanged production code invokes the real configuration and reconciliation methods concurrently and fails with PostgreSQL 'deadlock detected'. A temporary four-line lock-order correction then passed the same native concurrency regression and the existing restore-divergence reconciliation test. The fix and regression are preserved as a patch; neither is committed or pushed, and the reviewed branch still contains the defect.

  • Migration 0042 collides with #255's 0042_factory_claim_authority.sql. Coordinate the common migration lineage without altering already-applied migration checksums.

  • When combined with #288, prepare/validate State Audit configuration before database migration/recovery; keep service startup after the store exists. The new Service::from_environment call currently occurs after those database effects.

  • The branch preserves #290's original commit. Preserve its ancestry when integrating.

  • Bounded audit evidence, GitHub publication and local contract compatibility are the foundation scope. Base runtime/live testnet qualification, public-chain submission and production signing authority remain separate.

  • Temporary verification-only source changes were restored after preserving the fix/regression patch. The original workspace source remains unchanged.

Candidate fix and native regression tested locally (not applied to this PR)
diff --git a/crates/crony-store/src/state_audit.rs b/crates/crony-store/src/state_audit.rs
index 8436dd6..9615395 100644
--- a/crates/crony-store/src/state_audit.rs
+++ b/crates/crony-store/src/state_audit.rs
@@ -282,6 +282,10 @@ impl PgStore {
         for mission in missions {
             authorize(&mut tx, corp, actor, mission, "destination").await?;
         }
+        ensure!(
+            lock_ledger(&mut tx, corp).await? == Some(witness.ledger_id),
+            "retained witness ledger identity missing or changed"
+        );
         let destination_corp: Uuid = sqlx::query_scalar(
             "SELECT corp_id FROM state_audit_destinations WHERE id=$1 AND corp_id=$2 FOR UPDATE",
         )
@@ -291,10 +295,6 @@ impl PgStore {
         .await?
         .context("audit destination not found in Corp")?;
         ensure!(destination_corp == corp, "foreign Corp destination");
-        ensure!(
-            lock_ledger(&mut tx, corp).await? == Some(witness.ledger_id),
-            "retained witness ledger identity missing or changed"
-        );
         let archive = Self::load_archive(&mut tx, corp, witness.ledger_id).await?;
         if archive.signing_keys.is_empty() {
             archive.verify_history(key)?;
diff --git a/crates/crony-store/src/state_audit_tests.rs b/crates/crony-store/src/state_audit_tests.rs
index 4c1d70d..ed1b4e5 100644
--- a/crates/crony-store/src/state_audit_tests.rs
+++ b/crates/crony-store/src/state_audit_tests.rs
@@ -1653,3 +1653,137 @@ async fn issue281_nonpolicy_error_is_not_fabricated_as_refusal(pool: PgPool) ->
     );
     Ok(())
 }
+
+// Review regression: unmodified PR #283 production code deadlocks in this schedule.
+// Two real store operations run concurrently; a synthetic membership row lock
+// provides a deterministic schedule; both calls complete only after the fix.
+async fn review283_wait_for_lock(pool: &PgPool, blocker: i32, query_pattern: &str) -> Result<i32> {
+    tokio::time::timeout(std::time::Duration::from_secs(10), async {
+        loop {
+            let waiting = sqlx::query_scalar::<_, i32>(
+                "SELECT pid FROM pg_stat_activity WHERE datname=current_database() AND wait_event_type='Lock' AND query LIKE $2 AND $1=ANY(pg_blocking_pids(pid)) LIMIT 1",
+            )
+            .bind(blocker)
+            .bind(query_pattern)
+            .fetch_optional(pool)
+            .await?;
+            if let Some(pid) = waiting {
+                return Ok::<_, anyhow::Error>(pid);
+            }
+            tokio::time::sleep(std::time::Duration::from_millis(20)).await;
+        }
+    })
+    .await
+    .context("expected native operation did not reach its database lock")?
+}
+
+#[sqlx::test(migrations = "../../db/migrations")]
+#[ignore = "requires the explicitly owned disposable PR review database"]
+async fn review283_configure_reconcile_complete_without_deadlock(pool: PgPool) -> Result<()> {
+    let (store, ids, mission, _, _) = fixture(pool).await?;
+    sqlx::query("UPDATE actors SET role='admin' WHERE id=$1 AND corp_id=$2")
+        .bind(ids.bob_actor_id)
+        .bind(ids.corp_id)
+        .execute(&store.pool)
+        .await?;
+    let ledger = Uuid::new_v4();
+    store
+        .initialize_state_audit(ids.corp_id, ids.alice_actor_id, ledger)
+        .await?;
+    store
+        .cover_mission(ids.corp_id, ids.alice_actor_id, mission.mission_id)
+        .await?;
+    let key = crony_audit::SigningKey::from_bytes(&[7; 32]);
+    let checkpoint = store
+        .audit_checkpoint(ids.corp_id, "fixture-key", &key)
+        .await?;
+    let destination = state_audit::AuditDestination {
+        id: Uuid::new_v4(),
+        corp_id: ids.corp_id,
+        kind: "github".into(),
+        interval_seconds: 60,
+        calendar_schedule: None,
+        overdue_after_seconds: 3600,
+        workflow_gate: "published".into(),
+        config: json!({"repository":"fixture/audit","branch":"main","path":"audit"}),
+    };
+    store
+        .configure_audit_destination(ids.alice_actor_id, &destination)
+        .await?;
+    store
+        .disable_audit_destination_for_divergence(destination.id)
+        .await?;
+
+    let mut barrier = store.pool.begin().await?;
+    let barrier_pid: i32 = sqlx::query_scalar("SELECT pg_backend_pid()")
+        .fetch_one(&mut *barrier)
+        .await?;
+    sqlx::query(
+        "SELECT actor_id FROM room_memberships WHERE room_id=$1 AND actor_id=$2 FOR UPDATE",
+    )
+    .bind(ids.room_id)
+    .bind(ids.alice_actor_id)
+    .fetch_one(&mut *barrier)
+    .await?;
+
+    let configuration_store = store.clone();
+    let configuration_destination = destination.clone();
+    let config_actor = ids.alice_actor_id;
+    let configuration = tokio::spawn(async move {
+        configuration_store
+            .configure_audit_destination(config_actor, &configuration_destination)
+            .await
+    });
+    // Configuration already holds the ledger and pauses at Alice's membership.
+    let configuration_pid =
+        review283_wait_for_lock(&store.pool, barrier_pid, "%FOR KEY SHARE OF rm%").await?;
+
+    let reconciliation_store = store.clone();
+    let reconcile_actor = ids.bob_actor_id;
+    let corp = ids.corp_id;
+    let reconciliation = tokio::spawn(async move {
+        let publication = PublicationFixture::default();
+        let retained_commit = "aa".repeat(20);
+        reconciliation_store
+            .reconcile_audit_destination(
+                corp,
+                reconcile_actor,
+                destination.id,
+                state_audit::AuditReconciliation {
+                    ledger_id: ledger,
+                    checkpoint_digest: &checkpoint.digest,
+                    github_commit: &retained_commit,
+                },
+                &publication,
+                &key.verifying_key(),
+            )
+            .await
+    });
+    // Before the fix, reconciliation holds the destination while waiting on the ledger.
+    let reconciliation_pid = review283_wait_for_lock(
+        &store.pool,
+        configuration_pid,
+        "%state_audit_ledgers%FOR UPDATE%",
+    )
+    .await?;
+    println!(
+        "native configuration pid={configuration_pid}; native reconciliation pid={reconciliation_pid}; both required lock waits observed"
+    );
+    barrier.rollback().await?;
+
+    let (configuration, reconciliation) =
+        tokio::time::timeout(std::time::Duration::from_secs(20), async {
+            futures_util::join!(configuration, reconciliation)
+        })
+        .await
+        .context("native audit management operations did not finish")?;
+    let configuration = configuration?;
+    let reconciliation = reconciliation?;
+    println!("configure_audit_destination: {configuration:?}");
+    println!("reconcile_audit_destination: {reconciliation:?}");
+    assert!(
+        configuration.is_ok() && reconciliation.is_ok(),
+        "both valid concurrent management operations must complete without a deadlock"
+    );
+    Ok(())
+}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: In Review

Development

Successfully merging this pull request may close these issues.

4 participants