Fix running multiple nix-toolbox containers simultaneously#37
Open
Fix running multiple nix-toolbox containers simultaneously#37
Conversation
Previously, each new container checked `[ ! -e "/nix" ]` to decide whether to install Nix. Since `/nix` is a mount point that doesn't persist across containers, every new container attempted to re-run the installer against the shared Nix store, causing profile format conflicts (`nix-env` vs `nix profile`). Now the script: - Mounts the shared Nix store first, before checking installation state - Detects existing Nix installation via the profile script rather than the `/nix` mount point - Makes `nix.conf` creation idempotent Assisted-by: Claude Code Signed-off-by: Miroslav Vadkerti <mvadkert@redhat.com>
Reviewer's GuideEnsures the Nix store is always bind-mounted from persistent storage, detects Nix installation via the user profile instead of /nix, and makes flake configuration in nix.conf idempotent so multiple containers can safely share the same store. Sequence diagram for container startup with shared Nix storesequenceDiagram
participant Container
participant nix_sh as nix_sh_script
participant FS as Host_filesystem
participant NixInstaller
Container->>nix_sh: Run nix.sh on startup
nix_sh->>FS: Ensure $XDG_DATA_HOME/nix exists
nix_sh->>FS: Check /nix existence and type
alt /nix exists but not directory
nix_sh-->>Container: Print error and exit 1
else /nix dir ok
nix_sh->>FS: Create /nix if missing (sudo)
nix_sh->>FS: Bind-mount $XDG_DATA_HOME/nix to /nix if not mountpoint
nix_sh->>FS: Ensure /etc/nix/nix.conf enables flakes idempotently
nix_sh->>FS: Check for $HOME/.nix-profile/etc/profile.d/nix.sh
alt Profile script missing
nix_sh->>NixInstaller: Invoke single-user install
NixInstaller->>FS: Install Nix into shared store
end
nix_sh->>FS: Source profile script if present
nix_sh-->>Container: Nix environment ready
end
Flow diagram for updated Nix initialization and installation logicgraph TD
A_start["Start nix.sh"] --> B_ensureDataDir["Ensure $XDG_DATA_HOME/nix exists"]
B_ensureDataDir --> C_checkNixExists["Does /nix exist?"]
C_checkNixExists -->|yes| D_isDir["Is /nix a directory?"]
C_checkNixExists -->|no| F_createNixDir
D_isDir -->|no| E_errorExit["ERROR: /nix exists but is not a directory; exit 1"]
D_isDir -->|yes| G_checkMountpoint
F_createNixDir["Create /nix directory with sudo"] --> G_checkMountpoint["Is /nix a mountpoint?"]
G_checkMountpoint -->|yes| H_flakesConfig
G_checkMountpoint -->|no| I_bindMount["Bind-mount $XDG_DATA_HOME/nix to /nix (with sudo)"] --> H_flakesConfig
H_flakesConfig["Ensure /etc/nix/nix.conf enables flakes idempotently"] --> J_checkProfile["Does $HOME/.nix-profile/etc/profile.d/nix.sh exist?"]
J_checkProfile -->|no| K_installNix["Run Nix installer in single-user mode"] --> L_sourceEnv
J_checkProfile -->|yes| L_sourceEnv["Source nix.sh from user profile if present"]
L_sourceEnv --> M_end["Nix environment ready in container"]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The
mount | grep -q /nixcheck can produce false positives if another mount path contains/nix; consider matching the mountpoint more strictly (e.g.grep -q ' on /nix 'or usingfindmnt -M /nixif available). - The pattern
if [ ! -e "/nix" ]; then sudo mkdir /nix; fican still race between simultaneous containers; usingsudo mkdir -p /nixand ignoring the error if it already exists would be more robust for concurrent startup.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `mount | grep -q /nix` check can produce false positives if another mount path contains `/nix`; consider matching the mountpoint more strictly (e.g. `grep -q ' on /nix '` or using `findmnt -M /nix` if available).
- The pattern `if [ ! -e "/nix" ]; then sudo mkdir /nix; fi` can still race between simultaneous containers; using `sudo mkdir -p /nix` and ignoring the error if it already exists would be more robust for concurrent startup.
## Individual Comments
### Comment 1
<location path="nix.sh" line_range="37-42" />
<code_context>
-# Install nix with flakes if no Nix store found
+# Ensure /nix is bind-mounted from persistent storage
+mkdir -p "$XDG_DATA_HOME/nix"
if [ ! -e "/nix" ]; then
+ sudo mkdir /nix
+fi
+if ! mount | grep -q /nix; then
</code_context>
<issue_to_address>
**suggestion:** Consider handling the case where `/nix` exists but is not a directory.
`-e` will skip `mkdir` even if `/nix` is a regular file or symlink, leading to a confusing bind-mount failure. Use a directory-specific check like `if [ ! -d /nix ]; then` and explicitly error if `/nix` exists but is not a directory to make failures clearer.
```suggestion
# Ensure /nix is bind-mounted from persistent storage
mkdir -p "$XDG_DATA_HOME/nix"
# Ensure /nix exists as a directory; fail fast if it's some other type
if [ -e "/nix" ] && [ ! -d "/nix" ]; then
echo "Error: /nix exists but is not a directory; cannot bind-mount persistent Nix store." >&2
exit 1
fi
if [ ! -d "/nix" ]; then
sudo mkdir -p /nix
fi
if ! mount | grep -q /nix; then
```
</issue_to_address>
### Comment 2
<location path="nix.sh" line_range="42" />
<code_context>
if [ ! -e "/nix" ]; then
+ sudo mkdir /nix
+fi
+if ! mount | grep -q /nix; then
+ sudo mount --bind "$XDG_DATA_HOME/nix" /nix
+fi
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** The mount check via `grep -q /nix` can produce false positives.
Without anchoring, this can treat paths like `/nix-backup` as if `/nix` were mounted and skip the bind. Consider using `mountpoint -q /nix` or a stricter pattern like `mount | grep -qE ' on /nix( |$)'` to reliably detect the `/nix` mount.
```suggestion
if ! mountpoint -q /nix; then
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
- Check `/nix` is a directory, not just that it exists; error out if it's a file or symlink - Use `sudo mkdir -p` to avoid race between concurrent containers - Use `mountpoint -q /nix` instead of `mount | grep -q /nix` to avoid false positives from paths like `/nix-backup` Assisted-by: Claude Code Signed-off-by: Miroslav Vadkerti <mvadkert@redhat.com>
Owner
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The nix.conf handling only writes the file when it does not exist, so if an existing config is missing
experimental-features = nix-command flakesit will never be corrected; consider appending/updating that option when the file exists instead of skipping it entirely. - The
/nixbind mount assumes$XDG_DATA_HOMEis set and valid; it may be safer to ensure a default (e.g.,$HOME/.local/share) or fail with a clear error if it is unset before using it for the persistent store.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The nix.conf handling only writes the file when it does not exist, so if an existing config is missing `experimental-features = nix-command flakes` it will never be corrected; consider appending/updating that option when the file exists instead of skipping it entirely.
- The `/nix` bind mount assumes `$XDG_DATA_HOME` is set and valid; it may be safer to ensure a default (e.g., `$HOME/.local/share`) or fail with a clear error if it is unset before using it for the persistent store.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Check for the `nix-command` feature in existing `nix.conf` and rewrite it if missing, rather than skipping when the file exists. The `$XDG_DATA_HOME` concern from the review is already handled — it is defaulted to `$HOME/.local/share` on line 20 before any mount logic runs. Assisted-by: Claude Code Signed-off-by: Miroslav Vadkerti <mvadkert@redhat.com>
Collaborator
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The nix.conf handling is no longer idempotent with respect to existing user configuration: when the file exists but lacks
nix-command, the script overwrites it entirely; consider appending or editing just theexperimental-featuresline so other settings are preserved. - Before bind-mounting
$XDG_DATA_HOME/nix, it may be worth validating that the directory exists and is writable by the current user (and perhaps ensuring a sensible default ifXDG_DATA_HOMEis unset) to avoid subtle permission or env-related failures across containers.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The nix.conf handling is no longer idempotent with respect to existing user configuration: when the file exists but lacks `nix-command`, the script overwrites it entirely; consider appending or editing just the `experimental-features` line so other settings are preserved.
- Before bind-mounting `$XDG_DATA_HOME/nix`, it may be worth validating that the directory exists and is writable by the current user (and perhaps ensuring a sensible default if `XDG_DATA_HOME` is unset) to avoid subtle permission or env-related failures across containers.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
When `/etc/nix/nix.conf` already exists but lacks `nix-command`, append the experimental-features line instead of overwriting the file to preserve any existing user configuration. Assisted-by: Claude Code Signed-off-by: Miroslav Vadkerti <mvadkert@redhat.com>
Collaborator
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The
nix.confhandling only checks fornix-commandand may leave a profile withoutflakesenabled ifnix-commandis present butflakesis not; consider explicitly ensuring both flags are present in theexperimental-featuresline. - The bind-mount logic for
/nixcan still race if multiple containers start simultaneously (both might passmountpoint -q /nixand then try to mount); consider making the mount operation tolerant of an 'already mounted' error or re-checking the mountpoint after a failed mount.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `nix.conf` handling only checks for `nix-command` and may leave a profile without `flakes` enabled if `nix-command` is present but `flakes` is not; consider explicitly ensuring both flags are present in the `experimental-features` line.
- The bind-mount logic for `/nix` can still race if multiple containers start simultaneously (both might pass `mountpoint -q /nix` and then try to mount); consider making the mount operation tolerant of an 'already mounted' error or re-checking the mountpoint after a failed mount.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
- Tolerate mount race between concurrent containers: if `mount` fails, verify `/nix` is already a mountpoint before erroring out - Check for both `nix-command` and `flakes` in existing `nix.conf` to avoid leaving a partial configuration Assisted-by: Claude Code Signed-off-by: Miroslav Vadkerti <mvadkert@redhat.com>
Collaborator
Author
|
All feedback addressed in 2eef746. @sourcery-ai review — please do a thorough review of the full diff. |
Collaborator
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The bind-mount logic for /nix currently swallows failures (
sudo mount --bind ... || mountpoint -q /nix); it would be clearer and safer to fail fast with an explicit error if the mount command returns non-zero instead of just re-checking the mountpoint. - When appending the
experimental-featuresline to/etc/nix/nix.conf, you might want to ensure you don’t end up with multiple such lines over time (e.g., by normalizing or editing the existing line rather than always appending when the words are missing).
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The bind-mount logic for /nix currently swallows failures (`sudo mount --bind ... || mountpoint -q /nix`); it would be clearer and safer to fail fast with an explicit error if the mount command returns non-zero instead of just re-checking the mountpoint.
- When appending the `experimental-features` line to `/etc/nix/nix.conf`, you might want to ensure you don’t end up with multiple such lines over time (e.g., by normalizing or editing the existing line rather than always appending when the words are missing).Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
- Replace silent mount fallback with explicit error and exit - Remove existing `experimental-features` line before appending to prevent duplicates accumulating over time Assisted-by: Claude Code Signed-off-by: Miroslav Vadkerti <mvadkert@redhat.com>
Collaborator
Author
|
Both comments addressed in 9c2c816:
@sourcery-ai review Assisted-by: Claude Code |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes
nix-envvsnix profileformat conflict when running multiple nix-toolbox containers that share the same Nix store (~/.local/share/nix).Previously, each new container checked
[ ! -e "/nix" ]to decide whether to install Nix. Since/nixdoesn't persist across containers, every new container attempted to re-run the installer against the shared store, hitting:Now the script:
~/.nix-profile/etc/profile.d/nix.sh) rather than the/nixmount pointnix.confcreation idempotentAssisted-by: Claude Code
Summary by Sourcery
Ensure Nix is installed and configured based on the shared user profile rather than the /nix mount, so multiple nix-toolbox containers can safely share a persistent Nix store.
Bug Fixes:
Enhancements: