Only allow once instance of fleet desktop at once #25821

dantecatalfamo · 2025-01-28T18:04:06Z

If some of the following don't apply, delete the relevant line.

Changes file added for user-visible changes in changes/, orbit/changes/ or ee/fleetd-chrome/changes.
See Changes files for more information.
A detailed QA plan exists on the associated ticket (if it isn't there, work with the product group's QA engineer to add it)
- Install fleet agent on machine
- Check logs for fleetd (journalctl -u orbit), somewhere in there you should see a line showing the arguments used to launch fleet desktop
- Run that command in another terminal, that should launch a second valid copy of fleet desktop
- See that it closes instantly instead of having two fleet task bar items
- If you check the fleet desktop logs (~/.local/state/Fleet/fleet-desktop.log), you should see mention of a lock file and that another instance of fleet desktop is active
- Special cases to check:
  - killing the fleet-desktop process with SIGTERM and SIGKILL
  - killing orbit with SIGTERM and SIGKILL.
  - restarting orbit the usual way in Linux (sudo systemctl restart orbit).
  - pushing a new version of fleet-desktop will properly auto-update it.
  - Test macOS and Windows fleetd restarts (because of changes (1) and (2)).
Manual QA for all new/changed functionality
For Orbit and Fleet Desktop changes:
- Orbit runs on macOS, Linux and Windows. Check if the orbit feature/bugfix should only apply to one platform (runtime.GOOS).
- Manual QA must be performed in the three main OSs, macOS, Windows and Linux.
- Auto-update manual QA, from released version of component to new version (see tools/tuf/test).

codecov · 2025-01-28T18:29:31Z

Codecov Report

Attention: Patch coverage is 0% with 20 lines in your changes missing coverage. Please review.

Project coverage is 63.86%. Comparing base (076fe89) to head (1a1be69).
Report is 6 commits behind head on main.

Files with missing lines	Patch %	Lines
orbit/cmd/orbit/orbit.go	0.00%	7 Missing ⚠️
orbit/pkg/platform/platform.go	0.00%	5 Missing ⚠️
orbit/pkg/platform/platform_notwindows.go	0.00%	5 Missing ⚠️
pkg/open/open_linux.go	0.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #25821      +/-   ##
==========================================
- Coverage   63.86%   63.86%   -0.01%     
==========================================
  Files        1658     1658              
  Lines      159079   159101      +22     
  Branches     4144     4144              
==========================================
+ Hits       101592   101605      +13     
- Misses      49550    49556       +6     
- Partials     7937     7940       +3

Flag	Coverage Δ
backend	`64.69% <0.00%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

lucasmrod

We already have some functionality to make sure there's only one fleet-desktop process running:

fleet/orbit/cmd/orbit/orbit.go

Lines 1603 to 1609 in da42457

    
           log.Info().Msg("killing any pre-existing fleet-desktop instances") 
        
           if err := platform.SignalProcessBeforeTerminate(constant.DesktopAppExecName); err != nil && 
        
           	!errors.Is(err, platform.ErrProcessNotFound) && 
        
           	!errors.Is(err, platform.ErrComChannelNotFound) { 
        
           	log.Error().Err(err).Msg("desktop early terminate") 
        
           }

Were you able to reproduce the issue?
Do we know why the current functionality is sometimes not working?

dantecatalfamo · 2025-02-07T15:31:56Z

@lucasmrod I wasn't able to recreate the exact situation the customer was having, but if you launch Fleet Desktop independently using the command line, it will happily open many instances. I'm not entirely sure what the issue was, but it was likely some sort of race condition.

dantecatalfamo · 2025-02-07T15:39:09Z

Getting fleet-desktop to check a lock should prevent any situation where its launched more than once, regardless of how it happened

orbit/cmd/desktop/desktop.go

lucasmrod · 2025-02-20T10:43:28Z

We met with @dantecatalfamo and discussed the following changes (apart from the lock file change):

Move platform.SignalProcessBeforeTerminate invocation to for loop instead before the actual execuser.Run execution.
Make platform.SignalProcessBeforeTerminate kill all instances of fleet-desktop (for Linux), not just one (if the method is used elsewhere then add another method).
Lock file implementation for fleet-desktop only present on Linux.
Add QA notes on the issue to explicitly test:

killing the fleet-desktop process with SIGTERM and SIGKILL
killing orbit with SIGTERM and SIGKILL.
restarting orbit the usual way in Linux (sudo systemctl restart orbit).
pushing a new version of fleet-desktop will properly auto-update it.
Test macOS and Windows fleetd restarts (because of changes (1) and (2)).

Co-authored-by: Lucas Manuel Rodriguez <[email protected]>

orbit/cmd/desktop/desktop.go

lucasmrod · 2025-02-20T17:44:44Z

orbit/pkg/platform/platform_notwindows.go

@@ -56,7 +56,7 @@ func SignalProcessBeforeTerminate(processName string) error {

 // GetProcessByName gets a single running process object by its name.


Let's update the docs (and rename to GetProcessesByName?).

lucasmrod

Pending fixing some build issues, otherwise LGTM!

orbit/pkg/platform/platform_notwindows.go

orbit/pkg/platform/platform_windows.go

Co-authored-by: Lucas Manuel Rodriguez <[email protected]>

lucasmrod · 2025-02-20T20:20:17Z

orbit/pkg/platform/platform_windows.go

@@ -142,7 +142,7 @@ func SignalProcessBeforeTerminate(processName string) error {
 	return nil
 }

-// GetProcessByName gets a single running process object by its name.
+// GetProcessesByName gets a single running process object by its name.
 // Returns ErrProcessNotFound if the process was not found running.
 func GetProcessByName(name string) (*gopsutil_process.Process, error) {


We'll also need to amend the Windows implementation to return all processes, right?

Or document that on Windows we return only one for now (with a TODO in case there are issues in the future)

It looks like it might cause issues, I'll see about making the Windows implementation return a list as well

lucasmrod

LGTM!

dantecatalfamo added 3 commits January 28, 2025 12:52

Get file locking working, lockfile was getting garbage collected

6221e64

Remove comments

ce165b0

Rename secureLock -> getLockfile

ee7404f

dantecatalfamo had a problem deploying to Docker Hub January 28, 2025 18:04 — with GitHub Actions Error

Add comment about the purpose of lockfile

95882a1

dantecatalfamo had a problem deploying to Docker Hub January 28, 2025 18:07 — with GitHub Actions Error

Add orbit/changes

7bf93ff

dantecatalfamo had a problem deploying to Docker Hub January 28, 2025 18:08 — with GitHub Actions Error

Please gofmt

570298d

dantecatalfamo temporarily deployed to Docker Hub January 28, 2025 18:20 — with GitHub Actions Inactive

Remove redundant log

b7dc482

dantecatalfamo temporarily deployed to Docker Hub January 28, 2025 20:39 — with GitHub Actions Inactive

Only check lockfile on linux

191feef

dantecatalfamo temporarily deployed to Docker Hub January 28, 2025 21:45 — with GitHub Actions Inactive

dantecatalfamo marked this pull request as ready for review January 28, 2025 21:49

dantecatalfamo requested a review from a team as a code owner January 28, 2025 21:49

dantecatalfamo assigned lucasmrod Jan 31, 2025

lucasmrod reviewed Feb 4, 2025

View reviewed changes

lucasmrod reviewed Feb 7, 2025

View reviewed changes

orbit/cmd/desktop/desktop.go Show resolved Hide resolved

lucasmrod reviewed Feb 7, 2025

View reviewed changes

orbit/cmd/desktop/desktop.go Show resolved Hide resolved

lucasmrod reviewed Feb 20, 2025

View reviewed changes

orbit/cmd/desktop/desktop.go Outdated Show resolved Hide resolved

Update orbit/cmd/desktop/desktop.go

aa345a9

Co-authored-by: Lucas Manuel Rodriguez <[email protected]>

dantecatalfamo temporarily deployed to Docker Hub February 20, 2025 15:46 — with GitHub Actions Inactive

Review comment fixes

8da1afe

dantecatalfamo had a problem deploying to Docker Hub February 20, 2025 16:11 — with GitHub Actions Failure

Merge branch 'main' into 25396-fleet-desktop-lockfile

b442b1f

dantecatalfamo had a problem deploying to Docker Hub February 20, 2025 16:27 — with GitHub Actions Failure

lucasmrod reviewed Feb 20, 2025

View reviewed changes

orbit/cmd/desktop/desktop.go Show resolved Hide resolved

lucasmrod reviewed Feb 20, 2025

View reviewed changes

Fix xwayland use of GetProcessByName

ed58e15

dantecatalfamo had a problem deploying to Docker Hub February 20, 2025 18:09 — with GitHub Actions Error

GetProcessByName -> GetProcessesByName

0e9c8bd

dantecatalfamo temporarily deployed to Docker Hub February 20, 2025 18:17 — with GitHub Actions Inactive

Overly ambitious find and replace

81e8fa3

dantecatalfamo temporarily deployed to Docker Hub February 20, 2025 18:28 — with GitHub Actions Inactive

lucasmrod reviewed Feb 20, 2025

View reviewed changes

orbit/pkg/platform/platform_notwindows.go Outdated Show resolved Hide resolved

lucasmrod reviewed Feb 20, 2025

View reviewed changes

orbit/pkg/platform/platform_windows.go Outdated Show resolved Hide resolved

Update orbit/pkg/platform/platform_notwindows.go

d723f50

Co-authored-by: Lucas Manuel Rodriguez <[email protected]>

dantecatalfamo temporarily deployed to Docker Hub February 20, 2025 20:17 — with GitHub Actions Inactive

lucasmrod reviewed Feb 20, 2025

View reviewed changes

Windows adjustment for GetProcessesByName

1a1be69

dantecatalfamo temporarily deployed to Docker Hub February 20, 2025 22:01 — with GitHub Actions Inactive

dantecatalfamo requested a review from lucasmrod February 20, 2025 22:04

lucasmrod approved these changes Feb 21, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Only allow once instance of fleet desktop at once #25821

Only allow once instance of fleet desktop at once #25821

dantecatalfamo commented Jan 28, 2025 •

edited

Loading

codecov bot commented Jan 28, 2025 •

edited

Loading

lucasmrod left a comment

dantecatalfamo commented Feb 7, 2025

dantecatalfamo commented Feb 7, 2025

lucasmrod commented Feb 20, 2025 •

edited

Loading

lucasmrod Feb 20, 2025 •

edited

Loading

lucasmrod left a comment

lucasmrod Feb 20, 2025

lucasmrod Feb 20, 2025

dantecatalfamo Feb 20, 2025

dantecatalfamo Feb 20, 2025

lucasmrod left a comment

	log.Info().Msg("killing any pre-existing fleet-desktop instances")

	if err := platform.SignalProcessBeforeTerminate(constant.DesktopAppExecName); err != nil &&
	!errors.Is(err, platform.ErrProcessNotFound) &&
	!errors.Is(err, platform.ErrComChannelNotFound) {
	log.Error().Err(err).Msg("desktop early terminate")
	}

		@@ -56,7 +56,7 @@ func SignalProcessBeforeTerminate(processName string) error {

		// GetProcessByName gets a single running process object by its name.

Only allow once instance of fleet desktop at once #25821

Are you sure you want to change the base?

Only allow once instance of fleet desktop at once #25821

Conversation

dantecatalfamo commented Jan 28, 2025 • edited Loading

codecov bot commented Jan 28, 2025 • edited Loading

Codecov Report

lucasmrod left a comment

Choose a reason for hiding this comment

dantecatalfamo commented Feb 7, 2025

dantecatalfamo commented Feb 7, 2025

lucasmrod commented Feb 20, 2025 • edited Loading

lucasmrod Feb 20, 2025 • edited Loading

Choose a reason for hiding this comment

lucasmrod left a comment

Choose a reason for hiding this comment

lucasmrod Feb 20, 2025

Choose a reason for hiding this comment

lucasmrod Feb 20, 2025

Choose a reason for hiding this comment

dantecatalfamo Feb 20, 2025

Choose a reason for hiding this comment

dantecatalfamo Feb 20, 2025

Choose a reason for hiding this comment

lucasmrod left a comment

Choose a reason for hiding this comment

dantecatalfamo commented Jan 28, 2025 •

edited

Loading

codecov bot commented Jan 28, 2025 •

edited

Loading

lucasmrod commented Feb 20, 2025 •

edited

Loading

lucasmrod Feb 20, 2025 •

edited

Loading