Skip to content

[runner] Make the executor lifecycle explicit - #4286

Merged
un-def merged 1 commit into
masterfrom
pr_runner_executor_lifecycle
Sep 11, 2026
Merged

un-def merged 1 commit into
masterfrom
pr_runner_executor_lifecycle

Conversation

@un-def

@un-def un-def commented Sep 11, 2026

Copy link
Copy Markdown
Collaborator

GetJobInfo was not a getter. It configured runner logging and resolved the job user and working dir, and Run called the same preRun again, guarded by an "already called once" check. That guard returned nil when the first call had failed -- the log file field was already set by then -- so a second call reported success with jobUser and jobWorkingDir left unset. Only Run's jobStateHistory gate, which called this "already running or finished", kept the job from starting with those fields empty.

The pair is now Setup and Finalize, with JobInfo as a plain accessor. Setup has a single caller, so the guard goes with it. Setup finalizes itself when it fails and Run finalizes when it returns, so neither the handler nor a later caller has to remember to; the Finalize the handler defers is a safeguard and nothing more.

Finalize also takes over setting WaitLogsFinished. That used to run in a defer registered before postRun, and so ran before it. The stripper holds the last incomplete line until it is closed, which left the state that tells /api/pull its data is final published one flush early. A failed preRun was worse: it returned before postRun was deferred at all, so the buffered runner logs were never flushed and the state never became final.

`GetJobInfo` was not a getter. It configured runner logging and resolved
the job user and working dir, and `Run` called the same `preRun` again,
guarded by an "already called once" check. That guard returned nil when
the first call had failed -- the log file field was already set by then
-- so a second call reported success with `jobUser` and `jobWorkingDir`
left unset. Only `Run`'s `jobStateHistory` gate, which called this
"already running or finished", kept the job from starting with those
fields empty.

The pair is now `Setup` and `Finalize`, with `JobInfo` as a plain
accessor. `Setup` has a single caller, so the guard goes with it.
`Setup` finalizes itself when it fails and `Run` finalizes when it
returns, so neither the handler nor a later caller has to remember to;
the `Finalize` the handler defers is a safeguard and nothing more.

`Finalize` also takes over setting `WaitLogsFinished`. That used to run
in a defer registered before `postRun`, and so ran before it. The
stripper holds the last incomplete line until it is closed, which left
the state that tells `/api/pull` its data is final published one flush
early. A failed `preRun` was worse: it returned before `postRun` was
deferred at all, so the buffered runner logs were never flushed and the
state never became final.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@un-def
un-def merged commit aa1c153 into master Sep 11, 2026
26 checks passed
@un-def
un-def deleted the pr_runner_executor_lifecycle branch September 11, 2026 12:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant