Make the assignment start transition one atomic step

64ee368d4a9e · AtlantisPleb · · parent 9eb1b2cfbb57

Make the assignment start transition one atomic step

`start_target/7` read an assignment's state and then wrote `running` back,
with nothing holding the row between the two. A run that finalized inside
that window had already moved the assignment terminal and revoked its
credential, so the write left an attempt that looked live and could not
authenticate: `usable?/1` refuses a revoked credential, so every push it
tried failed while its state said the attempt was still going.

`Assignments.start_running/2` replaces the read and the write with one
conditional update. Postgres evaluates `state not in terminal` while it
holds the row's write lock, so a starter that arrives after `finish/4`
matches no row and changes nothing, whichever of the two reaches the row
first. It returns `{:ok, assignment}` when it made the transition and
`{:already_finished, assignment}` when it lost, and `start_target/7`
refuses with `:assignment_finished` rather than reporting work it did not
start. The Computer path had no guard at all and now takes the same step,
releasing the vaulted credential when it loses.

`assignment_start_race_test.exs` opens the window deliberately: a handler
on Ecto's 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 without a seam in the code under test. It states the invariant
as a query — no assignment is in a non-terminal state while its credential
is revoked — and fails on the read-then-write shape at every seed.

The 30-second sleep that `assignment_credential_auth_test.exs` used to
dodge this race is gone, and that case still passes without it.

Closes #257

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SoZMfWRSGnf6FZX2Ar9rQ2
Co-Authored-By
Claude Fable 5 <noreply@anthropic.com>
Closes
#257

Deploy story

What this commit did to the running system — joined from the forge receipt chain, the part a commit page elsewhere cannot show.

pushed
by user · WAL seq 428 · 2026-08-25T23:26:09.416509Z

Changed files

  • modified INVARIANTS.md
  • modified lib/openagents/forge/assignments.ex
  • modified test/openagents/forge/assignment_credential_auth_test.exs
  • added test/openagents/forge/assignment_start_race_test.exs

Diff

4 files changed, +321 -38

INVARIANTS.md modified +19 -3

@@ -410,9 +410,25 @@ operator, or access non-Git API routes. The credential stores only a digest,

410 410
expires with the assignment deadline, and is revoked when the assignment
411 411
reaches a terminal state.
412 412
413
**No assignment is in a non-terminal state while its credential is revoked.**
414
Revocation and the terminal state are written in one transaction by `finish/4`,
415
and the transition into `running` is one conditional statement:
416
`Assignments.start_running/2` writes only where the state is not already
417
terminal, so Postgres evaluates the guard while it holds the row's write lock.
418
A starter that arrives after the finalizer matches no row, changes nothing, and
419
returns `{:already_finished, assignment}`, which is how its caller tells
420
starting work from finding the work already over.
421
413 422
Evidence: `OpenAgents.Forge.Assignments`,
414
`OpenAgentsWeb.Plugs.ForgeGitAuth`, `OpenAgents.Forge.GitHTTP`, and
415
`test/openagents/forge/assignment_test.exs`.
423
`OpenAgentsWeb.Plugs.ForgeGitAuth`, `OpenAgents.Forge.GitHTTP`,
424
`test/openagents/forge/assignment_test.exs`, and
425
`test/openagents/forge/assignment_start_race_test.exs`.
426
427
(Amended 2026-08-25, issue #257: `start_target/7` read the state and then wrote
428
`running` back, with nothing holding the row between them. A run that finalized
429
inside that window had its credential revoked and its state overwritten, which
430
left an attempt that looked live and could not authenticate. The guard and the
431
write are now one statement.)
416 432
417 433
### IDENTITY-007 — Delegated Box control is explicit and revocable
418 434

@@ -5961,7 +5977,7 @@ contract; the invariant prose above defines the assertion, not the filename.

5961 5977
| IDENTITY-003 | `test/openagents/memory_portability_test.exs` |
5962 5978
| IDENTITY-004 | `test/openagents/agents_test.exs`, `test/openagents_web/controllers/agent_controller_test.exs` |
5963 5979
| IDENTITY-005 | `test/openagents_web/controllers/box_controller_test.exs` |
5964
| IDENTITY-006 | `test/openagents/forge/assignment_test.exs` |
5980
| IDENTITY-006 | `test/openagents/forge/assignment_test.exs`, `test/openagents/forge/assignment_start_race_test.exs` |
5965 5981
| IDENTITY-007 | `test/openagents/agents_test.exs` |
5966 5982
| IDENTITY-008 | `test/openagents_web/controllers/computer_control_api_test.exs`, `test/openagents/inference/computer_revocation_test.exs`, `test/openagents/computer_projection_test.exs`, `test/openagents/machines/index_reach_test.exs` |
5967 5983
| IDENTITY-009 | `test/openagents_web/controllers/delegations_controller_test.exs` |
lib/openagents/forge/assignments.ex modified +64 -21

@@ -100,23 +100,15 @@ defmodule OpenAgents.Forge.Assignments do

100 100
           assignment_credential: plaintext
101 101
         ) do
102 102
      {:ok, run} ->
103
        assignment =
104
          case Repo.get!(Assignment, assignment.id) do
105
            %Assignment{} = current when current.state in @terminal_states ->
106
              current
107
108
            %Assignment{} = current ->
109
              current
110
              |> Assignment.changeset(%{
111
                run_id: run.id,
112
                state: "running",
113
                started_at: DateTime.utc_now()
114
              })
115
              |> Repo.update!()
116
          end
117
118
        _ = announce(assignment)
119
        {:ok, assignment, plaintext}
103
        case start_running(assignment, run.id) do
104
          {:ok, started} ->
105
            _ = announce(started)
106
            {:ok, started, plaintext}
107
108
          {:already_finished, finished} ->
109
            _ = announce(finished)
110
            {:error, :assignment_finished}
111
        end
120 112
121 113
      {:error, reason} ->
122 114
        _ = finish(assignment, "failed", nil, inspect(reason))

@@ -128,11 +120,18 @@ defmodule OpenAgents.Forge.Assignments do

128 120
    if assignment.credential_delivery_status == "enabled",
129 121
      do: AssignmentCredentialVault.put(assignment.id, plaintext)
130 122
131
    assignment =
132
      assignment
133
      |> Assignment.changeset(%{state: "running", started_at: DateTime.utc_now()})
134
      |> Repo.update!()
123
    case start_running(assignment) do
124
      {:already_finished, finished} ->
125
        AssignmentCredentialVault.delete(finished.id)
126
        _ = announce(finished)
127
        {:error, :assignment_finished}
128
129
      {:ok, started} ->
130
        start_computer_job(started, machine, plaintext, attrs, owner, conversation)
131
    end
132
  end
135 133
134
  defp start_computer_job(assignment, machine, plaintext, attrs, owner, conversation) do
136 135
    _ = announce(assignment)
137 136
138 137
    params = %{

@@ -157,6 +156,50 @@ defmodule OpenAgents.Forge.Assignments do

157 156
    end
158 157
  end
159 158
159
  @doc """
160
  Marks an assignment running, unless it already finished.
161
162
  The guard and the write are one statement. Postgres evaluates
163
  `state not in terminal` while it holds the row's write lock, so a starter that
164
  arrives after `finish/4` matches no row and changes nothing, whichever of the
165
  two reaches the row first.
166
167
  Reading the state and then writing it back left a window between them. A run
168
  that finalized inside that window had its credential revoked and its state
169
  overwritten with `running`, which left an attempt that looked live and could
170
  not authenticate: `usable?/1` refuses a revoked credential, so every push it
171
  tried failed while its state said the attempt was still going.
172
173
  Returns `{:ok, assignment}` when this caller made the transition and
174
  `{:already_finished, assignment}` when it lost, so a caller can tell starting
175
  work from finding the work already over.
176
  """
177
  @spec start_running(Assignment.t(), String.t() | nil) ::
178
          {:ok, Assignment.t()} | {:already_finished, Assignment.t()}
179
  def start_running(assignment, run_id \\ nil)
180
181
  def start_running(%Assignment{id: id}, run_id) do
182
    now = DateTime.utc_now()
183
184
    set =
185
      [state: "running", started_at: now, updated_at: now]
186
      |> then(&if(run_id, do: Keyword.put(&1, :run_id, run_id), else: &1))
187
188
    {_count, rows} =
189
      Repo.update_all(
190
        from(a in Assignment,
191
          where: a.id == ^id and a.state not in ^@terminal_states,
192
          select: a
193
        ),
194
        set: set
195
      )
196
197
    case rows do
198
      [%Assignment{} = started] -> {:ok, started}
199
      [] -> {:already_finished, Repo.get!(Assignment, id)}
200
    end
201
  end
202
160 203
  # The work job carries execution: its steps, its report, its budget. The
161 204
  # assignment carries the attempt: which issue, which repository, which
162 205
  # branch, under whose authority. Recording the job id here makes the join
test/openagents/forge/assignment_credential_auth_test.exs modified +7 -14

@@ -39,10 +39,7 @@ defmodule OpenAgents.Forge.AssignmentCredentialAuthTest do

39 39
    original_key = Application.get_env(:openagents, :box_api_key)
40 40
41 41
    # The provider is stubbed so `start_target/7` succeeds and `create/1`
42
    # returns the plaintext. A poll has to answer "still alive": a run that
43
    # reaches a terminal state finalizes its assignment, which revokes the
44
    # credential, and this case would then be racing its own fixture rather
45
    # than testing the lookup.
42
    # returns the plaintext.
46 43
    Application.put_env(:openagents, :box_api,
47 44
      base_url: "https://box-api.internal",
48 45
      request_options: [plug: &stub_provider/1]

@@ -186,21 +183,17 @@ defmodule OpenAgents.Forge.AssignmentCredentialAuthTest do

186 183
    }
187 184
  end
188 185
189
  # The run exists only so `create/1` reaches its return. Any command POST
190
  # parks the worker before it can transition:  a run that reaches a terminal
191
  # state finalizes its assignment, which revokes the credential, and
192
  # `start_target/7` then writes `state: "running"` back over the terminal
193
  # state it read a moment earlier — leaving a live-looking assignment holding
194
  # a revoked credential. A real run takes seconds and never lands in that
195
  # window; a stubbed one closes it in microseconds. `start_run/6` returns as
196
  # soon as the worker's `init/1` does, so parking here does not block
197
  # `create/1`.
186
  # The run exists only so `create/1` reaches its return. It used to park the
187
  # worker on a 30-second sleep, because `start_target/7` read the assignment's
188
  # state and then wrote `running` back over whatever a finalizing run had
189
  # written in between — so a stubbed run that reached a terminal state revoked
190
  # the credential this case had just minted. The guard and the write are one
191
  # statement now (issue #257), so the worker runs at its own speed here.
198 192
  defp stub_provider(conn) do
199 193
    {:ok, raw, conn} = Plug.Conn.read_body(conn)
200 194
201 195
    case Jason.decode(raw) do
202 196
      {:ok, %{"command" => command}} when is_binary(command) ->
203
        Process.sleep(30_000)
204 197
        Req.Test.json(conn, %{"stdout" => "4242\n"})
205 198
206 199
      _other ->
test/openagents/forge/assignment_start_race_test.exs added +231

@@ -0,0 +1,231 @@

1
defmodule OpenAgents.Forge.AssignmentStartRaceTest do
2
  @moduledoc """
3
  A finalized assignment is never resurrected by a slower starter.
4
5
  `start_target/7` read the assignment's state and then wrote `running` back,
6
  with nothing holding the row between the two. A run that finalized inside
7
  that window had already moved the assignment terminal and revoked its
8
  credential, so the write left an attempt that looked live and could not
9
  authenticate: `usable?/1` refuses a revoked credential, and every push the
10
  attempt tried failed while its state said the attempt was still going.
11
12
  The window is microseconds wide, which is why no production run ever fell
13
  into it and why asserting the fix by inspection would prove nothing. These
14
  tests open it deliberately. Ecto publishes a `[:open_agents, :repo, :query]`
15
  event after every query, in the process that made it, so a handler attached
16
  to that event runs between one query the starter makes and the next — which
17
  is the window, exactly. Finalizing the assignment from there puts the
18
  finalizer inside it without a seam in the code under test.
19
20
  The invariant the whole case is about, and the one that fails when the guard
21
  and the write come apart, is stated once in
22
  `refute_live_assignment_with_revoked_credential/0`: no assignment is ever in
23
  a non-terminal state while its credential is revoked.
24
  """
25
26
  use OpenAgents.DataCase, async: false
27
28
  import OpenAgents.AccountsFixtures
29
30
  alias OpenAgents.Box.ConversationBox
31
  alias OpenAgents.Conversations
32
  alias OpenAgents.Forge.{Assignment, AssignmentCredential, Assignments}
33
  alias OpenAgents.Issues
34
  alias OpenAgents.Repo
35
36
  setup do
37
    Ecto.Adapters.SQL.Sandbox.mode(Repo, {:shared, self()})
38
39
    original_api = Application.get_env(:openagents, :box_api)
40
    original_key = Application.get_env(:openagents, :box_api_key)
41
42
    Application.put_env(:openagents, :box_api,
43
      base_url: "https://box-api.internal",
44
      request_options: [plug: &stub_provider/1]
45
    )
46
47
    Application.put_env(:openagents, :box_api_key, "assignment-start-race-test")
48
49
    on_exit(fn ->
50
      restore(:box_api, original_api)
51
      restore(:box_api_key, original_key)
52
    end)
53
54
    :ok
55
  end
56
57
  describe "a run that finalizes while the starter is writing" do
58
    test "leaves the assignment terminal and its credential revoked" do
59
      assignment = admit("start-race-window", "bx_racetst2")
60
61
      finalize_inside_the_starter_window(assignment)
62
63
      _result = Assignments.start_running(assignment, nil)
64
65
      refute_live_assignment_with_revoked_credential()
66
67
      current = Repo.get!(Assignment, assignment.id)
68
      credential = Assignments.credential(assignment)
69
70
      assert current.state == "completed"
71
      refute is_nil(credential.revoked_at)
72
    end
73
  end
74
75
  describe "a starter that arrives after the run finished" do
76
    test "reports that it lost and writes nothing" do
77
      assignment = admit("start-race-late", "bx_racetst3")
78
79
      assert {:ok, _finished} = Assignments.finish(assignment, "completed")
80
81
      assert {:already_finished, current} = Assignments.start_running(assignment, nil)
82
83
      assert current.state == "completed"
84
      assert is_nil(current.started_at)
85
      assert is_nil(current.run_id)
86
      refute_live_assignment_with_revoked_credential()
87
    end
88
89
    test "reports that it won when the assignment is still live" do
90
      assignment = admit("start-race-live", "bx_racetst4")
91
92
      assert {:ok, started} = Assignments.start_running(assignment, nil)
93
94
      assert started.state == "running"
95
      refute is_nil(started.started_at)
96
      refute_live_assignment_with_revoked_credential()
97
    end
98
  end
99
100
  # The invariant, as a query. An assignment whose credential is revoked has
101
  # reached a terminal state, so a row that is neither `completed`, `failed`,
102
  # nor `cancelled` while carrying a revoked credential is an attempt that
103
  # cannot do the work its state claims it is doing.
104
  defp refute_live_assignment_with_revoked_credential do
105
    live =
106
      Repo.all(
107
        from assignment in Assignment,
108
          join: credential in AssignmentCredential,
109
          on: credential.assignment_id == assignment.id,
110
          where:
111
            assignment.state not in ^Assignment.terminal_states() and
112
              not is_nil(credential.revoked_at),
113
          select: {assignment.id, assignment.state}
114
      )
115
116
    assert live == [],
117
           "assignments are live while their credential is revoked: #{inspect(live)}"
118
  end
119
120
  # Finalize the assignment the first time the starter touches its table, and
121
  # once only. The handler runs in the process that made the query, so this
122
  # lands between that query and whatever the starter does next — the window
123
  # the defect lived in. `finish/4` queries the same table, which is what the
124
  # flag stops from re-entering.
125
  defp finalize_inside_the_starter_window(%Assignment{} = assignment) do
126
    handler_id = {__MODULE__, System.unique_integer([:positive])}
127
128
    :telemetry.attach(
129
      handler_id,
130
      [:open_agents, :repo, :query],
131
      fn _event, _measurements, metadata, _config ->
132
        if metadata[:source] == "forge_assignments" and Process.get(:race_armed) do
133
          Process.delete(:race_armed)
134
          {:ok, _finished} = Assignments.finish(assignment, "completed")
135
        end
136
      end,
137
      nil
138
    )
139
140
    on_exit(fn -> :telemetry.detach(handler_id) end)
141
142
    Process.put(:race_armed, true)
143
    :ok
144
  end
145
146
  defp admit(owner_slug, box_id) do
147
    owner = repository_user_fixture(owner_slug)
148
    repository = repository_with_member_fixture(owner, %{visibility: "public"}, "owner")
149
150
    {:ok, issue} = Issues.create_issue(repository, %{title: "Prove the guard", body: "A body."})
151
152
    {:ok, conversation} = Conversations.ensure_conversation(owner)
153
154
    {:ok, box} =
155
      %ConversationBox{}
156
      |> ConversationBox.changeset(%{
157
        conversation_id: conversation.id,
158
        box_id: box_id,
159
        state: "ready",
160
        setup_status: "done"
161
      })
162
      |> Repo.insert()
163
164
    branch = "agent/issue-#{issue.number}"
165
166
    assert {:ok, assignment, _plaintext} =
167
             Assignments.create(%{
168
               "target_kind" => "box",
169
               "box_id" => box.box_id,
170
               "conversation_id" => conversation.id,
171
               "repository_id" => repository.id,
172
               "issue_number" => issue.number,
173
               "branch" => branch,
174
               "requesting_user" => owner,
175
               "requesting_principal" => owner
176
             })
177
178
    # `create/1` starts the attempt, and its run worker keeps polling a stubbed
179
    # provider for as long as it lives. These cases drive the transition
180
    # themselves, so retire the worker first and then put the row back where a
181
    # starter finds it. The assertion is what keeps the fixture honest: a
182
    # worker that finalized the attempt anyway would make this reset the very
183
    # resurrection the case is about, so it fails here instead.
184
    stop_run_worker(assignment)
185
186
    current = Repo.get!(Assignment, assignment.id)
187
    refute Assignment.terminal?(current)
188
189
    current
190
    |> Ecto.Changeset.change(%{state: "admitted", started_at: nil, run_id: nil})
191
    |> Repo.update!()
192
  end
193
194
  defp stop_run_worker(%Assignment{run_id: run_id}) when is_binary(run_id) do
195
    case Registry.lookup(OpenAgents.BoxRunRegistry, run_id) do
196
      [{pid, _value}] ->
197
        reference = Process.monitor(pid)
198
        _ = GenServer.stop(pid, :normal)
199
        assert_receive {:DOWN, ^reference, :process, ^pid, _reason}
200
201
      [] ->
202
        :ok
203
    end
204
  end
205
206
  defp stop_run_worker(%Assignment{}), do: :ok
207
208
  # The run exists only so `create/1` reaches its return. A dispatch that
209
  # answers with a pid parks the worker on its poll interval, which leaves the
210
  # run non-terminal for far longer than these cases take, and `admit/2` stops
211
  # the worker before it polls.
212
  defp stub_provider(conn) do
213
    {:ok, raw, conn} = Plug.Conn.read_body(conn)
214
215
    case Jason.decode(raw) do
216
      {:ok, %{"command" => command}} when is_binary(command) ->
217
        Req.Test.json(conn, %{"stdout" => "4242\n"})
218
219
      _other ->
220
        Req.Test.json(conn, %{
221
          "box" => %{"id" => requested_box_id(conn), "state" => "ready", "setupStatus" => "done"}
222
        })
223
    end
224
  end
225
226
  defp requested_box_id(%Plug.Conn{path_info: ["boxes", box_id | _rest]}), do: box_id
227
  defp requested_box_id(_conn), do: "bx_racetst2"
228
229
  defp restore(key, nil), do: Application.delete_env(:openagents, key)
230
  defp restore(key, value), do: Application.put_env(:openagents, key, value)
231
end

This page updates live while a promote is in flight · changelog