Issue 5755 change advisory lock to try advisory lock#5809
Conversation
|
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughConverted the PostgreSQL migration locking mechanism from a blocking advisory lock with timeout to a non-blocking ChangesAdvisory Lock Non-Blocking Refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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
🤖 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 `@schema-engine/connectors/sql-schema-connector/src/flavour/postgres.rs`:
- Around line 383-386: The current parsing of pg_try_advisory_lock result (the
let acquired = ... expression) uses unwrap_or(false), which treats parsing
failures as "lock not acquired"; change this to explicitly distinguish parsing
errors from a contended lock by: attempt to parse the boolean from
result.first().and_then(|row| row.at(0).and_then(|v| v.as_bool())) and if None
or a parsing error occurs, return/propagate a distinct error (e.g., a
parse/driver error) instead of treating it as false; only treat a successfully
parsed false as "lock contended". Ensure the code paths and error messages that
use the acquired variable (the parsing expression and the subsequent contention
check around pg_try_advisory_lock) are updated to handle and surface the parse
error separately.
- Around line 378-395: Extract the hardcoded advisory lock id (72707369) into a
named const (e.g., ADVISORY_LOCK_ID) and use that const when constructing the
SQL passed to connection.query_raw and when composing the
ConnectorError::from_msg string (and update the nearby comment to reference the
const as well); ensure the SQL string is formatted to include the const value
and the error message is built with the same const so the id is not duplicated
in literals anywhere in this block (look for connection.query_raw(...) and
ConnectorError::from_msg(...) to apply the change).
In `@schema-engine/sql-migration-tests/tests/migrations/advisory_locking.rs`:
- Around line 100-138: The test postgres_advisory_lock_contention_fails_fast
currently may leak the advisory lock if unwrap_err() or the assertion panics;
wrap the dedicated connection/lock_holder in a small RAII guard (e.g.,
LockHolderGuard) that holds the Quaint connection and issues "SELECT
pg_advisory_unlock(72707369)" in its Drop implementation so the unlock always
runs, replace the explicit tok(lock_holder.raw_cmd(...)) call with constructing
the guard, and relax the assertion by checking for the stable parts of the error
(e.g., "Another instance" plus the production constant if available such as an
ADVISORY_LOCK message constant in postgres.rs, otherwise keep a minimal
substring) instead of coupling to the literal "pg_try_advisory_lock".
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ce5a656e-4499-4de0-bab8-f45c81e38fe3
📒 Files selected for processing (3)
schema-engine/connectors/sql-schema-connector/src/flavour/postgres.rsschema-engine/connectors/sql-schema-connector/src/flavour/postgres/connector/wasm/mod.rsschema-engine/sql-migration-tests/tests/migrations/advisory_locking.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@schema-engine/connectors/sql-schema-connector/src/flavour/postgres.rs`:
- Around line 391-397: The error message in the ConnectorError creation within
the postgres.rs file hardcodes the lock ID value `72707369` instead of using the
`MIGRATE_ADVISORY_LOCK_ID` constant that is already used elsewhere in the same
file. Replace the hardcoded `72707369` string in the error message with a
reference to the `MIGRATE_ADVISORY_LOCK_ID` constant to maintain consistency
across the codebase.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f63404af-35f2-40e2-b702-abb9b5908b39
📒 Files selected for processing (1)
schema-engine/connectors/sql-schema-connector/src/flavour/postgres.rs
onno-vos-dev
force-pushed
the
issue-5755-change-advisory-lock-to-try-advisory-lock
branch
from
d3ccc56 to
77cc896
Compare
… concurrency issues when multiple nodes compete for the same advisory lock
onno-vos-dev
force-pushed
the
issue-5755-change-advisory-lock-to-try-advisory-lock
branch
from
77cc896 to
04768ef
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@schema-engine/sql-migration-tests/tests/migrations/advisory_locking.rs`:
- Around line 128-131: Add crosstarget-utils as a workspace dependency to
schema-engine/sql-migration-tests/Cargo.toml to enable timeout functionality.
Then wrap the await on the
me.apply_migrations(&migrations_directory).send().await call with a short async
timeout using the repository's established timeout pattern from
crosstarget-utils. This ensures that if the code regresses to blocking behavior
on the advisory lock, the test will fail quickly at the regression point rather
than hanging until the suite-level timeout.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 620861b6-5745-4509-92f1-c3fdcf7f9158
📒 Files selected for processing (3)
schema-engine/connectors/sql-schema-connector/src/flavour/postgres.rsschema-engine/connectors/sql-schema-connector/src/flavour/postgres/connector/wasm/mod.rsschema-engine/sql-migration-tests/tests/migrations/advisory_locking.rs
| let err = tok(async { me.apply_migrations(&migrations_directory).send().await }).unwrap_err(); | ||
|
|
||
| // Release the lock so the test database teardown is not blocked. | ||
| tok(lock_holder.raw_cmd("SELECT pg_advisory_unlock(72707369)")).unwrap(); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Read-only check for existing async timeout patterns and Tokio time feature availability.
# Expectation: Either existing Rust tests already use tokio::time::timeout/time::timeout,
# or Cargo.toml enables Tokio's `time`/`full` feature for the relevant test workspace.
rg -n -C3 'tokio::time::timeout|time::timeout' --type=rust
rg -n -C3 'tokio.*features|features\s*=\s*\[[^]]*(time|full)' Cargo.tomlRepository: prisma/prisma-engines
Length of output: 2922
🏁 Script executed:
# Check the specific Cargo.toml for the test workspace
find . -path "*/schema-engine/sql-migration-tests/Cargo.toml" -type fRepository: prisma/prisma-engines
Length of output: 112
🏁 Script executed:
# Look at the test file context around lines 128-131
sed -n '120,140p' schema-engine/sql-migration-tests/tests/migrations/advisory_locking.rsRepository: prisma/prisma-engines
Length of output: 835
🏁 Script executed:
# Check if crosstarget_utils is a dependency of the test workspace
rg -n 'crosstarget' schema-engine/sql-migration-tests/Cargo.tomlRepository: prisma/prisma-engines
Length of output: 47
🏁 Script executed:
# Check the tokio dependency in the test workspace's Cargo.toml
cat schema-engine/sql-migration-tests/Cargo.toml | grep -A 5 '^\[dependencies\]' | head -50Repository: prisma/prisma-engines
Length of output: 234
🏁 Script executed:
# Also check for any dev-dependencies that might include timeout utilities
rg -n 'tokio|timeout' schema-engine/sql-migration-tests/Cargo.toml -A 2Repository: prisma/prisma-engines
Length of output: 155
🏁 Script executed:
# Check the workspace-level tokio definition in the root Cargo.toml
rg -n 'tokio.*features' Cargo.toml | head -5Repository: prisma/prisma-engines
Length of output: 116
🏁 Script executed:
# Get more context around the tokio dependency in the workspace
rg -n 'tokio\s*=' Cargo.toml -A 3 | head -20Repository: prisma/prisma-engines
Length of output: 270
🏁 Script executed:
# Check what workspace members exist and if crosstarget_utils is available
fd -t f 'Cargo.toml' | grep -E '(workspace|crosstarget)' | head -10Repository: prisma/prisma-engines
Length of output: 99
🏁 Script executed:
# Check if crosstarget_utils is available as a workspace dependency
rg -n 'crosstarget' Cargo.toml | head -20Repository: prisma/prisma-engines
Length of output: 121
🏁 Script executed:
# Look for how other tests handle timeouts or async waits
rg -n 'tokio::time|timeout|Duration' schema-engine/sql-migration-tests/tests/ -t rust | head -20Repository: prisma/prisma-engines
Length of output: 3082
🏁 Script executed:
# Check the crosstarget_utils timeout implementation
cat libs/crosstarget-utils/src/native/time.rsRepository: prisma/prisma-engines
Length of output: 858
🏁 Script executed:
# Check if crosstarget_utils is already a dependency of sql-schema-connector or related packages
rg -n 'crosstarget' schema-engine/ -t toml | grep -v testRepository: prisma/prisma-engines
Length of output: 381
🏁 Script executed:
# Check how the test file imports and uses utilities
head -50 schema-engine/sql-migration-tests/tests/migrations/advisory_locking.rsRepository: prisma/prisma-engines
Length of output: 1698
Use crosstarget_utils timeout for fail-fast regression detection.
Line 128 holds the conflicting advisory lock and then awaits apply_migrations with no deadline; if this regresses back to a blocking lock, the test hangs until the suite-level timeout instead of failing at the regression point. Wrap only this await in a short async timeout.
To implement this, add crosstarget-utils as a dependency to the test workspace and wrap the await using the repository's established timeout pattern:
🧪 Proposed test hardening
- let err = tok(async { me.apply_migrations(&migrations_directory).send().await }).unwrap_err();
+ let err = tok(async {
+ crosstarget_utils::time::timeout(
+ std::time::Duration::from_secs(5),
+ me.apply_migrations(&migrations_directory).send(),
+ )
+ .await
+ .expect("apply_migrations did not fail fast while the advisory lock was held")
+ })
+ .unwrap_err();Also add to schema-engine/sql-migration-tests/Cargo.toml:
crosstarget-utils.workspace = true🤖 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 `@schema-engine/sql-migration-tests/tests/migrations/advisory_locking.rs`
around lines 128 - 131, Add crosstarget-utils as a workspace dependency to
schema-engine/sql-migration-tests/Cargo.toml to enable timeout functionality.
Then wrap the await on the
me.apply_migrations(&migrations_directory).send().await call with a short async
timeout using the repository's established timeout pattern from
crosstarget-utils. This ensures that if the code regresses to blocking behavior
on the advisory lock, the test will fail quickly at the regression point rather
than hanging until the suite-level timeout.
2 participants
Full disclosure: This is very AI assisted code but given my detailed description, it would have ended up in a similar state.
I noticed that sometimes migrations using Prisma can result in deadlocks when more than one instance is trying to run the migrations at a time. This is attempted to be mitigated through the use of advisory locks as can be seen
prisma-engines/schema-engine/connectors/sql-schema-connector/src/flavour/postgres.rs
Lines 375 to 378 in a09903a
Unfortunately the
ADVISORY_LOCK_TIMEOUTprisma-engines/schema-engine/connectors/sql-schema-connector/src/flavour/postgres.rs
Line 38 in a09903a
As per the documentation of Postgres: https://www.postgresql.org/docs/current/functions-admin.html#FUNCTIONS-ADVISORY-LOCKS there are two types of advisory locks that can be obtained here, one that is currently in use
pg_advisory_lockand one that (IMHO) is the better one since it would return instantly with a booleanfif it fails to obtain the lock. Essentially mitigating the risk of race conditions and deadlocks altogether.Proposal would be to change from
pg_advisory_locktopg_try_advisory_lockand handle the change from a timeout error type of scenario to a simple boolean shortcircuit of the failure to obtain the lock as observed here:prisma-engines/schema-engine/commands/src/commands/apply_migrations.rs
Lines 20 to 24 in a09903a
One can mimic the failure on a local Postgres quite easily by following below steps:
Produces following logs: