[ndam-hexagon]

system-manager switch exits 0 even when it failed to write a file under /etc. The failure is only a warning, so the generation is marked active while /etc still has the old content.

Now those failures are collected during the walk and reported at the end, so one bad file still doesn't stop the others from landing. Files we deliberately leave alone stay non-fatal: an unmanaged path without replaceExisting, and the distro units under /etc/systemd/system we can't own, keep warning with a zero exit. Only a failed write counts.

Five bugs found on the way, one commit each:

• A directory sitting at a managed target was skipped forever: remove_file gives EISDIR, and renaming it into the backup slot leaves something that can't be restored. Now an empty directory is removed and the file created normally; a non-empty one is an error, not a recursive delete under /etc.
• backup_existing_file used fs::rename, which overwrites. If a .system-manager-backup was already there (e.g. after losing the state file), the real original was destroyed and deactivation put our own file back instead.
• A managed target missing from disk was always re-filed as a plain file, even if the old state had it backed up. Deactivation only restores backed-up files, so the original was stranded.
• The create_dir_all result was ignored, so a parent that can't be created showed up later as a confusing ENOENT.
• A warning printed the target in the source slot: Can't link/copy /etc/nix/nix.conf to : Is a directory.

The tests run create_etc_files against a temp /etc, so they don't need the cgroups and auto-allocate-uids features the container tests need. Four fail without the fixes. The two skip cases pass either way on purpose, to pin down what must not fail activation.

No existing issue covers this as far as I could find; happy to open one if you want the link.

🤖 Generated with Claude Code

[jfroche]

I think we should also handle the failed configured files/dir and retain their previous state. Otherwise they are treated as removed from the configuration, removed from the persisted state and will be considered unmanaged on the next activation.

[jfroche]

We could also handle tmp failure the same way.
I did something similar in #378 maintaining a list of error.
Maybe we could merge this feature here ?

[ndam-hexagon]

Held off on this one. With the early bail above, there is no longer a list of etc errors running alongside the tmp error to merge; the only accumulation left lives inside the etc walk itself. Tmp handling is untouched here.

A shared error list across etc/tmp/services still seems like the right shape, but #378 already has it, and growing a second mechanism here would just make that merge harder. Happy to rebase on top of #378 once it lands.

[jeremy]

We hit the same bug running system-manager unattended (an upgrade loop that switches, checks health and rolls back), and wrote a fix for /etc before finding this PR. Yours goes further, so I've dropped ours and opened #582 for the part you don't touch: refused systemd jobs, a failed userborn.service, and exit 4 for managed units that are failed after an otherwise complete switch, as NixOS' switch-to-configuration does. The two touch different code. Merged together they conflict only where both add a mod tests to the end of activate.rs.

Your tests run create_etc_files against a temp /etc. If an end-to-end check would help, we have a container test (testFlake/container-tests/) that runs bin/activate and checks the exit status. Adapted to your semantics, it would cover:

• a non-empty directory at a managed target: exit 1, the error names the path, the directory is left alone, and the profile's other /etc entries are still written;
• with the path freed, the same profile exits 0 and the target holds the managed content;
• an unmanaged file at an entry without replaceExisting: exit 0, with the file untouched.

I'm happy to push it to your branch or open it as a follow-up once this lands, whichever you prefer.

One question about the last case. We made an unmanaged file in the way exit 1 rather than warn. Our reasoning was that the entry wasn't applied, and a copy entry in the same spot was already an error at the file level. #378's VM test expects a failure there too. I can see the case for keeping it a warning: a distribution image that ships a file at a path a module manages without replaceExisting would go from a warning to a failed activation. Was that the reason, or is there another case you had in mind? Either way, it would help a caller if the warning stayed easy to find in the output, since the exit status won't flag it.

[ndam-hexagon]

We hit the same bug running system-manager unattended (an upgrade loop that switches, checks health and rolls back), and wrote a fix for /etc before finding this PR. Yours goes further, so I've dropped ours and opened #582 for the part you don't touch: refused systemd jobs, a failed userborn.service, and exit 4 for managed units that are failed after an otherwise complete switch, as NixOS' switch-to-configuration does. The two touch different code. Merged together they conflict only where both add a mod tests to the end of activate.rs.

Your tests run create_etc_files against a temp /etc. If an end-to-end check would help, we have a container test (testFlake/container-tests/) that runs bin/activate and checks the exit status. Adapted to your semantics, it would cover:

* a non-empty directory at a managed target: exit 1, the error names the path, the directory is left alone, and the profile's other `/etc` entries are still written;

* with the path freed, the same profile exits 0 and the target holds the managed content;

* an unmanaged file at an entry without `replaceExisting`: exit 0, with the file untouched.

I'm happy to push it to your branch or open it as a follow-up once this lands, whichever you prefer.

One question about the last case. We made an unmanaged file in the way exit 1 rather than warn. Our reasoning was that the entry wasn't applied, and a copy entry in the same spot was already an error at the file level. #378's VM test expects a failure there too. I can see the case for keeping it a warning: a distribution image that ships a file at a path a module manages without replaceExisting would go from a warning to a failed activation. Was that the reason, or is there another case you had in mind? Either way, it would help a caller if the warning stayed easy to find in the output, since the exit status won't flag it.

That is actually an oversight on my end. On main copy_file returned an error for this case and the symlink branch only warned, and I made both warn to line them up. Should have gone the other way.

Feel free to push the container test onto my branch. In the meantime, I dropped the no-replace entry from existing-files.nix : activation now stops before restarting services, so that test would have lost its systemd coverage. Your file is arguably the better place for that case.

I added a CHANGELOG entry since this change would now be a breaking change.

#582 LGTM

[jeremy]

Thanks! I don't have push access to your fork, so the container test is on mine: c395fee, branch jeremy/551-container-test, on top of your 03f5fdb. To take it: git pull https://github.com/jeremy/system-manager jeremy/551-container-test. It adds testFlake/container-tests/etc-activation-failures.nix. It's written against the branch as it stands at 03f5fdb and runs bin/activate end to end:

• A non-empty directory at a target the entry may replace (replaceExisting): exit 1, and the error names the path. The directory and its contents are left alone, and the profile's other entries are still written.
• The same profile once the path is free: exit 0. The target links to the managed content, and no backup is made.
• An unmanaged file at an entry without replaceExisting, both a linked entry and a copied one: the file is left alone, a warning names it, and the exit is 0. Deactivating leaves the file too.

The last case encodes the warning as it is today: the CHANGELOG entry and the existing-files.nix change you mentioned aren't on the branch yet. When you make it an error, change UNMANAGED_FILE_EXIT = 0 to 1; the warning-text assertions should still match if the message stays the same. Two other places assume the warning:

• testFlake/vm-tests/example.nix activates with an unmanaged /etc/foo_test in the way through vm.succeed(...) and then greps for the message. With an error, that becomes a vm.fail(...) (or an exit-status check).
• existing-files.nix's no-replace-test entry, which you said you'd dropped.

I ran it on your branch plus this commit on GitHub's hosted runners. It passes on Debian 13 and Ubuntu 24.04 and 26.04, and the rest of the container suite was green there too (served from the numtide cache for 03f5fdb). Ubuntu 22.04 I couldn't check: on those runners a 22.04 container never finishes booting (logind and networkd time out), for every test, whatever the branch. Your nixbot run will cover it.

On how the two PRs fit together: once #551 and #582 are both in, an /etc failure here and #582's ActivationFailure::Incomplete both exit 1. Your early return before the services phase also keeps #582's unit checks from running on a half-written /etc, which is what we'd want.

[jfroche]

Thank you both @ndam-hexagon and @jeremy for all your work on this. It really proves error handling needs some love.

I would merge this one before #582

After that, and based on @picnoir recommendation in #378 (review) , I will try to reduce the possibility to face these /etc issues using pre-flight checks. home-manager is also doing this.

I would also like to prevent write error using the idea behind atomicSymlink from nixpkgs. For the moment we remove the file then try to symlink, we should write to a temporary target and rename, it would make the operation more atomic. Ideal would be what @r-vdp described in #212 but that one is much larger.