Skip to content

fix(janitor): never delete DB-registered plugin install roots; log every deleted path - #334

Open
claudegoogl-sudo wants to merge 1 commit into
masterfrom
ops/janitor-registered-package-paths
Open

fix(janitor): never delete DB-registered plugin install roots; log every deleted path#334
claudegoogl-sudo wants to merge 1 commit into
masterfrom
ops/janitor-registered-package-paths

Conversation

@claudegoogl-sudo

Copy link
Copy Markdown
Owner

Thinking Path

  • Paperclip is the open source app people use to manage AI agents for work
  • A deployment host runs the disk-retention janitor (scripts/host-disk-janitor.mjs) on a daily cron. It prunes backup dumps, run logs, stale worktrees, and /tmp scratch, and raises a disk alarm.
  • Worktree pruning deletes a directory when it is not a git repo (or is a clean, pushed repo) and no file inside is newer than 30 days.
  • A deployed plugin install can have exactly that shape: a plain package copy has no .git, and its file mtimes match the source tag it was cut from. The janitor read a live install as an abandoned worktree and deleted it. The plugin worker lost its package tree and crash-looped.
  • The install registry (plugins.package_path in the embedded Postgres) already knows which directories are live installs. The janitor never asked it, and the cron log showed only counts, so the deleted directories were not even named.
  • This pull request makes the janitor ask the registry first, fail closed when the registry is unreachable, and name every path it deletes or excludes.
  • The benefit: a live install can no longer look like garbage, and any future deletion is attributable from the log alone.

Linked Issues or Issue Description

No public issue exists for this change. The underlying problem, in bug-report fields:

What happened

The daily janitor --apply run classified a live plugin install directory as a stale worktree and deleted it. The install was a plain copy: no .git, and every file mtime older than the 30-day cutoff. The plugin worker restarted into a missing package and crash-looped. The cron log recorded only worktrees: 3/181 dirs deleted and could not name the directories.

Expected behavior

A directory at, inside, or containing a path registered as a live plugin install is never deletion-eligible, whatever its age or git state. Every deleted path is named in the output.

Steps to reproduce

  1. Copy a plugin package into a directory under the janitor's worktree scan root. Do not init git. Age all files past the cutoff (touch -d).
  2. Register that directory as plugins.package_path in the embedded Postgres.
  3. Run node scripts/host-disk-janitor.mjs --dry-run. Before this change the directory is listed as a deletion candidate. After this change it is excluded with the registered root named as the reason.

Version or commit

Janitor as deployed from the ops/host-disk-janitor merge (the file is byte-identical to scripts/host-disk-janitor.mjs before this PR).

Deployment mode

Self-hosted single host; embedded Postgres reachable over its UNIX socket only (listen_addresses empty); janitor runs unprivileged from a cron entry.

What Changed

  • New loadRegisteredPackagePaths(): reads plugins.package_path from the embedded Postgres over its UNIX socket (socket dir + port parsed from postmaster.opts; data dir picked from ps with a preference for the instance data dir). Credential comes from PGPASSWORD or the <dataDir>.pg-credential file, read only at mode 0600; the value is never printed or logged.
  • New findRegisteredOverlap(): a candidate directory that is equal to, inside, or containing a registered root is excluded. Both directions matter: deleting a parent would destroy a registered install inside it.
  • evaluateWorktree() / evaluateTmpEntry() take the registered set and never return eligible for an overlapping path. Exclusion is independent of age and git state.
  • Fail closed: if the registry lookup fails (Postgres down, credential missing or loose, no pg driver), worktree and /tmp pruning is disabled for that run. The output prints the failure loudly and lists what would otherwise have been deleted. Backup and run-log pruning still run.
  • Attribution: the human-readable output now lists every backup, run-log, worktree, and /tmp path deleted or would-be-deleted, one per line, plus per-candidate exclusion reasons and the full registered-path list. The JSON summary carries the same fields (prunedPaths, excludedRegistered, guardFailureExcludedPaths, registeredPackagePaths).
  • Header safety-model docs updated to describe the new invariant.

Verification

  • node --test scripts/host-disk-janitor.test.mjs -> 43 pass, 0 fail (34 existing + 9 new). Existing run() tests now inject a stub registry, so the suite is hermetic and does not need a Postgres (the real lookup fails closed by design, which would zero the worktree/tmp categories on a CI runner).
  • New tests cover: registry-aware evaluation (registered plain-copy install is never eligible; unregistered twin stays eligible), overlap in both directions, credential mode enforcement, parser quoting tolerance, JSON override, fail-closed run behavior, and exclusion reporting with the matched root.
  • Fixture on a live deployment host: a scratch database on the same Postgres socket registered a temp tree with touch -d "60 days ago" files. Dry-run output showed excluded (registered package path root: ...) for the registered tree and for a parent directory containing a registered root, while an unregistered aged control stayed listed as would delete. Fixture database dropped afterwards.
  • Before/after dry-run of the patched script on the same host: all category counts identical (backups 0/37, run-logs 2/13280, worktrees 1/180, review exclusions 69); the only new lines are the registered-path list, per-path would-delete lines, and the exclusion lines. The four registered install roots (messenger, cad, klipper, vault) are listed and the live messenger tree is explicitly excluded.

Risks

  • Fail-closed trade-off: while the registry lookup fails, worktree and /tmp pruning pause for that night. Backup and run-log pruning still run, and the disk alarm is unchanged. The pause is printed loudly, so it should not fail silently.
  • The pg driver is loaded through createRequire from an explicit candidate list (global paperclipai module, checkout node_modules, bare pg). If none resolves, the run fails closed rather than skipping the registry check.
  • The lookup assumes the embedded cluster publishes postmaster.opts next to its data dir. A TCP-only deployment fails the lookup and fails closed; it never misdeletes.
  • Related open PRs fix(db-backup): make the dump job crash-safe against empty/truncated gzip output #156/fix(ops): gate /tmp scratch reaping on process liveness, not a dead 30-day age cutoff #158 touch the same script (backup verification and /tmp liveness gating). Logical concerns do not overlap; whichever merges second needs a textual rebase.
  • Security review requested before merge: the new lookup reads the embedded-Postgres credential file. Mode 0600 is enforced, the value never enters output, logs, or errors, and connection errors redact it. This mirrors the credential handling already shipped in the messenger drift check.

For core feature work, check ROADMAP.md first and discuss it in #dev before opening the PR. Feature PRs that overlap with planned core work may need to be redirected — check the roadmap first. See CONTRIBUTING.md.

Model Used

Paperclip prime_local adapter (provider-routed model; the session does not expose the exact backing model id), extended thinking + tool use

Checklist

  • I have included a thinking path that traces from project context to this change
  • I have specified the model used (with version and capability details)
  • I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work
  • I have searched GitHub for duplicate or related PRs and linked them above
  • I have either (a) linked existing issues with Fixes: # / Closes # / Refs # OR (b) described the issue in-PR following the relevant issue template
  • I have not referenced internal/instance-local Paperclip issues or links (only public GitHub #NNN / github.com/paperclipai/paperclip URLs)
  • My branch name describes the change (e.g. docs/..., fix/...) and contains no internal Paperclip ticket id or instance-derived details
  • I have run tests locally and they pass
  • I have added or updated tests where applicable
  • I have updated relevant documentation to reflect my changes
  • I have considered and documented any risks above
  • All Paperclip CI gates are green (CI in progress at submission; result posted as a follow-up comment)
  • Greptile is 5/5 with no open P2s, recommendations, or follow-ups (N/A: Greptile does not engage on this fork's pull requests; noted rather than ticked)
  • I will address all Greptile and reviewer comments before requesting merge

…ery deleted path

The 2026-09-09 02:00Z apply reaped the live paperclip-messenger deploy tree
(plugins.package_path = ~/work/paperclip-messenger-live-0.2.20): a plain-copy
install has no .git and mtimes as old as its source tag, which classifyWorktree()
+ the age check read as an abandoned worktree. The cron log could not even name
the deleted dirs.

- Load plugins.package_path from the embedded Postgres (UNIX socket derived
  from the running postmaster, credential file enforced mode 0600, value never
  printed). A directory at, inside, or containing a registered root is never
  deletion-eligible regardless of age or git state.
- Fail closed: if the registry lookup fails, worktree and /tmp pruning is
  disabled for that run and the reason is printed loudly (under-deleting is
  recoverable; deleting a live install is not).
- Attribute every deletion: human output now lists every backup/run-log/
  worktree/tmp path it deletes or would delete, plus per-candidate exclusion
  reasons (registered root, lookup-failure) and the registered-path list.

Deployed copy on the host updated in the same shape (.bak kept beside it);
live dry-run verified: messenger/cad/klipper/vault roots listed and the live
messenger tree explicitly excluded; all other category counts unchanged.

Tests: 34 existing + 9 new (hermetic via injected registry), 43/43 pass.

Co-Authored-By: Paperclip <noreply@paperclip.ing>
@claudegoogl-sudo
claudegoogl-sudo force-pushed the ops/janitor-registered-package-paths branch from 471cb5c to c949440 Compare September 9, 2026 02:40
@claudegoogl-sudo

Copy link
Copy Markdown
Owner Author

Pushed c949440: the first push failed the policy check because an explanatory comment and two test names cited an internal ticket id. All internal references are reworded to plain English (dates + description); no code paths changed. Test suite still 43/43 (node --test scripts/host-disk-janitor.test.mjs). Review note for the security-sensitive part: the new lookup reads the embedded-Postgres credential file with mode-0600 enforcement and never prints the value — same pattern as the messenger drift check.

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.

1 participant