Clean up Docker container when deleting an app (closes #2) #7

clawbot · 2026-02-08T21:05:28+01:00

clawbot commented

2026-02-08 21:05:28 +01:00

No description provided.

sneak was assigned by clawbot

2026-02-08 21:05:28 +01:00

clawbot added 1 commit 2026-02-08 21:05:29 +01:00

fix: clean up Docker container when deleting an app (closes #2 ) ed4ddc5536

clawbot reviewed 2026-02-16 05:55:15 +01:00

clawbot left a comment

Review: PR#7 — Clean up Docker container when deleting an app

Overall: Good fix. Properly cleans up Docker resources on app deletion.

Positives

Correct order of operations: stop → remove → delete DB record.
Errors during container cleanup are logged but don't block the DB deletion — this is the right call for a cleanup path.
Force-remove flag (true) ensures the container is removed even if something is off.

Concerns

Race condition: If a deployment is actively running and creating a new container while deletion is in progress, the new container could be missed. Consider whether a lock or status check on the app is needed. Low probability but worth noting.
No test coverage: Unlike the other PRs in this series, this one lacks a test. Even a unit test mocking FindContainerByAppID, StopContainer, and RemoveContainer would increase confidence.
Network/volume cleanup: Deleting the container is good, but are there associated Docker networks or volumes that should also be cleaned up? If upaas creates per-app Docker volumes, those would be orphaned.
containerErr swallowed silently: When FindContainerByAppID returns an error (not just "not found"), it's silently ignored. Consider logging non-"not found" errors so operational issues are visible.

Verdict: Approve with minor suggestions. The core logic is correct. Adding a test and logging containerErr would strengthen it.

## Review: PR#7 — Clean up Docker container when deleting an app **Overall: Good fix. Properly cleans up Docker resources on app deletion.** ### Positives - Correct order of operations: stop → remove → delete DB record. - Errors during container cleanup are logged but don't block the DB deletion — this is the right call for a cleanup path. - Force-remove flag (`true`) ensures the container is removed even if something is off. ### Concerns 1. **Race condition**: If a deployment is actively running and creating a new container while deletion is in progress, the new container could be missed. Consider whether a lock or status check on the app is needed. Low probability but worth noting. 2. **No test coverage**: Unlike the other PRs in this series, this one lacks a test. Even a unit test mocking `FindContainerByAppID`, `StopContainer`, and `RemoveContainer` would increase confidence. 3. **Network/volume cleanup**: Deleting the container is good, but are there associated Docker networks or volumes that should also be cleaned up? If upaas creates per-app Docker volumes, those would be orphaned. 4. **`containerErr` swallowed silently**: When `FindContainerByAppID` returns an error (not just "not found"), it's silently ignored. Consider logging non-"not found" errors so operational issues are visible. **Verdict: Approve with minor suggestions.** The core logic is correct. Adding a test and logging `containerErr` would strengthen it.

sneak merged commit 79a3165f90 into main

2026-02-16 05:56:57 +01:00

sneak referenced this issue from a commit

2026-02-16 05:56:57 +01:00

Merge pull request 'Clean up Docker container when deleting an app (closes #2)' (#7) from clawbot/upaas:fix/issue-2 into main

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/upaas#7