Skip to content

[#1898] Add test coverage for signal-isolation behavior - #1900

Open
zacheryasc wants to merge 3 commits into
eclipse-iceoryx:mainfrom
zacheryasc:iox2-1898-signal-isolation-tests
Open

[#1898] Add test coverage for signal-isolation behavior#1900
zacheryasc wants to merge 3 commits into
eclipse-iceoryx:mainfrom
zacheryasc:iox2-1898-signal-isolation-tests

Conversation

@zacheryasc

@zacheryasc zacheryasc commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

The purpose is to complete test coverage that asserts the behavior #1898 prescribes, so a future fix PR can be validated against it. This PR does not fix #1898.

Coverage is spread across three test files:

Notes for Reviewer

Pre-Review Checklist for the PR Author

  • Add sensible notes for the reviewer
  • PR title is short, expressive and meaningful
  • Consider switching the PR to a draft (Convert to draft)
    • as draft PR, the CI will be skipped for pushes
  • Relevant issues are linked in the References section
  • Branch follows the naming format (iox2-123-introduce-posix-ipc-example)
  • Commits messages are according to this guideline
    • Commit messages have the issue ID ([#123] Add posix ipc example)
    • Keep in mind to use the same email that was used to sign the Eclipse Contributor Agreement
  • Tests follow the best practice for testing
  • Changelog updated in the unreleased section including API breaking changes

PR Reviewer Reminders

  • Commits are properly organized and messages are according to the guideline
  • Unit tests have been written for new behavior
  • Public API is documented
  • PR title describes the changes

References

Relates to #1898 #1458

@zacheryasc

Copy link
Copy Markdown
Contributor Author

Reasoning for the change in the epoll test behavior:

  • The previous watchdog-based guard would hang for over 10 minutes, and possibly flake if a signal slipped through somewhere, without actual guarantees on proper delivery.
  • The rewrite deterministically succeeds or fails with a send-receive check that asserts proper epoll delivery.

@zacheryasc

Copy link
Copy Markdown
Contributor Author

Reasoning for the change in the signalfd test behavior:

  • The previous delivery test used an unbounded retry loop that would spin forever when the fd was starved, never actually failing. The rewrite deterministically succeeds or fails within a bounded ~200 ms poll.
  • Two regression guards are added to pin the isolation contract any Signal handling lacks ownership and isolation across consumers, causing races and contention #1898 fix must preserve: dropping a SignalFd must not leave its signal monopolized away from the global handler, and owning one signal must not suppress delivery of an unrelated signal.

…rvation defect

A signal subscribed via `EpollBuilder::handle_signal` must be delivered
as `EpollEvent::Signal` even when the global `SignalHandler` has claimed
the disposition of that signal.

Today the claim starves the epoll internal signalfd: the subscribed
signal is never blocked for the fd, so `capture_signal` consumes it
first and the test fails. It stays `#[ignore]`d until the eclipse-iceoryx#1898 fix
lands; removing the ignore then verifies the fix.

Replaces the old retry-loop + 1000 s watchdog with a single
process-directed send and a bounded `timed_wait`, so a starved epoll
fails in ~200 ms instead of hanging on the watchdog.

Signed-off-by: Zachery Aaron Shores-Chmielewski <zacheryasc@gmail.com>
…on regression tests

A signal subscribed on a `SignalFd` must be delivered to the fd even
against competing SignalHandler calls. This implements the test guards
to ensure any fix for eclipse-iceoryx#1898 encodes the desired behavior.

`registered_signal_can_be_try_read` is rewritten from an unbounded loop
that hangs when the fd is starved into a bounded ~200 ms poll with
explicit handler activation, so a starved fd fails fast with a precise
assertion instead of spinning forever.

Two regression guards pin the isolation contract any fix must preserve:
`dropped_signal_fd_restores_handler_visibility` (dropping a fd must not
leave its signal monopolized away from the global handler) and
`signal_fd_does_not_mask_unsubscribed_signal` (owning one signal must
not suppress delivery of an unrelated signal).

Signed-off-by: Zachery Aaron Shores-Chmielewski <zacheryasc@gmail.com>
@zacheryasc
zacheryasc force-pushed the iox2-1898-signal-isolation-tests branch from 7dd3a8b to f4c3ab8 Compare August 10, 2026 11:08
@zacheryasc

Copy link
Copy Markdown
Contributor Author

Final coverage implemented. The reasoning for the change in signal test behavior:

  • directly and deterministically show we do not contend for the same signal
  • and we do not consume unrelated signals
  • and we do not deadlock

@zacheryasc
zacheryasc marked this pull request as ready for review August 10, 2026 11:23
@zacheryasc
zacheryasc force-pushed the iox2-1898-signal-isolation-tests branch from f4c3ab8 to f6caab6 Compare August 10, 2026 11:26
Replace the deadlocking `call_and_fetch_with_registered_handler_works`
guard with three tests:

- does_not_observe_unrelated_concurrent_signal [ignored]: a signal on
  another thread must not leak into a capture (eclipse-iceoryx#1898 race).
- observes_signal_directed_at_calling_thread: a self-directed signal must
  be captured (positive counterpart, passes today).
- with_registered_callback_completes [ignored]: capture + callback on the
  same signal must not deadlock, and both must be observed (eclipse-iceoryx#1458).

Signed-off-by: Zachery Aaron Shores-Chmielewski <zacheryasc@gmail.com>
@zacheryasc

Copy link
Copy Markdown
Contributor Author

@elBoberido This should provide coverage for the general problem exposed from our previous conversation. This PR and the associated issue is ready for review.

@codecov

codecov Bot commented Aug 10, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 76.30%. Comparing base (e1fbd77) to head (f6caab6).
⚠️ Report is 23 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1900      +/-   ##
==========================================
+ Coverage   76.21%   76.30%   +0.09%     
==========================================
  Files         452      454       +2     
  Lines       45109    45404     +295     
  Branches     1436     1436              
==========================================
+ Hits        34378    34645     +267     
- Misses       9525     9553      +28     
  Partials     1206     1206              
Flag Coverage Δ
CPP 62.71% <ø> (ø)
Rust 76.15% <ø> (+0.09%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.
see 10 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@zacheryasc

Copy link
Copy Markdown
Contributor Author

@elBoberido ping. This adds deterministic test coverage for race behavior and more when dealing with posix signals. It makes some extant tests more clear and straightforward, and checks for more possible violations of correct behavior.

@elBoberido

Copy link
Copy Markdown
Member

@zacheryasc sorry, this totally slipped my attention. Maybe @elfenpiff can take a look at the PR. He is deeper into this part of the code. I once had a brief look at the code in signal.rs and came to the conclusion that one could remove quite some code and potentially also get rid of LAST_SIGNAL, which opens the door to not have to use the mutex and run into deadlocks in a single threaded signal execution.

@zacheryasc

Copy link
Copy Markdown
Contributor Author

@zacheryasc sorry, this totally slipped my attention. Maybe @elfenpiff can take a look at the PR. He is deeper into this part of the code. I once had a brief look at the code in signal.rs and came to the conclusion that one could remove quite some code and potentially also get rid of LAST_SIGNAL, which opens the door to not have to use the mutex and run into deadlocks in a single threaded signal execution.

The issue, speaking broadly, is that posix signals are just very uncomfortable for concurrency, and twist your arm into building a singleton-router-ish signal handler. So we might solve single thread in such a way, but mutlithread is a different beast. I didn't make any architectural decisions here, though, except implied concurrency via the test construction. This is just setting guardrails to make correctness more explicit.

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.

Signal handling lacks ownership and isolation across consumers, causing races and contention

2 participants