[benjyw]

Concurrent Pants processes in the same buildroot would step on each
others' sandboxers because they all attempted to use the same socket path.

This ensures that each process has its own sandboxer socket path and therefore
its own dedicated sandboxer.

Also fixes a couple of flaky tests.

Fixes #23716

[benjyw]

LLM disclosure: Claude made the change under my guidance.

[hopper-signifyd]

I tested this out to see if it fixes #23716 and it gets us most of the way there. Seems like we need to also set max_readers to a higher value (like 2048) to fully resolve the issue.

I asked Claude to re-enable the sandboxer in our repo and then come up with a scenario that reproduces the issue we saw that's documented in the issue. This used Pants 2.32.0. For fun, I asked it to run that same scenario both against Pants main and then again with Pants with this PR.

Full response courtesy of Opus 5.5:

Claude says: Thanks, Benjy. I tested #23720 from source against main (bce3f09) on macOS. Each round ran 6 concurrent long pants --no-pantsd test --force runs plus 5 loops
of short test --force runs, each on a fresh isolated store, for 6 rounds.

#23720 alone made things worse in this setup: 27/36 long and 471/567 short runs failed, almost all with MDB_READERS_FULL, and some rounds hung. Per-process sandboxers mean many sandboxers read the store at once, and LMDB's reader table (126 slots per shard) is shared by every process. I saw 124 live sandboxer readers in one shard. The #23683 cap is per process, so it doesn't cover this. Once a read fails, the client's kill-and-respawn leaks slots. On macOS a process killed while holding LMDB's reader or write mutex wedges the store, because LMDB 0.9.24 uses non-robust POSIX semaphores there; a stack sample of a hung sandboxer showed it in sem_wait under mdb_reader_check.

#23720 plus .set_max_readers(2048) in ShardedLmdb::make_env: 0/36 and 0/245 failed, no hangs, no leaked slots. The lock file grows by about 128 KB per shard. One store test (remove_big_file_and_store_again) needs get_directory_size to skip lock.mdb, since the bigger lock files break its shrink assertion. Patch attached. Note that LMDB only resizes the reader table when no process has the store open, so existing stores pick it up after every pantsd stops once.

Optional hardening, which wasn't needed for this repro but covers Ctrl-C, OOM and SIGKILL deaths:

• Call mdb_reader_check when the store opens, and on MDB_READERS_FULL before retrying.
• Have the sandboxer drain in-flight requests and handle SIGINT, rather than calling exit(22) 50 ms after shutdown starts.

I have a tested patch for both if you want it. Also, the sandboxer integration tests are flaky on this Mac on main too (test_sandboxer_process failed 4 of 6 runs), and test_orphaned_sandboxer_exits failed both times I ran it on #23720.

minimal-max-readers.patch

[benjyw]

Thanks @hopper-signifyd - I've applied that patch to increase the reader slots (and slightly modified it).

[benjyw]

I'll take that patch for the "Optional hardening" if you have it

[hopper-signifyd]

I'll take that patch for the "Optional hardening" if you have it

I think it was one of these. Let me re-run my tests against the current state of the branch and get back to you

fix3-full.patch
check.patch

[hopper-signifyd]

Okay, I had Claude re-evaluate things. Looks like this branch in its current state solves the issues I'd originally reported. Ignore the patches I uploaded yesterday. This one is the new one Claude is suggesting:

sandboxer-hardening.patch

Full result output from Claude:

Claude says: I retested at 4d122ec, from source on macOS, with the same harness as before. The short version: your branch fixes the original issue, the patches uploaded above should be ignored, and there's a clean replacement attached that also fixes a sandboxer orphan race I found in this PR.

1. Your branch as-is passes the original repro. 6 rounds of 6 concurrent long test --force runs plus 5 loops of short runs: 0/36 long and 0/268 short runs failed, with no hangs and no leaked reader slots.

2. Please ignore fix3-full.patch / check.patch above (they're the same file). They predate your commit and conflict with your MAX_READERS and test-helper changes. They also carry a flate2 Cargo.lock bump that was only a workaround for our corporate proxy, and they put the new functions inside DEFAULT_LEASE_TIME's doc comment. The clean replacement is sandboxer-hardening.patch, which applies on 4d122ec. It contains:

• reclaim: mdb_reader_check when the store opens, and again on MDB_READERS_FULL before one retry.
• drain: the sandboxer handles SIGINT/SIGTERM, waits up to 5s for in-flight requests and then returns normally, instead of exit(22) 50ms after shutdown starts. The client's SIGINT→SIGKILL grace goes from 1s to 10s.
• --parent-pid: the fix for the race in (3).

Unit tests pass: sharded_lmdb 5/5, store 113/113, sandboxer lib 2/2.

3. Orphan race in this PR. serve() reads initial_parent_id = parent_id() only after process startup, socket cleanup, a 50ms sleep and bind(). If the parent is killed in that window, the sandboxer has already been reparented to launchd and records ppid 1 as its parent. It never sees a change, so it stays up until the 15-minute idle timeout, holding the store open. I observed exactly this: pants pid 35192 was SIGKILLed, and its sandboxer 35328 logged startup and was still serving with ppid 1 minutes later.

While an orphan like that holds the store open, LMDB never re-initializes the lock file, so dead readers pile up. Some of those slots pinned an old txnid, which also stops LMDB from reusing freed pages. The patch fixes this by having the client pass --parent-pid <its pid>, which the sandboxer uses instead of its late getppid(). A sandboxer started with a dead --parent-pid now exits after about 60ms.

4. Kill test, which is what the hardening is for. Each round runs 6 long runs ("bystanders") plus 3 loops of short runs ("victims"). Every 3s a random victim gets SIGINT or SIGKILL, alternating, to mimic Ctrl-C and OOM kills.

Build	Rounds	Victims signalled	Bystander failures	Hung rounds	Leaked reader slots after a round	Orphaned sandboxers left after a round
4d122ec	12	833	4/72 (all from the hang)	1/12	up to 449	yes (3 rounds)
+ reclaim + drain	10	860	9/60 (all from hangs)	2/10	0	yes (2 rounds)
+ reclaim + drain + --parent-pid (attached)	8	456	0/48	0/8	0	0

The attached version also passes the original repro: 0/36 long and 0/277 short runs failed.

5. The hangs are pre-existing and not caused by any of this. I took stack samples of every hung process. All of them were in sem_wait under mdb_txn_begin, coming from PersistentCache::new → create_db or from store_bytes. That means the write mutex was held by a pants client that had been SIGKILLed. On macOS, LMDB 0.9.24 uses POSIX semaphores, which aren't released when their owner dies, so the store stays wedged until every process using it exits. It happened both with and without the patch. The holder is a pants client, not the sandboxer, so I wouldn't claim the patch fixes it; its 0/8 is probably luck. A real fix would need to happen at the LMDB level, which is out of scope here, but it's worth knowing that kill -9 on a pants process during a write can hang every other pants process on macOS.

Two small notes:

• The new MAX_READERS comment says each process is capped at 120 readers. The sandboxer builds its store on Executor::new() inside #[tokio::main], so it gets tokio's default of 512 blocking threads and isn't covered by MAX_BLOCKING_THREADS_LMDB_SAFE. 4096 slots still covers this, but either the comment or the sandboxer's runtime should change.
• The sandboxer integration tests fail on this Mac at 4d122ec with or without the patch. materialize_directory returns Err("transport error") in test_sandboxer_process, and test_orphaned_sandboxer_exits is flaky. That looks environmental rather than caused by either change.

[benjyw]

Thanks. I already took my own stab at the "hardening", only part of it though, since the connection draining is a lot of new code and doesn't seem that necessary.

[benjyw]

Previously we assumed that the pid we capture when we first start up is the parent pid, but we might get reparented even before that happens, so we now receive it on startup from the parent.

[benjyw]

cc reviewers, thanks!

[tobni]

Looks sane to me!