Skip to content

Add safe Docker runtime cleanup#2

Open
NiveditJain wants to merge 4 commits into
LarsenCundric:mainfrom
NiveditJain:feature/docker-purge
Open

Add safe Docker runtime cleanup#2
NiveditJain wants to merge 4 commits into
LarsenCundric:mainfrom
NiveditJain:feature/docker-purge

Conversation

@NiveditJain

@NiveditJain NiveditJain commented May 10, 2026

Copy link
Copy Markdown

Summary

Adds best-effort Docker runtime cleanup alongside the filesystem scan:

  • exited containers (status=exited) and dangling images (dangling=true) only — running containers and tagged images are never targeted
  • scope to runtime artifacts via --category containers / --category images, or --containers-only / --images-only (mutually exclusive); runtime-only modes skip the filesystem walk
  • --older-than applied to dated artifacts; artifacts with no parseable creation date are kept (safe choice) with a warning
  • per-item delete with separate success/failure tracking

Safety

  • running containers never targeted (status=exited filter)
  • tagged images never targeted (dangling=true filter)
  • Docker invoked via execFile with an argv array (no shell), 15s timeout so a stuck daemon can't hang a cron job

⚠️ Stacked on #3 — merge #3 first

Per @LarsenCundric's review, this is rebased on top of #3 so there is a single glob/ignore implementation. The duplicated globToRegExp, --ignore/config loading, and the canCollectBloatAtDir root-bloat fix that an earlier version of this branch carried are dropped here and inherited from #3.

Merge order: #3 first, then this PR. Until #3 lands, the diff above also includes #3's changes (src/scanner.js, the --ignore parsing). Once #3 merges, this collapses to just src/docker.js, the runtime integration in src/index.js, and the Docker README section. The earlier package-lock.json rename noise is also gone.

Checks

  • node --check src/index.js / src/docker.js / src/scanner.js
  • --containers-only, --images-only, --category containers|images dry-runs against a live Docker daemon (filesystem walk correctly skipped)
  • combined default dry-run shows filesystem table + runtime summary
  • --containers-only --images-only together errors (mutually exclusive)
  • ignore + deferred-config behavior still passes (inherited from Fix ignore pattern handling #3)

NiveditJain and others added 2 commits May 5, 2026 11:33
Adds a repeatable --ignore CLI option and support for ~/.config/dev-purge/config.json with an ignore array. Patterns are glob-like (supports ~ and **). Scanner respects ignore patterns when discovering projects and bloat dirs.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@LarsenCundric

Copy link
Copy Markdown
Owner

Useful feature, but this PR overlaps significantly with #3 (ignore-pattern handling) — the two are diverging. Worth resolving before merge.

Critical

[P0] Overlap with #3, with diverged glob implementation. Both PRs add --ignore + config loading + globToRegExp. The version here (src/scanner.js:188) is the weaker one:

  • doesn't handle **/ as a path-segment-consuming wildcard (no slash awareness)
  • doesn't support bare directory names like node_modules
  • doesn't support scan-root-relative paths
  • doesn't anchor properly
  • escape regex includes \\ but misses * → results may be wrong on certain inputs

Recommend: rebase on top of #3 after that merges and drop the duplicated --ignore logic from this PR. Otherwise whichever merges second will silently regress the other.

Should fix

[P1] printResults no longer called when only runtime cleanup runs (src/index.js:187)
When --containers-only or --category containers is used, the filesystem table doesn't print (good), but results.length === 0 early-return is gone. If filesystem also has nothing AND no runtime candidates, the code still falls through — minor UX, but the message ordering is now subtle. Worth a quick walkthrough.

[P1] Docker --older-than policy: keep when timestamp unparseable (src/docker.js:30)
The intent in isKeptByAge is "safer to keep" when no timestamp. Correct, but this contradicts the conservative path for runtime cleanup: a malformed Docker date keeps an old container indefinitely. Worth a console.warn so users know why a container they expected to remove was kept.

[P2] --containers-only + --images-only together is undefined
Currently containersOnly ? true : imagesOnly ? false : .... If both flags are passed, images get silently disabled. Either explicitly error, or document precedence in --help.

[P2] parseJsonLines swallows JSON errors silently
If Docker changes its output format, the entire runtime feature appears as "0 containers found." Add a one-line dim warning when a line fails to parse so debugging isn't impossible.

Nice catches

  • execFile with argv (not shell string) on every Docker call — proper hygiene
  • Per-item delete with separate failure tracking — lets users see partial successes
  • keep flag preserves audit trail (what was found vs what's slated for delete)
  • Filtering to status=exited + dangling=true — correct safe scope
  • 15s timeout on Docker calls — won't hang cron jobs

Minor

  • package-lock.json rename from devcleandev-purge got bundled in. Fine, but worth calling out in the PR description since it affects npm install resolution.
  • README mentions "If Docker is not installed... continue" but the unavailable reason in printRuntimeSummary says "unavailable in this environment" — slight mismatch with what the README promises. The actual error from Docker (err.message) is also dropped. Consider showing it dimmed so users can debug.
  • --json doesn't include runtime artifacts. Called out in README. Reasonable for v1, but worth a follow-up issue.

Verdict

Block on the #3 overlap. Either:

  1. Merge Fix ignore pattern handling #3 first, rebase this PR to drop the duplicated ignore logic, OR
  2. Pull the ignore logic out of this PR and into Fix ignore pattern handling #3, leaving this purely Docker-focused

The Docker portion is solid and ready. Don't ship two diverging glob implementations.

- Make getFlagValue/getFlagValues/positional parsing consistent via a
  known-flag check, so an ignore value that starts with "-" (e.g.
  `--ignore -skip`) is captured instead of silently dropped, and a real
  flag after `--ignore` (e.g. `--ignore --json`) is not swallowed. (P1)
- Defer the config.json read into a runtime loadIgnorePatterns() so a slow
  or hung $XDG_CONFIG_HOME mount can no longer hang `dev-purge --help`. (P2)
- Warn on invalid (non-string/empty) config "ignore" entries instead of
  dropping them silently — predictable behavior for cron-driven runs. (P3)
- Guard against a bare "~" expanding to "" in envs with no HOME, which
  would otherwise compile to a match-everything pattern. (P2)
- README: note case-sensitivity of bare-name matching and config validation. (P3)

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Adds best-effort cleanup of Docker leftovers alongside the filesystem scan:
exited containers (status=exited) and dangling images (dangling=true) only —
running containers and tagged images are never targeted. Docker is invoked via
execFile with an argv array (no shell) with a 15s timeout.

New surface:
- `--containers-only` / `--images-only` (mutually exclusive) and
  `--category containers` / `--category images` to scope to runtime artifacts;
  runtime-only modes skip the filesystem walk entirely.
- `--older-than` is applied to dated artifacts; artifacts with no parseable
  creation date are kept (safe choice) with a warning explaining why.
- Per-item delete with separate success/failure tracking.

Rebased on top of LarsenCundric#3 (ignore-pattern handling) so there is a single glob/ignore
implementation: the duplicated `globToRegExp`, `--ignore`/config loading, and
`canCollectBloatAtDir` root-bloat fix that an earlier version of this branch
carried are dropped and inherited from LarsenCundric#3 instead.

Review fixes:
- error when --containers-only and --images-only are combined
- warn (don't silently drop) on unparseable Docker JSON output lines
- warn when --older-than keeps an artifact due to an unparseable date
- show the real Docker error dimmed when runtime cleanup is skipped
- combined early-return so runtime-only runs print/act correctly

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@NiveditJain NiveditJain force-pushed the feature/docker-purge branch from 29dbfca to 60d4a93 Compare May 26, 2026 21:04
@NiveditJain

Copy link
Copy Markdown
Author

Thanks — this was a great review. Force-pushed 60d4a93. Summary: the overlap is resolved by stacking on #3, and all the should-fix/minor points are addressed.

Critical

[P0] Overlap / diverged glob
Took option 1: rebased this branch on top of #3 and dropped the weaker duplicate. The globToRegExp, --ignore/config loading, and the canCollectBloatAtDir root-bloat fix are now inherited from #3 — there's one implementation, the stronger one. This PR is now purely src/docker.js + the runtime integration in src/index.js + Docker docs. Merge #3 first, then this (noted at the top of the description); until then the diff here also shows #3's changes, and collapses once #3 lands.

Should fix

[P1] printResults / early-return ordering
Walked it through. The old results.length === 0 return is replaced by a combined check that bails only when there's nothing actionable in either scope, after both summaries have printed. In runtime-only modes (--containers-only etc.) the filesystem table is skipped entirely (no misleading "No bloat found"), and only the relevant runtime section prints — verified against a live daemon.

[P1] --older-than keep-when-unparseable
Kept the conservative "keep when undated" behavior, but it now emits a warning naming the kept container/image and explaining --older-than couldn't be applied, so a surprise survivor is debuggable.

[P2] --containers-only + --images-only together
Now errors out explicitly (mutually exclusive; pass at most one) before doing anything — predictable for cron rather than silently picking one.

[P2] parseJsonLines silent failures
Each unparseable line now emits a dim warning (to stderr), so Docker format drift shows up as a breadcrumb instead of a silent "0 found".

Minor

  • Unavailable message + dropped error ✅ — printRuntimeSummary now prints "Docker unavailable — skipping runtime cleanup" plus the real Docker error dimmed, and the README wording matches.
  • package-lock.json noise ✅ — gone after the rebase; this PR no longer touches it.
  • --json excludes runtime — documented in README as a v1 limitation + planned follow-up.

Ready for re-review. I'll rebase/clean-collapse again right after #3 merges if you'd like a final pure-Docker diff before merging this one.

@NiveditJain

Copy link
Copy Markdown
Author

@LarsenCundricmerge this one after #3 (it’s stacked on it).

Ready to merge + ship 🚀

Both PRs are MERGEABLE / CLEAN and all review feedback is addressed. Since I only have read access here, the merge + publish are yours to do. Suggested order:

1. Merge #3 (fix-ignore-pattern-handling)   ← first
2. Merge #2 (feature/docker-purge)           ← stacked on #3; its diff collapses to just the Docker changes once #3 lands
3. On main:  npm version minor               # 1.0.0 -> 1.1.0
4.           npm publish                      # publishes the new features to npm

No release CI is configured, so step 3–4 are a manual publish by the npm package owner. Happy to prep the version bump as a separate PR if you’d prefer that to be in git first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants