telemetry: don't unlink the producer's file when attaching fails - #1826
EylonKrause wants to merge 8 commits into
Conversation
|
👋 Hi EylonKrause! Thank you for contributing to ai-dynamo/nixl. Your PR reviewers will review your contribution then trigger the CI to test your changes. 🚀 |
📝 WalkthroughWalkthrough
ChangesPreserve backing file on open errors
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
sharedRingBuffer::openCyclicBuffer() is the reader/attach path (create=false), but on two error branches it unlink()s the shared-memory file it only opened: header mmap failure and version mismatch. A reader built from a different NIXL version (or attaching to an exporter file from an older run) therefore deletes the producer agent's live telemetry file from the filesystem. The reader's other two error paths -- "File too small for buffer data" and the final whole-buffer mmap failure -- already only munmap and throw without unlinking, so this just makes all of openCyclicBuffer consistent: a reader never removes a file it did not create. The unlink()s in createCyclicBuffer() (the create=true path) are correct and left unchanged -- a creator may remove a file it just made. The file_fd unique-ptr still closes the descriptor on the error path. Signed-off-by: Eylon Krause <eylon1909@gmail.com>
5047ba2 to
c2779a7
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/utils/common/cyclic_buffer.tpp (1)
221-224: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate telemetry docs to match new mismatch handling.
The C++ reader no longer unlinks on version mismatch, but
docs/telemetry.mdstill documents unlink-on-mismatch behavior. Please update the docs contract to avoid operator confusion.🤖 Prompt for 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. In `@src/utils/common/cyclic_buffer.tpp` around lines 221 - 224, The version-mismatch handling in cyclic_buffer no longer unlinks the buffer, so the telemetry documentation contract is now outdated. Update docs/telemetry.md to reflect the current behavior described by the mismatch path in cyclic_buffer.tpp, using the existing version-mismatch handling and NIXL_ERROR semantics as the source of truth, and remove any mention that the reader unlinks on mismatch.
🤖 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.
Outside diff comments:
In `@src/utils/common/cyclic_buffer.tpp`:
- Around line 221-224: The version-mismatch handling in cyclic_buffer no longer
unlinks the buffer, so the telemetry documentation contract is now outdated.
Update docs/telemetry.md to reflect the current behavior described by the
mismatch path in cyclic_buffer.tpp, using the existing version-mismatch handling
and NIXL_ERROR semantics as the source of truth, and remove any mention that the
reader unlinks on mismatch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 97832167-6985-4419-b9fa-14521125d7fd
📒 Files selected for processing (1)
src/utils/common/cyclic_buffer.tpp
|
/build |
|
Disclosure: this contribution was authored with an AI coding assistant (Claude) and reviewed before submission. |
|
/build |
|
/build |
|
👀 Investigating |
|
🤖 CI Triage Agent — I have sufficient evidence for a confident diagnosis. Let me summarize. DiagnosisTimeline analysis of the log (timestamps show continuous activity, no multi-minute gaps):
Exit code 139 = 128 + 11 = SIGSEGV. This is a crash during shutdown, not a timeout — the process finished the transfer and printed its result, then segfaulted while tearing down the engine. This is confirmed by the Why it's intermittent / a race: The identical configuration ( The local same-node transfer path (added in #1428, "Enable local xfer for UCCL backend", [REDACTED:Hex High Entropy String]) plus the connection-teardown ordering in the destructor is the implicated area. Summary: The Root cause: Not a timeout — a crash on shutdown. The UCCL engine finished the transfer (result line printed, Implicated commit: [REDACTED:Hex High Entropy String] "Enable local xfer for UCCL backend (#1428)" (Pravein Govindan Kannan) — introduced/expanded the local-xfer and connection-lifecycle handling. (Definitive attribution requires the collected core dump backtrace.) File: Suggested fix:
Related: PR #1428 (local xfer for UCCL); no existing issue found for this exact SIGSEGV — recommend filing one and attaching the backtrace.
|
|
👀 Investigating |
|
🤖 CI Triage Agent — This is the UCCL plugin source. The crash is in the UCCL backend runtime path (a native SIGSEGV inside Let me confirm the crash context by examining the timeline. The key evidence in the log:
Exit 139 = SIGSEGV. The process crashed — there is continuous activity right up to the crash (transfers completing every few seconds, no multi-minute silent gap), so this is not a hang/timeout. It's a hard segfault in the UCCL WRITE VRAM→DRAM path on the cross-node (ETCD) run on I have enough evidence for a confident diagnosis. Summary: The Root cause: A native SIGSEGV inside the UCCL backend transfer path, not a timeout or hang. The log shows continuous benchmark activity right up to the crash (each transfer completing within seconds; no multi-minute silent gap), so the job was making progress and then hard-crashed. GNU
This is in the UCCL backend/ Implicated commit: unknown — not attributable to the tested commit File: Suggested fix:
Related: PR #1826 (the triggering PR — telemetry only, not the cause); UCCL history PRs #1428, #1595, #1271, #1151, #895. No existing issue specifically matches this segfault signature.
|
|
/build |
|
👀 Investigating |
|
🤖 CI Triage Agent — I have the key evidence. Let me analyze the two failing stages. Stage 124 (build_helper/x86_64/ucx-master): This is the "Compiling NIXL Docker Image" step for the ucx-master variant. The log shows the entire build actually succeeded — UCX master built and installed, NIXL built and installed, nixlbench built and installed, the image was tagged and pushed successfully. The failure came after the image push, in the environment allocation: Stage 200 (Allocate Environment): Same signature, and this is the real failing stage (~3610s duration ≈ 60 minutes). Let me examine the timestamps for the hang analysis:
That is a ~60-minute gap with zero output between the slurm allocation call and the next line. The This is not a code/build failure — the entire NIXL + UCX build compiled, linked, and installed cleanly. This is an infrastructure/resource-availability failure: the SLURM cluster had no free GPU node matching the request (or the scheduler control connection timed out) during the 1-hour immediate-allocation window. Note the largest gap here is a legitimate scheduler queue wait, not a hung application process — the job was Summary: The Root cause: SLURM could not allocate the requested GPU node within the 3600s immediate-allocation window on partition Implicated commit: unknown — not code-related. The failure is in CI infrastructure (SLURM/scctl allocation), independent of commit File: CI pipeline SLURM allocation step — Suggested fix:
Related: none found (infrastructure allocation timeout; not tied to a tracked issue/PR). |
|
/build |
|
/build |
|
👀 Investigating |
|
🤖 CI Triage Agent — The recent CI history confirms this is an actively-evolving DL EP pipeline (added in #1802). The failure has nothing to do with the PR's code changes. DiagnosisSummary: The Root cause: GPU cluster capacity/scheduling, not a NIXL code defect. The Docker image compile (stage 159) and NIXL/UCX/nixlbench builds all completed successfully. The terminal failure is:
The single ~1-hour gap is the Implicated commit: None. PR #1826 ( File: Slurm allocation invocation in the Suggested fix:
Related: PR #1802 (added the DL EP pipeline / allocation stage), PR #1889 ( |
|
👀 Investigating |
|
🤖 CI Triage Agent — The exact telemetry path isn't confirmed and I don't want to guess at source I can't read. Let me step back and focus on what the log definitively tells me, since I already have a strong log-based diagnosis. The critical facts from the log:
This means the This is a hang in nixlbench UCCL worker shutdown, not a slow test and not a timeout to be raised. Here is my diagnosis: Summary: The "Run Nixlbench tests" stage (v1.22.x pipeline, node 318) was ABORTED after Jenkins killed it with SIGTERM (exit code 143) following a ~47-minute hang. Root cause: A hang, not a slow test. The test script iterates through backend/op_type/seg_type combinations. All UCCL ASIO benchmarks produced results; the last one ( Implicated commit: unknown — the UCCL backend code ( File: Suggested fix:
Related: PR #1826 (the PR under test — telemetry, unrelated to the hang); PR #1724 "UCCL crash Fix + GDB trap"; commit 1cca7d3 (#1560, "Add sleep to prevent both nixlbench processes becoming clients") — prior UCCL/nixlbench process-coordination fix in the same test path.
|
|
👀 Investigating |
|
🤖 CI Triage Agent — I have enough evidence. Both failing stages (125 for ucx-v1.22.x and 150 for the corresponding Docker image compile) fail identically: the NIXL build itself succeeds completely, and the failure occurs during the final Let me note the key evidence and check timestamps around the failure. The critical lines:
This is a registry-side infrastructure failure, not a code or test failure. The 500 comes from the artifactory server while receiving a chunked layer upload. Note stage 124 (ucx-master variant) succeeded and stage 125 (ucx-v1.22.x variant) failed — same code, different registry outcome, which is characteristic of a flaky/overloaded registry rather than a code defect. Summary: The Root cause: Registry-side infrastructure error. The full NIXL/UCX/nixlbench compile and install succeeded and the image was tagged successfully. The failure is Implicated commit: None — commit [REDACTED:Hex High Entropy String] is not implicated; the failure is in artifact upload infrastructure. File: Not a source file. Failure at the Suggested fix: Retry the build — this is a transient artifactory HTTP 500. To harden the pipeline against recurrence, wrap the Related: none found (no issue/PR search performed; failure is infrastructure, not code).
|
|
👀 Investigating |
|
🤖 CI Triage Agent — I have a clear picture now. Both "Allocate DL EP Environment" stages failed identically. Let me confirm the timeline of the allocation failure. The evidence is clear from the "Allocate DL EP Environment" stage log:
The single ~60-minute gap (10:16 → 11:16) is the Summary: The Root cause: Cluster/infrastructure resource contention, not a code defect. The pipeline requested an exclusive node on partition Implicated commit: unknown — not attributable to commit File: The allocation logic lives in the pipeline Suggested fix: This is a transient infrastructure failure — retry the build once GB200 nodes are available; it is not caused by PR #1826. To reduce recurrence: (1) verify GB200NVL72 ( Related: none found. |
|
/build |
What?
sharedRingBuffer<T>::openCyclicBuffer()(src/utils/common/cyclic_buffer.tpp)is the reader/attach path (
create == false), but on two error branches itunlink()s the shared-memory file it only opened:mmapfailure (line 211)This PR removes those two
unlink()calls.Why?
A reader is attaching to a file that a producer agent created and is actively
using. Deleting it on the reader's error path removes the producer's live
telemetry file from the filesystem. The most realistic trigger is a version
mismatch: a reader built from a different NIXL version (or attaching to an
exporter file from an older run) reads
version != TELEMETRY_VERSIONand unlinksthe producer's file.
The reader's other two error paths already do the right thing — "File too small
for buffer data" (~line 236) and the final whole-buffer
mmapfailure (~line 247)both only
munmapandthrow, without unlinking. So this just makes all ofopenCyclicBufferconsistent: a reader never removes a file it did not create.The
unlink()s increateCyclicBuffer()(thecreate == truepath) are correctand left unchanged — a creator may remove a file it just made. The
file_fdunique-ptr still closes the descriptor on the error path.
Reproduction
A self-contained fs+mmap reproducer: a "producer" creates the file stamped with an
older version, and a "reader" attaches expecting a newer version.
How (verification)
buffer_exporter.cpp,telemetry.cpp— whichinstantiate
sharedRingBuffer/ includecyclic_buffer.tpp) in-tree with-Dsanitizer=address,undefined(exit 0).Happy to add a GoogleTest regression (construct the reader with a mismatched
version under
EXPECT_THROW, then assert the file still exists) intest/gtest/if you'd like one.
Related Issues
None.
Summary by CodeRabbit