timvisee / prs

🔐 A secure, fast & convenient password manager CLI using GPG and git to sync.
https://gitlab.com/timvisee/prs
GNU General Public License v3.0
211 stars 8 forks source link

mux_client_request_session: read from master failed: Broken pipe #5

Closed ivan-avalos closed 3 years ago

ivan-avalos commented 3 years ago

Hello, I've been getting this error recently, especially when syncing and now with editing. It seems to happen randomly with some commands, especially the ones that perform Git operations. A long delay occurs before the following message gets printed.

$ prs edit ...
[wait like two or three minutes or so]
mux_client_request_session: read from master failed: Broken pipe
[editor opens]

This happens on Intel macOS 11.2.2.

timvisee commented 3 years ago

Thanks for your report.

Not sure what is causing this. I don't recognize the error message, so it must be coming from something outside of prs itself.

You are saying that this happens randomly. Does this mean that these commands usually work as expected (instantly), but sometimes hang with this error?

ivan-avalos commented 3 years ago

I didn't pay a lot of attention previously, that's just what I remember. It's probably related with prs querying git and disconnecting from it, or something, that's what I can infer from the message.

timvisee commented 3 years ago

Yeah, it might be. I've implemented connection reuse for git to speed things up, that might be causing issues here. Though, I believe I only enabled it for Linux.

timvisee commented 3 years ago

For reference: https://github.com/timvisee/prs/blob/697a05231bce69de67a04405982017a8d87fc1ed/lib/src/git.rs#L29-L35

It appears to be enabled on macOS as well

ivan-avalos commented 3 years ago

Weird, it is possibly caused by my system killing the ssh connection for some reason. But maybe there should be an environment variable to disable connection reuse, it's usually not that slow to me, I think.

timvisee commented 3 years ago

The SSH connection state is stored in a file, so ssh doesn't keep running. It just continues to use the old session state when opening a connection again (and should reconnect on failure). So I'm not really sure what is going on here. This is an SSH feature by the way, so it's not like I've hacked this in myself.

You could set the environment variable GIT_SSH_COMMAND yourself, in which case prs won't override it with the connection reuse configuration. I believe setting the following will set it back to it's default:

GIT_SSH_COMMAND="ssh"

But I'd argue that this error should never happen in the first place.

ivan-avalos commented 3 years ago

I disconnected from WiFi and noticed that prs edit (and probably other commands too) always tries to reach the remote Git repository, because it gave me an error. I don't think that should be the default behavior, as edit should not attempt to sync any remote changes, only sync should. Maybe this should go on another issue?

timvisee commented 3 years ago

(...) I don't think that should be the default behavior, as edit should not attempt to sync any remote changes, only sync should. Maybe this should go on another issue?

This is intended behavior. I had the following requirements:

The two commands I wanted to keep quick were the show and copy command, as they are commonly used when requesting your password. So no syncing is used here, as this takes time. It would be very annoying to wait for syncing each time you access your current password. Commands that edit the store, such as edit, generate or move, should use a password store that is as up-to-date as possible to prevent weird conflicts. So, these commands sync to ensure this before executing the action. I want to avoid conflicts like this at all costs because resolving them requires some some manual operations by the end user, this can be tricky, especially for conflicts with gpg files.

So, in short. edit syncs before editing the secret to prevent merge conflicts. I thought it to be fine to allow syncing for a second to achieve this. All commands that modify the password store do this. Disabling syncing by default and requiring users to invoke sync is possible, but this could easily result in some nasty situations. This does assume that a user uses multiple machines, of course.

Do you think this is a valid approach?

By the way, I do agree that a flag such as --no-sync should be added to prevent syncing as desired.

msfjarvis commented 3 years ago

Passing note about connection reuse, it's unsupported on BitBucket so the documentation should at least make a note of how to disable it for the people who do use it as a Git host. Obviously it'd be much better if prs is able to automatically handle it.

We encountered this same problem in APS (issue link) and fixed it by special-casing the error message, setting a flag internally to not use SSH multiplexing and calling the connect function again so if there's a reliable way to detect this specific failure then that might be a viable approach for prs as well.

timvisee commented 3 years ago

Thanks for your input, that's very useful information. I wasn't aware of that.

Maybe I should disable this feature all together for now, until special handling is implemented as you've suggested. Just to stay on the safe side.

timvisee commented 3 years ago

I've released v0.2.5 which disables SSH connection reuse, until something better is implemented.

It should be available once this release pipeline completes: https://gitlab.com/timvisee/prs/-/pipelines/274647121

timvisee commented 3 years ago

Since the actual error is fixed in the latest build, I'm closing this now.

I've moved the idea to improve on git SSH connection reuse to this issue: https://gitlab.com/timvisee/prs/-/issues/31