Skip to content

fix(store): prevent steering and runner status deadlocks - #290

Merged
shyamsridhar123 merged 1 commit into
All-The-Vibes:mainfrom
dstkwll:codex/issue-223-steering-deadlock
Sep 17, 2026
Merged

shyamsridhar123 merged 1 commit into
All-The-Vibes:mainfrom
dstkwll:codex/issue-223-steering-deadlock

Conversation

@dstkwll

@dstkwll dstkwll commented Sep 15, 2026

Copy link
Copy Markdown
Collaborator

Summary

Steering could deadlock with a concurrent runner status event because the two transactions acquired agent and run locks in opposite orders. Lock the exact observed run first, retain the existing agent/lease protection, and revalidate the destination before inserting a command. Changed destinations fail with a conflict and rollback; committed replay retains its original destination.

Closes #223.

Provenance and scope

  • Base: main at b252396.
  • Candidate: 35b76f1, branch codex/issue-223-steering-deadlock.
  • Production scope: queue_message in crates/crony-store/src/lib.rs.
  • Tests: crates/crony-store/src/steering_lock_tests.rs; opt-in status stream in scripts/fake-codex-app-server.mjs; Tokio test dependency in crates/crony-store/Cargo.toml and Cargo.lock.
  • Documentation: docs/EVALS.md and docs/evidence/2026-09-15-steering-lock-order.md.
  • Native capability: SQLx 0.8.6 and existing PostgreSQL row/advisory locks. No migration, lock weakening, provider retry engine or new permission mechanism.

Validation

  • Exact baseline deadlock reproduced through native store operations with observed PostgreSQL blocking relationships.
  • Ten real-migration concurrency, authority, replay and rollback regressions passed in disposable PostgreSQL.
  • All six repository gates passed. Workspace tests: 517 passed, 0 failed, 333 ignored; opt-in tests were run separately.
  • Actual local server and runner with a synthetic Codex protocol fixture: forced status/steering contention, HTTP replay, browser steering, two durable acknowledgments, output, usage and completion passed. No paid provider execution.
  • GitHub Copilot Astra/high design and independent review completed. Test-strengthening findings addressed and retested.

Evidence and reproduction.

No hosted CI pass, real-vendor acceptance or global deadlock freedom is claimed. PRs #283 and #255 touch the store file but their inspected patches did not change these functions; refresh before integration. Merge, auto-merge, deployment and Factory intake remain disabled and unauthorized.

Copilot AI balanced review requested due to automatic review settings September 15, 2026 23:05

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.

🟢 Approval recommended

The lock inversion is corrected with consistent revalidation and comprehensive focused evidence.

Pull request overview

Prevents steering/run-status deadlocks by enforcing run-before-agent lock ordering and revalidating message destinations.

Changes:

  • Locks the observed run before agent grant checks.
  • Adds PostgreSQL concurrency and rollback regressions.
  • Adds a bounded status-stream fixture and validation evidence.
File summaries
File Description
crates/crony-store/src/lib.rs Implements lock ordering and destination revalidation.
crates/crony-store/src/steering_lock_tests.rs Adds concurrency and authority regressions.
crates/crony-store/Cargo.toml Adds Tokio test dependency.
Cargo.lock Records dependency metadata.
scripts/fake-codex-app-server.mjs Adds opt-in status contention fixture.
docs/EVALS.md Documents validation procedure.
docs/evidence/2026-09-15-steering-lock-order.md Records scope and runtime evidence.
Review details
  • Files reviewed: 6/7 changed files
  • Comments generated: 0
  • Review effort level: Balanced

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

@dstkwll
dstkwll marked this pull request as ready for review September 16, 2026 00:14
VirtualAdam added a commit that referenced this pull request Sep 16, 2026
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 added a commit that referenced this pull request Sep 16, 2026
Merge foundation32a7772 and preserve original PR #290 commit35b76f1. Retain both Alloy and Tokio development dependencies. Refs #223, #287, #290, #293.

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

@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 35b76f162e3fbf7f80ae2819ac639fab906de8de against ECorp's product, architecture, security, and eval contract.

Recommend approval.

Validation: Complete production/test/evidence diff reviewed; migration checker and cargo fmt passed; all 10 ignored issue223 PostgreSQL tests explicitly executed on owned PostgreSQL 17 fixture and passed (19.30s); six exact-head hosted checks green. Authored full-stack synthetic-fixture evidence inspected, not independently replayed.

@shyamsridhar123
shyamsridhar123 requested a review from a team September 17, 2026 04:51
@shyamsridhar123
shyamsridhar123 marked this pull request as draft September 17, 2026 04:52
@shyamsridhar123
shyamsridhar123 marked this pull request as ready for review September 17, 2026 04:52
@shyamsridhar123
shyamsridhar123 merged commit d4884a0 into All-The-Vibes:main Sep 17, 2026
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Prevent steering-message and run-status agent/run lock inversion

3 participants