Skip to content

libstore: skip Spotlight processes when finding GC roots #13051

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tomberek
Copy link
Contributor

@tomberek tomberek commented Apr 19, 2025

Modified from @winterqt 's patch:

Despite installations of Nix explicitly setting the nobrowse mount option on the store, mdworker processes continue to run on newly created files within it, which can result in spurious GC/path deletion failures if the stars are aligned right.

IMO, the best way to fix this is to simply ignore any processes started by the Spotlight user, so that's exactly what we do here.

Fixes: #6141

Makes an assumption of the Spotlight uid/gid.

Despite installations of Nix explicitly setting the `nobrowse` mount
option on the store, `mdworker` processes continue to run on newly
created files within it, which can result in spurious GC/path deletion
failures if the stars are aligned right.

IMO, the best way to fix this is to simply ignore any processes started
by the Spotlight user, so that's exactly what we do here.

Fixes: NixOS#6141
Change-Id: I6a6a636c70f3f443f07ba9280d5f1e04b2ed2985
@tomberek tomberek force-pushed the tomberek.fix_6141 branch from 0070b5f to c27b2a6 Compare April 19, 2025 08:44
auto lsofLines =
tokenizeString<std::vector<std::string>>(runProgram(LSOF, true, { "-n", "-w", "-F", "n" }), "\n");
tokenizeString<std::vector<std::string>>(runProgram(LSOF, true, { "-n", "-w", "-F", "n", "-u", "^89", "-g", "^89" }), "\n");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Echoing what @Qyriad said in https://gerrit.lix.systems/c/lix/+/3011/1/lix/libstore/platform/darwin.cc#53, the 89 should be pulled out into constant, with a name, and with (lots of!) docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still don't fully understand why excluding this user+processes from being GC roots makes GC more reliable.

Copy link
Member

@Mic92 Mic92 Apr 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume that uid/gid 89 is related to the mdworker? We should not check this process for live gcroots because that process shouldn't create a lock.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, this should only be done on macOS, since the uid 89 may refer to something else on other systems.

Copy link
Member

@grahamc grahamc Apr 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like lsof -u and -g supports taking names which I think would be better:
-u ^_spotlight -g ^_spotlight

grahamc@Grahams-MacBook-Pro detsys-ids-client % cat /etc/passwd | grep 89
_spotlight:*:89:89:Spotlight:/var/empty:/usr/bin/false
_biome:*:289:289:Biome:/var/db/biome:/usr/bin/false

grahamc@Grahams-MacBook-Pro detsys-ids-client % cat /etc/group  | grep 89
_spotlight:*:89:
_biome:*:289:_biome

ref:

       1) the `^' (negated) login name or user ID (UID), specified with the -u option;

       2) the `^' (negated) process ID (PID), specified with the -p option;

       3) the `^' (negated) process group ID (PGID), specified with the -g option;

       4) the `^' (negated) command, specified with the -c option;

       5) the (`^') negated TCP or UDP protocol state names, specified with the -s [p:s] option.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot delete store path with no roots
6 participants