CC @tomberek.

Thanks! A few comments:

Does this actually address safety issues? Optimistically proceeding with idempotent operations should generally be safe (e.g. downloading/substituting twice is wasteful but not a correctness problem). OTOH adding locking adds a lot of complexity, because now you need to worry about processes that are hanging, lock cleanup etc.

What sort of database corruption did you see with the eval cache? The intended behavior is to give up on updating the cache if we get a "busy" error (see 349d2c5).

Substitution used to be locked, but we lost that in aa3bc3d. Before then it was done like this: See here:

If we do reintroduce substitution locking, it should use the same lock file as used by LocalStore::addToStore(), since we don't want two different kinds of store path locks. However architecturally this is tricky because copyStorePath() doesn't know anything about LocalStore internals, so it can't lock paths. Maybe the Store interface needs to expose a method to lock a path.

Does this actually address safety issues? Optimistically proceeding with idempotent operations should generally be safe (e.g. downloading/substituting twice is wasteful but not a correctness problem).

That's a good distinction to make, I think I've conflated the two. Is there any way that correctness here would make assumptions about the behavior of the filesystem (e.g., fsync behavior)?

OTOH adding locking adds a lot of complexity, because now you need to worry about processes that are hanging, lock cleanup etc.

That is true, but (and I am biased here) I think that complexity is well worth it for the common case of running multiple Nix processes in parallel (like with nix-eval-jobs). The alternative (as far as I'm aware) is to leave it up to implementors building on top of Nix to somehow shim in and maintain such logic themselves.

What sort of database corruption did you see with the eval cache? The intended behavior is to give up on updating the cache if we get a "busy" error (see 349d2c5).

In hindsight, this could have been any number of things unrelated to multiple processes trying to interact with the database; I've been hacking on several ways of moving the functionality nix-eval-jobs provides back into the Nix CLI and some of them certainly did unsafe things with respect to the evaluation cache. My ZFS setup is also... nonstandard.

Substitution used to be locked, but we lost that in aa3bc3d. Before then it was done like this: See here:

nix/src/libstore/build.cc

Lines 3238 to 3243 in 21e9d18

	/* Acquire a lock on the output path. */
	outputLock = std::make_shared<PathLocks>();
	if (!outputLock->lockPaths(singleton<PathSet>(storePath), "", false)) {
	worker.waitForAWhile(shared_from_this());
	return;
	}

Thank you for the reference! I'll take a look when I get a chance. Do you have any historical context handy about why this functionality was removed?

If we do reintroduce substitution locking, it should use the same lock file as used by LocalStore::addToStore(), since we don't want two different kinds of store path locks. However architecturally this is tricky because copyStorePath() doesn't know anything about LocalStore internals, so it can't lock paths. Maybe the Store interface needs to expose a method to lock a path.

The thing I'm most interested in is deduplication of work across processes (so I guess process-safety is really a misnomer). I thought file-based locking would be a good way to do that; the only other alternative I could think of would be a shared-memory data structure (like through Boost). As you pointed out, introducing locking means introducing concerns about handling and cleaning up locking. Is there a simple way to do this that I'm missing?

Since there are really three different concerns here (allowing multiple Nix processes to interact with the evaluation cache, fetcher deduplication, and substitution deduplication) and the name of this PR is wrong, I'm leaning towards closing this PR and opening separate ones to address each concern individually. Thoughts?