diff --git a/docs/changes/unreleased/1539-chat-late-task-start.md b/docs/changes/unreleased/1539-chat-late-task-start.md new file mode 100644 index 000000000..904848fb3 --- /dev/null +++ b/docs/changes/unreleased/1539-chat-late-task-start.md @@ -0,0 +1,8 @@ +--- +kind: fixed +title: a late task start is an uncertain receipt +pr: 1539 +surface: [chat, remote] +invalidates: + - "A task start that did not answer within the connection's wait was reported as a refusal. It is now an uncertain receipt; the task may already be running, stays visible through its task updates, and is not retried automatically." +--- diff --git a/internal/manual/chat/tasks.md b/internal/manual/chat/tasks.md index 19f753a8e..e411f7c11 100644 --- a/internal/manual/chat/tasks.md +++ b/internal/manual/chat/tasks.md @@ -1,5 +1,12 @@ # Work that runs on its own — tasks, and finding out what one actually did +## The engine is slow to answer when starting a task + +If the task start takes longer than the connection's wait, the chat says +`the engine has not confirmed the start — the task may already be running`. This is an +uncertain receipt, not a refusal. The task can appear in the task rail when the +engine reports it. Check the running work before starting the same brief again. + ## What a task is A task is one self-contained piece of work handed off to run on its own while the diff --git a/internal/remote/client.go b/internal/remote/client.go index 76607ef0b..4d5d5314e 100644 --- a/internal/remote/client.go +++ b/internal/remote/client.go @@ -1548,7 +1548,7 @@ func (a *Agent) Cancel(id string) (string, error) { // brief is written beside the work (internal/session's task_shape.go), so there // is nothing on the far side worth a longer wait. func (a *Agent) StartTask(ctx context.Context, brief string, solo bool) (uint64, string, string, error) { - payload, err := a.c.call(ctx, MethodTaskStart, TaskStartArgs{Brief: brief, Solo: solo}) + payload, err := a.callTaskStart(ctx, MethodTaskStart, TaskStartArgs{Brief: brief, Solo: solo}) if err != nil { return 0, "", "", err } @@ -1579,7 +1579,7 @@ func (a *Agent) Delegates() session.DelegateReport { // and returns the same receipt StartTask does. It is an ordinary call with the // ordinary deadline: the engine admits the run at once. func (a *Agent) StartDelegate(ctx context.Context, name, brief string) (uint64, string, string, error) { - payload, err := a.c.call(ctx, MethodDelegateStart, DelegateStartArgs{Name: name, Brief: brief}) + payload, err := a.callTaskStart(ctx, MethodDelegateStart, DelegateStartArgs{Name: name, Brief: brief}) if err != nil { return 0, "", "", err } @@ -1594,7 +1594,7 @@ func (a *Agent) StartDelegate(ctx context.Context, name, brief string) (uint64, // (`/task --best`, `/task --cheap`); the engine's router reads it for this task // and nothing after it. func (a *Agent) StartTaskEffort(ctx context.Context, brief string, solo bool, effort string) (uint64, string, string, error) { - payload, err := a.c.call(ctx, MethodTaskStart, TaskStartArgs{Brief: brief, Solo: solo, Effort: effort}) + payload, err := a.callTaskStart(ctx, MethodTaskStart, TaskStartArgs{Brief: brief, Solo: solo, Effort: effort}) if err != nil { return 0, "", "", err } @@ -1605,6 +1605,15 @@ func (a *Agent) StartTaskEffort(ctx context.Context, brief string, solo bool, ef return started.ID, started.Title, started.Note, nil } +// A start whose answer never arrived may already have created work on the engine. +func (a *Agent) callTaskStart(ctx context.Context, method string, args any) (json.RawMessage, error) { + payload, answered, err := a.c.callAnswered(ctx, method, args, callDeadline) + if err != nil && !answered { + return nil, unanswered{said: err} + } + return payload, err +} + // RedoStronger runs a task again on the engine machine with a stronger crew // (`/redo stronger`); row 0 is the newest task the conversation started. func (a *Agent) RedoStronger(ctx context.Context, row uint64) (uint64, string, error) { diff --git a/internal/remote/task_start_unanswered_test.go b/internal/remote/task_start_unanswered_test.go new file mode 100644 index 000000000..cf5ff0c26 --- /dev/null +++ b/internal/remote/task_start_unanswered_test.go @@ -0,0 +1,110 @@ +package remote + +import ( + "context" + "errors" + "sync" + "testing" + "time" + + "github.com/Agent-Field/codeaf/internal/session" +) + +type slowTaskStartAgent struct { + *railAgent + accepted chan struct{} + release chan struct{} + starts int +} + +func (a *slowTaskStartAgent) StartTask(context.Context, string, bool) (uint64, string, string, error) { + a.mu.Lock() + a.starts++ + a.mu.Unlock() + a.land(taskEvent(7, "accepted work", session.TaskRunning)) + close(a.accepted) + <-a.release + return 7, "accepted work", "", nil +} + +func (a *slowTaskStartAgent) StartTaskEffort(ctx context.Context, brief string, solo bool, _ string) (uint64, string, string, error) { + return a.StartTask(ctx, brief, solo) +} + +func (a *slowTaskStartAgent) StartDelegate(ctx context.Context, _ string, brief string) (uint64, string, string, error) { + return a.StartTask(ctx, brief, true) +} + +func TestALateTaskStartKeepsOneAcceptedTaskOnTheHostedRail(t *testing.T) { + for _, entry := range []struct { + name string + start func(*Agent, context.Context) (uint64, string, string, error) + }{ + {"ordinary", func(a *Agent, ctx context.Context) (uint64, string, string, error) { + return a.StartTask(ctx, "accepted work", true) + }}, + {"effort", func(a *Agent, ctx context.Context) (uint64, string, string, error) { + return a.StartTaskEffort(ctx, "accepted work", true, "best") + }}, + {"delegate", func(a *Agent, ctx context.Context) (uint64, string, string, error) { + return a.StartDelegate(ctx, "builder", "accepted work") + }}, + } { + t.Run(entry.name, func(t *testing.T) { + far := &slowTaskStartAgent{ + railAgent: &railAgent{fakeAgent: &fakeAgent{}}, + accepted: make(chan struct{}), release: make(chan struct{}), + } + var releaseOnce sync.Once + defer func() { releaseOnce.Do(func() { close(far.release) }) }() + loop, err := Loopback(Hello{Version: Version}, Options{Boot: func(Hello) (*Engine, error) { + return &Engine{Agent: far, Workspace: "/srv/app", SessionFile: "/srv/app/j.jsonl"}, nil + }}) + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = loop.Close() }) + lane, stop := loop.Client.Agent().WatchTaskUpdates() + t.Cleanup(stop) + waitFor(t, "the task lane to open", func() bool { return far.opened() == 1 }) + + ctx, cancel := context.WithTimeout(context.Background(), time.Second) + t.Cleanup(cancel) + result := make(chan error, 1) + go func() { + _, _, _, err := entry.start(loop.Client.Agent(), ctx) + result <- err + }() + select { + case <-far.accepted: + case <-ctx.Done(): + t.Fatal("the engine did not accept the task before the deadline") + } + if err := <-result; !errors.Is(err, session.ErrSendUnanswered) { + t.Fatalf("accepted start without its receipt = %v, want an uncertain result", err) + } + row := nextTask(t, lane) + if row.Task == nil || row.Task.ID != 7 || row.Task.State != session.TaskRunning { + t.Fatalf("accepted task disappeared after its receipt timed out: %+v", row) + } + releaseOnce.Do(func() { close(far.release) }) + if _, err := loop.Client.Ping(); err != nil { + t.Fatalf("the late receipt broke the connection: %v", err) + } + far.mu.Lock() + defer far.mu.Unlock() + if far.starts != 1 || len(far.roster) != 1 { + t.Fatalf("start attempts = %d, tasks = %d, want one of each", far.starts, len(far.roster)) + } + }) + } +} + +func TestAnEngineRefusalToStartATaskIsStillDefinite(t *testing.T) { + client, engine := newEngine(t) + engine.fails[MethodTaskStart] = "the planner did not answer in time" + _, _, _, err := client.Agent().StartTask(context.Background(), "work", true) + if err == nil || errors.Is(err, session.ErrSendUnanswered) { + t.Fatalf("engine refusal = %v, want a definite failure", err) + } +} diff --git a/internal/tui3/app.go b/internal/tui3/app.go index d63a80753..50e0ab01c 100644 --- a/internal/tui3/app.go +++ b/internal/tui3/app.go @@ -5139,7 +5139,7 @@ func (a *app) route(msg tea.Msg) (tea.Model, tea.Cmd) { return a, nil } if msg.err != nil { - a.note("could not start the task · " + msg.err.Error()) + a.note(taskStartFailureNote(msg.err)) } else { // WHAT LANDED, AND WHAT IT IS CALLED (payload.go). The id is how a // person names this node to any other command on the surface and the diff --git a/internal/tui3/taskcommand.go b/internal/tui3/taskcommand.go index 353c3ae15..12135e351 100644 --- a/internal/tui3/taskcommand.go +++ b/internal/tui3/taskcommand.go @@ -2,6 +2,7 @@ package tui3 import ( "context" + "errors" "strconv" "strings" "time" @@ -93,6 +94,18 @@ type taskStartedMsg struct { conv string } +const taskStartLateNote = "the engine has not confirmed the start — the task may already be running" + +func taskStartFailureNote(err error) string { + if errors.Is(err, session.ErrSendUnanswered) { + return taskStartLateNote + } + if err == nil { + return "could not start the task" + } + return "could not start the task · " + err.Error() +} + func (a *app) runTaskCommand(arg string) tea.Cmd { // The word was typed, bare or with a brief (notice.go). a.noticeEvent(eventTaskTyped) diff --git a/internal/tui3/taskcommand_test.go b/internal/tui3/taskcommand_test.go index b955b2bc9..1c44006a7 100644 --- a/internal/tui3/taskcommand_test.go +++ b/internal/tui3/taskcommand_test.go @@ -28,6 +28,25 @@ type taskCommandFake struct { err error } +func TestTaskStartLateAnswerDoesNotSayTheTaskFailedToStart(t *testing.T) { + note := taskStartFailureNote(session.ErrSendUnanswered) + if note != taskStartLateNote { + t.Fatalf("late start note = %q, want %q", note, taskStartLateNote) + } + if strings.Contains(note, "could not start") { + t.Fatalf("late start was reported as a start failure: %q", note) + } + f := &taskCommandFake{Agent: &fakeAgent{model: "m"}, err: session.ErrSendUnanswered} + a := newTestApp(f) + _, _ = a.Update(a.runTaskCommand("accepted work")()) + if got := lastNote(t, a); got != taskStartLateNote { + t.Fatalf("task command displayed %q, want %q", got, taskStartLateNote) + } + if got := taskStartFailureNote(errors.New("the planner did not answer in time")); got != "could not start the task · the planner did not answer in time" { + t.Fatalf("a definite refusal was changed to uncertainty: %q", got) + } +} + func (f *taskCommandFake) StartTask(_ context.Context, brief string, solo bool) (uint64, string, string, error) { f.singleCalls++ f.brief, f.solo = brief, solo