diff --git a/docs/changes/unreleased/1262-a-time-limit-ends-a-run.md b/docs/changes/unreleased/1262-a-time-limit-ends-a-run.md new file mode 100644 index 0000000000..8f91a3f1e8 --- /dev/null +++ b/docs/changes/unreleased/1262-a-time-limit-ends-a-run.md @@ -0,0 +1,14 @@ +--- +kind: fixed +title: a time limit a person sets now ends a run, by the same road as the cost limit +pr: 1262 +surface: [chat, engine] +invalidates: + - "A session's elapsed-time limit (`--max-hours`) never reached a run that `/task` started. `chatBudget` turns the flag into `session.Budget.Wall`, the interactive check reads it before a new turn, and nothing handed it to the run engine: a run carried a dollar limit (the conversation's spend ceiling) and no time limit at all, so it ran on past the hours its person set." + - "`RunSpec.Elapsed` now carries what is left of the session's wall when the run starts, measured on the same clock the interactive check uses, and `run.Limits.Elapsed` ends the run on it. The ending is the cost limit's: nothing new is launched, the outcome is `a limit you set stopped it`, and what finished lands the ordinary way. A session with no time limit puts none on its runs." + - "THE ONE DIFFERENCE FROM THE COST LIMIT IS THE WORK IN FLIGHT. A cost limit lets it finish. A time limit ends it, and absorbs each ending the way the caller's wall does, so a cut task reads `incomplete` instead of `running` and what it spent is in the run's account." + - "A run that ended on a limit with no result of its own now says the limit's sentence on its row. It said nothing there before." +--- + +`--max-cost` still does not reach a run: a run's dollar limit is the conversation's +spend ceiling. The manual says which limit is which. diff --git a/internal/manual/chat/starting-codeaf.md b/internal/manual/chat/starting-codeaf.md index 3757525fcf..9c11c7b533 100644 --- a/internal/manual/chat/starting-codeaf.md +++ b/internal/manual/chat/starting-codeaf.md @@ -132,6 +132,19 @@ stale is running. and anything that acts in your name still ask, whatever the setting says. See the permissions page. +## Does a time limit stop a running task? + +Yes. The elapsed-time limit (`--max-hours`) is measured from when this session opened, +and a run that `/task` starts is handed whatever is left of it. When that time is up the +run starts nothing new, the work still going is ended where it is, and what had finished +comes home the ordinary way. The run's row ends `incomplete` and says +`a limit you set stopped it`, the same sentence a run says when it reaches the +conversation's spend ceiling. A task that was cut part way through reads `incomplete` too. + +The dollar limit on a run is the conversation's spend ceiling, not `--max-cost`. A run +that reaches it lets the work already going finish, because that money is already +committed; a time limit cannot wait, so it ends that work. + ## Leaving it running on its own · leaving a headless run going with a budget · --once yolo · no screen · unattended · overnight · nobody watching `--yolo` changes tool approvals. Normal interactive chat remains a conversation you diff --git a/internal/manual/chat/worker-harness.md b/internal/manual/chat/worker-harness.md index 604873918a..dafc0fbffb 100644 --- a/internal/manual/chat/worker-harness.md +++ b/internal/manual/chat/worker-harness.md @@ -405,6 +405,8 @@ requirement in them, and speed is no permission to skip the walk. allows — `/settings` → Spending → **per conversation** — which is off by default. The run's width and its dollar ceiling are the conversation's own numbers, so a run costs what the conversation costs and runs as wide as the conversation may. +- **The time limit.** An elapsed-time limit on the session ends a run too: see + "Does a time limit stop a running task?" on the page about starting codeaf. - **The step cap.** A worker stops at **200** finished tool calls — the same figure a node worker carries. The cap is a bound on spend and not a finding about the work: the turn is stopped there rather than judged, and a worker stopped this diff --git a/internal/manual/chat_test.go b/internal/manual/chat_test.go index ffcf06dcfb..36a1cc775e 100644 --- a/internal/manual/chat_test.go +++ b/internal/manual/chat_test.go @@ -893,6 +893,7 @@ func TestTheChatManualAnswersTheQuestionsPeopleAsk(t *testing.T) { {"can I leave a headless run going with a budget", "starting-codeaf"}, {"my --once yolo run never started a task", "starting-codeaf"}, {"does a run with no screen carry its own work on", "starting-codeaf"}, + {"does a time limit stop a running task", "starting-codeaf"}, {"it says needs your look but I ran it with yolo", "tasks"}, {"what does taken as it stands mean", "tasks"}, {"it says it could not be brought home", "tasks"}, diff --git a/internal/run/elapsed_limit_test.go b/internal/run/elapsed_limit_test.go new file mode 100644 index 0000000000..235a9be8f9 --- /dev/null +++ b/internal/run/elapsed_limit_test.go @@ -0,0 +1,74 @@ +package run + +import ( + "context" + "sync/atomic" + "testing" + "time" + + "github.com/Agent-Field/codeaf/internal/plandb" +) + +// TestElapsedLimitUsesTheLimitEndingRoad states the run-limit law at the +// boundary where it matters: reaching a time limit while work is in flight +// ends and drains that work, starts nothing afterwards, and reports the same +// outcome as every other run limit. Its timer is driven by the test, never by +// elapsed machine time. +func TestElapsedLimitUsesTheLimitEndingRoad(t *testing.T) { + store, err := plandb.Open(t.TempDir()+"/plan.json", "elapsed-limit", "root", "root", "run until the limit") + if err != nil { + t.Fatalf("open store: %v", err) + } + defer store.Close() + + fired := make(chan time.Time, 1) + started := make(chan struct{}) + ended := make(chan struct{}) + var launches atomic.Int32 + factory := func(task plandb.Task) Worker { + launches.Add(1) + return workerFunc(func(ctx context.Context, task plandb.Task) (Report, error) { + close(started) + <-ctx.Done() + if _, err := store.AddMany([]plandb.TaskSpec{{ID: "after-limit", Title: "must not launch", ParentID: task.ID}}); err != nil { + t.Errorf("add post-limit task: %v", err) + } + close(ended) + return Report{USD: 0.25}, ctx.Err() + }) + } + supervisor := NewSupervisor(store, t.TempDir(), 2, Limits{Elapsed: time.Hour}, factory) + supervisor.after = func(time.Duration) <-chan time.Time { return fired } + + result := make(chan Outcome, 1) + go func() { result <- supervisor.Run(context.Background()) }() + <-started + fired <- time.Time{} + + if outcome := <-result; outcome != OutcomeLimit { + t.Fatalf("outcome = %q, want %q", outcome, OutcomeLimit) + } + select { + case <-ended: + default: + t.Fatal("Run returned before its in-flight worker ended and drained") + } + // THE CUT WORKER'S ENDING WAS READ, NOT DROPPED: what it spent before the + // limit is in the run's account, and the run holds no claim on it any more. + if supervisor.spent != 0.25 { + t.Fatalf("run spend = %v, want the cut worker's 0.25: its return was dropped", supervisor.spent) + } + if len(supervisor.cancels) != 0 { + t.Fatalf("the run still holds %d workers after its limit ended it", len(supervisor.cancels)) + } + if got := launches.Load(); got != 1 { + t.Fatalf("worker launches = %d, want only the in-flight root", got) + } + if task := store.Task("after-limit"); task == nil || task.Status != plandb.StatusReady { + t.Fatalf("post-limit task = %#v, want ready and unlaunched", task) + } +} + +type workerFunc func(context.Context, plandb.Task) (Report, error) + +func (f workerFunc) Run(ctx context.Context, task plandb.Task) (Report, error) { return f(ctx, task) } diff --git a/internal/run/enginewire.go b/internal/run/enginewire.go index 886cb17abd..6bbee46b66 100644 --- a/internal/run/enginewire.go +++ b/internal/run/enginewire.go @@ -31,7 +31,7 @@ func (engine) Start(ctx context.Context, spec session.RunSpec) session.RunSummar // THE REVIEW ROUND IS ON for every task the chat's door opens: a leaf // that lands done is checked against its acceptance, and a check that // does not hold becomes a fix task the run waits on. - Limits: Limits{CostUSD: spec.CostUSD, StepsPerTask: spec.StepsPerTask, ReviewRound: true}, + Limits: Limits{CostUSD: spec.CostUSD, Elapsed: spec.Elapsed, StepsPerTask: spec.StepsPerTask, ReviewRound: true}, // THE CREW IS THE PROFILE'S, read again at each launch, and the seat's // provider is the door's own completer through the one seam a test // scripts ([CrewFactory]). diff --git a/internal/run/run.go b/internal/run/run.go index 75610971c7..84c233be44 100644 --- a/internal/run/run.go +++ b/internal/run/run.go @@ -76,6 +76,10 @@ type Supervisor struct { limits Limits factory WorkerFactory + // after provides the run elapsed-limit signal. Production uses time.After; + // a test supplies a driven channel so the limit law needs no real sleep. + after func(time.Duration) <-chan time.Time + // staleAfter is how long a claim may go untouched before this pass takes // it over, resolved from Limits (or its default) at the top of Run, so // every pass reads the same window. @@ -142,6 +146,7 @@ func NewSupervisor(store *plandb.Store, workspace string, slots int, limits Limi slots: slots, limits: limits, factory: factory, + after: time.After, finished: make(chan workerReturn, slots+1), cancels: make(map[string]context.CancelFunc), checkOf: make(map[string]string), @@ -201,6 +206,10 @@ func (s *Supervisor) Run(ctx context.Context) Outcome { timer := time.NewTimer(passInterval) defer timer.Stop() + var elapsed <-chan time.Time + if s.limits.Elapsed > 0 { + elapsed = s.after(s.limits.Elapsed) + } for { if outcome := s.pass(ctx, rootID); outcome != "" { s.drain() @@ -212,6 +221,28 @@ func (s *Supervisor) Run(ctx context.Context) Outcome { s.absorb(ret) case <-timer.C: timer.Reset(passInterval) + case <-elapsed: + // TIME AND COST SHARE ONE ENDING. Mark the same limit state the + // spend counter marks, so the next pass launches nothing and answers + // the existing OutcomeLimit. + // + // THE DIFFERENCE IS THE WORK IN FLIGHT. A cost limit lets it finish, + // because what it will spend is already committed; a time limit + // cannot, because the time is gone. So every worker is ended here, and + // ITS ENDING IS ABSORBED, the way the caller's wall below reads its + // endings: a return that was dropped would leave its task claimed and + // reading as running on a run that is over, and what the worker spent + // before it was cut would be missing from the run's account. + s.limitHit = true + elapsed = nil + for _, cancel := range s.cancels { + cancel() + } + for s.inFlight > 0 { + ret := <-s.finished + s.inFlight-- + s.absorb(ret) + } case <-ctx.Done(): // The caller's wall: workers still out there were handed this // context and end with it. Their endings are absorbed so their @@ -258,7 +289,8 @@ func (s *Supervisor) pass(ctx context.Context, rootID string) Outcome { if s.inFlight == 0 && (s.rootFailed || s.limitHit) { // Nothing of ours is running and the run cannot complete itself: the - // root's own worker failed, or the cost counter has reached its limit. + // root's own worker failed, or the run has reached a limit a person set, + // in dollars or in time. // The word says which; the store keeps whatever the run reached. if s.limitHit { return OutcomeLimit diff --git a/internal/run/worker.go b/internal/run/worker.go index c10b17f485..f96bfde4be 100644 --- a/internal/run/worker.go +++ b/internal/run/worker.go @@ -45,14 +45,18 @@ type Worker interface { // say no seat exists for this task; the task then fails rather than hangs. type WorkerFactory func(task plandb.Task) Worker -// Limits bound a run from the outside. Every field is optional: a CostUSD of -// zero (or less) sets no cost limit, a StepsPerTask of zero hands the worker -// no cap, and ReviewRound's false is the run every caller had before it. +// Limits bound a run from the outside. Every field is optional: a CostUSD or +// Elapsed of zero (or less) sets no corresponding run limit, a StepsPerTask +// of zero hands the worker no cap, and ReviewRound's false is the run every caller had before it. type Limits struct { // CostUSD is what the whole run may spend, as the sum of every Report's // USD. When the counter has reached it no new worker starts and the run // ends on the limit word of the outcome ladder. CostUSD float64 + // Elapsed is how long the whole run may remain active. When it passes, + // workers already in flight are ended and drained, no new worker starts, + // and the run ends on the same limit word as the cost counter. + Elapsed time.Duration // StepsPerTask is handed to every worker through its context, so the loop // a worker hosts can cap itself without the supervisor counting its steps. StepsPerTask int diff --git a/internal/session/task_run_belt.go b/internal/session/task_run_belt.go index b96ea50543..6facb8d2f9 100644 --- a/internal/session/task_run_belt.go +++ b/internal/session/task_run_belt.go @@ -63,6 +63,9 @@ type RunSpec struct { // wide as the conversation may. Slots int CostUSD float64 + // Elapsed is the conversation time still available when this run starts. + // Zero means no time ceiling, matching the run engine's Limits contract. + Elapsed time.Duration // StepsPerTask is the per-task step cap, the same figure a node of this // session's own tree carries. StepsPerTask int @@ -293,6 +296,13 @@ func (a *Agent) startKnownTaskRun(ctx context.Context, id uint64, title, brief s workSeat, _ := roles.TierModel(source, roles.TierWorker) planSeat, _ := roles.TierModel(source, roles.TierMastermind) + wallLeft, _ := a.config.Budget.Left() + if a.config.Budget.Wall > 0 && !a.startedAt.IsZero() { + wallLeft = a.config.Budget.Wall - time.Since(a.startedAt) + if wallLeft <= 0 { + wallLeft = time.Nanosecond + } + } spec := RunSpec{ Store: store, Workspace: run.workspace, @@ -300,6 +310,7 @@ func (a *Agent) startKnownTaskRun(ctx context.Context, id uint64, title, brief s Brief: brief, Slots: a.config.TaskParallel, CostUSD: a.config.SpendRailUSD, + Elapsed: wallLeft, // The step cap a node of this session's own tree carries, so a run // worker and a node worker stop at the same figure. StepsPerTask: taskMaxSteps, @@ -585,6 +596,9 @@ func (a *Agent) beltRunNotice(run *beltRun, summary RunSummary, landing RunLandi state = TaskFailed } report := strings.TrimSpace(summary.Result) + if report == "" && summary.Outcome != beltRunOutcomeDone { + report = strings.TrimSpace(summary.Outcome) + } if line := beltLandingLine(landing); line != "" { if report != "" { report += "\n" diff --git a/internal/session/task_run_belt_test.go b/internal/session/task_run_belt_test.go index 7b59ba8c1a..b97cca86de 100644 --- a/internal/session/task_run_belt_test.go +++ b/internal/session/task_run_belt_test.go @@ -369,6 +369,56 @@ func TestStartTaskBashBeltStartsARunOnTheStore(t *testing.T) { } } +func TestStartTaskBashBeltPassesTheConversationWallLeftToTheRun(t *testing.T) { + t.Setenv("CODEAF_TASK_BELT", "bash") + double := newBeltRunDouble("done") + registerBeltRunEngine(t, double) + + dir := t.TempDir() + agent, _ := newTestAgent(t, beltRunCompleter{text: "done"}, func(config *Config) { + config.Workspace = newTestRepo(t) + config.Place = Place{Dir: dir} + config.AskConsent = false + config.Budget = Budget{Wall: 2 * time.Hour} + }) + agent.startedAt = time.Now().Add(-90 * time.Minute) + + if _, _, _, err := agent.StartTask(context.Background(), "finish within the time left", false); err != nil { + t.Fatalf("StartTask: %v", err) + } + <-double.entered + double.mu.Lock() + got := double.spec.Elapsed + double.mu.Unlock() + if got < 29*time.Minute || got > 31*time.Minute { + t.Fatalf("run elapsed limit = %v, want the roughly 30 minutes left, not the original 2 hours", got) + } + endBeltRun(t, agent, double) +} + +func TestDriveBeltRunLimitUsesTheOrdinaryLandingRoad(t *testing.T) { + agent, _, run, _ := landingSummaryFixture(t, &scriptedCompleter{}) + landCalls := 0 + engine := landingRunDouble{ + summary: RunSummary{Outcome: "a limit you set stopped it"}, + landing: RunLanding{Branch: "task/limited", Changed: []string{"kept.txt"}}, + landCalls: &landCalls, + } + + agent.driveBeltRun(context.Background(), engine, run, RunSpec{}) + + if landCalls != 1 { + t.Fatalf("Land calls = %d, want exactly one ordinary landing", landCalls) + } + notice := agent.beltRunNotice(run, engine.summary, engine.landing) + if notice.State != TaskFailed { + t.Fatalf("limited run row state = %q, want the same failed state as a cost-limited run", notice.State) + } + if !strings.Contains(notice.Report, "a limit you set stopped it") { + t.Fatalf("limited run row report = %q, want the existing limit ending sentence", notice.Report) + } +} + // TestStartTaskBashBeltJoinsTheLiveRun: a run is one store, so a second `/task` // while one is live adds its work to that same store — a child of the run's one // root — rather than opening another. The second id is in the store, and the @@ -503,12 +553,16 @@ func TestLandingDigestIsUnchangedWithoutAStoredSummary(t *testing.T) { // the summary refresh must have stored its sentence before the outcome note is // composed. type landingRunDouble struct { - summary RunSummary - landing RunLanding + summary RunSummary + landing RunLanding + landCalls *int } func (d landingRunDouble) Start(context.Context, RunSpec) RunSummary { return d.summary } func (d landingRunDouble) Land(context.Context, *plandb.Store, string, string) (RunLanding, error) { + if d.landCalls != nil { + *d.landCalls++ + } return d.landing, nil }