Skip to content

fix: preserve semaphore state on overflow - #144

Merged
tisonkun merged 2 commits into
mainfrom
codex/semaphore-overflow-preserve-state
Aug 9, 2026
Merged

fix: preserve semaphore state on overflow#144
tisonkun merged 2 commits into
mainfrom
codex/semaphore-overflow-preserve-state

Conversation

@tisonkun

@tisonkun tisonkun commented Aug 9, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • check semaphore permit overflow before changing the counter
  • keep the existing fetch_add release path instead of adding a CAS retry loop
  • add Divan benchmarks for release and the common acquire/release round trip
  • cover both the public panic contract and post-panic semaphore usability

Problem

The previous implementation used fetch_add and checked its returned value afterward. Atomic integer addition wraps, so an overflowing Semaphore::release had already written the wrapped permit count before the assertion panicked. For example, releasing one permit at usize::MAX left the semaphore with zero available permits.

Implementation

Every path that adds available permits already holds the waiters lock. While that lock is held, concurrent lock-free operations can only remove permits. The implementation therefore loads and checks the current count under the lock, then uses the existing fetch_add; the count cannot grow between those two operations, so a successful check guarantees the addition cannot overflow.

The checked load is Relaxed because it is used only for the numeric overflow decision. The fetch_add remains Release, preserving the synchronization behavior of the original implementation.

This avoids both failure modes considered during the change: checking after fetch_add temporarily corrupts observable state, while a CAS loop adds unnecessary work and complexity to every successful release.

Tests

  • release_overflow_panics is an independent #[should_panic] test for the documented API contract
  • release_overflow_preserves_permits catches the panic, verifies the count remains usize::MAX, and confirms the semaphore is still usable
  • cargo x lint
  • cargo x test
  • cargo x bench
  • cargo +1.85.0 test --workspace --no-default-features
  • cargo +1.85.0 bench -p mea --bench primitives --no-run

Benchmarks

The same benchmark source was applied to main and this branch. Fixed-size Divan runs used 500 samples of 100,000 iterations to avoid timer-resolution artifacts.

  • Linux/aarch64, two paired runs: release was +0.8% and +2.2%; try_acquire_release was -1.2% and +1.4%
  • macOS/aarch64: release was +2.7%; try_acquire_release was +1.3%

The Linux results are within run-to-run variation, while macOS shows the small cost of the additional read and overflow check rather than the larger cost of a CAS retry loop.

@tisonkun
tisonkun merged commit 263582e into main Aug 9, 2026
10 checks passed
@tisonkun
tisonkun deleted the codex/semaphore-overflow-preserve-state branch August 9, 2026 19:28
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.

1 participant