Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add windows service functionality #344 #567

Merged
merged 37 commits into from
Sep 23, 2024
Merged

Conversation

MolotovCherry
Copy link
Contributor

@MolotovCherry MolotovCherry commented Sep 2, 2024

This is an initial implementation of a Windows service for pueued. #344

This is a draft and I would like to have some testing/feedback on it before it gets merged.

How to use:
The following subcommands now exist:

  • pueued service install
  • pueued service uninstall
  • pueued service start
  • pueued service stop
  • pueued service run (internal only)

In an admin terminal:

  • pueued service install
  • Go to your services manager and start the service, or run pueued service start. It's set to autostart, so it will start up with the system, though that can obviously be adjusted to manual if the user wants. After the initial install, the service is not started automatically, so you must start it yourself the first time.
  • To uninstall pueued service uninstall

image

This should also respect it when users log out and other users log in.

The service process itself runs as SYSTEM. It spawns a new pueued daemon process as the currently logged in user.

What I want tested:

  • Just the general functionality, but specifically I want to know how it works when:
  • - user logs out
  • - another user logs in
  • - on bootup/restart

Todo / possible bug:

  • What happens if the daemon process fails on its own?
  • Adding tasks then trying to follow / log them / status, gives these weird "cannot deserialize" errors. Possibly caused by not enough perms? Should they be inherited then? - Edit: not my service's fault.
  • unpark inside stop after process killed so it doesn't get stuck on parked without ever being able to stop service
  • logoff/logon stops service
  • Fix daemon -d to actually detach in windows using process creation flags
  • Fix daemon in -d launching visible shell windows

Possible additional features:

  • Make -d mode check if windows service exists, if so, start and run the service instead of the daemon, otherwise, fallback to forking daemon mode.

Checklist

Please make sure the PR adheres to this project's standards:

  • I included a new entry to the CHANGELOG.md.
  • I checked cargo clippy and cargo fmt. The CI will fail otherwise anyway.
  • (If applicable) I added tests for this feature or adjusted existing tests.
  • (If applicable) I checked if anything in the wiki needs to be changed.*

@MolotovCherry MolotovCherry marked this pull request as draft September 2, 2024 06:41
Copy link

codecov bot commented Sep 2, 2024

Codecov Report

Attention: Patch coverage is 75.80645% with 15 lines in your changes missing coverage. Please review.

Project coverage is 78.27%. Comparing base (98ae0d2) to head (5115c68).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
pueue/src/bin/pueued.rs 75.80% 15 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #567   +/-   ##
=======================================
  Coverage   78.26%   78.27%           
=======================================
  Files          75       75           
  Lines        5835     5896   +61     
=======================================
+ Hits         4567     4615   +48     
- Misses       1268     1281   +13     

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

Copy link

github-actions bot commented Sep 2, 2024

Test Results

  3 files  ±0   19 suites  ±0   3m 5s ⏱️ +2s
161 tests ±0  161 ✅ ±0  0 💤 ±0  0 ❌ ±0 
340 runs  ±0  340 ✅ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 54d4bfd. ± Comparison against base commit 1c5f4aa.

♻️ This comment has been updated with latest results.

@MolotovCherry
Copy link
Contributor Author

MolotovCherry commented Sep 2, 2024

Ok, I think it's good to go for review now.

The service name and service description are up for debate. I can change them to whatever you want. Currently they are:

Name = pueued
Description = pueued daemon is a task management tool for sequential and parallel execution of long-running tasks.

@MolotovCherry
Copy link
Contributor Author

MolotovCherry commented Sep 4, 2024

I've also fixed -d on Windows. This command properly detaches from the parent process (with no console for parent or children), so closing the window is fine, follow then ctrl+c works fine now, etc

Since -d is fixed separately, technically the service itself probably isn't needed. A person could just set up a startup or taskscheduler job to make it run on startup instead.

I can nuke the service out of this PR if you want. The only benefit of the service over -d is that it automatically starts the daemon for any user that logs in, which means the user doesn't have to manually configure autostart for pueued (this is pretty nice tbh)

@Nukesor
Copy link
Owner

Nukesor commented Sep 4, 2024

Keep it in, I think that's the main reason why many people wanted to have the service in the first place :)

Code looks clean on well-documented on the first glance! Much appreciated. The Windows portion of Pueue needed a bit of love since quite some time.

Copy link
Owner

@Nukesor Nukesor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First up, thanks again :)

Overall clean code structure, well documented and readable code.
The system setup was certainly more involved than I expected, but I guess it also gives the service itself a lot more control over itself.

A few remarks, mostly nitpicks and call for more documentation

Disclaimer:
I really didn't read up on the internals on how Windows services and their privileges work. I only read through the code, which I obviously didn't always fully understand because of that. So I'll just trust you and hope that you'll be available if anything breaks in this part 😁

pueue/src/bin/pueued.rs Outdated Show resolved Hide resolved
pueue/src/bin/pueued.rs Outdated Show resolved Hide resolved
pueue/src/daemon/service.rs Outdated Show resolved Hide resolved
pueue/src/daemon/service.rs Outdated Show resolved Hide resolved
pueue/src/daemon/service.rs Outdated Show resolved Hide resolved
pueue/src/daemon/service.rs Show resolved Hide resolved
pueue/src/daemon/service.rs Outdated Show resolved Hide resolved
pueue/src/daemon/service.rs Outdated Show resolved Hide resolved
@MolotovCherry
Copy link
Contributor Author

MolotovCherry commented Sep 18, 2024

I really didn't read up on the internals on how Windows services and their privileges work. I only read through the code, which I obviously didn't always fully understand because of that. So I'll just trust you and hope that you'll be available if anything breaks in this part

To keep it simple, some api calls require the calling process has certain privileges (which the windows docs state which one). As this is running as SYSTEM, the privileges for this are probably already granted by default. This is just me being explicit for safety's sake (but even if it's not granted by default, this grants it, so we're all good). I don't anticipate there's any way the privileges can even be broken in real world usage. The call is pretty much guaranteed to succeed.

But yeah, if there's any issue reports, feel free to ping me and I'm glad to lend a hand. :)

One example:

WTSQueryUserToken(session_id, &mut query_token.0)?;

https://learn.microsoft.com/en-us/windows/win32/api/wtsapi32/nf-wtsapi32-wtsqueryusertoken
https://learn.microsoft.com/en-us/windows/win32/secauthz/privilege-constants

- Turns out `arguments` does not need extra buffer space
- We can use the Free trait for HANDLE drop
- And made the dirty code checking more concise (added some comment to make it clear though)
- Added more docs.
- Reordered some code.
- Remove noop stop() just to ensure no race condition can occur.
Copy link
Owner

@Nukesor Nukesor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Boi, that's what I call thorough documentation :D

Thanks a lot, whoever will take a look at this in the future will have all the resources they need to understand what's going on :) Much appreciated!

I've no further remarks, lets merge this!

@Nukesor Nukesor merged commit e73d2a5 into Nukesor:main Sep 23, 2024
11 of 18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants