fix(connect): close TOCTOU race in connect sync lock acquisition

[omergk28]

Summary

loadState in internal/cli/connection/core/sync guarded ctx connect sync with a stat-then-write pair — racy by construction. Two processes racing past the os.Stat could both acquire, both load the same LastSequence, and both write duplicate entries into .context/hub/.

This PR replaces the pair with the atomic create-or-fail primitive the codebase already ships for exactly this purpose: io.SafeTryLock (O_CREATE|O_EXCL, one syscall) + io.SafeUnlock, with prior art at internal/cli/dream/core/pass/pass.go:62-70. !acquired maps to the pre-existing os.ErrExist contract, so caller-facing behavior is unchanged.

Closes #93.

Changes

• internal/cli/connection/core/sync/state.go — atomic lock acquisition; release delegates to SafeUnlock (warn-on-failure logging kept)
• internal/config/hub/{hub.go,doc.go} — LockSentinel removed: the lock is the file's existence, not its content, and the racy write was the constant's only consumer
• internal/cli/connection/core/sync/state_test.go — new; the package previously had zero tests
• specs/fix-connect-sync-lock-toctou.md — spec per the project's spec-per-commit invariant

Tests (mapped to the issue's Tests Required)

Issue ask	Test
N goroutines, exactly one succeeds, N−1 get os.ErrExist	TestLoadState_RejectsConcurrentSyncs (16-way; winners held until all results are collected, so the count is deterministic)
Lock released after failure so the next attempt proceeds	TestLoadState_ReleaseRemovesLock + TestLoadState_ReleasesLockOnCorruptState (post-acquisition failure must not leak the lock)
Lock lives at <ctxDir>/hub/.sync.lock	TestLoadState_LockFileLocation

Verification

• Mutation check: the new contention test was run against the old stat-then-write code — it fails with winners = 16, want exactly 1, proving both that the race is real and that the test catches it. Against the fix: exactly 1 winner, 15 × os.ErrExist.
• go test -race -count=10 ./internal/cli/connection/core/sync/ — clean.
• Full CI suite green (fmt, vet, golangci-lint, style, drift, docs, full test suite, shellcheck).

Notes

• One deliberate correction vs. the issue text: ctx connect sync lock has a TOCTOU race; concurrent syncs can both pass the check #93 says duplicates land in .context/shared/ — that's the pre-rename path; the renderer joins cfgHub.DirHub (render.go:32), so the spec and this PR say .context/hub/.
• Out of scope, per the issue: stale-lock detection (crashed process leaves a stale lock until a follow-up adds PID/age cleanup) and the flock alternative (Unix-only; SafeTryLock matches the existing sentinel model).
• Scope: the bash-3.2 make audit fix that previously rode along in this PR has been split into fix(hack): guard empty-array expansion in lint-drift for bash 3.2 #117, per the contributing guide's one-concern-per-PR preference. This PR is now surgical on ctx connect sync lock has a TOCTOU race; concurrent syncs can both pass the check #93. (Running make audit locally on stock-macOS bash 3.2 needs fix(hack): guard empty-array expansion in lint-drift for bash 3.2 #117; CI and Linux are unaffected.)

🤖 Generated with Claude Code

[josealekhine]

Thanks for your contributions @omergk28; as usual, your rock 🚀 .

Bottom line: textbook-correct concurrency fix, excellent tests.

The fix is correct, and I checked the part that matters

The old code was a genuine TOCTOU — os.Stat (check) then SafeWriteFile (use), with an unbounded window where two syncs both see "no lock" and both proceed:

// before
if _, statErr := os.Stat(lockPath); statErr == nil {
    return s, nil, os.ErrExist
}
if writeErr := io.SafeWriteFile(lockPath, []byte(cfgHub.LockSentinel), fs.PermFile); writeErr != nil {
    return s, nil, writeErr
}

The fix swaps it for io.SafeTryLock, and I confirmed that primitive is genuinely atomic rather than another stat-then-write:

// internal/io/security.go — the load-bearing primitive
f, openErr := os.OpenFile(clean, os.O_CREATE|os.O_EXCL|os.O_WRONLY, perm)
if openErr != nil {
    if os.IsExist(openErr) { return false, nil }   // already held → (false, nil)
    return false, openErr                            // real error → (false, err)
}
return true, f.Close()                               // acquired → (true, nil)

O_CREATE|O_EXCL is the canonical single-syscall create-or-fail, and the return semantics are exactly what state.go consumes:

// internal/cli/connection/core/sync/state.go — after
acquired, lockErr := io.SafeTryLock(lockPath, fs.PermFile)
if lockErr != nil {
    return s, nil, lockErr
}
if !acquired {
    return s, nil, os.ErrExist   // preserves the caller-facing contract
}

No window remains.

Things I checked that hold up

• Lock released on every post-acquire error path. loadState calls release() before returning on both the read-error and corrupt-JSON paths, so the lock can't leak (and TestLoadState_ReleasesLockOnCorruptState pins it).

• No nil-defer panic. The caller returns early on error before deferring release:

  // internal/cli/connection/core/sync/sync.go (Run)
  syncState, releaseLock, stateErr := loadState()
  if stateErr != nil {
      return stateErr     // returns before the defer; releaseLock is nil here
  }
  defer releaseLock()     // only runs when non-nil; lock held for the whole sync

• Clean removal. LockSentinel is dead after the change and fully removed (const + package-doc inventory); no dangling references anywhere in internal/.

• Reuses an established primitive. SafeTryLock/SafeUnlock already back internal/cli/dream/core/pass/pass.go:62-70, so this brings connect-sync in line with existing prior art rather than inventing a new lock.

• Lock and state files are distinct — FileSyncLock = ".sync.lock" vs FileSyncState = ".sync-state.json".

• Tests are excellent where there were none before. TestLoadState_RejectsConcurrentSyncs races 16 goroutines and asserts exactly 1 winner / 15 os.ErrExist, and deliberately holds the winner's lock until all attempts finish (so a late re-acquire can't skew the count). It passes under -race, stressed 5×. Plus release-removes-lock, release-on-corrupt-state, and lock-location tests.

---

Findings

1. Stale-lock wedge + opaque error

A crashed or SIGKILL'd sync leaves .sync.lock behind, and now that mutual exclusion is reliable, every subsequent sync hard-fails with os.ErrExist until the file is removed by hand. The spec acknowledges stale-lock recovery (PID-in-lockfile / age-based) as out-of-scope — fair — but making the lock correct raises the stakes of that deferred work. Compounding it, Run returns the raw os.ErrExist:

// sync.go
syncState, releaseLock, stateErr := loadState()
if stateErr != nil {
    return stateErr   // user sees "file already exists", no path hint
}

So a wedged lock surfaces as "file already exists" with no hint to remove <ctxDir>/hub/.sync.lock. The opaque message is pre-existing (the old code returned os.ErrExist too), but it's a cheap win to wrap it with the lock path + an "another sync is running, or remove the stale lock" hint — even ahead of full recovery. Low-medium; follow-up, not a blocker.

2. (theoretical) Lock leak if f.Close() fails after a successful create

In SafeTryLock, a create-succeeds-but-Close-fails returns (true, closeErr); the caller treats a non-nil error as "not acquired" and returns with no release func, yet the file was created → leaked lock. Close() on a freshly O_EXCL-created empty local file essentially never fails (only exotic ENOSPC-on-close / NFS). Pre-existing primitive; negligible. Footnote only.

3. (footnote) NFS caveat

O_EXCL atomicity holds on local filesystems and modern NFSv3+/v4; only ancient NFSv2 is unsafe. The lock lives in the project's .context/hub/, virtually always local. Negligible.