pidlock.Acquire read vaultik.pid, checked whether that PID was alive, and then wrote its own. Two writers started at the same moment (duplicate cron lines, or a timer plus a manual run) could both pass the check and both run, which the README's locking section says cannot happen.
The lock is now an exclusive, non-blocking flock on vaultik.pid, held while the file stays open for the whole run. The kernel drops it when the process exits for any reason, so the check for a stale PID is gone. The PID is still written to the file and still named in the "already running" error.
What the diff does not make obvious:
Release empties vaultik.pid and leaves it in place. Deleting it would let a process that opened the old file a moment earlier lock it while another creates and locks a new one, and both would run. TestAcquireAndRelease now expects an empty file instead of no file.
An flock belongs to the open file, not the process, so a second Acquire in the same process is refused too. The tests rely on that.
TestConcurrentAcquireAdmitsOne starts 50 Acquire calls together. Against the old code it fails only when the race is hit, not on every run; against the fix no two callers can both get the lock.
Judgement call: Release empties the file rather than leaving the last PID in it, so a clean exit leaves no PID behind.
Model: opus-5-5
Fixes https://git.eeqj.de/sneak/vaultik/issues/227.
`pidlock.Acquire` read `vaultik.pid`, checked whether that PID was alive, and then wrote its own. Two writers started at the same moment (duplicate cron lines, or a timer plus a manual run) could both pass the check and both run, which the README's locking section says cannot happen.
The lock is now an exclusive, non-blocking `flock` on `vaultik.pid`, held while the file stays open for the whole run. The kernel drops it when the process exits for any reason, so the check for a stale PID is gone. The PID is still written to the file and still named in the "already running" error.
What the diff does not make obvious:
- `Release` empties `vaultik.pid` and leaves it in place. Deleting it would let a process that opened the old file a moment earlier lock it while another creates and locks a new one, and both would run. `TestAcquireAndRelease` now expects an empty file instead of no file.
- An `flock` belongs to the open file, not the process, so a second `Acquire` in the same process is refused too. The tests rely on that.
- `TestConcurrentAcquireAdmitsOne` starts 50 `Acquire` calls together. Against the old code it fails only when the race is hit, not on every run; against the fix no two callers can both get the lock.
Judgement call: `Release` empties the file rather than leaving the last PID in it, so a clean exit leaves no PID behind.
Model: opus-5-5
clawbot
self-assigned this 2026-10-07 05:32:24 +02:00
Acquire read vaultik.pid, checked whether that PID was alive, then
wrote its own, so two writers started together could both pass the
check and both run. The lock is now an flock on vaultik.pid, held
while the file stays open; the kernel drops it when the process exits,
so the stale-PID check is gone.
Release empties the file instead of deleting it. Deleting it would let
a process that opened the old file a moment earlier lock it while
another creates and locks a new one.
The new concurrent test fails against the old code only when the race
is hit, not on every run; against the fix it cannot admit two callers.
Model: opus-5-5
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Fixes #227.
pidlock.Acquirereadvaultik.pid, checked whether that PID was alive, and then wrote its own. Two writers started at the same moment (duplicate cron lines, or a timer plus a manual run) could both pass the check and both run, which the README's locking section says cannot happen.The lock is now an exclusive, non-blocking
flockonvaultik.pid, held while the file stays open for the whole run. The kernel drops it when the process exits for any reason, so the check for a stale PID is gone. The PID is still written to the file and still named in the "already running" error.What the diff does not make obvious:
Releaseemptiesvaultik.pidand leaves it in place. Deleting it would let a process that opened the old file a moment earlier lock it while another creates and locks a new one, and both would run.TestAcquireAndReleasenow expects an empty file instead of no file.flockbelongs to the open file, not the process, so a secondAcquirein the same process is refused too. The tests rely on that.TestConcurrentAcquireAdmitsOnestarts 50Acquirecalls together. Against the old code it fails only when the race is hit, not on every run; against the fix no two callers can both get the lock.Judgement call:
Releaseempties the file rather than leaving the last PID in it, so a clean exit leaves no PID behind.Model: opus-5-5
Review passed.
Model: opus-5-5