neiam
/gitgud
Git Gud
public · Issues · Pulls · Labels · Forks · Compare · Actions success · Badges · Packages
⭐
1
Log in to mark this repository.
Actions: UpdateLog drops rows for a job id that isn't the runner's
0d2522e · Gabriel Morell · 2026-10-20 18:15
Message
{commit_body(@commit)}
Files changed
modified
lib/git_gud_web/controllers/runner_controller.ex
+8
−4
@@ -567,7 +567,11 @@ defmodule GitGudWeb.RunnerController do
| 567 | 567 | # |
| 568 | 568 | # The id is checked against the authenticated runner: it arrives as a |
| 569 | 569 | # bare integer, and nothing else would stop one runner appending to |
| 570 | − # another repository's job. | |
| 570 | + # another repository's job. An id that isn't one of this runner's jobs | |
| 571 | + # drops the rows (they are still acked, so the runner moves on). It | |
| 572 | + # used to fall back to whatever job the runner had running, which put | |
| 573 | + # rows meant for one job — a stale one, a reaped one — into another's | |
| 574 | + # log; the fallback is for a runner that sends no id at all. | |
| 571 | 575 | defp log_job(runner, tid) when is_integer(tid) and tid > 0 do |
| 572 | 576 | case GitGud.Repo.get(GitGud.Workflows.WorkflowJob, tid) do |
| 573 | 577 | %GitGud.Workflows.WorkflowJob{runner_id: rid} = job when rid == runner.id -> |
@@ -577,13 +581,13 @@ defmodule GitGudWeb.RunnerController do
| 577 | 581 | require Logger |
| 578 | 582 | |
| 579 | 583 | Logger.warning( |
| 580 | − "update_log: job #{tid} is not claimed by runner_id=#{runner.id}; ignoring the id" | |
| 584 | + "update_log: job #{tid} is not claimed by runner_id=#{runner.id}; dropping the rows" | |
| 581 | 585 | ) |
| 582 | 586 | |
| 583 | − claimed_job(runner) | |
| 587 | + nil | |
| 584 | 588 | |
| 585 | 589 | nil -> |
| 586 | − claimed_job(runner) | |
| 590 | + nil | |
| 587 | 591 | end |
| 588 | 592 | end |
| 589 | 593 |
modified
test/git_gud_web/controllers/runner_controller_test.exs
+73
−0
@@ -433,6 +433,79 @@ defmodule GitGudWeb.RunnerControllerTest do
| 433 | 433 | assert bodies.(first_step) == ["installing the android sdk"] |
| 434 | 434 | assert bodies.(second_step) == ["compiling the cli"] |
| 435 | 435 | end |
| 436 | + | |
| 437 | + test "UpdateLog naming a job that isn't this runner's lands in no job's log", %{ | |
| 438 | + conn: conn, | |
| 439 | + runner_token: token | |
| 440 | + } do | |
| 441 | + # The rows used to fall back to whatever job the runner had | |
| 442 | + # running; with an explicit id that isn't ours they are dropped. | |
| 443 | + user = GitGud.AccountsFixtures.user_fixture() | |
| 444 | + {:ok, repo} = GitGud.Repositories.create_repository(user, %{"name" => "strays"}) | |
| 445 | + runner = GitGud.Repo.one!(GitGud.Workflows.Runner) | |
| 446 | + | |
| 447 | + {:ok, other_runner, _} = | |
| 448 | + GitGud.Workflows.Runners.register(%{name: "other", labels: %{"labels" => ["x"]}}) | |
| 449 | + | |
| 450 | + run = | |
| 451 | + %GitGud.Workflows.WorkflowRun{repository_id: repo.id} | |
| 452 | + |> GitGud.Workflows.WorkflowRun.create_changeset(%{ | |
| 453 | + number: 1, | |
| 454 | + workflow_path: ".github/workflows/ci.yml", | |
| 455 | + trigger: "push", | |
| 456 | + head_sha: :crypto.strong_rand_bytes(20) | |
| 457 | + }) | |
| 458 | + |> GitGud.Repo.insert!() | |
| 459 | + | |
| 460 | + claim = fn job_id, runner_id -> | |
| 461 | + job = | |
| 462 | + %GitGud.Workflows.WorkflowJob{workflow_run_id: run.id} | |
| 463 | + |> GitGud.Workflows.WorkflowJob.create_changeset(%{job_id: job_id, definition: %{}}) | |
| 464 | + |> GitGud.Repo.insert!() | |
| 465 | + |> Ecto.Changeset.change( | |
| 466 | + status: "running", | |
| 467 | + runner_id: runner_id, | |
| 468 | + started_at: DateTime.utc_now(:second) | |
| 469 | + ) | |
| 470 | + |> GitGud.Repo.update!() | |
| 471 | + | |
| 472 | + step = | |
| 473 | + %GitGud.Workflows.WorkflowStep{workflow_job_id: job.id} | |
| 474 | + |> GitGud.Workflows.WorkflowStep.create_changeset(%{number: 0, name: job_id}) | |
| 475 | + |> GitGud.Repo.insert!() | |
| 476 | + | |
| 477 | + {job, step} | |
| 478 | + end | |
| 479 | + | |
| 480 | + {_mine, my_step} = claim.("mine", runner.id) | |
| 481 | + {theirs, their_step} = claim.("theirs", other_runner.id) | |
| 482 | + | |
| 483 | + for task_id <- [theirs.id, theirs.id + 1_000_000] do | |
| 484 | + body = | |
| 485 | + %UpdateLogRequest{task_id: task_id, index: 0, rows: [%LogRow{content: "stray"}]} | |
| 486 | + |> UpdateLogRequest.encode() | |
| 487 | + |> IO.iodata_to_binary() | |
| 488 | + | |
| 489 | + resp = | |
| 490 | + conn | |
| 491 | + |> put_req_header("content-type", "application/protobuf") | |
| 492 | + |> put_req_header("authorization", "Bearer " <> token) | |
| 493 | + |> post("/api/actions/_apis/runner.v1.RunnerService/UpdateLog", body) | |
| 494 | + | |
| 495 | + # Acked, so the runner doesn't retry forever… | |
| 496 | + assert resp.status == 200 | |
| 497 | + assert UpdateLogResponse.decode(resp.resp_body).ack_index == 1 | |
| 498 | + end | |
| 499 | + | |
| 500 | + # …but stored nowhere. | |
| 501 | + for step <- [my_step, their_step] do | |
| 502 | + assert GitGud.Repo.all( | |
| 503 | + from l in "workflow_step_logs", | |
| 504 | + where: l.workflow_step_id == ^step.id, | |
| 505 | + select: l.body | |
| 506 | + ) == [] | |
| 507 | + end | |
| 508 | + end | |
| 436 | 509 | end |
| 437 | 510 | |
| 438 | 511 | describe "needs and results" do |
Parents: 3d64eb5