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 main2026-02-16 05:56:57 +01:00
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.
clawbot
Review: PR#7 — Clean up Docker container when deleting an app
Overall: Good fix. Properly cleans up Docker resources on app deletion.
Positives
true) ensures the container is removed even if something is off.Concerns
FindContainerByAppID,StopContainer, andRemoveContainerwould increase confidence.containerErrswallowed silently: WhenFindContainerByAppIDreturns 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
containerErrwould strengthen it.