Skip to content

fix(test): avoid restarting finished processes in batch subtest#84

Open
okurz wants to merge 3 commits into
masterfrom
fix/flaky_parallel_batch_test
Open

fix(test): avoid restarting finished processes in batch subtest#84
okurz wants to merge 3 commits into
masterfrom
fix/flaky_parallel_batch_test

Conversation

@okurz

@okurz okurz commented Jul 6, 2026

Copy link
Copy Markdown
Member

Fix 1: t/02_parallel.t 'batch' subtest race (motivation for this PR)

CI for #83 failed on t/02_parallel.t line 74 (macOS, latest Perl):

#   Failed test at t/02_parallel.t line 74.
#          got: undef
#     expected: 'Hello world 3\n'

Root cause: after adding a 3rd process, $c->start() was called on the whole
Pool, which proxies to every member, re-forking the 2 already-finished
processes and racing with the new process's output. Fixed by using
$c->last->start() to start only the newly added process (matching the
pattern already used later in the same subtest).

Fix 2: t/13_shared.t semaphore-creation race

After fix 1, CI still failed in t/13_shared.t with pid counts off from the expected 20/21.

Root cause: Shared::Semaphore::_create() looks up an existing semaphore and,
if missing, creates one with IPC_CREAT|IPC_EXCL. When several forked
processes race to create the same semaphore concurrently, the losing
process's create call fails with EEXIST and confess()s, killing that
child instead of just attaching to the semaphore the winner created.

Fix: on a failed create, fall back to a lookup instead of failing (optimistically create first, fall back on failure to attach).

Fix 3: t/13_shared.t shared memory attachment resource leak

After fix 2, some runs under high resource contention / stress still failed in t/13_shared.t with shm_nattch > 0 assertions.

Root cause: IPC::SharedMem objects do not automatically detach from their shared memory segments upon garbage collection when they go out of scope. They only detach if detach() is explicitly called. This resulted in a cumulative resource leak of active memory attachments in the parent process from previous subtests (such as free_mem or manual locks), causing shm_nattch == 0 assertions to fail.

Fix: Added an explicit detach() call wrapped in eval inside Memory::DESTROY, ensuring all active attachments are cleaned up automatically as soon as a shared_memory helper instance goes out of scope.

Testing

  • perl -Ilib t/02_parallel.t passes repeatedly.
  • TEST_SHARED=1 perl -Ilib t/13_shared.t passes repeatedly (under extreme concurrent load + background stress testing).
  • TEST_SHARED=1 TEST_SUBREAPER=1 prove -l t (full suite) passes repeatedly.
  • perltidy reports no formatting changes needed for modified files.

Motivation: t/02_parallel.t's 'batch' subtest called `$c->start()`
on the whole Pool after adding a 3rd process. Pool proxies `start()`
to every member, so the 2 already-finished processes got re-forked
too, racing with the new process's output under CI resource
constraints (observed failing on macOS latest-Perl runners).

Design Choices: use `$c->last->start()` to start only the newly
added process, matching the pattern already used later in the same
subtest.

Benefits: removes unnecessary re-forking and the associated race,
making the test deterministic.

Related issue: #83 (review)
@codecov

codecov Bot commented Jul 6, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.61%. Comparing base (5831e64) to head (463ec35).

Additional details and impacted files
@@           Coverage Diff           @@
##           master      #84   +/-   ##
=======================================
  Coverage   91.60%   91.61%           
=======================================
  Files          45       45           
  Lines        2848     2849    +1     
=======================================
+ Hits         2609     2610    +1     
  Misses        239      239           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

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

Motivation: t/13_shared.t is flaky under concurrency (e.g. macOS CI
runners): Semaphore::_create() looks up an existing semaphore, and if
missing, creates one with IPC_CREAT|IPC_EXCL. When multiple forked
processes race to create the same semaphore, the loser's create call
fails with EEXIST and confess()es, killing that child instead of
just using the semaphore the winner created. Reproduced locally: 30
processes racing to create the same semaphore key fail ~15-35% of
the time; after the fix, 0/30 across repeated runs.

Design Choices: on a failed create, fall back to a lookup instead of
failing outright, since a failed create in this code path can only
mean another process won the race and already created it.

Benefits: removes a source of spurious child-process deaths under
concurrent semaphore/lock creation, without changing the semaphore's
external behavior.

Related issue: https://github.com/openSUSE/Mojo-IOLoop-ReadWriteProcess/actions/runs/28792986622/job/85376167573?pr=84
@okurz okurz force-pushed the fix/flaky_parallel_batch_test branch from b8fecea to 463ec35 Compare July 6, 2026 19:46
@okurz

okurz commented Jul 6, 2026

Copy link
Copy Markdown
Member Author

refactored code to ensure full coverage

# Try acquiring already existing semaphore
my $sem = IPC::Semaphore->new($key, $self->count, 0);
unless (defined $sem) {
# Try creating it. IPC_EXCL in flags fails with EEXIST if it already

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sentence isn't really comprehensible. What is "it" referring to? Maybe:

Suggested change
# Try creating it. IPC_EXCL in flags fails with EEXIST if it already
# Try acquiring a possibly existing semaphore using the IPC_EXCL flag. This will fail with EEXIST if it already

Motivation: t/13_shared.t was flaky on macOS CI runners, failing with
incorrect pid counts and stale process attachments. IPC::SharedMem
objects do not automatically detach from shared memory when garbage-
collected, causing persistent attachment leaks in the parent and child
processes that eventually corrupt and exhaust shared resources.

Design Choices: added an explicit detach() call to Memory::DESTROY
wrapped in an eval block, ensuring any active shared memory attachments
are safely released whenever a shared_memory instance goes out of
scope, while still executing cleanups (remove) when destroy is set.

Benefits: completely eliminates persistent shared memory attachment
leaks, ensuring clean test isolation, resource reclamation, and stable
execution under concurrent/stress environments.
@Martchus

Martchus commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator

Although it would still be nice to improve the wording as mentioned before.

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.

3 participants