Refactoring the shutdown process to fix a payment count bug #235

bjohnson5 · 2025-03-21T17:15:16Z

Closes #222

carlaKC

Taken a high level look at this and it looks reasonable! Much smaller diff than I was expecting as well, which is nice.

I do (sadly) think we should bite the bullet and add some meta-tests that test all this shutdown business, so that we can square it away once and for all.

Things like:

Spin up simulation with producers that finish their tasks, assert that we shut down
Shutdown with track payment currently executing, and make sure that we exit

I imagine it's going to be a hell of a lot of mocking, but I think it'll really be worthwhile in the long term. Happy to collaborate on a POC test to figure out the best way to get some utils set up to share the burden a bit!

simln-lib/src/lib.rs

bjohnson5 · 2025-03-31T19:24:43Z

Taken a high level look at this and it looks reasonable! Much smaller diff than I was expecting as well, which is nice.

I do (sadly) think we should bite the bullet and add some meta-tests that test all this shutdown business, so that we can square it away once and for all.

Things like:

Spin up simulation with producers that finish their tasks, assert that we shut down

Shutdown with track payment currently executing, and make sure that we exit

I imagine it's going to be a hell of a lot of mocking, but I think it'll really be worthwhile in the long term. Happy to collaborate on a POC test to figure out the best way to get some utils set up to share the burden a bit!

@carlaKC Absolutely agree that some tests should be added for this. Is there a test that requires mocking that would be good to look at as an example or would this be the first one of this kind?

carlaKC · 2025-03-31T19:35:00Z

Is there a test that requires mocking that would be good to look at as an example or would this be the first one of this kind?

The mocks in lib.rs should give you a decent idea of the way we've been approaching mocking. These are just gnarley because we'll want to mock internal_run and then assert whatever set of events we're trying to test. It will be more complicated that the quite simple tests that we currently have, but nothing fundamentally different.

bjohnson5 · 2025-03-31T20:52:24Z

For reference here is what is done in this PR:

Removed an unnecessary clone on the event_sender in internal_run so that the sender will be dropped and the receivers will close.
Removed the tokio::select! statement and shutdown listener from consume_simulation_results because this is a channel receiver and will close when all of its corresponding senders are dropped.
Removed the tokio::select! statement and shutdown listener from consume_events because this is a channel receiver and will close when all of its corresponding senders are dropped.
Removed the tokio::select! statement and shutdown listener from produce_simulation_results because this is a channel receiver and will close when all of its corresponding senders are dropped.
Refactored track_payment_result to use a timer to wait a given amount of time for current tracking to complete. This function is also now responsible for shutting down the node implementations that are tracking payments (node.track_payment) with a local trigger/listener.

The new shutdown flow looks like this:

The shutdown is triggered by either A.) The producer tasks finish due to payment count being met, B.) The total time has been met, or C.) The user presses ctrlc
The run_results_logger gets the shutdown signal and returns.
The produce_events function gets the shutdown signal (if it was not the one to trigger it) and returns. This drops its sender.
If simln is in the middle of tracking a payment, the track_payment_result function gets the shutdown signal and starts a timer.
The consume_events function quits its recv (because the produce_events dropped its sender), and then drops the consume_events sender.
The produce_simulation_results function quits its recv (because consume_events dropped its sender), and then drops the produce_simulation_results sender.
The consume_simulation_results function quits its recv because produce_simulation_results dropped its sender.
If simln is in the middle of tracking a payment, either the timer expires and it shuts down or the payments finish getting tracked and it shuts down.

carlaKC · 2025-04-07T13:02:01Z

Nice! Thanks for the thorough walkthrough.

The shutdown is triggered by either A.) The producer tasks finish due to payment count being met, B.) The total time has been met, or C.) The user presses ctrlc

Interested that we don't hit any unexpected errors that make us close out. Is that because we'll keep running even if we fail to send a payment (eg, one of the nodes has shut down)?

bjohnson5 · 2025-04-07T14:50:32Z

Nice! Thanks for the thorough walkthrough.

The shutdown is triggered by either A.) The producer tasks finish due to payment count being met, B.) The total time has been met, or C.) The user presses ctrlc

Interested that we don't hit any unexpected errors that make us close out. Is that because we'll keep running even if we fail to send a payment (eg, one of the nodes has shut down)?

Ok yes, good point. Another way the shutdown is triggered is D.) An error is thrown while setting up consumers and producers or while the producers and consumers are running.

bjohnson5 · 2025-04-08T21:38:12Z

@carlaKC I made a first attempt at adding some shutdown tests. Let me know what you think. There is probably some cleanup to do and a few other test cases we could add but this is a starting point.

carlaKC

Took a high level look at the tests and they look great, approach ACK

carlaKC · 2025-04-14T17:57:45Z

simln-lib/src/lib.rs

@@ -1691,4 +1693,224 @@ mod tests {

        assert!(result.is_ok());
    }
+
+    #[tokio::test]
+    async fn test_shutdown_timeout() {


Once we have #81, we'll be able to replace the clock with a test clock so that this can be much more precise.

I'm tempted to put this test off till then, had a lot of bad experiences with timing based tests falling apart on the potato that github runs CI on.

Yeah I wondered about that. Most of the shutdown tests have some kind of timing aspect... checking the runtime against expected runtime or sleeping for a few seconds before manually shutting down. Should we wait to do any of this until we figure out a better way to handle timing?

How about tokio advance?

carlaKC · 2025-04-14T17:58:24Z

simln-lib/src/lib.rs

+        let mut mock_node_2 = MockLightningNode::new();
+
+        // Set up node 1 expectations
+        mock_node_1.expect_get_info().return_const(node_1.clone());


This is great! Would be nice if we could pull some of this out into more generic helpers so that we can compose tests like this more easily.

Edit: you did this in the next commit, awesome. Happy for you to squash that now.

Yeah, this PR has several merge commits in it where I updated from master. Would you prefer just squashing the whole PR into 1 commit?

Happy to have multiple commits of changes, but I would like to get rid of the merge commits in the PR with a rebase.

Rebased to remove the merge commits. Cleaner looking git history now.

simln-lib/src/lib.rs

carlaKC

Looking great!

Would be nice to add some coverage for the cases where we have multiple activities, specifically:

One with a count, one without -> make sure the second one keeps running
Two activities, one has a permanent error -> make sure both exit

Also think that we need some coverage for the track_payment/timer stuff if possible, even if it's not end to end (I suspect this will be tricky to mock)

carlaKC · 2025-04-22T18:47:45Z

simln-lib/src/lib.rs

-                                }
-                            }
-                        };
+        let output = output_receiver.recv().await;


Above, "In the multiple-producer case, a single producer shutting down does not drop all sending channels so the consumer will not exit and a trigger is required" won't apply anymore so can be deleted

Do you mean just remove that part of the comment?

Yeah, just remove the old comment that will no longer apply

carlaKC · 2025-04-22T18:48:50Z

simln-lib/src/lib.rs

+            let (stop, listen) = triggered::trigger();
+
+            // Timer for waiting after getting the shutdown signal in order for current tracking to complete
+            let mut timer: Option<tokio::time::Sleep> = None;


nit: move tokio::time::Sleep to top level imports

carlaKC · 2025-04-22T18:51:36Z

simln-lib/src/lib.rs

-                },
+
+            // Trigger and listener to stop the implementation specific track payment functions (node.track_payment())
+            let (stop, listen) = triggered::trigger();


nit: rename track_payment_trigger/track_payment_listener so this is a little more readable.

carlaKC · 2025-04-22T18:57:01Z

simln-lib/src/lib.rs

+                    Some(_) = conditional_sleeper(timer) => {
+                        log::error!("Track payment failed for {}. The shutdown timer expired.", hex::encode(hash.0));
+                        stop.trigger();
+                        timer = None;


If we set this to None and our select is biased to always select the top case, don't we hit a loop here?

listener.is_triggered() && timer.is_none() = T

timer = (sleep 3 seconds)

conditional_sleeper = T (after 3 seconds)

stop.trigger()

timer=None

listener.clone().is_triggered && timer.is_none() = T

Loop repeats

I think that we can remove biased here (we don't need it) and not set timer=None - even without biased the select will randomly choose a branch that's ready, so we could end up looping a few times.

carlaKC · 2025-04-22T19:05:05Z

Also think that it would be nice to make a note about this in dev docs to explain a shorter version of what we've discussed in the issue (happy for that to be a followup or do it myself once this is in).

bjohnson5 · 2025-04-23T20:14:39Z

Looking great!

Would be nice to add some coverage for the cases where we have multiple activities, specifically:

One with a count, one without -> make sure the second one keeps running

Two activities, one has a permanent error -> make sure both exit

Also think that we need some coverage for the track_payment/timer stuff if possible, even if it's not end to end (I suspect this will be tricky to mock)

Added two more tests for these cases. I will have to think through the timer test case some more, that will be tricky.

bjohnson5 · 2025-04-23T20:18:37Z

Summary of remaining tasks for this PR:

Add explanation to the developer docs
Add a test case for track_payment timer expiration shutdown
Think about how to best handle time bases tests (maybe tokio::advance, maybe reference Feature: Simulation Time #81)
Think about more shutdown test coverage that is needed

bjohnson5 requested a review from carlaKC March 21, 2025 17:41

bjohnson5 marked this pull request as ready for review March 21, 2025 17:41

bjohnson5 mentioned this pull request Mar 21, 2025

Bug: count in defined activity produces count-1 events (one less than expected) #222

Open

carlaKC reviewed Mar 28, 2025

View reviewed changes

simln-lib/src/lib.rs Outdated Show resolved Hide resolved

bjohnson5 requested a review from carlaKC April 8, 2025 21:38

carlaKC reviewed Apr 14, 2025

View reviewed changes

carlaKC reviewed Apr 22, 2025

View reviewed changes

bjohnson5 added 6 commits April 23, 2025 11:30

Refactoring the shutdown process to fix a payment count bug

ea3cc3b

Removing unnecessary clone so that the event sender will be dropped

e4479da

Adding two tests to verify the shutdown process

b441940

Cleaning up the shutdown tests so that they share setup code

59bec01

Adding a shutdown test for the manual shutdown case

0121dcb

Adding a test for the error shutdown case

e031ddd

bjohnson5 force-pushed the 222-fixing-payment-count branch from f55644b to e031ddd Compare April 23, 2025 16:40

bjohnson5 added 2 commits April 23, 2025 13:06

Cleaning up comments, variable names, and imports

5936a15

Adding shutdown test cases for multiple activities

db96a89

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactoring the shutdown process to fix a payment count bug #235

Refactoring the shutdown process to fix a payment count bug #235

bjohnson5 commented Mar 21, 2025

carlaKC left a comment

bjohnson5 commented Mar 31, 2025

carlaKC commented Mar 31, 2025

bjohnson5 commented Mar 31, 2025

carlaKC commented Apr 7, 2025

bjohnson5 commented Apr 7, 2025

bjohnson5 commented Apr 8, 2025

carlaKC left a comment

carlaKC Apr 14, 2025

bjohnson5 Apr 15, 2025

carlaKC Apr 22, 2025

carlaKC Apr 14, 2025

bjohnson5 Apr 15, 2025

carlaKC Apr 22, 2025

bjohnson5 Apr 23, 2025

carlaKC left a comment

carlaKC Apr 22, 2025

bjohnson5 Apr 23, 2025

carlaKC Apr 23, 2025

carlaKC Apr 22, 2025

carlaKC Apr 22, 2025

carlaKC Apr 22, 2025

carlaKC commented Apr 22, 2025

bjohnson5 commented Apr 23, 2025

bjohnson5 commented Apr 23, 2025

Refactoring the shutdown process to fix a payment count bug #235

Are you sure you want to change the base?

Refactoring the shutdown process to fix a payment count bug #235

Conversation

bjohnson5 commented Mar 21, 2025

carlaKC left a comment

Choose a reason for hiding this comment

bjohnson5 commented Mar 31, 2025

carlaKC commented Mar 31, 2025

bjohnson5 commented Mar 31, 2025

carlaKC commented Apr 7, 2025

bjohnson5 commented Apr 7, 2025

bjohnson5 commented Apr 8, 2025

carlaKC left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlaKC left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlaKC commented Apr 22, 2025

bjohnson5 commented Apr 23, 2025

bjohnson5 commented Apr 23, 2025