Fix bug with Lockfile sticking around

[bfops]

Description of Changes

Fixes #1339.

I updated Config::save to use an atomic write, so we no longer have the race condition that the Lockfile was originally created to address.

Correspondingly, I've removed Config's usage of Lockfile entirely.

API and ABI breaking changes

Nope

Expected complexity level and risk

2

Testing

Describe any testing you've done, and any testing you'd like your reviewers to do,
so that you're confident that all the changes work as expected!

• Test that spacetime identity add still works as expected
• CI passes

[bfops]

move this to the fs_utils crate

[gefjon]

I don't agree with removing the use of Lockfile - even with atomic writes, we can still get into situations where concurrent writers attempt to use the config leading to one of the sets of changes overwriting the other. This is admittedly less severe than the original bug, but it's still an incorrect behavior.

[bfops]

I don't agree with removing the use of Lockfile - even with atomic writes, we can still get into situations where concurrent writers attempt to use the config leading to one of the sets of changes overwriting the other. This is admittedly less severe than the original bug, but it's still an incorrect behavior.

While this is true, I don't see any subcommands that spend a "meaningful" amount of time between reading the config and writing it; it seems unlikely that e.g. two parallel calls to spacetime identity will happen on the same machine. Even so, with the atomic solution, one will just clobber the other's changes rather than creating an invalid file. It's pretty minimal fallout that happens in a specific kind of edge case (the user actively trying to run multiple config-mutating commands in parallel).

Meanwhile, the Lockfiles are causing active issues, and even if we hold the lock for less time, they're still prone to erroneous "locking out" of a user who does a completely valid Ctrl-C at the wrong time. (Who among us hasn't run a command only to immediately realize it was the wrong one?) I claim this is, at least from a UX perspective, more incorrect than potentially losing a config update because the user ran multiple config-mutating things in parallel. The surface area for the Lockfile issue is all commands, regardless of whether they mutate the config.

If we were to address this remaining config-clobbering issue, I would actually push to just have save check whether the config file has been changed since the initial read, and fail in that edge case. But, given that this case seems pretty unlikely, I'm not compelled to fix it now.

[bfops]

no longer needed since Config is back to being "plain ol' data"

[gefjon]

If we were to address this remaining config-clobbering issue, I would actually push to just have save check whether the config file has been changed since the initial read, and fail in that edge case.

This is insufficient because the CLI may have already performed observable side-effects by the time you abort.

But, given that this case seems pretty unlikely, I'm not compelled to fix it now.

🤷

[gefjon]

As I've said, I don't agree with this direction, but I'm willing to defer to you on the design here. Code is clear and does the thing it's supposed to.

[bfops]

If we were to address this remaining config-clobbering issue, I would actually push to just have save check whether the config file has been changed since the initial read, and fail in that edge case.

This is insufficient because the CLI may have already performed observable side-effects by the time you abort.

I agree with this as a general point, but I didn't see any obvious commands where this seemed like an unrecoverable loss. I also didn't look very hard - Are there specific side-effects you're thinking about / know of?

[bfops]

As I've said, I don't agree with this direction, but I'm willing to defer to you on the design here. Code is clear and does the thing it's supposed to.

I appreciate it! But I'm pretty new here - let's get someone else to weigh in before I just override your objections.

[cloutiertyler]

Coincidentally these types of problems were what databases were invented to solve.

I tend to agree with Phoebe on this one, but I would also not block this merge because I think this does fix 80% of the problem.

One potential way to fix the correctness issues would be to use file locks (the OS concept, not lock files) when reading and writing the file.

https://chatgpt.com/share/ce2f67f5-b277-4594-8f21-6c4235f4b288

[cloutiertyler]

I don't know that we should ever reintroduce Lockfile. I think we need a more general solution to the problem. The issue with Lockfile is that it may never be released.

[cloutiertyler]

LGTM