You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Every integration image Gladys pulls stays on the disk forever: update pulls the new image and leaves the previous one, uninstall removes the container, the private network, the data folder and the t_service row but never the image. Gladys' own upgrade runs Watchtower with --cleanup, so the core leaves nothing behind — external integrations were the asymmetry, and users on the forum report reclaiming gigabytes by deleting the images by hand.
Why not a prune. The manifest requires an explicit tag and integrations ship versioned ones, so the image an update supersedes keeps its tag: it is never dangling, and docker image prune does not see it. Only prune -a would — and Gladys usually shares its Docker daemon with the rest of the user's containers, so sweeping images it does not own is not an option. Gladys knows exactly which images it pulled and which it still needs, so cleanup is done by reference, not by sweeping.
Two mechanisms, both best-effort: a cleanup failure is logged, never raised, and never turns a successful update or uninstall into a failed one.
Targeted removal, on the lifecycle events that make an image unnecessary. update captures the images of the version being replaced (main + declared sub-containers) before rewriting the row, and removes them only once the new containers have actually started — dropping them earlier would take away the one thing a broken update can fall back on. uninstall removes them after the t_service row is destroyed, so the in-use check no longer counts the integration being removed. An update that keeps a sub-container image, or a re-pull of the same tag (a :dev install), leaves the image in use and therefore untouched.
Nightly sweep (3:30 AM, EVENTS.EXTERNAL_INTEGRATION.CLEAN_IMAGES → cleanImages), which is what gives an already-bloated install its disk back — targeted removal only keeps a fresh install clean. It considers only images carrying the io.gladysassistant.manifest label, i.e. images built as Gladys integrations, never anything else on the machine. A multi-tag image is a candidate through each of its tags (removing one reference only untags it); an untagged one — a rebuilt :dev install — by its id. Sub-container images (a Mosquitto broker, a Frigate) carry no such label and are deliberately out of the sweep's reach: they are third-party images the user may well run elsewhere, and targeted removal already covers them, where the manifest is what tells us they were ours.
Two guards stand between a candidate and its deletion:getImagesInUse filters out every image still referenced by an installed integration — its own image or one its manifest declares — so a third-party image shared by two integrations survives the uninstall of one of them; and removeImagenever forces, so Docker itself refuses (HTTP 409) to delete an image a container still references, running or stopped. 404 and 409 are both non-events for the caller, distinguished from a real failure by the boolean the call returns.
Changes
server/lib/system/: new system.listImages.js and system.removeImage.js.
server/lib/external-integration/: new getImagesInUse (the in-use set), removeImages (guarded targeted removal) and cleanImages (the labelled sweep); update and uninstall now call the cleanup.
server/config/scheduler-jobs.js: daily-cleanup-of-unused-integration-images at 3:30 AM.
docs/specs/external-integrations.md: new section B.19 Docker image cleanup (same diff, as required by the spec-first process).
Tests pass: cd server && npm run coverage (Codecov requires 100% coverage on changed lines) and Cypress (npm run cypress:run) if the UI changed
Linter and prettier pass on both front and server (npm run eslint, npm run prettier)
No undocumented breaking change
Coverage of every file touched is 100% (statements / branches / functions / lines), via 25 new tests in system.listImages.test.js, system.removeImage.test.js and externalIntegration.imageCleanup.test.js, plus 5 added to the existing update / uninstall suites. No front change, so no Cypress run.
Note: 23 tests fail identically on a clean master checkout in this environment (15 in store.test.js, 8 in gateway.backup) — outbound-HTTP related, unrelated to this diff.
Every integration image Gladys pulled stayed on the disk forever: `update`
pulled the new image and left the previous one, `uninstall` removed the
container, the private network, the data folder and the `t_service` row but
never the image. Gladys' own upgrade runs Watchtower with `--cleanup`, so the
core leaves nothing behind — integrations were the asymmetry, and users
reported reclaiming gigabytes by deleting the images by hand.
A `prune` is not the answer. A manifest must declare an explicit tag and
integrations ship versioned ones, so the image an update supersedes keeps its
tag: it is never dangling and `docker image prune` does not see it. Only
`prune -a` would, and Gladys usually shares its Docker daemon with the rest of
the user's containers. Gladys knows which images it pulled and which it still
needs, so cleanup is done by reference.
Two mechanisms, both best-effort — a cleanup failure is logged, never raised,
and never turns a successful update or uninstall into a failed one:
- targeted removal, on the lifecycle events that make an image unnecessary.
`update` captures the images of the version being replaced before rewriting
the row and removes them only once the new containers have started — earlier
would take away the one thing a broken update can fall back on. `uninstall`
removes them after the row is destroyed, so the in-use check no longer counts
the integration being removed.
- a nightly sweep (3:30 AM), which is what gives an already-bloated install its
disk back. It only ever considers images carrying the
`io.gladysassistant.manifest` label — images built as Gladys integrations,
never anything else on the machine. Multi-tag images are candidates through
each tag, untagged ones (a rebuilt `:dev` install) by id.
Two guards stand between a candidate and its deletion: `getImagesInUse` filters
out every image an installed integration still references, so a third-party
image shared by two integrations survives the uninstall of one of them; and
`removeImage` never forces, so Docker itself refuses (409) to delete an image a
container still references.
Spec B.19 added in the same diff.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012zFg1ZJerC7KcgxrkApuri
External integrations now support Docker image discovery, pull-time protection, and best-effort removal. Updates and uninstall operations remove unused images at defined lifecycle points. A daily scheduler runs a labeled-image cleanup sweep.
Changes
External integration image cleanup
Layer / File(s)
Summary
Docker image API and pull tracking server/lib/system/*, server/test/lib/system/*
Added Docker image listing and removal helpers. The system records pull timestamps and exposes them through the public System API.
Image usage and cleanup helpers server/lib/external-integration/*, server/test/lib/external-integration/externalIntegration.imageCleanup.test.js, server/utils/constants.js
GladysAssistant/Gladys#2550: Both changes modify Docker image lifecycle handling during integration updates and removals.
Suggested labels:needs:cursor-review
Suggested reviewers:cursor
Poem
A rabbit guards each fresh image pull,
And keeps shared images safe and full.
Old unused images leave the stack,
While nightly hops sweep leftovers back.
Docker stays tidy by dawn.
The reason will be displayed to describe this comment to others. Learn more.
Stale comment
Thoughtful, well-scoped fix for a real disk-bloat issue on shared Docker hosts. The dual path (targeted cleanup on update/uninstall + label-scoped nightly sweep), the getImagesInUse shared-image guard, best-effort error handling, and the refusal to force image deletes are the right safety shape for this surface. Spec B.19 is updated in the same diff, and the new/extended tests cover the important branches.
Soft notes only (non-blocking):
Spec section number collision with open #2807, which already claims B.19 for the calendar type — please renumber this section (likely B.20) before or at merge so the living spec stays coherent.
“Fallback” wording vs start() semantics — start() returns once the Docker container is up (LOADING), not once the integration is RUNNING; a bad release that still starts will still drop the previous images. Registry re-pull remains the real rollback path.
Narrow race between nightly sweep and an in-flight install/update: after pull and before the t_service row points at the new tag, the freshly pulled labelled image is not in getImagesInUse yet.
No device category/type changes. Not marking risk:high — deletions are reference-based, label-scoped for the sweep, non-forcing, and never fail the parent lifecycle. No needs:human-review needed for this additive cleanup.
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/test/lib/system/system.listImages.test.js`:
- Around line 76-111: Add a test alongside the existing system.listImages tests
that configures system.dockerode.listImages to reject with a specific error,
then asserts system.listImages rejects with that exact error. Cover the failure
path without changing the existing successful mapping and filtering tests.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
Push a commit to this branch (recommended)
Create a new PR with the fixes
ℹ️ Review info⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 44b9cd25-20b7-4066-8c8e-9f8cd90ad863
📥 Commits
Reviewing files that changed from the base of the PR and between 869b7ad and e3b74c5.
✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.48%. Comparing base (d522931) to head (f927c91). ⚠️ Report is 1 commits behind head on master.
- Renumber the spec section B.19 -> B.20: open PR #2807 already reserves B.19
for the calendar provider type.
- Never sweep a freshly pulled image. `install` and `update` pull their images
*before* writing the `t_service` row that declares them; a sweep landing in
that window saw a brand new image as an orphan and deleted it under the
operation that had just fetched it — and Docker's 409 is no help there, the
container does not exist yet. `system.pull` now stamps every pull (before the
download, so a slow pull on a Raspberry Pi is covered end to end) and the
sweep skips anything pulled less than an hour ago.
- Pin `force: false` in `system.removeImage`: the "never force" rule is the
safety invariant of every caller, so it belongs in the helper rather than in
call-site discipline.
- Align the update comment and the spec with what `start()` actually
guarantees: it resolves on LOADING, not RUNNING, so the cleanup buys the
container being created and started, never a healthy integration. The
ordering still matters — a `start()` that throws skips the cleanup entirely.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012zFg1ZJerC7KcgxrkApuri
Merging master brought in #2820, which forbids the shared sinon singleton in
test files. The four test files added by this PR predate that rule, so CI
linted the merge result and failed on them.
They now use `require('sinon').createSandbox()`, and the system ones reset the
Dockerode mock's own sandbox through `DockerodeMock.resetMockHistory()` — the
mock's fakes no longer live in the consumer's sandbox, so the per-file
`sinon.reset()` does not reach them.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012zFg1ZJerC7KcgxrkApuri
GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.
Start the sentence at Line 7 with an uppercase character. Remove the repeated asterisks at Line 15.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/lib/external-integration/externalIntegration.cleanImages.js` around
lines 7 - 15, The JSDoc block near the external integration image cleanup
description has lint formatting issues: capitalize the first character of the
sentence beginning at Line 7 and replace the repeated asterisks in the sentence
near Line 15 with normal punctuation.
Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/lib/external-integration/externalIntegration.cleanImages.js`:
- Around line 47-51: Update the cleanup flow around cleanImages and removeImages
to use the same per-reference synchronization as system.pull, then recheck
getImagePullTime and image usage while holding that lock immediately before
deleting each image. Ensure a concurrent pull cannot be removed during the gap
between candidate evaluation and deletion, and add a regression test covering
this interleaving.
In `@server/test/lib/system/system.getImagePullTime.test.js`:
- Around line 50-58: Update the image pull timing test to keep the underlying
dockerode.pull operation pending while system.pull is in progress, then assert
getImagePullTime('my-image:latest') returns a valid timestamp before resolving
the pull. Follow the existing test path and stubbing setup that mirrors
system.pull.js, while preserving the current timestamp bounds after completion.
---
Nitpick comments:
In `@server/lib/external-integration/externalIntegration.cleanImages.js`:
- Around line 7-15: The JSDoc block near the external integration image cleanup
description has lint formatting issues: capitalize the first character of the
sentence beginning at Line 7 and replace the repeated asterisks in the sentence
near Line 15 with normal punctuation.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
Push a commit to this branch (recommended)
Create a new PR with the fixes
ℹ️ Review info⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d14a9cf8-0749-41cd-8d6d-6088143351b7
📥 Commits
Reviewing files that changed from the base of the PR and between d522931 and 2e030e4.
The reason will be displayed to describe this comment to others. Learn more.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Serialize pull and removal for the same image reference.
A cleanup can evaluate getImagePullTime at Line 48, then yield in removeImages. A concurrent install can then start system.pull and stamp the image before its service row exists. Cleanup can still remove that image because it does not check the pull time again.
Use shared per-reference synchronization between system.pull and cleanup removal. Recheck the pull timestamp and image usage while holding that synchronization before deletion. Add a regression test for this interleaving.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/lib/external-integration/externalIntegration.cleanImages.js` around
lines 47 - 51, Update the cleanup flow around cleanImages and removeImages to
use the same per-reference synchronization as system.pull, then recheck
getImagePullTime and image usage while holding that lock immediately before
deleting each image. Ensure a concurrent pull cannot be removed during the gap
between candidate evaluation and deletion, and add a regression test covering
this interleaving.
The reason will be displayed to describe this comment to others. Learn more.
Valid, and fixed in f927c91 — though with the cheap recheck rather than the mutex.
The gap is worse than "a yield in removeImages": removals are sequential (Promise.each), so for the last candidate the window is as long as all the preceding deletions, not microseconds. So the fix is to stop treating the pull time as a property of the list and read it as a property of each deletion: the check now runs immediately before each system.removeImage, and the up-front filter is gone. That same re-read also covers inUse going stale during the loop, since the install writes its t_service row after the pull the check sees.
I did not add per-reference synchronization between system.pull and the sweep. It would put a lock across a layer boundary (lib/system knowing about the supervisor's cleanup) to protect an operation whose worst outcome is a failed install the user retries — and the recheck already closes the interleaving you describe, at the cost of one Map.get per deletion.
One thing worth flagging since it constrains the fix: the protection is scoped to the sweep (removeImages(..., { skipRecentlyPulled: true })). Applying it to the targeted path would be a regression — update and uninstall name images they know are theirs to drop, so guarding them would leave the first image behind whenever an integration is updated twice within the hour. There is a test pinning that.
Regression test added for the interleaving ("should spare an image whose pull starts while the sweep is running"): an install stamps the second image while the first is being removed. I checked it fails against the previous up-front-filter implementation, so it pins the behaviour rather than just describing it.
@Pierre-Gilles, the change improves the sweep and the new test correctly covers the sequential-removal gap.
However, the original race remains unresolved:
In externalIntegration.removeImages.js, Line 51 reads getImagePullTime(image).
At Line 58, system.removeImage(image) starts after that read.
A concurrent system.pull(image) can stamp the pull time in that interval.
There is no shared synchronization, so the sweep can still delete the image.
Also, getImagesInUse() runs once before Promise.each. The implementation does not re-read image usage for each deletion. A service row that appears during the loop is not visible in inUse.
The regression test proves that a pull which starts during an earlier deletion protects the later candidate. It does not test a pull that starts after the later candidate’s final pull-time check and before its deletion.
Please keep the finding open. A per-reference lock, shared by system.pull and the sweep deletion, is still required to make the final check and deletion atomic with respect to a pull.
✏️ Learnings added
Learnt from: Pierre-Gilles
Repo: GladysAssistant/Gladys PR: 2822
File: server/lib/external-integration/externalIntegration.cleanImages.js:0-0
Timestamp: 2026-08-10T13:57:14.040Z
Learning: For Gladys external integration image cleanup, `skipRecentlyPulled: true` applies only to the scheduled sweep through `externalIntegration.cleanImages`. Targeted cleanup through `externalIntegration.update` and `externalIntegration.uninstall` must not use this protection, because those paths identify images that are known to be obsolete and may run multiple times within the one-hour protection window.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
The reason will be displayed to describe this comment to others. Learn more.
Stale comment
Re-reviewed at 2e030e4 after the follow-up fixes.
All four points from the earlier review are addressed: section renumbered to B.20, start() wording aligned with LOADING (not RUNNING), force: false pinned in system.removeImage, and the install/update vs nightly-sweep race closed via a pre-download pull stamp + one-hour RECENTLY_PULLED_PROTECTION_MS skip in cleanImages (documented in B.20).
Guards still look sound for a shared Docker host: label-scoped sweep only, getImagesInUse before delete, never-force removal, best-effort cleanup that cannot fail an update/uninstall. CI is green including codecov/patch.
Not risk:high / not needs:human-review. One residual soft note left inline on a narrower TOCTOU; not a merge blocker.
…letion
Follow-up on the residual race in the nightly sweep. The pull-time guard was
evaluated once over the whole candidate list, then `removeImages` awaited the
in-use set and removed the images one by one. An install starting after the
sweep began — but before its image's turn came — could stamp the image in that
gap and still have it deleted, since nothing re-read the stamp. Sequential
removals make that gap as long as the removals themselves, not microseconds.
The check now runs right before each deletion instead. That same re-read also
covers the in-use set going stale during the loop: the install writes its
`t_service` row after the pull the check sees.
Scoped to the sweep via `removeImages(..., { skipRecentlyPulled: true })`.
Update and uninstall deliberately keep removing recently pulled images: they
name images they know are theirs to drop, and guarding them would leave the
first image behind whenever an integration is updated twice within the hour.
Also fix the pull-time test, which read the stamp only after `system.pull`
resolved and so passed whether the stamp was written before or after the
download. It now holds the pull pending and asserts the stamp is already
there — which is the property the sweep relies on for slow pulls.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012zFg1ZJerC7KcgxrkApuri
The reason will be displayed to describe this comment to others. Learn more.
Re-reviewed at f927c91.
The follow-up closes the residual sweep race from the previous pass: skipRecentlyPulled is checked inside removeImages immediately before each delete (not once over the candidate list), scoped to the nightly path only so targeted update/uninstall cleanup still reclaims superseded tags within the hour. Spec B.20 and the mid-sweep / targeted-path tests match that contract. Prior soft points (B.20 numbering, start() LOADING wording, pull stamp + 1h protection, pinned force: false) remain addressed.
No device category/type changes. Not risk:high / not needs:human-review. CI green including codecov/patch. Ready to merge.
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
area:serverNode.js server codetype:choreDeps, CI, refactoring, docs. Hidden from user changelog
2 participants
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.
github-actions
Bot
added
type:chore
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Description
Every integration image Gladys pulls stays on the disk forever:
updatepulls the new image and leaves the previous one,uninstallremoves the container, the private network, the data folder and thet_servicerow but never the image. Gladys' own upgrade runs Watchtower with--cleanup, so the core leaves nothing behind — external integrations were the asymmetry, and users on the forum report reclaiming gigabytes by deleting the images by hand.Why not a
prune. The manifest requires an explicit tag and integrations ship versioned ones, so the image an update supersedes keeps its tag: it is never dangling, anddocker image prunedoes not see it. Onlyprune -awould — and Gladys usually shares its Docker daemon with the rest of the user's containers, so sweeping images it does not own is not an option. Gladys knows exactly which images it pulled and which it still needs, so cleanup is done by reference, not by sweeping.Two mechanisms, both best-effort: a cleanup failure is logged, never raised, and never turns a successful update or uninstall into a failed one.
updatecaptures the images of the version being replaced (main + declared sub-containers) before rewriting the row, and removes them only once the new containers have actually started — dropping them earlier would take away the one thing a broken update can fall back on.uninstallremoves them after thet_servicerow is destroyed, so the in-use check no longer counts the integration being removed. An update that keeps a sub-container image, or a re-pull of the same tag (a:devinstall), leaves the image in use and therefore untouched.EVENTS.EXTERNAL_INTEGRATION.CLEAN_IMAGES→cleanImages), which is what gives an already-bloated install its disk back — targeted removal only keeps a fresh install clean. It considers only images carrying theio.gladysassistant.manifestlabel, i.e. images built as Gladys integrations, never anything else on the machine. A multi-tag image is a candidate through each of its tags (removing one reference only untags it); an untagged one — a rebuilt:devinstall — by its id. Sub-container images (a Mosquitto broker, a Frigate) carry no such label and are deliberately out of the sweep's reach: they are third-party images the user may well run elsewhere, and targeted removal already covers them, where the manifest is what tells us they were ours.Two guards stand between a candidate and its deletion:
getImagesInUsefilters out every image still referenced by an installed integration — its own image or one its manifest declares — so a third-party image shared by two integrations survives the uninstall of one of them; andremoveImagenever forces, so Docker itself refuses (HTTP 409) to delete an image a container still references, running or stopped. 404 and 409 are both non-events for the caller, distinguished from a real failure by the boolean the call returns.Changes
server/lib/system/: newsystem.listImages.jsandsystem.removeImage.js.server/lib/external-integration/: newgetImagesInUse(the in-use set),removeImages(guarded targeted removal) andcleanImages(the labelled sweep);updateanduninstallnow call the cleanup.server/config/scheduler-jobs.js:daily-cleanup-of-unused-integration-imagesat 3:30 AM.docs/specs/external-integrations.md: new section B.19 Docker image cleanup (same diff, as required by the spec-first process).Forum
Forum: https://community.gladysassistant.com/t/integration-externe-nettoyage-des-vielles-images/10501
Checklist
cd server && npm run coverage(Codecov requires 100% coverage on changed lines) and Cypress (npm run cypress:run) if the UI changednpm run eslint,npm run prettier)Coverage of every file touched is 100% (statements / branches / functions / lines), via 25 new tests in
system.listImages.test.js,system.removeImage.test.jsandexternalIntegration.imageCleanup.test.js, plus 5 added to the existingupdate/uninstallsuites. No front change, so no Cypress run.Note: 23 tests fail identically on a clean
mastercheckout in this environment (15 instore.test.js, 8 ingateway.backup) — outbound-HTTP related, unrelated to this diff.Generated by Claude Code
Summary by CodeRabbit
New Features
Tests