libstore: Do not mark connections as bad when RemoteStore::narFromPath is called as a coroutine

[xokdvium]

Motivation

forced_unwind is thrown by Boost.Context when destroying the coroutine.
This lead to us resetting the remote connection for each narFromPath
with the ssh-ng:// store, so copying was very slow.

Also fixes some interrupt issues with ssh-ng store.

Context

Fixes #6950.

---

Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

[edolstra]

Hm, I'm not sure this is sound? If the coroutine is abandoned before the entire NAR has been copied (e.g. because the consumer of the coroutine throws an exception half-way through), won't that leave the connection in an undefined state? We only want to ignore the unwind exception if it occurs after the entire NAR has been copied.

In general, it's annoying that coroutines that produce NARs don't run to completion, since it causes a lot of unexpected issues (e.g. 7ba8443). I wonder if there is a way to fix that generically.

[xokdvium]

In general, it's annoying that coroutines that produce NARs don't run to completion

Hm, yeah I think I have a fix for that (look at 446e7f1). The reason is that they don't call read and the coroutine remains suspended, but we can opportunistically call resume the coroutine in the last read if the data is empty (that seems pretty safe, unless some code out there depends on the coroutine not completing). That way it runs to completion and we can set a flag that we've read the whole NAR.

There's also the question of what to do when the coroutine has produced more data than what we expect. Currently we just discard anything extra. I wonder if there are any bugs lurking in there.

[xokdvium]

These duplicate ifs seem tricky, but first first if doesn't get run on the first iteration because hasCoro is false. We don't want to call the coroutine if it's completed on the last iteration - otherwise it would segfault.

[edolstra]

If the coroutine runs to completion, then it won't get an unwind exception, right (since there's nothing to unwind)? So then withConnection() shouldn't be needed, unless I'm missing something.

[xokdvium]

Hm this is quite confusing indeed. I'll double-check if just the fix to serialise.cc is enough.

[xokdvium]

So then withConnection() shouldn't be needed

Yup, dropped all those changes. Resuming the coroutine in read is enough. I suppose it means that we could even revert 7ba8443.