fix(cron): reserve job IDs atomically#686
Conversation
|
Warning Review limit reached
Next review available in: 30 minutes Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughCron job creation now atomically reserves job directories before writing metadata, retries conflicting IDs, cleans up failed reservations, and adds concurrency and failure-cleanup tests. ChangesCron job reservation flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant StoreAdd
participant reserveID
participant FileSystem
participant writeReservedJob
StoreAdd->>reserveID: Reserve a unique job directory
reserveID->>FileSystem: Atomically create directory
reserveID-->>StoreAdd: Return reserved job ID
StoreAdd->>writeReservedJob: Write metadata
writeReservedJob->>FileSystem: Persist metadata
writeReservedJob->>FileSystem: Remove reservation on failure
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR fixes a race in internal/cron where concurrent Store.Add calls could allocate the same timestamp-based job ID by switching from a check-then-create flow to an atomic directory reservation, and adds regression tests to validate concurrent adds and reservation cleanup behavior.
Changes:
- Replace
allocIDwithreserveID, reserving job IDs atomically viaos.Mkdiron the job directory. - Add
writeReservedJobto clean up reserved job directories when initial metadata persistence fails. - Add concurrent-add and reservation-cleanup tests using a frozen clock to force collisions deterministically.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| internal/cron/store.go | Atomically reserves job IDs by directory creation and cleans up reservations on write failure. |
| internal/cron/store_test.go | Adds concurrent allocation regression coverage and validates reservation cleanup on metadata write failure. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| return "", fmt.Errorf("reserve cron job id %q: %w", id, err) | ||
| } | ||
| return "", errors.New("could not allocate a unique cron job id") |
| func (s *Store) writeReservedJob(job Job) error { | ||
| if err := s.writeJob(job); err != nil { | ||
| if cleanupErr := os.RemoveAll(s.jobDir(job.ID)); cleanupErr != nil { | ||
| return errors.Join(err, fmt.Errorf("remove failed cron job reservation: %w", cleanupErr)) |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
- [P1] Coordinate removal with an in-flight reservation
internal/cron/store.go:102
reserveIDmakes the timestamp directory visible before the metadata is written, butRemovetreats that empty directory as a committed job and deletes it under a lock thatAddnever takes. A concurrentzero cron rm <timestamp-id>can therefore report success after deleting the reservation;writeJobthen callsMkdirAll, recreates the directory, and persists the job anyway. A subsequent add can also re-reserve that same timestamp while the original writer is still running, putting two writers back on the shared metadata path. Hold the same per-ID lock from reservation through initial persistence, or make removal reliably distinguish and coordinate with an uncommitted reservation, and cover the add/remove/add interleaving.
Address code review on PR Gitlawb#686: reserveID made the timestamp directory visible via os.Mkdir before metadata.json existed, but Remove (which takes the per-ID lock before deleting) never coordinated with that window. A concurrent `zero cron rm <id>` could delete the bare reservation and report success right after Add reserved it; writeJob's MkdirAll would then silently resurrect the directory and persist the job anyway. A second Add could also re-reserve the same id while the first writer was still running, putting two writers on the same metadata path. reserveID now acquires the same per-ID lock (used by Get/Update/ Mutate/Remove/AppendRun) for each candidate id before attempting os.Mkdir, and returns it still held; Add releases it only after writeReservedJob commits (or tears down) the reservation. A concurrent Remove for that id now blocks until the reservation is resolved one way or the other, and a concurrent Add attempting the same candidate id serializes through the lock rather than only racing the Mkdir. Adds two regression tests: one proving Remove waits for and removes the committed job rather than the bare reservation, and one proving a second reserveID call for the same candidate id blocks on the shared lock and moves on to the next candidate. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Review
The atomic Mkdir reservation plus holding the per-ID lock from reservation through writeReservedJob closes the race @jatmn flagged on the first commit (55d8c55). Remove can no longer delete a bare reservation while Add is still persisting, and a second Add cannot re-enter the same id until the first writer commits or tears down.
Lock lifecycle: reserveID acquires lockJob before Mkdir, returns the lock held on success, and releases it on collision or non-ErrExist mkdir errors. Add's defer unlock() covers both success and writeReservedJob failure (cleanup runs under the held lock; lock file lives outside the job dir).
Tests: go test ./internal/cron -count=1 -race passes locally, including the concurrent-add (32 goroutines), Add-vs-Remove, lock-serialization, and failed-reservation cleanup regressions.
Two minor follow-ups (non-blocking):
- Rename the exhausted-id error from "allocate" → "reserve" (
store.go:149). - Include
job.IDin the reservation cleanup failure message (store.go:184).
Process note: Issue #678 still lacks the issue-approved label — maintainers should add it before merge per CONTRIBUTING.md.
Otherwise LGTM — ready to merge.
anandh8x
left a comment
anandh8x
left a comment
There was a problem hiding this comment.
Re-reviewing against commit ef7537b1. The per-ID lock is now held from reserveID through writeReservedJob (deferred unlock in Add, returned unlock from reserveID, acquired via lockJob before Mkdir), which closes the P1 race I flagged. The two new regression tests cover both halves of the finding — concurrent Remove waiting on the lock, and a second reserveID serializing on the same lock rather than racing the Mkdir — and the existing 32-way concurrent add test still holds. Withdrawing my prior CHANGES_REQUESTED. LGTM.
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
The atomic reservation is sound — holding the per-ID lock from Mkdir through the metadata.json commit closes the Remove-vs-Add resurrect race, and the rest of the cron suite is green under -race. But TestStoreAddReservesUniqueIDsAcrossConcurrentStores is flaky: the frozen clock (time.Unix(1000,0)) pins all 32 writers to the same base id, so they convoy through one per-ID lock at a time and, under load, the tail waiters blow past the 10s lock timeout (I reproduced it under CPU load with "timed out acquiring job lock for 19700101-001640" and "unique jobs = 27, want 32"). Either dial the concurrency down to ~8 or advance the injected clock per writer so they don't all collide on id 0.
Summary
Root cause
Store.Addpreviously checked for an available ID before creating its directory. Concurrent stores could both observe the same timestamp-based ID as available and then write through the same temporary metadata path.Validation
go fmt ./...go vet ./...go test ./internal/cron -count=1govulncheck ./...(no vulnerabilities)go test ./...run: cron and all other packages pass except the existing Windows process-cleanup failures ininternal/toolsmake lint: blocked by the existing Windows-incompatiblefmt-checkshell commandFixes #678
Summary by CodeRabbit