feat(image-cleanup): event-driven cleanup of orphaned ExApp Docker images via HaRP#865
feat(image-cleanup): event-driven cleanup of orphaned ExApp Docker images via HaRP#865oleksandr-nc wants to merge 1 commit into
Conversation
|
Warning Review limit reached
Next review available in: 3 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (23)
📝 WalkthroughWalkthroughChangesThis PR adds automated Docker image cleanup for ExApps. New constants and an Sequence Diagram(s)See hidden artifact. Compact metadata
Related issues: None specified Related PRs: None specified Suggested labels: feature, enhancement, needs-review Suggested reviewers: Backend maintainers familiar with DaemonConfigService/AppAPIService and Docker/HaRP integration Poem A rabbit sniffs at images old and cold, 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
oleksandr-nc
force-pushed
the
feat/exapp-image-cleanup
branch
2 times, most recently
from
3a4c171 to
08d6aff
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
src/components/AdminSettings.vue (1)
61-70: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGrace-hours max (720) hardcoded twice in this file.
The
720bound appears both in the template (:max="720") and inonGraceHoursInput's clamp (Math.min(720, ...)), duplicating the backend'sApplication::MAX_IMAGE_CLEANUP_GRACE_HOURS. If that constant ever changes, this file must be updated in three places (two here plus the backend) to stay in sync.Consider deriving the bound from a single local constant, or better, exposing it via
admin-initial-dataso frontend and backend stay authoritative from one source.♻️ Example: single local constant
+const MAX_IMAGE_CLEANUP_GRACE_HOURS = 720 + export default { ... methods: { onGraceHoursInput(value) { clearTimeout(this.graceHoursSaveTimer) this.graceHoursSaveTimer = setTimeout(() => { - const clamped = Math.min(720, Math.max(0, parseInt(value, 10) || 0)) + const clamped = Math.min(MAX_IMAGE_CLEANUP_GRACE_HOURS, Math.max(0, parseInt(value, 10) || 0)) this.state.image_cleanup_grace_hours = clamped this.saveOptions({ image_cleanup_grace_hours: clamped }) }, 2000) }, }, }Also applies to: 151-161
tests/php/BackgroundJob/OrphanedImageCleanupJobTest.php (1)
48-52: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
invokeRun()can't exercise the non-array-argument guard.
private function invokeRun(array $argument): voidtype-hints$argumentasarray, so the job's!is_array($argument)defensive branch is untestable through this helper, even though production code can receive an untyped argument from the queue infrastructure.♻️ Loosen the helper's type to cover the guard branch
- private function invokeRun(array $argument): void { + private function invokeRun(mixed $argument): void { $reflection = new \ReflectionMethod($this->job, 'run'); $reflection->setAccessible(true); $reflection->invoke($this->job, $argument); }tests/test_image_cleanup.py (1)
140-146: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse the
occ()helper for the job-delete cleanup loop.Line 145 re-implements the
occ()command construction ([*OCC_PREFIX, "--no-warnings", ...]) instead of callingocc(...), causing drift risk if the helper's behavior changes later.♻️ Proposed refactor
def reset_state() -> None: """Best-effort uninstall + image purge so each scenario starts clean.""" occ("app_api:app:unregister", APP_ID, "--silent", "--force", "--purge-now", check=False) # Drop any stray queued jobs from prior runs of this test. for j in list_pending_jobs("OrphanedImageCleanupJob"): - run([*OCC_PREFIX, "--no-warnings", "background-job:delete", str(j["id"])], check=False, stdout=DEVNULL, stderr=DEVNULL) + occ("background-job:delete", str(j["id"]), check=False)
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9c071afc-1c1f-4fb2-b5a5-9220881a2619
⛔ Files ignored due to path filters (1)
js/app_api-adminSettings.js.mapis excluded by!**/*.map
📒 Files selected for processing (23)
.github/workflows/tests-deploy.ymlAGENTS.mdjs/app_api-adminSettings.jslib/AppInfo/Application.phplib/BackgroundJob/OrphanedImageCleanupJob.phplib/Command/ExApp/Unregister.phplib/Command/ExApp/Update.phplib/Controller/ConfigController.phplib/Controller/ExAppsPageController.phplib/DeployActions/DockerActions.phplib/Service/AppAPIService.phplib/Service/DaemonConfigService.phplib/Service/ExAppImageCleanupService.phplib/Service/ImageCleanupChoice.phplib/Settings/Admin.phpopenapi-administration.jsonopenapi-full.jsonsrc/components/AdminSettings.vuetests/php/BackgroundJob/OrphanedImageCleanupJobTest.phptests/php/Controller/ExAppsPageControllerTest.phptests/php/Service/AppAPIServiceTest.phptests/php/Service/ExAppImageCleanupServiceTest.phptests/test_image_cleanup.py
| // The daemon config is deleted right after this, so a grace-period | ||
| // job could never reach its Docker socket again; when automatic | ||
| // cleanup is enabled, purge the image immediately instead | ||
| // (best-effort, Docker 409 protects shared images). This is an | ||
| // automatic path, so the master toggle is honored here even | ||
| // though PURGE_NOW mechanics are used. | ||
| $imageRef = $this->imageCleanupService->isMasterEnabled() | ||
| ? $this->imageCleanupService->captureImageRef($daemonConfig, $exApp->getAppid(), ImageCleanupChoice::PURGE_NOW) | ||
| : null; | ||
| if (boolval($exApp->getDeployConfig()['harp'] ?? false)) { | ||
| $this->dockerActions->removeExApp($this->dockerActions->buildDockerUrl($daemonConfig), $exApp->getAppid(), true); | ||
| } else { | ||
| $this->dockerActions->removeContainer($this->dockerActions->buildDockerUrl($daemonConfig), $this->dockerActions->buildExAppContainerName($exApp->getAppid())); | ||
| $this->dockerActions->removeVolume($this->dockerActions->buildDockerUrl($daemonConfig), $this->dockerActions->buildExAppVolumeName($exApp->getAppid())); | ||
| } | ||
| if ($imageRef !== null) { | ||
| $this->imageCleanupService->scheduleCleanup( | ||
| $imageRef, | ||
| $exApp, | ||
| $daemonConfig, | ||
| ImageCleanupChoice::PURGE_NOW, | ||
| ); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
New network calls now sit inside a loop-wide catch-all that silently aborts remaining ExApps.
The whole foreach ($targetDaemonExApps as $exApp) loop is wrapped by a single try { ... } catch (Exception) {} around the entire method body (unchanged, lines 797/832-833), with no logging. Adding isMasterEnabled()/captureImageRef()/scheduleCleanup() calls inside this loop means a single Guzzle failure while purging one ExApp's image can now silently stop disableExApp()/unregisterExApp() from running for all remaining ExApps on that daemon, with zero diagnostics — during a daemon-removal flow, that's the worst time for orphaned/undisabled ExApp registrations to go unnoticed.
Consider wrapping just the new image-cleanup calls in their own try/catch with a warning log (matching DaemonConfigService::unregisterDaemonConfig()'s best-effort pattern) so an image-purge failure can't halt disabling/unregistering the rest of the daemon's ExApps.
…ages via HaRP Signed-off-by: Oleksander Piskun <oleksandr2088@icloud.com>
oleksandr-nc
force-pushed
the
feat/exapp-image-cleanup
branch
from
08d6aff to
5bea568
Compare
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. Requires HaRP >= 0.4.1 for the companion endpoints (
MINIMUM_HARP_VERSIONbumped, so the setup check flags older daemons); against an older HaRP the feature degrades to a no-op.Per-call CLI overrides on
app:unregister(--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). The master toggle governs only the automatic (grace) path: an explicit--purge-nowis always honored, and--keep-imagealso cancels a still-pending cleanup job of that app, so a keep decision wins over a job queued by an earlier uninstall or update.Unregistering a daemon is the last moment its Docker host is reachable, so grace periods cannot be honored there: the web flow removes the daemon's ExApps and purges their images immediately (when automatic cleanup is enabled), and both flows flush still-queued cleanup jobs of that daemon by deleting their images right away before dropping the jobs. The occ flow requires apps to be removed first, so their queued jobs are what the flush picks up.
Targets Nextcloud 35.
Companion HaRP endpoint already landed in nextcloud/HaRP#102, released in HaRP 0.4.1.
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
--keep-image/--purge-nowcancel a still-queued job for the app, and the job re-checks the master toggle at run time, so disabling the feature also stops already-scheduled deletions.image_cleanup_enabled/image_cleanup_grace_hoursare stored typed (bool/int) so the read side can usegetValueBool/getValueInt(withlazy: trueon both sides).setAdminConfigcoerces them by key name; unknown keys keep the legacy string path, so an API caller sending e.g. an intinit_timeoutcan't flip a legacy setting's stored type and break its readers.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.apps/appstorefrontend, not this repo, so the dropdown + "don't ask again" UX needs a coordinated server PR. The HTTP endpoint here already accepts animageCleanup=grace|now|keeprequest param so the future UI PR can wire to it without back-end changes.tests/test_image_cleanup.py(7 scenarios, incl. reinstall-before-cron, multi-tagbytes_freed, keep-cancels-stale-job, purge-now with the toggle off) is wired into thetests-deploy.ymlHaRP job.Screenshots