diff --git a/.codecov.yml b/.codecov.yml index 84c209a..43c47c2 100644 --- a/.codecov.yml +++ b/.codecov.yml @@ -4,9 +4,9 @@ codecov: require_ci_to_pass: yes notify: - # Wait for all 6 coverage uploads before reporting: - # 2 unit (latest + canary) + 3 contracts (redis, postgres, firestore) + 1 e2e - after_n_builds: 6 + # Wait for all 5 coverage uploads before reporting: + # 1 unit + 3 contracts (redis, postgres, firestore) + 1 e2e + after_n_builds: 5 coverage: precision: 2 diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a6de60b..5c466fd 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -8,6 +8,7 @@ on: permissions: contents: read packages: read + id-token: write jobs: unit: @@ -35,11 +36,12 @@ jobs: - run: bun run test:unit:coverage - name: Upload coverage to Codecov + if: matrix.bun-version == 'latest' uses: codecov/codecov-action@v5 with: - token: ${{ secrets.CODECOV_TOKEN }} files: ./coverage/lcov.info flags: unit + use_oidc: true fail_ci_if_error: false lint: diff --git a/.github/workflows/integration.yml b/.github/workflows/integration.yml index e6a43b8..b0260e1 100644 --- a/.github/workflows/integration.yml +++ b/.github/workflows/integration.yml @@ -9,6 +9,7 @@ on: permissions: contents: read packages: read + id-token: write jobs: contracts: @@ -90,9 +91,9 @@ jobs: - name: Upload coverage to Codecov uses: codecov/codecov-action@v5 with: - token: ${{ secrets.CODECOV_TOKEN }} files: ./coverage/lcov.info flags: contracts-${{ matrix.backend }} + use_oidc: true fail_ci_if_error: false - name: Kill Firestore emulator @@ -161,16 +162,18 @@ jobs: run: | for i in {1..30}; do nc -z 127.0.0.1 6379 && break || sleep 1; done for i in {1..60}; do nc -z 127.0.0.1 5432 && break || sleep 1; done + # Wait for Firestore emulator port, then additional warmup for full readiness for i in {1..60}; do nc -z 127.0.0.1 8080 && break || sleep 1; done + sleep 5 - run: bun run test:e2e:coverage - name: Upload coverage to Codecov uses: codecov/codecov-action@v5 with: - token: ${{ secrets.CODECOV_TOKEN }} files: ./coverage/lcov.info flags: e2e + use_oidc: true fail_ci_if_error: false - name: Kill Firestore emulator diff --git a/common/auto-lock.ts b/common/auto-lock.ts index 5c2abff..7fcdc5f 100644 --- a/common/auto-lock.ts +++ b/common/auto-lock.ts @@ -232,8 +232,6 @@ export async function lock( // Execute user function, auto-release in finally block try { return await fn(); - } catch (error) { - throw error; // Re-throw after release in finally } finally { // Best-effort release: don't throw, lock expires via TTL try { diff --git a/common/disposable.ts b/common/disposable.ts index 177270b..f783dbf 100644 --- a/common/disposable.ts +++ b/common/disposable.ts @@ -228,6 +228,9 @@ export function createDisposableHandle( type State = "active" | "disposing" | "disposed"; let state: State = "active"; let disposePromise: Promise | null = null; + // Track in-flight release() so dispose() can wait for it (fixes race condition + // where dispose() was called while release() awaited backend, returning null) + let pendingRelease: Promise | null = null; const handle: DisposableLockHandle = { async release(signal?: AbortSignal): Promise { @@ -241,20 +244,33 @@ export function createDisposableHandle( // even if the call throws (network error, timeout, etc.) state = "disposing"; + // Store promise BEFORE await so dispose() can wait for it if called concurrently. + // Wrap in try/catch to handle synchronous throws (e.g., validation failures). + let releaseOp: Promise; try { - // Throw on errors for consistency with backend API - // Only disposal swallows errors (see asyncDispose below) - const releaseResult = await backend.release({ + releaseOp = backend.release({ lockId: result.lockId, signal, }); - state = "disposed"; - return releaseResult; } catch (error) { - // Release failed - mark as disposed anyway (at-most-once semantics) + // Synchronous throw - mark disposed to maintain at-most-once semantics state = "disposed"; throw error; } + + pendingRelease = releaseOp.then( + () => { + state = "disposed"; + }, + () => { + state = "disposed"; + }, + ); + + // Throw on errors for consistency with backend API + // Only disposal swallows errors (see asyncDispose below) + // State is set to "disposed" by pendingRelease handler on both success/failure + return releaseOp; }, async extend(ttlMs: number, signal?: AbortSignal): Promise { @@ -269,8 +285,14 @@ export function createDisposableHandle( return; } - // Re-entry during disposal: return same promise (idempotent) + // Re-entry during disposal: wait for in-flight operation if (state === "disposing") { + // If release() initiated disposal, wait for it (pendingRelease is set) + // If dispose() initiated, wait for disposePromise + if (pendingRelease) { + await pendingRelease; + return; + } return disposePromise!; } @@ -493,7 +515,18 @@ export async function acquireHandle( return result; } - // Result is already decorated by backend, just return it - // Type assertion safe here since we checked ok: true + // Validate backend returned decorated result (catches misconfigured mocks) + const lock = result as AsyncLock; + if ( + typeof lock.release !== "function" || + typeof lock.extend !== "function" || + typeof lock[Symbol.asyncDispose] !== "function" + ) { + throw new Error( + "Backend.acquire() must return a decorated result. " + + "Use decorateAcquireResult() or implement LockBackend correctly.", + ); + } + return result as AsyncLock; } diff --git a/package.json b/package.json index 573b0dd..6a5ef49 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "syncguard", - "version": "2.5.1", + "version": "2.5.2", "description": "Functional TypeScript library for distributed locking across microservices. Prevents race conditions with Redis, PostgreSQL, Firestore, and custom backends. Features automatic lock management, timeout handling, and extensible architecture.", "keywords": [ "atomic", diff --git a/test/e2e/cross-backend.test.ts b/test/e2e/cross-backend.test.ts index ff928e1..78cec6c 100644 --- a/test/e2e/cross-backend.test.ts +++ b/test/e2e/cross-backend.test.ts @@ -25,7 +25,6 @@ import { expect, it, } from "bun:test"; -import { makeStorageKey } from "../../common/crypto.js"; import type { LockBackend } from "../../common/types.js"; import { getAvailableBackends } from "../fixtures/backends.js"; @@ -144,7 +143,7 @@ describe("E2E: Cross-Backend Consistency", async () => { expect(isLocked).toBe(false); }); - it("should handle lookup consistently within tolerance window", async () => { + it("should return lock info for active lock", async () => { const key = "lookup:consistency:test"; // Acquire lock @@ -233,111 +232,4 @@ describe("E2E: Cross-Backend Consistency", async () => { }); }); } - - describe("Storage Key Consistency", () => { - it("should produce identical base storage keys for same user key", () => { - const userKey = "resource:payment:12345"; - - // Redis-style computation - const redisReserve = 25; // "id:" + 22-char lockId - const redisBaseKey = makeStorageKey("test:", userKey, 1000, redisReserve); - - // Firestore-style computation - const firestoreReserve = 0; // No derived keys - const firestoreBaseKey = makeStorageKey( - "", - userKey, - 1500, - firestoreReserve, - ); - - // Both should preserve user key when no truncation needed - expect(redisBaseKey).toContain(userKey); - expect(firestoreBaseKey).toBe(userKey); - - // Verify both use makeStorageKey() for consistent hashing - const longKey = "x".repeat(600); - const redisLongKey = makeStorageKey("test:", longKey, 1000, redisReserve); - const firestoreLongKey = makeStorageKey( - "", - longKey, - 1500, - firestoreReserve, - ); - - // When truncation occurs, both should use same hash algorithm - expect(redisLongKey.length).toBeLessThanOrEqual(1000); - expect(firestoreLongKey.length).toBeLessThanOrEqual(1500); - }); - - it("should derive fence keys from same base storage key (ADR-006 two-step pattern)", () => { - const userKey = "resource:critical:operation"; - - // Redis: two-step derivation - const redisReserve = 25; - // Step 1: Compute base storage key - const redisBaseKey = makeStorageKey("test:", userKey, 1000, redisReserve); - // Step 2: Derive fence key from base - const redisFenceKey = makeStorageKey( - "test:", - `fence:${redisBaseKey}`, - 1000, - redisReserve, - ); - - // Firestore: two-step derivation - const firestoreReserve = 0; - // Step 1: Compute base storage key - const firestoreBaseKey = makeStorageKey( - "", - userKey, - 1500, - firestoreReserve, - ); - // Step 2: Derive fence document ID from base - const firestoreFenceDocId = makeStorageKey( - "", - `fence:${firestoreBaseKey}`, - 1500, - firestoreReserve, - ); - - // Verify both backends use two-step pattern - expect(redisFenceKey).toContain("fence:"); - expect(firestoreFenceDocId).toContain("fence:"); - - // Test with long keys - const longKey = "x".repeat(2000); - const redisBaseLong = makeStorageKey( - "test:", - longKey, - 1000, - redisReserve, - ); - const redisFenceLong = makeStorageKey( - "test:", - `fence:${redisBaseLong}`, - 1000, - redisReserve, - ); - const firestoreBaseLong = makeStorageKey( - "", - longKey, - 1500, - firestoreReserve, - ); - const firestoreFenceLong = makeStorageKey( - "", - `fence:${firestoreBaseLong}`, - 1500, - firestoreReserve, - ); - - // Both backends ensure 1:1 mapping - expect(redisBaseLong.length).toBeLessThanOrEqual(1000); - expect(redisFenceLong.length).toBeLessThanOrEqual(1000); - expect(firestoreBaseLong.length).toBeLessThanOrEqual(1500); - expect(firestoreFenceLong.length).toBeLessThanOrEqual(1500); - }); - }); }); diff --git a/test/e2e/disposal-patterns.test.ts b/test/e2e/disposal-patterns.test.ts index 65f82e1..27699d2 100644 --- a/test/e2e/disposal-patterns.test.ts +++ b/test/e2e/disposal-patterns.test.ts @@ -27,6 +27,9 @@ import { import type { LockBackend } from "../../common/types.js"; import { getAvailableBackends } from "../fixtures/backends.js"; +// 30s per test is generous for E2E; prevents CI hangs from emulator issues +const TEST_TIMEOUT_MS = 30_000; + describe("E2E: Disposal Patterns", async () => { const availableBackends = await getAvailableBackends(); @@ -56,207 +59,243 @@ describe("E2E: Disposal Patterns", async () => { await teardown(); }); - it("should automatically release lock on scope exit", async () => { - const key = "disposal:auto-release"; - - { - await using lock = await backend.acquire({ key, ttlMs: 30000 }); - - if (lock.ok) { - // Lock should be held - expect(await backend.isLocked({ key })).toBe(true); - } - } - - // Lock should be automatically released - expect(await backend.isLocked({ key })).toBe(false); - }); - - it("should release lock even if scope exits with error", async () => { - const key = "disposal:error-release"; + it( + "should automatically release lock on scope exit", + async () => { + const key = "disposal:auto-release"; - const testFn = async () => { - await using lock = await backend.acquire({ key, ttlMs: 30000 }); + { + await using lock = await backend.acquire({ key, ttlMs: 30000 }); - if (lock.ok) { - expect(await backend.isLocked({ key })).toBe(true); - throw new Error("Test error"); + if (lock.ok) { + // Lock should be held + expect(await backend.isLocked({ key })).toBe(true); + } } - }; - - await expect(testFn()).rejects.toThrow("Test error"); - // Lock should still be released - expect(await backend.isLocked({ key })).toBe(false); - }); + // Lock should be automatically released + expect(await backend.isLocked({ key })).toBe(false); + }, + TEST_TIMEOUT_MS, + ); - it("should support manual release with disposal handle", async () => { - const key = "disposal:manual-release"; + it( + "should release lock even if scope exits with error", + async () => { + const key = "disposal:error-release"; - await using lock = await backend.acquire({ key, ttlMs: 30000 }); + const testFn = async () => { + await using lock = await backend.acquire({ key, ttlMs: 30000 }); - if (lock.ok) { - expect(await backend.isLocked({ key })).toBe(true); + if (lock.ok) { + expect(await backend.isLocked({ key })).toBe(true); + throw new Error("Test error"); + } + }; - // Manual release - const result = await lock.release(); - expect(result.ok).toBe(true); + await expect(testFn()).rejects.toThrow("Test error"); - // Lock should be released + // Lock should still be released expect(await backend.isLocked({ key })).toBe(false); - } - }); + }, + TEST_TIMEOUT_MS, + ); - it("should support extend operation with disposal handle", async () => { - const key = "disposal:extend"; + it( + "should support manual release with disposal handle", + async () => { + const key = "disposal:manual-release"; - await using lock = await backend.acquire({ key, ttlMs: 500 }); - - if (lock.ok) { - const originalExpiry = lock.expiresAtMs; + await using lock = await backend.acquire({ key, ttlMs: 30000 }); - // Wait a bit - await Bun.sleep(200); + if (lock.ok) { + expect(await backend.isLocked({ key })).toBe(true); - // Extend lock - const extendResult = await lock.extend(5000); - expect(extendResult.ok).toBe(true); + // Manual release + const result = await lock.release(); + expect(result.ok).toBe(true); - if (extendResult.ok) { - expect(extendResult.expiresAtMs).toBeGreaterThan(originalExpiry); + // Lock should be released + expect(await backend.isLocked({ key })).toBe(false); } + }, + TEST_TIMEOUT_MS, + ); - // Wait past original TTL - await Bun.sleep(400); + it( + "should support extend operation with disposal handle", + async () => { + const key = "disposal:extend"; - // Lock should still be held - expect(await backend.isLocked({ key })).toBe(true); - } - }); + await using lock = await backend.acquire({ key, ttlMs: 500 }); - it("should handle failed acquisition gracefully", async () => { - const key = "disposal:contended"; - - // Hold lock - const firstLock = await backend.acquire({ key, ttlMs: 30000 }); - expect(firstLock.ok).toBe(true); - - { - // Try to acquire same lock (should fail) - await using lock = await backend.acquire({ key, ttlMs: 100 }); + if (lock.ok) { + const originalExpiry = lock.expiresAtMs; - expect(lock.ok).toBe(false); + // Wait a bit + await Bun.sleep(200); - // No disposal should happen for failed acquisition - } + // Extend lock + const extendResult = await lock.extend(5000); + expect(extendResult.ok).toBe(true); - // First lock should still be held - expect(await backend.isLocked({ key })).toBe(true); + if (extendResult.ok) { + expect(extendResult.expiresAtMs).toBeGreaterThan(originalExpiry); + } - // Clean up - if (firstLock.ok) { - await firstLock.release(); - } - }); + // Wait past original TTL + await Bun.sleep(400); - it("should handle double release gracefully", async () => { - const key = "disposal:double-release"; + // Lock should still be held + expect(await backend.isLocked({ key })).toBe(true); + } + }, + TEST_TIMEOUT_MS, + ); - await using lock = await backend.acquire({ key, ttlMs: 30000 }); + it( + "should handle failed acquisition gracefully", + async () => { + const key = "disposal:contended"; - if (lock.ok) { - // First manual release - const release1 = await lock.release(); - expect(release1.ok).toBe(true); + // Hold lock + const firstLock = await backend.acquire({ key, ttlMs: 30000 }); + expect(firstLock.ok).toBe(true); - // Second manual release - const release2 = await lock.release(); - expect(release2.ok).toBe(false); - } + { + // Try to acquire same lock (should fail) + await using lock = await backend.acquire({ key, ttlMs: 100 }); - // Disposal should handle already-released lock gracefully - }); + expect(lock.ok).toBe(false); - it("should allow nested disposal scopes", async () => { - const key1 = "disposal:nested:1"; - const key2 = "disposal:nested:2"; + // No disposal should happen for failed acquisition + } - { - await using lock1 = await backend.acquire({ - key: key1, - ttlMs: 30000, - }); + // First lock should still be held + expect(await backend.isLocked({ key })).toBe(true); - if (lock1.ok) { - expect(await backend.isLocked({ key: key1 })).toBe(true); + // Clean up + if (firstLock.ok) { + await firstLock.release(); + } + }, + TEST_TIMEOUT_MS, + ); - { - await using lock2 = await backend.acquire({ - key: key2, - ttlMs: 30000, - }); + it( + "should handle double release gracefully", + async () => { + const key = "disposal:double-release"; - if (lock2.ok) { - expect(await backend.isLocked({ key: key2 })).toBe(true); - } - } + await using lock = await backend.acquire({ key, ttlMs: 30000 }); - // Inner lock should be released - expect(await backend.isLocked({ key: key2 })).toBe(false); + if (lock.ok) { + // First manual release + const release1 = await lock.release(); + expect(release1.ok).toBe(true); - // Outer lock should still be held - expect(await backend.isLocked({ key: key1 })).toBe(true); + // Second manual release + const release2 = await lock.release(); + expect(release2.ok).toBe(false); } - } - // Both locks should be released - expect(await backend.isLocked({ key: key1 })).toBe(false); - expect(await backend.isLocked({ key: key2 })).toBe(false); - }); + // Disposal should handle already-released lock gracefully + }, + TEST_TIMEOUT_MS, + ); + + it( + "should allow nested disposal scopes", + async () => { + const key1 = "disposal:nested:1"; + const key2 = "disposal:nested:2"; + + { + await using lock1 = await backend.acquire({ + key: key1, + ttlMs: 30000, + }); + + if (lock1.ok) { + expect(await backend.isLocked({ key: key1 })).toBe(true); + + { + await using lock2 = await backend.acquire({ + key: key2, + ttlMs: 30000, + }); + + if (lock2.ok) { + expect(await backend.isLocked({ key: key2 })).toBe(true); + } + } - it("should provide consistent disposal behavior across manual operations", async () => { - const key = "disposal:manual-ops"; + // Inner lock should be released + expect(await backend.isLocked({ key: key2 })).toBe(false); - await using lock = await backend.acquire({ key, ttlMs: 30000 }); + // Outer lock should still be held + expect(await backend.isLocked({ key: key1 })).toBe(true); + } + } - if (lock.ok) { - // Manual release - const releaseResult = await lock.release(); - expect(releaseResult.ok).toBe(true); + // Both locks should be released + expect(await backend.isLocked({ key: key1 })).toBe(false); + expect(await backend.isLocked({ key: key2 })).toBe(false); + }, + TEST_TIMEOUT_MS, + ); - // Subsequent release delegates to backend (returns ok: false - lock absent) - const releaseResult2 = await lock.release(); - expect(releaseResult2.ok).toBe(false); + it( + "should provide consistent disposal behavior across manual operations", + async () => { + const key = "disposal:manual-ops"; - // Extend after release delegates to backend (returns ok: false - lock absent) - const extendResult = await lock.extend(5000); - expect(extendResult.ok).toBe(false); - } - }); + await using lock = await backend.acquire({ key, ttlMs: 30000 }); - it("should handle concurrent disposal correctly", async () => { - const keys = [ - "disposal:concurrent:1", - "disposal:concurrent:2", - "disposal:concurrent:3", - ]; + if (lock.ok) { + // Manual release + const releaseResult = await lock.release(); + expect(releaseResult.ok).toBe(true); - await Promise.all( - keys.map(async (key) => { - await using lock = await backend.acquire({ key, ttlMs: 30000 }); + // Subsequent release delegates to backend (returns ok: false - lock absent) + const releaseResult2 = await lock.release(); + expect(releaseResult2.ok).toBe(false); - if (lock.ok) { - expect(await backend.isLocked({ key })).toBe(true); - await Bun.sleep(50); - } - }), - ); + // Extend after release delegates to backend (returns ok: false - lock absent) + const extendResult = await lock.extend(5000); + expect(extendResult.ok).toBe(false); + } + }, + TEST_TIMEOUT_MS, + ); + + it( + "should handle concurrent disposal correctly", + async () => { + const keys = [ + "disposal:concurrent:1", + "disposal:concurrent:2", + "disposal:concurrent:3", + ]; + + await Promise.all( + keys.map(async (key) => { + await using lock = await backend.acquire({ key, ttlMs: 30000 }); + + if (lock.ok) { + expect(await backend.isLocked({ key })).toBe(true); + await Bun.sleep(50); + } + }), + ); - // All locks should be released - for (const key of keys) { - expect(await backend.isLocked({ key })).toBe(false); - } - }); + // All locks should be released + for (const key of keys) { + expect(await backend.isLocked({ key })).toBe(false); + } + }, + TEST_TIMEOUT_MS, + ); }); } }); diff --git a/test/unit/common/auto-lock.test.ts b/test/unit/common/auto-lock.test.ts index 47e38f3..2ee427e 100644 --- a/test/unit/common/auto-lock.test.ts +++ b/test/unit/common/auto-lock.test.ts @@ -282,6 +282,212 @@ describe("lock() retry behavior", () => { expect(calls).toBe(3); }); + + it("should apply full jitter strategy (delay = random * baseDelay)", async () => { + // Full jitter formula: delay = Math.random() * baseDelay + // Mock Math.random to return 0.25, so delay should be 0.25 * 100 = 25ms + // Mock setTimeout to capture exact delay values without wall-clock timing + const originalRandom = Math.random; + const originalDateNow = Date.now; + const originalSetTimeout = globalThis.setTimeout; + + Math.random = () => 0.25; + let mockTime = 1000; + const delaysPassed: number[] = []; + + Date.now = () => mockTime; + + // Mock setTimeout to capture delay values and resolve immediately + globalThis.setTimeout = ((callback: () => void, ms: number) => { + delaysPassed.push(ms); + mockTime += ms; // Advance mock time + callback(); // Resolve immediately + return 0 as unknown as ReturnType; + }) as typeof setTimeout; + + let calls = 0; + const mockBackendOps = { + release: mock(async () => ({ ok: true as const })), + extend: mock(async () => ({ + ok: true as const, + expiresAtMs: mockTime + 30000, + })), + }; + + (mockBackend.acquire as ReturnType).mockImplementation( + async () => { + calls++; + if (calls < 3) { + return { ok: false, reason: "locked" }; + } + const result: AcquireResult = { + ok: true, + lockId: "test-lock-id", + expiresAtMs: mockTime + 30000, + fence: "000000000000001", + }; + return decorateAcquireResult(mockBackendOps, result, "test-key"); + }, + ); + + try { + const baseDelay = 100; + await lock(mockBackend, async () => "success", { + key: "test-key", + ttlMs: 30000, + acquisition: { + maxRetries: 10, + retryDelayMs: baseDelay, + timeoutMs: 5000, + backoff: "fixed", + jitter: "full", + }, + }); + + expect(calls).toBe(3); + + // With random() = 0.25, full jitter: delay = 0.25 * 100 = 25ms exactly + // 2 retries = 2 delays + expect(delaysPassed.length).toBe(2); + expect(delaysPassed[0]).toBe(25); // Exact value, not 100 (no jitter) + expect(delaysPassed[1]).toBe(25); + } finally { + Math.random = originalRandom; + Date.now = originalDateNow; + globalThis.setTimeout = originalSetTimeout; + } + }); + + it("should apply equal jitter strategy (delay = base/2 + random * base/2)", async () => { + // Equal jitter formula: delay = baseDelay/2 + Math.random() * baseDelay/2 + // Mock Math.random to return 0.5, so delay = 50 + 0.5 * 50 = 75ms + const originalRandom = Math.random; + const originalDateNow = Date.now; + const originalSetTimeout = globalThis.setTimeout; + + Math.random = () => 0.5; + let mockTime = 1000; + const delaysPassed: number[] = []; + + Date.now = () => mockTime; + + globalThis.setTimeout = ((callback: () => void, ms: number) => { + delaysPassed.push(ms); + mockTime += ms; + callback(); + return 0 as unknown as ReturnType; + }) as typeof setTimeout; + + let calls = 0; + const mockBackendOps = { + release: mock(async () => ({ ok: true as const })), + extend: mock(async () => ({ + ok: true as const, + expiresAtMs: mockTime + 30000, + })), + }; + + (mockBackend.acquire as ReturnType).mockImplementation( + async () => { + calls++; + if (calls < 3) { + return { ok: false, reason: "locked" }; + } + const result: AcquireResult = { + ok: true, + lockId: "test-lock-id", + expiresAtMs: mockTime + 30000, + fence: "000000000000001", + }; + return decorateAcquireResult(mockBackendOps, result, "test-key"); + }, + ); + + try { + const baseDelay = 100; + await lock(mockBackend, async () => "success", { + key: "test-key", + ttlMs: 30000, + acquisition: { + maxRetries: 10, + retryDelayMs: baseDelay, + timeoutMs: 5000, + backoff: "fixed", + jitter: "equal", + }, + }); + + expect(calls).toBe(3); + + // With random() = 0.5, equal jitter: delay = 50 + 0.5 * 50 = 75ms exactly + expect(delaysPassed.length).toBe(2); + expect(delaysPassed[0]).toBe(75); // Exact value, not 100 or 50 + expect(delaysPassed[1]).toBe(75); + } finally { + Math.random = originalRandom; + Date.now = originalDateNow; + globalThis.setTimeout = originalSetTimeout; + } + }); + }); + + describe("timeout edge cases", () => { + it("should throw when retry delay is clamped to zero by remaining time", async () => { + // This tests the retryDelay <= 0 branch (lines 206-210) which guards against + // a race where remaining time becomes <= 0 between the pre-acquire timeout + // check (line 150) and the retry delay calculation (line 195-203). + // + // We use Date.now() mocking to deterministically trigger this edge case: + // - First call: time = 0 (start), passes timeout check + // - acquire() contends + // - After acquire returns: time = 101 (exceeds 100ms timeout) + // - retryDelay calculation sees remainingTime = 100 - 101 = -1 + // - retryDelay is clamped to max(0, -1) = 0 + // - Branch throws "Timeout reached before next retry" + const originalDateNow = Date.now; + let mockTime = 1000; + + Date.now = () => mockTime; + + let calls = 0; + (mockBackend.acquire as ReturnType).mockImplementation( + async () => { + calls++; + if (calls === 1) { + // Simulate time passing during acquire - exceeds timeout + mockTime += 101; + return { ok: false, reason: "locked" }; + } + return { ok: false, reason: "locked" }; + }, + ); + + try { + await lock(mockBackend, async () => "success", { + key: "test-key", + ttlMs: 30000, + acquisition: { + maxRetries: 100, + retryDelayMs: 10, + timeoutMs: 100, // 100ms timeout, but mock jumps to 101ms + backoff: "fixed", + jitter: "none", + }, + }); + expect.unreachable("Should have thrown"); + } catch (error) { + expect(error).toBeInstanceOf(LockError); + expect((error as LockError).code).toBe("AcquisitionTimeout"); + // Verify we hit the specific retryDelay <= 0 branch + expect((error as LockError).message).toMatch( + /Timeout reached before next retry/, + ); + } finally { + Date.now = originalDateNow; + } + + expect(calls).toBe(1); + }); }); describe("AbortSignal support", () => { @@ -417,4 +623,142 @@ describe("lock() retry behavior", () => { }); }); }); + + describe("release failure handling", () => { + it("should return function result when release fails", async () => { + (mockBackend.release as ReturnType).mockRejectedValueOnce( + new Error("Release failed"), + ); + + const result = await lock(mockBackend, async () => "success", { + key: "test-key", + ttlMs: 30000, + }); + + expect(result).toBe("success"); + }); + + it("should call default error handler when release fails (non-production)", async () => { + const originalEnv = process.env.NODE_ENV; + process.env.NODE_ENV = "development"; + const consoleSpy = mock((..._args: unknown[]) => {}); + const originalError = console.error; + console.error = consoleSpy; + + try { + (mockBackend.release as ReturnType).mockRejectedValueOnce( + new Error("Network error"), + ); + + await lock(mockBackend, async () => "success", { + key: "test-key", + ttlMs: 30000, + }); + + expect(consoleSpy).toHaveBeenCalled(); + const callArgs = consoleSpy.mock.calls[0]!; + expect(callArgs[0]).toBe("[SyncGuard] Lock disposal failed:"); + expect(callArgs[1]).toMatchObject({ + error: "Network error", + errorName: "Error", + source: "disposal", + }); + } finally { + process.env.NODE_ENV = originalEnv; + console.error = originalError; + } + }); + + it("should call custom onReleaseError when release fails", async () => { + const onReleaseError = mock( + ( + _err: Error, + _ctx: { lockId: string; key: string; source: string }, + ) => {}, + ); + (mockBackend.release as ReturnType).mockRejectedValueOnce( + new Error("Release failed"), + ); + + await lock(mockBackend, async () => "success", { + key: "test-key", + ttlMs: 30000, + onReleaseError, + }); + + expect(onReleaseError).toHaveBeenCalledTimes(1); + const [err, ctx] = onReleaseError.mock.calls[0]!; + expect(err).toBeInstanceOf(Error); + expect(err.message).toBe("Release failed"); + expect(ctx).toMatchObject({ + lockId: "test-lock-id", + key: "test-key", + source: "disposal", + }); + }); + + it("should normalize non-Error release failures", async () => { + const onReleaseError = mock((_err: Error, _ctx: unknown) => {}); + (mockBackend.release as ReturnType).mockRejectedValueOnce( + "string error", + ); + + await lock(mockBackend, async () => "success", { + key: "test-key", + ttlMs: 30000, + onReleaseError, + }); + + expect(onReleaseError).toHaveBeenCalledTimes(1); + const [err] = onReleaseError.mock.calls[0]!; + expect(err).toBeInstanceOf(Error); + expect(err.message).toBe("string error"); + expect((err as Error & { originalError?: unknown }).originalError).toBe( + "string error", + ); + }); + + it("should swallow errors thrown by onReleaseError callback", async () => { + const onReleaseError = mock(() => { + throw new Error("Callback error"); + }); + (mockBackend.release as ReturnType).mockRejectedValueOnce( + new Error("Release failed"), + ); + + // Should not throw despite callback error + const result = await lock(mockBackend, async () => "success", { + key: "test-key", + ttlMs: 30000, + onReleaseError, + }); + + expect(result).toBe("success"); + expect(onReleaseError).toHaveBeenCalled(); + }); + + it("should propagate function error even when release fails", async () => { + const onReleaseError = mock(() => {}); + (mockBackend.release as ReturnType).mockRejectedValueOnce( + new Error("Release failed"), + ); + + await expect( + lock( + mockBackend, + async () => { + throw new Error("Function error"); + }, + { + key: "test-key", + ttlMs: 30000, + onReleaseError, + }, + ), + ).rejects.toThrow("Function error"); + + // onReleaseError should still be called + expect(onReleaseError).toHaveBeenCalled(); + }); + }); }); diff --git a/test/unit/common/crypto.test.ts b/test/unit/common/crypto.test.ts index 1b803d2..92b26c2 100644 --- a/test/unit/common/crypto.test.ts +++ b/test/unit/common/crypto.test.ts @@ -2,9 +2,9 @@ // SPDX-License-Identifier: MIT /** - * Unit tests for cryptographic functions + * Unit tests for crypto utilities * - * Tests hash key generation and fence token formatting + * Tests makeStorageKey, generateLockId, hashKey, and formatFence */ import { describe, expect, it } from "bun:test"; @@ -12,6 +12,7 @@ import { formatFence, generateLockId, hashKey, + makeStorageKey, } from "../../../common/crypto.js"; import { LockError } from "../../../common/errors.js"; @@ -45,6 +46,114 @@ describe("generateLockId", () => { }); }); +describe("makeStorageKey", () => { + it("should preserve user key when no truncation needed", () => { + const userKey = "resource:payment:12345"; + + // Redis-style (prefix with reserve for derived keys) + const redisReserve = 25; // "id:" + 22-char lockId + const redisKey = makeStorageKey("test", userKey, 1000, redisReserve); + + // Firestore-style (no prefix, no reserve) + const firestoreKey = makeStorageKey("", userKey, 1500, 0); + + expect(redisKey).toBe("test:resource:payment:12345"); + expect(firestoreKey).toBe(userKey); + }); + + it("should hash keys that exceed backend limits", () => { + const longKey = "x".repeat(2000); + const redisReserve = 25; + + const redisKey = makeStorageKey("test", longKey, 1000, redisReserve); + const firestoreKey = makeStorageKey("", longKey, 1500, 0); + + // When truncation occurs, both should use hashed form (base64url) + expect(redisKey.length).toBeLessThanOrEqual(1000 - redisReserve); + expect(firestoreKey.length).toBeLessThanOrEqual(1500); + // Hashed keys use base64url chars + expect(redisKey).toMatch(/^test:[A-Za-z0-9_-]+$/); + expect(firestoreKey).toMatch(/^[A-Za-z0-9_-]+$/); + }); + + it("should derive fence keys from base storage key (two-step pattern)", () => { + const userKey = "resource:critical:operation"; + + // Redis: two-step derivation + const redisReserve = 25; + const redisBaseKey = makeStorageKey("test", userKey, 1000, redisReserve); + const redisFenceKey = makeStorageKey( + "test", + `fence:${redisBaseKey}`, + 1000, + redisReserve, + ); + + // Firestore: two-step derivation + const firestoreBaseKey = makeStorageKey("", userKey, 1500, 0); + const firestoreFenceDocId = makeStorageKey( + "", + `fence:${firestoreBaseKey}`, + 1500, + 0, + ); + + // Verify both backends use two-step pattern + expect(redisFenceKey).toContain("fence:"); + expect(firestoreFenceDocId).toContain("fence:"); + }); + + it("should ensure 1:1 mapping for long keys with hash truncation", () => { + const longKey = "x".repeat(2000); + const redisReserve = 25; + + const redisBaseLong = makeStorageKey("test", longKey, 1000, redisReserve); + const redisFenceLong = makeStorageKey( + "test", + `fence:${redisBaseLong}`, + 1000, + redisReserve, + ); + const firestoreBaseLong = makeStorageKey("", longKey, 1500, 0); + const firestoreFenceLong = makeStorageKey( + "", + `fence:${firestoreBaseLong}`, + 1500, + 0, + ); + + // Both backends ensure keys stay within limits + expect(redisBaseLong.length).toBeLessThanOrEqual(1000 - redisReserve); + expect(redisFenceLong.length).toBeLessThanOrEqual(1000 - redisReserve); + expect(firestoreBaseLong.length).toBeLessThanOrEqual(1500); + expect(firestoreFenceLong.length).toBeLessThanOrEqual(1500); + }); + + it("should strip trailing colons from prefix", () => { + const key = "resource"; + + // Both should produce the same result + const withColon = makeStorageKey("prefix:", key, 1000, 0); + const withoutColon = makeStorageKey("prefix", key, 1000, 0); + + expect(withColon).toBe("prefix:resource"); + expect(withoutColon).toBe("prefix:resource"); + }); + + it("should throw on empty key", () => { + expect(() => makeStorageKey("test", "", 1000, 0)).toThrow( + "Key must not be empty", + ); + }); + + it("should throw when prefix exceeds backend limit", () => { + const longPrefix = "x".repeat(500); + expect(() => makeStorageKey(longPrefix, "key", 100, 0)).toThrow( + "Prefix exceeds backend limit", + ); + }); +}); + describe("hashKey", () => { it("should produce 24-character hex string", () => { const hash = hashKey("test-key"); diff --git a/test/unit/disposable/concurrent.test.ts b/test/unit/disposable/concurrent.test.ts index e52a071..98b8da7 100644 --- a/test/unit/disposable/concurrent.test.ts +++ b/test/unit/disposable/concurrent.test.ts @@ -239,4 +239,132 @@ describe("Concurrent Disposal and Re-entry", () => { // Backend should be called exactly once expect(mockBackend.release).toHaveBeenCalledTimes(1); }); + + it("should wait for in-flight release() when dispose() is called", async () => { + // This tests the race condition where release() starts first, + // then dispose() is called while release() is still awaiting backend + let releaseStarted = false; + let releaseCompleted = false; + + (mockBackend.release as ReturnType).mockImplementation( + () => + new Promise((resolve) => { + releaseStarted = true; + setTimeout(() => { + releaseCompleted = true; + resolve({ ok: true }); + }, 100); + }), + ); + + const handle = createDisposableHandle( + mockBackend, + mockAcquireResult, + "test-key", + ); + + // Start manual release (don't await yet) + const releasePromise = handle.release(); + + // Wait for release to start but not complete + await Bun.sleep(10); + expect(releaseStarted).toBe(true); + expect(releaseCompleted).toBe(false); + + // Call dispose while release is in-flight + const disposePromise = handle[Symbol.asyncDispose](); + + // dispose() should NOT return null - it should wait for release + expect(disposePromise).toBeInstanceOf(Promise); + + // Wait for dispose to complete + await disposePromise; + + // Release should have completed by now + expect(releaseCompleted).toBe(true); + + // Verify release also completes successfully + const releaseResult = await releasePromise; + expect(releaseResult.ok).toBe(true); + + // Backend should be called exactly once + expect(mockBackend.release).toHaveBeenCalledTimes(1); + }); + + it("should handle dispose() during failing release()", async () => { + // Test that dispose() waits for release() even when it fails + const releaseError = new Error("Network failure"); + const onReleaseErrorSpy = mock(); + let releaseStarted = false; + + (mockBackend.release as ReturnType).mockImplementation( + () => + new Promise((_, reject) => { + releaseStarted = true; + setTimeout(() => reject(releaseError), 50); + }), + ); + + const handle = createDisposableHandle( + mockBackend, + mockAcquireResult, + "test-key", + onReleaseErrorSpy, + ); + + // Start manual release (don't await yet) + const releasePromise = handle.release(); + + // Wait for release to start + await Bun.sleep(10); + expect(releaseStarted).toBe(true); + + // Call dispose while release is in-flight + const disposePromise = handle[Symbol.asyncDispose](); + + // dispose() should wait and complete without throwing + await disposePromise; + + // release() should throw + await expect(releasePromise).rejects.toThrow("Network failure"); + + // Backend called once, no error callback (release() throws, doesn't swallow) + expect(mockBackend.release).toHaveBeenCalledTimes(1); + expect(onReleaseErrorSpy).not.toHaveBeenCalled(); + }); + + it("should handle synchronous throw from backend.release()", async () => { + // Test that synchronous throws (e.g., validation failures) don't leave + // state stuck at "disposing" with null pendingRelease + const syncError = new Error("Synchronous validation failure"); + + (mockBackend.release as ReturnType).mockImplementation(() => { + throw syncError; // Throws synchronously, not returning a promise + }); + + const handle = createDisposableHandle( + mockBackend, + mockAcquireResult, + "test-key", + ); + + // release() should throw synchronously + await expect(handle.release()).rejects.toThrow( + "Synchronous validation failure", + ); + + // State should be "disposed", not stuck at "disposing" + // Verify by calling dispose() - it should be a no-op (state === "disposed") + await handle[Symbol.asyncDispose](); + + // Backend should have been called once (the failing call) + expect(mockBackend.release).toHaveBeenCalledTimes(1); + + // Subsequent release() should return { ok: false } (idempotent) + const secondRelease = await handle.release(); + expect(secondRelease.ok).toBe(false); + + // No additional backend calls + expect(mockBackend.release).toHaveBeenCalledTimes(1); + }); }); diff --git a/test/unit/disposable/create-handle.test.ts b/test/unit/disposable/create-handle.test.ts index d159420..e8ee56d 100644 --- a/test/unit/disposable/create-handle.test.ts +++ b/test/unit/disposable/create-handle.test.ts @@ -244,3 +244,75 @@ describe("createDisposableHandle", () => { }); }); }); + +describe("acquireHandle", () => { + // Import here to avoid polluting the main describe scope + const { acquireHandle } = require("../../../common/disposable.js"); + + it("should throw if backend returns undecorated result", async () => { + // Mock a backend that returns raw AcquireResult without decoration + const rawBackend = { + acquire: async () => ({ + ok: true, + lockId: "raw-lock-123", + expiresAtMs: Date.now() + 30000, + // Missing: release(), extend(), [Symbol.asyncDispose]() + }), + }; + + await expect( + acquireHandle(rawBackend, { key: "test", ttlMs: 30000 }), + ).rejects.toThrow(/Backend\.acquire\(\) must return a decorated result/); + }); + + it("should pass through failed acquisition without validation", async () => { + const rawBackend = { + acquire: async () => ({ + ok: false, + reason: "locked" as const, + }), + }; + + const result = await acquireHandle(rawBackend, { + key: "test", + ttlMs: 30000, + }); + expect(result.ok).toBe(false); + }); + + it("should return decorated result for successful acquisition", async () => { + const { decorateAcquireResult } = require("../../../common/disposable.js"); + + const mockOps = { + release: async () => ({ ok: true }), + extend: async () => ({ ok: true, expiresAtMs: Date.now() + 30000 }), + }; + + const decoratedBackend = { + acquire: async () => { + const result = { + ok: true, + lockId: "decorated-lock-456", + expiresAtMs: Date.now() + 30000, + }; + return decorateAcquireResult(mockOps, result, "test-key"); + }, + }; + + const handle = await acquireHandle(decoratedBackend, { + key: "test", + ttlMs: 30000, + }); + + expect(handle.ok).toBe(true); + if (handle.ok) { + expect(handle.lockId).toBe("decorated-lock-456"); + expect(typeof handle.release).toBe("function"); + expect(typeof handle.extend).toBe("function"); + expect(typeof handle[Symbol.asyncDispose]).toBe("function"); + + // Clean up + await handle[Symbol.asyncDispose](); + } + }); +}); diff --git a/test/unit/disposable/timeout.test.ts b/test/unit/disposable/timeout.test.ts index 970221b..2370664 100644 --- a/test/unit/disposable/timeout.test.ts +++ b/test/unit/disposable/timeout.test.ts @@ -229,6 +229,81 @@ describe("Disposal Timeout", () => { }); }); + it("should invoke error callback when release fails before timeout", async () => { + const onReleaseErrorSpy = mock(); + const releaseError = new Error("Connection refused"); + + // Release fails immediately (before timeout would trigger) + (mockBackend.release as ReturnType).mockRejectedValue( + releaseError, + ); + + const handle = createDisposableHandle( + mockBackend, + mockAcquireResult, + "test-key", + onReleaseErrorSpy, + 5000, // Long timeout - release fails before it triggers + ); + + await handle[Symbol.asyncDispose](); + + // Error callback should be invoked with the release error + expect(onReleaseErrorSpy).toHaveBeenCalledTimes(1); + expect(onReleaseErrorSpy).toHaveBeenCalledWith(releaseError, { + lockId: "test-lock-timeout", + key: "test-key", + source: "disposal", + }); + }); + + it("should invoke error callback for deferred release failure after timeout", async () => { + const onReleaseErrorSpy = mock(); + const releaseError = new Error("Deferred connection error"); + + // Release hangs, then fails after timeout fires + let rejectRelease: (err: Error) => void; + (mockBackend.release as ReturnType).mockImplementation( + () => + new Promise((_, reject) => { + rejectRelease = reject; + }), + ); + + const handle = createDisposableHandle( + mockBackend, + mockAcquireResult, + "test-key", + onReleaseErrorSpy, + 50, // Short timeout + ); + + const disposalPromise = handle[Symbol.asyncDispose](); + + // Wait for timeout to fire + await Bun.sleep(100); + + // Disposal should complete (timeout fired) + await disposalPromise; + + // No error yet - release is still pending + expect(onReleaseErrorSpy).not.toHaveBeenCalled(); + + // Now release fails (fire-and-forget path) + rejectRelease!(releaseError); + + // Give the fire-and-forget handler time to run + await Bun.sleep(10); + + // Error callback should be invoked for the deferred failure + expect(onReleaseErrorSpy).toHaveBeenCalledTimes(1); + expect(onReleaseErrorSpy).toHaveBeenCalledWith(releaseError, { + lockId: "test-lock-timeout", + key: "test-key", + source: "disposal", + }); + }); + it("should work with decorateAcquireResult", async () => { const onReleaseErrorSpy = mock();