fix(iroh): improve shutdown logic in the magicsock #3763

dignifiedquire · 2025-12-11T10:19:56Z

Description

An attempt at improving the speed and reliability of the shutdown in magicsock

set closing asap
ensure remote actors are shut down using cancellation tokens
shutdown actors before waiting for wait_idle
use compare_exchange to improve closing logic

Ref #3762

github-actions · 2025-12-11T10:22:45Z

Documentation for this PR has been generated and is available at: https://n0-computer.github.io/iroh/pr/3763/docs/iroh/

Last updated: 2025-12-12T23:16:02Z

Frando · 2025-12-12T15:46:27Z

iroh/examples/transfer.rs


    let duration = start.elapsed();
    println!(
        "Received {} in {:.4}s ({}/s, time to first byte {}s, {} chunks)",


What about we move this log above the close right after drain_stream, and add another log "Closed cleanly" or such after the close timeout?
Then when we run it and it completes the transfer but only fails during shutdown we can see that, and not like now where there's no visible difference between the cases (because if the shutdown errors the whole fn errors)

Frando · 2025-12-12T15:48:02Z

iroh/src/magicsock.rs

+            .compare_exchange(false, true, Ordering::Acquire, Ordering::Relaxed)
+            .is_err()
+        {
+            return;


This means that if you call Endpoint::close twice on a clone of the endpoint, only the first call will wait for quinn::Endpoint::wait_idle, whereas the second call will complete immediately. I think we should always await Endpoint::wait_idle, even if closing is already true.

fixed, though we never actually come here, because Endpoint::close shortcircuits way before this

Frando · 2025-12-12T15:52:34Z

iroh/src/magicsock.rs

+        }
+
+        // Cancel any running netreports
+        self.shutdown_token_netreport.cancel();


We now have quite a few variables in play and I'm wondering if we can simplify it: I think we could remove self.msock.closing and instead use only two cancel tokens:
self.shutdown_token_close_start and self.shutdown_token_close_endpoint_closed
The former would be passed to netreport (i.e. replace shutdown_token_netreport and the closing AtomicBool), and the latter would be used for everything else.

(i.e. instead of having both a cancel token and an atomicbool, just use cancel_token.is_cancelled() for when the atomicbool was used)

resolved by merging @FRANDOS PR

flub · 2025-12-12T17:34:06Z

iroh-relay/src/server/client.rs

    /// Packets queued to send to the client
    send_queue: mpsc::Receiver<Packet>,
-    /// Important packets queued to send to the client
-    disco_send_queue: mpsc::Receiver<Packet>,


I had no idea we were prioritising these! That was probably a good call :)

flub · 2025-12-12T17:36:43Z

iroh/examples/transfer.rs

            );
            if res.is_err() {
-                println!("[{remote}] Did not disconnect within 3 seconds");
+                println!("[{remote}] Error: Did not disconnect within 4 seconds");


erm? uses 3s above?

flub · 2025-12-12T17:40:07Z

iroh/src/endpoint.rs


    #[tokio::test]
-    #[traced_test]
+    // #[traced_test]


i assume this was debugging attempts that you forgot to clean up?

flub · 2025-12-12T17:41:01Z

iroh/src/endpoint.rs

+        let barrier = Arc::new(tokio::sync::Barrier::new(2));
+
        // The server accepts the connections of the clients sequentially.
+        let s_b = barrier.clone();


I'd add some comments as to why this is needed as otherwise the next reader will be puzzled.

yeah, I am not sure yet I will leave it in, but it makes it a lot saner to debug these tests

flub · 2025-12-12T17:42:38Z

iroh/src/magicsock.rs


    fn is_closing(&self) -> bool {
-        self.closing.load(Ordering::Relaxed)
+        self.closing.load(Ordering::SeqCst)


Someone smarter than me says that using SeqCst means you haven't really thought about what you're doing...

flub · 2025-12-12T17:45:14Z

iroh/src/magicsock.rs

+        if self.msock.is_closed() {
+            return;
+        }


Maybe even move that before the trace?

flub · 2025-12-12T17:47:37Z

iroh/src/net_report.rs

                        }
-                        None => {}
+                        None => {
+                            debug!("report canceled");


these should probably be trace since they're entirely normal behaviour?

## Description Based on #3763 Tries to simplify shutdown state to be simpler to reason about. ## Breaking Changes  ## Notes & open questions  ## Change checklist  - [ ] Self-review. - [ ] Documentation updates following the [style guide](https://rust-lang.github.io/rfcs/1574-more-api-documentation-conventions.html#appendix-a-full-conventions-text), if relevant. - [ ] Tests if relevant. - [ ] All breaking changes documented. - [ ] List all breaking changes in the above "Breaking Changes" section. - [ ] Open an issue or PR on any number0 repos that are affected by this breaking change. Give guidance on how the updates should be handled or do the actual updates themselves. The major ones are: - [ ] [`quic-rpc`](https://github.com/n0-computer/quic-rpc) - [ ] [`iroh-gossip`](https://github.com/n0-computer/iroh-gossip) - [ ] [`iroh-blobs`](https://github.com/n0-computer/iroh-blobs) - [ ] [`dumbpipe`](https://github.com/n0-computer/dumbpipe) - [ ] [`sendme`](https://github.com/n0-computer/sendme)

- set closing asap - ensure remote actors are shut down using cancellation tokens - shutdown actors before waiting for `wait_idle`

## Description Based on #3763 Tries to simplify shutdown state to be simpler to reason about. ## Breaking Changes  ## Notes & open questions  ## Change checklist  - [ ] Self-review. - [ ] Documentation updates following the [style guide](https://rust-lang.github.io/rfcs/1574-more-api-documentation-conventions.html#appendix-a-full-conventions-text), if relevant. - [ ] Tests if relevant. - [ ] All breaking changes documented. - [ ] List all breaking changes in the above "Breaking Changes" section. - [ ] Open an issue or PR on any number0 repos that are affected by this breaking change. Give guidance on how the updates should be handled or do the actual updates themselves. The major ones are: - [ ] [`quic-rpc`](https://github.com/n0-computer/quic-rpc) - [ ] [`iroh-gossip`](https://github.com/n0-computer/iroh-gossip) - [ ] [`iroh-blobs`](https://github.com/n0-computer/iroh-blobs) - [ ] [`dumbpipe`](https://github.com/n0-computer/dumbpipe) - [ ] [`sendme`](https://github.com/n0-computer/sendme)

dignifiedquire marked this pull request as ready for review December 11, 2025 10:20

n0bot bot added this to iroh Dec 11, 2025

github-project-automation bot moved this to 🏗 In progress in iroh Dec 11, 2025

Frando reviewed Dec 12, 2025

View reviewed changes

Frando mentioned this pull request Dec 12, 2025

refactor: simplify shutdown state #3769

Merged

11 tasks

flub reviewed Dec 12, 2025

View reviewed changes

dignifiedquire and others added 18 commits December 12, 2025 23:17

fix(iroh): improve shutdown logic in the magicsock

c262f36

- set closing asap - ensure remote actors are shut down using cancellation tokens - shutdown actors before waiting for `wait_idle`

use compare_exachange for closing

43a7160

refactor(iroh-relay)!: remove remaining disco code

f145b83

test(iroh): improve spans

aef4fb9

update quinn

7208189

improve test and logging

9eda170

fix(iroh): cancel net_reports on shutdown

255d2d5

examples: expand timeouts in the transfer example

baa4978

fix: move actor shutdown later

79c7b9d

update quinn

7e99532

fix: improve netreport shutdown

8f96bca

example: add close to transfer example and reduce timeouts

4aa1b9c

fix(iroh): do not use the portmapper during shutdown

8f4678d

CR and fixups

7d87ab7

cleanup debugging

b684c8c

fix(iroh): only run qad as long as there are probes to run

926a9d4

update quinn

4be5f94

dignifiedquire force-pushed the fix-improve-shutdown branch from e5a46d9 to 4be5f94 Compare December 12, 2025 22:18

dignifiedquire added 2 commits December 12, 2025 23:26

fixup: boolean logic is hard

af3946b

fix: wire up actor shutdown again

594470c

fix(iroh): improve shutdown logic in the magicsock #3763

Are you sure you want to change the base?

fix(iroh): improve shutdown logic in the magicsock #3763

Conversation

dignifiedquire commented Dec 11, 2025

Description

Uh oh!

github-actions bot commented Dec 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

github-actions bot commented Dec 11, 2025 •

edited

Loading