From 6031352707a3b34a7a5fac87223c359a0e532c59 Mon Sep 17 00:00:00 2001 From: Bruno Bornsztein Date: Mon, 20 Jul 2026 16:01:53 -0500 Subject: [PATCH 1/2] fix: moving a blocked task to In Progress no longer bounces it to Backlog (#674) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A blocked task's executor goroutine stays parked inside pollTmuxSession — the tmux session idles at a prompt, so taskExecutor.Execute never returns and runningTasks[id] stays set. When a human moved the task to "In Progress" (which writes status `queued`), the parked poller saw `queued`, returned `Interrupted: true`, and executeTask's finalization unconditionally wrote `StatusBacklog`, clobbering the requeue. The worker then never ran it because the status was backlog. Because a blocked task's poller is essentially always parked, this was closer to deterministic than racy. Fix: distinguish a deliberate re-queue from a genuine interrupt/cancel. - Add `Requeued` to ExecResult / execResult (same field order — the two convert by direct struct conversion). - pollTmuxSession's `queued` branch now returns {Interrupted, Requeued}; the ctx-cancel and `backlog` branches stay plain {Interrupted} (genuine cancels that should still land in backlog). - executeTask finalization gains a Requeued branch that runs BEFORE the generic interrupt handler: it preserves the queued status (writes no status), kills the stale session, and wakes the worker. The running-task slot is freed in the goroutine's defer, which the worker's admission gate waits on, so the fresh spawn is sequential — no double-session. The TUI modal is unchanged: it already sets `queued`, which now behaves correctly. Same fix covers the retry and web-requeue paths, which also set queued. Not addressed here: executeTask releases the runningTasks/cancelFuncs slot by bare task ID with no ownership token. It is not reachable today (the slot-gate keeps executions strictly sequential), but it is a latent footgun if a future path ever overlaps executions for one task. Flagged for a separate hardening. Co-Authored-By: Claude Opus 4.8 (1M context) --- internal/executor/executor.go | 23 ++++- internal/executor/poll_tmux_requeue_test.go | 95 +++++++++++++++++++++ internal/executor/task_executor.go | 1 + 3 files changed, 115 insertions(+), 4 deletions(-) create mode 100644 internal/executor/poll_tmux_requeue_test.go diff --git a/internal/executor/executor.go b/internal/executor/executor.go index 002612bc..ddcec8fa 100644 --- a/internal/executor/executor.go +++ b/internal/executor/executor.go @@ -1933,7 +1933,18 @@ func (e *Executor) executeTask(ctx context.Context, task *db.Task) { // Update final status and trigger hooks // Respect status set by hooks - don't override blocked with done - if result.Interrupted { + if result.Requeued { + // The task was deliberately re-queued while its session was parked (e.g. a + // human moved a blocked task back to In Progress). Preserve the queued + // status the requeue set — do NOT write backlog — kill the stale session so + // the fresh run starts clean, and let the worker pick it up. Releasing the + // running-task slot happens in this goroutine's defer, which the worker's + // admission gate waits on, so the handoff is sequential (no double-session). + e.logLine(task.ID, "system", "Re-queued by user — starting a fresh run") + taskExecutor.Kill(task.ID) + e.TriggerProcessing() // wake the worker immediately instead of waiting for the next tick + return + } else if result.Interrupted { // Explicitly set to backlog - don't assume Interrupt() already did it, // as the interruption may have come from pollTmuxSession detecting a // stale status or context cancellation. @@ -2415,6 +2426,7 @@ type execResult struct { NeedsInput bool Interrupted bool Message string + Requeued bool } // TmuxDaemonSession is the default session name that holds all Claude task windows. @@ -4094,9 +4106,12 @@ func (e *Executor) pollTmuxSession(ctx context.Context, taskID int64, sessionNam return execResult{Success: true} } if task.Status == db.StatusQueued { - // Task was re-queued (e.g. retry) - stop polling so the - // worker can pick it up fresh without a duplicate session - return execResult{Interrupted: true} + // Task was re-queued (e.g. retry, or a human moving a blocked + // task back to In Progress) - stop polling so the worker can + // pick it up fresh without a duplicate session. Signal the + // requeue distinctly (Requeued) so the caller preserves the + // queued status instead of clobbering it with backlog. + return execResult{Interrupted: true, Requeued: true} } } diff --git a/internal/executor/poll_tmux_requeue_test.go b/internal/executor/poll_tmux_requeue_test.go new file mode 100644 index 00000000..eee7834c --- /dev/null +++ b/internal/executor/poll_tmux_requeue_test.go @@ -0,0 +1,95 @@ +package executor + +import ( + "context" + "testing" + "time" + + "github.com/bborn/workflow/internal/db" +) + +// TestPollTmuxSessionRequeueVsBacklog covers the fix for issue #674: a parked +// pollTmuxSession must distinguish a deliberate re-queue (a human moving a +// blocked task back to "In Progress", which sets the task to queued) from a +// genuine interrupt/cancel (task set to backlog). +// +// - queued -> {Interrupted: true, Requeued: true} so executeTask's +// finalization PRESERVES the queued status instead of clobbering it with +// backlog, and lets the worker spawn a fresh run. +// - backlog -> {Interrupted: true} a genuine interrupt; the +// finalization writes backlog as before. +// +// Both statuses are checked at the very top of pollTmuxSession's tick, before +// any tmux window probe, so a non-existent session name is fine here — the +// function returns from the DB-status branch on the first tick (~1s). +// +// Note: the executeTask finalization branch itself (which turns a Requeued +// result into "preserve queued + kill session + TriggerProcessing") is not +// unit-tested here because executeTask spawns a real executor backend, sets up +// git worktrees, and runs a live tmux session — none of which can be stubbed +// with the current test helpers. The poll-level contract asserted below is the +// signal that branch keys off of, so verifying it deterministically guards the +// behavior that was broken. +func TestPollTmuxSessionRequeueVsBacklog(t *testing.T) { + tests := []struct { + name string + status string + wantRequeued bool + }{ + { + name: "re-queued task signals Requeued so caller preserves queued", + status: db.StatusQueued, + wantRequeued: true, + }, + { + name: "backlog task is a genuine interrupt without Requeued", + status: db.StatusBacklog, + wantRequeued: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + exec, database := newTestExecutor(t) + + task := &db.Task{Title: "poll requeue test", Type: "task", Project: "test"} + if err := database.CreateTask(task); err != nil { + t.Fatal(err) + } + if err := database.UpdateTaskStatus(task.ID, tt.status); err != nil { + t.Fatal(err) + } + + // Bound the test so a logic regression that fails to return can't hang + // the suite. pollTmuxSession returns from the DB-status branch on the + // first 1s tick; give it generous headroom. + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + + done := make(chan execResult, 1) + go func() { + done <- exec.pollTmuxSession(ctx, task.ID, "nonexistent-session-674") + }() + + var result execResult + select { + case result = <-done: + case <-ctx.Done(): + t.Fatalf("pollTmuxSession did not return for status %q within timeout", tt.status) + } + + if !result.Interrupted { + t.Errorf("status %q: Interrupted = false, want true", tt.status) + } + if result.Requeued != tt.wantRequeued { + t.Errorf("status %q: Requeued = %v, want %v", tt.status, result.Requeued, tt.wantRequeued) + } + // A genuine interrupt must never masquerade as a requeue and a requeue + // must always still be flagged interrupted (the caller's if/else chain + // checks Requeued first, then Interrupted). + if result.Requeued && !result.Interrupted { + t.Errorf("status %q: Requeued set without Interrupted", tt.status) + } + }) + } +} diff --git a/internal/executor/task_executor.go b/internal/executor/task_executor.go index 2a6a5122..de9a1902 100644 --- a/internal/executor/task_executor.go +++ b/internal/executor/task_executor.go @@ -13,6 +13,7 @@ type ExecResult struct { NeedsInput bool // Task is waiting for user input Interrupted bool // Task was interrupted by user Message string // Status message or error + Requeued bool // Task was deliberately re-queued while running; caller must preserve queued, not write backlog } // toInternal converts ExecResult to the internal execResult type. From 0c557ff552dfb85889e08ddc7b9c2f6e96222786 Mon Sep 17 00:00:00 2001 From: Bruno Bornsztein Date: Mon, 20 Jul 2026 21:50:39 -0500 Subject: [PATCH 2/2] refactor: report a requeue as Requeued only, not Interrupted MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adopts the cleaner signal from the parallel fix (#677): a re-queue is not a cancellation, so pollTmuxSession now returns {Requeued: true} rather than {Interrupted: true, Requeued: true}. Verified safe — the only reader of Interrupted is executeTask's finalizer else-if, which the Requeued branch already short-circuits (it kills the stale session and returns). Keeps #676's explicit Kill + early return, which close the brief window where the stale process would otherwise outlive the handoff. Test updated to assert the two signals are mutually exclusive. Co-Authored-By: Claude Opus 4.8 (1M context) --- internal/executor/executor.go | 10 +++--- internal/executor/poll_tmux_requeue_test.go | 39 ++++++++++++--------- 2 files changed, 28 insertions(+), 21 deletions(-) diff --git a/internal/executor/executor.go b/internal/executor/executor.go index ddcec8fa..a6784be8 100644 --- a/internal/executor/executor.go +++ b/internal/executor/executor.go @@ -4108,10 +4108,12 @@ func (e *Executor) pollTmuxSession(ctx context.Context, taskID int64, sessionNam if task.Status == db.StatusQueued { // Task was re-queued (e.g. retry, or a human moving a blocked // task back to In Progress) - stop polling so the worker can - // pick it up fresh without a duplicate session. Signal the - // requeue distinctly (Requeued) so the caller preserves the - // queued status instead of clobbering it with backlog. - return execResult{Interrupted: true, Requeued: true} + // pick it up fresh without a duplicate session. Report this as + // Requeued, NOT Interrupted: a requeue is not a cancellation, and + // the finalizer must preserve the queued status rather than + // clobber it with backlog. (The only reader of Interrupted is the + // finalizer's else-if, which the Requeued branch short-circuits.) + return execResult{Requeued: true} } } diff --git a/internal/executor/poll_tmux_requeue_test.go b/internal/executor/poll_tmux_requeue_test.go index eee7834c..c0b2519c 100644 --- a/internal/executor/poll_tmux_requeue_test.go +++ b/internal/executor/poll_tmux_requeue_test.go @@ -32,19 +32,24 @@ import ( // behavior that was broken. func TestPollTmuxSessionRequeueVsBacklog(t *testing.T) { tests := []struct { - name string - status string - wantRequeued bool + name string + status string + wantRequeued bool + wantInterrupted bool }{ { - name: "re-queued task signals Requeued so caller preserves queued", - status: db.StatusQueued, - wantRequeued: true, + // A requeue is NOT a cancellation: Requeued only, never Interrupted, so + // the finalizer preserves 'queued' instead of writing backlog. + name: "re-queued task signals Requeued (not Interrupted)", + status: db.StatusQueued, + wantRequeued: true, + wantInterrupted: false, }, { - name: "backlog task is a genuine interrupt without Requeued", - status: db.StatusBacklog, - wantRequeued: false, + name: "backlog task is a genuine interrupt without Requeued", + status: db.StatusBacklog, + wantRequeued: false, + wantInterrupted: true, }, } @@ -78,17 +83,17 @@ func TestPollTmuxSessionRequeueVsBacklog(t *testing.T) { t.Fatalf("pollTmuxSession did not return for status %q within timeout", tt.status) } - if !result.Interrupted { - t.Errorf("status %q: Interrupted = false, want true", tt.status) - } if result.Requeued != tt.wantRequeued { t.Errorf("status %q: Requeued = %v, want %v", tt.status, result.Requeued, tt.wantRequeued) } - // A genuine interrupt must never masquerade as a requeue and a requeue - // must always still be flagged interrupted (the caller's if/else chain - // checks Requeued first, then Interrupted). - if result.Requeued && !result.Interrupted { - t.Errorf("status %q: Requeued set without Interrupted", tt.status) + if result.Interrupted != tt.wantInterrupted { + t.Errorf("status %q: Interrupted = %v, want %v", tt.status, result.Interrupted, tt.wantInterrupted) + } + // The two signals are mutually exclusive: a requeue must never also be + // flagged as an interrupt, or the finalizer's else-if could finalize it + // to backlog and undo the requeue. + if result.Requeued && result.Interrupted { + t.Errorf("status %q: both Requeued and Interrupted set", tt.status) } }) }