fix(shim): reject task creation during shutdown - #280
Conversation
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
This PR tightens shim lifecycle behavior by preventing new initial-task creation once the shim begins retiring, serializing Create against terminal lifecycle transitions, and surfacing shutdown callback failures to callers.
Changes:
- Reject
CreatewithUnavailableonce the shim is retiring. - Serialize
Createwith terminalDelete/Shutdownvia a lifecycle mutex and retiring flag. - Add tests covering
Createduring shutdown and propagating shutdown stop errors.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| internal/shim/task/service_lifecycle_test.go | Adds lifecycle regression tests plus test doubles for controlled shutdown and VM lifecycle. |
| internal/shim/task/service.go | Introduces lifecycle serialization (lifecycleMu/retiring), blocks Create during shutdown, and returns shutdown callback errors. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
c12a2a9 to
5e51e20
Compare
|
|
||
| // Delete the initial process and container | ||
| func (s *service) Delete(ctx context.Context, r *taskAPI.DeleteRequest) (*taskAPI.DeleteResponse, error) { | ||
| if r.ExecID == "" { |
There was a problem hiding this comment.
Do we need to handle this case here? Shutdown is the actual start of the teardown process and as-is, this is incompatible with #254 where deleting a container does not mean tearing down the VM.
There was a problem hiding this comment.
Addressed, thanks for pointing this out @Kern--
5e51e20 to
cd80bc6
Compare
Make the task service single-generation once shutdown begins, so replacement creation cannot race teardown of the old VM. Propagate shutdown callback failures to the caller. Signed-off-by: Esteban Ginez <esteban.ginez@docker.com>
cd80bc6 to
bc4fad0
Compare
|
|
||
| s.lifecycleMu.Lock() | ||
| s.retiring = true | ||
| s.lifecycleMu.Unlock() |
austinvazquez
left a comment
austinvazquez
left a comment
There was a problem hiding this comment.
LGTM, just need to address that conflict with the 1:many feature.
Summary
Unavailableafter the shim begins retiring.Testing
go test -count=1 ./api/... ./cmd/... ./internal/... ./pkg/... ./plugins/...docker buildx bake validate