- Notifications
You must be signed in to change notification settings - Fork5
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
ethanndickson commentedMar 7, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
This stack of pull requests is managed byGraphite. Learn more aboutstacking. |
6f638fd toe9fa904Compare| 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 theObservableObject conformance, but can't hurt to make it explicit.
Uh oh!
There was an error while loading.Please reload this page.
| } | ||
| @MainActor | ||
| classMutagenDaemon:FileSyncDaemon{ |
ethanndicksonMar 7, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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?awaitconnect() | ||
| try?awaitstop() |
ethanndicksonMar 7, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 theMUTAGEN_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 theDaemonState.
ThomasK33 left a comment
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.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| @@ -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?
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
a013cde to3384471Compare3384471 to7f92c54CompareUh oh!
There was an error while loading.Please reload this page.
ThomasK33 left a comment
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
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
ThomasK33 left a comment
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 toad1b24dCompareethanndickson commentedMar 12, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
6f6049e intomainUh oh!
There was an error while loading.Please reload this page.
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.

Uh oh!
There was an error while loading.Please reload this page.
Closescoder/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
Processwithhttps://github.com/jamf/Subprocess