Skip to content

Commit 0517275

Browse files
Keep retry dispatch fresh without leaking claims
#### Context A retry performs a fresh by-id lookup, then dispatch revalidates before starting work. If that second read missed or went stale, the popped retry could stay claimed forever; bypassing it instead could dispatch stale work. #### TL;DR *Keep dispatch-time freshness and let retries release or reschedule from its outcome.* #### Summary - Preserve dispatch-time issue revalidation for retried work. - Release the retry claim when the second refresh is missing or stale. - Reschedule rather than drop the claim when dispatch-time refresh errors. - Add a regression proving a missing second read cannot dispatch stale work or strand a claim. #### Alternatives - Reusing the first retry lookup was smaller but reopened a stale-dispatch window. #### Test Plan - [x] `HEX_HOME=/private/tmp/symphony-hex mise exec -- make -C elixir all` - [x] `mise exec -- mix test test/symphony_elixir/core_test.exs:875` - [x] `mise exec -- mix test test/symphony_elixir/core_test.exs`
1 parent 7cf29df commit 0517275

2 files changed

Lines changed: 87 additions & 5 deletions

File tree

elixir/lib/symphony_elixir/orchestrator.ex

Lines changed: 39 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -905,22 +905,35 @@ defmodule SymphonyElixir.Orchestrator do
905905
end
906906

907907
defp dispatch_issue(%State{} = state, issue, attempt \\ nil, preferred_worker_host \\ nil) do
908-
case revalidate_issue_for_dispatch(issue, &Tracker.fetch_issues_by_ids/1, terminal_state_set()) do
908+
case refresh_issue_for_dispatch(issue) do
909909
{:ok, %Issue{} = refreshed_issue} ->
910910
do_dispatch_issue(state, refreshed_issue, attempt, preferred_worker_host)
911911

912+
{:skip, _reason} ->
913+
state
914+
915+
{:error, _reason} ->
916+
state
917+
end
918+
end
919+
920+
defp refresh_issue_for_dispatch(issue) do
921+
case revalidate_issue_for_dispatch(issue, &Tracker.fetch_issues_by_ids/1, terminal_state_set()) do
922+
{:ok, %Issue{} = refreshed_issue} ->
923+
{:ok, refreshed_issue}
924+
912925
{:skip, :missing} ->
913926
Logger.info("Skipping dispatch; issue no longer active or visible: #{issue_context(issue)}")
914-
state
927+
{:skip, :missing}
915928

916929
{:skip, %Issue{} = refreshed_issue} ->
917930
Logger.info("Skipping stale dispatch after issue refresh: #{issue_context(refreshed_issue)} state=#{inspect(refreshed_issue.state)} blocked_by=#{length(refreshed_issue.blocked_by)}")
918931

919-
state
932+
{:skip, refreshed_issue}
920933

921934
{:error, reason} ->
922935
Logger.warning("Skipping dispatch; issue refresh failed for #{issue_context(issue)}: #{inspect(reason)}")
923-
state
936+
{:error, reason}
924937
end
925938
end
926939

@@ -1169,7 +1182,28 @@ defmodule SymphonyElixir.Orchestrator do
11691182
if retry_candidate_issue?(issue, terminal_state_set()) and
11701183
dispatch_slots_available?(issue, state) and
11711184
worker_slots_available?(state, metadata[:worker_host]) do
1172-
{:noreply, dispatch_issue(state, issue, attempt, metadata[:worker_host])}
1185+
case refresh_issue_for_dispatch(issue) do
1186+
{:ok, %Issue{} = refreshed_issue} ->
1187+
{:noreply, do_dispatch_issue(state, refreshed_issue, attempt, metadata[:worker_host])}
1188+
1189+
{:skip, :missing} ->
1190+
{:noreply, release_issue_claim(state, issue.id)}
1191+
1192+
{:skip, %Issue{} = refreshed_issue} ->
1193+
handle_retry_issue_lookup(refreshed_issue, state, issue.id, attempt, metadata)
1194+
1195+
{:error, reason} ->
1196+
{:noreply,
1197+
schedule_issue_retry(
1198+
state,
1199+
issue.id,
1200+
attempt + 1,
1201+
Map.merge(metadata, %{
1202+
identifier: issue.identifier,
1203+
error: "retry dispatch refresh failed: #{inspect(reason)}"
1204+
})
1205+
)}
1206+
end
11731207
else
11741208
Logger.debug("No available slots for retrying #{issue_context(issue)}; retrying again")
11751209

elixir/test/symphony_elixir/core_test.exs

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -953,6 +953,54 @@ defmodule SymphonyElixir.CoreTest do
953953
refute Map.has_key?(updated_state.retry_attempts, issue_id)
954954
end
955955

956+
test "retry releases its claim when dispatch revalidation no longer finds the issue" do
957+
test_root =
958+
Path.join(
959+
System.tmp_dir!(),
960+
"symphony-elixir-retry-refresh-#{System.unique_integer([:positive])}"
961+
)
962+
963+
issue_id = "retry-refreshed-issue"
964+
965+
try do
966+
write_workflow_file!(Workflow.workflow_file_path(),
967+
tracker_kind: "memory",
968+
workspace_root: test_root,
969+
hook_before_run: "exit 1"
970+
)
971+
972+
Application.put_env(:symphony_elixir, :memory_tracker_issues, [])
973+
{:ok, task_supervisor} = Task.Supervisor.start_link()
974+
975+
state = %Orchestrator.State{
976+
task_supervisor: task_supervisor,
977+
claimed: MapSet.new([issue_id]),
978+
retry_attempts: %{}
979+
}
980+
981+
issue = %Issue{
982+
id: issue_id,
983+
identifier: "MT-566",
984+
title: "Retry refreshed issue",
985+
state: "In Progress",
986+
dispatchable: true,
987+
labels: []
988+
}
989+
990+
updated_state =
991+
Orchestrator.handle_retry_issue_lookup_for_test(issue, state, issue_id, 1, %{
992+
identifier: issue.identifier,
993+
error: "agent exited"
994+
})
995+
996+
refute MapSet.member?(updated_state.claimed, issue_id)
997+
refute Map.has_key?(updated_state.running, issue_id)
998+
refute Map.has_key?(updated_state.retry_attempts, issue_id)
999+
after
1000+
File.rm_rf(test_root)
1001+
end
1002+
end
1003+
9561004
test "agent runner does not continue after a required label is removed" do
9571005
write_workflow_file!(Workflow.workflow_file_path(), tracker_required_labels: ["symphony"])
9581006

0 commit comments

Comments
 (0)