Skip to content

Add PostgreSQL node failover leases - #1000

Open
joostjager wants to merge 1 commit into
lightningdevkit:mainfrom
joostjager:postgres-node-failover
Open

Add PostgreSQL node failover leases#1000
joostjager wants to merge 1 commit into
lightningdevkit:mainfrom
joostjager:postgres-node-failover

Conversation

@joostjager

Copy link
Copy Markdown
Contributor

Addresses #932 by adding minimal active/passive failover support for nodes using the built-in PostgreSQL store.

PostgresStore acquires an exclusive lease for its KV table, renews it in the background, and fences every mutation by validating and renewing the lease in the same transaction. Lease loss is terminal for the node and is exposed to the application (ldk-server) so the process can exit and restart from persisted state.

This initial implementation is intentionally limited to PostgreSQL. Generic stores and VSS are out of scope. The API, timing configuration, and backend support can be extended later if operational experience requires it.

@joostjager
joostjager requested a review from tnull July 22, 2026 14:14
@ldk-reviews-bot

ldk-reviews-bot commented Jul 22, 2026

Copy link
Copy Markdown

👋 Thanks for assigning @tnull 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.

@joostjager

joostjager commented Jul 22, 2026

Copy link
Copy Markdown
Contributor Author

I’m primarily looking for a concept ACK on the overall direction at this stage. The code has already gone through several iterations, and it will probably need another pass once we agree that the lease, fencing, and process-exit approach is the right path.

ldk-server branch I've been testing with: lightningdevkit/ldk-server@main...joostjager:ldk-server:postgres-node-failover

@joostjager
joostjager force-pushed the postgres-node-failover branch from 629386d to 056be7a Compare July 22, 2026 14:17
@benthecarman

Copy link
Copy Markdown
Contributor

Would be nice if this is abstracted out to also work with VSS. That way multi-device with a VSS backend can work

@joostjager

Copy link
Copy Markdown
Contributor Author

The safety-critical part seems mostly backend-specific to me, since lease validation and the KV mutation must be atomic within the backend. What layer of this do you envision abstracting?

If generalizing this later would not require much rewriting, I would prefer to leave any necessary code movement to a future VSS lease PR.

@joostjager

Copy link
Copy Markdown
Contributor Author

Friendly ping @tnull. I would like to agree on the overall approach before doing further implementation cleanup. If you do not currently have the bandwidth to take a look, please let me know too.

@tnull

tnull commented Jul 27, 2026

Copy link
Copy Markdown
Collaborator

Friendly ping @tnull. I would like to agree on the overall approach before doing further implementation cleanup. If you do not currently have the bandwidth to take a look, please let me know too.

I'll try to get back to a first look some time this week, but review on PRs for v0.8 has priority right now.

I think to begin with my first question is whether we don't need a more general version of this anyways, i.e., something a Node wrapper that allows to safely shutdown/restart the node when connectivity drops and we need to safely restart the node on persistence failures.

@joostjager

joostjager commented Jul 27, 2026

Copy link
Copy Markdown
Contributor Author

whether we don't need a more general version of this anyway

For an initial version, I think rebuilding can remain the responsibility of the infrastructure, for example Kubernetes. ldk-node reports the lease lost, ldk-server exits, and the infrastructure restarts the process.

The wrapper you propose seems quite a bit broader, and I do not think it is needed to meet the initial objective of failover? As mentioned in #1000 (comment), making lease+mutation atomic is backend-specific, so a general wrapper would not replace that work.

Make PostgresStore acquire a table-scoped lease during construction
and fence schema setup, migrations, and every mutation with it. Track
renewals conservatively and fail closed when the local deadline elapses,
including while database I/O is stalled.

Treat runtime lease loss as process-fatal, reject startup after lease
loss, and notify servers so they can exit without final persistence and
restart from durable state.
@joostjager
joostjager force-pushed the postgres-node-failover branch from 9d0c61a to 0ff95ef Compare August 3, 2026 14:18
@joostjager

Copy link
Copy Markdown
Contributor Author

Pushed a few AI fixes in the mean time. Once we agree on general direction and scope, I can go in and make this PR merge ready.

@tnull tnull added this to the 0.9 milestone Aug 7, 2026
@tnull

tnull commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator

Moved this to the v0.9 milestone. It seems we need more time here for general architectural discussions, but even out side of these "big picture" questions (whether to find a solution that also incorporates VSS or not etc) it seems that this PR still has a lot of edge cases uncovered. E.g:

  • External-effect window: a process can pause after a fenced persistence commit, lose its lease, then resume and send the already-authorized peer message or broadcast. PostgreSQL cannot fence Lightning peers or Bitcoin broadcasts.
  • Non-PostgreSQL stores: VSS and custom KVStore implementations have no lease/fencing support.
  • Non-participating processes: older LDK Node versions, direct SQL writers, or other implementations can ignore the lease.
  • Copied state: the same node state duplicated into another table, database, or backend gets a separate lease and can still produce duplicate active nodes.
  • Application containment: safe process termination depends on the application observing wait_for_lease_loss() and exiting correctly.
  • Generic builder path: constructing PostgresStore manually and passing it through build_with_store retains write fencing but does not install the node-level shutdown/notification handler.
  • Unusual schema resolution: normal qualified/unqualified names are substantially improved, but physical table identity and unusual search_path configurations still deserve explicit integration tests.
  • Database-level data loss: promotion of an asynchronously replicated PostgreSQL standby can lose both recent lease and node-state commits; application-level leasing cannot compensate for that.
  • Planned handoff ergonomics: Node::stop() deliberately retains the lease; takeover requires dropping the node/process.
  • Stale reads/API calls: reads are unfenced, and application threads may continue calling APIs after loss, although mutations should fail.
  • Current implementation readiness: the present PR head also has a non-Postgres build failure and needs cleanup plus broader failure-injection testing before merging.

@joostjager

Copy link
Copy Markdown
Contributor Author

Moved this to the v0.9 milestone. It seems we need more time here for general architectural discussions, but even out side of these "big picture" questions (whether to find a solution that also incorporates VSS or not etc) it seems that this PR still has a lot of edge cases uncovered. E.g:

My review request was explicitly for feedback on the general direction and proposed scope. Many of the items in this list restate scope boundaries and threat-model assumptions that were already called out, rather than identifying newly uncovered implementation edge cases.

Within the supported configuration, I do not currently see a concrete safety violation in the list. Mutations are atomically fenced by the lease. Some other scenarios may cause stale reads, duplicate network activity, reduced availability, or unsupported behavior, but it is not clear which one results in state corruption or funds risk.

Upgrades were not previously a compatibility concern because PostgreSQL support had not shipped. If #1012 ships first, that changes: its advisory lock and this PR's lease row do not coordinate.

@tnull

tnull commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator

Upgrades were not previously a compatibility concern because PostgreSQL support had not shipped. If #1012 ships first, that changes: its advisory lock and this PR's lease row do not coordinate.

Hmm? Why can't we revert #1012 in v0.9, worst case with a migration step?

@joostjager

Copy link
Copy Markdown
Contributor Author

Hmm? Why can't we revert #1012 in v0.9, worst case with a migration step?

The migration step indeed. How to migrate from one type of lock to another.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants