diff --git a/apps/backend/internal/agent/docker/client.go b/apps/backend/internal/agent/docker/client.go index b0e28b2a49..d35eadf108 100644 --- a/apps/backend/internal/agent/docker/client.go +++ b/apps/backend/internal/agent/docker/client.go @@ -422,11 +422,9 @@ func (c *Client) RemoveContainer(ctx context.Context, containerID string, force // removed the container already, which is the desired end state. return nil } - if errdefs.IsConflict(err) && strings.Contains(strings.ToLower(err.Error()), "already in progress") { - // Docker rejects a concurrent ContainerRemove with a conflict while - // the first removal is still completing. The other owner has already - // taken responsibility for this container's teardown, so treat this - // response as the same converged end state as a not-found response. + if containerRemovalInProgress(err) { + // A concurrent cleanup owner is already removing the container. Treat + // this conflict as success because both callers converge on removal. return nil } c.logger.Error("Failed to remove container", zap.String("container_id", containerID), zap.Error(err)) @@ -437,6 +435,16 @@ func (c *Client) RemoveContainer(ctx context.Context, containerID string, force return nil } +func containerRemovalInProgress(err error) bool { + if !errdefs.IsConflict(err) { + return false + } + + message := strings.ToLower(err.Error()) + return strings.Contains(message, "removal of container") && + strings.Contains(message, "already in progress") +} + func (c *Client) containerRemover() containerRemover { if c.remover != nil { return c.remover diff --git a/apps/backend/internal/orchestrator/event_handlers_agent.go b/apps/backend/internal/orchestrator/event_handlers_agent.go index 810a8bcbb9..cf8aef6c0c 100644 --- a/apps/backend/internal/orchestrator/event_handlers_agent.go +++ b/apps/backend/internal/orchestrator/event_handlers_agent.go @@ -1540,7 +1540,10 @@ func (s *Service) RegisterExecutionStopOwner(sessionID, executionID string, forc return } lock, release := s.acquireCancelInFlightGuard(sessionID) - lock.Lock() + if !lock.TryLock() { + release() + return + } defer func() { lock.Unlock() release() diff --git a/apps/backend/internal/orchestrator/execution_teardown_ownership_test.go b/apps/backend/internal/orchestrator/execution_teardown_ownership_test.go index c226ea82cc..72aeab1b68 100644 --- a/apps/backend/internal/orchestrator/execution_teardown_ownership_test.go +++ b/apps/backend/internal/orchestrator/execution_teardown_ownership_test.go @@ -54,6 +54,49 @@ func TestRegisterExecutionStopOwner_SuppressesOrphanCleanupAndRecordsForceEscala require.Equal(t, executionTeardownIntentForce, claim.intent) } +// @covers AC-TASKS-RUNTIME-CLEANUP-001.1 +func TestRegisterExecutionStopOwner_ContendedGuardDoesNotBlock(t *testing.T) { + svc := newCoordinatorStopTestService( + setupTestRepo(t), + newMockTaskRepo(), + &mockAgentManager{}, + ) + const ( + sessionID = "session-contended" + executionID = "execution-contended" + ) + guard, release := svc.acquireCancelInFlightGuard(sessionID) + guardLocked := false + t.Cleanup(func() { + if guardLocked { + guard.Unlock() + } + release() + }) + guard.Lock() + guardLocked = true + + started := make(chan struct{}) + done := make(chan struct{}) + go func() { + close(started) + svc.RegisterExecutionStopOwner(sessionID, executionID, false) + close(done) + }() + <-started + + select { + case <-done: + _, claimed := svc.executionTeardownClaims.Load(terminalExecutionKey(sessionID, executionID)) + require.False(t, claimed) + case <-time.After(time.Second): + guard.Unlock() + guardLocked = false + <-done + t.Fatal("advisory stop-owner registration blocked on the session guard") + } +} + func TestHandleAgentStopped_OwnedPredecessorDoesNotCancelStartingReplacement(t *testing.T) { ctx := context.Background() repo := setupTestRepo(t) diff --git a/docs/plans/nonblocking-stop-owner-registration/plan.md b/docs/plans/nonblocking-stop-owner-registration/plan.md new file mode 100644 index 0000000000..7e08bde582 --- /dev/null +++ b/docs/plans/nonblocking-stop-owner-registration/plan.md @@ -0,0 +1,79 @@ +--- +created: 2026-08-28 +status: done +requirements: + - REQ-TASKS-RUNTIME-CLEANUP-001 +system_design: + - ../../specs/tasks/system-design/runtime-cleanup.md +legacy_specs: [] +--- + +# Implementation Plan: Non-blocking Stop-Owner Registration + +## Overview + +Make advisory stop-owner registration non-blocking when another operation owns +the session guard. Add a focused regression test before the production change. + +## Scope + +### In scope + +- Prevent `RegisterExecutionStopOwner` from waiting on a contended session guard. +- Preserve teardown claims and force escalation when the guard is available. +- Treat an overlapping Docker container removal as an idempotent cleanup result. +- Add deterministic concurrency coverage for the contended path. + +### Out of scope + +- Finding or changing every possible long-running owner of `cancelInFlightGuard`. +- Changing queue dispatch, session state, or durable cleanup scheduling. +- Changing HTTP, WebSocket, frontend, or public documentation contracts. + +## Technical approach + +Update `RegisterExecutionStopOwner` in +`apps/backend/internal/orchestrator/event_handlers_agent.go`. Use `TryLock` and +release the guard reference when the lock is not available. Keep the existing +claim and force-escalation logic unchanged after successful lock acquisition. + +Add a channel-based regression test in +`apps/backend/internal/orchestrator/execution_teardown_ownership_test.go`. Hold +the guard, call registration in another goroutine, and require registration to +return before the guard is released. + +Make `Client.RemoveContainer` converge when Docker reports that removal of the +same container is already in progress. The non-blocking registration can allow +the explicit stop and durable cleanup owners to overlap, and Docker's conflict +response represents the desired removal state rather than a failed cleanup. +Cover this response with a focused Docker client test. + +## Tests + +- `AC-TASKS-RUNTIME-CLEANUP-001.1`: The contended registration test proves that + an advisory claim cannot stall the explicit runtime stop path. +- Existing stop-owner tests prove that uncontended registration still suppresses + duplicate orphan cleanup and preserves force escalation. + +## Work orders + +- [x] [Task 01: Make stop-owner registration non-blocking](task-01-nonblocking-stop-owner-registration.md) + +## Verification results + +- `go test -tags fts5 -run 'TestRegisterExecutionStopOwner_' ./internal/orchestrator -count=1 -v`: passed, 2 tests. +- `go test -race -tags fts5 -run 'TestRegisterExecutionStopOwner_' ./internal/orchestrator -count=1`: passed, 2 tests. +- CodeRabbit follow-up: registered the test guard cleanup with `t.Cleanup`; the + focused normal and race tests still pass. +- CI-only `TestPortProxyCapabilityRoundTrip` failure was reproduced locally with + `-race` and passed, so no unrelated gateway change was made. +- The containers E2E exposed an overlapping Docker teardown race. The exact + archive-and-remove test reproduced the failure locally; treating Docker's + "removal already in progress" conflict as convergence fixed the race. + +## Risks + +- A skipped advisory claim can permit duplicate teardown attempts. Runtime stop + and durable cleanup already require idempotent behavior for this condition. +- The regression test needs a bounded deadline only to fail a blocking build. + Channels control the ownership boundary and cleanup order. diff --git a/docs/plans/nonblocking-stop-owner-registration/task-01-nonblocking-stop-owner-registration.md b/docs/plans/nonblocking-stop-owner-registration/task-01-nonblocking-stop-owner-registration.md new file mode 100644 index 0000000000..84190b2ae4 --- /dev/null +++ b/docs/plans/nonblocking-stop-owner-registration/task-01-nonblocking-stop-owner-registration.md @@ -0,0 +1,91 @@ +--- +id: "01-nonblocking-stop-owner-registration" +title: "Make stop-owner registration non-blocking" +status: done +wave: 1 +depends_on: [] +plan: "plan.md" +requirements: + - REQ-TASKS-RUNTIME-CLEANUP-001 +acceptance_criteria: + - AC-TASKS-RUNTIME-CLEANUP-001.1 +system_design: + - ../../specs/tasks/system-design/runtime-cleanup.md +--- + +# Task 01: Make Stop-Owner Registration Non-blocking + +## Summary + +Prevent advisory teardown registration from waiting on a contended session +guard. Preserve existing registration behavior when the guard is available. + +## In scope + +- Add a failing concurrency regression test for contended registration. +- Replace the blocking registration lock with a non-blocking lock attempt. +- Preserve reference release, teardown claims, and force escalation. +- Make overlapping Docker removal requests converge successfully. + +## Out of scope + +- Change guard ownership in queue or cancellation operations. +- Change runtime stop scheduling or durable cleanup retry policy. +- Add browser or end-to-end coverage. + +## Acceptance + +- Contended stop-owner registration returns before the guard owner releases the + lock. +- Uncontended registration still records the exact teardown claim. +- A repeated force registration still upgrades a graceful claim. + +## Verification + +```bash +go test -tags fts5 -run 'TestRegisterExecutionStopOwner_' ./internal/orchestrator -count=1 -v +go test -race -tags fts5 -run 'TestRegisterExecutionStopOwner_' ./internal/orchestrator -count=1 +``` + +Run these commands from `apps/backend`. + +## Files likely touched + +- `apps/backend/internal/orchestrator/event_handlers_agent.go` +- `apps/backend/internal/orchestrator/execution_teardown_ownership_test.go` +- `apps/backend/internal/agent/docker/client.go` +- `apps/backend/internal/agent/docker/storage_test.go` + +## Dependencies + +None. + +## Risks + +- A skipped claim can permit an idempotent duplicate teardown attempt. + +## Parallelism + +`sequential` + +## Inputs + +- `AC-TASKS-RUNTIME-CLEANUP-001.1` +- Task Runtime Cleanup System Design sections about stop-owner registration. +- Existing `RegisterExecutionStopOwner` claim and force-escalation tests. + +## Results + +- Added `TestRegisterExecutionStopOwner_ContendedGuardDoesNotBlock` as the + regression test for `AC-TASKS-RUNTIME-CLEANUP-001.1`. +- Replaced the blocking registration lock with `TryLock`. A contended call now + releases its guard reference and returns without a teardown claim. +- The focused test failed before the production change and passed after it. +- Both work-order verification commands passed. The race command reported no + data races. +- Applied the CodeRabbit follow-up by registering guard cleanup with `t.Cleanup`. +- Reproduced the unrelated CI failure in `TestPortProxyCapabilityRoundTrip`; + the local `-race` run passed, so the gateway code remains unchanged. +- Reproduced the containers E2E archive cleanup failure with overlapping Docker + removal owners. `Client.RemoveContainer` now treats Docker's conflict for an + already-in-progress removal as successful convergence, with focused coverage. diff --git a/docs/specs/tasks/system-design/runtime-cleanup.md b/docs/specs/tasks/system-design/runtime-cleanup.md index 4cc476626d..5e43247cfe 100644 --- a/docs/specs/tasks/system-design/runtime-cleanup.md +++ b/docs/specs/tasks/system-design/runtime-cleanup.md @@ -4,7 +4,7 @@ system: tasks requirements: - REQ-TASKS-RUNTIME-CLEANUP-001 created: 2026-06-22 -updated: 2026-08-27 +updated: 2026-08-28 owners: - cfl --- @@ -114,6 +114,10 @@ worktrees, or executor rows behind and the machine slowly runs out of memory. - Archive, delete, cascade, workspace-delete, and quick-chat expiration persist a cleanup intent and resource snapshot before mutating or deleting task state. Cleanup is performed by a durable worker rather than a detached goroutine. +- Stop-owner registration is advisory. It uses a non-blocking attempt to acquire + the per-session `cancelInFlightGuard` before it records teardown ownership. +- If that guard is in use, registration skips the claim and the requested stop + continues. Durable cleanup and idempotent runtime teardown handle later races. - A durable cleanup job makes at most eight attempts. Failed attempts back off for 1 minute, 5 minutes, 15 minutes, 1 hour, 3 hours, 6 hours, and 12 hours before the next claim. An eighth failed attempt becomes terminal `failed` @@ -137,32 +141,22 @@ worktrees, or executor rows behind and the machine slowly runs out of memory. ## Archive cleanup disposition -The cleanup job trigger defines the resource disposition. Direct archive and -cascade archive use the same disposition. A caller-specific Boolean must not -change that disposition. - -Archive cleanup performs these actions: +Direct and cascade archive use the same cleanup disposition. A caller-specific +Boolean must not change it: - Stop the task runtimes and remove executor-specific runtime resources. - Remove the physical worktree directory and mark its repository row deleted. -- Preserve the owning `task_environments` row. -- Preserve every `task_environment_repos` row, including its worktree ID, - path, branch, and branch slug. -- Preserve the local Git branch ref. This keeps committed work recoverable when - a hosting provider deletes the remote branch after merge. +- Preserve the owning `task_environments` row, every + `task_environment_repos` row (including worktree ID, path, branch, and slug), + and the local Git branch ref. -The worktree can appear in both the environment snapshot and the batch -worktree snapshot. Both cleanup passes must use the archive disposition. A -later pass must not delete a branch that an earlier pass preserved. +If a worktree appears in both snapshots, every pass uses this disposition and +must not delete a branch preserved earlier. -Unarchive does not invent a new workspace owner. It keeps the preserved -environment link. When the repository row is not live, resume enters normal -preparation and reactivates that row. The recreated worktree uses the preserved -local branch first, then the existing remote or pull-request recovery paths. - -Delete behavior remains separate. A delete trigger can remove durable owner -rows after it captures the cleanup snapshot. This archive rule does not weaken -delete cleanup. +Unarchive keeps the preserved environment link. If its repository row is not +live, normal preparation reactivates it and uses the preserved local branch +before remote or pull-request recovery. Delete remains separate and may remove +owner rows after capturing its cleanup snapshot. ## Data Model @@ -260,6 +254,10 @@ type ExecutorRunningRepository interface { primary stop operation when `agent_execution_id` is available. Fallback cleanup is runtime-specific and must be bounded by context. +`TaskExecutionStopper.RegisterExecutionStopOwner(sessionID, executionID, force)` +must not wait for the per-session cancellation guard. Registration can skip a +contended claim because it is advisory and does not replace the stop operation. + ## State Machine Runtime cleanup for a task follows this lifecycle: @@ -301,6 +299,8 @@ The durable cleanup job wraps that resource lifecycle: the task only if existing product behavior requires it, but destructive runtime cleanup must fail closed: do not remove runtime rows or worktrees based on an empty or partial inventory. +- If stop-owner registration finds the session guard in use, it skips the claim. + The explicit stop continues, and durable cleanup preserves any retryable work. - If stopping a runtime execution times out, the process manager escalates to a process-group kill and waits for confirmation. If confirmation still fails, the runtime row remains retryable.