fix(jobs): id reuse across restarts, same-second reconcile, kill race, trim-temp orphans#13
Conversation
…, trim-temp orphans - JobId::generate re-checks suffixed candidates and run() also treats an on-disk <id>.log as taken: a post-restart job could mint a retained id, truncating the old job's log, losing its own row to the PK conflict, and letting persist_final clobber the old row's final state. - Startup reconcile now flips rows with started_unix <= boot (was <), excluding ids live in this process while holding the jobs lock — a crash loop restarting within the same second left a dead job reading 'running' for up to 24h. Also fixes NOT IN () syntax for the empty case. - kill_job reports by final state when the KILL finds the group already gone after a delivered TERM — that's a successful kill, not a failure. - reap_orphans sweeps aged <id>.log.trim temps left by a crashed trim; no reaper path deleted them before. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
WalkthroughJob id generation now loops through suffix candidates re-checking availability instead of appending one fixed suffix. JobStore's restart reconcile excludes live in-memory job ids and widens staleness comparison, and rejects id reuse when a retained log exists. Reaper now sweeps aged ChangesJob lifecycle correctness fixes
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Process
participant JobStore
participant Database
participant Filesystem
Process->>JobStore: new(boot)
JobStore->>Database: snapshot in-memory job ids
JobStore->>Database: mark stale running (started_unix <= boot, id NOT IN ids) as failed
Process->>JobStore: run(job)
JobStore->>JobStore: JobId::generate loop
JobStore->>Filesystem: check {id}.log exists
Filesystem-->>JobStore: exists/not exists
JobStore-->>Process: unique job id assigned
Estimated code review effort: 3 (Moderate) | ~25 minutes Related PRs: None specified. Suggested labels: bug, jobs, reliability Suggested reviewers: None specified. Poem: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/jobs/reaper.rs`:
- Around line 462-472: The test setup in reaper’s retention-age block currently
uses the external touch command with the GNU-only -d `@0` flag, which is not
portable. Update the logic in the loop over trim_tmp, orphan, and known to set
the file’s modified time in-process using std::fs::File::set_modified with
UNIX_EPOCH instead of spawning touch, so the age-reset step works on BSD/macOS
as well.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: adcf214d-56f8-4315-837f-4de5b6a0c7b7
📒 Files selected for processing (3)
src/jobs/id.rssrc/jobs/mod.rssrc/jobs/reaper.rs
Version bump for the v1.3.0 release: GlitchTip error tracking (#12) + bug-fix PRs #13–#16. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Bug-hunt findings in the job engine, all with tests:
JobId::generate'sexistscheck only consulted the in-memory map, but the id namespace (DB rows +<id>.logfiles) survives restarts for 24h. A new job minting a retained id would truncate the old job's log, lose its own row to the PK conflict, andpersist_finalwould clobber the old row. Now the on-disk log counts as taken, and suffixed candidates (<base>-1, which also restarts each boot) are re-checked in a loop.started_unix < boot; a crash loop restarting within one wall-clock second missed the previous process's row. Now<= bootexcluding ids live in this process (snapshotted under the jobs lock, so this process's own jobs are never clobbered). Also fixes theNOT IN ()empty-list syntax error.kill_jobreturned false even though its own TERM worked. Now reports by final job state..log.trimtemps accumulated forever. A trim crashing between write and rename left a temp no reaper path ever matched (extensiontrim, notlog).reap_orphansnow sweeps them past the mtime gate.🤖 Generated with Claude Code
Summary by CodeRabbit