diff --git a/internal/executor/executor.go b/internal/executor/executor.go index 002612bc..a6784be8 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,14 @@ 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. 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 new file mode 100644 index 00000000..c0b2519c --- /dev/null +++ b/internal/executor/poll_tmux_requeue_test.go @@ -0,0 +1,100 @@ +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 + wantInterrupted bool + }{ + { + // 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, + wantInterrupted: true, + }, + } + + 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.Requeued != tt.wantRequeued { + t.Errorf("status %q: Requeued = %v, want %v", tt.status, result.Requeued, tt.wantRequeued) + } + 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) + } + }) + } +} 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.