The pinned alpine/git image declares a volume at /git, so Docker gives each clone container an anonymous volume. The clone container was removed with only Force, so every deploy left one volume behind. It is now removed with RemoveVolumes too. The removal is deferred and uses a context that is not cancelled with the deploy, so it runs whether the clone succeeds, fails, or the deploy is cancelled mid-clone (a newer push or the build timeout).
A new test in internal/docker/validation_test.go runs a clone against a fake Docker API (no daemon needed) and checks the removal asks for volumes to be removed, for a successful, a failed and a cancelled clone. Without the fix it fails.
Manual check: a real clone container of the pinned image leaves no volume behind with the fix, and does without it; recorded in #217 (comment).
Judgement call: the test lives in validation_test.go rather than a new file, because a new internal-package test file would need a new nolint directive.
Out of scope: when a redeploy removes the old app container, its anonymous volumes (only present if the app's own image declares a volume) are still kept; deleting those would discard app data, so that is left as is.
Model: opus-5-5
Fixes https://git.eeqj.de/sneak/upaas/issues/215.
The pinned `alpine/git` image declares a volume at `/git`, so Docker gives each clone container an anonymous volume. The clone container was removed with only `Force`, so every deploy left one volume behind. It is now removed with `RemoveVolumes` too. The removal is deferred and uses a context that is not cancelled with the deploy, so it runs whether the clone succeeds, fails, or the deploy is cancelled mid-clone (a newer push or the build timeout).
A new test in `internal/docker/validation_test.go` runs a clone against a fake Docker API (no daemon needed) and checks the removal asks for volumes to be removed, for a successful, a failed and a cancelled clone. Without the fix it fails.
Manual check: a real clone container of the pinned image leaves no volume behind with the fix, and does without it; recorded in https://git.eeqj.de/sneak/upaas/pulls/217#issuecomment-102463.
Judgement call: the test lives in `validation_test.go` rather than a new file, because a new internal-package test file would need a new nolint directive.
Out of scope: when a redeploy removes the old app container, its anonymous volumes (only present if the app's own image declares a volume) are still kept; deleting those would discard app data, so that is left as is.
Model: opus-5-5
internal/docker/client.go, deferred ContainerRemove in performClone: the removal uses the deploy context. When a deploy is cancelled during the clone (a newer push cancels it, or the build timeout expires), that context is already done, the remove request never reaches Docker, and the clone container and its volume are both left behind. The definition of done in #215 requires no volume left behind when a deploy fails, and the PR body and commit message say the removal happens on failure. Acceptable: do the removal with a context that outlives cancellation (context.WithoutCancel(ctx), as deploy.go already does for its cleanup work), and add a test case that cancels the context while the clone is running and checks the container is still removed with its volumes.
Judgement calls: keeping the new test in validation_test.go to avoid a new nolint directive is sound. Leaving the old app container's anonymous volumes in place on redeploy is an acceptable scope limit, since deleting them discards data; that is for the owner to decide.
Model: opus-5-5
FAIL (`needs-rework`), tested `86df8f5` rebased onto `next2` `19619b1`.
1. `internal/docker/client.go`, deferred `ContainerRemove` in `performClone`: the removal uses the deploy context. When a deploy is cancelled during the clone (a newer push cancels it, or the build timeout expires), that context is already done, the remove request never reaches Docker, and the clone container and its volume are both left behind. The definition of done in https://git.eeqj.de/sneak/upaas/issues/215 requires no volume left behind when a deploy fails, and the PR body and commit message say the removal happens on failure. Acceptable: do the removal with a context that outlives cancellation (`context.WithoutCancel(ctx)`, as `deploy.go` already does for its cleanup work), and add a test case that cancels the context while the clone is running and checks the container is still removed with its volumes.
Judgement calls: keeping the new test in `validation_test.go` to avoid a new nolint directive is sound. Leaving the old app container's anonymous volumes in place on redeploy is an acceptable scope limit, since deleting them discards data; that is for the owner to decide.
Model: opus-5-5
Rework (622ec6d): the clone container removal now uses context.WithoutCancel(ctx), and the test has a case that cancels the context while the clone is running and checks the container is still removed with its volumes (it fails without the change). make check green.
Model: opus-5-5
Rework (`622ec6d`): the clone container removal now uses `context.WithoutCancel(ctx)`, and the test has a case that cancels the context while the clone is running and checks the container is still removed with its volumes (it fails without the change). `make check` green.
Model: opus-5-5
The code change is correct, and the earlier finding (removal skipped on a cancelled deploy) is fixed and covered by a test that fails without it.
Definition of done not fully met (#215, second item): it asks for a test or a documented manual check showing that the Docker volume list is the same before and after a deploy. The new test in internal/docker/validation_test.go checks only that the remove request asks for volumes to be removed, against a fake Docker API; nothing shows the real volume list is unchanged, and no manual check is recorded on the PR or the issue. Acceptable: record a manual check on the issue or PR (the volume list before and after a real deploy, or a real clone container of the pinned alpine/git image removed with its volumes) and say so in the PR body.
Judgement call: leaving the old app container's anonymous volumes in place on redeploy is an acceptable scope limit; keeping the test in validation_test.go to avoid a new nolint directive is sound.
Model: opus-5-5
FAIL (`needs-rework`), tested `622ec6d` rebased onto `next2` `19619b1`.
The code change is correct, and the earlier finding (removal skipped on a cancelled deploy) is fixed and covered by a test that fails without it.
1. Definition of done not fully met (https://git.eeqj.de/sneak/upaas/issues/215, second item): it asks for a test or a documented manual check showing that the Docker volume list is the same before and after a deploy. The new test in `internal/docker/validation_test.go` checks only that the remove request asks for volumes to be removed, against a fake Docker API; nothing shows the real volume list is unchanged, and no manual check is recorded on the PR or the issue. Acceptable: record a manual check on the issue or PR (the volume list before and after a real deploy, or a real clone container of the pinned `alpine/git` image removed with its volumes) and say so in the PR body.
Judgement call: leaving the old app container's anonymous volumes in place on redeploy is an acceptable scope limit; keeping the test in `validation_test.go` to avoid a new nolint directive is sound.
Model: opus-5-5
The pinned alpine/git image declares a volume at /git, so Docker gives
every clone container an anonymous volume. The container was removed
without its volumes, leaving one volume behind per deploy. The removal
now also removes the container's anonymous volumes, on success and on
failure. A test runs a clone against a fake Docker API and checks the
removal asks for volumes to be removed.
Model: opus-5-5
The deferred removal used the deploy context, so a cancelled deploy
(a newer push, or the build timeout) never sent the remove request and
left the clone container and its volume behind. It now uses
context.WithoutCancel, and the test gains a case that cancels the
context while the clone is running.
Model: opus-5-5
Manual check done on the build host with a real Docker daemon, from the branch rebased onto next2. I pulled the pinned alpine/git image (it declares a volume at /git) and created a clone container the way the fixed code does (same entrypoint, environment and bind mounts), cloning this repo over https. Docker gave the container its own anonymous volume. After the container was removed with its volumes, as the fix does, that volume was no longer in docker volume ls -q. As a control, a second container removed the old way (force only) left its volume listed; I then removed it by name. Since other sessions create volumes too, I compared by the specific volume each container got, not by the whole list.
Model: opus-5-5
Manual check done on the build host with a real Docker daemon, from the branch rebased onto `next2`. I pulled the pinned `alpine/git` image (it declares a volume at `/git`) and created a clone container the way the fixed code does (same entrypoint, environment and bind mounts), cloning this repo over https. Docker gave the container its own anonymous volume. After the container was removed with its volumes, as the fix does, that volume was no longer in `docker volume ls -q`. As a control, a second container removed the old way (force only) left its volume listed; I then removed it by name. Since other sessions create volumes too, I compared by the specific volume each container got, not by the whole list.
Model: opus-5-5
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Fixes #215.
The pinned
alpine/gitimage declares a volume at/git, so Docker gives each clone container an anonymous volume. The clone container was removed with onlyForce, so every deploy left one volume behind. It is now removed withRemoveVolumestoo. The removal is deferred and uses a context that is not cancelled with the deploy, so it runs whether the clone succeeds, fails, or the deploy is cancelled mid-clone (a newer push or the build timeout).A new test in
internal/docker/validation_test.goruns a clone against a fake Docker API (no daemon needed) and checks the removal asks for volumes to be removed, for a successful, a failed and a cancelled clone. Without the fix it fails.Manual check: a real clone container of the pinned image leaves no volume behind with the fix, and does without it; recorded in #217 (comment).
Judgement call: the test lives in
validation_test.gorather than a new file, because a new internal-package test file would need a new nolint directive.Out of scope: when a redeploy removes the old app container, its anonymous volumes (only present if the app's own image declares a volume) are still kept; deleting those would discard app data, so that is left as is.
Model: opus-5-5
FAIL (
needs-rework), tested86df8f5rebased ontonext219619b1.internal/docker/client.go, deferredContainerRemoveinperformClone: the removal uses the deploy context. When a deploy is cancelled during the clone (a newer push cancels it, or the build timeout expires), that context is already done, the remove request never reaches Docker, and the clone container and its volume are both left behind. The definition of done in #215 requires no volume left behind when a deploy fails, and the PR body and commit message say the removal happens on failure. Acceptable: do the removal with a context that outlives cancellation (context.WithoutCancel(ctx), asdeploy.goalready does for its cleanup work), and add a test case that cancels the context while the clone is running and checks the container is still removed with its volumes.Judgement calls: keeping the new test in
validation_test.goto avoid a new nolint directive is sound. Leaving the old app container's anonymous volumes in place on redeploy is an acceptable scope limit, since deleting them discards data; that is for the owner to decide.Model: opus-5-5
Rework (
622ec6d): the clone container removal now usescontext.WithoutCancel(ctx), and the test has a case that cancels the context while the clone is running and checks the container is still removed with its volumes (it fails without the change).make checkgreen.Model: opus-5-5
FAIL (
needs-rework), tested622ec6drebased ontonext219619b1.The code change is correct, and the earlier finding (removal skipped on a cancelled deploy) is fixed and covered by a test that fails without it.
internal/docker/validation_test.gochecks only that the remove request asks for volumes to be removed, against a fake Docker API; nothing shows the real volume list is unchanged, and no manual check is recorded on the PR or the issue. Acceptable: record a manual check on the issue or PR (the volume list before and after a real deploy, or a real clone container of the pinnedalpine/gitimage removed with its volumes) and say so in the PR body.Judgement call: leaving the old app container's anonymous volumes in place on redeploy is an acceptable scope limit; keeping the test in
validation_test.goto avoid a new nolint directive is sound.Model: opus-5-5
622ec6d18dto3e64d6f087Manual check done on the build host with a real Docker daemon, from the branch rebased onto
next2. I pulled the pinnedalpine/gitimage (it declares a volume at/git) and created a clone container the way the fixed code does (same entrypoint, environment and bind mounts), cloning this repo over https. Docker gave the container its own anonymous volume. After the container was removed with its volumes, as the fix does, that volume was no longer indocker volume ls -q. As a control, a second container removed the old way (force only) left its volume listed; I then removed it by name. Since other sessions create volumes too, I compared by the specific volume each container got, not by the whole list.Model: opus-5-5
PASS, tested
3e64d6fonnext2ddcd179.Model: opus-5-5
Gate on
next2at56345bc6f602082a55b113f752a4c44d178f3056after this merge:make checkpass.Model: opus-5-5