From 4d27323311484559e6ecd8b72a374f1ecfa2af29 Mon Sep 17 00:00:00 2001 From: KRATOS <84986124+gnanam1990@users.noreply.github.com> Date: Tue, 25 Aug 2026 13:08:53 +0530 Subject: [PATCH 1/3] fix(swarm): stop source before handoff --- internal/swarm/coordinator.go | 63 ++++++++ internal/swarm/coordinator_test.go | 39 +++++ internal/swarm/lifecycle.go | 91 ++++++++--- internal/swarm/lifecycle_test.go | 241 +++++++++++++++++++++++++++++ internal/swarm/team.go | 91 ++++++++++- internal/swarm/tools.go | 4 +- 6 files changed, 502 insertions(+), 27 deletions(-) diff --git a/internal/swarm/coordinator.go b/internal/swarm/coordinator.go index 123f0b6a9..4e550cbaf 100644 --- a/internal/swarm/coordinator.go +++ b/internal/swarm/coordinator.go @@ -57,6 +57,7 @@ type Task struct { SessionID string CreatedAt time.Time UpdatedAt time.Time + handoff bool // internal claim; status stays non-terminal until the source exits } // agentColors palette for stable per-agent coloring in the TUI/status. Provider @@ -144,12 +145,68 @@ func (c *Coordinator) SetStatus(id string, status TaskStatus) error { if t.Status.terminal() && t.Status != status { return fmt.Errorf("swarm: task %s is %s (terminal); cannot move to %s", id, t.Status, status) } + if t.handoff { + return fmt.Errorf("swarm: task %s is being handed off; cannot move to %s", id, status) + } t.Status = status t.UpdatedAt = c.now() c.notifyChangeLocked() return nil } +// BeginHandoff atomically claims a non-terminal task for one handoff caller +// without reporting it terminal. The visible status remains pending/running +// until FinishHandoff, so collectors cannot observe settled state while the +// source member is still unwinding. +func (c *Coordinator) BeginHandoff(id string) (Task, error) { + c.mu.Lock() + defer c.mu.Unlock() + t, ok := c.tasks[id] + if !ok { + return Task{}, fmt.Errorf("%w: %s", ErrUnknownTask, id) + } + if t.Status.terminal() { + return Task{}, fmt.Errorf("swarm: task %s already %s; cannot hand off", id, t.Status) + } + if t.handoff { + return Task{}, fmt.Errorf("swarm: task %s is already being handed off", id) + } + t.handoff = true + return *t, nil +} + +// AbortHandoff releases a claim before the source member is cancelled. It is +// used when preparing the successor (for example, delivering its note) fails. +func (c *Coordinator) AbortHandoff(id string) { + c.mu.Lock() + defer c.mu.Unlock() + if t, ok := c.tasks[id]; ok && t.handoff && !t.Status.terminal() { + t.handoff = false + } +} + +// FinishHandoff publishes the terminal state only after the source execution +// barrier has closed. +func (c *Coordinator) FinishHandoff(id string) error { + c.mu.Lock() + defer c.mu.Unlock() + t, ok := c.tasks[id] + if !ok { + return fmt.Errorf("%w: %s", ErrUnknownTask, id) + } + if !t.handoff { + return fmt.Errorf("swarm: task %s has no handoff in progress", id) + } + if t.Status.terminal() { + return fmt.Errorf("swarm: task %s already %s", id, t.Status) + } + t.handoff = false + t.Status = StatusHandedOff + t.UpdatedAt = c.now() + c.notifyChangeLocked() + return nil +} + // Complete marks a task done with its result. func (c *Coordinator) Complete(id, result string) error { return c.finish(id, StatusDone, result, "", "") @@ -182,6 +239,9 @@ func (c *Coordinator) finish(id string, status TaskStatus, result, errMsg, sessi if t.Status.terminal() { return fmt.Errorf("swarm: task %s already %s", id, t.Status) } + if t.handoff { + return fmt.Errorf("swarm: task %s is being handed off", id) + } t.Status = status t.Result = result t.Err = errMsg @@ -205,6 +265,9 @@ func (c *Coordinator) Reassign(id, newAgentID string) error { if t.Status.terminal() { return fmt.Errorf("swarm: task %s already %s; cannot reassign", id, t.Status) } + if t.handoff { + return fmt.Errorf("swarm: task %s is being handed off; cannot reassign", id) + } t.AgentID = newAgentID t.Status = StatusPending t.UpdatedAt = c.now() diff --git a/internal/swarm/coordinator_test.go b/internal/swarm/coordinator_test.go index 27d581da8..1a8f61103 100644 --- a/internal/swarm/coordinator_test.go +++ b/internal/swarm/coordinator_test.go @@ -103,6 +103,45 @@ func TestCoordinatorReassign(t *testing.T) { } } +func TestCoordinatorHandoffClaimKeepsTaskNonTerminalUntilFinished(t *testing.T) { + c := NewCoordinator() + _, _ = c.Register("t1", "a1", "team", "desc") + _ = c.SetStatus("t1", StatusRunning) + if _, err := c.BeginHandoff("t1"); err != nil { + t.Fatalf("BeginHandoff: %v", err) + } + if task, _ := c.Get("t1"); task.Status != StatusRunning { + t.Fatalf("status during handoff = %v, want running until source exits", task.Status) + } + if _, err := c.BeginHandoff("t1"); err == nil { + t.Fatal("a second handoff claim must fail") + } + if err := c.Complete("t1", "late result"); err == nil { + t.Fatal("member completion must not win after handoff is claimed") + } + if err := c.Reassign("t1", "a2"); err == nil { + t.Fatal("orphan adoption must not race a claimed handoff") + } + if err := c.FinishHandoff("t1"); err != nil { + t.Fatalf("FinishHandoff: %v", err) + } + if task, _ := c.Get("t1"); task.Status != StatusHandedOff { + t.Fatalf("status after FinishHandoff = %v, want handed-off", task.Status) + } +} + +func TestCoordinatorAbortHandoffRestoresNormalCompletion(t *testing.T) { + c := NewCoordinator() + _, _ = c.Register("t1", "a1", "team", "desc") + if _, err := c.BeginHandoff("t1"); err != nil { + t.Fatalf("BeginHandoff: %v", err) + } + c.AbortHandoff("t1") + if err := c.Complete("t1", "done"); err != nil { + t.Fatalf("Complete after AbortHandoff: %v", err) + } +} + func TestCoordinatorColorStability(t *testing.T) { c := NewCoordinator() first := c.Color("a1") diff --git a/internal/swarm/lifecycle.go b/internal/swarm/lifecycle.go index c6e03b8f2..4d76730d3 100644 --- a/internal/swarm/lifecycle.go +++ b/internal/swarm/lifecycle.go @@ -28,6 +28,7 @@ func (s *Swarm) Spawn(pol Policy, teamName, agentType, task, cwd string) (string return "", err } s.rememberCwd(id, cwd) + s.startTaskRun(id) spec := s.buildSpec(pol, id, id, team, def, task, cwd) s.dispatchAdmitted(spec) return id, nil @@ -76,9 +77,11 @@ func (s *Swarm) dispatchAdmitted(spec MemberSpec) { // failure fails the task and frees the slot. // The caller must hold an admission ticket from beginLifecycleAdmission. func (s *Swarm) launchAdmitted(t *Team, spec MemberSpec) { - handle, err := s.launchMemberAdmitted(spec) + run := s.ensureTaskRun(spec.TaskID) + handle, err := s.launchMemberAdmitted(spec, run) if err != nil { _ = s.coord.Fail(spec.TaskID, "launch: "+err.Error()) + run.finish() if err == ErrSwarmClosed { t.releaseSlot() return @@ -87,19 +90,27 @@ func (s *Swarm) launchAdmitted(t *Team, spec MemberSpec) { return } - committed := s.commitLaunch(handle, func() { + committed := s.commitLaunch(handle, run, func() { m := &Member{ID: spec.ID, AgentType: spec.AgentType, TaskID: spec.TaskID, handle: handle} t.addMember(m) _ = s.coord.SetStatus(spec.TaskID, StatusRunning) s.watchers.Add(1) go func() { defer s.watchers.Done() - s.watch(t, m, spec) + s.watch(t, m, spec, run) }() }) if !committed { _ = s.coord.Fail(spec.TaskID, "launch: "+ErrSwarmClosed.Error()) - t.releaseSlot() + run.finish() + s.lifecycleMu.RLock() + closed := s.closed + s.lifecycleMu.RUnlock() + if closed { + t.releaseSlot() + } else { + s.afterExitAdmitted(t) + } } } @@ -111,15 +122,15 @@ func (s *Swarm) launchAdmitted(t *Team, spec MemberSpec) { // ownership (registration, status transition, watcher Add, handle swap) are // atomic with respect to Close setting closed and beginning its waits. // -// It reports whether the handle was adopted. When shutdown won, the handle is -// reaped before returning false: its launch context was already cancelled by -// Close, and MemberHandle has no separate cancellation operation — Launch's -// context is its cancellation contract — so waiting is how a -// successfully-created process/goroutine avoids being abandoned. The caller -// records the terminal outcome for the task it was launching. -func (s *Swarm) commitLaunch(handle MemberHandle, adopt func()) bool { +// It reports whether the handle was adopted. When shutdown or task cancellation +// won, the handle is reaped before returning false: MemberHandle has no separate +// cancellation operation — Launch's context is its cancellation contract — so +// waiting is how a successfully-created process/goroutine avoids being +// abandoned. The caller records the terminal outcome for the task it was +// launching. +func (s *Swarm) commitLaunch(handle MemberHandle, run *taskRun, adopt func()) bool { s.lifecycleMu.RLock() - if s.closed { + if s.closed || run.stopped() { s.lifecycleMu.RUnlock() _, _ = handle.Wait() return false @@ -133,14 +144,17 @@ func (s *Swarm) commitLaunch(handle MemberHandle, adopt func()) bool { // invoking the external launcher. A lifecycle ticket proves the operation won // admission before Close, but the caller may not reach its launch until after // Close has set closed; in that case no new member should be started. -func (s *Swarm) launchMemberAdmitted(spec MemberSpec) (MemberHandle, error) { +func (s *Swarm) launchMemberAdmitted(spec MemberSpec, run *taskRun) (MemberHandle, error) { s.lifecycleMu.RLock() closed := s.closed s.lifecycleMu.RUnlock() if closed { return nil, ErrSwarmClosed } - return s.launcher.Launch(s.baseCtx, spec) + if run.stopped() { + return nil, context.Canceled + } + return s.launcher.Launch(run.ctx, spec) } // watch awaits a member, applies bounded relaunch on temporary failures, records @@ -150,11 +164,11 @@ func (s *Swarm) launchMemberAdmitted(spec MemberSpec) (MemberHandle, error) { // retry. This is sound because a Member is bound 1:1 to its spec for its whole // life (Member.ID == MemberSpec.ID); a retry never reuses the struct for a // different spec. -func (s *Swarm) watch(t *Team, m *Member, spec MemberSpec) { +func (s *Swarm) watch(t *Team, m *Member, spec MemberSpec, run *taskRun) { for { res, err := m.handle.Wait() if err != nil { - if isRetryable(err) && m.restarts < maxMemberRestarts && s.relaunchAdmitted(m, spec) { + if isRetryable(err) && m.restarts < maxMemberRestarts && s.relaunchAdmitted(m, spec, run) { continue } // Fall through if shutdown started or the relaunch failed. @@ -167,6 +181,10 @@ func (s *Swarm) watch(t *Team, m *Member, spec MemberSpec) { break } t.removeMember(m.ID) + // Publish the execution barrier before draining the team queue. Queue drain + // invokes the external launcher synchronously and may block; the completed + // source must not keep a handoff waiting on unrelated successor work. + run.finish() s.afterExit(t) } @@ -179,17 +197,17 @@ func (s *Swarm) watch(t *Team, m *Member, spec MemberSpec) { // consulting its context, so a Close landing between the pre-check and the return // would otherwise resume supervising a member started after shutdown. When any // gate refuses, the caller records the member's terminal failure instead. -func (s *Swarm) relaunchAdmitted(m *Member, spec MemberSpec) bool { +func (s *Swarm) relaunchAdmitted(m *Member, spec MemberSpec, run *taskRun) bool { release, err := s.beginLifecycleAdmission() if err != nil { return false } defer release() - handle, err := s.launchMemberAdmitted(spec) + handle, err := s.launchMemberAdmitted(spec, run) if err != nil { return false } - return s.commitLaunch(handle, func() { + return s.commitLaunch(handle, run, func() { m.restarts++ m.handle = handle }) @@ -228,14 +246,17 @@ func (s *Swarm) afterExitAdmitted(t *Team) { if closed { t.releaseSlot() _ = s.coord.Fail(next.TaskID, ErrSwarmClosed.Error()) + s.finishTaskRun(next.TaskID) return } s.launchAdmitted(t, next) } // Handoff transfers a task to a fresh member of toAgentType, delivering a note to -// the new member's inbox and marking the original task handed-off. It returns the -// new task id. A handoff of an already-terminal task is rejected (fail closed). +// the new member's inbox and marking the original task handed-off. It cancels and +// joins the source task before dispatching the successor, so only one member can +// execute the objective at a time. It returns the new task id. A handoff of an +// already-terminal task is rejected (fail closed). func (s *Swarm) Handoff(pol Policy, teamName, taskID, toAgentType, note string) (string, error) { release, err := s.beginLifecycleAdmission() if err != nil { @@ -255,6 +276,13 @@ func (s *Swarm) Handoff(pol Policy, teamName, taskID, toAgentType, note string) return "", err } team := sanitizeName(teamName) + if task.Team != team { + return "", fmt.Errorf("swarm: task %s belongs to team %s, not %s", taskID, task.Team, team) + } + task, err = s.coord.BeginHandoff(taskID) + if err != nil { + return "", err + } newID := s.nextID(toAgentType) handoffTask := task.Description if note != "" { @@ -267,16 +295,30 @@ func (s *Swarm) Handoff(pol Policy, teamName, taskID, toAgentType, note string) if mbErr := s.mailbox.Send(team, newID, Message{ From: task.AgentID, Subject: "handoff", Body: note, Type: "handoff", Time: nowRFC3339(), }); mbErr != nil { + s.coord.AbortHandoff(taskID) return "", fmt.Errorf("swarm: deliver handoff note: %w", mbErr) } } + cwd := s.cwdFor(taskID) + // Stop a running member, remove a queued one, or cancel a launch currently + // between slot reservation and handle adoption. The completion barrier closes + // only after no source member can execute further side effects. + run := s.taskRun(taskID) + if run != nil { + run.stop() + if s.team(team).removeQueuedTask(taskID) { + run.finish() + } + <-run.done + } + if err := s.coord.FinishHandoff(taskID); err != nil { + return "", err + } if _, err := s.coord.Register(newID, newID, team, handoffTask); err != nil { return "", err } - cwd := s.cwdFor(taskID) s.rememberCwd(newID, cwd) - // Retire the original task (it has been re-delegated). - _ = s.coord.SetStatus(taskID, StatusHandedOff) + s.startTaskRun(newID) spec := s.buildSpec(pol, newID, newID, team, def, handoffTask, cwd) s.dispatchAdmitted(spec) return newID, nil @@ -313,6 +355,7 @@ func (s *Swarm) AdoptOrphans(pol Policy, teamName, toAgentType string) ([]string } cwd := s.cwdFor(task.ID) s.rememberCwd(task.ID, cwd) + s.startTaskRun(task.ID) spec := s.buildSpec(pol, newAgent, task.ID, team, def, task.Description, cwd) s.dispatchAdmitted(spec) adopted = append(adopted, task.ID) diff --git a/internal/swarm/lifecycle_test.go b/internal/swarm/lifecycle_test.go index e119550c1..198c650bf 100644 --- a/internal/swarm/lifecycle_test.go +++ b/internal/swarm/lifecycle_test.go @@ -5,6 +5,7 @@ import ( "errors" "strings" "sync" + "sync/atomic" "testing" "time" ) @@ -409,6 +410,246 @@ func TestHandoffDeliversNoteAndRetiresOriginal(t *testing.T) { } } +func TestHandoffStopsOriginalBeforeSuccessorStarts(t *testing.T) { + originalStarted := make(chan struct{}) + originalStopped := make(chan struct{}) + successorStarted := make(chan struct{}) + var successorOverlapped atomic.Bool + + launcher := FuncLauncher{Run: func(ctx context.Context, spec MemberSpec) (MemberResult, error) { + if spec.AgentType == "teammate" { + close(originalStarted) + <-ctx.Done() + close(originalStopped) + return MemberResult{}, ctx.Err() + } + select { + case <-originalStopped: + default: + successorOverlapped.Store(true) + } + close(successorStarted) + return MemberResult{Result: "continued"}, nil + }} + sw := newSwarmFor(t, launcher) + pol := Policy{Model: "m"} + origID, err := sw.Spawn(pol, "team", "teammate", "original task", "/w") + if err != nil { + t.Fatalf("Spawn: %v", err) + } + select { + case <-originalStarted: + case <-time.After(3 * time.Second): + t.Fatal("original member never started") + } + + if _, err := sw.Handoff(pol, "team", origID, "subagent", "continue safely"); err != nil { + t.Fatalf("Handoff: %v", err) + } + select { + case <-successorStarted: + case <-time.After(3 * time.Second): + t.Fatal("successor member never started") + } + if successorOverlapped.Load() { + t.Fatal("successor started before the original member stopped") + } +} + +func TestHandoffWaitsForOriginalThatIgnoresCancellation(t *testing.T) { + originalStarted := make(chan struct{}) + releaseOriginal := make(chan struct{}) + originalStopped := make(chan struct{}) + successorStarted := make(chan struct{}) + defer func() { + select { + case <-releaseOriginal: + default: + close(releaseOriginal) + } + }() + + launcher := FuncLauncher{Run: func(_ context.Context, spec MemberSpec) (MemberResult, error) { + if spec.AgentType == "teammate" { + close(originalStarted) + <-releaseOriginal + close(originalStopped) + return MemberResult{}, nil + } + close(successorStarted) + return MemberResult{Result: "continued"}, nil + }} + sw := newSwarmFor(t, launcher) + pol := Policy{Model: "m"} + origID, err := sw.Spawn(pol, "team", "teammate", "original task", "/w") + if err != nil { + t.Fatalf("Spawn: %v", err) + } + select { + case <-originalStarted: + case <-time.After(3 * time.Second): + t.Fatal("original member never started") + } + + handoffDone := make(chan error, 1) + go func() { + _, err := sw.Handoff(pol, "team", origID, "subagent", "continue safely") + handoffDone <- err + }() + select { + case err := <-handoffDone: + t.Fatalf("Handoff returned before the original stopped: %v", err) + case <-successorStarted: + t.Fatal("successor started while the original was still running") + case <-time.After(100 * time.Millisecond): + } + if task, ok := sw.Coordinator().Get(origID); !ok || task.Status != StatusRunning { + t.Fatalf("original status while it is still running = %+v, want running", task) + } + + close(releaseOriginal) + select { + case <-originalStopped: + case <-time.After(3 * time.Second): + t.Fatal("original member did not stop") + } + select { + case err := <-handoffDone: + if err != nil { + t.Fatalf("Handoff: %v", err) + } + case <-time.After(3 * time.Second): + t.Fatal("Handoff did not return after the original stopped") + } + select { + case <-successorStarted: + case <-time.After(3 * time.Second): + t.Fatal("successor member never started") + } +} + +func TestHandoffDoesNotWaitForUnrelatedQueuedLaunch(t *testing.T) { + originalStarted := make(chan struct{}) + queuedLaunchStarted := make(chan struct{}) + releaseQueuedLaunch := make(chan struct{}) + defer close(releaseQueuedLaunch) + + launcher := &handoffQueueLauncher{ + originalStarted: originalStarted, + queuedLaunchStarted: queuedLaunchStarted, + releaseQueuedLaunch: releaseQueuedLaunch, + } + sw := newSwarmFor(t, launcher) + sw.maxTeamSize = 1 + pol := Policy{Model: "m"} + origID, err := sw.Spawn(pol, "team", "teammate", "original task", "/w") + if err != nil { + t.Fatalf("Spawn original: %v", err) + } + select { + case <-originalStarted: + case <-time.After(3 * time.Second): + t.Fatal("original member never started") + } + if _, err := sw.Spawn(pol, "team", "teammate", "queued task", "/w"); err != nil { + t.Fatalf("Spawn queued: %v", err) + } + + handoffDone := make(chan error, 1) + go func() { + _, err := sw.Handoff(pol, "team", origID, "subagent", "continue safely") + handoffDone <- err + }() + select { + case <-queuedLaunchStarted: + case <-time.After(3 * time.Second): + t.Fatal("queued launch never started") + } + select { + case err := <-handoffDone: + if err != nil { + t.Fatalf("Handoff: %v", err) + } + case <-time.After(3 * time.Second): + t.Fatal("Handoff waited for an unrelated queued launch") + } +} + +type handoffQueueLauncher struct { + originalStarted chan struct{} + queuedLaunchStarted chan struct{} + releaseQueuedLaunch chan struct{} +} + +func (l *handoffQueueLauncher) Launch(ctx context.Context, spec MemberSpec) (MemberHandle, error) { + switch spec.Task { + case "original task": + close(l.originalStarted) + h := &funcHandle{id: spec.ID, done: make(chan struct{})} + go func() { + defer close(h.done) + <-ctx.Done() + h.err = ctx.Err() + }() + return h, nil + case "queued task": + close(l.queuedLaunchStarted) + <-l.releaseQueuedLaunch + } + return &funcHandle{ + id: spec.ID, + done: closedChan(), + res: MemberResult{Result: "continued"}, + }, nil +} + +func TestHandoffRemovesQueuedOriginalBeforeDispatchingSuccessor(t *testing.T) { + gate := make(chan struct{}) + defer func() { + select { + case <-gate: + default: + close(gate) + } + }() + l := newLauncher(okFor) + l.gate = gate + sw := newSwarmFor(t, l) + sw.maxTeamSize = 1 + pol := Policy{Model: "m"} + + if _, err := sw.Spawn(pol, "team", "teammate", "slot blocker", "/w"); err != nil { + t.Fatalf("Spawn blocker: %v", err) + } + waitFor(t, "blocker running", func() bool { return len(l.recorded()) == 1 }) + origID, err := sw.Spawn(pol, "team", "teammate", "queued original", "/w") + if err != nil { + t.Fatalf("Spawn queued original: %v", err) + } + if got := sw.team("team").QueueDepth(); got != 1 { + t.Fatalf("queue depth = %d, want original queued", got) + } + + newID, err := sw.Handoff(pol, "team", origID, "subagent", "continue safely") + if err != nil { + t.Fatalf("Handoff: %v", err) + } + close(gate) + waitFor(t, "successor launch", func() bool { + for _, spec := range l.recorded() { + if spec.ID == newID { + return true + } + } + return false + }) + for _, spec := range l.recorded() { + if spec.ID == origID { + t.Fatal("queued original launched after it was handed off") + } + } +} + func TestAdoptOrphans(t *testing.T) { l := newLauncher(okFor) sw := newSwarmFor(t, l) diff --git a/internal/swarm/team.go b/internal/swarm/team.go index 1c242a2d1..5282bd11b 100644 --- a/internal/swarm/team.go +++ b/internal/swarm/team.go @@ -79,7 +79,8 @@ type Swarm struct { mu sync.Mutex teams map[string]*Team taskCwd map[string]string // taskID -> cwd, for handoff/adoption relaunch - scheduler *Scheduler // lazily created by Scheduler(); nil until first use + taskRuns map[string]*taskRun + scheduler *Scheduler // lazily created by Scheduler(); nil until first use idSeq atomic.Uint64 } @@ -104,6 +105,39 @@ type Member struct { restarts int } +// taskRun owns the cancellation and completion boundary for one coordinator +// task. The context is shared by every bounded relaunch of that task; done is +// closed only after no current or queued member can execute it. +type taskRun struct { + ctx context.Context + cancel context.CancelFunc + done chan struct{} + once sync.Once +} + +func newTaskRun(parent context.Context) *taskRun { + ctx, cancel := context.WithCancel(parent) + return &taskRun{ctx: ctx, cancel: cancel, done: make(chan struct{})} +} + +func (r *taskRun) stop() { r.cancel() } + +func (r *taskRun) stopped() bool { + select { + case <-r.ctx.Done(): + return true + default: + return false + } +} + +func (r *taskRun) finish() { + r.once.Do(func() { + r.cancel() + close(r.done) + }) +} + // New validates options and returns a Swarm. func New(opts Options) (*Swarm, error) { if opts.Launcher == nil { @@ -145,6 +179,7 @@ func New(opts Options) (*Swarm, error) { cancel: cancel, teams: map[string]*Team{}, taskCwd: map[string]string{}, + taskRuns: map[string]*taskRun{}, }, nil } @@ -187,6 +222,7 @@ func (s *Swarm) Close() { for _, team := range teams { for _, spec := range team.clearQueue() { _ = s.coord.Fail(spec.TaskID, ErrSwarmClosed.Error()) + s.finishTaskRun(spec.TaskID) } } // Every watcher is added while its admitting lifecycle ticket is still held, @@ -250,6 +286,42 @@ func (s *Swarm) cwdFor(taskID string) string { return s.taskCwd[taskID] } +// startTaskRun installs a fresh execution boundary for taskID. Freshly spawned +// tasks use unique ids; orphan adoption intentionally replaces the completed +// boundary for the task it is reviving under a new member. +func (s *Swarm) startTaskRun(taskID string) *taskRun { + run := newTaskRun(s.baseCtx) + s.mu.Lock() + s.taskRuns[taskID] = run + s.mu.Unlock() + return run +} + +// ensureTaskRun returns the task's execution boundary, creating one for +// internal/adopted paths that registered a coordinator task directly. +func (s *Swarm) ensureTaskRun(taskID string) *taskRun { + s.mu.Lock() + defer s.mu.Unlock() + if run := s.taskRuns[taskID]; run != nil { + return run + } + run := newTaskRun(s.baseCtx) + s.taskRuns[taskID] = run + return run +} + +func (s *Swarm) taskRun(taskID string) *taskRun { + s.mu.Lock() + defer s.mu.Unlock() + return s.taskRuns[taskID] +} + +func (s *Swarm) finishTaskRun(taskID string) { + if run := s.taskRun(taskID); run != nil { + run.finish() + } +} + // Registry exposes the roster (for tool listing / user-defined registration). func (s *Swarm) Registry() *Registry { return s.registry } @@ -378,6 +450,23 @@ func (t *Team) clearQueue() []MemberSpec { return queued } +// removeQueuedTask cancels a handoff source that has not taken a running slot +// yet. It closes the queue/dequeue race under the same lock used by onExit. +func (t *Team) removeQueuedTask(taskID string) bool { + t.mu.Lock() + defer t.mu.Unlock() + for i := range t.queue { + if t.queue[i].TaskID != taskID { + continue + } + copy(t.queue[i:], t.queue[i+1:]) + t.queue[len(t.queue)-1] = MemberSpec{} + t.queue = t.queue[:len(t.queue)-1] + return true + } + return false +} + func (t *Team) addMember(m *Member) { t.mu.Lock() t.members[m.ID] = m diff --git a/internal/swarm/tools.go b/internal/swarm/tools.go index 496a78b8b..8172bc580 100644 --- a/internal/swarm/tools.go +++ b/internal/swarm/tools.go @@ -365,7 +365,7 @@ type handoffTool struct { func (t *handoffTool) Name() string { return HandoffToolName } func (t *handoffTool) Description() string { - return "Hand a task off to a fresh member of another agent type, delivering an optional note to the new member's inbox." + return "Stop a task's current member, then hand it off to a fresh member of another agent type, delivering an optional note to the new member's inbox." } func (t *handoffTool) Parameters() tools.Schema { return tools.Schema{ @@ -384,7 +384,7 @@ func (t *handoffTool) Safety() tools.Safety { return tools.Safety{ SideEffect: tools.SideEffectShell, Permission: tools.PermissionPrompt, - Reason: "Spawns a replacement swarm member to take over a task, and writes the handoff note to the new member's inbox.", + Reason: "Stops the current swarm member, spawns its replacement, and writes the handoff note to the new member's inbox.", AdvertiseInAuto: true, } } From c3a1b27d933625c57da572501b767b3d447ee2a9 Mon Sep 17 00:00:00 2001 From: KRATOS <84986124+gnanam1990@users.noreply.github.com> Date: Thu, 27 Aug 2026 16:04:21 +0530 Subject: [PATCH 2/3] fix(swarm): make handoff recovery fail closed --- internal/swarm/coordinator.go | 118 +++++++++++-- internal/swarm/coordinator_test.go | 51 ++++++ internal/swarm/lifecycle.go | 67 ++++++-- internal/swarm/lifecycle_test.go | 256 ++++++++++++++++++++++++++++- internal/swarm/team.go | 42 +++-- 5 files changed, 498 insertions(+), 36 deletions(-) diff --git a/internal/swarm/coordinator.go b/internal/swarm/coordinator.go index 4e550cbaf..2f7146332 100644 --- a/internal/swarm/coordinator.go +++ b/internal/swarm/coordinator.go @@ -67,11 +67,12 @@ var agentColors = []string{"cyan", "magenta", "green", "yellow", "blue", "red"} // Coordinator is the in-memory task registry + team color assigner shared by an // orchestrator and its members. It is safe for concurrent use. type Coordinator struct { - mu sync.RWMutex - tasks map[string]*Task - colors map[string]string // agentID -> color - colorIndex int - now func() time.Time // injectable clock for tests + mu sync.RWMutex + tasks map[string]*Task + handoffReservations map[string]string // successor task id -> claimed source task id + colors map[string]string // agentID -> color + colorIndex int + now func() time.Time // injectable clock for tests // changed is closed (and replaced) on every task state change so WaitSettled // can block for a transition without polling. Always non-nil after construction. changed chan struct{} @@ -80,10 +81,11 @@ type Coordinator struct { // NewCoordinator returns an empty coordinator using the wall clock. func NewCoordinator() *Coordinator { return &Coordinator{ - tasks: map[string]*Task{}, - colors: map[string]string{}, - now: time.Now, - changed: make(chan struct{}), + tasks: map[string]*Task{}, + handoffReservations: map[string]string{}, + colors: map[string]string{}, + now: time.Now, + changed: make(chan struct{}), } } @@ -113,6 +115,9 @@ func (c *Coordinator) Register(id, agentID, team, description string) (Task, err if _, ok := c.tasks[id]; ok { return Task{}, fmt.Errorf("%w: %s", ErrTaskExists, id) } + if _, reserved := c.handoffReservations[id]; reserved { + return Task{}, fmt.Errorf("%w: %s is reserved for handoff", ErrTaskExists, id) + } now := c.now() t := &Task{ ID: id, @@ -175,14 +180,49 @@ func (c *Coordinator) BeginHandoff(id string) (Task, error) { return *t, nil } -// AbortHandoff releases a claim before the source member is cancelled. It is -// used when preparing the successor (for example, delivering its note) fails. +// AbortHandoff releases a claim and any successor reservation when handoff +// preparation fails or a stopped source does not reach its completion barrier. func (c *Coordinator) AbortHandoff(id string) { c.mu.Lock() defer c.mu.Unlock() if t, ok := c.tasks[id]; ok && t.handoff && !t.Status.terminal() { t.handoff = false } + for successorID, sourceID := range c.handoffReservations { + if sourceID == id { + delete(c.handoffReservations, successorID) + } + } +} + +// ReserveHandoffSuccessor claims successorID for sourceID before any mailbox or +// cancellation side effect. Register rejects reserved IDs, so another Swarm +// sharing this Coordinator cannot steal the id between note delivery and the +// atomic handoff commit. +func (c *Coordinator) ReserveHandoffSuccessor(sourceID, successorID string) error { + if successorID == "" { + return errors.New("swarm: successor task id is required") + } + c.mu.Lock() + defer c.mu.Unlock() + source, ok := c.tasks[sourceID] + if !ok { + return fmt.Errorf("%w: %s", ErrUnknownTask, sourceID) + } + if !source.handoff || source.Status.terminal() { + return fmt.Errorf("swarm: task %s has no active handoff claim", sourceID) + } + if _, exists := c.tasks[successorID]; exists { + return fmt.Errorf("%w: %s", ErrTaskExists, successorID) + } + if _, reserved := c.handoffReservations[successorID]; reserved { + return fmt.Errorf("%w: %s is reserved for handoff", ErrTaskExists, successorID) + } + if c.handoffReservations == nil { + c.handoffReservations = map[string]string{} + } + c.handoffReservations[successorID] = sourceID + return nil } // FinishHandoff publishes the terminal state only after the source execution @@ -200,6 +240,11 @@ func (c *Coordinator) FinishHandoff(id string) error { if t.Status.terminal() { return fmt.Errorf("swarm: task %s already %s", id, t.Status) } + for _, sourceID := range c.handoffReservations { + if sourceID == id { + return fmt.Errorf("swarm: task %s has a reserved successor; commit the handoff atomically", id) + } + } t.handoff = false t.Status = StatusHandedOff t.UpdatedAt = c.now() @@ -207,6 +252,57 @@ func (c *Coordinator) FinishHandoff(id string) error { return nil } +// CommitHandoff atomically registers the successor and publishes the source's +// handed-off terminal state. The successor is inserted first while c.mu keeps +// the intermediate state invisible, so an ID collision cannot retire the +// source and orphan adoption cannot observe a successor without its source +// transition committed. +func (c *Coordinator) CommitHandoff(sourceID, successorID, successorAgentID, team, description string) (Task, error) { + if successorID == "" { + return Task{}, errors.New("swarm: successor task id is required") + } + c.mu.Lock() + defer c.mu.Unlock() + + source, ok := c.tasks[sourceID] + if !ok { + return Task{}, fmt.Errorf("%w: %s", ErrUnknownTask, sourceID) + } + if !source.handoff { + return Task{}, fmt.Errorf("swarm: task %s has no handoff in progress", sourceID) + } + if source.Status.terminal() { + return Task{}, fmt.Errorf("swarm: task %s already %s", sourceID, source.Status) + } + if reservedFor, ok := c.handoffReservations[successorID]; !ok || reservedFor != sourceID { + return Task{}, fmt.Errorf("swarm: successor task %s is not reserved for handoff from %s", successorID, sourceID) + } + if _, exists := c.tasks[successorID]; exists { + return Task{}, fmt.Errorf("%w: %s", ErrTaskExists, successorID) + } + + now := c.now() + successor := &Task{ + ID: successorID, + AgentID: successorAgentID, + Team: team, + Description: description, + Status: StatusPending, + CreatedAt: now, + UpdatedAt: now, + } + // Registration precedes retirement under the same lock. No reader can see + // either half until both ownership records are valid. + c.tasks[successorID] = successor + delete(c.handoffReservations, successorID) + c.assignColorLocked(successorAgentID) + source.handoff = false + source.Status = StatusHandedOff + source.UpdatedAt = now + c.notifyChangeLocked() + return *successor, nil +} + // Complete marks a task done with its result. func (c *Coordinator) Complete(id, result string) error { return c.finish(id, StatusDone, result, "", "") diff --git a/internal/swarm/coordinator_test.go b/internal/swarm/coordinator_test.go index 1a8f61103..a0d392754 100644 --- a/internal/swarm/coordinator_test.go +++ b/internal/swarm/coordinator_test.go @@ -142,6 +142,57 @@ func TestCoordinatorAbortHandoffRestoresNormalCompletion(t *testing.T) { } } +func TestCoordinatorHandoffReservationBlocksRegistrationUntilAbort(t *testing.T) { + c := NewCoordinator() + _, _ = c.Register("source", "a1", "team", "desc") + if _, err := c.BeginHandoff("source"); err != nil { + t.Fatalf("BeginHandoff: %v", err) + } + if err := c.ReserveHandoffSuccessor("source", "successor"); err != nil { + t.Fatalf("ReserveHandoffSuccessor: %v", err) + } + if _, err := c.Register("successor", "other", "team", "collision"); !errors.Is(err, ErrTaskExists) { + t.Fatalf("Register reserved id error = %v, want ErrTaskExists", err) + } + if err := c.FinishHandoff("source"); err == nil { + t.Fatal("FinishHandoff must not bypass a reserved successor") + } + + c.AbortHandoff("source") + if _, err := c.Register("successor", "other", "team", "available again"); err != nil { + t.Fatalf("reservation was not released by AbortHandoff: %v", err) + } + if err := c.Complete("source", "source continued"); err != nil { + t.Fatalf("source claim was not released by AbortHandoff: %v", err) + } +} + +func TestCoordinatorCommitHandoffPublishesBothSides(t *testing.T) { + c := NewCoordinator() + _, _ = c.Register("source", "a1", "team", "desc") + _ = c.SetStatus("source", StatusRunning) + if _, err := c.BeginHandoff("source"); err != nil { + t.Fatalf("BeginHandoff: %v", err) + } + if err := c.ReserveHandoffSuccessor("source", "successor"); err != nil { + t.Fatalf("ReserveHandoffSuccessor: %v", err) + } + successor, err := c.CommitHandoff("source", "successor", "a2", "team", "continued") + if err != nil { + t.Fatalf("CommitHandoff: %v", err) + } + if successor.Status != StatusPending || successor.AgentID != "a2" { + t.Fatalf("successor = %+v, want pending ownership by a2", successor) + } + source, _ := c.Get("source") + if source.Status != StatusHandedOff { + t.Fatalf("source status = %s, want handed-off", source.Status) + } + if _, err := c.Register("successor", "other", "team", "duplicate"); !errors.Is(err, ErrTaskExists) { + t.Fatalf("committed successor was not registered: %v", err) + } +} + func TestCoordinatorColorStability(t *testing.T) { c := NewCoordinator() first := c.Color("a1") diff --git a/internal/swarm/lifecycle.go b/internal/swarm/lifecycle.go index 4d76730d3..10335de49 100644 --- a/internal/swarm/lifecycle.go +++ b/internal/swarm/lifecycle.go @@ -3,6 +3,7 @@ package swarm import ( "context" "fmt" + "time" ) // Spawn registers a task and launches a member of agentType to run it under the @@ -283,7 +284,16 @@ func (s *Swarm) Handoff(pol Policy, teamName, taskID, toAgentType, note string) if err != nil { return "", err } + run := s.taskRun(taskID) + if run == nil { + s.coord.AbortHandoff(taskID) + return "", fmt.Errorf("swarm: task %s has no local execution boundary; refusing handoff", taskID) + } newID := s.nextID(toAgentType) + if err := s.coord.ReserveHandoffSuccessor(taskID, newID); err != nil { + s.coord.AbortHandoff(taskID) + return "", fmt.Errorf("swarm: reserve handoff successor: %w", err) + } handoffTask := task.Description if note != "" { handoffTask += "\n\nHandoff note: " + note @@ -303,19 +313,27 @@ func (s *Swarm) Handoff(pol Policy, teamName, taskID, toAgentType, note string) // Stop a running member, remove a queued one, or cancel a launch currently // between slot reservation and handle adoption. The completion barrier closes // only after no source member can execute further side effects. - run := s.taskRun(taskID) - if run != nil { - run.stop() - if s.team(team).removeQueuedTask(taskID) { - run.finish() - } - <-run.done + run.stop() + if s.team(team).removeQueuedTask(taskID) { + run.finish() } - if err := s.coord.FinishHandoff(taskID); err != nil { + if err := s.waitForHandoffSource(taskID, run); err != nil { + s.coord.AbortHandoff(taskID) + // If the execution barrier raced the cancellation/timeout, its watcher may + // have tried to finish while the claim was still held. Fail the stopped + // source rather than leave a permanently-running coordinator record. + if run.finished() { + _ = s.coord.Fail(taskID, "handoff aborted after source stopped: "+err.Error()) + } return "", err } - if _, err := s.coord.Register(newID, newID, team, handoffTask); err != nil { - return "", err + if _, err := s.coord.CommitHandoff(taskID, newID, newID, team, handoffTask); err != nil { + s.coord.AbortHandoff(taskID) + failErr := s.coord.Fail(taskID, "handoff successor registration failed after source stopped: "+err.Error()) + if failErr != nil { + return "", fmt.Errorf("swarm: register handoff successor: %w (source recovery failed: %v)", err, failErr) + } + return "", fmt.Errorf("swarm: register handoff successor: %w", err) } s.rememberCwd(newID, cwd) s.startTaskRun(newID) @@ -324,6 +342,35 @@ func (s *Swarm) Handoff(pol Policy, teamName, taskID, toAgentType, note string) return newID, nil } +// waitForHandoffSource waits for the task-specific execution barrier without +// letting a cancellation-insensitive member wedge a model tool forever. Close +// cancels baseCtx before waiting for lifecycle admissions, so that path also +// releases Handoff's admission ticket and avoids a shutdown wait cycle. +func (s *Swarm) waitForHandoffSource(taskID string, run *taskRun) error { + timeout := s.handoffStopTimeout + if timeout <= 0 { + timeout = defaultHandoffStopTimeout + } + timer := time.NewTimer(timeout) + defer timer.Stop() + select { + case <-run.done: + return nil + case <-s.baseCtx.Done(): + // Prefer a completed barrier if cancellation and member exit became ready + // together; a stopped source can still commit safely. + if run.finished() { + return nil + } + return fmt.Errorf("swarm: stop handoff source %s: %w", taskID, s.baseCtx.Err()) + case <-timer.C: + if run.finished() { + return nil + } + return fmt.Errorf("%w: task %s after %s", ErrHandoffStopTimeout, taskID, timeout) + } +} + // AdoptOrphans re-parents tasks in a team whose owning member is no longer live // (e.g. a crashed worker) onto fresh members of toAgentType, returning the // adopted task ids. Terminal tasks and tasks with a live owner are left alone. diff --git a/internal/swarm/lifecycle_test.go b/internal/swarm/lifecycle_test.go index 198c650bf..2150ba1c3 100644 --- a/internal/swarm/lifecycle_test.go +++ b/internal/swarm/lifecycle_test.go @@ -72,8 +72,12 @@ func (l *controllableLauncher) attemptCount(id string) int { } func newSwarmFor(t *testing.T, l MemberLauncher) *Swarm { + return newSwarmForWithSize(t, l, 2) +} + +func newSwarmForWithSize(t *testing.T, l MemberLauncher, maxTeamSize int) *Swarm { t.Helper() - sw, err := New(Options{BaseDir: t.TempDir(), Launcher: l, MaxTeamSize: 2}) + sw, err := New(Options{BaseDir: t.TempDir(), Launcher: l, MaxTeamSize: maxTeamSize}) if err != nil { t.Fatalf("New: %v", err) } @@ -456,6 +460,247 @@ func TestHandoffStopsOriginalBeforeSuccessorStarts(t *testing.T) { } } +func TestHandoffMailboxFailureRestoresSourceCompletion(t *testing.T) { + gate := make(chan struct{}) + l := newLauncher(okFor) + l.gate = gate + sw := newSwarmFor(t, l) + pol := Policy{Model: "m"} + origID, err := sw.Spawn(pol, "team", "teammate", "original task", "/w") + if err != nil { + t.Fatalf("Spawn: %v", err) + } + waitFor(t, "original running", func() bool { + task, ok := sw.Coordinator().Get(origID) + return ok && task.Status == StatusRunning + }) + + // The successor id is deterministic: the original consumed sequence 1. + sw.Mailbox().MaxMessages = 1 + if err := sw.Mailbox().Send("team", "subagent-2", Message{From: "test", Body: "occupy inbox"}); err != nil { + t.Fatalf("prefill successor inbox: %v", err) + } + if _, err := sw.Handoff(pol, "team", origID, "subagent", "continue safely"); !errors.Is(err, ErrMailboxFull) { + t.Fatalf("Handoff error = %v, want ErrMailboxFull", err) + } + + close(gate) + waitFor(t, "source completion after aborted handoff", func() bool { + task, ok := sw.Coordinator().Get(origID) + return ok && task.Status == StatusDone + }) +} + +func TestHandoffFailsClosedWithoutLocalTaskRun(t *testing.T) { + coord := NewCoordinator() + if _, err := coord.Register("external-task", "external-agent", "team", "external work"); err != nil { + t.Fatalf("Register: %v", err) + } + if err := coord.SetStatus("external-task", StatusRunning); err != nil { + t.Fatalf("SetStatus: %v", err) + } + l := newLauncher(okFor) + sw, err := New(Options{BaseDir: t.TempDir(), Launcher: l, Coordinator: coord}) + if err != nil { + t.Fatalf("New: %v", err) + } + t.Cleanup(sw.Close) + + if _, err := sw.Handoff(Policy{Model: "m"}, "team", "external-task", "subagent", ""); err == nil { + t.Fatal("Handoff without a local task run must fail") + } + if err := coord.Complete("external-task", "external owner finished"); err != nil { + t.Fatalf("handoff claim was not aborted: %v", err) + } + if got := len(l.recorded()); got != 0 { + t.Fatalf("unexpected successor launches = %d", got) + } +} + +func TestHandoffSuccessorCollisionDoesNotStopSource(t *testing.T) { + gate := make(chan struct{}) + l := newLauncher(okFor) + l.gate = gate + coord := NewCoordinator() + sw, err := New(Options{BaseDir: t.TempDir(), Launcher: l, Coordinator: coord, MaxTeamSize: 2}) + if err != nil { + t.Fatalf("New: %v", err) + } + t.Cleanup(sw.Close) + pol := Policy{Model: "m"} + origID, err := sw.Spawn(pol, "team", "teammate", "original task", "/w") + if err != nil { + t.Fatalf("Spawn: %v", err) + } + waitFor(t, "original running", func() bool { + task, ok := coord.Get(origID) + return ok && task.Status == StatusRunning + }) + // Handoff will mint subagent-2. A shared coordinator can already contain it + // because each Swarm has an independent id sequence. + if _, err := coord.Register("subagent-2", "other-agent", "other-team", "existing task"); err != nil { + t.Fatalf("register collision: %v", err) + } + + if _, err := sw.Handoff(pol, "team", origID, "subagent", "do not misdeliver"); !errors.Is(err, ErrTaskExists) { + t.Fatalf("Handoff error = %v, want ErrTaskExists", err) + } + task, _ := coord.Get(origID) + if task.Status != StatusRunning { + t.Fatalf("source status = %s, want running because collision was detected before stop", task.Status) + } + msgs, err := sw.Mailbox().ReadAndConsume("team", "subagent-2") + if err != nil { + t.Fatalf("read colliding inbox: %v", err) + } + if len(msgs) != 0 { + t.Fatalf("collision misdelivered %d handoff notes to the existing task", len(msgs)) + } + close(gate) + waitFor(t, "source completion after rejected collision", func() bool { + task, ok := coord.Get(origID) + return ok && task.Status == StatusDone + }) +} + +func TestCloseReleasesBlockedHandoffBeforeMemberExit(t *testing.T) { + releaseOriginal := make(chan struct{}) + originalStarted := make(chan struct{}) + successorStarted := make(chan struct{}, 1) + launcher := FuncLauncher{Run: func(_ context.Context, spec MemberSpec) (MemberResult, error) { + if spec.AgentType == "teammate" { + close(originalStarted) + <-releaseOriginal + return MemberResult{Result: "source finished"}, nil + } + successorStarted <- struct{}{} + return MemberResult{Result: "successor finished"}, nil + }} + sw, err := New(Options{BaseDir: t.TempDir(), Launcher: launcher}) + if err != nil { + t.Fatalf("New: %v", err) + } + t.Cleanup(sw.Close) + defer func() { + select { + case <-releaseOriginal: + default: + close(releaseOriginal) + } + }() + origID, err := sw.Spawn(Policy{Model: "m"}, "team", "teammate", "original task", "/w") + if err != nil { + t.Fatalf("Spawn: %v", err) + } + select { + case <-originalStarted: + case <-time.After(3 * time.Second): + t.Fatal("original member never started") + } + + handoffDone := make(chan error, 1) + go func() { + _, err := sw.Handoff(Policy{Model: "m"}, "team", origID, "subagent", "") + handoffDone <- err + }() + time.Sleep(50 * time.Millisecond) + closeDone := make(chan struct{}) + go func() { + sw.Close() + close(closeDone) + }() + select { + case err := <-handoffDone: + if !errors.Is(err, context.Canceled) { + t.Fatalf("Handoff error = %v, want Close cancellation", err) + } + case <-time.After(500 * time.Millisecond): + t.Fatal("Handoff stayed blocked after Close canceled the swarm") + } + select { + case <-successorStarted: + t.Fatal("successor started while the source member was still running") + default: + } + select { + case <-closeDone: + t.Fatal("Close returned before the cancellation-insensitive member exited") + default: + } + + close(releaseOriginal) + select { + case <-closeDone: + case <-time.After(3 * time.Second): + t.Fatal("Close did not return after the source member exited") + } + waitFor(t, "source completion after handoff cancellation", func() bool { + task, ok := sw.Coordinator().Get(origID) + return ok && task.Status == StatusDone + }) +} + +func TestHandoffTimesOutWithoutStartingSuccessor(t *testing.T) { + releaseOriginal := make(chan struct{}) + originalStarted := make(chan struct{}) + successorStarted := make(chan struct{}, 1) + launcher := FuncLauncher{Run: func(_ context.Context, spec MemberSpec) (MemberResult, error) { + if spec.AgentType == "teammate" { + close(originalStarted) + <-releaseOriginal + return MemberResult{Result: "source finished"}, nil + } + successorStarted <- struct{}{} + return MemberResult{Result: "successor finished"}, nil + }} + sw := newSwarmFor(t, launcher) + sw.handoffStopTimeout = 50 * time.Millisecond + defer func() { + select { + case <-releaseOriginal: + default: + close(releaseOriginal) + } + }() + origID, err := sw.Spawn(Policy{Model: "m"}, "team", "teammate", "original task", "/w") + if err != nil { + t.Fatalf("Spawn: %v", err) + } + select { + case <-originalStarted: + case <-time.After(3 * time.Second): + t.Fatal("original member never started") + } + + handoffDone := make(chan error, 1) + go func() { + _, err := sw.Handoff(Policy{Model: "m"}, "team", origID, "subagent", "") + handoffDone <- err + }() + select { + case err := <-handoffDone: + if !errors.Is(err, ErrHandoffStopTimeout) { + t.Fatalf("Handoff error = %v, want ErrHandoffStopTimeout", err) + } + case <-time.After(500 * time.Millisecond): + t.Fatal("Handoff exceeded its source-stop deadline") + } + select { + case <-successorStarted: + t.Fatal("successor started after the source-stop deadline expired") + default: + } + if task, ok := sw.Coordinator().Get(origID); !ok || task.Status != StatusRunning { + t.Fatalf("source status after timeout = %+v, want running until its member exits", task) + } + + close(releaseOriginal) + waitFor(t, "source completion after handoff timeout", func() bool { + task, ok := sw.Coordinator().Get(origID) + return ok && task.Status == StatusDone + }) +} + func TestHandoffWaitsForOriginalThatIgnoresCancellation(t *testing.T) { originalStarted := make(chan struct{}) releaseOriginal := make(chan struct{}) @@ -539,8 +784,7 @@ func TestHandoffDoesNotWaitForUnrelatedQueuedLaunch(t *testing.T) { queuedLaunchStarted: queuedLaunchStarted, releaseQueuedLaunch: releaseQueuedLaunch, } - sw := newSwarmFor(t, launcher) - sw.maxTeamSize = 1 + sw := newSwarmForWithSize(t, launcher, 1) pol := Policy{Model: "m"} origID, err := sw.Spawn(pol, "team", "teammate", "original task", "/w") if err != nil { @@ -554,6 +798,9 @@ func TestHandoffDoesNotWaitForUnrelatedQueuedLaunch(t *testing.T) { if _, err := sw.Spawn(pol, "team", "teammate", "queued task", "/w"); err != nil { t.Fatalf("Spawn queued: %v", err) } + if got := sw.team("team").QueueDepth(); got != 1 { + t.Fatalf("queue depth = %d, want one queued task before handoff", got) + } handoffDone := make(chan error, 1) go func() { @@ -614,8 +861,7 @@ func TestHandoffRemovesQueuedOriginalBeforeDispatchingSuccessor(t *testing.T) { }() l := newLauncher(okFor) l.gate = gate - sw := newSwarmFor(t, l) - sw.maxTeamSize = 1 + sw := newSwarmForWithSize(t, l, 1) pol := Policy{Model: "m"} if _, err := sw.Spawn(pol, "team", "teammate", "slot blocker", "/w"); err != nil { diff --git a/internal/swarm/team.go b/internal/swarm/team.go index 5282bd11b..447a2e6f0 100644 --- a/internal/swarm/team.go +++ b/internal/swarm/team.go @@ -16,6 +16,11 @@ import ( // on-demand worker model). const defaultMaxTeamSize = 8 +// defaultHandoffStopTimeout bounds the model-facing handoff tool while it waits +// for a cancellation-insensitive source member. Timing out never starts the +// successor, so the single-owner guarantee remains fail closed. +const defaultHandoffStopTimeout = 30 * time.Second + // maxMemberRestarts bounds automatic relaunches of a member that exits with a // temporary error, mirroring daemon.Pool's bounded backoff retries. const maxMemberRestarts = 2 @@ -60,6 +65,9 @@ type Swarm struct { mailbox *Mailbox launcher MemberLauncher maxTeamSize int + // handoffStopTimeout is fixed to the default in production and kept on the + // instance so lifecycle tests can exercise the deadline without a global race. + handoffStopTimeout time.Duration baseCtx context.Context cancel context.CancelFunc @@ -138,6 +146,15 @@ func (r *taskRun) finish() { }) } +func (r *taskRun) finished() bool { + select { + case <-r.done: + return true + default: + return false + } +} + // New validates options and returns a Swarm. func New(opts Options) (*Swarm, error) { if opts.Launcher == nil { @@ -170,16 +187,17 @@ func New(opts Options) (*Swarm, error) { } ctx, cancel := context.WithCancel(parent) return &Swarm{ - registry: registry, - coord: coord, - mailbox: mb, - launcher: opts.Launcher, - maxTeamSize: maxTeam, - baseCtx: ctx, - cancel: cancel, - teams: map[string]*Team{}, - taskCwd: map[string]string{}, - taskRuns: map[string]*taskRun{}, + registry: registry, + coord: coord, + mailbox: mb, + launcher: opts.Launcher, + maxTeamSize: maxTeam, + handoffStopTimeout: defaultHandoffStopTimeout, + baseCtx: ctx, + cancel: cancel, + teams: map[string]*Team{}, + taskCwd: map[string]string{}, + taskRuns: map[string]*taskRun{}, }, nil } @@ -272,6 +290,10 @@ func (s *Swarm) Scheduler() *Scheduler { // ErrSwarmClosed reports lifecycle work submitted after shutdown begins. var ErrSwarmClosed = errors.New("swarm: closed") +// ErrHandoffStopTimeout means the source member ignored cancellation long +// enough that the handoff declined to start a successor. +var ErrHandoffStopTimeout = errors.New("swarm: handoff source did not stop before timeout") + // rememberCwd records a task's working dir so a handoff/adoption relaunch keeps it. func (s *Swarm) rememberCwd(taskID, cwd string) { s.mu.Lock() From 897a18bccd4688aba959fa824db1f4b8a91a204a Mon Sep 17 00:00:00 2001 From: KRATOS <84986124+gnanam1990@users.noreply.github.com> Date: Thu, 27 Aug 2026 16:15:40 +0530 Subject: [PATCH 3/3] test(swarm): remove handoff timing assumptions --- internal/swarm/coordinator.go | 29 +---------------------------- internal/swarm/coordinator_test.go | 13 ++----------- internal/swarm/lifecycle_test.go | 13 +++++++++++-- 3 files changed, 14 insertions(+), 41 deletions(-) diff --git a/internal/swarm/coordinator.go b/internal/swarm/coordinator.go index 2f7146332..aad2ffe0a 100644 --- a/internal/swarm/coordinator.go +++ b/internal/swarm/coordinator.go @@ -161,7 +161,7 @@ func (c *Coordinator) SetStatus(id string, status TaskStatus) error { // BeginHandoff atomically claims a non-terminal task for one handoff caller // without reporting it terminal. The visible status remains pending/running -// until FinishHandoff, so collectors cannot observe settled state while the +// until CommitHandoff, so collectors cannot observe settled state while the // source member is still unwinding. func (c *Coordinator) BeginHandoff(id string) (Task, error) { c.mu.Lock() @@ -225,33 +225,6 @@ func (c *Coordinator) ReserveHandoffSuccessor(sourceID, successorID string) erro return nil } -// FinishHandoff publishes the terminal state only after the source execution -// barrier has closed. -func (c *Coordinator) FinishHandoff(id string) error { - c.mu.Lock() - defer c.mu.Unlock() - t, ok := c.tasks[id] - if !ok { - return fmt.Errorf("%w: %s", ErrUnknownTask, id) - } - if !t.handoff { - return fmt.Errorf("swarm: task %s has no handoff in progress", id) - } - if t.Status.terminal() { - return fmt.Errorf("swarm: task %s already %s", id, t.Status) - } - for _, sourceID := range c.handoffReservations { - if sourceID == id { - return fmt.Errorf("swarm: task %s has a reserved successor; commit the handoff atomically", id) - } - } - t.handoff = false - t.Status = StatusHandedOff - t.UpdatedAt = c.now() - c.notifyChangeLocked() - return nil -} - // CommitHandoff atomically registers the successor and publishes the source's // handed-off terminal state. The successor is inserted first while c.mu keeps // the intermediate state invisible, so an ID collision cannot retire the diff --git a/internal/swarm/coordinator_test.go b/internal/swarm/coordinator_test.go index a0d392754..0e098135d 100644 --- a/internal/swarm/coordinator_test.go +++ b/internal/swarm/coordinator_test.go @@ -103,7 +103,7 @@ func TestCoordinatorReassign(t *testing.T) { } } -func TestCoordinatorHandoffClaimKeepsTaskNonTerminalUntilFinished(t *testing.T) { +func TestCoordinatorHandoffClaimBlocksCompetingTransitions(t *testing.T) { c := NewCoordinator() _, _ = c.Register("t1", "a1", "team", "desc") _ = c.SetStatus("t1", StatusRunning) @@ -122,12 +122,7 @@ func TestCoordinatorHandoffClaimKeepsTaskNonTerminalUntilFinished(t *testing.T) if err := c.Reassign("t1", "a2"); err == nil { t.Fatal("orphan adoption must not race a claimed handoff") } - if err := c.FinishHandoff("t1"); err != nil { - t.Fatalf("FinishHandoff: %v", err) - } - if task, _ := c.Get("t1"); task.Status != StatusHandedOff { - t.Fatalf("status after FinishHandoff = %v, want handed-off", task.Status) - } + c.AbortHandoff("t1") } func TestCoordinatorAbortHandoffRestoresNormalCompletion(t *testing.T) { @@ -154,10 +149,6 @@ func TestCoordinatorHandoffReservationBlocksRegistrationUntilAbort(t *testing.T) if _, err := c.Register("successor", "other", "team", "collision"); !errors.Is(err, ErrTaskExists) { t.Fatalf("Register reserved id error = %v, want ErrTaskExists", err) } - if err := c.FinishHandoff("source"); err == nil { - t.Fatal("FinishHandoff must not bypass a reserved successor") - } - c.AbortHandoff("source") if _, err := c.Register("successor", "other", "team", "available again"); err != nil { t.Fatalf("reservation was not released by AbortHandoff: %v", err) diff --git a/internal/swarm/lifecycle_test.go b/internal/swarm/lifecycle_test.go index 2150ba1c3..a1b0ebcb8 100644 --- a/internal/swarm/lifecycle_test.go +++ b/internal/swarm/lifecycle_test.go @@ -566,10 +566,15 @@ func TestHandoffSuccessorCollisionDoesNotStopSource(t *testing.T) { func TestCloseReleasesBlockedHandoffBeforeMemberExit(t *testing.T) { releaseOriginal := make(chan struct{}) originalStarted := make(chan struct{}) + originalCanceled := make(chan struct{}) successorStarted := make(chan struct{}, 1) - launcher := FuncLauncher{Run: func(_ context.Context, spec MemberSpec) (MemberResult, error) { + launcher := FuncLauncher{Run: func(ctx context.Context, spec MemberSpec) (MemberResult, error) { if spec.AgentType == "teammate" { close(originalStarted) + go func() { + <-ctx.Done() + close(originalCanceled) + }() <-releaseOriginal return MemberResult{Result: "source finished"}, nil } @@ -603,7 +608,11 @@ func TestCloseReleasesBlockedHandoffBeforeMemberExit(t *testing.T) { _, err := sw.Handoff(Policy{Model: "m"}, "team", origID, "subagent", "") handoffDone <- err }() - time.Sleep(50 * time.Millisecond) + select { + case <-originalCanceled: + case <-time.After(3 * time.Second): + t.Fatal("Handoff never canceled the source run") + } closeDone := make(chan struct{}) go func() { sw.Close()