Skip to content

fix(cron): remove output dirs for completed one-shot jobs - #23560

Open
LeonSGP43 wants to merge 1 commit into
NousResearch:mainfrom
LeonSGP43:codex/22065-cron-output-cleanup
Open

fix(cron): remove output dirs for completed one-shot jobs#23560
LeonSGP43 wants to merge 1 commit into
NousResearch:mainfrom
LeonSGP43:codex/22065-cron-output-cleanup

Conversation

@LeonSGP43

Copy link
Copy Markdown
Contributor

What does this PR do?

Cleans up cron output directories when a completed one-shot job is auto-removed by the repeat-limit path, so stale output folders do not accumulate after the job record is deleted.

Related Issue

Fixes #22065

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

  • Added output-directory cleanup to the repeat-limit deletion path in cron/jobs.py
  • Added a regression test in tests/cron/test_jobs.py

How to Test

  1. Run uv run --frozen pytest -q -o addopts='' tests/cron/test_jobs.py -k 'repeat_limit_removes_job or repeat_limit_removes_output_dir'
  2. Run uv run --frozen ruff check cron/jobs.py tests/cron/test_jobs.py
  3. Confirm both commands pass

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: macOS 15.x

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

Screenshots / Logs

  • uv run --frozen pytest -q -o addopts='' tests/cron/test_jobs.py -k 'repeat_limit_removes_job or repeat_limit_removes_output_dir' -> 2 passed

@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 11, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Duplicate of #22066 — same fix adding output-dir cleanup in mark_job_run() repeat-limit path. Both fix #22065.

@alt-glitch alt-glitch added the duplicate This issue or pull request already exists label May 11, 2026

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for identifying the orphaned-output path. The premise remains valid on current main: cron/jobs.py:1426-1430 removes an exhausted job without cleanup, while save_job_output() creates its directory at cron/jobs.py:1914-1919.

Problems

  • The direct OUTPUT_DIR / job_id path bypasses _job_output_dir() (cron/jobs.py:255-268). Current remove_job() resolves the safe path before persisting deletion (cron/jobs.py:1363-1370); this branch needs the same fail-closed ordering.
  • Current main has two additional exhausted-one-shot deletion paths: claim_dispatch() (cron/jobs.py:1501-1506) and _get_due_jobs_locked() (cron/jobs.py:1818-1830). They need equivalent cleanup for the bug class to be covered.

Suggested changes

  • Resolve _job_output_dir(job["id"]) before removal/save, then clean it after persistence in all three paths.
  • Add seeded-output tests for repeat-limit, dispatch-limit, and stale-due removal.

Automated hermes-sweeper review.

Comment thread cron/jobs.py
# Remove the job (limit reached)
jobs.pop(i)
save_jobs(jobs)
job_output_dir = OUTPUT_DIR / job_id

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use _job_output_dir(job["id"]) and resolve it before jobs.pop() / save_jobs(), as remove_job() now does. Directly composing OUTPUT_DIR / job_id bypasses the current legacy-ID path-escape guard in cron/jobs.py:255-268.

@teknium1 teknium1 added sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data 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 duplicate This issue or pull request already exists P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform 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