-
Notifications
You must be signed in to change notification settings - Fork 5
chore: manage mutagen daemon lifecycle #98
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
6f638fd to
e9fa904
Compare
| import NetworkExtension | ||
| import SwiftUI | ||
|
|
||
| @MainActor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
drive-by fix: I believe this is implicit in the ObservableObject conformance, but can't hurt to make it explicit.
| } | ||
|
|
||
| @MainActor | ||
| class MutagenDaemon: FileSyncDaemon { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Open to suggestions on how to test this. We can easily mock the gRPC calls, but mocking the child process is ???.
I've just done a good amount of manual testing for now.
| // Stop an orphaned daemon, if there is one | ||
| try? await connect() | ||
| try? await stop() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For context, mutagen daemon run acquires a file lock in the MUTAGEN_DATA_DIRECTORY. Multiple instances of the daemon will fail to start, so we need to make sure one isn't running before we start ours. If, for whatever reason, we were unable to kill the daemon, the new one will immediately fail and change the DaemonState.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR looks good on the surface, I'll have to test it locally later and look for edge cases.
| @@ -0,0 +1,11 @@ | |||
| syntax = "proto3"; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
QQ: Haven't seen a discussion before, but have we considered using the edition2023 version instead of proto3?
a013cde to
3384471
Compare
3384471 to
7f92c54
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I think there's an issue in the Coder_DesktopApp.swift file with quitting the VPN on quit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
1d0a6e1 to
ad1b24d
Compare
This makes a few improvements to #98: - The mutagen path & data directory can be now be configured on the MutagenDaemon, to support overriding it in tests (coming soon). - A mutagen daemon failure now kills the process, such that can it be restarted (TBC). - Makes start & stop transitions mutually exclusive via a semaphore, to account for actor re-entrancy. - The start operation now waits for the daemon to respond to a version request before completing. - The daemon is always started on launch, but then immediately stopped if it doesn't manage any file sync sessions, as to not run in the background unncessarily.

Closes coder/internal#381.
VPNfolder.Resourcesfolder whose contents are copied into the bundle at build time.MutagenDaemonabstraction for managing the mutagen daemon lifecycle, this class:mutagen daemon run, with aMUTAGEN_DATA_DIRECTORYinApplication Support/Coder Desktop/Mutagen, to avoid collisions with a system mutagen using~/.mutagen.gRPCconnection to the daemon socket.gRPCmutagen daemon runprocess if one exists.This PR does not embed the mutagen binaries within the bundle, it just handles the case where they're present.
Why is the file sync code in VPNLib?
When I had the FileSync code (namely protobuf definitions) in either:
FSLibframework targetEither the network extension crashed (in the first case) or the app crashed (in the second case) on launch.
The crash was super obtuse:
especially considering
SwiftProtobufdoesn't have a stable ABI and shouldn't be compiled as a framework.At least one other person has ran into this issue when importing
SwiftProtobufmultiple times:apple/swift-protobuf#1506 (comment)
Curiously, this also wasn't happening on local development builds (building and running via the XCode GUI), only when exporting via our build script.
Solution
We're just going to overload
VPNLibas the source of all our SwiftProtobuf & GRPC code. Since it's pretty big, and we don't want to embed it twice, we'll embed it once within the System Extension, and then have the app look for it in that bundle, seeLD_RUNPATH_SEARCH_PATHS. It's not exactly ideal, but I don't think it's worth going to war with XCode over.TODO
Processwith https://github.com/jamf/Subprocess