fix: resolve degraded systemd user services - #710
Conversation
shunkakinoki
added
the
bug
- docker-postgres: Use /usr/bin/sg to run with docker group (Nix sg lacks SUID bit), add Restart=on-failure with 30s delay for resilience when Docker daemon starts slowly - make-updater: Add go and cargo to service PATH so Go and Rust projects can build - Remove multiclaude from .local-binaries.txt (directory does not exist)
Summary of ChangesHello @shunkakinoki, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on improving the reliability and functionality of systemd user services. It addresses critical permission issues preventing the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
📝 WalkthroughWalkthroughThree independent configuration updates to dotfiles: removal of a local binary entry, addition of a wrapper script and restart policies for the Docker Postgres systemd service, and expansion of the PATH environment for the make-updater service. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
Mesa DescriptionTL;DRResolved degraded systemd user services by fixing Docker socket permissions, updating build tool paths, and removing a non-existent binary entry. What changed?
Description generated by Mesa. Update settings |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request fixes several issues with systemd user services. For docker-postgres, it resolves a permissions issue by using a wrapper with sg to switch to the docker group and adds a restart policy for more resilience. For make-updater, it expands the PATH to include Go and Cargo toolchains. Finally, it cleans up an obsolete entry in .local-binaries.txt. The changes are well-reasoned. My main feedback is a comment on the docker-postgres service regarding a hardcoded path, which could impact portability.
| startPostgresWrapper = pkgs.writeShellScript "start-postgres-wrapper" '' | ||
| exec /usr/bin/sg docker -c "${pkgs.bash}/bin/bash ${./start-postgres.sh}" | ||
| ''; |
There was a problem hiding this comment.
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.
![mesa-dot-dev[bot]](https://avatars.githubusercontent.com/in/1050077?s=60&v=4){kind=small}
There was a problem hiding this comment.
Performed full review of ba452a4...42577b8
Analysis
-
Reproducibility Violation: Hardcoded dependency on
/usr/bin/sgbreaks Nix's core principle of self-contained, reproducible builds, creating an undeclared system dependency that may vary across distributions. -
Inconsistent Abstraction: The solution mixes Nix-managed paths with hardcoded system paths, resulting in an architecture that's difficult to maintain and reason about.
-
Service Configuration Issues: Possible misunderstanding of systemd service types, using
Restart = "on-failure"withType = "oneshot"indicates conceptual confusion about service lifecycle management. -
Architectural Debt: While functionally solving the immediate problem, the PR introduces technical debt by violating Nix principles that will need to be addressed in the future.
Tip
Help
Slash Commands:
/review- Request a full code review/review latest- Review only changes since the last review/describe- Generate PR description. This will update the PR body or issue comment depending on your configuration/help- Get help with Mesa commands and configuration options
0 files reviewed | 2 comments | Edit Agent Settings • Read Docs
| let | ||
| inherit (pkgs) lib; | ||
| startPostgresWrapper = pkgs.writeShellScript "start-postgres-wrapper" '' | ||
| exec /usr/bin/sg docker -c "${pkgs.bash}/bin/bash ${./start-postgres.sh}" |
There was a problem hiding this comment.
Hardcoding /usr/bin/sg breaks 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:
- Run the Docker daemon in rootless mode, eliminating the need for group switching
- Use a systemd system service instead of a user service, which would properly inherit group memberships
- Document this as a known limitation and add a runtime check to fail gracefully if
/usr/bin/sgdoesn't exist
If /usr/bin/sg is truly required, add a comment explaining why the SUID bit is necessary and why alternatives were rejected.
Prompt for Agent
Task: Address review feedback left on GitHub.
Repository: shunkakinoki/dotfiles#710
File: home-manager/services/docker-postgres/default.nix#L5
Action: Open this file location in your editor, inspect the highlighted code, and resolve the issue described below.
Feedback:
Hardcoding `/usr/bin/sg` breaks 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:
1. Run the Docker daemon in rootless mode, eliminating the need for group switching
2. Use a systemd system service instead of a user service, which would properly inherit group memberships
3. Document this as a known limitation and add a runtime check to fail gracefully if `/usr/bin/sg` doesn't exist
If `/usr/bin/sg` is truly required, add a comment explaining why the SUID bit is necessary and why alternatives were rejected.
| Service = { | ||
| Type = "oneshot"; | ||
| RemainAfterExit = true; | ||
| Restart = "on-failure"; |
There was a problem hiding this comment.
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.
Pull request overview
This PR addresses reliability issues with systemd user services on Linux, specifically fixing Docker socket permission issues and ensuring build tools are available for compilation tasks.
Changes:
- Fixed docker-postgres systemd service to use
/usr/bin/sgwrapper for proper Docker group permissions, with automatic restart on failure - Added Go and Cargo to make-updater service PATH to enable building Go and Rust projects
- Removed non-existent multiclaude binary entry from .local-binaries.txt
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| home-manager/services/docker-postgres/default.nix | Added wrapper script using /usr/bin/sg for Docker group switching and configured automatic restart on failure with 30s delay |
| home-manager/services/make-updater/default.nix | Added pkgs.cargo and pkgs.go to PATH to support building Rust and Go projects in the update script |
| .local-binaries.txt | Removed multiclaude entry that references a non-existent directory |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let | ||
| inherit (pkgs) lib; | ||
| startPostgresWrapper = pkgs.writeShellScript "start-postgres-wrapper" '' | ||
| exec /usr/bin/sg docker -c "${pkgs.bash}/bin/bash ${./start-postgres.sh}" |
There was a problem hiding this comment.
Using a hardcoded system path /usr/bin/sg creates a dependency on the system installation outside of Nix's control. This breaks Nix's reproducibility guarantees and will fail on systems where /usr/bin/sg doesn't exist or differs. While the PR description mentions that the Nix-packaged sg lacks the SUID bit, consider one of these alternatives:
- Add the user to the docker group in the system configuration rather than using sg
- Use systemd's
DynamicUserwith appropriate group permissions - Document that this is a system dependency requirement
This pattern is already used in cliproxyapi/default.nix (line 26), suggesting it's an accepted workaround in this codebase, but it's worth noting the limitation.
| exec /usr/bin/sg docker -c "${pkgs.bash}/bin/bash ${./start-postgres.sh}" | |
| exec ${pkgs.bash}/bin/bash ${./start-postgres.sh} |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 3 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="home-manager/services/docker-postgres/default.nix">
<violation number="1" location="home-manager/services/docker-postgres/default.nix:5">
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.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| let | ||
| inherit (pkgs) lib; | ||
| startPostgresWrapper = pkgs.writeShellScript "start-postgres-wrapper" '' | ||
| exec /usr/bin/sg docker -c "${pkgs.bash}/bin/bash ${./start-postgres.sh}" |
There was a problem hiding this comment.
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>
Changes
permission deniedon Docker socket in systemd user service by wrapping ExecStart with/usr/bin/sg docker(the Nix-packagedsglacks the SUID bit required for group switching). AddRestart=on-failurewith 30s delay so the service auto-retries when Docker daemon starts slowly at boot.pkgs.goandpkgs.cargoto the systemd service PATH so Go and Rust projects can build successfully.multiclaudeentry (directory does not exist).Technical Details
dockersupplementary group (GID 30001) even though the user is in the group — the user session was started before group membership was applied./usr/bin/sg(SUID) is used instead of${pkgs.shadow}/bin/sg(no SUID) becausenewgrp/sgrequires the setuid bit to switch groups.SupplementaryGroups=directive doesn't work in user-level systemd units (exit code 216).Test plan
make buildpassesmake formatpassesdocker-postgres.servicestarts successfully and Postgres accepts connectionsrunningwith zero failed unitsGenerated with Claude Code by Claude Opus 4.5
Summary by cubic
Fix degraded systemd user services by running docker-postgres with the docker group and adding retry on failure. make-updater now builds Go and Rust projects; removed a stale local binary entry.
Written for commit 42577b8. Summary will update on new commits.