feat(infra): Lock *Async API — pure CF chain, no task.get()#1304
Conversation
- 28 real blocking sites across module-infra + module-external-api - original plan (docs/superpowers + ADR) was based on wrong code assumptions - 5 recommended sub-PRs for future work - no code changes
…r (no commonPool)
…n-test not in deps)
…tinel, race assumption
…tedLockException check
…isoryLockStrategyAsyncTest
Migrated MonitoringAlertService and PopularCharacterWarmupScheduler to Lock *Async methods in feature/lock-async-api. The unit test mocks in module-app still stubbed the legacy sync API (executeWithLock, tryLockImmediately), so the production code's calls to *Async methods returned null on the mocks, causing NPE in whenComplete. Update both test classes to mock the new async signatures and return CompletableFuture values. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ab1c151a06
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| waitTime: Long, | ||
| leaseTime: Long, | ||
| supplier: () -> CompletableFuture<T>, | ||
| ): CompletableFuture<T> = tryAcquireSessionLockWithPollAsync(key, waitTime, leaseTime) |
There was a problem hiding this comment.
This async path switches callers from the old transaction-scoped lock to pg_try_advisory_lock, but the acquisition is executed through JdbcTemplate as a standalone query; Spring returns that connection to Hikari immediately after the query, while PostgreSQL session advisory locks remain bound to that physical connection. The later pg_advisory_unlock can run on a different pooled connection, so it may not release the lock, and a later caller that happens to reuse the original connection can re-enter the same lock, breaking mutual exclusion for the schedulers/bulk loader now using executeWithLockAsync. Hold a single connection for acquire/work/release, or keep using transaction-scoped advisory locks for this API.
Useful? React with 👍 / 👎.
| supplier().whenComplete { _, _ -> | ||
| releaseSessionLock(key, lockId) |
There was a problem hiding this comment.
If the protected supplier throws before it returns a CompletableFuture, the whenComplete callback is never attached and releaseSessionLock is skipped. Several migrated callers wrap synchronous work as CompletableFuture.completedFuture(doWork()), so exceptions from report generation, recovery, CSV reads, or warmup happen exactly in this window and leave the distributed lock held/leaked instead of releasing it like the old transaction-scoped path did. Wrap supplier() so release is registered in a finally-equivalent path even for synchronous throws.
Useful? React with 👍 / 👎.
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
1 participant
Summary
Adds async-returning methods to the Lock port. Eliminates all 5
task.get()blocking sites inPostgresAdvisoryLockStrategy+OrderedLockExecutor(and the hiddentask.get()inAbstractLockStrategyreached byPostgresLockStrategy). Migrates all 5 module-infra direct Lock callers to the new*AsyncAPI.Spec / Audit
docs/05_Reports/2026-06-18-blocking-audit.mddocs/superpowers/plans/2026-06-18-lock-async.mdDecisions (grill-me Q1-Q6)
LockAcquisitionExceptionThread.sleepinsideCompletableFuture.supplyAsync)pg_try_advisory_lock(session-scoped) + explicitpg_advisory_unlockinwhenCompletemodule-applegacy = follow-up PRChanges
module-infra/lock/LockStrategy.kt: added 5*Asyncmethods (executeWithLockAsync× 2,tryLockImmediatelyAsync,unlockAsync,executeWithOrderedLocksAsync). Sync methods marked@Deprecated. DefaultexecuteWithOrderedLockspreserved (composite-key fallback for legacy callers).LeaderElectionStrategy.kt: addedexecuteWithLeaderElectionAsync. Sync marked@Deprecated.AbstractLockStrategy.kt: implements*Asynctemplate-style via 2 abstract helper methods.PostgresAdvisoryLockStrategy.kt: implements*Async(session-scoped PG lock + explicit unlock). Uses@Qualifier("defaultAsyncExecutor")for executor injection.PostgresLockStrategy.kt: implements*Async(mirror pattern). Also fixed pre-existingunlockInternalregistry leak.GuavaLockStrategy.kt: implements*Async(Striped.tryLock + lock.unlock). Test-only CachedThreadPool (no Spring DI).OrderedLockExecutor.kt: addedexecuteWithOrderedLocksAsync+ 5 internal async methods. Migrated 2task.get()sites. Sync methods marked@Deprecated.module-infra/aop/LockAspect.kt: AOP wrapper now usesexecuteWithLockAsync..get()at AOP boundary (documented exception perasync-patterns.md). FixedExecutionException.causeunwrap forDistributedLockExceptiondetection.module-infra/(callers — 5 of 6 plan files migrated)BatchJobRecoveryScheduler.kt,MonitoringReportJob.kt,MonitoringAlertService.kt,PopularCharacterWarmupScheduler.kt,BulkLoaderService.kt: migrated to*AsyncAPI.RuleBasedAnalyzer.kt: excluded (no actual Lock API usage, only a string literal).module-app/(test mock follow-ups)MonitoringAlertServiceUnitTest.java: mocks now stubtryLockImmediatelyAsync(...)returningCompletableFuture<Boolean>.PopularCharacterWarmupSchedulerTest.java: mocks now stubexecuteWithLockAsync(...)returningCompletableFuture<T>viaFunction0supplier.CI gate
New test
LockBlockingPrimitiveGateTestgrepsmodule-infra/lock/fortask.get(),runBlocking,Thread.sleep. Allowlist for legacy@Deprecatedsync methods (multi-line span detection) and VT-friendlyThread.sleepinsideCompletableFuture.supplyAsyncblocks. Failure = build red.Verification
./gradlew compileKotlin compileJava --continueclean./gradlew testcleanOut of scope (follow-up PR)
module-applegacy migration (1 file:ExpectationBatchWriteScheduler.java)MonitoringAlertServicelock leak (4-sec lease, pre-existing behavior preserved)