nix-shell: Add temp roots for outputs

[jsoo1]

The garbage collector uses heuristics (i.e. /proc/*/environ) to check for live roots - meaning that a nix-shell should theoretically be safe from gc. In practice this detection does not always work. This change makes nix-shell spawn a child process and creates temproots for the shell.

[jsoo1]

This was very hard to make a test case for without ps at least:

$ git rev-parse HEAD
239f1e5249a9894a491a9a273f9f32d4ac88a330

$ $(nix-build --no-out-link)/bin/nix-shell -p jq --run '
  stat /nix/var/nix/temproots/$(pgrep -f "nix-daemon $PPID" || echo false)
'
  File: /nix/var/nix/temproots/3377944
  Size: 53206           Blocks: 104        IO Block: 4096   regular file
Device: 254,1   Inode: 143941455   Links: 1
Access: (0600/-rw-------)  Uid: (    0/    root)   Gid: (    0/    root)
Access: 2022-11-08 18:59:38.294816016 -0800
Modify: 2022-11-08 18:59:38.615815786 -0800
Change: 2022-11-08 18:59:38.615815786 -0800
 Birth: 2022-11-08 18:59:38.294816016 -0800

$ /run/current-system/sw/bin/nix-shell -p jq --run '
  stat /nix/var/nix/temproots/$(pgrep -f "nix-daemon $PPID" || echo false)
'
stat: cannot statx '/nix/var/nix/temproots/false': No such file or directory

[thufschmitt]

In practice this detection does not always work

Do you have some examples of it not working? Because if so that's annoying beyond just nix-shell

[jsoo1]

In practice this detection does not always work

Do you have some examples of it not working? Because if so that's annoying beyond just nix-shell

I think #2208 mentions a few. In our CI system we run into nix-shell interpreter script dependencies being gc'd pretty often when they overlap with a garbage collection. I mentioned that this might be papering over a bug here: #7248 (comment)

Nonetheless I think temproots are still worth creating for nix-shells since i'd rather not rely on the gc heuristic to catch everything in the shell.

Edit: The runtime checks like those checking /proc/*/environ are explicitly named as a heuristic. To make sure the garbage collector picks up roots for the shell, some roots ought to be created explicitly:

nix/src/libstore/gc.cc

Line 288 in ffca3e3

/* Add additional roots returned by different platforms-specific

[jsoo1]

I think this change also needs an ifdef for darwin.

[thufschmitt]

Mh, I couldn't find anything really relevant in #2208, it seems to really be related to keeping the gc root once the shell is closed (which is also something important that we've left aside for too long).

Nonetheless I think temproots are still worth creating for nix-shells since i'd rather not rely on the gc heuristic to catch everything in the shell.

I don't think quoting this as an heuristic is really fair here. It's heuristic in that a program could refer to a store path without it being in one of the searched paths, but it is a fairly reliable mechanism otherwise.

A possible explanation could be that this interacts with the non-blocking GC mechanism. It kinda requires new roots to be explicitely registered when the GC is running, and maybe the /proc scanning doesn't do it properly. @edolstra does that sound possible?

[jsoo1]

I don't think quoting this as an heuristic is really fair here. It's heuristic in that a program could refer to a store path without it being in one of the searched paths, but it is a fairly reliable mechanism otherwise.

I agree it checks most (if not all!) the relevant places. It seems very good actually!

But: It is explicitly called a heuristic in the commentary at least:

nix/src/libstore/gc.cc

Line 289 in ffca3e3

heuristics. This is typically used to add running programs to

The reason I think gc roots should be created explicitly has to do with a general philosophy I think is useful when making systems: prefer correct by construction over set building if possible.

The runtime checks in /proc and friends is a set/predicate kind of selection after the fact whereas the existence of gc roots for a shell may (should?) be part of the nix-shell introduction rule.

[edolstra]

I'm not in favor of this. Keeping the nix-shell parent around is potentially very expensive, since a Nix evaluation can consume gigabytes of memory. It would be much better to investigate/fix whatever the GC issue is.

[jsoo1]

a Nix evaluation can consume gigabytes of memory.

Ahh, right.

It would be much better to investigate/fix whatever the GC issue is.

I agree. If nix-shell is to be deprecated, perhaps this change is irrelevant anyways