Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

Haven't really tested this yet. We should reproduce the race in that bug and sanity-check it fixes it before we do this.

Of course, there's an argument to be made here that rebooting right after staging is silly since you could just as well have not used staging. But we also shouldn't be prone to races in the default deployment path.

Of course, there's an argument to be made here that rebooting right after staging is silly since you could just as well have not used staging.

I wouldn't want to recommend that because it re-introduces the possibility of races if there's anything else running at the time that wants to serialize state to /etc (not that there should generally be such things, but still).

But we also shouldn't be prone to races in the default deployment path.

IOW yes this - I think we should always use staging, and we should definitely fix this race.

(That said there is a longer term thing I think we talked about at some point by moving the finalization into the initramfs, i.e. pivoting back to the initramfs at shutdown, which would also have avoided any race like this)

I have nothing against this change, but how does it fix the race? Whether you trigger via a path systemd is monitoring or you call systemctl (which makes a dbus call), there's still a window between creating the staged deployment and enqueuing ostree-finalize-staged.service, isn't there?

I'd think that the only way to truly fix this issue would be to inhibit shutdown during the staging process.

I have nothing against this change, but how does it fix the race? Whether you trigger via a path systemd is monitoring or you call systemctl (which makes a dbus call), there's still a window between creating the staged deployment and enqueuing ostree-finalize-staged.service, isn't there?

We invoke systemctl start ostree-finalize-staged.service before we do the staging. The invocation will not return until the unit is active. So there's no longer a window during which we staged something and the service isn't already active.

(The more important fix in here though isn't really that we do it before, but just that we do it at all synchronously as part of the staging API call. That forces the client calling into it to not move on to initiating a reboot -- since it's likely the same software -- until we're guaranteed the service is active instead of relying on the path unit and its dependency tree to have kicked in.)

I think it's less risky if we only have the change in sysroot-deploy to synchronously start -finalize-staged.service. It shouldn't hurt to still have the .path unit attempting to also queue the start, right?

But, I dunno, I'm fine with this too, it is perhaps clearer to only have one way it's done.

Could go either way on this. I guess if we were to backport, I would agree it'd be less risky to just backport the systemctl start bit. Maybe I split this commit in two to make that easier? OTOH, I'm not sure it's worth backporting either. The workaround for the specific client affected here doesn't seem too painful.

OK, this is trivial to reproduce actually by just adding a dropin for ostree-finalize-staged.service with

[Service]
ExecStart=sleep 10

And indeed, this patch fixes the issue.