thwap/thwap-pagesd

Watch

Fork 0

feat(docker): implement site container removal #658

Merged

fuzzy merged 1 commit from feat/docker-container-remove into main

2026-08-14 07:10:58 +00:00

automation commented

2026-08-14 04:06:48 +00:00

Owner

What

Implement site container removal for standalone mode (Phase 2). Adds the container-removal primitive to internal/docker.

Stop and remove (#350) — StandaloneDeployer.RemoveSiteContainer stops the container gracefully (ContainerStop), then removes it (ContainerRemove with force), for a production deployment (site+version) or a preview (site+branch).
Prune (#351) — a Prune option also removes the site's host directory (os.RemoveAll of SiteDir), matching the deployment model's storage resolution where "volume cleanup" maps to unused host-directory removal (standalone uses no Docker named volumes). --prune in the roadmap wording becomes the primitive's Prune flag; the CLI flag lands with Phase 7 cleanup commands (#460).
Not-found handling (#352) — missing containers are treated as success via errdefs.IsNotFound (stop 404 and remove 404 → nil), giving idempotent removal.

github.com/containerd/errdefs promoted from indirect to a direct dependency (used for not-found detection).

StandaloneDeployer.DeleteSite/DeletePreview remain not-implemented; this primitive is consumed by Phase 4 site operations.

Why

Phase 2 container lifecycle: creation (#349) now has its removal counterpart (#353), covering the lifecycle Remove stage (site deletion, tag removal, explicit cleanup) defined in the deployment model.

Testing

internal/docker — fake daemon answers POST /containers/{id}/stop and DELETE /containers/{id}
Production removal (name tpagectl-blog-v1.2.3) and preview removal (tpagectl-blog-preview-fix-nav) — stop + remove both called
Not-found idempotency: stop 404 + remove 404 → nil
Stop error and remove error propagation
Prune removes the host site directory; missing dir tolerated
go vet ./... clean; go test -race -count=1 ./... full suite, no failures
Pre-commit hooks pass (golangci-lint, go fmt, go mod tidy, go test, gitleaks)

Breaking Changes

None. New primitive; errdefs promoted to direct require (no version change).

Notes

ContainerRemove(Force:true) handles both running and stopped containers; the explicit ContainerStop preserves the graceful-stop behavior from the design doc's "stopped and removed" lifecycle stage. Removing an already-stopped container returns 304 from Docker, which the SDK treats as success.

Closes #350
Closes #351
Closes #352
Closes #353

## What Implement site container removal for standalone mode (Phase 2). Adds the container-removal primitive to `internal/docker`. - **Stop and remove (#350)** — `StandaloneDeployer.RemoveSiteContainer` stops the container gracefully (`ContainerStop`), then removes it (`ContainerRemove` with force), for a production deployment (site+version) or a preview (site+branch). - **Prune (#351)** — a `Prune` option also removes the site's host directory (`os.RemoveAll` of `SiteDir`), matching the deployment model's storage resolution where "volume cleanup" maps to unused host-directory removal (standalone uses no Docker named volumes). `--prune` in the roadmap wording becomes the primitive's `Prune` flag; the CLI flag lands with Phase 7 cleanup commands (#460). - **Not-found handling (#352)** — missing containers are treated as success via `errdefs.IsNotFound` (stop 404 and remove 404 → nil), giving idempotent removal. `github.com/containerd/errdefs` promoted from indirect to a direct dependency (used for not-found detection). `StandaloneDeployer.DeleteSite`/`DeletePreview` remain not-implemented; this primitive is consumed by Phase 4 site operations. ## Why Phase 2 container lifecycle: creation (#349) now has its removal counterpart (#353), covering the lifecycle Remove stage (site deletion, tag removal, explicit cleanup) defined in the deployment model. ## Testing - [x] `internal/docker` — fake daemon answers `POST /containers/{id}/stop` and `DELETE /containers/{id}` - [x] Production removal (name `tpagectl-blog-v1.2.3`) and preview removal (`tpagectl-blog-preview-fix-nav`) — stop + remove both called - [x] Not-found idempotency: stop 404 + remove 404 → nil - [x] Stop error and remove error propagation - [x] Prune removes the host site directory; missing dir tolerated - [x] `go vet ./...` clean; `go test -race -count=1 ./...` full suite, no failures - [x] Pre-commit hooks pass (golangci-lint, go fmt, go mod tidy, go test, gitleaks) ## Breaking Changes None. New primitive; `errdefs` promoted to direct require (no version change). ## Notes `ContainerRemove(Force:true)` handles both running and stopped containers; the explicit `ContainerStop` preserves the graceful-stop behavior from the design doc's "stopped and removed" lifecycle stage. Removing an already-stopped container returns 304 from Docker, which the SDK treats as success. Closes #350 Closes #351 Closes #352 Closes #353

automation added 1 commit

2026-08-14 04:06:48 +00:00

feat(docker): implement site container removal

CI / test (pull_request) Successful in 7m27s

Details

CI / build (pull_request) Successful in 2m17s

Details

CI / docker (pull_request) Successful in 3m20s

Details

CI / lint (pull_request) Successful in 6m44s

Details

CI / test (push) Successful in 6m48s

Details

CI / lint (push) Successful in 7m4s

Details

CI / build (push) Successful in 7m59s

Details

CI / docker (push) Successful in 30m14s

Details

f648566126

Add the container-removal primitive for standalone mode: stop and remove
a site deployment's container, tolerating missing containers.

- docker: StandaloneDeployer.RemoveSiteContainer stops the container
  (graceful), removes it with force, and returns nil when the container
  does not exist (idempotent removal); a Prune option removes the site's
  host directory for final cleanup
- go.mod: promote github.com/containerd/errdefs to a direct dependency
  (used for not-found detection)
- tests: fake daemon answers stop/remove; covers production and preview
  removal, not-found idempotency, stop/remove errors, and host-directory
  pruning (including a missing dir)
- DOCKER-ROADMAP.md: mark #353 and subtasks complete

Closes #350
Closes #351
Closes #352
Closes #353

automation added the

Kind/Feature

label

2026-08-14 04:06:52 +00:00

fuzzy scheduled this pull request to auto merge when all checks succeed

2026-08-14 04:07:43 +00:00

the.auditor referenced this pull request

2026-08-14 07:10:46 +00:00

docker: guard RemoveSiteContainer prune against path escape #659

the.auditor referenced this pull request

2026-08-14 07:10:47 +00:00

docker: cover stop-404 idempotency branch #660

the.auditor approved these changes

2026-08-14 07:10:55 +00:00

the.auditor left a comment

Correct removal primitive. Verified against the vendored SDK that HTTP errors are classified by status code via httpErrorFromStatusCode → errdefs (client/request.go:199, client/errors.go:108-127), so errdefs.IsNotFound reliably matches both the fake and real daemon 404s; the 304 on stopping an already-stopped container falls in the success range → nil. Ordering (stop → remove → prune, early return on real errors) preserves site data when removal fails, and prune only runs after the container is gone. errdefs promotion is consistent with vendor/modules.txt (already ## explicit). Referenced issues #350–#353 are all closed and the roadmap checkboxes match.

Suggestions

internal/docker/container.go RemoveSiteContainer — os.RemoveAll(opts.SiteDir) is a recursive destructive operation with no containment guard; a wrong path (base_dir root, /, etc.) would delete arbitrarily. Add a filepath.Rel containment check against d.baseDir before pruning. Filed as #659.
internal/docker/container_test.go — TestRemoveSiteContainerNotFound only exercises the remove-404 branch (stopStatus defaults to 204); the ContainerStop 404 → nil branch is untested and the comment misstates the stop behavior. Add a stopStatus: 404 case and fix the comment. Filed as #660.

No blocking issues. Approving.

Correct removal primitive. Verified against the vendored SDK that HTTP errors are classified by status code via `httpErrorFromStatusCode` → `errdefs` (`client/request.go:199`, `client/errors.go:108-127`), so `errdefs.IsNotFound` reliably matches both the fake and real daemon 404s; the 304 on stopping an already-stopped container falls in the success range → nil. Ordering (stop → remove → prune, early return on real errors) preserves site data when removal fails, and prune only runs after the container is gone. `errdefs` promotion is consistent with `vendor/modules.txt` (already `## explicit`). Referenced issues #350–#353 are all closed and the roadmap checkboxes match. ## Suggestions 1. `internal/docker/container.go` `RemoveSiteContainer` — `os.RemoveAll(opts.SiteDir)` is a recursive destructive operation with no containment guard; a wrong path (base_dir root, `/`, etc.) would delete arbitrarily. Add a `filepath.Rel` containment check against `d.baseDir` before pruning. Filed as #659. 2. `internal/docker/container_test.go` — `TestRemoveSiteContainerNotFound` only exercises the remove-404 branch (`stopStatus` defaults to 204); the `ContainerStop` 404 → nil branch is untested and the comment misstates the stop behavior. Add a `stopStatus: 404` case and fix the comment. Filed as #660. No blocking issues. Approving.

fuzzy merged commit f648566126 into main

2026-08-14 07:10:58 +00:00

fuzzy deleted branch feat/docker-container-remove

2026-08-14 07:10:58 +00:00