From 759396855a967a3d509498cc55faa3b4d8cadfba Mon Sep 17 00:00:00 2001 From: Peter Stone Date: Thu, 26 Mar 2026 09:36:30 +0000 Subject: fix: story stays PENDING when all subtasks complete but parent task stays QUEUED MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a story is approved with pre-created subtasks, parent tasks are QUEUED but never run. Their subtasks complete, but: - maybeUnblockParent only handled BLOCKED parents, not QUEUED ones - checkStoryCompletion required ALL tasks (incl. subtasks) to be done Fixes: - maybeUnblockParent now also promotes QUEUED parents to READY when all subtasks are COMPLETED - checkStoryCompletion only checks top-level tasks (parent_task_id="") - RecoverStaleBlocked now also scans QUEUED parents on startup and triggers checkStoryCompletion if it promotes them - Add QUEUED→READY to valid state transitions (subtask delegation path) Co-Authored-By: Claude Sonnet 4.6 --- internal/executor/executor.go | 57 +++++++++++++++++++++++++----------- internal/executor/executor_test.go | 59 ++++++++++++++++++++++++++++++++------ internal/task/task.go | 2 +- 3 files changed, 92 insertions(+), 26 deletions(-) (limited to 'internal') diff --git a/internal/executor/executor.go b/internal/executor/executor.go index ae040c2..48626ec 100644 --- a/internal/executor/executor.go +++ b/internal/executor/executor.go @@ -511,8 +511,10 @@ func (p *Pool) handleRunResult(ctx context.Context, t *task.Task, exec *storage. p.resultCh <- &Result{TaskID: t.ID, Execution: exec, Err: err} } -// checkStoryCompletion checks whether all tasks in a story have reached a terminal -// success state and transitions the story to SHIPPABLE if so. +// checkStoryCompletion checks whether all top-level tasks in a story have reached +// a terminal success state and transitions the story to SHIPPABLE if so. +// Subtasks are intentionally excluded — a parent task reaching READY/COMPLETED +// already accounts for its subtasks. func (p *Pool) checkStoryCompletion(ctx context.Context, storyID string) { tasks, err := p.store.ListTasksByStory(storyID) if err != nil { @@ -522,11 +524,19 @@ func (p *Pool) checkStoryCompletion(ctx context.Context, storyID string) { if len(tasks) == 0 { return } + topLevelCount := 0 for _, t := range tasks { + if t.ParentTaskID != "" { + continue // subtasks are covered by their parent + } + topLevelCount++ if t.State != task.StateCompleted && t.State != task.StateReady { - return // not all tasks done + return // not all top-level tasks done } } + if topLevelCount == 0 { + return // no top-level tasks — don't auto-complete + } if err := p.store.UpdateStoryStatus(storyID, task.StoryShippable); err != nil { p.logger.Error("checkStoryCompletion: failed to update story status", "storyID", storyID, "error", err) return @@ -1016,18 +1026,31 @@ func (p *Pool) RecoverStaleQueued(ctx context.Context) { } } -// RecoverStaleBlocked promotes any BLOCKED parent task to READY when all of its -// subtasks are already COMPLETED. This handles the case where the server was -// restarted after subtasks finished but before maybeUnblockParent could fire. +// RecoverStaleBlocked promotes any BLOCKED or QUEUED parent task to READY when +// all of its subtasks are already COMPLETED. This handles the case where the +// server was restarted after subtasks finished but before maybeUnblockParent +// could fire, and also the case where story approval pre-created subtasks +// without ever running the parent task. // Call this once on server startup, after RecoverStaleRunning and RecoverStaleQueued. func (p *Pool) RecoverStaleBlocked() { - tasks, err := p.store.ListTasks(storage.TaskFilter{State: task.StateBlocked}) - if err != nil { - p.logger.Error("RecoverStaleBlocked: list tasks", "error", err) - return - } - for _, t := range tasks { - p.maybeUnblockParent(t.ID) + ctx := context.Background() + for _, state := range []task.State{task.StateBlocked, task.StateQueued} { + tasks, err := p.store.ListTasks(storage.TaskFilter{State: state}) + if err != nil { + p.logger.Error("RecoverStaleBlocked: list tasks", "error", err, "state", state) + continue + } + for _, t := range tasks { + if t.ParentTaskID != "" { + continue // only promote actual parents + } + before := t.State + p.maybeUnblockParent(t.ID) + // If the parent was promoted, check story completion. + if after, err := p.store.GetTask(t.ID); err == nil && after.State != before && t.StoryID != "" { + p.checkStoryCompletion(ctx, t.StoryID) + } + } } } @@ -1102,16 +1125,16 @@ func withFailureHistory(t *task.Task, execs []*storage.Execution, err error) *ta return © } -// maybeUnblockParent transitions the parent task from BLOCKED to READY if all -// of its subtasks are in the COMPLETED state. If any subtask is not COMPLETED -// (including FAILED, CANCELLED, RUNNING, etc.) the parent stays BLOCKED. +// maybeUnblockParent transitions the parent task to READY if all of its subtasks +// are in the COMPLETED state. Handles both BLOCKED parents (ran, created subtasks, +// paused) and QUEUED parents (story approval created subtasks without running parent). func (p *Pool) maybeUnblockParent(parentID string) { parent, err := p.store.GetTask(parentID) if err != nil { p.logger.Error("maybeUnblockParent: get parent", "parentID", parentID, "error", err) return } - if parent.State != task.StateBlocked { + if parent.State != task.StateBlocked && parent.State != task.StateQueued { return } subtasks, err := p.store.ListSubtasks(parentID) diff --git a/internal/executor/executor_test.go b/internal/executor/executor_test.go index e16185d..64e3ecb 100644 --- a/internal/executor/executor_test.go +++ b/internal/executor/executor_test.go @@ -747,6 +747,52 @@ func TestPool_RecoverStaleBlocked_KeepsBlockedWhenSubtaskIncomplete(t *testing.T } } +func TestPool_RecoverStaleBlocked_PromotesQueuedParentWithAllSubtasksDone(t *testing.T) { + store := testStore(t) + logger := slog.New(slog.NewTextHandler(os.Stderr, &slog.HandlerOptions{Level: slog.LevelError})) + pool := NewPool(2, map[string]Runner{"claude": &mockRunner{}}, store, logger) + + now := time.Now().UTC() + story := &task.Story{ + ID: "story-queued-parent", Name: "Queued Parent Story", + Status: task.StoryInProgress, CreatedAt: now, UpdatedAt: now, + } + store.CreateStory(story) + + // Parent task stuck QUEUED (approved with pre-created subtasks, never run). + parent := makeTask("queued-parent-1") + parent.State = task.StateQueued + parent.StoryID = story.ID + store.CreateTask(parent) + + for i := 0; i < 2; i++ { + sub := makeTask(fmt.Sprintf("queued-sub-%d", i)) + sub.ParentTaskID = parent.ID + sub.StoryID = story.ID + sub.State = task.StateCompleted + store.CreateTask(sub) + } + + pool.RecoverStaleBlocked() + + got, err := store.GetTask(parent.ID) + if err != nil { + t.Fatalf("GetTask: %v", err) + } + if got.State != task.StateReady { + t.Errorf("parent state: want READY, got %s", got.State) + } + + // Story should have been advanced to SHIPPABLE. + s, err := store.GetStory(story.ID) + if err != nil { + t.Fatalf("GetStory: %v", err) + } + if s.Status != task.StoryShippable { + t.Errorf("story status: want SHIPPABLE, got %s", s.Status) + } +} + func TestPool_ActivePerAgent_DeletesZeroEntries(t *testing.T) { store := testStore(t) runner := &mockRunner{} @@ -1659,16 +1705,15 @@ func TestPool_CheckStoryCompletion_AllComplete(t *testing.T) { t.Fatalf("CreateStory: %v", err) } - // Create two story tasks and drive them through valid transitions to COMPLETED. + // Create two top-level story tasks and drive them through valid transitions to READY. for i, id := range []string{"sctask-1", "sctask-2"} { tk := makeTask(id) tk.StoryID = "story-comp-1" - tk.ParentTaskID = "fake-parent" // so it goes to COMPLETED tk.State = task.StatePending if err := store.CreateTask(tk); err != nil { t.Fatalf("CreateTask %d: %v", i, err) } - for _, s := range []task.State{task.StateQueued, task.StateRunning, task.StateCompleted} { + for _, s := range []task.State{task.StateQueued, task.StateRunning, task.StateReady} { if err := store.UpdateTaskState(id, s); err != nil { t.Fatalf("UpdateTaskState %s → %s: %v", id, s, err) } @@ -1703,19 +1748,17 @@ func TestPool_CheckStoryCompletion_PartialComplete(t *testing.T) { t.Fatalf("CreateStory: %v", err) } - // First task driven to COMPLETED. + // First top-level task driven to READY. tk1 := makeTask("sptask-1") tk1.StoryID = "story-partial-1" - tk1.ParentTaskID = "fake-parent" store.CreateTask(tk1) - for _, s := range []task.State{task.StateQueued, task.StateRunning, task.StateCompleted} { + for _, s := range []task.State{task.StateQueued, task.StateRunning, task.StateReady} { store.UpdateTaskState("sptask-1", s) } - // Second task still in PENDING (not done). + // Second top-level task still in PENDING (not done). tk2 := makeTask("sptask-2") tk2.StoryID = "story-partial-1" - tk2.ParentTaskID = "fake-parent" store.CreateTask(tk2) pool.checkStoryCompletion(context.Background(), "story-partial-1") diff --git a/internal/task/task.go b/internal/task/task.go index ba87360..0d1026f 100644 --- a/internal/task/task.go +++ b/internal/task/task.go @@ -128,7 +128,7 @@ type BatchFile struct { // BLOCKED may advance to READY when all subtasks complete, or back to QUEUED on user answer. var validTransitions = map[State]map[State]bool{ StatePending: {StateQueued: true, StateCancelled: true}, - StateQueued: {StateRunning: true, StateCancelled: true}, + StateQueued: {StateRunning: true, StateCancelled: true, StateReady: true}, // READY: parent task completed via subtask delegation StateRunning: {StateReady: true, StateCompleted: true, StateFailed: true, StateTimedOut: true, StateCancelled: true, StateBudgetExceeded: true, StateBlocked: true}, StateReady: {StateCompleted: true, StatePending: true}, StateFailed: {StateQueued: true}, // retry -- cgit v1.2.3