Skip to content

fix(promise): record error before signalling channel in all()#4

Merged
abnegate merged 3 commits into
mainfrom
fix/promise-all-reject-race
Jun 8, 2026
Merged

fix(promise): record error before signalling channel in all()#4
abnegate merged 3 commits into
mainfrom
fix/promise-all-reject-race

Conversation

@abnegate

@abnegate abnegate commented Jun 8, 2026

Copy link
Copy Markdown
Member

Problem

The Swoole Coroutine adapter's all() rejection handler signals the coordination Channel before assigning $error:

}, function ($err) use ($channel, &$error) {
    $channel->push(true);          // <- signals first
    if ($error === null) {
        $error = $err;             // <- assigns after
    }
});

The awaiting coroutine's while ($ticks--) $channel->pop() loop can drain the channel and evaluate if ($error !== null) while $error is still null, so all() resolves instead of rejecting — violating the documented contract ("rejects when any input promise rejects").

This is timing-dependent: branches that reject after a coroutine yield (the realistic async case, e.g. an API call that sleeps then throws) reliably expose it, especially under scheduler pressure from other concurrent coroutines.

Fix

Assign $error before pushing to the channel — matching the ordering already used by the success handler in all() and by both handlers in allSettled().

Test

Adds an E2e regression (testAllRejectsWhenABranchRejectsAfterYielding) with an async branch that sleeps then throws alongside a slower-resolving sibling. It fails deterministically against the old ordering (all() resolves) and passes with the fix. Full Swoole Promise E2e suite: 24/24 green. Pint PSR-12 clean.

🤖 Generated with Claude Code

The Swoole Coroutine adapter's all() rejection handler pushed to the
coordination channel before assigning $error. The awaiting coroutine
could drain the channel and evaluate the still-null $error, causing
all() to resolve instead of reject — violating the documented "rejects
when any input promise rejects" contract. Branches that reject after a
coroutine yield (the realistic async case) reliably expose the ordering.

Assign $error before pushing, matching the ordering already used by the
success handler and by allSettled(). Adds an E2e regression covering an
async branch that rejects after yielding alongside a slower-resolving
sibling.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings June 8, 2026 04:01

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@greptile-apps

greptile-apps Bot commented Jun 8, 2026

Copy link
Copy Markdown

Greptile Summary

This PR fixes a timing-dependent bug in Coroutine::all() where the rejection handler pushed to the coordination channel before assigning $error, allowing the awaiting coroutine to drain the channel and observe a still-null error — causing all() to resolve instead of reject. The fix swaps the two lines so $error is recorded first, consistent with the success handler and with allSettled().

  • src/Promise/Adapter/Swoole/Coroutine.php — core ordering fix in all(); also corrects an incidental undefined-class reference in race() where new PromiseException(...) (no import, would fatal at runtime on an empty-array call) is replaced with new Promise(...) (Utopia\\Async\\Exception\\Promise, already imported and used by any()/all()).
  • src/Parallel/Pool/Swoole/Process.php — adds a kill($pid, 0) existence probe each non-reap iteration to drain stale PIDs from $pidsToWait, with an early-exit once the set is empty; the timeout kill loop also probes before sending SIGKILL.
  • src/Timer/Adapter/Sync.php — removes the now-redundant $running array and replaces the dual-check tick loop with while ($this->doExists($timerId)), which is equivalent since doExists checks isset($this->timers[$timerId]) and the adapter is fully synchronous.

Confidence Score: 5/5

Safe to merge — the change is a minimal, targeted reorder of two lines with a deterministic regression test that fails before the fix and passes after it.

The all() fix is correct and tightly scoped. The incidental fixes (undefined PromiseException in race(), dead-PID cleanup in Process, Sync tick simplification) are all improvements with no regressions. The new E2e test exercises the exact failure mode described in the PR.

No files require special attention.

Important Files Changed

Filename Overview
src/Promise/Adapter/Swoole/Coroutine.php Core fix: records $error before channel push in all() to eliminate the race; also corrects an undefined-class reference in race() (PromiseException → Promise)
src/Parallel/Pool/Swoole/Process.php Adds dead-PID cleanup probe (kill $pid, 0) each iteration when no child is reaped, and breaks early once pidsToWait empties; timeout kill loop also probes before sending SIGKILL
src/Timer/Adapter/Sync.php Simplifies doTick: removes separate $running array and replaces dual-check with a single doExists loop; behaviour is equivalent because Sync is fully synchronous
tests/E2e/Promise/Swoole/CoroutineTest.php Adds deterministic regression test for the all() ordering bug using two async branches that each sleep then resolve/reject

Reviews (2): Last reviewed commit: "fix: resolve PHPStan level-max errors in..." | Re-trigger Greptile

@github-actions

github-actions Bot commented Jun 8, 2026

Copy link
Copy Markdown

Benchmark Results

Swoole Adapters (Sync, Swoole Thread, Swoole Process, Amp, React)

╔══════════════════════════════════════════════════════════════════════════════╗
║                    Parallel Adapter Benchmark                                ║
╚══════════════════════════════════════════════════════════════════════════════╝

System Info:
  PHP Version: 8.4.22
  Swoole Version: 6.1.3
  CPU Cores: 4
  Iterations: 10 per test
  Load: 50% (workload intensity)

Detected adapters:
  [x] Sync
  [x] Swoole Thread
  [x] Swoole Process
  [x] Amp
  [x] React
  [x] ext-parallel

┌──────────────────────────────────────────────────────────────────────────────┐
│ CPU-Intensive Workloads                                                      │
└──────────────────────────────────────────────────────────────────────────────┘

  Prime calculation (4 tasks, primes up to 200000)
--------------------------------------------------------------------------------

ext-parallel Adapter (Sync, Amp, React, ext-parallel)

╔══════════════════════════════════════════════════════════════════════════════╗
║                    Parallel Adapter Benchmark                                ║
╚══════════════════════════════════════════════════════════════════════════════╝

System Info:
  PHP Version: 8.4.22
  CPU Cores: 4
  Iterations: 10 per test
  Load: 50% (workload intensity)

Detected adapters:
  [x] Sync
  [ ] Swoole Thread
  [ ] Swoole Process
  [x] Amp
  [x] React
  [x] ext-parallel

┌──────────────────────────────────────────────────────────────────────────────┐
│ CPU-Intensive Workloads                                                      │
└──────────────────────────────────────────────────────────────────────────────┘

  Prime calculation (4 tasks, primes up to 200000)
--------------------------------------------------------------------------------
  Sync:             0.396s (std: 0.017s, range: 0.382-0.432s)
  Amp:              0.242s (std: 0.047s, range: 0.182-0.353s)  1.63x speedup
  React:            0.323s (std: 0.044s, range: 0.284-0.392s)  1.22x speedup
  ext-parallel:     0.183s (std: 0.009s, range: 0.171-0.196s)  2.16x speedup
  Winner: ext-parallel (53.6% faster than sync)

  Matrix multiply (4 tasks, 200x200)
--------------------------------------------------------------------------------
  Sync:             1.149s (std: 0.008s, range: 1.142-1.171s)
  Amp:              0.453s (std: 0.045s, range: 0.432-0.575s)  2.54x speedup
  React:            0.571s (std: 0.044s, range: 0.501-0.612s)  2.01x speedup
  ext-parallel:     0.442s (std: 0.010s, range: 0.434-0.466s)  2.60x speedup
  Winner: ext-parallel (61.5% faster than sync)

┌──────────────────────────────────────────────────────────────────────────────┐
│ I/O-Simulated Workloads                                                      │
└──────────────────────────────────────────────────────────────────────────────┘

  Sleep tasks (4 tasks, 50ms each)
--------------------------------------------------------------------------------
  Sync:             0.200s (std: 0.000s, range: 0.200-0.200s)
  Amp:              0.067s (std: 0.048s, range: 0.051-0.204s)  3.00x speedup
  React:            0.115s (std: 0.001s, range: 0.115-0.116s)  1.74x speedup
  ext-parallel:     0.053s (std: 0.007s, range: 0.050-0.073s)  3.79x speedup
  Winner: ext-parallel (73.6% faster than sync)

  Mixed workload (4 tasks)
--------------------------------------------------------------------------------
  Sync:             0.135s (std: 0.001s, range: 0.132-0.138s)
  Amp:              0.064s (std: 0.042s, range: 0.051-0.183s)  2.11x speedup
  React:            0.114s (std: 0.001s, range: 0.113-0.114s)  1.19x speedup
  ext-parallel:     0.058s (std: 0.010s, range: 0.050-0.084s)  2.33x speedup
  Winner: ext-parallel (57.1% faster than sync)

┌──────────────────────────────────────────────────────────────────────────────┐
│ Scaling Benchmarks                                                           │
└──────────────────────────────────────────────────────────────────────────────┘

  Scaling test (1 tasks)
--------------------------------------------------------------------------------
  Sync:             0.040s (std: 0.000s, range: 0.040-0.040s)
  Amp:              0.041s (std: 0.019s, range: 0.035-0.094s)  0.96x speedup
  React:            0.079s (std: 0.002s, range: 0.074-0.082s)  0.50x speedup
  ext-parallel:     0.043s (std: 0.007s, range: 0.040-0.063s)  0.93x speedup
  Winner: Amp (-3.8% faster than sync)

  Scaling test (2 tasks)
--------------------------------------------------------------------------------
  Sync:             0.075s (std: 0.002s, range: 0.074-0.080s)
  Amp:              0.047s (std: 0.028s, range: 0.038-0.127s)  1.58x speedup
  React:            0.081s (std: 0.001s, range: 0.080-0.083s)  0.92x speedup
  ext-parallel:     0.043s (std: 0.007s, range: 0.040-0.064s)  1.76x speedup
  Winner: ext-parallel (43.0% faster than sync)

  Scaling test (4 tasks)
--------------------------------------------------------------------------------
  Sync:             0.149s (std: 0.001s, range: 0.148-0.150s)
  Amp:              0.080s (std: 0.036s, range: 0.051-0.179s)  1.86x speedup
  React:            0.163s (std: 0.047s, range: 0.131-0.232s)  0.91x speedup
  ext-parallel:     0.053s (std: 0.007s, range: 0.051-0.074s)  2.79x speedup
  Winner: ext-parallel (64.2% faster than sync)

  Scaling test (8 tasks)
--------------------------------------------------------------------------------
  Sync:             0.296s (std: 0.002s, range: 0.295-0.300s)
  Amp:              0.131s (std: 0.039s, range: 0.101-0.232s)  2.26x speedup
  React:            0.309s (std: 0.048s, range: 0.261-0.367s)  0.96x speedup
  ext-parallel:     0.105s (std: 0.008s, range: 0.102-0.128s)  2.83x speedup
  Winner: ext-parallel (64.7% faster than sync)
   16 tasks: Sync=0.475s, Amp=0.261s (1.8x), React=0.542s (0.9x), parallel=0.206s (2.3x)
   32 tasks: Sync=0.927s, Amp=0.523s (1.8x), React=1.044s (0.9x), parallel=0.410s (2.3x)

┌──────────────────────────────────────────────────────────────────────────────┐
│ Summary                                                                      │
└──────────────────────────────────────────────────────────────────────────────┘

  Adapter             Wins  Avg Speedup  Max Speedup
----------------------------------------------------
  Amp                    1        1.95x        3.00x
  React                  0        1.12x        2.01x
  ext-parallel           9        2.38x        3.79x

  Recommendation: ext-parallel for best overall performance.
  Theoretical max speedup: 4x (limited by CPU cores)


Benchmarks run with 10 iterations on GitHub Actions runner

abnegate and others added 2 commits June 8, 2026 16:57
During pool shutdown, workers that already exited and were reaped by
Swoole's own signal handling are no longer waitable, so the wait loop
ran to its 5s timeout and then SIGKILLed their stale PIDs. SwooleProcess::kill()
emits an E_WARNING on a dead PID, and PHPUnit's error handler converts
that into an uncaught NoTestCaseObjectOnCallStackException when it fires
from __destruct()/the shutdown function (no TestCase on the call stack),
crashing the Swoole Process suite.

Probe liveness with kill($pid, 0) — which sends no signal and does not
warn when the PID is gone — to drop already-reaped workers from the wait
set and to guard the timeout SIGKILL. This removes the warning and also
eliminates the wasted 5s timeout per shutdown (suite drops from ~2m to ~1s).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The CodeQL job runs `composer check` (PHPStan level max) and reported 14 errors:

- Coroutine::race() rejected with `new PromiseException(...)`, a class that
  does not exist — an empty-array race would fatal at runtime. Use the
  imported Utopia\Async\Exception\Promise, matching the other handlers.
- Timer\Adapter uses `new static()` in getInstance(); annotate the base
  class @phpstan-consistent-constructor as Promise\Adapter already does.
- Sync timer carried a $running array that was always in lockstep with
  $timers, so the analyzer proved its `?? false` guards redundant. Drop the
  redundant state and drive the tick loop off the existing doExists() check.
- Timer tests asserted assertIsInt() on values already typed int and
  compared `$count >= 1` after `$count++` (always true). Remove the
  redundant assertions and clear the tick on first fire.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@abnegate abnegate merged commit 3ee4fc3 into main Jun 8, 2026
19 checks passed
@abnegate abnegate deleted the fix/promise-all-reject-race branch June 8, 2026 05:10
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.

2 participants