feat(image-cleanup): event-driven cleanup of orphaned ExApp Docker images via HaRP#865
Draft
oleksandr-nc wants to merge 1 commit into
Draft
feat(image-cleanup): event-driven cleanup of orphaned ExApp Docker images via HaRP#865oleksandr-nc wants to merge 1 commit into
oleksandr-nc wants to merge 1 commit into
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
…ages via HaRP Signed-off-by: Oleksander Piskun <oleksandr2088@icloud.com>
oleksandr-nc
force-pushed
the
feat/exapp-image-cleanup
branch
from
ab3e6fe to
2812588
Compare
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Implements event-driven cleanup of orphaned ExApp Docker images (HaRP-only) after uninstall/update, with admin-configurable enablement + grace period and per-command overrides.
Changes:
- Add
ExAppImageCleanupService+OrphanedImageCleanupJobto capture image refs and delete images immediately or via grace-period queued jobs. - Add admin settings + typed config persistence for
image_cleanup_enabledandimage_cleanup_grace_hours. - Add unit tests for scheduling/job behavior and an end-to-end test validating HaRP/Docker behavior (including multi-tag
bytes_freedsemantics).
Reviewed changes
Copilot reviewed 15 out of 17 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_image_cleanup.py | New E2E test covering uninstall/update image cleanup scenarios via HaRP + Docker CLI. |
| tests/php/Service/ExAppImageCleanupServiceTest.php | Unit tests for capture/schedule behavior and grace clamping. |
| tests/php/Controller/ExAppsPageControllerTest.php | Updates controller test wiring for new cleanup service dependency. |
| tests/php/BackgroundJob/OrphanedImageCleanupJobTest.php | Unit tests for the queued cleanup job behavior and logging paths. |
| src/components/AdminSettings.vue | Adds admin UI for cleanup toggle and grace period input. |
| lib/Settings/Admin.php | Exposes cleanup config values to the admin settings template. |
| lib/Service/ExAppImageCleanupService.php | New service to capture refs, schedule/purge cleanup, and cancel pending jobs on daemon removal. |
| lib/Service/DaemonConfigService.php | Cancels pending cleanup jobs when unregistering a daemon. |
| lib/DeployActions/DockerActions.php | Adds HaRP-only removeImage() and getRunningImageRef() helpers. |
| lib/Controller/ExAppsPageController.php | Captures image ref on uninstall and schedules cleanup based on request choice. |
| lib/Controller/ConfigController.php | Persists admin config using bool/int/string setters based on value type. |
| lib/Command/ExApp/Update.php | Adds options to purge/keep old image on update and schedules cleanup post-success. |
| lib/Command/ExApp/Unregister.php | Adds options to purge/keep image on uninstall and schedules cleanup once container removed. |
| lib/BackgroundJob/OrphanedImageCleanupJob.php | New queued job that attempts image deletion and logs outcomes (409/in use, not found, etc.). |
| lib/AppInfo/Application.php | Adds config keys and defaults for image cleanup. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+155
to
+161
| onGraceHoursInput(value) { | ||
| delay(() => { | ||
| const clamped = Math.min(720, Math.max(0, parseInt(value, 10) || 0)) | ||
| this.state.image_cleanup_grace_hours = clamped | ||
| this.saveOptions({ image_cleanup_grace_hours: clamped }) | ||
| }, 2000)() | ||
| }, |
| if (!is_array($arg) || ($arg['daemon_id'] ?? null) !== $daemonName) { | ||
| continue; | ||
| } | ||
| $this->jobList->removeById((string)$row['id']); |
Comment on lines
+142
to
+145
| try: | ||
| occ("app_api:app:unregister", APP_ID, "--silent", "--force", "--purge-now", check=False) | ||
| except CalledProcessError: | ||
| pass |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds automatic cleanup of ExApp Docker images orphaned by uninstall or update, scheduled per-event via a one-shot QueuedJob with a configurable grace period (default 24h). Docker's own 409 response handles the "still in use" case, so no AppAPI-side state tracks image references.
HaRP-only by design; direct Docker support is intentionally not implemented since it's being removed in NC35.
Per-call CLI overrides on
app:remove(--purge-now/--keep-image) andapp:update(--purge-old-image-now/--keep-old-image). Admin settings expose a master toggle and the grace period (0–720 hours).Targets Nextcloud 35.
Companion HaRP endpoint already landed in nextcloud/HaRP#102.
A few non-obvious choices, documented for future reference:
1. Per-event one-shot job vs periodic prune
Each orphan event (uninstall, update) schedules a
QueuedJobthat fires after the grace period, deletes the image, then removes itself from the queue.dangling=trueprune (the original approach in feat(IS-667): Add automatic cleanup of outdated ExApp Docker images #837): wrong scope (only catches already-untagged images), no per-event control, can't honor a per-uninstall override.2. Live Docker query for image ref vs AppAPI-side state
At hook time AppAPI asks HaRP for the container's
Imagefield via the extended/docker/exapp/existsresponse (added in nextcloud/HaRP#102).current_image_refcolumn onex_apps: schema migration, duplicates Docker's state, drift risk if a container is rebuilt out-of-band.3. "In use" check delegated to Docker
The cleanup job just calls DELETE; Docker returns 409 if the image is still referenced by any container (running or stopped). No AppAPI-side refcount.
4. HaRP-only by design
The hooks short-circuit for non-HaRP Docker daemons. Direct Docker (and DockerSocketProxy) are being removed in NC35; carrying a parallel cleanup path would be code we'd delete in a few months. Existing direct-Docker users keep working, just without auto-cleanup until they migrate to HaRP.
Other choices worth noting
bytes_freedaccuracy (caught in review of HaRP#102): parse Docker's DELETE response, only count the inspected size when at least one entry has aDeletedkey. A multi-tag delete that only untags reportsbytes_freed: 0instead of lying.nc-server'sapps/settings/, not this repo, so the dropdown + "don't ask again" UX needs a coordinated server PR. The HTTP endpoint here already accepts animageCleanuprequest param so the future UI PR can wire to it without back-end changes.Screenshots
TODO