Repository navigation
Scope worker cancellation and closure acknowledgements to executions - #163
Conversation
Co-Authored-By: Warp <agent@warp.dev>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| return executeError(newBackendFailure(metrics.TaskFailurePhaseBackend, metrics.TaskFailureReasonWorkspaceSetup, fmt.Errorf("invalid execution ID for workspace path: %w", err))) | ||
| } | ||
| var err error | ||
| workspaceDir, err = os.MkdirTemp(b.config.WorkspaceRoot, taskID+"-"+params.ExecutionID+"-") |
There was a problem hiding this comment.
I think this is a false positive. Both IDs are validated as single path components before reaching os.MkdirTemp. The validator rejects /, , empty values, and ./.., and failures return immediately. The IDs affect only the temporary directory’s filename pattern; the parent is the operator-configured WorkspaceRoot. They cannot introduce an absolute path or parent-directory traversal.
| return executeError(newBackendFailure(metrics.TaskFailurePhaseBackend, metrics.TaskFailureReasonWorkspaceSetup, fmt.Errorf("invalid execution ID for workspace path: %w", err))) | ||
| } | ||
| var err error | ||
| workspaceDir, err = os.MkdirTemp(b.config.WorkspaceRoot, taskID+"-"+params.ExecutionID+"-") |
There was a problem hiding this comment.
we're incorporating ExecutionID here so that, if a worker gets assigned a second execution for the same task, it starts in a fresh directory?
There was a problem hiding this comment.
Yes, will add a comment
| platform string // Docker daemon platform (e.g., "linux/amd64" or "linux/arm64") | ||
| platformSpec ocispec.Platform | ||
| sidecarVolumes sync.Map // volume name -> *sidecarVolumeState | ||
| taskContainers sync.Map // taskExecution -> container ID; retained after cleanup failures |
There was a problem hiding this comment.
can this grow without bound?
There was a problem hiding this comment.
Under normal cleanup, entries are removed when the container is removed. There’s an avoidable accumulation with NoCleanup; I’ll drop tracking once the container is confirmed exited, while preserving the container itself.
Failed cleanup and uncertain creation outcomes can still accumulate entries—we retain those so cancellation doesn’t silently lose track of potentially running containers. But this isn't the only example of potential unbounded growth in the worker and is uncommon
| } | ||
| key := taskExecution{cancellation.TaskID, cancellation.ExecutionID} | ||
| if cancellation.ExecutionID == "" { | ||
| // A task-only request is explicitly legacy run-wide cancellation. |
There was a problem hiding this comment.
is this legacy logic something that we'll be able to remove once the server support for execution-scoped cancellation rolls out?
There was a problem hiding this comment.
Yes, added a todo
3 participants
Scope cancellation to an execution so delayed cleanup cannot stop a replacement execution of the same run. Treat successful backend cancellation as an accepted shutdown request; do not wait for or acknowledge confirmed physical closure.
Review guide
Start with
internal/worker/worker.goand the message types, then review execution targeting in the Docker, Kubernetes, Direct, and Command backends. Focus on delayed cancellation/completion, provisioning races, and retries of failed backend operations.What changed
execution-scoped-cancellation-v1capability and carry execution identity in assignment, claim, completion/failure, and cancellation handling.execution_closedmessage, closure-verification API, or physical-closure polling.Compatibility and limits
The protocol remains compatible with legacy run-only cancellation requests. This capability is an optional improvement, not a prerequisite for deploying the server change. The server supports older worker deployments through guarded run-ID cancellation and preserves early handoff.
A successful cancellation does not prove all descendants or customer-managed runtime resources have exited; completion is delegated to the backend/operator. Tracking and failed-operation retry state survive WebSocket reconnects, not worker-process restarts. Legacy run-ID cancellation cannot provide the same execution isolation as scoped cancellation.
Validation
go test ./...go test -race ./...go vet ./...,golangci-lint run --allow-serial-runners(0 issues),go build -v ./..., and diff whitespace checksAll passed for the simplified accepted-request implementation. Backend tests use fakes; these results do not establish live customer-infrastructure closure.
Co-Authored-By: Warp agent@warp.dev