Skip to content

fix(cron): re-validate stale cron-output entries before deletion #37721 - #37868

Closed
kyssta-exe wants to merge 1 commit into
NousResearch:mainfrom
kyssta-exe:auto-fix/issue-37721
Closed

fix(cron): re-validate stale cron-output entries before deletion #37721#37868
kyssta-exe wants to merge 1 commit into
NousResearch:mainfrom
kyssta-exe:auto-fix/issue-37721

Conversation

@kyssta-exe

Copy link
Copy Markdown
Contributor

Summary

Fixes #37721 — stale tracked.json entries with category: "cron-output" pointing to cron control-plane files (e.g. cron/jobs.json) could cause quick() to delete the live scheduler registry.

Root Cause

  1. guess_category() on fork was missing the fix(cron): stop silent cron-job loss across update (disk-cleanup + snapshot restore) #34840 fix — it classified ALL cron/* paths as cron-output, not just cron/output/**.
  2. quick() and dry_run() trusted the stored category without re-validating at delete time. Stale entries from before fix(cron): stop silent cron-job loss across update (disk-cleanup + snapshot restore) #34840 could carry category: "cron-output" for paths like cron/jobs.json.

Fix

  • Fix guess_category(): Only classify cron/output/** as cron-output (matching the fix(cron): stop silent cron-job loss across update (disk-cleanup + snapshot restore) #34840 fix on main).
  • Re-validate at delete time: In quick() and dry_run(), re-run guess_category() for cron-output entries. Stale entries that no longer match are skipped and removed from tracked.json.
  • Defense-in-depth: Add _is_protected_cron_path() that blocks deletion of cron, cronjobs, jobs.json, and .tick.lock regardless of stored category.

Changes

  • plugins/disk-cleanup/disk_cleanup.py: 3 fixes + new _is_protected_cron_path() helper
  • tests/plugins/test_disk_cleanup_plugin.py: 5 new regression tests in TestStaleCronEntryMigration, 1 existing test updated

Test Results

All 43 tests pass (38 existing + 5 new regression tests).

…sResearch#37721)

quick() and dry_run() previously trusted the stored category from
tracked.json without re-validating at delete time. Stale entries from
before NousResearch#34840 could carry category="cron-output" for cron control-plane
paths (e.g. cron/jobs.json), causing quick() to delete the live
scheduler registry.

Fix:
- Fix guess_category() to only classify cron/output/** as cron-output
  (was classifying ALL cron/* paths, missing the NousResearch#34840 fix).
- Re-validate cron-output entries via guess_category() at delete time
  in quick() and dry_run(); stale entries that are no longer classified
  as cron-output are skipped and removed from tracked.json.
- Add _is_protected_cron_path() as a hard defense-in-depth guard that
  blocks deletion of cron/cronjobs directories and known control-plane
  files (jobs.json, .tick.lock) regardless of stored category.
- Update test_cron_subtree_categorised to match fixed guess_category
  (only cron/output/* is cron-output, not all of cron/).

Tests: add 5 regression tests in TestStaleCronEntryMigration.
@alt-glitch alt-glitch added type/bug Something isn't working P1 High — major feature broken, no workaround comp/cron Cron scheduler and job management comp/plugins Plugin system and bundled plugins labels Jun 3, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Duplicate of #37726 (earliest open fix for #37721, triaged first). Also competes with #37760. All three re-validate cron-output paths before disk-cleanup quick()/dry_run() deletes them. Consolidate on one.

@teknium1

teknium1 commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

Merged via #39067 (commit 30412a9 on main). Your commit was cherry-picked with your authorship preserved in git log.

I dropped the guess_category() hunk during salvage since that fix already landed on main via #34840 — kept your genuinely-new contribution: the delete-time re-validation in quick()/dry_run() plus the _is_protected_cron_path() defense-in-depth guard, and all 5 of your regression tests.

Verified live: quick() against a stale tracked.json mislabeling cron/jobs.json as cron-output now preserves the registry intact while still cleaning genuine cron/output/ artifacts. Thanks for the fix and the clear root-cause writeup.

@teknium1 teknium1 closed this Jun 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/cron Cron scheduler and job management comp/plugins Plugin system and bundled plugins P1 High — major feature broken, no workaround type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: disk-cleanup can still delete cron/jobs.json from stale tracked.json entries

3 participants