refactor: one file lock for seven packages, not fourteen - #296
Merged
Conversation
internal/platform owned LockFileEx and nothing else, so the four names it exports existed only on Windows and each of the seven packages that lock a file carried its own flock(2) shim. The copies drifted: internal/secrets retries EINTR around the non-blocking lock and internal/oauthflow does not, so one signal delivered mid-syscall costs secrets a retry and makes oauthflow's offline refresh report a hard failure. filelock_unix.go answers LockFile, TryLockFile, UnlockFile and IsLockBusy with flock(2) and one EINTR policy — retry, below every entry point rather than in some of them — leaving syscall the only import, as the zero-dependency rule requires. filelock_other.go narrows to the platforms with neither implementation and keeps the fail-closed stand-ins there. Unix is the half that can actually be executed, so it gets the exclusion, blocking-wait and unknown-error tests the Windows branch has never had a machine to run. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Each of the seven packages that keeps a single-writer file carried a flock_unix.go and a flock_windows.go. The Windows halves already delegated to internal/platform and were identical; the Unix halves were hand-written and no longer agreed — internal/secrets retried EINTR, internal/oauthflow did not, internal/ratelimit had a third variant — so the same interrupted syscall meant different things in different packages, and the comments still called the copies structurally identical. Now that internal/platform answers both platforms, each package owns one flock.go tagged darwin || linux || windows, four lines over LockFile/TryLockFile/UnlockFile/IsLockBusy, and keeps flock_stub.go for everything else. Blocking-versus-polling is preserved per caller: calllog and ratelimit wait in the kernel, the other five poll a non-blocking attempt so they can honour a context. The parity test moves with the syscalls. It demanded a flock_windows.go beside every flock_unix.go; it now demands that every flock.go reach internal/platform and that nothing outside internal/platform call flock(2) or LockFileEx itself, which is the drift it could not see before. Stale prose goes with it: oauthflow's comment cited internal/integrity and promised Windows "in M2", both years out of date, and docs/windows.md counted six packages where seven take a lock. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
dinstein
force-pushed
the
refactor/platform-unix-flock
branch
from
August 10, 2026 03:30
f82bb5d to
a16dc0a
Compare
dinstein
marked this pull request as ready for review
August 10, 2026 03:30
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Seven packages hand-carried a
flock_unix.go/flock_windows.go/flock_stub.gotriplet. TheWindows halves were already one implementation in
internal/platformand byte-for-byte identical;the Unix halves were seven hand-written copies and had drifted —
internal/secretsretriedEINTRaround its non-blocking lock,internal/oauthflow's same-purpose function did not (so asignal delivered mid-syscall reported the offline refresh path as broken rather than retryable), and
internal/ratelimitwas a third variant. The comments still described the copies as structurallyidentical.
internal/platformgains the Unix half of the seam —filelock_unix.goanswersLockFile/TryLockFile/UnlockFile/IsLockBusywithflock(2)and one EINTRpolicy (retry, below every entry point).
filelock_other.gonarrows to the platforms withneither implementation.
syscallonly: the package is depguard-locked to$gostd.New tests exercise exclusion, the blocking wait and the busy predicate — Unix is the half that
can actually be executed, and the Windows branch has never had a machine to run on.
flock.goeach (taggeddarwin || linux || windows,four lines over
internal/platform) plus their existingflock_stub.go. Blocking-versus-polling preserved per caller:
calllogandratelimitwait in the kernel, the other five polla non-blocking attempt so they can honour a context. Stale prose removed with it —
oauthflowcitedinternal/integrity(retired) and promised Windows "in M2" (shipped), anddocs/windows.mdcounted six packages where seven take a lock.test/buildrules/flockparity_test.gomoves with the syscalls: it demanded aflock_windows.gobeside everyflock_unix.go; it now demands everyflock.goreachinternal/platformand fails any package outsideinternal/platformthat callssyscall.FlockorLockFileExitself. That second claim is the drift the old check could notsee.
The poll ladder: read, not hoisted
registry/lock.go,httpbridge/fileio.go,skills/lock.go— and, not in the original finding,secrets/vaultlock.goandoauthflow/refresh.go— carry five near-identicalopen / non-blocking-attempt / poll / deadline ladders. They are not hoisted here, and the reason is
that hoisting the wait loop is not a smaller change than it looks:
platformowns the loop, it also owns the non-blocking attempt and the busy predicate,so each package's
flockExclusiveNBandisWouldBlockbecome unreachable and the whole per-packagelock seam —
flock.go,flock_stub.go, and the three packages'crossProcessLockSupportedfail-closed prose — dissolves into
internal/platformwith it. That is 21 files and fourdocumented failure directions, on top of this branch's 34.
fileLocktype andLockTimeoutError(no store may import another's error model, andtest/buildrulespins eachsentinel into the CLI's exit-7 parity table). Every call site would keep an error-mapping switch.
So it deserves its own branch, judged on its own. Two things found while reading them, recorded here
rather than fixed:
oauthflowpolls with a timeout but returns plainfmt.Errorf, so lock contentionthere reaches the CLI as exit 1 while the identical contention in four other stores exits 7 —
and
locktimeoutregistry_test.go's comment claimed oauthflow "tries once", which it does not. Thecomment is corrected in this branch; the exit-code asymmetry is not.
Verification
make fmt,make ci, andmake cross-windows— the last because this branch touches the_windows.gofiles and cross-compilation is the only Windows gate that exists.