runner: accept 200 reused re-registration from manager (closes #31) #32

Merged

linus merged 1 commit from fix/31-runner-reregister into develop

2026-10-24 17:29:37 +00:00

ash commented

2026-10-24 17:26:56 +00:00

Collaborator

Closes #31

Why

When a known runner (same fingerprint and API key) registers again, e.g. after a restart, the manager answers 200 OK with reused: true (internal/manager/runners.go). The runner client in internal/runner/api/manager.go only accepted 201 Created, so it treated the reuse as a failure and a restarted runner could never come back (0.0.10 through 0.0.13). This stalled render job 1 for about 10 hours.

What changed

Register accepts both 201 (new runner) and 200 (reused runner). Every other status still errors as before, including the token-error path for 400/401.
RegisterResponse now also decodes reused. Both responses carry id, which is still what sets the runner ID, so a reused runner keeps its existing ID.
A 200/201 body with no usable id (<= 0) is now an error instead of quietly setting runner ID 0.

Tests

New internal/runner/api/register_test.go, using httptest managers:

TestRegister_ReusedRunner200: 200 + reused:true succeeds and returns/sets id 42.
TestRegister_NewRunner201: 201 succeeds and sets the id.
TestRegister_OtherStatusesError: 202, 204, 400, 401, 409, 500 all error and leave runner ID 0.
TestRegister_SuccessWithoutIDErrors: 200/201 without an id errors.

On the old code, TestRegister_ReusedRunner200 fails with registration failed (status 200): {"id":42,...,"reused":true}. With the fix, all four pass.

Risk / rollback

Client-side only, one function. Roll back by reverting this commit.

Closes #31 ## Why When a known runner (same fingerprint and API key) registers again, e.g. after a restart, the manager answers `200 OK` with `reused: true` (internal/manager/runners.go). The runner client in `internal/runner/api/manager.go` only accepted `201 Created`, so it treated the reuse as a failure and a restarted runner could never come back (0.0.10 through 0.0.13). This stalled render job 1 for about 10 hours. ## What changed - `Register` accepts both `201` (new runner) and `200` (reused runner). Every other status still errors as before, including the token-error path for 400/401. - `RegisterResponse` now also decodes `reused`. Both responses carry `id`, which is still what sets the runner ID, so a reused runner keeps its existing ID. - A 200/201 body with no usable `id` (<= 0) is now an error instead of quietly setting runner ID 0. ## Tests New `internal/runner/api/register_test.go`, using httptest managers: - `TestRegister_ReusedRunner200`: 200 + `reused:true` succeeds and returns/sets id 42. - `TestRegister_NewRunner201`: 201 succeeds and sets the id. - `TestRegister_OtherStatusesError`: 202, 204, 400, 401, 409, 500 all error and leave runner ID 0. - `TestRegister_SuccessWithoutIDErrors`: 200/201 without an id errors. On the old code, `TestRegister_ReusedRunner200` fails with `registration failed (status 200): {"id":42,...,"reused":true}`. With the fix, all four pass. ## Risk / rollback Client-side only, one function. Roll back by reverting this commit.

ash added 1 commit

2026-10-24 17:26:56 +00:00

runner: accept 200 reused re-registration from manager (closes #31 )

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

Details

f86161d5e1

ash requested review from eva

2026-10-24 17:26:56 +00:00

eva approved these changes

2026-10-24 17:28:57 +00:00

eva left a comment

The tests cover the actual restart case plus the 201 path, the error statuses, and a success body with no id, and CI is green on f86161d. The head sits on the current develop tip, and it's client-side only with a one-commit revert.

Not security-sensitive (no change to how the manager checks the key), so Linus can merge. After that, a release so ai1 can drop the 0.0.12-jerry1 hotfix.

GO. This is the right fix for #31: the runner now takes the manager's 200 with reused:true as success and keeps the existing runner ID, while every other status still errors the same way as before. The new id <= 0 check is a nice guard against quietly running as runner 0. The tests cover the actual restart case plus the 201 path, the error statuses, and a success body with no id, and CI is green on f86161d. The head sits on the current develop tip, and it's client-side only with a one-commit revert. Not security-sensitive (no change to how the manager checks the key), so Linus can merge. After that, a release so ai1 can drop the 0.0.12-jerry1 hotfix.