Replace PostgreSQL advisory locking with fenced leases

[joostjager]

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.

[ldk-reviews-bot]

👋 Thanks for assigning @benthecarman as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

[tnull]

This needs a rebase for CI to run. Let me know when fully ready for review.

[joostjager]

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.

[tnull]

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.

[joostjager]

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]

Thanks, yet to do a very detailed review. Also tagging @benthecarman as a secondary reviewer as he did the first approach.

[tnull]

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.

[joostjager]

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.

[joostjager]

Although, it seems in other places in ldk-node, it's not an explicit process abort?

[tnull]

Hmm, not the biggest fan of extracting helpers that are less than 5-7 lines of code, as they simply tend to increase clutter and make it harder to see what's going on. Is there a particular benefit from this?

[joostjager]

In the later commit, it is expanded and no longer 5-7 lines.

[tnull]

Good to mention it's not handled automatically, but should we at least give the user half a sentence of guidance what this effectively means, i.e., what they are supposed to do if it panics?

[joostjager]

Added

[tnull]

Do we want/need these docs in test-only code to begin with?

[joostjager]

Reduced and made comment more generic

[tnull]

Seems we need to make sure we hard panic here, too:

Codex:

2. High — Renewal failure leaves the node running. The renewal task (/home/tnull/workspace/ldk-node-pr-1111-20260928-cPRuYZ/src/io/postgres_store/mod.rs:216) panics on failure, but nothing observes its handle during normal operation. Renewal stops while the node continues running; the
   process wide abort discussed earlier is still absent.

[joostjager]

The downside of hard panic is that we can't cleanly unit test this behavior anymore, and need to spawn a child process to let it panic. I tend towards just documenting to set panic=abort. ldk-server already has that.

[tnull]

Yes we can, see for example

ldk-node/src/io/test_utils.rs

Lines 172 to 195 in 96c1225

	async fn catch_future_unwind<F: Future>(future: F) -> std::thread::Result<F::Output> {
	let mut future = std::pin::pin!(future);
	std::future::poll_fn(|cx| {
	match std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| future.as_mut().poll(cx))) {
	Ok(std::task::Poll::Ready(output)) => std::task::Poll::Ready(Ok(output)),
	Ok(std::task::Poll::Pending) => std::task::Poll::Pending,
	Err(panic) => std::task::Poll::Ready(Err(panic)),
	}
	})
	.await
	}
	async fn assert_invalid_write_fails<K: KVStore + RefUnwindSafe>(
	kv_store: &K, primary_namespace: &str, secondary_namespace: &str, key: &str, data: Vec<u8>,
	) {
	let res = std::panic::catch_unwind(|| {
	KVStore::write(kv_store, primary_namespace, secondary_namespace, key, data)
	});
	if let Ok(fut) = res {
	if let Ok(write_res) = catch_future_unwind(fut).await {
	assert!(write_res.is_err());
	}
	}
	}

[joostjager]

But can this catch a process abort? The docs say:

This function only catches unwinding panics, not those that abort the process.

[joostjager]

I left it a panic! because of the unit tests, and also consistency with panics elsewhere in ldk-node. But open to different trade offs.

[tnull]

Hmm, so it should def. be consistent between the different cases, and it seems we want to enforce process abortion consistently. You're indeed correct about the catch_unwind behavior, forgot about that, but that still doesn't change the core issue, IMO. So either we just leave this behavior untested or could do something where we switch to a catchable panic! under cfg(test) while doing a 'real' process exit in production?

[joostjager]

I see the benefit, but I’d prefer to keep normal panics for this PR. LDK Server already uses panic = "abort", and we document that requirement for direct LDK Node users. Without it, lease checks still prevent stale writes, though the process may remain partially running.

I’d rather address library-enforced process termination consistently in a separate change than introduce test-specific behavior just for lease loss.

[tnull]

Default behavior is Burst - should we set something like:

 interval.set_missed_tick_behavior(tokio::time::MissedTickBehavior::Skip);

[joostjager]

Fixed

[tnull]

Should this have a graceful shutdown path like:

 let mut interval = tokio::time::interval(NODE_LEASE_RENEWAL_INTERVAL);
 loop {
-    interval.tick().await;
+    tokio::select! {
+        biased;
+        _ = &mut shutdown_rx => break,
+        _ = interval.tick() => {}
+    }
+
     let renewal = async {
         let mut locked = inner_ref.locked_client().await?;
         // Existing renewal body...
     };
-    tokio::time::timeout(NODE_LEASE_RENEWAL_TIMEOUT, renewal)
-        .await 
-        .expect("PostgreSQL node lease renewal timed out")
-        .expect("Failed to renew PostgreSQL node lease");
+    tokio::select! {
+        biased;
+        _ = &mut shutdown_rx => break,
+        result = tokio::time::timeout(NODE_LEASE_RENEWAL_TIMEOUT, renewal) => {
+            result
+                .expect("PostgreSQL node lease renewal timed out")
+                .expect("Failed to renew PostgreSQL node lease");
+        }
+    }
 }
+
+let release_result =
+    tokio::time::timeout(NODE_LEASE_RELEASE_TIMEOUT, inner_ref.release_node_lease()).await;
+// Send release_result to a synchronous completion channel observed by Drop.

[joostjager]

Refactored the shutdown path

[tnull]

I think instead of spawning yet another thread here we'll rather want to first send a stop signal and handle graceful shutdown as one arm in the lease task loop, something like:

 impl Drop for PostgresStore {
     fn drop(&mut self) {
-        if let Some(internal_runtime) = self.internal_runtime.as_ref() {
-            let renewal_task = self.lease_renewal_task.take();
-            if let Some(task) = renewal_task.as_ref() {
-                task.abort();
-            }
-
-            let runtime_handle = internal_runtime.handle().clone();
-            let inner = Arc::clone(&self.inner);
-            let _ = std::thread::spawn(move || {
-                runtime_handle.block_on(async move {
-                    if let Some(task) = renewal_task {
-                        let _ = task.await;
-                    }
-
-                    let _ = tokio::time::timeout(
-                        NODE_LEASE_RELEASE_TIMEOUT,
-                        inner.release_node_lease(),
-                    )
-                    .await;
-                });
-            })
-            .join();
+        if let Some(sender) = self.lease_shutdown_sender.take() {
+            let _ = sender.send(());
+        }
+        if let Some(receiver) = self.lease_shutdown_complete.take() {
+            let wait = NODE_LEASE_RELEASE_TIMEOUT + Duration::from_secs(1);
+            let result = receiver.recv_timeout(wait);
+            if let Some(logger) = self.inner.logger.as_ref() {
+                match result {
+                    Ok(Ok(())) => {},
+                    Ok(Err(e)) => log_error!(logger, "Failed to release PostgreSQL node lease: {e}"),
+                    Err(e) => log_error!(logger, "PostgreSQL lease shutdown did not complete: {e}"),
+                }
+            }
+        }
+        if let Some(task) = self.lease_renewal_task.take() {
+            task.abort(); // Only matters if the completion wait failed or timed out.
         }

         if let Some(internal_runtime) = self.internal_runtime.take() {
             if let Ok(internal_runtime) = Arc::try_unwrap(internal_runtime) {
                 internal_runtime.shutdown_background();
             }
         }
     }
 }

[joostjager]

Done

[joostjager]

I plan to follow up with an LDK Server PR adding a small retry loop when the lease is already held. A deployment supervisor could also handle this by restarting the server after failed acquisition, but keeping the standby running and waiting for the lease inside LDK Server seems cleaner.

[tnull]

I plan to follow up with an LDK Server PR adding a small retry loop when the lease is already held.

Hmm, I think conceptually stuff like that (which easily will get bigger/more complicated) should really live in LDK Node. IMO, we'll want to keep LDK Server as close to a simple API wrapper as possible.

[joostjager]

Hmm, I think conceptually stuff like that (which easily will get bigger/more complicated) should really live in LDK Node. IMO, we'll want to keep LDK Server as close to a simple API wrapper as possible.

Yes, we can also put a small retry loop in LDK Node. The trade-off is that the synchronous build call would remain blocked while waiting for the lease. Keeping the retry outside that call makes it easier for the application to expose its standby status and control cancellation. We don’t currently have a way to cancel an ongoing build.

A broader node lifecycle API could change that, but my immediate focus is failover for server deployments, which seems achievable with a small change today. I’m open to either location, provided we can keep that scope narrow.

[joostjager]

Regardless of where we put the retry loop, I don’t think it needs to block this PR. The priority here is replacing the temporary advisory locks with fenced leases. We can settle the retry behavior and its location in a follow-up.

[benthecarman]

claude review

[benthecarman]

This UPDATE holds the lease row lock until COMMIT, so every write across the store is serialized, even with the 10-connection pool. Could we use SELECT 1 FROM <lease> WHERE id = 1 AND owner_id = $1 AND expires_at > clock_timestamp() FOR SHARE here instead? Takeover and renewal still conflict with the share lock, so it fences the same way, and writes to different keys can run in parallel again. Keep it after the mutation so the lock order matches setup. The trade-off is that writes stop extending the lease, so only the background renewal keeps it alive.

[joostjager]

Sounds like a good idea: less writing and less locking, leaving renewal to the background task. I've pushed a fixup for that.

I also inlined renew_node_lease now that it only had one caller, which allowed some further simplification of the renewal and shutdown code.

[benthecarman]

One catch with claude's FOR SHARE suggestion: PostgreSQL lets new share lockers through even while an UPDATE is waiting, so continuous writes can starve the renewal past its 10s timeout. (The new test aborts the renewal task for this reason.) Adding UNIQUE (owner_id) to the lease table and using FOR KEY SHARE here fixes it. The renewal only touches expires_at, so it no longer conflicts, but a takeover changes owner_id, which is now a key column, so it still blocks until our commit. I checked this in psql and with an updated test_postgres_store_lease.

One gotcha: CREATE TABLE IF NOT EXISTS won't add UNIQUE to an existing lease table, and without it FOR KEY SHARE doesn't block takeover. So drop any *_node_lease tables created by earlier revisions of this PR. cleanup_store should drop the lease table too, since leftover tables from earlier runs hit exactly this.