Replace PostgreSQL advisory locking with fenced leases - #1111
joostjager wants to merge 3 commits into
Conversation
|
👋 Thanks for assigning @benthecarman as a reviewer! |
1c20a0c to
a964bfc
Compare
|
If we indeed add this on the 0.8 milestone, I'll remove the migration because the temp. locking solution won't be in a release. |
Do it. |
Create the KV table, record its schema version, and create the listing index in one transaction so failed initialization rolls back schema changes. Keep database creation outside the transaction and preserve the session advisory lock for the store's lifetime. Add a regression test that forces index creation to fail and verifies the schema version update is rolled back.
Move pooled connection acquisition and query retry handling for writes and removals into execute_mutation. Preserve advisory lock checks, per-key write ordering, SQL, and error mapping so lease enforcement can be added to the shared helper separately.
a964bfc to
8f19873
Compare
Replace the temporary session advisory lock with an expiring lease in a companion table. Acquire it after schema setup and validate and renew it after each KV mutation in the same transaction, so stale writes and deletes roll back before commit. Renew idle leases in the background and release them by owner ID on drop. Explicitly panic on lease loss in the calling task or background renewal task, and on background failure or timeout, without adding recovery APIs. Cover rejected setup rollback, repeated idle renewal, renewal timeouts, stale writes and deletes, and releasing an old owner after takeover.
8f19873 to
6532d1b
Compare
|
I’ve removed the migration. This now directly replaces advisory locks with the lease table. I’m particularly happy to see all the scattered checks before and after operations to confirm we still hold the lock disappear. PostgreSQL now checks lease validity within the transaction. I had a good back-and-forth with AI to minimize the diff. I think it’s looking pretty good now. |
tnull
left a comment
tnull
left a comment
There was a problem hiding this comment.
Thanks, yet to do a very detailed review. Also tagging @benthecarman as a secondary reviewer as he did the first approach.
| .execute(&update_sql, &[&self.lease_owner_id.as_slice(), &lease_duration_secs]) | ||
| .await?; | ||
| if updated != 1 { | ||
| panic!("PostgreSQL node lease was lost; continuing may corrupt node state"); |
There was a problem hiding this comment.
This probably should be std::process:abort as panicking the tokio task won't abort the whole process, but just have the task return a JoinError in the end.
There was a problem hiding this comment.
The previous advisory-lock implementation used assert! rather than abort, and LDK Server sets panic = "abort" for both dev and release builds. But definitely seems safer to use std::process:abort, will change.
Note that not panicking wouldn't be a data consistency issue, because the consistency is guarded in each transaction.
There was a problem hiding this comment.
Although, it seems in other places in ldk-node, it's not an explicit process abort?
3 participants
Replace the temporary PostgreSQL advisory lock introduced in #1012 with a renewable lease in a companion table. The advisory-lock mechanism has not been released, so no migration is needed.
Acquire the lease atomically with schema setup, before loading persisted state. Validate ownership and expiry and renew the lease within each KV mutation transaction, so stale writes and deletes cannot commit. This replaces the separate lock checks before and after operations.
Renew idle leases in the background and release them by owner ID on drop. Lease loss panics in the calling task for mutations or in the background renewal task; background renewal errors and timeouts also panic. No recovery API or node lifecycle changes are introduced.
Tests cover setup rollback, contention, idle renewal, renewal timeouts, stale mutations, and release after takeover. Also verified contention, takeover after a crash, graceful restart, and stale-owner failure using two real LDK Server instances sharing a database.