fix(subagents): synchronize shutdown attribution
- commit
- 36e149dedec67589475912a4269d88d09933a952
- author
- brice · · 2026-08-09 08:58 UTC
- parent
- 4a46e7b
- signature
- GPG Verified
CD88F9E564E0102646FB6077A1A07C7C7C3E349D· arch-gpg Verified for the committer email
7 files · +155 -47
M packages/agent/modes/auto_subagents_test.go +27 -0
| @@ -166,6 +166,33 @@ func TestTrackStoppedSubagentWorkerDeliversTerminalUpdate(t *testing.T) { | |||
| 166 | 166 | } | |
| 167 | 167 | } | |
| 168 | 168 | ||
| 169 | + | func TestTrackDeadlineFailureUsesAttributedResultError(t *testing.T) { | |
| 170 | + | mgr := subagents.New(subagents.Config{ | |
| 171 | + | Root: t.TempDir(), | |
| 172 | + | RepoRoot: t.TempDir(), | |
| 173 | + | NewRunner: func(*subagents.Agent) subagents.Runner { | |
| 174 | + | return subagents.RunnerFunc(func(context.Context, subagents.Sink) error { | |
| 175 | + | return context.DeadlineExceeded | |
| 176 | + | }) | |
| 177 | + | }, | |
| 178 | + | }) | |
| 179 | + | a, err := mgr.Spawn(context.Background(), "run until deadline") | |
| 180 | + | if err != nil { | |
| 181 | + | t.Fatal(err) | |
| 182 | + | } | |
| 183 | + | ||
| 184 | + | iv := newQueuedAutoSubagentsInteractive() | |
| 185 | + | iv.TrackSubagentWorker(a, a.Task) | |
| 186 | + | update := waitForQueuedPrompt(t, iv) | |
| 187 | + | if !strings.Contains(update, "status: failed") { | |
| 188 | + | t.Fatalf("deadline update changed failed status: %q", update) | |
| 189 | + | } | |
| 190 | + | want := subagents.ShutdownResultError(subagents.ShutdownOriginDeadline).Message | |
| 191 | + | if !strings.Contains(update, want) { | |
| 192 | + | t.Fatalf("deadline update missing attributed result error: %q", update) | |
| 193 | + | } | |
| 194 | + | } | |
| 195 | + | ||
| 169 | 196 | func TestTrackResumedSubagentWorkerDeliversFollowUp(t *testing.T) { | |
| 170 | 197 | started := make(chan struct{}) | |
| 171 | 198 | mgr := subagents.New(subagents.Config{ | |
| @@ -166,6 +166,33 @@ func TestTrackStoppedSubagentWorkerDeliversTerminalUpdate(t *testing.T) { | |||||
| 166 | } | 166 | } | ||
| 167 | } | 167 | } | ||
| 168 | 168 | ||||
| 169 | + | func TestTrackDeadlineFailureUsesAttributedResultError(t *testing.T) { | |||
| 170 | + | mgr := subagents.New(subagents.Config{ | |||
| 171 | + | Root: t.TempDir(), | |||
| 172 | + | RepoRoot: t.TempDir(), | |||
| 173 | + | NewRunner: func(*subagents.Agent) subagents.Runner { | |||
| 174 | + | return subagents.RunnerFunc(func(context.Context, subagents.Sink) error { | |||
| 175 | + | return context.DeadlineExceeded | |||
| 176 | + | }) | |||
| 177 | + | }, | |||
| 178 | + | }) | |||
| 179 | + | a, err := mgr.Spawn(context.Background(), "run until deadline") | |||
| 180 | + | if err != nil { | |||
| 181 | + | t.Fatal(err) | |||
| 182 | + | } | |||
| 183 | + | ||||
| 184 | + | iv := newQueuedAutoSubagentsInteractive() | |||
| 185 | + | iv.TrackSubagentWorker(a, a.Task) | |||
| 186 | + | update := waitForQueuedPrompt(t, iv) | |||
| 187 | + | if !strings.Contains(update, "status: failed") { | |||
| 188 | + | t.Fatalf("deadline update changed failed status: %q", update) | |||
| 189 | + | } | |||
| 190 | + | want := subagents.ShutdownResultError(subagents.ShutdownOriginDeadline).Message | |||
| 191 | + | if !strings.Contains(update, want) { | |||
| 192 | + | t.Fatalf("deadline update missing attributed result error: %q", update) | |||
| 193 | + | } | |||
| 194 | + | } | |||
| 195 | + | ||||
| 169 | func TestTrackResumedSubagentWorkerDeliversFollowUp(t *testing.T) { | 196 | func TestTrackResumedSubagentWorkerDeliversFollowUp(t *testing.T) { | ||
| 170 | started := make(chan struct{}) | 197 | started := make(chan struct{}) | ||
| 171 | mgr := subagents.New(subagents.Config{ | 198 | mgr := subagents.New(subagents.Config{ | ||
M packages/agent/modes/interactive.go +4 -2
| @@ -8822,8 +8822,10 @@ func (i *Interactive) flushSupervisorSummary(batch []*subagentWatchEntry) { | |||
| 8822 | 8822 | status = string(snap.Status) | |
| 8823 | 8823 | } | |
| 8824 | 8824 | resultErr := "" | |
| 8825 | - | if snap.Result != nil && snap.Result.Status == subagents.ResultCanceled && snap.Result.ShutdownOrigin != "" { | |
| 8826 | - | status = "cancelled" | |
| 8825 | + | if snap.Result != nil && snap.Result.ShutdownOrigin != "" { | |
| 8826 | + | if snap.Result.Status == subagents.ResultCanceled { | |
| 8827 | + | status = "cancelled" | |
| 8828 | + | } | |
| 8827 | 8829 | if snap.Result.Error != nil { | |
| 8828 | 8830 | resultErr = snap.Result.Error.Message | |
| 8829 | 8831 | } | |
| @@ -8822,8 +8822,10 @@ func (i *Interactive) flushSupervisorSummary(batch []*subagentWatchEntry) { | |||||
| 8822 | status = string(snap.Status) | 8822 | status = string(snap.Status) | ||
| 8823 | } | 8823 | } | ||
| 8824 | resultErr := "" | 8824 | resultErr := "" | ||
| 8825 | - | if snap.Result != nil && snap.Result.Status == subagents.ResultCanceled && snap.Result.ShutdownOrigin != "" { | 8825 | + | if snap.Result != nil && snap.Result.ShutdownOrigin != "" { |
| 8826 | - | status = "cancelled" | 8826 | + | if snap.Result.Status == subagents.ResultCanceled { |
| 8827 | + | status = "cancelled" | |||
| 8828 | + | } | |||
| 8827 | if snap.Result.Error != nil { | 8829 | if snap.Result.Error != nil { | ||
| 8828 | resultErr = snap.Result.Error.Message | 8830 | resultErr = snap.Result.Error.Message | ||
| 8829 | } | 8831 | } | ||
M packages/agent/subagents/agent.go +21 -10
| @@ -152,6 +152,7 @@ type Agent struct { | |||
| 152 | 152 | streamingAssistantLines int | |
| 153 | 153 | finished time.Time | |
| 154 | 154 | lastErr error | |
| 155 | + | shutdownOrigin ShutdownOrigin | |
| 155 | 156 | transcriptLoaded bool | |
| 156 | 157 | legacyEventState bool | |
| 157 | 158 | transcriptMu sync.Mutex | |
| @@ -163,7 +164,6 @@ type Agent struct { | |||
| 163 | 164 | updatedAt time.Time | |
| 164 | 165 | lastActivity time.Time | |
| 165 | 166 | result *TurnResult | |
| 166 | - | shutdownOrigin ShutdownOrigin | |
| 167 | 167 | resultRef string | |
| 168 | 168 | patchRef string | |
| 169 | 169 | changedFiles []string | |
| @@ -411,19 +411,30 @@ func (a *Agent) setResult(result *TurnResult) { | |||
| 411 | 411 | a.lifecycleMu.Unlock() | |
| 412 | 412 | } | |
| 413 | 413 | ||
| 414 | - | func (a *Agent) setShutdownOrigin(origin ShutdownOrigin) { | |
| 415 | - | a.lifecycleMu.Lock() | |
| 416 | - | a.shutdownOrigin = origin.Sanitized() | |
| 417 | - | a.updatedAt = time.Now() | |
| 418 | - | a.lifecycleMu.Unlock() | |
| 419 | - | } | |
| 420 | - | ||
| 421 | 414 | func (a *Agent) shutdownOriginValue() ShutdownOrigin { | |
| 422 | - | a.lifecycleMu.Lock() | |
| 423 | - | defer a.lifecycleMu.Unlock() | |
| 415 | + | a.mu.Lock() | |
| 416 | + | defer a.mu.Unlock() | |
| 424 | 417 | return a.shutdownOrigin | |
| 425 | 418 | } | |
| 426 | 419 | ||
| 420 | + | // prepareShutdown publishes the shutdown origin in the same critical section | |
| 421 | + | // as the killed status. Finalization can therefore observe either the previous | |
| 422 | + | // terminal state or a fully attributed cancellation, never an intermediate one. | |
| 423 | + | func (a *Agent) prepareShutdown(origin ShutdownOrigin) (Status, bool) { | |
| 424 | + | a.mu.Lock() | |
| 425 | + | defer a.mu.Unlock() | |
| 426 | + | status := a.status | |
| 427 | + | if status == StatusDone || status == StatusFailed || status == StatusKilled { | |
| 428 | + | return status, false | |
| 429 | + | } | |
| 430 | + | a.shutdownOrigin = origin.Sanitized() | |
| 431 | + | if status != StatusDetached { | |
| 432 | + | a.status = StatusKilled | |
| 433 | + | a.activity = "canceling" | |
| 434 | + | } | |
| 435 | + | return status, true | |
| 436 | + | } | |
| 437 | + | ||
| 427 | 438 | // Transcript returns a copy of the running transcript. Detached agents | |
| 428 | 439 | // hydrate their durable event history the first time it is requested. | |
| 429 | 440 | func (a *Agent) Transcript() []string { | |
| @@ -152,6 +152,7 @@ type Agent struct { | |||||
| 152 | streamingAssistantLines int | 152 | streamingAssistantLines int | ||
| 153 | finished time.Time | 153 | finished time.Time | ||
| 154 | lastErr error | 154 | lastErr error | ||
| 155 | + | shutdownOrigin ShutdownOrigin | |||
| 155 | transcriptLoaded bool | 156 | transcriptLoaded bool | ||
| 156 | legacyEventState bool | 157 | legacyEventState bool | ||
| 157 | transcriptMu sync.Mutex | 158 | transcriptMu sync.Mutex | ||
| @@ -163,7 +164,6 @@ type Agent struct { | |||||
| 163 | updatedAt time.Time | 164 | updatedAt time.Time | ||
| 164 | lastActivity time.Time | 165 | lastActivity time.Time | ||
| 165 | result *TurnResult | 166 | result *TurnResult | ||
| 166 | - | shutdownOrigin ShutdownOrigin | |||
| 167 | resultRef string | 167 | resultRef string | ||
| 168 | patchRef string | 168 | patchRef string | ||
| 169 | changedFiles []string | 169 | changedFiles []string | ||
| @@ -411,19 +411,30 @@ func (a *Agent) setResult(result *TurnResult) { | |||||
| 411 | a.lifecycleMu.Unlock() | 411 | a.lifecycleMu.Unlock() | ||
| 412 | } | 412 | } | ||
| 413 | 413 | ||||
| 414 | - | func (a *Agent) setShutdownOrigin(origin ShutdownOrigin) { | |||
| 415 | - | a.lifecycleMu.Lock() | |||
| 416 | - | a.shutdownOrigin = origin.Sanitized() | |||
| 417 | - | a.updatedAt = time.Now() | |||
| 418 | - | a.lifecycleMu.Unlock() | |||
| 419 | - | } | |||
| 420 | - | ||||
| 421 | func (a *Agent) shutdownOriginValue() ShutdownOrigin { | 414 | func (a *Agent) shutdownOriginValue() ShutdownOrigin { | ||
| 422 | - | a.lifecycleMu.Lock() | 415 | + | a.mu.Lock() |
| 423 | - | defer a.lifecycleMu.Unlock() | 416 | + | defer a.mu.Unlock() |
| 424 | return a.shutdownOrigin | 417 | return a.shutdownOrigin | ||
| 425 | } | 418 | } | ||
| 426 | 419 | ||||
| 420 | + | // prepareShutdown publishes the shutdown origin in the same critical section | |||
| 421 | + | // as the killed status. Finalization can therefore observe either the previous | |||
| 422 | + | // terminal state or a fully attributed cancellation, never an intermediate one. | |||
| 423 | + | func (a *Agent) prepareShutdown(origin ShutdownOrigin) (Status, bool) { | |||
| 424 | + | a.mu.Lock() | |||
| 425 | + | defer a.mu.Unlock() | |||
| 426 | + | status := a.status | |||
| 427 | + | if status == StatusDone || status == StatusFailed || status == StatusKilled { | |||
| 428 | + | return status, false | |||
| 429 | + | } | |||
| 430 | + | a.shutdownOrigin = origin.Sanitized() | |||
| 431 | + | if status != StatusDetached { | |||
| 432 | + | a.status = StatusKilled | |||
| 433 | + | a.activity = "canceling" | |||
| 434 | + | } | |||
| 435 | + | return status, true | |||
| 436 | + | } | |||
| 437 | + | ||||
| 427 | // Transcript returns a copy of the running transcript. Detached agents | 438 | // Transcript returns a copy of the running transcript. Detached agents | ||
| 428 | // hydrate their durable event history the first time it is requested. | 439 | // hydrate their durable event history the first time it is requested. | ||
| 429 | func (a *Agent) Transcript() []string { | 440 | func (a *Agent) Transcript() []string { | ||
M packages/agent/subagents/result.go +0 -12
| @@ -557,18 +557,6 @@ func (f *Supervisor) ensureResult(a *Agent, status Status, runErr error) { | |||
| 557 | 557 | } | |
| 558 | 558 | if errors.Is(runErr, context.DeadlineExceeded) { | |
| 559 | 559 | result.Status = ResultFailed | |
| 560 | - | // The parent deadline is authoritative for terminal categorization, so | |
| 561 | - | // it intentionally overrides any child result error, including | |
| 562 | - | // context_limit. The worker was first asked to stop cleanly, and any | |
| 563 | - | // streamed output remains in this result and HistoryRef(agentID). | |
| 564 | - | message := "subagent deadline exceeded; inspect history for received output" | |
| 565 | - | if result.Output != "" { | |
| 566 | - | message = "subagent deadline exceeded; partial output is preserved in the result and history" | |
| 567 | - | } | |
| 568 | - | result.Error = &ResultError{ | |
| 569 | - | Code: "deadline_exceeded", | |
| 570 | - | Message: message, | |
| 571 | - | } | |
| 572 | 560 | result.ShutdownOrigin = ShutdownOriginDeadline | |
| 573 | 561 | } else if result.Error == nil { | |
| 574 | 562 | result.Error = &ResultError{Code: "runner_failed", Message: truncate(runErr.Error(), 500)} | |
| @@ -557,18 +557,6 @@ func (f *Supervisor) ensureResult(a *Agent, status Status, runErr error) { | |||||
| 557 | } | 557 | } | ||
| 558 | if errors.Is(runErr, context.DeadlineExceeded) { | 558 | if errors.Is(runErr, context.DeadlineExceeded) { | ||
| 559 | result.Status = ResultFailed | 559 | result.Status = ResultFailed | ||
| 560 | - | // The parent deadline is authoritative for terminal categorization, so | |||
| 561 | - | // it intentionally overrides any child result error, including | |||
| 562 | - | // context_limit. The worker was first asked to stop cleanly, and any | |||
| 563 | - | // streamed output remains in this result and HistoryRef(agentID). | |||
| 564 | - | message := "subagent deadline exceeded; inspect history for received output" | |||
| 565 | - | if result.Output != "" { | |||
| 566 | - | message = "subagent deadline exceeded; partial output is preserved in the result and history" | |||
| 567 | - | } | |||
| 568 | - | result.Error = &ResultError{ | |||
| 569 | - | Code: "deadline_exceeded", | |||
| 570 | - | Message: message, | |||
| 571 | - | } | |||
| 572 | result.ShutdownOrigin = ShutdownOriginDeadline | 560 | result.ShutdownOrigin = ShutdownOriginDeadline | ||
| 573 | } else if result.Error == nil { | 561 | } else if result.Error == nil { | ||
| 574 | result.Error = &ResultError{Code: "runner_failed", Message: truncate(runErr.Error(), 500)} | 562 | result.Error = &ResultError{Code: "runner_failed", Message: truncate(runErr.Error(), 500)} | ||
M packages/agent/subagents/result_test.go +20 -13
| @@ -286,21 +286,22 @@ func checkCaptureFailureResult(t *testing.T, agent *Agent, stateDir string) { | |||
| 286 | 286 | ||
| 287 | 287 | func TestEnsureResultReconcilesChildStatusWithRunnerError(t *testing.T) { | |
| 288 | 288 | tests := []struct { | |
| 289 | - | name string | |
| 290 | - | status Status | |
| 291 | - | runErr error | |
| 292 | - | childCode string | |
| 293 | - | shutdownOrigin ShutdownOrigin | |
| 294 | - | wantState TurnStatus | |
| 295 | - | wantCode string | |
| 296 | - | wantOutput string | |
| 289 | + | name string | |
| 290 | + | status Status | |
| 291 | + | runErr error | |
| 292 | + | childCode string | |
| 293 | + | shutdownOrigin ShutdownOrigin | |
| 294 | + | wantShutdownOrigin ShutdownOrigin | |
| 295 | + | wantState TurnStatus | |
| 296 | + | wantCode string | |
| 297 | + | wantOutput string | |
| 297 | 298 | }{ | |
| 298 | 299 | {name: "runner failure", status: StatusFailed, runErr: errors.New("runner failed"), wantState: ResultFailed, wantCode: "runner_failed"}, | |
| 299 | 300 | {name: "runner cancellation", status: StatusFailed, runErr: context.Canceled, wantState: ResultCanceled, wantCode: "runner_failed"}, | |
| 300 | - | {name: "deadline preserves recovery guidance", status: StatusFailed, runErr: context.DeadlineExceeded, wantState: ResultFailed, wantCode: "deadline_exceeded", wantOutput: "partial answer"}, | |
| 301 | - | {name: "deadline overrides child error", status: StatusFailed, runErr: context.DeadlineExceeded, childCode: "context_limit", wantState: ResultFailed, wantCode: "deadline_exceeded", wantOutput: "partial answer"}, | |
| 301 | + | {name: "deadline preserves recovery guidance", status: StatusFailed, runErr: context.DeadlineExceeded, wantShutdownOrigin: ShutdownOriginDeadline, wantState: ResultFailed, wantCode: "deadline_exceeded", wantOutput: "partial answer"}, | |
| 302 | + | {name: "deadline overrides child error", status: StatusFailed, runErr: context.DeadlineExceeded, childCode: "context_limit", wantShutdownOrigin: ShutdownOriginDeadline, wantState: ResultFailed, wantCode: "deadline_exceeded", wantOutput: "partial answer"}, | |
| 302 | 303 | {name: "killed", status: StatusKilled, runErr: context.Canceled, wantState: ResultCanceled, wantCode: "runner_failed"}, | |
| 303 | - | {name: "session shutdown", status: StatusKilled, runErr: context.Canceled, shutdownOrigin: ShutdownOriginSession, wantState: ResultCanceled, wantCode: "shutdown"}, | |
| 304 | + | {name: "session shutdown", status: StatusKilled, runErr: context.Canceled, shutdownOrigin: ShutdownOriginSession, wantShutdownOrigin: ShutdownOriginSession, wantState: ResultCanceled, wantCode: "shutdown"}, | |
| 304 | 305 | } | |
| 305 | 306 | for _, tt := range tests { | |
| 306 | 307 | t.Run(tt.name, func(t *testing.T) { | |
| @@ -348,8 +349,14 @@ func TestEnsureResultReconcilesChildStatusWithRunnerError(t *testing.T) { | |||
| 348 | 349 | if durable.Error == nil || durable.Error.Code != tt.wantCode { | |
| 349 | 350 | t.Fatalf("durable result error = %#v, want code %q", durable.Error, tt.wantCode) | |
| 350 | 351 | } | |
| 351 | - | if durable.ShutdownOrigin != tt.shutdownOrigin && tt.shutdownOrigin != "" { | |
| 352 | - | t.Fatalf("durable shutdown origin = %q, want %q", durable.ShutdownOrigin, tt.shutdownOrigin) | |
| 352 | + | if durable.ShutdownOrigin != tt.wantShutdownOrigin { | |
| 353 | + | t.Fatalf("durable shutdown origin = %q, want %q", durable.ShutdownOrigin, tt.wantShutdownOrigin) | |
| 354 | + | } | |
| 355 | + | if tt.wantShutdownOrigin != "" { | |
| 356 | + | wantErr := ShutdownResultError(tt.wantShutdownOrigin) | |
| 357 | + | if *durable.Error != *wantErr { | |
| 358 | + | t.Fatalf("durable result error = %#v, want canonical %#v", durable.Error, wantErr) | |
| 359 | + | } | |
| 353 | 360 | } | |
| 354 | 361 | }) | |
| 355 | 362 | } | |
| @@ -286,21 +286,22 @@ func checkCaptureFailureResult(t *testing.T, agent *Agent, stateDir string) { | |||||
| 286 | 286 | ||||
| 287 | func TestEnsureResultReconcilesChildStatusWithRunnerError(t *testing.T) { | 287 | func TestEnsureResultReconcilesChildStatusWithRunnerError(t *testing.T) { | ||
| 288 | tests := []struct { | 288 | tests := []struct { | ||
| 289 | - | name string | 289 | + | name string |
| 290 | - | status Status | 290 | + | status Status |
| 291 | - | runErr error | 291 | + | runErr error |
| 292 | - | childCode string | 292 | + | childCode string |
| 293 | - | shutdownOrigin ShutdownOrigin | 293 | + | shutdownOrigin ShutdownOrigin |
| 294 | - | wantState TurnStatus | 294 | + | wantShutdownOrigin ShutdownOrigin |
| 295 | - | wantCode string | 295 | + | wantState TurnStatus |
| 296 | - | wantOutput string | 296 | + | wantCode string |
| 297 | + | wantOutput string | |||
| 297 | }{ | 298 | }{ | ||
| 298 | {name: "runner failure", status: StatusFailed, runErr: errors.New("runner failed"), wantState: ResultFailed, wantCode: "runner_failed"}, | 299 | {name: "runner failure", status: StatusFailed, runErr: errors.New("runner failed"), wantState: ResultFailed, wantCode: "runner_failed"}, | ||
| 299 | {name: "runner cancellation", status: StatusFailed, runErr: context.Canceled, wantState: ResultCanceled, wantCode: "runner_failed"}, | 300 | {name: "runner cancellation", status: StatusFailed, runErr: context.Canceled, wantState: ResultCanceled, wantCode: "runner_failed"}, | ||
| 300 | - | {name: "deadline preserves recovery guidance", status: StatusFailed, runErr: context.DeadlineExceeded, wantState: ResultFailed, wantCode: "deadline_exceeded", wantOutput: "partial answer"}, | 301 | + | {name: "deadline preserves recovery guidance", status: StatusFailed, runErr: context.DeadlineExceeded, wantShutdownOrigin: ShutdownOriginDeadline, wantState: ResultFailed, wantCode: "deadline_exceeded", wantOutput: "partial answer"}, |
| 301 | - | {name: "deadline overrides child error", status: StatusFailed, runErr: context.DeadlineExceeded, childCode: "context_limit", wantState: ResultFailed, wantCode: "deadline_exceeded", wantOutput: "partial answer"}, | 302 | + | {name: "deadline overrides child error", status: StatusFailed, runErr: context.DeadlineExceeded, childCode: "context_limit", wantShutdownOrigin: ShutdownOriginDeadline, wantState: ResultFailed, wantCode: "deadline_exceeded", wantOutput: "partial answer"}, |
| 302 | {name: "killed", status: StatusKilled, runErr: context.Canceled, wantState: ResultCanceled, wantCode: "runner_failed"}, | 303 | {name: "killed", status: StatusKilled, runErr: context.Canceled, wantState: ResultCanceled, wantCode: "runner_failed"}, | ||
| 303 | - | {name: "session shutdown", status: StatusKilled, runErr: context.Canceled, shutdownOrigin: ShutdownOriginSession, wantState: ResultCanceled, wantCode: "shutdown"}, | 304 | + | {name: "session shutdown", status: StatusKilled, runErr: context.Canceled, shutdownOrigin: ShutdownOriginSession, wantShutdownOrigin: ShutdownOriginSession, wantState: ResultCanceled, wantCode: "shutdown"}, |
| 304 | } | 305 | } | ||
| 305 | for _, tt := range tests { | 306 | for _, tt := range tests { | ||
| 306 | t.Run(tt.name, func(t *testing.T) { | 307 | t.Run(tt.name, func(t *testing.T) { | ||
| @@ -348,8 +349,14 @@ func TestEnsureResultReconcilesChildStatusWithRunnerError(t *testing.T) { | |||||
| 348 | if durable.Error == nil || durable.Error.Code != tt.wantCode { | 349 | if durable.Error == nil || durable.Error.Code != tt.wantCode { | ||
| 349 | t.Fatalf("durable result error = %#v, want code %q", durable.Error, tt.wantCode) | 350 | t.Fatalf("durable result error = %#v, want code %q", durable.Error, tt.wantCode) | ||
| 350 | } | 351 | } | ||
| 351 | - | if durable.ShutdownOrigin != tt.shutdownOrigin && tt.shutdownOrigin != "" { | 352 | + | if durable.ShutdownOrigin != tt.wantShutdownOrigin { |
| 352 | - | t.Fatalf("durable shutdown origin = %q, want %q", durable.ShutdownOrigin, tt.shutdownOrigin) | 353 | + | t.Fatalf("durable shutdown origin = %q, want %q", durable.ShutdownOrigin, tt.wantShutdownOrigin) |
| 354 | + | } | |||
| 355 | + | if tt.wantShutdownOrigin != "" { | |||
| 356 | + | wantErr := ShutdownResultError(tt.wantShutdownOrigin) | |||
| 357 | + | if *durable.Error != *wantErr { | |||
| 358 | + | t.Fatalf("durable result error = %#v, want canonical %#v", durable.Error, wantErr) | |||
| 359 | + | } | |||
| 353 | } | 360 | } | ||
| 354 | }) | 361 | }) | ||
| 355 | } | 362 | } | ||
M packages/agent/subagents/supervisor.go +2 -10
| @@ -800,10 +800,8 @@ func (f *Supervisor) stop(ctx context.Context, id string, origin ShutdownOrigin) | |||
| 800 | 800 | // the pending cleanup below could then finalize an agent that was already | |
| 801 | 801 | // admitted to the runner. | |
| 802 | 802 | f.mu.Lock() | |
| 803 | - | a.mu.Lock() | |
| 804 | - | status := a.status | |
| 805 | - | if status == StatusDone || status == StatusFailed || status == StatusKilled { | |
| 806 | - | a.mu.Unlock() | |
| 803 | + | status, shutdownStarted := a.prepareShutdown(origin) | |
| 804 | + | if !shutdownStarted { | |
| 807 | 805 | f.mu.Unlock() | |
| 808 | 806 | f.operationMu.Unlock() | |
| 809 | 807 | return nil | |
| @@ -812,16 +810,12 @@ func (f *Supervisor) stop(ctx context.Context, id string, origin ShutdownOrigin) | |||
| 812 | 810 | path := a.InboxPath | |
| 813 | 811 | inbox := a.inbox | |
| 814 | 812 | pid := a.ProcessPIDValue() | |
| 815 | - | a.mu.Unlock() | |
| 816 | 813 | f.mu.Unlock() | |
| 817 | - | a.setShutdownOrigin(origin) | |
| 818 | 814 | // The detached worker may take the full grace period to exit. Do not | |
| 819 | 815 | // serialize unrelated supervisor operations for that entire wait. | |
| 820 | 816 | f.operationMu.Unlock() | |
| 821 | 817 | return f.stopDetached(ctx, a, path, inbox, pid, origin) | |
| 822 | 818 | } | |
| 823 | - | a.status = StatusKilled | |
| 824 | - | a.activity = "canceling" | |
| 825 | 819 | atomic.CompareAndSwapInt32(&a.launchState, 0, 2) | |
| 826 | 820 | if status == StatusPending { | |
| 827 | 821 | for i, queued := range f.queue { | |
| @@ -831,9 +825,7 @@ func (f *Supervisor) stop(ctx context.Context, id string, origin ShutdownOrigin) | |||
| 831 | 825 | } | |
| 832 | 826 | } | |
| 833 | 827 | } | |
| 834 | - | a.mu.Unlock() | |
| 835 | 828 | f.mu.Unlock() | |
| 836 | - | a.setShutdownOrigin(origin) | |
| 837 | 829 | f.operationMu.Unlock() | |
| 838 | 830 | a.setTurnState(TurnCanceling, a.CurrentTurnID()) | |
| 839 | 831 | ||
| @@ -800,10 +800,8 @@ func (f *Supervisor) stop(ctx context.Context, id string, origin ShutdownOrigin) | |||||
| 800 | // the pending cleanup below could then finalize an agent that was already | 800 | // the pending cleanup below could then finalize an agent that was already | ||
| 801 | // admitted to the runner. | 801 | // admitted to the runner. | ||
| 802 | f.mu.Lock() | 802 | f.mu.Lock() | ||
| 803 | - | a.mu.Lock() | 803 | + | status, shutdownStarted := a.prepareShutdown(origin) |
| 804 | - | status := a.status | 804 | + | if !shutdownStarted { |
| 805 | - | if status == StatusDone || status == StatusFailed || status == StatusKilled { | |||
| 806 | - | a.mu.Unlock() | |||
| 807 | f.mu.Unlock() | 805 | f.mu.Unlock() | ||
| 808 | f.operationMu.Unlock() | 806 | f.operationMu.Unlock() | ||
| 809 | return nil | 807 | return nil | ||
| @@ -812,16 +810,12 @@ func (f *Supervisor) stop(ctx context.Context, id string, origin ShutdownOrigin) | |||||
| 812 | path := a.InboxPath | 810 | path := a.InboxPath | ||
| 813 | inbox := a.inbox | 811 | inbox := a.inbox | ||
| 814 | pid := a.ProcessPIDValue() | 812 | pid := a.ProcessPIDValue() | ||
| 815 | - | a.mu.Unlock() | |||
| 816 | f.mu.Unlock() | 813 | f.mu.Unlock() | ||
| 817 | - | a.setShutdownOrigin(origin) | |||
| 818 | // The detached worker may take the full grace period to exit. Do not | 814 | // The detached worker may take the full grace period to exit. Do not | ||
| 819 | // serialize unrelated supervisor operations for that entire wait. | 815 | // serialize unrelated supervisor operations for that entire wait. | ||
| 820 | f.operationMu.Unlock() | 816 | f.operationMu.Unlock() | ||
| 821 | return f.stopDetached(ctx, a, path, inbox, pid, origin) | 817 | return f.stopDetached(ctx, a, path, inbox, pid, origin) | ||
| 822 | } | 818 | } | ||
| 823 | - | a.status = StatusKilled | |||
| 824 | - | a.activity = "canceling" | |||
| 825 | atomic.CompareAndSwapInt32(&a.launchState, 0, 2) | 819 | atomic.CompareAndSwapInt32(&a.launchState, 0, 2) | ||
| 826 | if status == StatusPending { | 820 | if status == StatusPending { | ||
| 827 | for i, queued := range f.queue { | 821 | for i, queued := range f.queue { | ||
| @@ -831,9 +825,7 @@ func (f *Supervisor) stop(ctx context.Context, id string, origin ShutdownOrigin) | |||||
| 831 | } | 825 | } | ||
| 832 | } | 826 | } | ||
| 833 | } | 827 | } | ||
| 834 | - | a.mu.Unlock() | |||
| 835 | f.mu.Unlock() | 828 | f.mu.Unlock() | ||
| 836 | - | a.setShutdownOrigin(origin) | |||
| 837 | f.operationMu.Unlock() | 829 | f.operationMu.Unlock() | ||
| 838 | a.setTurnState(TurnCanceling, a.CurrentTurnID()) | 830 | a.setTurnState(TurnCanceling, a.CurrentTurnID()) | ||
| 839 | 831 | ||||
M packages/agent/subagents/supervisor_test.go +81 -0
| @@ -190,6 +190,87 @@ func TestStopCancelsRunningAgent(t *testing.T) { | |||
| 190 | 190 | } | |
| 191 | 191 | } | |
| 192 | 192 | ||
| 193 | + | func TestStopPublishesShutdownOriginBeforeConcurrentFinalization(t *testing.T) { | |
| 194 | + | started := make(chan struct{}) | |
| 195 | + | finish := make(chan struct{}) | |
| 196 | + | finalizing := make(chan struct{}) | |
| 197 | + | f := newTestSupervisor(t, func(a *Agent) Runner { | |
| 198 | + | return RunnerFunc(func(context.Context, Sink) error { | |
| 199 | + | close(started) | |
| 200 | + | <-finish | |
| 201 | + | close(finalizing) | |
| 202 | + | return nil | |
| 203 | + | }) | |
| 204 | + | }) | |
| 205 | + | a, err := f.Spawn(context.Background(), "finish while stopping") | |
| 206 | + | if err != nil { | |
| 207 | + | t.Fatal(err) | |
| 208 | + | } | |
| 209 | + | <-started | |
| 210 | + | ||
| 211 | + | // Hold lifecycle finalization after Runner.Run returns. Shutdown must publish | |
| 212 | + | // its origin before exposing StatusKilled, without relying on finalization | |
| 213 | + | // ordering to fill the durable result correctly. | |
| 214 | + | a.lifecycleMu.Lock() | |
| 215 | + | lifecycleLocked := true | |
| 216 | + | finishClosed := false | |
| 217 | + | defer func() { | |
| 218 | + | if !finishClosed { | |
| 219 | + | close(finish) | |
| 220 | + | } | |
| 221 | + | if lifecycleLocked { | |
| 222 | + | a.lifecycleMu.Unlock() | |
| 223 | + | } | |
| 224 | + | }() | |
| 225 | + | stopDone := make(chan error, 1) | |
| 226 | + | go func() { stopDone <- f.stop(context.Background(), a.ID, ShutdownOriginSession) }() | |
| 227 | + | ||
| 228 | + | deadline := time.Now().Add(time.Second) | |
| 229 | + | for a.Status() != StatusKilled { | |
| 230 | + | if time.Now().After(deadline) { | |
| 231 | + | t.Fatal("shutdown did not publish killed status") | |
| 232 | + | } | |
| 233 | + | runtime.Gosched() | |
| 234 | + | } | |
| 235 | + | a.mu.Lock() | |
| 236 | + | origin := a.shutdownOrigin | |
| 237 | + | a.mu.Unlock() | |
| 238 | + | if origin != ShutdownOriginSession { | |
| 239 | + | t.Fatalf("shutdown origin = %q when killed became observable, want %q", origin, ShutdownOriginSession) | |
| 240 | + | } | |
| 241 | + | ||
| 242 | + | close(finish) | |
| 243 | + | finishClosed = true | |
| 244 | + | <-finalizing | |
| 245 | + | deadline = time.Now().Add(time.Second) | |
| 246 | + | for { | |
| 247 | + | a.mu.Lock() | |
| 248 | + | finalizationStarted := !a.finished.IsZero() | |
| 249 | + | a.mu.Unlock() | |
| 250 | + | if finalizationStarted { | |
| 251 | + | break | |
| 252 | + | } | |
| 253 | + | if time.Now().After(deadline) { | |
| 254 | + | t.Fatal("runner finalization did not start") | |
| 255 | + | } | |
| 256 | + | runtime.Gosched() | |
| 257 | + | } | |
| 258 | + | a.lifecycleMu.Unlock() | |
| 259 | + | lifecycleLocked = false | |
| 260 | + | if err := <-stopDone; err != nil { | |
| 261 | + | t.Fatal(err) | |
| 262 | + | } | |
| 263 | + | a.Wait() | |
| 264 | + | ||
| 265 | + | result := a.Result() | |
| 266 | + | if result == nil || result.Status != ResultCanceled || result.ShutdownOrigin != ShutdownOriginSession { | |
| 267 | + | t.Fatalf("final result = %#v, want attributed session cancellation", result) | |
| 268 | + | } | |
| 269 | + | if result.Error == nil || result.Error.Code != "shutdown" { | |
| 270 | + | t.Fatalf("final result error = %#v, want shutdown", result.Error) | |
| 271 | + | } | |
| 272 | + | } | |
| 273 | + | ||
| 193 | 274 | func TestStopByRootSessionLeavesOtherSessionsRunning(t *testing.T) { | |
| 194 | 275 | started := make(chan string, 2) | |
| 195 | 276 | stopped := make(chan string, 2) | |
| @@ -190,6 +190,87 @@ func TestStopCancelsRunningAgent(t *testing.T) { | |||||
| 190 | } | 190 | } | ||
| 191 | } | 191 | } | ||
| 192 | 192 | ||||
| 193 | + | func TestStopPublishesShutdownOriginBeforeConcurrentFinalization(t *testing.T) { | |||
| 194 | + | started := make(chan struct{}) | |||
| 195 | + | finish := make(chan struct{}) | |||
| 196 | + | finalizing := make(chan struct{}) | |||
| 197 | + | f := newTestSupervisor(t, func(a *Agent) Runner { | |||
| 198 | + | return RunnerFunc(func(context.Context, Sink) error { | |||
| 199 | + | close(started) | |||
| 200 | + | <-finish | |||
| 201 | + | close(finalizing) | |||
| 202 | + | return nil | |||
| 203 | + | }) | |||
| 204 | + | }) | |||
| 205 | + | a, err := f.Spawn(context.Background(), "finish while stopping") | |||
| 206 | + | if err != nil { | |||
| 207 | + | t.Fatal(err) | |||
| 208 | + | } | |||
| 209 | + | <-started | |||
| 210 | + | ||||
| 211 | + | // Hold lifecycle finalization after Runner.Run returns. Shutdown must publish | |||
| 212 | + | // its origin before exposing StatusKilled, without relying on finalization | |||
| 213 | + | // ordering to fill the durable result correctly. | |||
| 214 | + | a.lifecycleMu.Lock() | |||
| 215 | + | lifecycleLocked := true | |||
| 216 | + | finishClosed := false | |||
| 217 | + | defer func() { | |||
| 218 | + | if !finishClosed { | |||
| 219 | + | close(finish) | |||
| 220 | + | } | |||
| 221 | + | if lifecycleLocked { | |||
| 222 | + | a.lifecycleMu.Unlock() | |||
| 223 | + | } | |||
| 224 | + | }() | |||
| 225 | + | stopDone := make(chan error, 1) | |||
| 226 | + | go func() { stopDone <- f.stop(context.Background(), a.ID, ShutdownOriginSession) }() | |||
| 227 | + | ||||
| 228 | + | deadline := time.Now().Add(time.Second) | |||
| 229 | + | for a.Status() != StatusKilled { | |||
| 230 | + | if time.Now().After(deadline) { | |||
| 231 | + | t.Fatal("shutdown did not publish killed status") | |||
| 232 | + | } | |||
| 233 | + | runtime.Gosched() | |||
| 234 | + | } | |||
| 235 | + | a.mu.Lock() | |||
| 236 | + | origin := a.shutdownOrigin | |||
| 237 | + | a.mu.Unlock() | |||
| 238 | + | if origin != ShutdownOriginSession { | |||
| 239 | + | t.Fatalf("shutdown origin = %q when killed became observable, want %q", origin, ShutdownOriginSession) | |||
| 240 | + | } | |||
| 241 | + | ||||
| 242 | + | close(finish) | |||
| 243 | + | finishClosed = true | |||
| 244 | + | <-finalizing | |||
| 245 | + | deadline = time.Now().Add(time.Second) | |||
| 246 | + | for { | |||
| 247 | + | a.mu.Lock() | |||
| 248 | + | finalizationStarted := !a.finished.IsZero() | |||
| 249 | + | a.mu.Unlock() | |||
| 250 | + | if finalizationStarted { | |||
| 251 | + | break | |||
| 252 | + | } | |||
| 253 | + | if time.Now().After(deadline) { | |||
| 254 | + | t.Fatal("runner finalization did not start") | |||
| 255 | + | } | |||
| 256 | + | runtime.Gosched() | |||
| 257 | + | } | |||
| 258 | + | a.lifecycleMu.Unlock() | |||
| 259 | + | lifecycleLocked = false | |||
| 260 | + | if err := <-stopDone; err != nil { | |||
| 261 | + | t.Fatal(err) | |||
| 262 | + | } | |||
| 263 | + | a.Wait() | |||
| 264 | + | ||||
| 265 | + | result := a.Result() | |||
| 266 | + | if result == nil || result.Status != ResultCanceled || result.ShutdownOrigin != ShutdownOriginSession { | |||
| 267 | + | t.Fatalf("final result = %#v, want attributed session cancellation", result) | |||
| 268 | + | } | |||
| 269 | + | if result.Error == nil || result.Error.Code != "shutdown" { | |||
| 270 | + | t.Fatalf("final result error = %#v, want shutdown", result.Error) | |||
| 271 | + | } | |||
| 272 | + | } | |||
| 273 | + | ||||
| 193 | func TestStopByRootSessionLeavesOtherSessionsRunning(t *testing.T) { | 274 | func TestStopByRootSessionLeavesOtherSessionsRunning(t *testing.T) { | ||
| 194 | started := make(chan string, 2) | 275 | started := make(chan string, 2) | ||
| 195 | stopped := make(chan string, 2) | 276 | stopped := make(chan string, 2) | ||