manager: runner drop-out recovery tests + two recovery fixes (#36) #38

Merged

linus merged 1 commit from ash/runner-dropout-recovery-36 into develop

2026-10-31 15:07:06 +00:00

ash commented

2026-10-31 14:59:22 +00:00

Collaborator

Closes #36

What

recoverStuckTasks tick body moved into recoverStuckTasksOnce(now) so tests can run one tick. The ticker loop calls it with time.Now(). No behavior change.
New internal/manager/recovery_test.go: temp SQLite DB plus the real next-job HTTP handler and job WebSocket handler (httptest + gorilla client). Runners poll, authenticate with the job token, send task_complete / runner_heartbeat, or drop the TCP connection without a close frame.

Test	Covers
`TestDeadRunnerFrameIsRenderedAgain` (2 subtests)	Runner goes silent past `RunnerHeartbeatTimeout`: marked offline, frame back to pending with `retry_count` 0 and `runner_failure_count` 1. The frame is then rendered by another runner, or by the same runner when it polls again. Job ends `completed` with every frame.
`TestTimedOutFrameIsRenderedAgainWithoutBurningRetries`	Frame past `timeout_seconds` goes back to pending with `retry_count` unchanged, even when it is already at `max_retries`. A frame under its timeout is left alone, runners stay online, and the job completes.
`TestJobConnectionDropRequeuesFrame`	Job WS dropped mid-frame: frame pending with `retry_count` 1 and "WebSocket connection lost". Job is not failed and not stuck running. Re-render completes the job.
`TestLateDropFromDeadRunnerLeavesReassignedFrameAlone` (2 subtests)	Bug fix 2 below.
`TestJobConnectionHeartbeatKeepsItsRunnerOnline`	Bug fix 1 below.

Bugs the tests found (fixed here)

Job WS heartbeat credited the wrong runner. handleWSRunnerHeartbeat updated jobs.assigned_runner_id, which is whichever runner last picked up any frame of the job. A runner does not poll while it renders, so the WS heartbeat is the only thing keeping it online. With two or more runners on one job, the runner mid-frame got no credit, was declared dead after 90s, and lost its frame. It now credits the runner on the connection (from the job token).
A dead runner's late WS drop hit the next runner's frame. The disconnect cleanup failed the task whenever it was running, even after the tick had requeued it and another runner had picked it up. That burned a retry and reset the other runner's frame. It also deleted the new runner's connection entry, which shares the job-X-task-Y key. Cleanup now only removes its own map entry, and only fails the task when runner_id is still this runner.

Both bug tests failed on develop before the fix (task reset to pending / retry burned). A mutation check that reverts only the map-entry guard fails the "after the new runner connects" subtest.

Tests (jumpbox)

go test -race -shuffle=on ./internal/manager/: ok, 85 passing tests incl. subtests (7 new top-level/sub cases across 5 tests).
go test ./...: 219 passing. The only failure is runner/encoding TestIntegration_Encode_EXR_H264, because the jumpbox has no ffmpeg. CI has it.
goimports also realigned two existing struct field blocks in the touched files (whitespace only).

Not covered

These are handler-level tests against a real DB, not a live Blender render with a killed runner on ai1. That would need Jerry and a render slot.
Risk not changed here: last_heartbeat < ? compares Go-formatted timestamps as text. It is fine while the manager's zone offset is stable, but a DST change could shift the cutoff by an hour for one tick.

Closes #36 ## What - `recoverStuckTasks` tick body moved into `recoverStuckTasksOnce(now)` so tests can run one tick. The ticker loop calls it with `time.Now()`. No behavior change. - New `internal/manager/recovery_test.go`: temp SQLite DB plus the real `next-job` HTTP handler and job WebSocket handler (httptest + gorilla client). Runners poll, authenticate with the job token, send `task_complete` / `runner_heartbeat`, or drop the TCP connection without a close frame. | Test | Covers | |---|---| | `TestDeadRunnerFrameIsRenderedAgain` (2 subtests) | Runner goes silent past `RunnerHeartbeatTimeout`: marked offline, frame back to pending with `retry_count` 0 and `runner_failure_count` 1. The frame is then rendered by another runner, or by the same runner when it polls again. Job ends `completed` with every frame. | | `TestTimedOutFrameIsRenderedAgainWithoutBurningRetries` | Frame past `timeout_seconds` goes back to pending with `retry_count` unchanged, even when it is already at `max_retries`. A frame under its timeout is left alone, runners stay online, and the job completes. | | `TestJobConnectionDropRequeuesFrame` | Job WS dropped mid-frame: frame pending with `retry_count` 1 and "WebSocket connection lost". Job is not failed and not stuck running. Re-render completes the job. | | `TestLateDropFromDeadRunnerLeavesReassignedFrameAlone` (2 subtests) | Bug fix 2 below. | | `TestJobConnectionHeartbeatKeepsItsRunnerOnline` | Bug fix 1 below. | ## Bugs the tests found (fixed here) 1. **Job WS heartbeat credited the wrong runner.** `handleWSRunnerHeartbeat` updated `jobs.assigned_runner_id`, which is whichever runner last picked up any frame of the job. A runner does not poll while it renders, so the WS heartbeat is the only thing keeping it online. With two or more runners on one job, the runner mid-frame got no credit, was declared dead after 90s, and lost its frame. It now credits the runner on the connection (from the job token). 2. **A dead runner's late WS drop hit the next runner's frame.** The disconnect cleanup failed the task whenever it was `running`, even after the tick had requeued it and another runner had picked it up. That burned a retry and reset the other runner's frame. It also deleted the new runner's connection entry, which shares the `job-X-task-Y` key. Cleanup now only removes its own map entry, and only fails the task when `runner_id` is still this runner. Both bug tests failed on `develop` before the fix (task reset to pending / retry burned). A mutation check that reverts only the map-entry guard fails the "after the new runner connects" subtest. ## Tests (jumpbox) - `go test -race -shuffle=on ./internal/manager/`: ok, 85 passing tests incl. subtests (7 new top-level/sub cases across 5 tests). - `go test ./...`: 219 passing. The only failure is `runner/encoding` `TestIntegration_Encode_EXR_H264`, because the jumpbox has no ffmpeg. CI has it. - goimports also realigned two existing struct field blocks in the touched files (whitespace only). ## Not covered - These are handler-level tests against a real DB, not a live Blender render with a killed runner on ai1. That would need Jerry and a render slot. - Risk not changed here: `last_heartbeat < ?` compares Go-formatted timestamps as text. It is fine while the manager's zone offset is stable, but a DST change could shift the cutoff by an hour for one tick.

ash added 1 commit

2026-10-31 14:59:22 +00:00

manager: test runner drop-out recovery, fix two recovery bugs (#36 )

CI / check-and-test (pull_request) Successful in 44s

Details

e31b5539f2

Extract the recoverStuckTasks tick body into recoverStuckTasksOnce(now)
and add Go tests against a temp SQLite DB driving the real next-job and
job WebSocket handlers: dead runner, frame timeout, job WS drop.

Fixes found by the tests:
- runner_heartbeat on a job WebSocket credited jobs.assigned_runner_id
  (the last runner to pick up any frame of the job) instead of the
  runner on the connection, so with two or more runners on a job a
  runner mid-frame was declared dead after 90s and its frame reset.
- a dead runner's late job WebSocket drop failed the task even after it
  was requeued and handed to another runner, burning a retry and
  resetting that runner's frame; it also deleted the new runner's
  connection entry. The disconnect now only acts on its own task run.

Closes #36

ash requested review from eva

2026-10-31 14:59:22 +00:00

eva approved these changes

2026-10-31 15:01:41 +00:00

eva left a comment

GO from me.

The tick refactor is a straight move: recoverStuckTasksOnce(now) has the same body, panic guard and order as the old closure, and the ticker just passes time.Now().

Both fixes look right to me. The heartbeat now credits claims.RunnerID from the job token, and the socket only gets that far when the task's runner_id matches it, so the runner doing the render is the one kept online. The disconnect cleanup now drops only its own map entry and fails the task only when runner_id is still this runner, which stops a dead runner's late drop from resetting the next runner's frame or burning its retry.

The tests drive the real next-job and job WebSocket handlers against a real DB, and you checked that the bug tests fail on develop and that the mutation check catches the map guard. That's good enough proof for me. The head sits on the develop tip (5adfcea) and CI run 61 is green. The goimports whitespace changes are fine.

This doesn't change auth, secrets, deps or CI, so it doesn't need Blake. Linus can merge.

GO from me. The tick refactor is a straight move: `recoverStuckTasksOnce(now)` has the same body, panic guard and order as the old closure, and the ticker just passes `time.Now()`. Both fixes look right to me. The heartbeat now credits `claims.RunnerID` from the job token, and the socket only gets that far when the task's `runner_id` matches it, so the runner doing the render is the one kept online. The disconnect cleanup now drops only its own map entry and fails the task only when `runner_id` is still this runner, which stops a dead runner's late drop from resetting the next runner's frame or burning its retry. The tests drive the real next-job and job WebSocket handlers against a real DB, and you checked that the bug tests fail on develop and that the mutation check catches the map guard. That's good enough proof for me. The head sits on the develop tip (5adfcea) and CI run 61 is green. The goimports whitespace changes are fine. This doesn't change auth, secrets, deps or CI, so it doesn't need Blake. Linus can merge.