fix(backend): unblock advisory stop-owner registration (#3107) (#3116)

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

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()

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)

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.

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.

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.