Skip to content

Make generated ids unique across processes - #10

Merged
AjayThorve merged 1 commit into
NVIDIA:mainfrom
AjayThorve:ajay/fix-runtime-id-collision
Jun 23, 2026
Merged

Make generated ids unique across processes#10
AjayThorve merged 1 commit into
NVIDIA:mainfrom
AjayThorve:ajay/fix-runtime-id-collision

Conversation

@AjayThorve

@AjayThorve AjayThorve commented Jun 23, 2026

Copy link
Copy Markdown
Collaborator

Problem

runtime_id (and invocation_id, plus the artifact roots derived from them) can collide between two concurrent runs on the process-backed path. smoke_sdk_concurrency fails intermittently — ~3/12 runs in a local baseline.

Root cause

new_id builds ids from a millisecond timestamp + a process-global AtomicU64 counter:

let counter = NEXT_ID.fetch_add(1, Ordering::Relaxed);
format!("{prefix}-{}-{counter}", now_millis())

The counter only differentiates ids within one process. A process-backed runner (FabricClient(command=("cargo","run",…))) spawns a fresh fabric-cli per call, so the counter resets to 1 every time. Two concurrent runs in the same millisecond both produce …-<millis>-1 → collision.

Fix

Include std::process::id(), which the OS keeps distinct across concurrently running processes, so ids stay unique across processes too. new_id is the single source for request/environment/runtime/invocation/event ids, so one change covers all. No new dependency.

Verification

  • smoke_sdk_concurrency: 3/12 fail before → 0/20 after.
  • cargo test --workspace green; smoke_cli / smoke_sdk / smoke_native_sdk pass (ids feed artifact dir names).
  • ids are opaque (nothing in the tree parses them by -).

Follow-up

Once this and the CI PR (#8) both land, smoke_sdk_concurrency can be re-added to ci_python.yml (it's excluded there only because of this flake).

Summary by CodeRabbit

  • Bug Fixes
    • Enhanced ID generation to ensure uniqueness across concurrent processes.

new_id derived uniqueness from a millisecond timestamp plus a process-global
atomic counter. A process-backed runner spawns a fresh fabric-cli per call, so
the counter resets to 1 every time and two concurrent runs in the same
millisecond produced identical ids (runtime_id, invocation_id, and the
artifact roots derived from them).

Include the process id, which the OS keeps distinct across concurrently
running processes, so ids stay unique on the process-backed path. Single fix
point: new_id feeds request/environment/runtime/invocation/event ids.

smoke_sdk_concurrency failed ~25% of runs before; 0/20 after.

Signed-off-by: Ajay Thorve <athorve@nvidia.com>
@coderabbitai

coderabbitai Bot commented Jun 23, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 908a39e3-4d09-42cf-9f9d-234bb9fdc98b

📥 Commits

Reviewing files that changed from the base of the PR and between 7c9b0c1 and 09e7281.

📒 Files selected for processing (1)
  • crates/fabric-core/src/runtime.rs

📝 Walkthrough

Walkthrough

The new_id helper in crates/fabric-core/src/runtime.rs now includes the current OS process ID in the generated identifier string, changing the format from prefix-<millis>-<counter> to prefix-<millis>-<pid>-<counter>.

Changes

Process ID in ID Generation

Layer / File(s) Summary
Include PID in new_id format
crates/fabric-core/src/runtime.rs
new_id now calls std::process::id() and embeds it between the timestamp and atomic counter in the formatted ID string; comments are updated to match.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~2 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: updating the ID generation function to include process ID, making generated IDs unique across concurrent processes.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@AjayThorve
AjayThorve merged commit 7f93642 into NVIDIA:main Jun 23, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant