start_target/7 can write a running state over a finalized assignment, leaving it live with a revoked credential #257

Closed AtlantisPleb opened this 2d ago 1 comment

Evidence

Shipped in the release at c67cf39, promoted 2026-08-26.

1 pushes receipt

What happens

OpenAgents.Forge.Assignments.start_target/7 reads an assignment and writes it
back outside a transaction:

case Repo.get!(Assignment, assignment.id) do
  %Assignment{} = current when current.state in @terminal_states -> current
  %Assignment{} = current -> current |> Assignment.changeset(%{state: "running", ...}) |> Repo.update!()
end

The guard reads the state, and the update writes running — with nothing
holding the row between them. If the run worker finalizes in that window,
finish/4 has already moved the assignment to a terminal state and revoked its
credential, and this update then writes running back over it.

The result is an assignment that looks live while holding a credential that is
revoked: usable?/1 refuses it, so every push it attempts fails
authentication, and the state says the attempt is still going.

Why it has not been seen

The window is the gap between the read and the update. A production run takes
seconds to reach a terminal state, so it is never inside that gap. It is
reachable with a stubbed run that finalizes in microseconds, which is how it
was found: the assignment-credential authentication test hit it reliably until
the fixture worked around it.

Suggested shape

Make the guard and the write one atomic step, the way the rest of this module
already treats a state change that must not race — a conditional update that
refuses when the row has moved, rather than a read followed by a write. The
terminal states are the ones that must win: a finalized assignment must not be
resurrected by a slower starter.

Acceptance criteria

A run that finalizes between the read and the write leaves the assignment in
its terminal state, and the starter reports that it lost rather than
overwriting.
The invariant a test can state: no assignment is ever in a non-terminal state
while its credential is revoked.
The workaround in assignment_credential_auth_test.exs is removed, and the
test still passes without it.

Found while fixing the credential-lookup defect in authenticate/1.

AtlantisPleb opened this issue 2d ago
AtlantisPleb closed this as completed in 64ee368 2d ago
A AtlantisPleb Author 2d ago

Fixed in 64ee368.

start_target/7 now transitions through Assignments.start_running/2, which replaces the read and the write with one conditional update: UPDATE ... WHERE id = $1 AND state NOT IN ('completed','failed','cancelled'). Postgres evaluates the guard while it holds the row's write lock, so a starter arriving after finish/4 matches no row and changes nothing, and a finalizer arriving after the starter waits and then moves the row terminal. Neither order can leave an attempt live with a revoked credential.

Why a conditional update rather than :global.trans or a locking read. The contested resource is one row, and Postgres already serializes writers on it; :global.trans in Targets guards multi-row, cluster-wide deploy sequences, which this is not. There is a second reason that turned out to be load-bearing: an implementation with no read has nothing to interleave into. A FOR UPDATE version would be correct in production but untestable in the Ecto sandbox, where every process shares one connection and a "concurrent" finalizer simply joins the starter's own transaction and defeats the lock. The single statement is correct for a reason a test can observe.

The starter reports which happened. start_running/2 returns {:ok, assignment} when it made the transition and {:already_finished, assignment} when it lost, rather than collapsing the two. start_target/7 refuses with :assignment_finished instead of returning a credential finish/4 has already revoked.

The Computer path had no guard at all — this issue only named the box path. It wrote state: "running" from a stale struct while racing finish_for_machine/2, expire/0, and cancel/2. It now takes the same step and deletes its vaulted credential when it loses.

The test opens the race rather than asserting the fix by inspection. A handler on Ecto's [:open_agents, :repo, :query] telemetry runs in the process that made the query, so finalizing the assignment from there lands between the starter's read and its write with no seam added to the code under test. The assertion is the invariant stated as a query over the ledger: no assignment is in a non-terminal state while its credential is revoked. Reverting to the read-then-write shape fails it identically at seeds 1, 2, and 3; the fix passes at seeds 1 through 5.

The workaround is gone. assignment_credential_auth_test.exs had a Process.sleep(30_000) parking the run worker so it could never transition, inserted to dodge this race while fixing the credential lookup. Removed, along with two comments describing the race as live; that file passes 6/6 without it.

IDENTITY-006 gained the atomicity clause with the new test as evidence and an amendment note. mix precommit green: 4900 passed.