-
Notifications
You must be signed in to change notification settings - Fork 0
fix: resolve degraded systemd user services #710
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -1,6 +1,9 @@ | ||||||
| { pkgs, ... }: | ||||||
| let | ||||||
| inherit (pkgs) lib; | ||||||
| startPostgresWrapper = pkgs.writeShellScript "start-postgres-wrapper" '' | ||||||
| exec /usr/bin/sg docker -c "${pkgs.bash}/bin/bash ${./start-postgres.sh}" | ||||||
|
||||||
| exec /usr/bin/sg docker -c "${pkgs.bash}/bin/bash ${./start-postgres.sh}" | |
| exec ${pkgs.bash}/bin/bash ${./start-postgres.sh} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P2: Hardcoding /usr/bin/sg creates a host system dependency that breaks Nix's reproducibility guarantees. This path may not exist on pure NixOS installations or systems where sg is installed elsewhere. Consider adding a runtime check that fails gracefully with an informative error if the path doesn't exist, or make the path configurable. At minimum, add a comment explaining why the SUID-enabled system sg is required instead of the Nix-packaged version.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At home-manager/services/docker-postgres/default.nix, line 5:
<comment>Hardcoding `/usr/bin/sg` creates a host system dependency that breaks Nix's reproducibility guarantees. This path may not exist on pure NixOS installations or systems where `sg` is installed elsewhere. Consider adding a runtime check that fails gracefully with an informative error if the path doesn't exist, or make the path configurable. At minimum, add a comment explaining why the SUID-enabled system `sg` is required instead of the Nix-packaged version.</comment>
<file context>
@@ -1,6 +1,9 @@
let
inherit (pkgs) lib;
+ startPostgresWrapper = pkgs.writeShellScript "start-postgres-wrapper" ''
+ exec /usr/bin/sg docker -c "${pkgs.bash}/bin/bash ${./start-postgres.sh}"
+ '';
in
</file context>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hardcoding /usr/bin/sg makes this configuration less portable and breaks the hermeticity that Nix aims for. It might fail on systems where sg is not at this path, such as a pure NixOS installation. While I understand from the PR description that this is a deliberate workaround for the SUID issue with the Nix-packaged sg, it's a trade-off worth highlighting. For better portability, you could consider making the path to sg configurable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using Restart = "on-failure" with Type = "oneshot" is unconventional. Oneshot services are designed to run once and exit successfully, then remain marked as active due to RemainAfterExit = true.
The restart logic may not behave as expected because:
- If the service succeeds, it won't restart (by design)
- If it fails during startup (e.g., Docker not ready), systemd will restart it
- If it fails after success (e.g., container stops), systemd won't restart it because the service already exited successfully
Consider:
- Changing to
Type = "simple"orType = "forking"if you need ongoing monitoring - Or, if the goal is just to retry on initial boot failures, document this specific use case in a comment
Prompt for Agent
Task: Address review feedback left on GitHub.
Repository: shunkakinoki/dotfiles#710
File: home-manager/services/docker-postgres/default.nix#L37
Action: Open this file location in your editor, inspect the highlighted code, and resolve the issue described below.
Feedback:
Using `Restart = "on-failure"` with `Type = "oneshot"` is unconventional. Oneshot services are designed to run once and exit successfully, then remain marked as active due to `RemainAfterExit = true`.
The restart logic may not behave as expected because:
- If the service succeeds, it won't restart (by design)
- If it fails during startup (e.g., Docker not ready), systemd will restart it
- If it fails after success (e.g., container stops), systemd won't restart it because the service already exited successfully
Consider:
1. Changing to `Type = "simple"` or `Type = "forking"` if you need ongoing monitoring
2. Or, if the goal is just to retry on initial boot failures, document this specific use case in a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hardcoding
/usr/bin/sgbreaks Nix's reproducibility guarantees and creates a hidden dependency on the host system. This path may not exist on all NixOS/Linux distributions. Consider alternative approaches:/usr/bin/sgdoesn't existIf
/usr/bin/sgis truly required, add a comment explaining why the SUID bit is necessary and why alternatives were rejected.Prompt for Agent