Reserve run IDs atomically - #184
Merged
Merged
Conversation
Signed-off-by: Jakub Sztandera <oss@kubuxu.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves the reliability of run ID generation by turning it into an atomic “claim” operation: a candidate run ID is only accepted if its corresponding run directory can be created, preventing concurrent processes from sharing the same run ID and run state.
Changes:
- Update
generate_run_idto atomically reserve the run ID via directory creation, retrying on collisions. - Add internal helpers for candidate generation and reservation, and adjust tests to validate reservation behavior.
- Propagate run ID reservation errors to the CLI entrypoint (
main) viaResult.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/run_id/mod.rs |
Implements atomic run ID reservation using create_dir and updates tests to validate reserved IDs. |
src/main.rs |
Adapts to the new fallible generate_run_id() API by propagating errors with ?. |
Suppressed comments (1)
src/run_id/mod.rs:146
- This test unconditionally imports
std::os::unix::fs::symlink, which will fail to compile on Windows targets. Sincecreate_latest_symlinkhas Windows support, the test should be gated to Unix (or implemented with platform-specific symlink creation).
#[test]
fn test_create_latest_symlink_handles_broken_symlinks() {
// This test verifies that create_latest_symlink properly handles
// removing broken symlinks and creating new ones
use std::os::unix::fs::symlink;
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
Author
|
CI failed with |
rvagg
approved these changes
Aug 26, 2026
beck-8
approved these changes
Aug 26, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Rationale
Random run ID collisions can occur in tests with an estimated probability of 1 in 1,600. Concurrent processes currently have no atomic claim step, so duplicate IDs can cause flaky tests and shared run state. Atomic directory creation removes that race.
This change was split out of #180 so it can be reviewed and merged separately.
Verification