Reset `persistence_in_flight` counter on error in LSPS1/LSPS2
What the developer wrote
Authored by Elias Rohrer
Reset `persistence_in_flight` counter on error in LSPS1/LSPS2
Previously, if any `.await?` in the persist loop returned an error, the
`?` would propagate out of `persist()` before reaching the `fetch_sub`
at the end of the loop. This left the counter permanently > 0, causing
all subsequent `persist()` calls to early-return and effectively
disabling persistence for the lifetime of the handler.
Fix this by extracting the loop into `do_persist()` and unconditionally
resetting the counter via `store(0, Release)` in the outer `persist()`
after `do_persist()` returns, regardless of success or failure.
Co-Authored-By: HAL 9000
What changed, and why it matters
This commit fixes a bug in two Lightning liquidity service modules (LSPS1 and LSPS2). Previously, if saving data to disk failed partway through, an internal 'persistence in flight' counter would stay stuck above zero. Once stuck, the code would skip all future disk-saving attempts for the lifetime of the running service. This could silently stop important state from being saved, potentially leading to data loss or inconsistent service state after a restart. The fix wraps the save loop in a helper function and always resets the counter to zero afterward, whether the save succeeded or failed.
Review whether any other atomic counters or locks in the codebase rely on `fetch_sub` at the end of an async loop with `?` early returns, and apply the same wrapper/cleanup pattern. Consider adding a regression test that injects an I/O error during `persist()` and verifies subsequent persistence calls still execute.
Security signals we found
Denial-of-service-like liveness failure: persistence disabled for the lifetime of the handler after any I/O error
State inconsistency risk: unsaved state could lead to stale or lost LSPS1/LSPS2 service data
Atomic counter leak via early return on `?` error path
Fix pattern: unconditional resource cleanup in wrapper function
Evidence from the diff
In lightning-liquidity/src/lsps1/service.rs and lightning-liquidity/src/lsps2/service.rs, the persist() method used an atomic persistence_in_flight counter as a coalescing lock: fetch_add(1) on entry and fetch_sub(1) at the end of the loop. Any .await? inside the loop could return an Err before the fetch_sub, leaving the counter permanently > 0. Subsequent callers would see fetch_add(1) > 0 and early-return Ok(false), disabling persistence. The patch extracts the loop into do_persist() and unconditionally executes self.persistence_in_flight.store(0, Ordering::Release) in the outer persist() after do_persist() returns, plus adds a debug_assert that the counter is zero on success. This is a correctness fix for a liveness/state-loss bug rather than a memory-safety issue.
Changed components
lightning-liquidity/src/lsps1/service.rslightning-liquidity/src/lsps2/service.rsLSPS1 service handler persistence loopLSPS2 service handler persistence loopInspect captured patch +20 / −4
diff --git a/lightning-liquidity/src/lsps1/service.rs b/lightning-liquidity/src/lsps1/service.rs
index 0ac2420..0e13990 100644
--- a/lightning-liquidity/src/lsps1/service.rs
+++ b/lightning-liquidity/src/lsps1/service.rs
@@ -145,14 +145,22 @@ where
// TODO: We should eventually persist in parallel, however, when we do, we probably want to
// introduce some batching to upper-bound the number of requests inflight at any given
// time.
- let mut did_persist = false;
if self.persistence_in_flight.fetch_add(1, Ordering::AcqRel) > 0 {
// If we're not the first event processor to get here, just return early, the increment
// we just did will be treated as "go around again" at the end.
- return Ok(did_persist);
+ return Ok(false);
}
+ let res = self.do_persist().await;
+ debug_assert!(res.is_err() || self.persistence_in_flight.load(Ordering::Acquire) == 0);
+ self.persistence_in_flight.store(0, Ordering::Release);
+ res
+ }
+
+ async fn do_persist(&self) -> Result<bool, lightning::io::Error> {
+ let mut did_persist = false;
+
loop {
let mut need_remove = Vec::new();
let mut need_persist = Vec::new();
diff --git a/lightning-liquidity/src/lsps2/service.rs b/lightning-liquidity/src/lsps2/service.rs
index 665cda1..b7f6f2f 100644
--- a/lightning-liquidity/src/lsps2/service.rs
+++ b/lightning-liquidity/src/lsps2/service.rs
@@ -1786,14 +1786,22 @@ where
// TODO: We should eventually persist in parallel, however, when we do, we probably want to
// introduce some batching to upper-bound the number of requests inflight at any given
// time.
- let mut did_persist = false;
if self.persistence_in_flight.fetch_add(1, Ordering::AcqRel) > 0 {
// If we're not the first event processor to get here, just return early, the increment
// we just did will be treated as "go around again" at the end.
- return Ok(did_persist);
+ return Ok(false);
}
+ let res = self.do_persist().await;
+ debug_assert!(res.is_err() || self.persistence_in_flight.load(Ordering::Acquire) == 0);
+ self.persistence_in_flight.store(0, Ordering::Release);
+ res
+ }
+
+ async fn do_persist(&self) -> Result<bool, lightning::io::Error> {
+ let mut did_persist = false;
+
loop {
let mut need_remove = Vec::new();
let mut need_persist = Vec::new();
Why this scored 60/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.