Skip to content

Fix subject deadlocks during concurrent cancellation - #59

Merged
twittemb merged 1 commit into
mainfrom
codex/fix-subject-cancellation-deadlock
Oct 3, 2026
Merged

twittemb merged 1 commit into
mainfrom
codex/fix-subject-cancellation-deadlock

Conversation

@twittemb

@twittemb twittemb commented Oct 3, 2026 •

Copy link
Copy Markdown
Contributor

Description

When a subject sends a value or termination while a suspended consumer is cancelled, delivery can hold the subject state lock while continuation resumption waits for the task-status lock. The synchronous cancellation handler holds the task-status lock and waits for the subject lock in unregister(), producing a deadlock. The buffered channels already resume outside their own locks; the enclosing subject lock is the problem.

Apply the channel-snapshot solution from the issue analysis to all six subjects:

  • Capture subscriber channels under the subject lock, then send, finish, or fail after releasing it.
  • Update current-value/replay state in the same critical region as the snapshot. Record termination and remove subscribers before delivering it.
  • Add cancellation regressions that register a consumer and wait for its installed continuation before racing cancel() against value delivery, finish, and throwing failure. The six tests exercise 15,000 races and verify consumer exit, with bounded test timeouts.
  • Update the changelog. No public API or dependency changes.

Closes #52.

Validation

Rebased without conflicts onto main at 495d616, which includes merged #57. The new subscriber-registration implementation and all ten race regressions from #57 are retained unchanged.

On an Apple Silicon Mac with Apple Swift 6.4:

  • Before the original fix: the new passthrough regression failed with a 30-second timeout. A process sample confirmed both sides of the lock inversion: cancel → unregister → subject lock and subject.send → channel.send → continuation.resume → task-status lock.
  • swift test --filter 'Async.*Subject.*Tests': 49 tests passed, including 100,000 subscription races from Fix race condition in subjects #57 and 15,000 cancellation races from this PR across all six subject variants.
  • swift test --enable-code-coverage -Xswiftc -suppress-warnings: 148 tests passed on the confirmation run after rebasing.
  • swift build -Xswiftc -suppress-warnings: passed.
  • git diff --check: passed.

The first full-suite run after rebasing hit the existing missing-value assertion in AsyncMulticastSequenceTests.test_multiple_loops_uses_provided_stream ([1, 1] instead of [1, 1, 1]). The same assertion also failed in an isolated checkout of the newly updated, untouched main at 495d616. All subject tests passed in both full-suite runs. This separate multicast race remains outside the cancellation fix; the green confirmation run does not establish that the suite is free of flakiness.

GitHub Actions attempt 1 for the rebased commit passed the build and all subject tests, but failed on the same multicast assertion reproduced on current main. The single retry (attempt 2) passed the full test job, coverage generation, and upload. Both build and test checks are green for the rebased commit a1b8658. No tests were skipped or assertions weakened.

Compatibility and ordering

This PR builds on #57's atomic subscriber registration. Replay and immediate termination remain inside handleNewConsumer's critical region: that channel is still private, has no awaiting consumer, and those operations cannot resume a continuation. Values and termination sent to already registered channels are delivered outside the subject lock, removing the deadlock while retaining the registration fix.

The snapshot removes the subject-lock/task-status-lock inversion without introducing a second delivery lock. Concurrent sends can interleave their deliveries after taking their snapshots; callers needing strict order across producers must serialize those sends. Sequential sends remain synchronous and ordered.

Checklist

  • this PR is based on the main branch and is up-to-date
  • the commits inside this PR have explicit commit messages
  • unit tests cover the new feature or the bug fix
  • the feature is documented in the README.md if it makes sense (no new feature or API; the lock invariant is documented in source)
  • the CHANGELOG is up-to-date

@codecov-commenter

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 96.74419% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 94.92%. Comparing base (1f0729e) to head (806c2e5).
⚠️ Report is 7 commits behind head on main.

Files with missing lines Patch % Lines
...s/AsyncSubjets/AsyncSubjectCancellationTests.swift 94.89% 7 Missing ⚠️
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #59      +/-   ##
==========================================
- Coverage   95.41%   94.92%   -0.49%     
==========================================
  Files          78       79       +1     
  Lines        7541     7767     +226     
==========================================
+ Hits         7195     7373     +178     
- Misses        346      394      +48     

☔ 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.

Snapshot subscriber channels under the subject state lock, then deliver values and termination after releasing it. Keep current-value and replay state updates atomic with the snapshot.

Cover all six subject variants with suspended-consumer cancellation races for value sends, finishes, and failures.
@twittemb
twittemb force-pushed the codex/fix-subject-cancellation-deadlock branch from 806c2e5 to a1b8658 Compare October 3, 2026 08:44
@twittemb
twittemb merged commit b23ae9c into main Oct 3, 2026
3 of 4 checks passed
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.

[BUG] Possible deadlock in Async subjects

2 participants