Only log if we actually persisted LiquidityManager data (0.2 backport)#4662
Only log if we actually persisted LiquidityManager data (0.2 backport)#4662tnull wants to merge 1 commit into
LiquidityManager data (0.2 backport)#4662Conversation
Previously, we logged "Persisting LiquidityManager..." on each background processor wakeup, which can be very spammy, even on TRACE level. Here, we opt to only log if something actually needed to be repersisted and we did so (in case of failure we're logging that anyways, too).
|
I've assigned @valentinewallace as a reviewer! |
tnull
changed the title
LiquidityManager dataLiquidityManager data (0.2 backport)
| /// Persists the state of the service handlers towards the given [`KVStore`] implementation if | ||
| /// needed. | ||
| /// | ||
| /// Returns `true` if it persisted sevice handler data. |
There was a problem hiding this comment.
Typo: "sevice" → "service"
| @@ -1285,8 +1289,10 @@ where | |||
|
|
|||
| /// Persists the state of the service handlers towards the given [`KVStoreSync`] implementation. | |||
| /// | |||
There was a problem hiding this comment.
Same typo: "sevice" → "service"
| future.await?; | ||
| did_persist = true; | ||
| } else { | ||
| self.persist_peer_state(counterparty_node_id).await?; |
There was a problem hiding this comment.
Bug: did_persist is not set to true here. When a peer was initially flagged for removal but is no longer prunable (it got new state while we were persisting), needs_persist is forced to true on line 1845, and persist_peer_state here will actually write to the KV store (it checks needs_persist, finds it true, clears it, and persists). So real persistence happens, but the return value won't reflect it.
Low severity since it only affects logging, but the return value is inaccurate.
| self.persist_peer_state(counterparty_node_id).await?; | |
| self.persist_peer_state(counterparty_node_id).await?; | |
| did_persist = true; |
| future.await?; | ||
| did_persist = true; | ||
| } else { | ||
| self.persist_peer_state(client_id).await?; |
There was a problem hiding this comment.
Same bug as in lsps2: did_persist is not set to true when persist_peer_state actually writes data in the need_remove fallback path. When a peer was initially flagged for removal but is no longer prunable, needs_persist is forced to true on line 307, and persist_peer_state here will actually persist. The return value will be inaccurate.
| self.persist_peer_state(client_id).await?; | |
| self.persist_peer_state(client_id).await?; | |
| did_persist = true; |
Review SummaryIssues found:
Issues 1 and 2 are low severity since they only affect logging accuracy, but they make the |
valentinewallace
left a comment
valentinewallace
left a comment
There was a problem hiding this comment.
LGTM aside from Claude's feedback
4 participants
Backport of #4246.
Previously, we logged "Persisting LiquidityManager..." on each background processor wakeup, which can be very spammy, even on TRACE level.
Here, we opt to only log if something actually needed to be repersisted and we did so (in case of failure we're logging that anyways, too).