Skip to content

fix(cron): clean up output dir when auto-removing repeat-limited job - #22066

Open
nftpoetrist wants to merge 1 commit into
NousResearch:mainfrom
nftpoetrist:fix/cron-cleanup-output-on-repeat-limit
Open

fix(cron): clean up output dir when auto-removing repeat-limited job#22066
nftpoetrist wants to merge 1 commit into
NousResearch:mainfrom
nftpoetrist:fix/cron-cleanup-output-on-repeat-limit

Conversation

@nftpoetrist

Copy link
Copy Markdown
Contributor

What does this PR do?

mark_job_run() removes a job from jobs.json when its repeat limit is reached, but the corresponding OUTPUT_DIR/<job_id> directory (written by save_job_output() on every run) was never deleted. Each completed one-shot job leaves behind an orphaned output directory that accumulates indefinitely.

remove_job() gained the identical cleanup in PR #21882 (merged today). This PR extends that fix to mark_job_run(), the only other code path that deletes a job — parity with remove_job() lines 700–703.

One-line fix in mark_job_run(), scoped to the repeat-limit branch. No behavior change for recurring jobs or jobs without a repeat limit.

Related Issue

Fixes #22065

Type of Change

  • 🐛 Bug fix
  • ✨ New feature
  • 🔒 Security fix
  • 📝 Documentation update
  • ✅ Tests
  • ♻️ Refactor
  • 🎯 New skill

Changes Made

  • cron/jobs.py: add shutil.rmtree(OUTPUT_DIR / job_id) guard in the repeat-limit auto-delete branch of mark_job_run() (+4 lines)
  • tests/cron/test_jobs.py: two new tests in TestMarkJobRun (+33 lines)
    • test_repeat_limit_cleans_up_output_dir — verifies the dir is removed after auto-delete
    • test_repeat_limit_no_output_dir_is_safe — verifies no crash when no output was saved

How to Test

python3.11 -m pytest tests/cron/test_jobs.py::TestMarkJobRun -v --override-ini="addopts="

All 12 tests pass (1 skip for missing croniter — pre-existing on macOS).

Checklist

Code

  • Contributing Guide read
  • Conventional Commits
  • No duplicate PR
  • This fix only
  • pytest run
  • Tests added
  • Platform: macOS

Documentation & Housekeeping

  • Docs updated — N/A
  • cli-config.yaml.example — N/A
  • CONTRIBUTING.md/AGENTS.md — N/A
  • Cross-platform impact — N/A (shutil.rmtree is cross-platform)
  • Tool descriptions — N/A

mark_job_run() pops a job from jobs.json when its repeat limit is reached
(lines 737-741) but did not delete OUTPUT_DIR/<job_id>. save_job_output()
creates that directory on every run, so completed one-shot jobs leave one
orphaned output directory per job. remove_job() gained the identical cleanup
in PR NousResearch#21882 — this extends the fix to the auto-delete path inside
mark_job_run(), which is the only other code path that removes a job.

Fix: add the same OUTPUT_DIR/<job_id> rmtree guard used by remove_job(),
scoped to the repeat-limit branch. No behavior change for recurring jobs
or jobs without a repeat limit.

Fixes NousResearch#22065
@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/cron Cron scheduler and job management labels May 9, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for identifying the orphaned-output path. The premise is still valid on current main: cron/scheduler.py:3427 saves output before cron/scheduler.py:3484 calls mark_job_run(), while the repeat-limit branch at cron/jobs.py:1426-1430 removes only the job record.

Problems

  • The proposed OUTPUT_DIR / job_id path bypasses current path validation. cron/jobs.py:255-268 defines _job_output_dir() to reject escape attempts, and remove_job() resolves it before saving at cron/jobs.py:1363-1370. Use that same ordering in the repeat-limit branch.
  • Current main has two additional exhausted-one-shot removal paths: claim_dispatch() at cron/jobs.py:1501-1506 and _get_due_jobs_locked() at cron/jobs.py:1818-1830. Both can leave output behind, so fixing only mark_job_run() does not cover the current bug class.

Suggested changes

  • Resolve the safe output path before removing/persisting each job, then delete it after the store update.
  • Add seeded-output regression tests for the repeat-limit, dispatch-limit, and stale-due removal paths.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 13, 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 P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(cron): mark_job_run() auto-delete leaves orphaned output dirs when repeat limit is reached

3 participants