[client] auto-update #4732
[client] auto-update #4732
Conversation
Co-authored-by: Bethuel Mmbaga <[email protected]>
…rsion-networkmap Move autoUpdateVersion inside NetworkMap
Modify client-side behavior
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
version/update.go (1)
172-178: Nil pointer dereference on first fetch.On the first call to
fetchVersion(),u.latestAvailableisnil(not initialized inNewUpdate). Callingu.latestAvailable.Equal(latestAvailable)on line 175 will panic.u.versionsLock.Lock() defer u.versionsLock.Unlock() +if u.latestAvailable != nil && u.latestAvailable.Equal(latestAvailable) { -if u.latestAvailable.Equal(latestAvailable) { return false } u.latestAvailable = latestAvailable
♻️ Duplicate comments (3)
client/internal/updatemanager/installer/installer_run_darwin.go (1)
160-168: Critical:cmd.Start()followed bycmd.CombinedOutput()causes double execution.
CombinedOutput()internally callsStart(), so calling both results in an error or undefined behavior. This was flagged in a past review. Usecmd.Stdoutandcmd.Stderrbuffers withcmd.Start()andcmd.Wait()instead.func (u *Installer) installPkgFile(ctx context.Context, path string) error { log.Infof("installing pkg file") volume := "/" for _, v := range strings.Split(path, "\n") { trimmed := strings.TrimSpace(v) if strings.HasPrefix(trimmed, "volume: ") { volume = strings.Split(trimmed, ": ")[1] } } cmd := exec.CommandContext(ctx, "installer", "-pkg", path, "-target", volume) - if err := cmd.Start(); err != nil { - return fmt.Errorf("error running pkg file: %w", err) - } - res, err := cmd.CombinedOutput() - // todo write out log result to file too - if err != nil { - return fmt.Errorf("error running pkg file: %w, output: %s", err, string(res)) + output, err := cmd.CombinedOutput() + if err != nil { + return fmt.Errorf("error running pkg file: %w, output: %s", err, string(output)) } log.Infof("pkg file installed successfully") return nil }client/internal/updatemanager/manager.go (2)
171-184: Stop() reads m.cancel outside the lock - acknowledged as intentional.Per past review comments, the author confirmed that
m.cancelaccess outside the mutex is intentional, as the mutex only protectsupdateandexpectedVersionfields. The caller is responsible for synchronization of Start/Stop calls.
83-116: CheckUpdateSuccess has potential race condition with state deletion.The method deletes the update state (line 322-328 in loadAndDeleteUpdateState) before publishing the success event (lines 107-113). If the process crashes between deletion and event publication, the user won't be notified of a successful update.
This was noted in the PR comments. Consider reordering to publish the event first, or at minimum document this known limitation.
Consider restructuring to publish the event before deleting state:
func (m *Manager) CheckUpdateSuccess(ctx context.Context) { + // Load state first without deleting + stateType := &UpdateState{} + m.stateManager.RegisterState(stateType) + if err := m.stateManager.LoadState(stateType); err != nil { + // handle error + } + state := m.stateManager.GetState(stateType) + // ... validate and publish event ... + // Then delete state after successful publication
🟡 Minor comments (11)
client/internal/updatemanager/installer/doc.go-1-4 (1)
1-4: Update package description to include all supported platforms.The description mentions "Windows, macOS" but the Update Flow section includes Linux. Update the package description for accuracy and completeness.
-// Package installer provides functionality for managing NetBird application -// updates and installations across Windows, macOS. It handles +// Package installer provides functionality for managing NetBird application +// updates and installations across Windows, macOS, and Linux. It handlesclient/internal/updatemanager/installer/doc.go-71-90 (1)
71-90: Add Linux file location documentation.The File Locations section documents Windows and macOS temporary directory paths but omits Linux, despite Linux being supported in the Update Flow. Add the Linux path for consistency and completeness.
// Files created during installation: // // tmp-install/ @@ -71,6 +71,11 @@ // macOS: // - Path: /var/lib/netbird/tmp-install // - Requires root permissions +// +// Linux: +// - Path: /var/lib/netbird/tmp-install +// - Requires root permissions (if installed system-wide) +// - Or ~/.local/netbird/tmp-install for user installationsclient/internal/updatemanager/installer/doc.go-92-92 (1)
92-92: Complete or clarify the API Reference section.Line 92 introduces an "API Reference" section but provides no content. Either document the public API and command-line parameters mentioned in line 32 (--temp-dir, --service-dir, --installer-file, --dry-run), or remove this section header to avoid implying incomplete documentation.
// # API Reference // +// Command-line parameters for updater process: +// - --temp-dir: Temporary directory path containing installer and updater binary +// - --service-dir: Service installation directory path +// - --installer-file: Path to downloaded installer file (if applicable) +// - --dry-run: Optional flag; when set, validates without executing installation +// // # Cleanupclient/internal/updatemanager/installer/installer.go-30-32 (1)
30-32:TempDir()ignores the storedtempDirfield.The method returns an empty string instead of
u.tempDir, makingNewWithDir()ineffective.func (u *Installer) TempDir() string { - return "" + return u.tempDir }client/internal/updatemanager/installer/installer_common.go-261-265 (1)
261-265: Typo: log message says "source file" for destination file.The warning on line 263 refers to closing the destination file (
out), but the message says "source file."defer func() { if err := out.Close(); err != nil { - log.Warnf("failed to close source file: %v", err) + log.Warnf("failed to close destination file: %v", err) } }()client/internal/updatemanager/installer/result.go-194-198 (1)
194-198: Wrong variable in log message.Line 196 logs
err(the rename error) instead ofcleanupErr(the remove error).if err := os.Rename(tmpPath, rh.resultFile); err != nil { if cleanupErr := os.Remove(tmpPath); cleanupErr != nil { - log.Warnf("Failed to remove temp result file: %v", err) + log.Warnf("Failed to remove temp result file: %v", cleanupErr) } return err }client/internal/updatemanager/reposign/verify.go-56-86 (1)
56-86: Storing revocation list on struct is not thread-safe.Line 63 assigns
a.revocationList = revocationListwhich mutates struct state. IfVerifyis called concurrently on the sameArtifactVerifyinstance, this could cause a data race.Consider passing the revocation list as a parameter to
loadArtifactKeysinstead:func (a *ArtifactVerify) Verify(ctx context.Context, version string, artifactFile string) error { version = strings.TrimPrefix(version, "v") revocationList, err := a.loadRevocationList(ctx) if err != nil { return fmt.Errorf("failed to load revocation list: %v", err) } - a.revocationList = revocationList - artifactPubKeys, err := a.loadArtifactKeys(ctx) + artifactPubKeys, err := a.loadArtifactKeys(ctx, revocationList) if err != nil { return fmt.Errorf("failed to load artifact keys: %v", err) }And update
loadArtifactKeysaccordingly:-func (a *ArtifactVerify) loadArtifactKeys(ctx context.Context) ([]PublicKey, error) { +func (a *ArtifactVerify) loadArtifactKeys(ctx context.Context, revocationList *RevocationList) ([]PublicKey, error) { // ... - return ValidateArtifactKeys(a.rootKeys, data, *signature, a.revocationList) + return ValidateArtifactKeys(a.rootKeys, data, *signature, revocationList) }Also remove the
revocationListfield from the struct if no longer needed.Committable suggestion skipped: line range outside the PR's diff.
client/internal/updatemanager/reposign/artifact_test.go-370-371 (1)
370-371: Error fromParseSignatureis ignored.The error return value is discarded with
_. If parsing fails, subsequent assertions will pass a nil signature. Userequire.NoErrorfor consistency with the rest of the test suite.- sig, _ := ParseSignature(sigData) + sig, err := ParseSignature(sigData) + require.NoError(t, err)client/internal/updatemanager/reposign/artifact_test.go-882-933 (1)
882-933: Test name and logic mismatch—only one key is expired.The test
TestValidateArtifactKeys_TwoKeysBothExpiredgenerates one key with 24-hour expiration and another with 1ms expiration. After a 10ms sleep, only the second key expires. The assertion at line 932 expects 1 valid key, confirming only one expired—contradicting the test name.Either rename to reflect the actual scenario or fix the test to expire both keys.
-func TestValidateArtifactKeys_TwoKeysBothExpired(t *testing.T) { - // Test ValidateArtifactKeys with a bundle containing two expired keys - // Should fail because no valid keys remain +func TestValidateArtifactKeys_TwoKeysOneExpired_SecondKey(t *testing.T) { + // Test ValidateArtifactKeys with a bundle where the second key expires + // Should return only the first (still valid) keyOr to actually test both expired:
- _, _, pubPEM1, _, err := GenerateArtifactKey(rootKey, 24*time.Hour) + _, _, pubPEM1, _, err := GenerateArtifactKey(rootKey, 1*time.Millisecond)client/ui/client_ui.go-1112-1131 (1)
1112-1131: Potential race condition ons.updateContextCancelaccess.The event handler reads and writes
s.updateContextCancelwithout synchronization. If multiple events arrive concurrently, this could lead to a race condition.Consider protecting access with a mutex:
+ s.mutex.Lock() if s.updateContextCancel != nil { s.updateContextCancel() s.updateContextCancel = nil } subCtx, cancel := context.WithCancel(s.ctx) go s.eventHandler.runSelfCommand(subCtx, "update", "--update-version", targetVersion) s.updateContextCancel = cancel + s.mutex.Unlock()Note: You may need to add a
mutex sync.Mutexfield toserviceClientif one doesn't already exist for this purpose, or use an existing synchronization mechanism.Committable suggestion skipped: line range outside the PR's diff.
client/internal/updatemanager/reposign/artifact.go-166-219 (1)
166-219: Improve error semantics when all keys are filtered out inValidateArtifactKeysThe filtering logic for expired/revoked keys looks correct, but the final error message is slightly misleading:
- If all keys are expired (but not revoked), or filtered for other reasons, you still return
fmt.Errorf("all %d artifact keys are revoked", len(pubKeys))(Line 215).This can confuse operators trying to distinguish between expiry vs revocation.
Consider a more generic message and, optionally, including counts:
if len(validKeys) == 0 { log.Debugf("no valid public keys found for artifact keys") - return nil, fmt.Errorf("all %d artifact keys are revoked", len(pubKeys)) + return nil, fmt.Errorf("no valid artifact keys found (checked %d; may be expired or revoked)", len(pubKeys)) }The rest of the timestamp and revocation handling here reads well.
🧹 Nitpick comments (46)
client/internal/updatemanager/installer/repourl_dev.go (1)
1-7: Avoid committing developer-specific, plain-HTTP dev URLsThe build tag separation looks correct, but this dev constant hard-codes a private IP and uses HTTP. That’s fine for local testing, but it’s easy for this to become stale or accidentally reused elsewhere.
Consider:
- Driving this URL from env/config (with this value as a local default only), and/or
- Adding a short comment clarifying this is for local dev signrepo only and must never be used in prod or shared environments.
client/internal/updatemanager/installer/repourl_prod.go (1)
1-7: Make production signing-keys base URL overridableThe constant and build tag usage look fine, but hard-coding a GitHub raw URL as the only production source may be limiting (e.g., self‑hosted or air‑gapped environments, custom key repos).
Consider:
- Allowing an override via config/env flag (with this constant as the default), and
- Documenting how operators can point clients to an alternative signing-keys repository if needed.
client/internal/updatemanager/installer/doc.go (1)
76-80: Note that hardcoded paths should be configurable.The PR objectives and prior review comments flagged that hardcoded paths like /var/lib/netbird/tmp-install should be configurable rather than baked into the documentation. Consider adding a note that these paths are currently hardcoded but should be made configurable in a future iteration, or link to configuration mechanisms if they exist.
// Windows: // - Path: %ProgramData%\Netbird\tmp-install // - Example: C:\ProgramData\Netbird\tmp-install // // macOS: // - Path: /var/lib/netbird/tmp-install +// - Note: Currently hardcoded; consider making configurable via package configuration // - Requires root permissionsmanagement/internals/controllers/network_map/controller/controller.go (1)
380-380: TODO comment noted for future enhancement.This TODO suggests adding
PeerConfigto theSyncResponseinDeletePeer. Currently, the response sends empty configurations without peer-specific config data.Would you like me to help implement the
ToPeerConfigcall here, or open an issue to track this task?management/server/activity/codes.go (1)
183-184: LGTM!The new activity code
AccountAutoUpdateVersionUpdated(value 91) is properly added and follows the established pattern. The mapping entry correctly includes both the human-readable message and the machine-readable code.Minor observation: The message uses "AutoUpdate" (CamelCase) while other messages use space-separated words (e.g., "Account peer login expiration enabled"). Consider "Account auto-update version updated" for consistency, though this is optional.
Also applies to: 291-295
client/internal/updatemanager/installer/installer.go (1)
10-12: Unused constant and inconsistent receiver naming.
- The
updaterBinaryconstant is defined but never used in this file.- The receiver name is
con line 34 butueverywhere else.-const ( - updaterBinary = "updater" -)-func (c *Installer) LogFiles() []string { +func (u *Installer) LogFiles() []string { return []string{} }Also applies to: 34-36
version/update.go (1)
110-114: Potential race condition inStartFetcherguard.The nil check and assignment of
fetchTicker(lines 111-114) are not atomic. IfStartFetcheris called concurrently from multiple goroutines, both could pass the nil check before either assignsfetchTicker, resulting in multiple tickers and goroutines.While current usage via
NewUpdateAndStartappears to call this only once, consider adding synchronization for robustness.func (u *Update) StartFetcher() { + u.versionsLock.Lock() if u.fetchTicker != nil { + u.versionsLock.Unlock() return } u.fetchTicker = time.NewTicker(fetchPeriod) + u.versionsLock.Unlock() if changed := u.fetchVersion(); changed {Alternatively, use
sync.Oncefor cleaner initialization semantics.client/internal/updatemanager/downloader/downloader.go (2)
38-40: Consider using an HTTP client with timeout.
http.DefaultClienthas no timeout, which can cause the download to hang indefinitely if the server is slow or unresponsive. This is especially problematic for an auto-update flow that may block user interaction.Consider creating a dedicated client with appropriate timeouts:
+var httpClient = &http.Client{ + Timeout: 5 * time.Minute, +} + func DownloadToFile(ctx context.Context, url, dstFile string) error { ... - resp, err := http.DefaultClient.Do(req) + resp, err := httpClient.Do(req)
20-28: Orphan file left on download failure.The destination file is created before the HTTP request succeeds. If the request fails or returns a non-200 status, the empty or partial file remains on disk. Consider creating a temporary file and renaming it on success, or deferring file creation until after status validation.
func DownloadToFile(ctx context.Context, url, dstFile string) error { log.Debugf("starting download from %s", url) - out, err := os.Create(dstFile) - if err != nil { - return fmt.Errorf("failed to create destination file %q: %w", dstFile, err) - } - defer func() { - if cerr := out.Close(); cerr != nil { - log.Warnf("error closing file %q: %v", dstFile, cerr) - } - }() - req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil) if err != nil { return fmt.Errorf("failed to create HTTP request: %w", err) @@ ... validate status first, then create file ...Also applies to: 48-54
client/internal/updatemanager/installer/installer_common.go (1)
206-211: Directory permission 0o755 may be too permissive for sensitive update artifacts.The temp directory is created with world-readable/executable permissions. For security-sensitive update operations, consider using 0o700 to restrict access to the owner only.
func (u *Installer) mkTempDir() error { - if err := os.MkdirAll(u.tempDir, 0o755); err != nil { + if err := os.MkdirAll(u.tempDir, 0o700); err != nil { log.Debugf("failed to create tempdir: %s", u.tempDir) return err }client/internal/updatemanager/reposign/key.go (1)
158-171: SimplifyverifyAnyby eliminating intermediate slice.The function creates
rootKeysslice unnecessarily. You can iterate directly overpublicRootKeys.func verifyAny(publicRootKeys []PublicKey, msg, sig []byte) bool { - // Verify with root keys - var rootKeys []ed25519.PublicKey - for _, r := range publicRootKeys { - rootKeys = append(rootKeys, r.Key) - } - - for _, k := range rootKeys { - if ed25519.Verify(k, msg, sig) { + for _, pk := range publicRootKeys { + if ed25519.Verify(pk.Key, msg, sig) { return true } } return false }client/cmd/root.go (1)
86-92: Consider guarding against a nilupdateCmdfor future safetyRight now build‑tagged files ensure
isUpdateBinary()is false whereupdateCmdis nil, but a future change could flip this and cause a panic. A small defensive check could make this more robust:func Execute() error { - if isUpdateBinary() { - return updateCmd.Execute() - } + if isUpdateBinary() { + if updateCmd == nil { + return fmt.Errorf("update binary detected but update command is not configured") + } + return updateCmd.Execute() + } return rootCmd.Execute() }management/server/types/settings.go (1)
56-58: Auto‑update version field and copy semantics look consistentAdding
AutoUpdateVersiontoSettingsand propagating it inCopy()keeps the new setting stable across copies; no structural issues detected.If you expect
"latest"to be the effective default at the Go level as well (not only at the DB schema level), consider initializing it in code paths that constructSettingsso it’s never an empty string in memory.Also applies to: 78-79
shared/management/http/api/openapi.yml (1)
148-151: Consider adding validation for auto_update_version values.The field accepts three types of values ("latest", "disabled", or version strings like "0.51.2"), but there's no schema-level validation to enforce this. Consider:
- Adding an enum for the special values if they should be validated at the API layer, or
- Documenting that validation happens server-side, or
- Adding a pattern to validate version string format
This would improve API clarity and help catch invalid inputs earlier.
client/internal/debug/debug.go (1)
638-659: Consider adjusting log level for routine operations.Line 645 uses
log.Infof("adding updater logs")which may be too verbose for routine debug bundle generation. Consider usinglog.Debugfinstead to reduce noise in standard operation.Otherwise, the implementation looks good with appropriate error handling that skips unreadable log files while still including available logs.
- log.Infof("adding updater logs") + log.Debugf("adding updater logs")management/internals/shared/grpc/conversion.go (2)
126-126: Redundant assignment ofNetworkMap.PeerConfig.
NetworkMap.PeerConfigis already assigned on line 117 during struct initialization. This line overwrites it withresponse.PeerConfig. If the intent is to use the same value, consider removing either line 117's assignment or this line to avoid confusion about which value is actually used.If the intent is to always use the top-level PeerConfig (with full arguments), remove the inline assignment:
response := &proto.SyncResponse{ PeerConfig: toPeerConfig(peer, networkMap.Network, dnsName, settings, httpConfig, deviceFlowConfig), NetworkMap: &proto.NetworkMap{ Serial: networkMap.Network.CurrentSerial(), Routes: toProtocolRoutes(networkMap.Routes), DNSConfig: toProtocolDNSConfig(networkMap.DNSConfig, dnsCache, dnsFwdPort), - PeerConfig: toPeerConfig(peer, networkMap.Network, dnsName, settings), },
104-107: Based on my verification, I can now provide the rewritten review comment:
Consider whether
AlwaysUpdateshould be made configurable alongsideVersion.The
AutoUpdateSettingsproto message has two fields:VersionandAlwaysUpdate. Currently, onlyVersionis populated from account settings (viaAutoUpdateVersion). TheAlwaysUpdatefield controls whether auto-update checks run continuously or only on initial connection (client/internal/engine.go:754). If this should be user-configurable, it would require adding the field to theSettingsstruct and exposing it through the API.client/internal/updatemanager/reposign/doc.go (1)
76-77: Minor documentation clarification needed for build tags.The documentation states:
- Production builds:
//go:build !devartifactsign- Development builds:
//go:build devartifactsignThis is technically correct (production uses the negation), but consider rephrasing for clarity:
- Default (production) builds use
certs/(no special build tag needed)- Development builds require the
devartifactsigntag to usecertsdev/client/internal/updatemanager/reposign/verify_test.go (2)
189-206: Clarify "missing artifact" test by aligning signature path with artifact nameRight now the test signs arbitrary
testDataand serves it under/.../missing.bin.sigwhile callingVerifywith"file.bin". The test still passes because the local file is missing, but the mismatch between"missing.bin"and"file.bin"plus the comment “Create signature for non-existent file” is a bit confusing.Consider updating the served path/comment to match the artifact filename (e.g.
file.bin.sig) so the test intention is clearer and closer to the real URL layout.
133-165: Reduce coupling to exact error message strings in testsSeveral tests assert on substrings of
err.Error()(e.g."failed to load revocation list","no signing Key found","failed to validate artifact"). This makes the tests brittle to harmless wording changes in error messages.If/when you introduce structured/typed errors for the verifier, it would be more robust to assert using
errors.Is/errors.Asor exported sentinel values rather than string contents. For now this is acceptable, but worth keeping in mind for future refactors.Also applies to: 277-349, 424-486
client/internal/engine_test.go (1)
239-257:NewEnginecall sites correctly updated; ensurestateManagercan be nil in testsAll updated invocations of
NewEnginenow pass the extra trailing argument asnilafter the existingchecksargument, which matches the new signature and keeps the tests focused on their original concerns.Two minor points to keep in mind:
- Engine methods exercised by these tests should defensively handle a nil
stateManagerto avoid panics when auto‑update/state features are wired in.- The existing pattern in
createEngine(e, err := NewEngine(...), nil) is unusual but functionally fine givenNewEnginedoesn’t return an error. If you touch this again, consider simplifying it to a single return value or removing the unusederrfor readability.Also applies to: 418-424, 637-644, 802-809, 1003-1011, 1536-1538
management/server/http/handlers/accounts/accounts_handler_test.go (1)
96-175: Account settings tests correctly exerciseauto_update_versiondefaults and updatesExtending the table tests to:
- Expect
AutoUpdateVersion: sr("")for existing accounts and PUTs that don’t mentionauto_update_version, and- Verify
"latest"is echoed back when explicitly set,gives good confidence that the handler correctly maps between internal
types.Settingsand the public API model.Just keep in mind that, with
omitemptyon a*string, these expectations effectively codify the contract that the API returns an empty string rather than omitting the field when auto‑update is “not configured”. If you later decide that “unset” vs""should be distinguishable, both the handler and these tests will need to be adjusted.client/cmd/update_supported.go (2)
51-63: Consider using the command's context instead ofcontext.Background().The
cmdparameter provides a context viacmd.Context()that respects cancellation signals. Usingcontext.Background()ignores any cancellation from the CLI framework.func updateFunc(cmd *cobra.Command, args []string) error { if err := setupLogToFile(tempDirFlag); err != nil { return err } log.Infof("updater started: %s", serviceDirFlag) updater := installer.NewWithDir(tempDirFlag) - if err := updater.Setup(context.Background(), dryRunFlag, installerFile, serviceDirFlag); err != nil { + if err := updater.Setup(cmd.Context(), dryRunFlag, installerFile, serviceDirFlag); err != nil { log.Errorf("failed to update application: %v", err) return err } return nil }
65-75: Log file removal error is silently swallowed.When
os.Removefails at line 69-71, the error is only logged but execution continues. If the old log file can't be removed (e.g., permission issues), the subsequentutil.InitLogcall may still fail or produce unexpected behavior when appending to a locked file.Consider returning an error if removal fails for reasons other than the file not existing:
func setupLogToFile(dir string) error { logFile := filepath.Join(dir, installer.LogFile) if _, err := os.Stat(logFile); err == nil { if err := os.Remove(logFile); err != nil { - log.Errorf("failed to remove existing log file: %v\n", err) + return fmt.Errorf("failed to remove existing log file: %w", err) } } return util.InitLog(logLevel, util.LogConsole, logFile) }management/server/account.go (1)
459-466: Inconsistent indentation detected.The function body uses spaces instead of tabs, which is inconsistent with the rest of the file. This may cause issues with Go formatting tools.
func (am *DefaultAccountManager) handleAutoUpdateVersionSettings(ctx context.Context, oldSettings, newSettings *types.Settings, userID, accountID string) { - if oldSettings.AutoUpdateVersion != newSettings.AutoUpdateVersion { - am.StoreEvent(ctx, userID, accountID, accountID, activity.AccountAutoUpdateVersionUpdated, map[string]any{ - "version": newSettings.AutoUpdateVersion, - }) - } + if oldSettings.AutoUpdateVersion != newSettings.AutoUpdateVersion { + am.StoreEvent(ctx, userID, accountID, accountID, activity.AccountAutoUpdateVersionUpdated, map[string]any{ + "version": newSettings.AutoUpdateVersion, + }) + } }client/internal/updatemanager/manager_test.go (2)
84-92: Timing-based assertions may cause flaky tests.The 10ms timeout used in
selectstatements could be insufficient on slow CI systems or under load, leading to intermittent test failures. Consider increasing the timeout or using synchronization primitives for more deterministic testing.select { case targetVersion := <-targetVersionChan: if targetVersion != c.initialLatestVersion.String() { t.Errorf("%s: Initial update version mismatch, expected %v, got %v", c.name, c.initialLatestVersion.String(), targetVersion) } triggeredInit = true - case <-time.After(10 * time.Millisecond): + case <-time.After(100 * time.Millisecond): triggeredInit = false }Also applies to: 100-109
26-30: Mock'sSetDaemonVersionalways returns false.The mock unconditionally returns
falsefromSetDaemonVersion, which means tests won't exercise the code path where the daemon version is successfully set. Consider making this configurable if that path needs testing.client/internal/updatemanager/reposign/root_test.go (1)
343-370: Potential test flakiness with 10ms sleep.The 10ms sleep may not be sufficient on slow CI runners or heavily loaded systems, potentially causing flaky tests if timestamps are compared at second granularity.
Consider increasing the delay or adjusting the assertion:
// Sign twice with a small delay sig1, err := SignArtifactKey(*rootKey, data) require.NoError(t, err) - time.Sleep(10 * time.Millisecond) + time.Sleep(1100 * time.Millisecond) // Ensure timestamp difference at second granularity sig2, err := SignArtifactKey(*rootKey, data) require.NoError(t, err)Alternatively, if the signature timestamps are sub-second precision, the current delay may be fine.
client/cmd/signer/artifactkey.go (2)
42-57: Consider addingSilenceUsage: truefor consistency.The
createArtifactKeyCmdhasSilenceUsage: truebutbundlePubKeysCmddoes not. This inconsistency means errors inbundle-pub-keyswill print usage whilecreate-artifact-keywon't.var bundlePubKeysCmd = &cobra.Command{ Use: "bundle-pub-keys", Short: "Bundle multiple artifact public keys into a signed package", Long: `Bundle one or more artifact public keys into a signed package using the root private key. This command is typically used to distribute or authorize a set of valid artifact signing keys.`, + SilenceUsage: true, RunE: func(cmd *cobra.Command, args []string) error {
146-158: Add file path context to error messages in the loop.When iterating over multiple public key files, the error messages at lines 150 and 155 don't include which file caused the failure, making debugging difficult.
for _, pubFile := range artifactPubKeyFiles { pubPem, err := os.ReadFile(pubFile) if err != nil { - return fmt.Errorf("read public key file: %w", err) + return fmt.Errorf("read public key file (%s): %w", pubFile, err) } pk, err := reposign.ParseArtifactPubKey(pubPem) if err != nil { - return fmt.Errorf("failed to parse artifact key: %w", err) + return fmt.Errorf("failed to parse artifact key (%s): %w", pubFile, err) } publicKeys = append(publicKeys, pk) }client/internal/updatemanager/installer/types_windows.go (1)
42-51: Minor optimization: callToLoweronce.
strings.ToLower(filePath)is called twice for each check. Consider storing it in a variable.func typeByFileExtension(filePath string) (Type, error) { + lowerPath := strings.ToLower(filePath) switch { - case strings.HasSuffix(strings.ToLower(filePath), ".exe"): + case strings.HasSuffix(lowerPath, ".exe"): return TypeExe, nil - case strings.HasSuffix(strings.ToLower(filePath), ".msi"): + case strings.HasSuffix(lowerPath, ".msi"): return TypeMSI, nil default: return Type{}, fmt.Errorf("unsupported installer type for file: %s", filePath) } }client/cmd/signer/artifactsign.go (1)
269-276: Function name is misleading — it parses a single key, not multiple.The function
parseRootPublicKeys(plural) only parses a single root public key and wraps it in a slice. This is inconsistent with the naming and doesn't support bundled root keys like the artifact key verification does.Consider either:
- Rename to
parseRootPublicKeyand update calling code, or- Implement actual bundle parsing similar to how artifact keys support bundles:
-// parseRootPublicKeys parses a root public key from PEM data -func parseRootPublicKeys(data []byte) ([]reposign.PublicKey, error) { - key, err := reposign.ParseRootPublicKey(data) - if err != nil { - return nil, err - } - return []reposign.PublicKey{key}, nil +// parseRootPublicKey parses a root public key from PEM data +func parseRootPublicKey(data []byte) ([]reposign.PublicKey, error) { + key, err := reposign.ParseRootPublicKey(data) + if err != nil { + return nil, err + } + return []reposign.PublicKey{key}, nil }client/internal/updatemanager/reposign/revocation_test.go (1)
46-71: Test missingexpires_atfield validation.The test JSON at Lines 54-59 omits the
expires_atfield. While this test passes because it only testsjson.Unmarshal(notParseRevocationList), the test nameTestRevocationList_UnmarshalJSONcould be misleading since a complete unmarshal with validation would fail due to missingexpires_at.Consider adding
expires_atto the test data for consistency with the full struct requirements:jsonData := map[string]interface{}{ "revoked": map[string]string{ keyID.String(): revokedTime.Format(time.RFC3339), }, "last_updated": lastUpdated.Format(time.RFC3339), + "expires_at": lastUpdated.Add(90 * 24 * time.Hour).Format(time.RFC3339), }client/cmd/signer/revocation.go (1)
212-220: Consider atomic writes to prevent partial file corruption.If writing the signature file fails after successfully writing the revocation list, the system is left in an inconsistent state with an updated list but stale signature.
Consider writing to temporary files first, then atomically renaming:
func writeOutputFiles(rlPath, sigPath string, rlBytes, sigBytes []byte) error { - if err := os.WriteFile(rlPath, rlBytes, 0o600); err != nil { - return fmt.Errorf("failed to write revocation list file: %w", err) + rlTmp := rlPath + ".tmp" + sigTmp := sigPath + ".tmp" + + if err := os.WriteFile(rlTmp, rlBytes, 0o600); err != nil { + return fmt.Errorf("failed to write revocation list file: %w", err) + } + if err := os.WriteFile(sigTmp, sigBytes, 0o600); err != nil { + os.Remove(rlTmp) + return fmt.Errorf("failed to write signature file: %w", err) } - if err := os.WriteFile(sigPath, sigBytes, 0o600); err != nil { - return fmt.Errorf("failed to write signature file: %w", err) + if err := os.Rename(rlTmp, rlPath); err != nil { + os.Remove(rlTmp) + os.Remove(sigTmp) + return fmt.Errorf("failed to rename revocation list file: %w", err) + } + if err := os.Rename(sigTmp, sigPath); err != nil { + return fmt.Errorf("failed to rename signature file: %w", err) } return nil }client/internal/updatemanager/installer/installer_run_windows.go (2)
173-173: Document magic numbers for Windows creation flags.The creation flags use raw hex values which are not immediately clear. Consider extracting these to named constants for clarity.
+const ( + createNewProcessGroup = 0x00000200 + detachedProcess = 0x00000008 + createUnicodeEnvironment = 0x00000400 +) + func (u *Installer) startUIAsUser(daemonFolder string) error { // ... - creationFlags := uint32(0x00000200 | 0x00000008 | 0x00000400) // CREATE_NEW_PROCESS_GROUP | DETACHED_PROCESS | CREATE_UNICODE_ENVIRONMENT + creationFlags := uint32(createNewProcessGroup | detachedProcess | createUnicodeEnvironment)
106-118: Consider extracting the timeout constant.The 15-second timeout is also used in the Darwin implementation. Consider defining it as a package-level constant for consistency.
+const daemonStartTimeout = 15 * time.Second + func (u *Installer) startDaemon(daemonFolder string) error { log.Infof("starting netbird service") - ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second) + ctx, cancel := context.WithTimeout(context.Background(), daemonStartTimeout) defer cancel()client/internal/updatemanager/installer/installer_run_darwin.go (1)
76-81: Consider extracting the dry-run sleep duration as a constant.The 7-second sleep is a magic number. If intentional (e.g., simulating installation duration), document the purpose and extract to a constant.
+const dryRunSimulationDuration = 7 * time.Second + if dryRun { - time.Sleep(7 * time.Second) + time.Sleep(dryRunSimulationDuration) log.Infof("dry-run mode enabled, skipping actual installation")client/internal/updatemanager/reposign/revocation.go (1)
39-65: UnusedVersionfield parsed in UnmarshalJSON.The
Versionfield is parsed at line 44 but never used or stored. If this is for future compatibility, consider adding a comment. If versions should be validated or stored, implement that logic.var temp struct { Revoked map[string]time.Time `json:"revoked"` LastUpdated time.Time `json:"last_updated"` ExpiresAt time.Time `json:"expires_at"` - Version int `json:"version"` + Version int `json:"version"` // Reserved for future use } + + // Note: Version field is parsed but currently unused. + // Future versions may use this for format evolution.client/internal/connect.go (1)
61-61: Typo in parameter name:doInitalAutoUpdateshould bedoInitialAutoUpdate.The parameter name has a typo ("Inital" instead of "Initial"). While the internal field is correctly named
doInitialAutoUpdate, the parameter name inconsistency can cause confusion.func NewConnectClient( ctx context.Context, config *profilemanager.Config, statusRecorder *peer.Status, - doInitalAutoUpdate bool, + doInitialAutoUpdate bool, ) *ConnectClient { return &ConnectClient{ ctx: ctx, config: config, statusRecorder: statusRecorder, - doInitialAutoUpdate: doInitalAutoUpdate, + doInitialAutoUpdate: doInitialAutoUpdate, engineMutex: sync.Mutex{}, } }client/internal/updatemanager/manager.go (4)
348-348: Magic number for throttle duration should be a named constant.The 5-minute throttle duration is embedded inline. Consider extracting it as a named constant for clarity and maintainability.
const ( latestVersion = "latest" // this version will be ignored developmentVersion = "development" + // updateThrottleDuration is the minimum time between update attempts + updateThrottleDuration = 5 * time.Minute ) // In shouldUpdate: - if time.Since(m.lastTrigger) < 5*time.Minute { + if time.Since(m.lastTrigger) < updateThrottleDuration {
252-257: Remove or address TODO comment about context timeout.The commented-out code block with the TODO suggests incomplete implementation. Either implement the timeout or remove the commented code to avoid confusion.
If a timeout is needed, implement it. Otherwise, remove the commented block:
- // todo: review the usage of this context - /* - ctx, cancel := context.WithTimeout(ctx, time.Minute) - defer cancel() - - */
356-360: lastResultErrReason creates new Installer instance just to get temp directory.This creates a new Installer instance solely to access its temp directory path. Consider passing the temp directory as a configuration or caching the installer reference.
One option is to store the temp directory path in the Manager:
type Manager struct { // ... installerTempDir string } func NewManager(...) *Manager { inst := installer.New() return &Manager{ // ... installerTempDir: inst.TempDir(), } } func (m *Manager) lastResultErrReason() string { result := installer.NewResultHandler(m.installerTempDir) return result.GetErrorResultReason() }
261-267: Consider using INFO level for the initial auto-update event.Using
CRITICALfor the "Automatically updating client" event may be too severe. Consider usingINFOorWARNINGsince this is expected behavior, not an error condition.m.statusRecorder.PublishEvent( - cProto.SystemEvent_CRITICAL, + cProto.SystemEvent_INFO, cProto.SystemEvent_SYSTEM, "Automatically updating client", "Your client version is older than auto-update version set in Management, updating client now.", nil, )client/internal/updatemanager/reposign/artifact.go (3)
31-38: Panic onNewArtifactHasherror is acceptable but could be made explicit
blake2s.New256(nil)should not fail in normal conditions, so panicking is defensible. If you want to be extra explicit for future maintainers, consider clarifying this in the comment or wrapping in a package‑levelinit()that would fail fast at startup rather than at first call site.
221-265: Consider validatingAlgorithmandHashAlgoinValidateArtifact
ValidateArtifactreconstructs the message and verifies it withed25519.Verify, which is correct for the current scheme. However, it ignoressignature.Algorithmandsignature.HashAlgofields from theSignaturestruct.Given
SignDatasetsAlgorithm: "ed25519"andHashAlgo: "blake2s", a quick sanity check would:
- Catch malformed or unexpected signatures early.
- Make future multi‑algorithm support clearer and safer.
Example:
func ValidateArtifact(artifactPubKeys []PublicKey, data []byte, signature Signature) error { + if signature.Algorithm != "ed25519" { + return fmt.Errorf("unsupported signature algorithm: %s", signature.Algorithm) + } + if signature.HashAlgo != "blake2s" { + return fmt.Errorf("unsupported hash algorithm for artifacts: %s", signature.HashAlgo) + }You may want analogous checks for root key bundle validation if/when needed.
Please double‑check the intended set of allowed algorithms/hash algos for this release against your design doc before hard‑coding these strings.
267-301: TightenSignDataAPI surface (stale comment, optional precondition clarity)A couple of small polish points:
- The comment
// Check happens too late(Line 268) is now inaccurate since the length check is the first thing executed. It’s misleading and can be removed.- If zero‑length data is always invalid by design, the current length check is fine. If that’s a soft constraint, consider documenting it in the function comment instead of returning an error directly.
Suggested minimal cleanup:
func SignData(artifactKey ArtifactKey, data []byte) ([]byte, error) { - if len(data) == 0 { // Check happens too late + if len(data) == 0 { return nil, fmt.Errorf("artifact length must be positive, got %d", len(data)) }Otherwise the construction of the signed message (
hash || length || timestamp) and the timestamp/expiry checks look consistent withValidateArtifact.
| currentPID := os.Getpid() | ||
|
|
||
| // Restart netbird service after the fact | ||
| // This is a workaround since attempting to restart using launchctl will kill the service and die before starting | ||
| // the service again as it's a child process | ||
| // using SIGTERM should ensure a clean shutdown | ||
| process, err := os.FindProcess(currentPID) | ||
| if err != nil { | ||
| return fmt.Errorf("error finding current process: %w", err) | ||
| } | ||
| err = process.Signal(syscall.SIGTERM) | ||
| if err != nil { | ||
| return fmt.Errorf("error sending SIGTERM to current process: %w", err) | ||
| } | ||
| // We're dying now, which should restart us | ||
| log.Infof("homebrew updated successfully") | ||
| return nil |
There was a problem hiding this comment.
Result may not be written before self-termination in Homebrew path.
After brew upgrade, the process sends SIGTERM to itself (lines 228-231) and logs success (line 233). However, the deferred result handler runs after the function returns—but SIGTERM may terminate the process before the defer executes, leaving no result file.
Consider writing the result before sending the signal, or restructuring to ensure cleanup completes.
+ log.Infof("homebrew updated successfully, writing result before restart")
+ // Write success result before self-termination
+ if err := NewResultHandler(u.tempDir).WriteSuccess(); err != nil {
+ log.Errorf("failed to write update result before restart: %v", err)
+ }
+
currentPID := os.Getpid()
// ...
err = process.Signal(syscall.SIGTERM)Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In client/internal/updatemanager/installer/installer_run_darwin.go around lines
218-234, the code sends SIGTERM to the current process and logs success before
the deferred result handler can run, risking the result file not being written;
modify the flow to write and flush the result (ensure any result file is closed
and synced) before sending SIGTERM — i.e., call the existing
result-writing/cleanup logic (or move the log and return) so the result is
produced and flushed, then find the process and call
process.Signal(syscall.SIGTERM) as the last action; alternatively, if defers
must run, spawn a short-lived goroutine that sends SIGTERM after a small sleep
to allow deferred cleanup to complete.
| func (h *eventHandler) runSelfCommand(ctx context.Context, command string, args ...string) { | ||
| proc, err := os.Executable() | ||
| if err != nil { | ||
| log.Errorf("error getting executable path: %v", err) | ||
| return | ||
| } | ||
|
|
||
| cmd := exec.CommandContext(ctx, proc, | ||
| fmt.Sprintf("--%s=%s", command, arg), | ||
| // Build the full command arguments | ||
| cmdArgs := []string{ | ||
| fmt.Sprintf("--%s=true", command), | ||
| fmt.Sprintf("--daemon-addr=%s", h.client.addr), | ||
| ) | ||
| } | ||
| cmdArgs = append(cmdArgs, args...) | ||
|
|
||
| cmd := exec.CommandContext(ctx, proc, cmdArgs...) |
There was a problem hiding this comment.
Potential bug: Callers still pass "true" as an argument.
The function now internally constructs --<command>=true (line 249), but callers like handleAdvancedSettingsClick (line 188), handleCreateDebugBundleClick (line 196), and handleNetworksClick (line 220) still pass "true" as an additional argument. This results in commands like:
netbird-ui --settings=true --daemon-addr=<addr> true
The stray "true" at the end may cause unexpected behavior or be ignored.
Either remove the extra argument from callers:
func (h *eventHandler) handleAdvancedSettingsClick() {
h.client.mAdvancedSettings.Disable()
go func() {
defer h.client.mAdvancedSettings.Enable()
defer h.client.getSrvConfig()
- h.runSelfCommand(h.client.ctx, "settings", "true")
+ h.runSelfCommand(h.client.ctx, "settings")
}()
}Or revert to the old signature if the "true" was meant to be the flag value.
Committable suggestion skipped: line range outside the PR's diff.
| go func() { | ||
| conn, err := s.getSrvClient(defaultFailTimeout) | ||
| if err != nil { | ||
| log.Warnf("Failed to connect to server: %v", err) | ||
| return | ||
| } | ||
|
|
||
| resp, err := conn.GetInstallerResult(updateWindowCtx, &proto.InstallerResultRequest{}) | ||
| if err != nil { | ||
| log.Warnf("get update result RPC failed: %v", err) | ||
| return | ||
| } | ||
|
|
||
| if !resp.Success { | ||
| resultErrCh <- mapInstallError(resp.ErrorMsg) | ||
| return | ||
| } | ||
|
|
||
| // Success | ||
| close(resultOkCh) | ||
| }() |
There was a problem hiding this comment.
RPC errors should be reported to the error channel.
When getSrvClient or GetInstallerResult fails, the goroutine returns without signaling the error channel. This causes the UI to wait for the full 30-second timeout instead of showing an immediate error.
go func() {
conn, err := s.getSrvClient(defaultFailTimeout)
if err != nil {
log.Warnf("Failed to connect to server: %v", err)
+ resultErrCh <- fmt.Errorf("failed to connect to server: %w", err)
return
}
resp, err := conn.GetInstallerResult(updateWindowCtx, &proto.InstallerResultRequest{})
if err != nil {
log.Warnf("get update result RPC failed: %v", err)
+ resultErrCh <- fmt.Errorf("get update result failed: %w", err)
return
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| go func() { | |
| conn, err := s.getSrvClient(defaultFailTimeout) | |
| if err != nil { | |
| log.Warnf("Failed to connect to server: %v", err) | |
| return | |
| } | |
| resp, err := conn.GetInstallerResult(updateWindowCtx, &proto.InstallerResultRequest{}) | |
| if err != nil { | |
| log.Warnf("get update result RPC failed: %v", err) | |
| return | |
| } | |
| if !resp.Success { | |
| resultErrCh <- mapInstallError(resp.ErrorMsg) | |
| return | |
| } | |
| // Success | |
| close(resultOkCh) | |
| }() | |
| go func() { | |
| conn, err := s.getSrvClient(defaultFailTimeout) | |
| if err != nil { | |
| log.Warnf("Failed to connect to server: %v", err) | |
| resultErrCh <- fmt.Errorf("failed to connect to server: %w", err) | |
| return | |
| } | |
| resp, err := conn.GetInstallerResult(updateWindowCtx, &proto.InstallerResultRequest{}) | |
| if err != nil { | |
| log.Warnf("get update result RPC failed: %v", err) | |
| resultErrCh <- fmt.Errorf("get update result failed: %w", err) | |
| return | |
| } | |
| if !resp.Success { | |
| resultErrCh <- mapInstallError(resp.ErrorMsg) | |
| return | |
| } | |
| // Success | |
| close(resultOkCh) | |
| }() |
🤖 Prompt for AI Agents
In client/ui/update.go around lines 45 to 65 the goroutine returns on failures
from getSrvClient or GetInstallerResult without sending anything to resultErrCh
causing the UI to wait for the full timeout; send the error into resultErrCh
(using mapInstallError for RPC error messages where appropriate) before
returning, and use a non-blocking send or select with the updateWindowCtx to
avoid goroutine leaks or blocking if the receiver is gone.
| settings, err := h.updateAccountRequestSettings(req) | ||
| if err != nil { | ||
| util.WriteError(r.Context(), err, w) | ||
| } |
There was a problem hiding this comment.
Critical: Missing return after error causes nil pointer dereference.
When updateAccountRequestSettings returns an error, util.WriteError is called but execution continues. The settings variable will be nil, leading to a nil pointer dereference at line 262 (settings.NetworkRange = prefix).
settings, err := h.updateAccountRequestSettings(req)
if err != nil {
util.WriteError(r.Context(), err, w)
+ return
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| settings, err := h.updateAccountRequestSettings(req) | |
| if err != nil { | |
| util.WriteError(r.Context(), err, w) | |
| } | |
| settings, err := h.updateAccountRequestSettings(req) | |
| if err != nil { | |
| util.WriteError(r.Context(), err, w) | |
| return | |
| } |
🤖 Prompt for AI Agents
In management/server/http/handlers/accounts/accounts_handler.go around lines 248
to 251, after calling util.WriteError when updateAccountRequestSettings returns
an error you must stop execution to avoid using the nil settings pointer; add an
immediate return (exit the handler) right after util.WriteError(r.Context(),
err, w) so the function does not continue and dereference settings.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
client/internal/updatemanager/installer/installer_run_darwin.go (1)
32-36: Homebrew SIGTERM prevents deferred result writing and restart logic from running.
Setuprelies on adeferto write the installerResultand (for non–dry-run) restart the daemon and UI. In the Homebrew path,updateHomeBrewcallsos.FindProcess(os.Getpid())and sendsSIGTERMto the current process afterbrew upgradesucceeds:currentPID := os.Getpid() process, err := os.FindProcess(currentPID) ... err = process.Signal(syscall.SIGTERM)Because the default action for
SIGTERMis process termination and no custom handler is installed, the process is very likely to exit beforeSetupcan return and execute its deferred cleanup. That means:
- The JSON result file may never be written for successful Homebrew upgrades.
- Any restart logic in the deferred block (daemon/UI handling) will not run for this path.
This risks leaving the manager waiting indefinitely for a result and makes Homebrew behavior inconsistent with the PKG path.
Consider restructuring so that the result is written (and any necessary cleanup is performed) before sending
SIGTERM. Options include:
- Factor the deferred body into a helper (e.g.,
finalizeSetup(...)) and call it explicitly in the Homebrew path before signaling the process.- Alternatively, write the success result inside
updateHomeBrewjust before sendingSIGTERM, ensuring the result file is flushed, and keep the deferred handler as-is for non-Homebrew paths.Also applies to: 37-61, 83-91, 166-227
🧹 Nitpick comments (1)
client/internal/updatemanager/installer/installer_run_darwin.go (1)
190-202: Preserve existing environment when overridingHOMEfor Homebrew upgrade.In
updateHomeBrew, the environment for thesudocommand is constructed as:cmd = exec.CommandContext(ctx, "sudo", upgradeArgs...) cmd.Env = append(cmd.Env, "HOME="+homeDir)Since
cmd.Envis initiallynil, this results in an environment containing onlyHOME, droppingPATH, locale, and other inherited variables. While this may work due to the absolute brew path, it’s brittle and inconsistent withstartUIAsUser, which usesappend(os.Environ(), ...).Recommend preserving the base environment and then overriding
HOME:- cmd = exec.CommandContext(ctx, "sudo", upgradeArgs...) - cmd.Env = append(cmd.Env, "HOME="+homeDir) + cmd = exec.CommandContext(ctx, "sudo", upgradeArgs...) + cmd.Env = append(os.Environ(), "HOME="+homeDir)This keeps behavior predictable and aligns with the pattern you already use in
startUIAsUser.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
client/internal/updatemanager/installer/installer_run_darwin.go(1 hunks)client/internal/updatemanager/installer/types_darwin.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- client/internal/updatemanager/installer/types_darwin.go
🧰 Additional context used
🧬 Code graph analysis (1)
client/internal/updatemanager/installer/installer_run_darwin.go (5)
client/internal/updatemanager/installer/installer_common.go (1)
Installer(23-25)client/internal/updatemanager/installer/installer.go (1)
Installer(14-16)client/internal/updatemanager/installer/result.go (2)
NewResultHandler(33-42)Result(20-24)client/internal/updatemanager/installer/types_darwin.go (2)
TypePKG(10-10)TypeHomebrew(9-9)client/internal/updatemanager/installer/types.go (1)
Type(3-6)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: Client / Unit
- GitHub Check: Android / Build
- GitHub Check: iOS / Build
- GitHub Check: release_ui_darwin
- GitHub Check: Client / Unit
- GitHub Check: release
- GitHub Check: release_ui
- GitHub Check: Windows
- GitHub Check: Client / Unit
- GitHub Check: Darwin
- GitHub Check: Linux
- GitHub Check: JS / Lint
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
client/internal/updatemanager/installer/installer_run_darwin.go (1)
158-220: Homebrew SIGTERM likely prevents result writing and restart logic; also tightly couples to/opt/homebrewpathThe Homebrew path has two intertwined concerns:
SIGTERM vs deferred result writing & restart (major, previously raised).
Aftercmd.Run()completes,updateHomeBrewfinds the current process and sends itselfSIGTERM(Lines 203–219). Because this happens beforeSetupreturns, the deferred block inSetup(Lines 37–66) that writes the update result and restarts daemon/UI will never run: a terminating signal bypasses normal Go defers. This matches the earlier bot review concern about results not being written before self‑termination and, in this design, also skips the standardized restart path.A simpler, safer approach here would be:
- Let
updateHomeBrewfocus purely on runningbrew upgradeand thenreturn nilon success.- Rely on
Setup’s deferredstartDaemon/startUIAsUserfor restart behavior, just like the PKG path, instead of self‑SIGTERM.Concretely, you can drop the self‑SIGTERM block and obsolete comments:
err = cmd.Run() if err != nil { return fmt.Errorf("error running brew upgrade: %w", err) }
- currentPID := os.Getpid()
- // Restart netbird service after the fact
- // This is a workaround since attempting to restart using launchctl will kill the service and die before starting
- // the service again as it's a child process
- // using SIGTERM should ensure a clean shutdown
- process, err := os.FindProcess(currentPID)
- if err != nil {
return fmt.Errorf("error finding current process: %w", err)- }
- err = process.Signal(syscall.SIGTERM)
- if err != nil {
return fmt.Errorf("error sending SIGTERM to current process: %w", err)- }
- // We're dying now, which should restart us
- log.Infof("homebrew updated successfully")
- return nil
- log.Infof("homebrew updated successfully")
- return nil
This keeps the result‑file contract and restart path unified across installer types. 2. **Hard‑coded `/opt/homebrew` paths reduce compatibility.** Both the tap path and brew binary path are hard‑coded to `/opt/homebrew/...` (Lines 163, 184), which works on Apple Silicon but will fail on Intel installs that still use `/usr/local/Homebrew` and `/usr/local/bin/brew`. Consider either: - Detecting the Homebrew prefix dynamically (e.g., via `brew --prefix` or probing both common roots), or - Making the prefix configurable (env/flag) with a sensible default. That would avoid surprising failures for non‑`/opt/homebrew` setups. </blockquote></details> </blockquote></details> <details> <summary>🧹 Nitpick comments (6)</summary><blockquote> <details> <summary>client/internal/updatemanager/installer/installer_run_darwin.go (1)</summary><blockquote> `142-156`: **Consider capturing PKG installer output for easier debugging** Right now `installPkgFile` only returns `fmt.Errorf("error running pkg file: %w", err)` without preserving installer output. That can make field debugging painful when `installer` fails. You could attach stdout/stderr to logs (or a temp file) while still using `Start`/`Wait`, e.g., by wiring `cmd.Stdout`/`cmd.Stderr` to `logrus` hooks or a buffer you log on error. </blockquote></details> <details> <summary>client/internal/updatemanager/manager_unsupported.go (1)</summary><blockquote> `13-19`: **Doc comment contradicts implementation for unsupported platforms** `NewManager`’s comment says it “returns a no-op manager”, but it actually returns `nil` with an error. Either adjust the comment or return a stub instance without error so behavior matches the documentation. </blockquote></details> <details> <summary>client/internal/engine.go (2)</summary><blockquote> `75-76`: **Clarify the `disableAutoUpdate` sentinel meaning** Consider expanding the comment around `disableAutoUpdate = "disabled"` to explicitly tie it to the server‑side AutoUpdateSettings semantics (i.e. that setting `version="disabled"` from management fully disables client auto‑update), so future readers don’t treat it as just another version string. --- `226-251`: **Constructor now requires a non-nil stateManager** `NewEngine` now takes a `*statemanager.Manager` and stores it without nil checks; `Stop()` unconditionally calls `e.stateManager.Stop`. This effectively makes `stateManager` a required dependency. That’s fine, but it would be good to document the expectation that callers must never pass `nil`, otherwise `Stop()` will panic. </blockquote></details> <details> <summary>client/internal/connect.go (1)</summary><blockquote> `45-70`: **ConnectClient wiring changes look good; minor naming nit** Storing `ctx`, `config`, `statusRecorder`, and `doInitialAutoUpdate` on `ConnectClient` and guarding engine access with `engineMutex` is consistent with the existing design. One small nit: the parameter name `doInitalAutoUpdate` is misspelled; if you touch this again, consider correcting it to `doInitialAutoUpdate` for consistency with the field. </blockquote></details> <details> <summary>client/internal/updatemanager/manager.go (1)</summary><blockquote> `75-110`: **CheckUpdateSuccess works, but deletion-before-notify has a small reliability risk** Functionally this flow makes sense: - Read the last installer error reason and emit an “Auto-update failed” event if present. - Load the persisted `UpdateState`. - If the persisted `TargetVersion` matches the current binary version, emit an “Auto-update completed” event. However, `loadAndDeleteUpdateState` deletes the state and persists that deletion before you publish the success event. If the client crashes between deletion and event publication, the update will be considered “handled” (state gone) but the success event will never be emitted on the next start. If you want more robust behavior, consider: - Either publishing the success event first and then deleting the state, or - Storing an explicit “completed” flag in the state and only deleting it after a successful notification path. Given this is a post‑update UX concern rather than a functional bug, it’s more of a reliability polish than a blocker. </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: CodeRabbit UI **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 3be2d633dd8efbcda36690b7be9be07ac90e7eaa and 0ef5765077ed62f6cd397321823a66a37b996677. </details> <details> <summary>⛔ Files ignored due to path filters (2)</summary> * `client/proto/daemon.pb.go` is excluded by `!**/*.pb.go` * `shared/management/proto/management.pb.go` is excluded by `!**/*.pb.go` </details> <details> <summary>📒 Files selected for processing (7)</summary> * `client/internal/connect.go` (6 hunks) * `client/internal/engine.go` (8 hunks) * `client/internal/updatemanager/installer/installer.go` (1 hunks) * `client/internal/updatemanager/installer/installer_run_darwin.go` (1 hunks) * `client/internal/updatemanager/manager.go` (1 hunks) * `client/internal/updatemanager/manager_unsupported.go` (1 hunks) * `client/internal/updatemanager/update.go` (1 hunks) </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (1)</summary> * client/internal/updatemanager/installer/installer.go </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🧬 Code graph analysis (3)</summary> <details> <summary>client/internal/engine.go (3)</summary><blockquote> <details> <summary>client/internal/updatemanager/manager.go (2)</summary> * `Manager` (39-59) * `NewManager` (61-73) </details> <details> <summary>client/internal/statemanager/manager.go (1)</summary> * `Manager` (53-65) </details> <details> <summary>shared/management/proto/management.pb.go (6)</summary> * `AutoUpdateSettings` (1857-1866) * `AutoUpdateSettings` (1881-1881) * `AutoUpdateSettings` (1896-1898) * `PeerConfig` (1749-1767) * `PeerConfig` (1782-1782) * `PeerConfig` (1797-1799) </details> </blockquote></details> <details> <summary>client/internal/updatemanager/manager_unsupported.go (1)</summary><blockquote> <details> <summary>shared/management/status/error.go (1)</summary> * `Errorf` (70-75) </details> </blockquote></details> <details> <summary>client/internal/connect.go (4)</summary><blockquote> <details> <summary>client/internal/engine.go (2)</summary> * `Engine` (138-214) * `NewEngine` (227-251) </details> <details> <summary>client/internal/updatemanager/installer/installer.go (1)</summary> * `New` (19-21) </details> <details> <summary>client/internal/statemanager/manager.go (1)</summary> * `New` (68-75) </details> <details> <summary>client/internal/updatemanager/manager.go (1)</summary> * `NewManager` (61-73) </details> </blockquote></details> </details> </details> <details> <summary>⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (24)</summary> * GitHub Check: Management / Benchmark (amd64, postgres) * GitHub Check: Management / Benchmark (amd64, sqlite) * GitHub Check: Management / Integration (amd64, sqlite) * GitHub Check: Signal / Unit (386) * GitHub Check: Management / Unit (amd64, mysql) * GitHub Check: Management / Unit (amd64, postgres) * GitHub Check: Management / Unit (amd64, sqlite) * GitHub Check: Signal / Unit (amd64) * GitHub Check: Relay / Unit (amd64, -race) * GitHub Check: Relay / Unit (386) * GitHub Check: Client / Unit (amd64) * GitHub Check: Client / Unit (386) * GitHub Check: Client (Docker) / Unit * GitHub Check: iOS / Build * GitHub Check: Android / Build * GitHub Check: Client / Unit * GitHub Check: Darwin * GitHub Check: Linux * GitHub Check: Windows * GitHub Check: Client / Unit * GitHub Check: JS / Lint * GitHub Check: Client / Unit * GitHub Check: release_ui_darwin * GitHub Check: release </details> <details> <summary>🔇 Additional comments (21)</summary><blockquote> <details> <summary>client/internal/updatemanager/installer/installer_run_darwin.go (4)</summary><blockquote> `32-83`: **Setup flow and deferred restart look consistent with the two‑process design** The Setup function’s structure (result handler + single deferred block that writes the result and restarts daemon/UI, with a dry‑run fast‑path) is clear and consistent. No immediate correctness issues seen here; remaining concerns about Homebrew restarts are localized to `updateHomeBrew` and its interaction with this defer. --- `85-97`: **Service restart command is straightforward and bounded** `startDaemon` uses a 15s context timeout and logs both failure and success around `netbird service start`, which is a reasonable, bounded restart strategy. No issues from a correctness/robustness standpoint. --- `99-140`: **UI launch as console user looks sane for macOS** The `startUIAsUser` flow (console user via `stat /dev/console`, `user.Lookup`, `launchctl asuser … open -a`, HOME override, and `Process.Release` to detach) is a solid macOS‑idiomatic approach and addresses earlier concerns about `Start` + `CombinedOutput`. No further changes seem required here. --- `222-225`: **URL helper is simple and correct** `urlWithVersionArch` cleanly replaces `%version` and `%arch` in the template. No issues spotted here. </blockquote></details> <details> <summary>client/internal/updatemanager/update.go (1)</summary><blockquote> `3-11`: **UpdateInterface abstraction looks appropriate** The interface cleanly captures the subset of methods the manager needs and keeps the dependency on the concrete updater narrow. No changes needed here. </blockquote></details> <details> <summary>client/internal/engine.go (4)</summary><blockquote> `204-206`: **Engine lifecycle: updateManager field is well-placed** Adding `updateManager *updatemanager.Manager` on the Engine keeps auto‑update concerns localized and allows clean Start/Stop handling. No issues with this addition. --- `298-300`: **Stopping updateManager during Engine shutdown is correct** Stopping the `updateManager` as part of `Engine.Stop()` is the right place for this, and the nil check avoids panics. No further changes needed. --- `522-527`: **InitialUpdateHandling correctly reuses the existing sync mutex** Using `syncMsgMux` here keeps auto‑update initialization serialized with other sync‑related operations, which matches the rest of the Engine’s concurrency model. This method is a clean entry point for one‑off initial handling. --- `776-778`: **Auto-update handling on sync events is wired correctly** Calling `handleAutoUpdateVersion` from `handleSync` only when `NetworkMap.PeerConfig` is present is consistent with how other peer-config‑driven features are updated. Combined with the `AlwaysUpdate` and `initialCheck` logic, this matches the intended behavior of background vs. on-connect updates. </blockquote></details> <details> <summary>client/internal/connect.go (2)</summary><blockquote> `310-313`: **Engine construction with shared stateManager is consistent** Passing the shared `stateManager` into `NewEngine` centralizes state handling and keeps engine responsibilities clear. Locking around `c.engine` assignment remains correct. --- `320-327`: **Initial auto-update is correctly gated on explicit user-trigger flag** The logic: - Requires `loginResp.PeerConfig.AutoUpdate != nil`, and - Only runs `InitialUpdateHandling` when `c.doInitialAutoUpdate` is true, then - Resets `doInitialAutoUpdate` to false, ensures that the initial auto-update check only runs for explicitly flagged connections (e.g. UI “Connect” button) and not for background reconnects. This matches the PR’s stated objective. </blockquote></details> <details> <summary>client/internal/updatemanager/manager.go (10)</summary><blockquote> `30-37`: **UpdateState struct and naming are clear** `UpdateState` cleanly captures the minimal pre/target version info and `Name()` returning `"autoUpdate"` gives it a stable key for the state manager. No issues here. --- `39-73`: **Manager initialization is coherent** `NewManager` wires the status recorder, state manager, channels, current version, and `UpdateInterface` implementation in one place and assigns `triggerUpdateFn` for indirection (useful for tests). This is a solid constructor; no changes needed. --- `112-133`: **Start() behavior matches expected lifecycle** `Start` guards against double‑start via `m.cancel != nil`, wires the update fetcher callbacks, and runs `updateLoop` in a goroutine. This matches how `Engine` uses the manager and integrates cleanly with `Stop`. --- `134-163`: **SetVersion logic correctly handles “latest” vs specific versions** The method: - Validates that the manager is started (`m.cancel != nil`), - Uses the mutex to protect `expectedVersion` and `updateToLatestVersion`, - Treats `"latest"` specially, and - Avoids re‑triggering when the expected version hasn’t changed, which is exactly what the update loop needs. The non‑blocking send on `mgmUpdateChan` also avoids piling up signals. No changes needed. --- `165-179`: **Stop() cleans up updater resources safely** Calling `m.cancel()`, then, under the mutex, stopping the updater and nil’ing it before waiting on `m.wg` is a reasonable shutdown sequence and avoids data races on `update`. Given earlier discussion in this PR about `m.cancel` synchronization, this matches the intended contract with callers. --- `194-209`: **updateLoop control flow is straightforward** The loop reacts to: - Context cancellation, - Management‑driven version changes, and - New fetched version info, and then invokes `handleUpdate`. This separation keeps the loop simple and focused. Logging on `<-m.updateChannel` is helpful for debugging. --- `211-294`: **handleUpdate implements the decision logic correctly** Key points that look good: - Safely snapshots `expectedVersion`, `updateToLatestVersion`, and `LatestVersion()` under the mutex. - Resolves `"latest"` to the concrete fetched version when available. - Short‑circuits when there’s no target version. - Delegates gating to `shouldUpdate`, including throttling. - Persists `UpdateState` before triggering the installer so restart detection has the necessary info. - Emits both a human‑readable “updating client” event and a structured `progress_window` event for the UI. Given the legacy discussion about writing state before `triggerUpdateFn`, this implementation maintains the intended semantics. --- `296-325`: **loadAndDeleteUpdateState correctly uses the state manager APIs** Registering `UpdateState`, then loading, retrieving, deleting, and persisting via `stateManager` all look consistent with the state manager’s intended usage. Returning `errNoUpdateState` when no state is present is also a clean way for callers to distinguish “no update to report” from real errors. --- `327-348`: **shouldUpdate has sensible guards and throttling** Skipping updates for the `"development"` version, ensuring the target is strictly greater than the current version, and enforcing a 5‑minute minimum interval between triggers are all reasonable policies to prevent unnecessary or unsafe updates. --- `350-359`: **Result/error handling via installer is minimal but sufficient** `lastResultErrReason` and `triggerUpdate` create a fresh `Installer` and delegate to it, keeping installer concerns out of the manager. This separation is good; if you add richer result handling later, this indirection will make it easy to evolve. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| sm := profilemanager.NewServiceManager("") | ||
|
|
||
| path := sm.GetStatePath() | ||
| if runtime.GOOS == "ios" || runtime.GOOS == "android" { | ||
| if !fileExists(mobileDependency.StateFilePath) { | ||
| err := createFile(mobileDependency.StateFilePath) | ||
| if err != nil { | ||
| log.Errorf("failed to create state file: %v", err) | ||
| // we are not exiting as we can run without the state manager | ||
| } | ||
| } | ||
|
|
||
| path = mobileDependency.StateFilePath | ||
| } | ||
| stateManager := statemanager.New(path) | ||
| stateManager.RegisterState(&sshconfig.ShutdownState{}) | ||
|
|
||
| updateManager, err := updatemanager.NewManager(c.statusRecorder, stateManager) | ||
| if err == nil { | ||
| updateManager.CheckUpdateSuccess(c.ctx) | ||
|
|
||
| inst := installer.New() | ||
| if err := inst.CleanUpInstallerFiles(); err != nil { | ||
| log.Errorf("failed to clean up temporary installer file: %v", err) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
State manager + one-shot update success handling is reasonable, but consider logging unsupported platforms
Using ServiceManager to determine the state path, creating a shared stateManager, and registering sshconfig.ShutdownState is a solid approach. Calling CheckUpdateSuccess once on startup and then cleaning up installer files ensures that:
- Successful updates can be reported after a restart.
- Temporary installer artifacts don’t accumulate.
Two small improvements:
-
Log when auto-update manager is unsupported or fails to construct
On non‑Windows/non‑Darwin builds,updatemanager.NewManagerreturns an error; this path currently silently skips bothCheckUpdateSuccessandCleanUpInstallerFiles. Adding at least a debug or info log here would make it clear that auto‑update is not available on this platform. -
(Optional) Explicitly comment that
stateManager.Start()is handled by Engine
SincestateManageris created here butStart()/Stop()is managed byEngine, a short comment can help future readers understand why it’s not started in this block.
Example tweak:
- updateManager, err := updatemanager.NewManager(c.statusRecorder, stateManager)
- if err == nil {
+ updateManager, err := updatemanager.NewManager(c.statusRecorder, stateManager)
+ if err == nil {
updateManager.CheckUpdateSuccess(c.ctx)
inst := installer.New()
if err := inst.CleanUpInstallerFiles(); err != nil {
log.Errorf("failed to clean up temporary installer file: %v", err)
}
+ } else {
+ log.Debugf("auto-update manager not available: %v", err)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| sm := profilemanager.NewServiceManager("") | |
| path := sm.GetStatePath() | |
| if runtime.GOOS == "ios" || runtime.GOOS == "android" { | |
| if !fileExists(mobileDependency.StateFilePath) { | |
| err := createFile(mobileDependency.StateFilePath) | |
| if err != nil { | |
| log.Errorf("failed to create state file: %v", err) | |
| // we are not exiting as we can run without the state manager | |
| } | |
| } | |
| path = mobileDependency.StateFilePath | |
| } | |
| stateManager := statemanager.New(path) | |
| stateManager.RegisterState(&sshconfig.ShutdownState{}) | |
| updateManager, err := updatemanager.NewManager(c.statusRecorder, stateManager) | |
| if err == nil { | |
| updateManager.CheckUpdateSuccess(c.ctx) | |
| inst := installer.New() | |
| if err := inst.CleanUpInstallerFiles(); err != nil { | |
| log.Errorf("failed to clean up temporary installer file: %v", err) | |
| } | |
| } | |
| sm := profilemanager.NewServiceManager("") | |
| path := sm.GetStatePath() | |
| if runtime.GOOS == "ios" || runtime.GOOS == "android" { | |
| if !fileExists(mobileDependency.StateFilePath) { | |
| err := createFile(mobileDependency.StateFilePath) | |
| if err != nil { | |
| log.Errorf("failed to create state file: %v", err) | |
| // we are not exiting as we can run without the state manager | |
| } | |
| } | |
| path = mobileDependency.StateFilePath | |
| } | |
| stateManager := statemanager.New(path) | |
| stateManager.RegisterState(&sshconfig.ShutdownState{}) | |
| updateManager, err := updatemanager.NewManager(c.statusRecorder, stateManager) | |
| if err == nil { | |
| updateManager.CheckUpdateSuccess(c.ctx) | |
| inst := installer.New() | |
| if err := inst.CleanUpInstallerFiles(); err != nil { | |
| log.Errorf("failed to clean up temporary installer file: %v", err) | |
| } | |
| } else { | |
| log.Debugf("auto-update manager not available: %v", err) | |
| } |
🤖 Prompt for AI Agents
In client/internal/connect.go around lines 173 to 199, the code silently ignores
an error from updatemanager.NewManager so on unsupported platforms callers get
no feedback and CheckUpdateSuccess/CleanUpInstallerFiles are skipped; add a log
(Info or Debug) when NewManager returns an error that clearly states auto-update
is unsupported or failed to construct on this platform and include the error
details, and add a short inline comment above stateManager creation noting that
stateManager.Start()/Stop() are handled by Engine so it is intentionally not
started here.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
client/internal/engine.go (2)
758-767: Add logging when NewManager fails to improve diagnosability.When
updatemanager.NewManagerreturns an error, the code silently returns without logging. This makes it difficult to understand why auto-update is not running on certain platforms (e.g., unsupported platforms where the stub returns "not supported"). Based on past review comments, this should be addressed.// Start manager if needed if e.updateManager == nil { log.Infof("starting auto-update manager") updateManager, err := updatemanager.NewManager(e.statusRecorder, e.stateManager) if err != nil { + log.Warnf("failed to create auto-update manager: %v", err) return } e.updateManager = updateManager + e.updateManager.Start(e.ctx) + log.Infof("handling auto-update version: %s", autoUpdateSettings.Version) + e.updateManager.SetVersion(autoUpdateSettings.Version) + return } - e.updateManager.Start(e.ctx)
765-769: Calling Start() on every invocation may spam error logs.Based on past review comments,
Manager.Startlogs an error if it's already started. The current code callse.updateManager.Start(e.ctx)on every invocation ofhandleAutoUpdateVersion, which will repeatedly log errors whenAlwaysUpdateis true during config syncs.Move
Start()inside theif e.updateManager == nilblock so it's only called once when the manager is first created.if e.updateManager == nil { log.Infof("starting auto-update manager") updateManager, err := updatemanager.NewManager(e.statusRecorder, e.stateManager) if err != nil { + log.Warnf("failed to create auto-update manager: %v", err) return } e.updateManager = updateManager + e.updateManager.Start(e.ctx) } - e.updateManager.Start(e.ctx) log.Infof("handling auto-update version: %s", autoUpdateSettings.Version) e.updateManager.SetVersion(autoUpdateSettings.Version)
🧹 Nitpick comments (7)
client/internal/updatemanager/manager_test.go (2)
82-93: Add defensive nil check for clearer test failures.If the manager has a bug and triggers an update when
initialLatestVersionis nil, the test will panic at line 84 instead of failing with a clear error message.Consider adding a nil check:
select { case targetVersion := <-targetVersionChan: + if c.initialLatestVersion == nil { + t.Errorf("%s: Update triggered but initialLatestVersion is nil", c.name) + } else if targetVersion != c.initialLatestVersion.String() { t.Errorf("%s: Initial update version mismatch, expected %v, got %v", c.name, c.initialLatestVersion.String(), targetVersion) } triggeredInit = true
197-207: Add defensive nil check for clearer test failures.Several test cases have
latestVersionset to nil (e.g., lines 125-130, 167-172). If the manager has a bug and triggers an update whenexpectedVersion == "latest"butlatestVersionis nil, the test will panic at line 199 instead of failing with a clear error message.Consider adding a nil check:
select { case targetVersion := <-targetVersionChan: + if c.expectedVersion == "latest" && c.latestVersion == nil { + t.Errorf("%s: Update triggered for 'latest' but latestVersion is nil", c.name) + } else if c.expectedVersion == "latest" && targetVersion != c.latestVersion.String() { t.Errorf("%s: Update version mismatch, expected %v, got %v", c.name, c.latestVersion.String(), targetVersion) } else if c.expectedVersion != "latest" && targetVersion != c.expectedVersion { t.Errorf("%s: Update version mismatch, expected %v, got %v", c.name, c.expectedVersion, targetVersion) } updateTriggered = trueclient/internal/updatemanager/installer/installer_common.go (5)
112-116: Clarify or remove the commented-out Windows process detachment code.This commented block references Windows-specific
SysProcAttrwithDETACHED_PROCESSflags but provides no explanation for why it's disabled. Per PR objectives, this was flagged as incomplete handling.Either implement the Windows-specific detachment or add a comment explaining why it's not needed.
- /* - updateCmd.SysProcAttr = &syscall.SysProcAttr{ - CreationFlags: syscall.CREATE_NEW_PROCESS_GROUP | 0x00000008, // 0x00000008 is DETACHED_PROCESS - } - */ + // Note: Process detachment is handled via Start() + Release() pattern. + // Platform-specific SysProcAttr may be set in installer_run_*.go files if needed.
118-126: Use defer for Process.Release() to prevent handle leaks on intermediate failures.Per PR objectives, calling
Process.Release()without defer risks leaked handles if an error occurs betweenStart()andRelease(). While there's no code between them currently, using defer is more defensive.// Start the updater process asynchronously if err := updateCmd.Start(); err != nil { return err } - // Release the process so the OS can fully detach it - if err := updateCmd.Process.Release(); err != nil { - log.Warnf("failed to release updater process: %v", err) - } + // Release the process so the OS can fully detach it + defer func() { + if err := updateCmd.Process.Release(); err != nil { + log.Warnf("failed to release updater process: %v", err) + } + }() log.Infof("updater started with PID %d", updateCmd.Process.Pid)
149-157: Consider continuing cleanup even if ReadDir fails after partial cleanup.If
os.Removefor updater binary succeeds butos.ReadDirfails, the function returns the ReadDir error, losing the context that partial cleanup succeeded. Consider accumulating this error too.if err := os.Remove(filepath.Join(u.tempDir, updaterBinary)); err != nil { - merr = multierror.Append(merr, fmt.Errorf("failed to remove updater binary: %w", err)) + if !os.IsNotExist(err) { + merr = multierror.Append(merr, fmt.Errorf("failed to remove updater binary: %w", err)) + } } entries, err := os.ReadDir(u.tempDir) if err != nil { - return err + merr = multierror.Append(merr, fmt.Errorf("failed to read temp dir: %w", err)) + return merr.ErrorOrNil() }
210-216: Directory permissions are overly permissive.The temp directory is created with mode
0o755(world-readable/executable). For a directory containing installer binaries, consider using0o700to restrict access to the owner only.func (u *Installer) mkTempDir() error { - if err := os.MkdirAll(u.tempDir, 0o755); err != nil { + if err := os.MkdirAll(u.tempDir, 0o700); err != nil { log.Debugf("failed to create tempdir: %s", u.tempDir) return err } return nil }
249-276: Consider syncing destination file before closing to ensure durability.For an installer binary, ensuring the data is flushed to disk before proceeding is important to avoid corruption if the system crashes mid-update. Consider calling
out.Sync()before closing.if _, err := io.Copy(out, in); err != nil { return fmt.Errorf("copy: %w", err) } + if err := out.Sync(); err != nil { + return fmt.Errorf("sync destination: %w", err) + } + return nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
client/internal/engine.go(8 hunks)client/internal/updatemanager/installer/doc.go(1 hunks)client/internal/updatemanager/installer/installer_common.go(1 hunks)client/internal/updatemanager/manager_test.go(1 hunks)client/ui/quickactions.go(1 hunks)management/internals/shared/grpc/conversion.go(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- client/internal/updatemanager/installer/doc.go
🚧 Files skipped from review as they are similar to previous changes (1)
- management/internals/shared/grpc/conversion.go
🧰 Additional context used
🧬 Code graph analysis (3)
client/internal/updatemanager/installer/installer_common.go (7)
client/internal/updatemanager/installer/result.go (1)
NewResultHandler(33-42)shared/management/status/error.go (2)
Errorf(70-75)Error(54-57)client/internal/updatemanager/reposign/verify.go (1)
NewArtifactVerify(34-41)client/internal/updatemanager/installer/repourl_dev.go (1)
DefaultSigningKeysBaseURL(6-6)client/internal/updatemanager/installer/repourl_prod.go (1)
DefaultSigningKeysBaseURL(6-6)client/internal/updatemanager/installer/types.go (1)
Type(3-6)client/internal/updatemanager/downloader/downloader.go (1)
DownloadToFile(17-58)
client/internal/updatemanager/manager_test.go (3)
client/internal/updatemanager/manager.go (1)
NewManager(61-73)client/internal/peer/status.go (1)
NewRecorder(223-234)shared/management/status/error.go (1)
Errorf(70-75)
client/internal/engine.go (3)
client/internal/updatemanager/manager.go (2)
Manager(39-59)NewManager(61-73)shared/management/proto/management.pb.go (9)
AutoUpdateSettings(1857-1866)AutoUpdateSettings(1881-1881)AutoUpdateSettings(1896-1898)NetworkMap(1915-1945)NetworkMap(1960-1960)NetworkMap(1975-1977)PeerConfig(1749-1767)PeerConfig(1782-1782)PeerConfig(1797-1799)management/server/networks/resources/manager.go (1)
NewManager(42-49)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (24)
- GitHub Check: Management / Unit (amd64, sqlite)
- GitHub Check: Management / Unit (amd64, mysql)
- GitHub Check: Management / Unit (amd64, postgres)
- GitHub Check: Relay / Unit (386)
- GitHub Check: Management / Integration (amd64, postgres)
- GitHub Check: Management / Benchmark (API) (amd64, sqlite)
- GitHub Check: Management / Benchmark (API) (amd64, postgres)
- GitHub Check: Management / Benchmark (amd64, sqlite)
- GitHub Check: Relay / Unit (amd64, -race)
- GitHub Check: Client / Unit (386)
- GitHub Check: Management / Benchmark (amd64, postgres)
- GitHub Check: Client / Unit (amd64)
- GitHub Check: Client (Docker) / Unit
- GitHub Check: JS / Lint
- GitHub Check: Client / Unit
- GitHub Check: Darwin
- GitHub Check: Client / Unit
- GitHub Check: release_ui_darwin
- GitHub Check: Linux
- GitHub Check: Windows
- GitHub Check: release
- GitHub Check: Android / Build
- GitHub Check: Client / Unit
- GitHub Check: iOS / Build
🔇 Additional comments (14)
client/internal/updatemanager/manager_test.go (1)
18-37: LGTM! Clean mock implementation.The mock provides a minimal, focused implementation of the version update interface suitable for testing the manager's update decision logic.
client/internal/engine.go (7)
50-50: LGTM: Import added for update manager integration.The import is correctly placed and necessary for the new auto-update functionality.
75-75: Consider defining this constant alongside other auto-update constants.The
disableAutoUpdatesentinel value is appropriately defined as a constant. This is good practice for magic strings.
204-206: LGTM: Update manager field added to Engine struct.The field is properly typed and the comment provides context.
227-227: LGTM: Constructor signature updated to accept stateManager.The stateManager is now passed externally rather than dynamically created, which improves testability and explicit dependency injection.
Also applies to: 243-243
298-300: LGTM: Update manager properly stopped during engine shutdown.The nil check before calling Stop() prevents panics if the manager was never initialized.
522-527: LGTM: Initial update handling method correctly delegates to internal handler.The method properly acquires the sync mutex before calling the internal handler, maintaining thread safety.
776-778: LGTM: Auto-update version handling integrated into sync flow.The integration correctly passes
falseforinitialCheckduring sync operations, ensuring updates only trigger on sync whenAlwaysUpdateis enabled.client/internal/updatemanager/installer/installer_common.go (6)
1-3: LGTM: Build tags correctly restrict to supported platforms.The build constraint
//go:build windows || darwinproperly limits this implementation to the two supported auto-update platforms.
23-39: LGTM: Clean constructor pattern for Installer.The dual-constructor pattern with
New()for service use andNewWithDir()for updater process use is clear and well-documented.
73-82: Signature verification occurs after updater is copied — consider verifying before copying.Per PR objectives, the artifact signature verification runs in the service process but after downloading. This is good. However, the updater binary is copied (line 86) after verification, which is the correct order. The concern from PR objectives about verifying "before copying/launching the updater" appears to be addressed here since:
- Download happens (line 67)
- Signature verification happens (lines 73-82)
- Only then is the updater copied and launched (lines 86-129)
135-147: Nil pointer dereference fix verified.The previous review correctly identified that
info.IsDir()could panic ifos.Statreturned a non-"not exist" error with nilinfo. This is now properly fixed by separating the error check (lines 138-143) from theIsDir()check (lines 145-147).
176-176: Error accumulation fix verified.The previous review correctly identified that
merrwas being discarded. This is now properly fixed by returningmerr.ErrorOrNil().
290-292: LGTM: Dry-run feature enabled via environment variable.The
isDryRunEnabled()function provides a safe testing mechanism without modifying production code paths.
| defer func() { | ||
| if err := out.Close(); err != nil { | ||
| log.Warnf("failed to close source file: %v", err) | ||
| } | ||
| }() |
There was a problem hiding this comment.
Fix incorrect log message: "source file" should be "destination file".
The log message on line 267 incorrectly says "source file" when closing the destination file.
defer func() {
if err := out.Close(); err != nil {
- log.Warnf("failed to close source file: %v", err)
+ log.Warnf("failed to close destination file: %v", err)
}
}()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| defer func() { | |
| if err := out.Close(); err != nil { | |
| log.Warnf("failed to close source file: %v", err) | |
| } | |
| }() | |
| defer func() { | |
| if err := out.Close(); err != nil { | |
| log.Warnf("failed to close destination file: %v", err) | |
| } | |
| }() |
🤖 Prompt for AI Agents
In client/internal/updatemanager/installer/installer_common.go around lines 265
to 269, the defer's log message incorrectly refers to "source file" when closing
the destination file; change the log message to "failed to close destination
file" (or similar) so it correctly describes the file being closed and preserve
the existing error formatting.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/internal/updatemanager/installer/repourl_prod.go(1 hunks)
🔇 Additional comments (1)
client/internal/updatemanager/installer/repourl_prod.go (1)
1-3: LGTM: Build constraint correctly separates prod and dev configurations.The build tag ensures production builds use this URL while development builds use the alternative from
repourl_dev.go.
| const ( | ||
| DefaultSigningKeysBaseURL = "https://netbirdio.github.io/public-keys/artifact-signatures" | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the signing keys URL is accessible and returns valid content
echo "=== Checking URL accessibility and response ==="
echo "URL: https://netbirdio.github.io/public-keys/artifact-signatures"
echo ""
echo "Response headers:"
curl -I -L -s "https://netbirdio.github.io/public-keys/artifact-signatures" 2>&1 | head -n 20
echo -e "\n\n=== Checking content response ==="
echo "First 1000 chars of response:"
curl -L -s "https://netbirdio.github.io/public-keys/artifact-signatures" 2>&1 | head -c 1000
echo -e "\n\n=== HTTP status code ==="
curl -s -o /dev/null -w "%{http_code}\n" "https://netbirdio.github.io/public-keys/artifact-signatures"Repository: netbirdio/netbird
Length of output: 1923
The signing keys URL is inaccessible—it returns HTTP 404 after a redirect. This will cause artifact signature verification to fail in production.
The URL https://netbirdio.github.io/public-keys/artifact-signatures redirects to https://netbirdio.github.io/public-keys/artifact-signatures/ but ultimately returns a "Page not found" error. This is a critical issue because signature verification relies on accessing this endpoint to retrieve the public keys needed to validate artifact signatures. Either the URL needs to be corrected to point to where the keys are actually hosted, or the keys repository needs to be deployed to the expected location.
🤖 Prompt for AI Agents
In client/internal/updatemanager/installer/repourl_prod.go around lines 5-7, the
DefaultSigningKeysBaseURL constant points to an inaccessible location (404 after
redirect) which breaks signature verification; replace the constant with the
correct public-keys hosting URL (or the exact path where the artifact-signatures
files are deployed), or ensure the keys repo is published to that exact path;
verify the new URL by fetching it in CI and add a small runtime fallback/error
message that clearly fails if the signing keys endpoint returns non-200 so the
issue surfaces immediately.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
client/internal/updatemanager/installer/installer_run_windows.go (1)
36-104: Installer flow looks solid; consider handling “expected” non‑zero installer exit codesThe overall Setup flow (deferred restart + result writing) is clean and easy to follow. One edge case to double‑check is that any non‑zero installer exit code is treated as a hard failure. If your MSI/EXE uses specific non‑zero codes like
3010(“reboot required”) to signal success with reboot, this will currently surface as a failed update to the caller.If those codes are expected, consider mapping them to a distinct “success with reboot” status (or at least logging them differently) instead of treating them as generic errors, so the updater logic can react appropriately.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
client/internal/updatemanager/installer/installer_run_darwin.go(1 hunks)client/internal/updatemanager/installer/installer_run_windows.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- client/internal/updatemanager/installer/installer_run_darwin.go
🧰 Additional context used
🧬 Code graph analysis (1)
client/internal/updatemanager/installer/installer_run_windows.go (6)
client/internal/updatemanager/installer/installer_common.go (1)
Installer(23-25)client/internal/updatemanager/installer/installer.go (1)
Installer(14-16)client/internal/updatemanager/installer/result.go (1)
NewResultHandler(33-42)shared/management/status/error.go (1)
Errorf(70-75)client/internal/updatemanager/installer/types_windows.go (1)
TypeExe(18-18)client/internal/updatemanager/installer/types.go (1)
Type(3-6)
🔇 Additional comments (1)
client/internal/updatemanager/installer/installer_run_windows.go (1)
106-118: Service restart helper is straightforward and bounded; no issuesThe helper that restarts the daemon is minimal and clear: it bounds the start attempt with a timeout, captures combined output for debugging on failure, and returns a plain error for the caller/deferred cleanup to log. No changes needed from my side here.
| const ( | ||
| daemonName = "netbird.exe" | ||
| uiName = "netbird-ui.exe" | ||
| updaterBinary = "updater.exe" | ||
|
|
||
| msiLogFile = "msi.log" | ||
|
|
||
| msiDownloadURL = "https://github.com/mlsmaycon/netbird/releases/download/v%version/netbird_installer_%version_windows_%arch.msi" | ||
| exeDownloadURL = "https://github.com/mlsmaycon/netbird/releases/download/v%version/netbird_installer_%version_windows_%arch.exe" | ||
| ) | ||
|
|
||
| var ( | ||
| defaultTempDir = filepath.Join(os.Getenv("ProgramData"), "Netbird", "tmp-install") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n client/internal/updatemanager/installer/installer_run_windows.go | head -40Repository: netbirdio/netbird
Length of output: 1376
🏁 Script executed:
cat -n client/internal/updatemanager/installer/installer_run_windows.go | sed -n '200,220p'Repository: netbirdio/netbird
Length of output: 508
🏁 Script executed:
rg "mlsmaycon" --type-listRepository: netbirdio/netbird
Length of output: 450
🏁 Script executed:
rg "mlsmaycon" --type goRepository: netbirdio/netbird
Length of output: 725
🏁 Script executed:
rg "netbirdio" --type go | head -20Repository: netbirdio/netbird
Length of output: 1558
🏁 Script executed:
rg "GOARCH" --type go -B 2 -A 2Repository: netbirdio/netbird
Length of output: 3211
🏁 Script executed:
# Look for any arch mapping or configuration
rg "amd64|x86_64|arm|arch" --type go | grep -E "(map|const|var)" | head -20Repository: netbirdio/netbird
Length of output: 1995
🏁 Script executed:
cat -n version/url_windows.goRepository: netbirdio/netbird
Length of output: 842
🏁 Script executed:
cat -n version/url_darwin.goRepository: netbirdio/netbird
Length of output: 868
🏁 Script executed:
# Check if there are any recent commits or comments about these URLs
git log --oneline --all -- "client/internal/updatemanager/installer/installer_run_windows.go" | head -10Repository: netbirdio/netbird
Length of output: 100
🏁 Script executed:
rg "TODO|FIXME|XXX" -B 2 -A 2 client/internal/updatemanager/installer/installer_run_windows.goRepository: netbirdio/netbird
Length of output: 43
🏁 Script executed:
# Look for environment variable or config related to download URLs
rg "NETBIRD.*URL|DOWNLOAD.*URL|INSTALLER.*URL" --type goRepository: netbirdio/netbird
Length of output: 43
🏁 Script executed:
# Check if there's any documentation or issue references
find . -name "*.md" -type f -exec grep -l "mlsmaycon\|installer.*url\|download.*url" {} \;Repository: netbirdio/netbird
Length of output: 43
Installer URLs must point to official repository and handle architecture mapping
The download URLs currently have two blocking issues for production use:
-
GitHub URLs hardcoded to personal fork (
mlsmaycon)The
msiDownloadURLandexeDownloadURLconstants point togithub.com/mlsmaycon/netbird. Production builds must use the official repository (or official package host likepkgs.netbird.ioas shown inversion/url_*.go), otherwise clients will fetch from a personal fork. -
No architecture mapping despite platform differences
The code drops
runtime.GOARCHdirectly into the URL template without mapping. Unlikeversion/url_windows.goandversion/url_darwin.gowhich explicitly handle architecture variants (e.g.,amd64vsx64,arm64vsarm64), the installer code assumes artifact naming matchesruntime.GOARCHexactly. A mismatch will cause 404s at runtime.
Update the constants to use the official repository URL and add architecture mapping logic consistent with version/url_*.go. The defaultTempDir hardcoded path should also be centralized with any installer configuration mechanism you introduce.
🤖 Prompt for AI Agents
In client/internal/updatemanager/installer/installer_run_windows.go around lines
18 to 30, the download URL constants point to a personal GitHub fork and assume
runtime.GOARCH matches the artifact naming; change the msiDownloadURL and
exeDownloadURL to the official host (e.g., pkgs.netbird.io or the official
github.com/netbird repo used in version/url_*.go) and implement architecture
mapping consistent with version/url_windows.go (map runtime.GOARCH values to the
release artifact names, e.g., "amd64" => "x64", "arm64" => "arm64", etc.) so URL
templates produce valid artifact names; also remove or centralize the hardcoded
defaultTempDir into the installer configuration (or a shared constant) instead
of using a local filepath.Join call so the temp path is configurable and
consistent with other installer configuration.
| func (u *Installer) startUIAsUser(daemonFolder string) error { | ||
| uiPath := filepath.Join(daemonFolder, uiName) | ||
| log.Infof("starting netbird-ui: %s", uiPath) | ||
|
|
||
| // Get the active console session ID | ||
| sessionID := windows.WTSGetActiveConsoleSessionId() | ||
| if sessionID == 0xFFFFFFFF { | ||
| return fmt.Errorf("no active user session found") | ||
| } | ||
|
|
||
| // Get the user token for that session | ||
| var userToken windows.Token | ||
| err := windows.WTSQueryUserToken(sessionID, &userToken) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to query user token: %w", err) | ||
| } | ||
| defer func() { | ||
| if err := userToken.Close(); err != nil { | ||
| log.Warnf("failed to close user token: %v", err) | ||
| } | ||
| }() | ||
|
|
||
| // Duplicate the token to a primary token | ||
| var primaryToken windows.Token | ||
| err = windows.DuplicateTokenEx( | ||
| userToken, | ||
| windows.MAXIMUM_ALLOWED, | ||
| nil, | ||
| windows.SecurityImpersonation, | ||
| windows.TokenPrimary, | ||
| &primaryToken, | ||
| ) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to duplicate token: %w", err) | ||
| } | ||
| defer func() { | ||
| if err := primaryToken.Close(); err != nil { | ||
| log.Warnf("failed to close token: %v", err) | ||
| } | ||
| }() | ||
|
|
||
| // Prepare startup info | ||
| var si windows.StartupInfo | ||
| si.Cb = uint32(unsafe.Sizeof(si)) | ||
| si.Desktop = windows.StringToUTF16Ptr("winsta0\\default") | ||
|
|
||
| var pi windows.ProcessInformation | ||
|
|
||
| cmdLine, err := windows.UTF16PtrFromString(uiPath) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to convert path to UTF16: %w", err) | ||
| } | ||
|
|
||
| creationFlags := uint32(0x00000200 | 0x00000008 | 0x00000400) // CREATE_NEW_PROCESS_GROUP | DETACHED_PROCESS | CREATE_UNICODE_ENVIRONMENT | ||
|
|
||
| err = windows.CreateProcessAsUser( | ||
| primaryToken, | ||
| nil, | ||
| cmdLine, | ||
| nil, | ||
| nil, | ||
| false, | ||
| creationFlags, | ||
| nil, | ||
| nil, | ||
| &si, | ||
| &pi, | ||
| ) | ||
| if err != nil { | ||
| return fmt.Errorf("CreateProcessAsUser failed: %w", err) | ||
| } | ||
|
|
||
| // Close handles | ||
| if err := windows.CloseHandle(pi.Process); err != nil { | ||
| log.Warnf("failed to close process handle: %v", err) | ||
| } | ||
| if err := windows.CloseHandle(pi.Thread); err != nil { | ||
| log.Warnf("failed to close thread handle: %v", err) | ||
| } | ||
|
|
||
| log.Infof("netbird-ui started successfully in session %d", sessionID) | ||
| return nil |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, verify the file exists and examine the current code
cat -n client/internal/updatemanager/installer/installer_run_windows.go | sed -n '120,201p'Repository: netbirdio/netbird
Length of output: 2737
🏁 Script executed:
# Check if there's any recent history or comments about this issue
rg -B2 -A2 "CreateProcessAsUser" client/internal/updatemanager/installer/Repository: netbirdio/netbird
Length of output: 1532
🏁 Script executed:
# Look for any other references to path handling or quoting in this file
rg -i "quote|space|path" client/internal/updatemanager/installer/installer_run_windows.go | head -20Repository: netbirdio/netbird
Length of output: 720
🌐 Web query:
Windows CreateProcessAsUser nil ApplicationName unquoted command line behavior spaces paths
💡 Result:
Short answer: CreateProcessAsUser follows the same CreateProcess parsing rules. If lpApplicationName is NULL the system treats the first whitespace-delimited token of lpCommandLine as the module name — so an unquoted path that contains spaces is ambiguous and Windows will try to interpret substrings (and append .exe if no extension) in a number of ways. This can cause the wrong program to be launched (or fail) if a different file matching an early token exists. To avoid this, either pass the full executable in lpApplicationName (recommended) or surround the full executable path in quotes as the first token of lpCommandLine. (learn.microsoft.com)
Key notes from MS docs:
- If lpApplicationName is NULL, the first white-space token of lpCommandLine is the module; use quotes for paths with spaces. (learn.microsoft.com)
- Windows will try multiple interpretations (splitting at spaces, appending .exe) when the path is ambiguous — so unquoted "C:\Program Files\sub dir\program name" can be parsed in several ways. (learn.microsoft.com)
Recommendation: supply lpApplicationName with the exact executable path (no parsing ambiguity) or ensure lpCommandLine begins with the quoted executable path (e.g. ""C:\Program Files\MyApp\MyApp.exe" arg1 ..."). (learn.microsoft.com)
Citations:
- 1: https://learn.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-createprocessa?utm_source=openai
- 2: https://learn.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-createprocessa?utm_source=openai
- 3: https://learn.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-createprocesswithlogonw?utm_source=openai
- 4: https://learn.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-createprocessa?utm_source=openai
Fix: CreateProcessAsUser command line must be quoted for paths with spaces
The code passes uiPath directly to cmdLine without quoting, but when ApplicationName is nil, the Windows process creation API parses the first whitespace-delimited token of the command line as the executable. For installations under Program Files (e.g., C:\Program Files\Netbird\netbird-ui.exe), this causes Windows to attempt to execute C:\Program instead, leading to process creation failure.
Wrap uiPath in quotes as recommended by Microsoft documentation:
- cmdLine, err := windows.UTF16PtrFromString(uiPath)
+ cmdLine, err := windows.UTF16PtrFromString(fmt.Sprintf("\"%s\"", uiPath))This ensures correct parsing regardless of spaces in the installation path.
🤖 Prompt for AI Agents
In client/internal/updatemanager/installer/installer_run_windows.go around lines
120-201, the CreateProcessAsUser call passes uiPath unquoted as the command-line
which breaks for paths with spaces (e.g., under "Program Files"); wrap the full
uiPath in double quotes before converting to UTF16 (e.g., build a string like
`"` + uiPath + `"`), then pass that quoted string to windows.UTF16PtrFromString
so CreateProcessAsUser receives a quoted executable path and Windows parses it
correctly.
Describe your changes
Issue ticket number and link
Stack
Checklist
Documentation
Select exactly one:
Docs PR URL (required if "docs added" is checked)
Paste the PR link from https://github.com/netbirdio/docs here:
https://github.com/netbirdio/docs/pull/__
Summary by CodeRabbit
New Features
Documentation
Other
✏️ Tip: You can customize this high-level summary in your review settings.