Session lock can admit two writers: replace the stale-takeover heuristic with an OS advisory lock

[kanushka]

Problem

internal/auth/session/lock.go guards refresh-token rotation so two wso2 processes cannot rotate one session at once — WithLock's own comment calls it "how refresh-token rotation stays single-writer across processes". Two processes can hold it at the same time.

The lock is the existence of a file, and O_EXCL creation is atomic, so the ordinary path is sound. The defect is in the stale-takeover path that exists so a lock abandoned by a crashed process does not block the reference forever:

os.Stat(path)     // older than lockStaleAfter?
os.Remove(path)   // yes — clear it
O_EXCL create     // claim it

Three syscalls, nothing atomic across them.

How two processes end up inside the lock

Both start while an abandoned lock file is present:

Step	Process A	Process B
1	Stat → 45s old, stale
2		Stat → 45s old, stale
3	Remove → the old file is gone
4	O_EXCL create → A holds the lock
5		Remove → deletes A's fresh lock
6		O_EXCL create → succeeds
7	running fn	running fn

Step 5 is the defect. Remove never checks that it is deleting the file that Stat examined. The lock file is empty — no PID, no inode comparison — so nothing distinguishes "the stale lock I decided to clear" from "the lock another process acquired 200µs ago". B silently evicts a live holder.

It then cascades: release is os.Remove(path), so when A finishes it deletes B's lock, and a third process can enter while B is still working.

tookOverStale allows one takeover per call, which bounds repetition but does not help here — both processes are on their first.

What it costs

WithLock wraps browserSource.mint (internal/auth/source_browser.go:61) and the login path (internal/app/login.go:136): load the session, refresh against the issuer, persist the rotated refresh token. Two of those interleaved, against an issuer that rotates:

• A refreshes with T and receives T'. B refreshes with T, which the issuer has now retired, and fails.
• Or both write. A stores T'; B stores a token derived from its stale read. The store is left holding a refresh token the issuer already replaced.

The second outcome is a dead session: the next command refuses auth.login_required and the user must log in again. That is exactly the corruption this lock exists to prevent.

Reproducing

Needs a lock file older than lockStaleAfter (30s) — a previous invocation crashed or was killed mid-refresh — and two invocations entering acquireLock within the same few milliseconds. Rare in ordinary use, but the trigger is a crash, and crashes cluster: a CI job retrying, or a script running commands in parallel.

TestWithLockSerializesWriters already covers the uncontended path, which is why this has not surfaced. Nothing exercises a stale file and a race together.

Fix

Replace the heuristic with an OS advisory lock — flock(2) on Unix, LockFileEx on Windows. The kernel ties ownership to an open file descriptor and releases it when the process exits, however it exits. There is then no stale lock and no takeover path to race, so the entire tookOverStale block is deleted rather than repaired.

Two decisions belong to whoever picks this up:

The dependency. github.com/gofrs/flock covers both platforms and is the obvious choice, but it would be a 5th direct dependency in a module that deliberately carries 4, all load-bearing. The alternative is syscall.Flock plus a Windows implementation, which is a small amount of platform-specific code the repo does not otherwise have.

The test that goes away. TestWithLockTakesOverStaleLock seeds an aged lock file and asserts takeover. Under an advisory lock that scenario cannot arise, so the test is not updated — it is removed, and its replacement is a concurrency test that runs several writers against one reference and asserts fn never overlaps. Removing a passing test needs the reason recorded in the commit, because the behavior it pinned is deliberately gone.

Also confirm the behavior when a lock is genuinely held: lockBusy() currently reports auth.login_required, chosen because the stable code list is closed and the recovery is the same. Worth re-reading once the acquisition path changes.

Acceptance

• Two processes cannot both be inside fn for one credential reference, with or without an abandoned lock file present.
• A process killed while holding the lock leaves nothing that blocks the next acquisition.
• A concurrency test fails against the current implementation, verified rather than assumed.

Found by review on #38, where it was deferred as a runtime change that did not belong in a release PR.