Skip to content

fix(executor): stop counting skipped tasks twice on retry runs - #251

Merged
LauJosefsen merged 2 commits into
mainfrom
lejo/fix-send-on-closed-output-channel
Aug 10, 2026
Merged

fix(executor): stop counting skipped tasks twice on retry runs#251
LauJosefsen merged 2 commits into
mainfrom
lejo/fix-send-on-closed-output-channel

Conversation

@LauJosefsen

Copy link
Copy Markdown
Collaborator

Problem

Retrying a failed task from the TUI (r) could crash the whole run:

panic: send on closed channel

goroutine 17356 [running]:
executor.ToChannelOutputHandler.HandleOutput(...)  executor/types.go:39
actions.emitTaskPreamble.func1(...)               actions/runner.go:316
actions.runGroupTask(...)                         actions/runner.go:278
executor.(*Executor).startReadyTasks.func1(...)   executor/executor.go:247

Cause

A retry run builds a new executor over the full task set and calls WithPreCompleted(succeeded, failed). Inside Execute the ordering was:

  1. startReadyTasks — a pending task whose dependency is already statusFailed takes the skip path, which sets status = statusFailed and reports a CommandResult on completionCh.
  2. then finished was seeded by counting tasks already in statusSuccess/statusFailed.

A task skipped during that first call was therefore counted twice: once by the seed, once when its completion was read in the main loop. On a first run nothing is pre-failed so the skip path never fires that early; on a retry run it does, as soon as a re-queued task has a sibling dependency that failed and is not being retried.

finished then reaches total while other retried tasks are still starting, the loop exits, close(outputCh) runs, and the next task goroutine to come off the parallelism semaphore panics in emitTaskPreamble — which is why the crash lands in the preamble of a task that had only just begun.

Fix

  • Seed finished/total before startReadyTasks, so a startup skip is counted only via its completion. This is the actual bug.
  • Stop closing outputCh. Draining now ends on a dedicated drainStop signal, with the buffer flushed on exit as before. A goroutine that outlives the run drops its line instead of killing the process, so a future accounting slip costs a log line rather than the session.

Two regression tests: against the unfixed executor.go both fail with the identical panic; with the fix go test -race -count=3 ./... is clean.

Not addressed here

  • The skip path's completionCh <- ... is an unconditional send on the main goroutine. The buffer is len(tasks)*2, so repeated retry/skip cycles could in principle exhaust it and deadlock instead of panicking. A select cannot fix that from the main goroutine; it needs a different shape.
  • WithPreCompleted marks skipped tasks as plain statusFailed with skipped = false, so resetForRetry's cascade will not re-queue a skipped dependent on a second retry round.

A retry run creates a new executor over the full task set with pre-completed
tasks. A pending task whose dependency is already failed takes the skip path in
the first startReadyTasks call, which marks it failed and reports it on the
completion channel. The finished counter was seeded after that call, so the skip
was counted twice and the run could end while other tasks were still starting,
panicking with "send on closed channel" when the next task emitted its preamble.

Seed the counter before starting tasks, and stop closing the output channel:
draining now ends on a dedicated signal, so a line from a goroutine that
outlives the run is dropped rather than fatal.
@LauJosefsen LauJosefsen self-assigned this Aug 10, 2026
@LauJosefsen
LauJosefsen merged commit d97db50 into main Aug 10, 2026
12 checks passed
@LauJosefsen
LauJosefsen deleted the lejo/fix-send-on-closed-output-channel branch August 10, 2026 10:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants