fix: auto updater logic #126
Conversation
WalkthroughRemoves the entire UpdateService subsystem and related frontend bindings/UI: backend update interface and implementation, mocks, frontend UpdateNotification and UpdateInfo model, Wails JS/TS bindings, README changelog section, and associated dependencies and startup hooks are deleted. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (13)
💤 Files with no reviewable changes (12)
✅ Files skipped from review due to trivial changes (1)
⏰ 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). (1)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
SCANOSS SCAN Completed 🚀
View more details on SCANOSS Action Summary |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/service/update_service_impl.go (1)
304-315: Create the mount point before callinghdiutil attach.Line 305 currently invokes
hdiutil attach … -mountpointon a directory that never gets created, so the command fails with “No such file or directory,” aborting every macOS update.Apply this diff to prepare the mount point:
// Mount the DMG mountPoint := filepath.Join(os.TempDir(), "scanoss-update-mount") + if err := os.MkdirAll(mountPoint, 0o755); err != nil { + return fmt.Errorf("failed to prepare mount point: %w", err) + } log.Info().Msgf("Mounting DMG to %s...", mountPoint) cmd = exec.Command("hdiutil", "attach", dmgPath, "-nobrowse", "-mountpoint", mountPoint)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
CHANGELOG.md(1 hunks)backend/service/update_service_impl.go(2 hunks)go.mod(1 hunks)main.go(0 hunks)
💤 Files with no reviewable changes (1)
- main.go
⏰ 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). (1)
- GitHub Check: build
SCANOSS SCAN Completed 🚀
View more details on SCANOSS Action Summary |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/service/update_service_impl.go (1)
455-460: Guard Windows update failures with a rollback.
Same concern here: ifupdate.Applyfails we need to callupdate.RollbackError(err)before returning, otherwise the running exe may be left unusable.- // Apply the update using go-update - log.Info().Msg("Applying update...") - err = update.Apply(newBinary, update.Options{ - TargetPath: currentExe, - }) - if err != nil { - return fmt.Errorf("failed to apply update: %w", err) - } + // Apply the update using go-update + log.Info().Msg("Applying update...") + if err := update.Apply(newBinary, update.Options{ + TargetPath: currentExe, + }); err != nil { + if rollbackErr := update.RollbackError(err); rollbackErr != nil { + log.Error().Err(rollbackErr).Msg("failed to rollback after Windows update error") + } + return fmt.Errorf("failed to apply update: %w", err) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/service/update_service_impl.go(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
backend/service/update_service_impl.go (3)
internal/config/config.go (1)
GetInstance(111-116)frontend/wailsjs/go/main/App.js (1)
GetScanRoot(17-19)frontend/wailsjs/runtime/runtime.js (1)
Quit(184-186)
SCANOSS SCAN Completed 🚀
View more details on SCANOSS Action Summary |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
backend/service/update_service_impl.go (1)
454-461: Add rollback when go-update Apply fails on Windows.The Windows update path does not invoke
update.RollbackErrorwhenupdate.Applyfails, while macOS (Lines 370-372) and Linux (Lines 563-565) both include this critical recovery mechanism. Without rollback, a failed update can leave the binary in a corrupted state.Apply this diff to add rollback handling:
// Apply the update using go-update log.Info().Msg("Applying update...") - err = update.Apply(newBinary, update.Options{ + if err := update.Apply(newBinary, update.Options{ TargetPath: currentExe, - }) - if err != nil { + }); err != nil { + if rollbackErr := update.RollbackError(err); rollbackErr != nil { + log.Error().Err(rollbackErr).Msg("failed to rollback after Windows update error") + } return fmt.Errorf("failed to apply update: %w", err) }
🧹 Nitpick comments (2)
backend/service/update_service_impl.go (2)
466-466: Consider explicit scan-root argument for consistency with macOS.The Windows restart uses
os.Args[1:]while macOS explicitly passes--scan-root(line 388). This creates platform-specific behavior where Windows may not preserve the scan root configuration if the application was started without those arguments.For consistency, consider mirroring the macOS approach:
+ currentScanRoot := config.GetInstance().GetScanRoot() + var args []string + if currentScanRoot != "" { + args = []string{"--scan-root", currentScanRoot} + } + cmd = exec.Command(currentExe, args...) - cmd = exec.Command(currentExe, os.Args[1:]...)
572-572: Consider explicit scan-root argument for cross-platform consistency.Similar to the Windows restart logic, Linux uses
os.Args[1:]while macOS explicitly passes--scan-root. This creates platform-specific behavior in how the scan root configuration is preserved across updates.For consistency across all platforms, consider:
+ currentScanRoot := config.GetInstance().GetScanRoot() + var args []string + if currentScanRoot != "" { + args = []string{"--scan-root", currentScanRoot} + } + cmd = exec.Command(currentExe, args...) - cmd = exec.Command(currentExe, os.Args[1:]...)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/service/update_service_impl.go(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
backend/service/update_service_impl.go (1)
internal/config/config.go (1)
GetInstance(111-116)
⏰ 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). (2)
- GitHub Check: integration-tests
- GitHub Check: unit_tests
🔇 Additional comments (3)
backend/service/update_service_impl.go (3)
45-45: LGTM!The config import is properly used at line 387 to retrieve the current scan root for preserving application state across restarts.
306-308: LGTM!The mount point preparation logic is correct with appropriate error handling.
557-567: LGTM on rollback implementation!The Linux update path now correctly includes rollback error handling, consistent with the macOS implementation. This addresses the critical issue from previous reviews.
SCANOSS SCAN Completed 🚀
View more details on SCANOSS Action Summary |
SCANOSS SCAN Completed 🚀
View more details on SCANOSS Action Summary |
SCANOSS SCAN Completed 🚀
View more details on SCANOSS Action Summary |
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
backend/service/update_service_impl.go (2)
576-581: Add rollback when go-update Apply fails on Windows.The past review flagged that
update.Applydoes not restore the original binary on failure. The suggested fix to wrap withupdate.RollbackError(err)was not applied in this PR.Apply this diff:
// Apply the update using go-update log.Info().Msg("Applying update...") - err = update.Apply(newBinary, update.Options{ + if err := update.Apply(newBinary, update.Options{ TargetPath: currentExe, - }) - if err != nil { + }); err != nil { + if rollbackErr := update.RollbackError(err); rollbackErr != nil { + log.Error().Err(rollbackErr).Msg("failed to rollback after Windows update error") + } return fmt.Errorf("failed to apply update: %w", err) }
494-495: Validate scan root before passing to helper script.
GetScanRoot()may return an empty string or invalid path. The helper script does check for empty string (line 131), but invalid paths are not validated. This could cause the restarted app to fail or lose user configuration, as flagged in past reviews.Apply this diff:
// Get current scan root currentScanRoot := config.GetInstance().GetScanRoot() + + // Validate scan root if provided + if currentScanRoot != "" { + if info, err := os.Stat(currentScanRoot); err != nil || !info.IsDir() { + log.Warn().Str("scan_root", currentScanRoot).Msg("Scan root is invalid, will not pass to restarted app") + currentScanRoot = "" + } + }
🧹 Nitpick comments (3)
backend/service/update_service_impl.go (3)
86-90: Reduce timeout from 30 to 10 seconds.A 30-second wait is excessive for a graceful app quit triggered by
wailsruntime.Quit(). The helper launches after the quit is initiated, so the process should exit within a few seconds. A 10-second timeout provides adequate buffer while improving user experience.
498-502: Check for existing helper script and handle write errors.If
/tmp/scanoss-update-helper.shalready exists from a previous failed update, it could have incorrect permissions or content. Also,/tmpmay be mounted withnoexecon some systems.Consider these improvements:
// Write the helper script to a temporary file - helperScriptPath := filepath.Join(os.TempDir(), "scanoss-update-helper.sh") + helperScriptPath := filepath.Join(os.TempDir(), fmt.Sprintf("scanoss-update-helper-%d.sh", os.Getpid())) + + // Remove any existing script + os.Remove(helperScriptPath) + if err := os.WriteFile(helperScriptPath, []byte(updateHelperScript), 0o755); err != nil { return fmt.Errorf("failed to write helper script: %w", err) }
507-510: Add error context and verify script is executable.If the helper script fails to launch, the error message doesn't indicate why. Common causes include
noexecmount flags or permission issues.+ // Verify script is executable + if info, err := os.Stat(helperScriptPath); err != nil { + return fmt.Errorf("helper script not accessible: %w", err) + } else if info.Mode()&0o111 == 0 { + return fmt.Errorf("helper script is not executable (permissions: %v)", info.Mode()) + } + // Launch the helper script in the background cmd = exec.Command(helperScriptPath, currentAppPath, newAppPath, backupPath, currentScanRoot, fmt.Sprintf("%d", pid)) + cmd.Stdout = os.Stdout + cmd.Stderr = os.Stderr if err := cmd.Start(); err != nil { - return fmt.Errorf("failed to launch helper script: %w", err) + return fmt.Errorf("failed to launch helper script (check /tmp mount flags): %w", err) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/service/update_service_impl.go(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
backend/service/update_service_impl.go (3)
internal/config/config.go (1)
GetInstance(111-116)frontend/wailsjs/go/service/UpdateServiceImpl.js (3)
GetCurrentVersion(21-23)VerifyUpdateSuccess(29-31)CheckForFailedUpdate(9-11)backend/entities/version.go (1)
AppVersion(29-29)
⏰ 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). (1)
- GitHub Check: build
🔇 Additional comments (4)
backend/service/update_service_impl.go (4)
45-45: LGTM!The config import is appropriately used to retrieve the scan root for passing to restarted applications.
586-591: LGTM on scan root handling.The scan root is properly retrieved and conditionally passed as args only when non-empty. This correctly handles the case where scan root is not configured.
675-702: LGTM! Rollback and scan root handling are correct.The Linux update path correctly implements:
- Rollback handling via
update.RollbackError(err)(lines 688-690) - this addresses the critical issue from past reviews- Proper scan root retrieval and conditional passing to restarted app (lines 697-701)
745-748: LGTM!Simple getter method correctly returns the application version.
| log "Old process exited" | ||
| # Additional grace period for file locks to release | ||
| sleep 2 |
There was a problem hiding this comment.
Make grace period configurable or increase duration.
The hardcoded 2-second delay may be insufficient on slower systems or under heavy I/O load. Consider increasing to 5 seconds or making it configurable.
🤖 Prompt for AI Agents
In backend/service/update_service_impl.go at line 94, the hardcoded "sleep 2"
delay is too short and should be made configurable or increased; replace the
literal sleep with a time-based sleep using a configurable duration (e.g., a new
config field or environment variable) that defaults to 5 seconds, parse and
validate the configured value (fallback to 5s on parse error), and use
time.Duration(value)*time.Second for the sleep; update any initialization to
expose the new config option and adjust/ add tests or documentation to reflect
the configurable grace period.
| // Current app bundle path (assuming /Applications installation) | ||
| currentAppPath := "/Applications/scanoss-cc.app" | ||
| log.Info().Msgf("Current app bundle: %s", currentAppPath) | ||
|
|
||
| // Verify the current app exists | ||
| if _, err := os.Stat(currentAppPath); err != nil { | ||
| return fmt.Errorf("current app not found at %s: %w", currentAppPath, err) | ||
| } |
There was a problem hiding this comment.
Derive app bundle path from running executable instead of hardcoding.
Line 480 hardcodes /Applications/scanoss-cc.app, which will fail for users who installed the app in a different location (e.g., ~/Applications, custom directories, or running from DMG). This is a repeat of the issue flagged in past reviews.
Apply this diff to derive the actual bundle path:
- // Current app bundle path (assuming /Applications installation)
- currentAppPath := "/Applications/scanoss-cc.app"
- log.Info().Msgf("Current app bundle: %s", currentAppPath)
-
- // Verify the current app exists
- if _, err := os.Stat(currentAppPath); err != nil {
- return fmt.Errorf("current app not found at %s: %w", currentAppPath, err)
- }
+ // Derive current app bundle path from running executable
+ currentExe, err := os.Executable()
+ if err != nil {
+ return fmt.Errorf("failed to get current executable: %w", err)
+ }
+
+ // Resolve symlinks
+ currentExe, err = filepath.EvalSymlinks(currentExe)
+ if err != nil {
+ return fmt.Errorf("failed to resolve executable path: %w", err)
+ }
+
+ // Navigate up to .app bundle (exe is in App.app/Contents/MacOS/)
+ currentAppPath := filepath.Dir(filepath.Dir(filepath.Dir(currentExe)))
+ if !strings.HasSuffix(currentAppPath, ".app") {
+ return fmt.Errorf("could not determine .app bundle path from executable: %s", currentExe)
+ }
+
+ log.Info().Msgf("Current app bundle: %s", currentAppPath)
+
+ // Verify the current app exists
+ if _, err := os.Stat(currentAppPath); err != nil {
+ return fmt.Errorf("current app bundle not accessible: %w", err)
+ }🤖 Prompt for AI Agents
In backend/service/update_service_impl.go around lines 479 to 486, the code
currently hardcodes "/Applications/scanoss-cc.app" which breaks for installs in
other locations; replace this with logic that derives the app bundle path from
the running executable: call os.Executable(), resolve symlinks with
filepath.EvalSymlinks, then walk up parent directories until you find a path
that ends with ".app" (or otherwise determine the bundle root) and use that as
currentAppPath; retain the existing os.Stat check and logging but log the
derived path and return an error if no .app bundle root can be found.
| // Backup path | ||
| backupPath := "/Applications/.scanoss-cc.app.backup" |
There was a problem hiding this comment.
Derive backup path from current app location.
The hardcoded backup path /Applications/.scanoss-cc.app.backup assumes the app is in /Applications. This will fail or create orphaned backups when the app is installed elsewhere.
Apply this diff:
- // Backup path
- backupPath := "/Applications/.scanoss-cc.app.backup"
+ // Backup path (same directory as current app)
+ backupPath := filepath.Join(filepath.Dir(currentAppPath), "."+filepath.Base(currentAppPath)+".backup")Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In backend/service/update_service_impl.go around lines 488-489, the backup path
is hardcoded to /Applications/.scanoss-cc.app.backup; change it to derive the
backup path from the running application's location: call os.Executable() to get
the executable path, use filepath.Dir to find the app directory, then build the
backup path by joining that directory with ".scanoss-cc.app.backup" (or place
the backup sibling to the executable as appropriate), and handle errors from
os.Executable() (logging/returning) so we don't proceed with an invalid path.
SCANOSS SCAN Completed 🚀
View more details on SCANOSS Action Summary |
Summary by CodeRabbit
Changed
Removed
Documentation