[#1880] Avoid panic when cleanup finds live node

[xiao-yang25]

Notes for Reviewer

During concurrent node creation on Windows, a starting node can temporarily be classified as dead. When stale-resource cleanup rechecks the node state, the monitoring layer correctly returns MonitoringCreateCleanerError::InstanceStillAlive.

This result was previously handled with fatal_panic!, terminating the unrelated process performing the cleanup.

This PR:

• maps InstanceStillAlive to NodeCleanupFailure::ResourcesAlreadyCleanedUp;
• prevents cleanup from continuing against a live node;
• avoids adding a new public error variant or changing the existing ABI;
• updates the Rust, C, and C++ error documentation;
• adds a deterministic conformance test covering IPC, local, and their thread-safe variants;
• adds the fix to the unreleased changelog.

Windows reproducer before the fix:

total=64 ok=36 returned_err=0 killed=28

After the fix:

total=64 ok=64 returned_err=0 killed=0

The same reproducer was also exercised on macOS/aarch64, Linux/aarch64 (Jetson Orin), and Linux/x86_64. Across 5,760 concurrent node creations, no processes were killed.

Validation

• cargo fmt --all -- --check
• Clippy with warnings denied
• Conformance test: 4 passed, 0 failed
• Windows concurrent reproducer: 64 passed, 0 killed

Pre-Review Checklist for the PR Author

• Add sensible notes for the reviewer
• PR title is short, expressive and meaningful
• Relevant issues are linked in the References section
• Branch follows the naming format
• Commit message contains the issue ID
• Tests cover the new behavior
• Changelog updated

References

Closes #1880

[xiao-yang25]

The remaining CI failures appear to be unrelated to this PR:

• nightly-check fails because the latest Nightly workflow run is currently failing.
• The unstable job fails while compiling the third-party zenoh 1.9.0 dependency with rustc 1.100.0-nightly. The Zenoh jobs using stable and Rust 1.89 pass on all tested platforms.

The regular main build does not run this unstable job because it is explicitly skipped for refs/heads/main. Since this PR does not modify the Zenoh integration or its dependencies, I have not changed the PR to work around the failure.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 76.40%. Comparing base (49178c1) to head (8c52143).
⚠️ Report is 15 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1936      +/-   ##
==========================================
+ Coverage   76.33%   76.40%   +0.06%     
==========================================
  Files         457      457              
  Lines       46068    46068              
  Branches     1489     1489              
==========================================
+ Hits        35165    35197      +32     
+ Misses       9641     9609      -32     
  Partials     1262     1262              

Flag	Coverage Δ
CPP	62.61% <ø> (ø)
Rust	76.25% <100.00%> (+0.06%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
iceoryx2/src/node/mod.rs	67.68% <100.00%> (+1.70%)	⬆️

... and 13 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[elBoberido]

@xiao-yang25 yes, the failures seem unrelated. There is a flaky test on Windows which we need to have a closer look at some time. I re-triggered the nightly, which suffered from the same flaky test on Windows.

The cleanup mechanism is not easy to get right and @elfenpiff is the final authority to tell whether this bugfix does not violate invariants. From my point of view, it should be okay and I guess we just overlooked the different behavior on Windows.

[elBoberido]

@xiao-yang25 the unstable build is not mandatory for the CI, so it is not a blocker. It seems to be an issue in Zenoh and it also occurs when the Zenoh dependency is updated to 1.10.

The Rust team is currently experimenting with a new trait resolver on the nightly Rust compiler and that might cause the issue. If it persists, we will temporarily disable the Zenoh tunnle build on the unstable builds.

[xiao-yang25]

@elBoberido Thanks for the clarification and for re-triggering the nightly. I will leave the unrelated CI failures as they are and wait for @elfenpiff's review regarding the cleanup invariants.

[elBoberido]

It seems the issue with the nighty compiler fixed itself over the weekend. We now just need to wait for @elfenpiff :)

[xiao-yang25]

Looks like nightly just needed the weekend off 😄 I’ll wait for @elfenpiff’s verdict.

[elfenpiff]

I am a bit hesitant to approve the PR. The reason I introduced a fatal_panic here was that when the node is detected as dead and, in the cleanup, as alive, we have a race in our implementation in iceoryx2-bb/posix/src/process_state.rs:982 in state().

It should, in theory, cleanly distinguish between in set up, alive, dead, and cleanup. Somehow, it categorized in set up falsly as dead, then the cleanup branch is triggered and the next state check identifies the node as alive.

The question is now: is this some weird Windows sync issue (and Windows has a lot of weirdness inside its stack), and we need to add something to the Windows platform layer, or is it a bug in Iceoryx2 in process_state.rs:982, and we need to fix it there? I will investigate this ...

[elfenpiff]

So I studied the implementation and the windows documentation. The problem seems to be that the initialization realized via file permissions can be insufficiently synchronized in Windows. In the documentation of FindNextFileW is a remark for the GetFileInformationByHandle function (who knows why, it is in the docs of an unrelated function)

Note:  In rare cases or on a heavily loaded system, file attribute information on NTFS file systems may not be current at the time this function is called. To be assured of getting the current NTFS file system file attributes, call the [GetFileInformationByHandle](https://learn.microsoft.com/en-us/windows/desktop/api/fileapi/nf-fileapi-getfileinformationbyhandle) function.

But we cannot use this function here, since we do not have a handle - it is a foreign process and we need to acquire the file information by path, which could lead to a misclassification in the ProcessState::state() function. So despite that the file permissions are set correctly, the other process sees a different version, misclassifies the process state and the cleanup path is used.

It seems that the second call to the resource cleanup established the synchronization and therefore caused a correct fatal_panic, but the bigger issue would be: what would happen if the misclassification still happens since the process state is still misclassified because of the sync issue? Then the process would remove the node by force from the service and with all of its files. This could actually corrupt the system.

I will talk tomorrow to @elBoberido after a good night's sleep, and let's see what we can come up with. If we get this wrong, and my suspicion is correct, such a fix could hide the bigger, more dangerous problem.

[xiao-yang25]

Thanks for the detailed investigation. Your concern makes sense: my change only handles the case where the second check observes the node as alive. If the state is misclassified repeatedly, cleanup could still proceed against a live node, so the change could indeed hide the more serious underlying issue.

One observation from the Windows PAL implementation: fstat appears to already have an open file handle, but permission retrieval is still performed by path through GetFileSecurityA. Microsoft documents [GetSecurityInfo](https://learn.microsoft.com/en-us/windows/win32/api/aclapi/nf-aclapi-getsecurityinfo) as retrieving an object’s security descriptor by handle. Would it be worth evaluating here, or is there an access or lifetime constraint I’m overlooking?

I’m happy to help test a revised approach on Windows.

[xiao-yang25]

I ran several local experiments to distinguish delayed DACL visibility from a broader lifecycle race.

Observations

In a controlled test where the initialization files remained unchanged for two seconds, the monitor observed Starting 5,793 times and never reported Dead.

I also ran an isolated cross-process DACL test with 10,000 permission transitions and 20,000 reads. After explicit synchronization, it observed no stale values, path/handle mismatches, or API errors. This does not rule out every narrow race, but I could not reproduce a simple delayed DACL update.

A separate stress test repeatedly created and removed a real ProcessGuard 3,000 times using the same path. This reproduced one transient Dead state. During the test, the Windows PAL frequently reported failures from GetFileSecurityA and occasionally from GetFinalPathNameByHandleA.

The current Windows fstat() implementation converts an existing handle back into a path before reading its DACL. If the file disappears or is recreated during that sequence, the permission lookup can fail. However, fstat() still returns success with no permission bits set, allowing ProcessState::state() to continue with incomplete information.

This appears to be a concrete path-based TOCTOU issue combined with a swallowed permission-read error.

Prototype

I prototyped the following changes:

• Read permissions directly from the existing handle using [GetSecurityInfo](https://learn.microsoft.com/en-us/windows/win32/api/aclapi/nf-aclapi-getsecurityinfo).
• Return an error from fstat() when the DACL cannot be retrieved.
• Use [SetSecurityInfo](https://learn.microsoft.com/en-us/windows/win32/api/aclapi/nf-aclapi-setsecurityinfo) for handle-based permission updates where possible.
• Request WRITE_DAC for exclusively created files and protect generated DACLs from inherited directory ACEs.

A deterministic regression confirmed that the previous fchmod() also depended on the path: updating permissions through an open descriptor failed after the path was removed. The handle-based implementation succeeds.

With the initial read-side fix, the original 3,000-iteration stress test completed without observing Dead.

Remaining observation

I then ran a stronger 5,000-iteration lifecycle test. It still observed one transient Dead state.

One possible explanation is a separate lifecycle race: the monitor may still hold an unlocked handle from the previous lifecycle while a new lifecycle is already starting under the same path. A later state check would then see the new lifecycle as Alive.

This suggests there may be two distinct effects:

1. A confirmed path-based permission race in the Windows PAL.
2. A narrower transition involving handles from different lifecycles.

The safety invariant seems to be that cleanup must not continue from incomplete or inconsistent state information. Permission-query failures should be propagated, and a later Alive result must always stop cleanup.

With the candidate changes, the complete Windows iceoryx2-bb-posix test suite passes: 327 passed, 4 ignored, and 0 failed.

The lifecycle test intentionally reuses the same path very aggressively, so it may not accurately represent the original failure. I do not want to overinterpret that result. Since this touches core process-state and cleanup logic, I think these findings require further careful discussion and validation before we draw a firm conclusion. The current tests may still miss relevant assumptions or Windows-specific behavior.