fix: pin connection for advisory lock lifecycle#1168
Conversation
Allow overwriting existing files during stub generation by passing --force to any make:* command. The make:model command forwards the flag to sub-commands (migration, controller, transformer, factory).
…ELEASE_LOCK Advisory locks (PG_TRY_ADVISORY_LOCK/PG_ADVISORY_UNLOCK in PostgreSQL, GET_LOCK/RELEASE_LOCK in MySQL) are session-scoped — they are bound to the specific database connection that acquired them. Previously, both acquire and release used rawQuery which picks a random connection from the pool each time, causing the release to fail when it lands on a different connection. When the pool has more than one connection, the migration runner now pins a dedicated connection and passes it through to the dialect's lock methods. With a single-connection pool, pinning is skipped since it would starve other queries and isn't needed (only one connection exists). Closes #1153
|
This pull request has been marked as stale because it has been inactive for more than 21 days. Please reopen if you still intend to submit this pull request |
|
Hey @thetutlage, I went through this and looks good to me (minus the unrelated commit ofc). It is very hard to test since we got issues related to this only 3 times in 2 years and we do daily deployments but I will keep an eye when this get merged. |
|
This pull request has been marked as stale because it has been inactive for more than 21 days. Please reopen if you still intend to submit this pull request |
|
This pull request has been automatically closed because it has been inactive for more than 4 weeks. Please reopen if you still intend to submit this pull request |
When acquiring the advisory lock failed (for example, another process already holds it), the migration runner still attempted to release it during shutdown. The release then failed and threw E_UNABLE_RELEASE_LOCK, masking the real E_UNABLE_ACQUIRE_LOCK error and causing run() to reject instead of surfacing the failure via migrator.error. Track whether the lock was actually acquired and skip the release when it was not, so the original acquire error is preserved and run() resolves. Also add migrator coverage for a pool of 2 (the tightest pinning case), rollback with a multi-connection pool, and lock contention.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughMigration advisory locks now use pinned write connections across acquisition and release. PostgreSQL and MySQL dialect APIs accept optional connections, while migration and query-client tests cover pool sizing, contention, rollback, and cleanup. ChangesSession-bound advisory locks
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant MigrationRunner
participant WritePool
participant DatabaseSession
MigrationRunner->>WritePool: acquire pinned write connection
MigrationRunner->>DatabaseSession: acquire advisory lock
DatabaseSession-->>MigrationRunner: lock_status
MigrationRunner->>DatabaseSession: release advisory lock on same connection
DatabaseSession-->>MigrationRunner: lock_status
MigrationRunner->>WritePool: return pinned connection
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
commands/make_model.ts (1)
48-57: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDuplicate alias for
--transformerand--controller.The
--transformerflag usesalias: 'c', which is already assigned to--controller(line 43). This conflict means-cwill not behave correctly. Consider changing the transformer alias to't'or removing it.🐛 Proposed fix
`@flags.boolean`({ name: 'transformer', - alias: 'c', + alias: 't', description: 'Generate the transformer for the model', }) declare transformer: boolean🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@commands/make_model.ts` around lines 48 - 57, Update the transformer flag declaration in commands/make_model.ts to remove the duplicate alias `c`; assign it the unique alias `t` or omit the alias, while preserving the existing controller flag alias.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/dialects/pg.ts`:
- Around line 255-258: Parameterize all advisory-lock SQL inputs to prevent
injection: update getAdvisoryLock and the PostgreSQL unlock flow in
src/dialects/pg.ts (sites 255-258 and 271-274) to bind key, and update the MySQL
GET_LOCK and RELEASE_LOCK flows in src/dialects/mysql.ts (sites 252-255 and
268-269) to bind key and timeout through the query builder rather than
interpolating values into SQL.
In `@src/migration/runner.ts`:
- Around line 304-313: Move the acquire:lock emission out of the try/catch
surrounding lock acquisition in the migration runner. Keep getAdvisoryLock,
failure handling, and lockAcquired assignment inside the try, then emit
acquire:lock only after the catch block so listener errors cannot trigger
releaseLockConnection or leave inconsistent lock state.
In `@test/database/query_client.spec.ts`:
- Around line 539-559: Rename the cleanup callback parameter in all three
affected .run callbacks to avoid shadowing the file-level cleanup declaration,
and update each corresponding cleanup invocation to use the new alias. Preserve
the existing cleanup behavior and async teardown logic.
---
Outside diff comments:
In `@commands/make_model.ts`:
- Around line 48-57: Update the transformer flag declaration in
commands/make_model.ts to remove the duplicate alias `c`; assign it the unique
alias `t` or omit the alias, while preserving the existing controller flag
alias.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3daa64c2-b677-48e3-96ef-06fbc4ac0a25
📒 Files selected for processing (16)
commands/make_factory.tscommands/make_migration.tscommands/make_model.tscommands/make_seeder.tssrc/database/main.tssrc/dialects/mysql.tssrc/dialects/pg.tssrc/migration/runner.tssrc/migration/schema_dump/schema_states/sqlite.tssrc/query_client/index.tssrc/transaction_client/index.tssrc/types/database.tssrc/types/model.tssrc/types/querybuilder.tstest/database/query_client.spec.tstest/migrations/migrator.spec.ts
| async getAdvisoryLock(key: string, _timeout?: number, connection?: any): Promise<boolean> { | ||
| const query = this.client | ||
| .getWriteClient() | ||
| .raw(`SELECT PG_TRY_ADVISORY_LOCK('${key}') as lock_status;`) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file map =="
git ls-files 'src/dialects/*' 'src/**' | sed -n '1,200p'
echo
echo "== pg.ts relevant section =="
sed -n '230,290p' src/dialects/pg.ts
echo
echo "== mysql.ts relevant section =="
sed -n '230,290p' src/dialects/mysql.ts
echo
echo "== search advisory lock usages =="
rg -n "getAdvisoryLock|releaseAdvisoryLock|PG_TRY_ADVISORY_LOCK|PG_ADVISORY_UNLOCK|GET_LOCK|RELEASE_LOCK" src
echo
echo "== search for caller inputs around advisory lock keys =="
rg -n "getAdvisoryLock\(|releaseAdvisoryLock\(" srcRepository: adonisjs/lucid
Length of output: 13146
Parameterize advisory-lock SQL
src/dialects/pg.ts: bindkeyinPG_TRY_ADVISORY_LOCK/PG_ADVISORY_UNLOCK.src/dialects/mysql.ts: bindkeyandtimeoutinGET_LOCK, and bindkeyinRELEASE_LOCK.
The current string interpolation exposes this public API to SQL injection.
📍 Affects 2 files
src/dialects/pg.ts#L255-L258(this comment)src/dialects/pg.ts#L271-L274src/dialects/mysql.ts#L252-L255src/dialects/mysql.ts#L268-L269
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/dialects/pg.ts` around lines 255 - 258, Parameterize all advisory-lock
SQL inputs to prevent injection: update getAdvisoryLock and the PostgreSQL
unlock flow in src/dialects/pg.ts (sites 255-258 and 271-274) to bind key, and
update the MySQL GET_LOCK and RELEASE_LOCK flows in src/dialects/mysql.ts (sites
252-255 and 268-269) to bind key and timeout through the query builder rather
than interpolating values into SQL.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/database/query_client.spec.ts (1)
569-599: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winPin the connection for this lock test. It still depends on pool ordering: if
acquireConnection()returns the session that rangetAdvisoryLock(),releaseAdvisoryLock()will succeed and the assertion flips.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/database/query_client.spec.ts` around lines 569 - 599, Update the “release advisory lock fails with pool of 2 without pinned connection” test to explicitly pin or otherwise retain the connection used by getAdvisoryLock before testing releaseAdvisoryLock. Remove the pool-ordering-dependent heldConnection approach, ensure release runs on a different connection, and preserve the expected false result.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@test/database/query_client.spec.ts`:
- Around line 569-599: Update the “release advisory lock fails with pool of 2
without pinned connection” test to explicitly pin or otherwise retain the
connection used by getAdvisoryLock before testing releaseAdvisoryLock. Remove
the pool-ordering-dependent heldConnection approach, ensure release runs on a
different connection, and preserve the expected false result.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 70cd0533-b9b5-4dbf-aef6-f55321cf14ac
📒 Files selected for processing (4)
src/dialects/mysql.tssrc/dialects/pg.tssrc/types/database.tstest/database/query_client.spec.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/dialects/pg.ts
- src/dialects/mysql.ts
A throwing acquire:lock listener would trigger the catch block, which releases the pinned connection back to the pool while the advisory lock is still held on it, leaving the lock stranded and masking the listener error during shutdown. Emit the event only after the lock is confirmed acquired, outside the try/catch.
2 participants
Summary
rawQuerypicks a random connection from the pool each time. This causesreleaseAdvisoryLockto fail withE_UNABLE_RELEASE_LOCKwhen it lands on a different connection thangetAdvisoryLock.getAdvisoryLockandreleaseAdvisoryLockaccept an optionalconnectionparameter to use a specific connection instead of acquiring one from the pool.Closes #1153
Test plan
Summary by CodeRabbit
Bug Fixes
Tests