Skip to content

fix: dispose() race condition when release() is in-flight#26

Merged
koistya merged 9 commits intomainfrom
dev
Dec 2, 2025
Merged

fix: dispose() race condition when release() is in-flight#26
koistya merged 9 commits intomainfrom
dev

Conversation

@koistya
Copy link
Member

@koistya koistya commented Dec 2, 2025

Summary

  • Fix race condition where dispose() returned null when called while release() was awaiting backend
  • Handle synchronous throws from backend.release() to prevent state getting stuck at "disposing"
  • Add runtime validation to acquireHandle() for misconfigured backends/mocks

Changes

common/disposable.ts

  • Track pendingRelease promise so dispose() can wait for in-flight release() operations
  • Wrap backend.release() call to handle synchronous throws
  • Validate decorated result methods in acquireHandle()

Tests Added

Test Coverage
dispose() during in-flight release() Waits for release to complete
dispose() during failing release() Waits even when release throws
Synchronous backend throw State transitions to "disposed" correctly
Undecorated backend result Throws descriptive error

- Remove unnecessary catch-rethrow in auto-lock try/finally
- Add 6 tests for release failure handling:
  - Result preservation when release fails
  - Default error handler logging
  - Custom onReleaseError callback invocation
  - Non-Error normalization with originalError
  - Callback error swallowing
  - Function error propagation with release failure
@codecov
Copy link

codecov bot commented Dec 2, 2025

Codecov Report

❌ Patch coverage is 60.00000% with 16 lines in your changes missing coverage. Please review.
✅ Project coverage is 76.37%. Comparing base (d29ec05) to head (2881f90).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
common/disposable.ts 60.00% 16 Missing ⚠️

❌ Your patch status has failed because the patch coverage (60.00%) is below the target coverage (77.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #26      +/-   ##
==========================================
+ Coverage   75.88%   76.37%   +0.49%     
==========================================
  Files          42       42              
  Lines        2658     2688      +30     
==========================================
+ Hits         2017     2053      +36     
+ Misses        641      635       -6     
Flag Coverage Δ
contracts-firestore 47.05% <2.50%> (-0.91%) ⬇️
contracts-postgres 43.02% <2.50%> (-0.79%) ⬇️
contracts-redis 40.21% <2.50%> (-0.75%) ⬇️
e2e 59.40% <30.76%> (-0.31%) ⬇️
unit 64.37% <100.00%> (+1.37%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
common/auto-lock.ts 78.64% <ø> (+2.87%) ⬆️
common/disposable.ts 68.06% <60.00%> (+4.96%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

- Rename misleading "should handle lookup consistently within tolerance
  window" to "should return lock info for active lock"
- Move Storage Key Consistency tests from E2E to new crypto.test.ts
- Add comprehensive unit tests for generateLockId, hashKey, formatFence
- Track pending release() operations so dispose() can wait for them
- Handle synchronous throws from backend.release()
- Add runtime validation to acquireHandle() for misconfigured backends
@koistya koistya changed the title fix: remove redundant catch block and add release error tests fix: dispose() race condition when release() is in-flight Dec 2, 2025
- Add tests for disposal timeout edge cases (release fails before/after timeout)
- Add test for acquireHandle success path
- Add deterministic jitter tests using mocked Math.random and setTimeout
- Add test for retryDelay <= 0 timeout edge case

Coverage: disposable.ts 100%, auto-lock.ts 99.34%
Firestore emulator can be slow to fully initialize in CI, causing
tests to hang for 5+ minutes. Adding explicit per-test timeouts
ensures fast failure with actionable error messages.
- Use OIDC token instead of secret for codecov uploads
- Add 5s warmup delay after Firestore emulator port check
@koistya koistya marked this pull request as ready for review December 2, 2025 22:28
@koistya koistya merged commit ef85a16 into main Dec 2, 2025
12 of 13 checks passed
@koistya koistya deleted the dev branch December 2, 2025 22: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