mirror of
https://github.com/Twigpine/zero.git
synced 2026-11-11 23:40:28 -10:00
* fix(lock): prevent POSIX lock file overwrite and leak on Windows/Unix During lock reclamation, reclaiming a suspected stale lock renames the file aside and renames it back if not stale. On POSIX systems, the rename back silently overwrites any new lock file created in the gap. On Windows, it fails to rename back and leaks the stale file. Introduce a cross-platform restoreLockFile helper: - On POSIX: Use os.Link to link the file back (which atomic-fails if the target already exists), then remove the temporary link. - On Windows: Use os.Rename (which safely fails if the target exists). If restoring fails, delete the temporary file immediately to prevent leaks. * fix(lockutil): fix Windows build by using golang.org/x/sys/windows Addresses jatmn's follow-up review: the required Windows smoke check was failing because syscall.MoveFileEx is not exported by the standard library's syscall package on this platform, so internal/lockutil (and everything importing it: cron, daemon, hooks, oauth) failed to compile on Windows. Switched to golang.org/x/sys/windows, which does export MoveFileEx with the same signature, already a direct module dependency. Verified on a real Windows machine that the no-flags call still fails with ERROR_ALREADY_EXISTS when the destination exists, and that this satisfies errors.Is(err, os.ErrExist), which cron/daemon/hooks/oauth's reclaim paths all depend on to decide whether the sidelined lock was genuinely superseded (safe to discard) versus some other, unrelated failure (must be preserved). TestRestoreLockFile already covered both the successful-restore and fail-when-target-exists cases; it now passes on Windows. * fix(lockutil): route lock file deletes through RemoveLockFile with Windows retry loop * fix(lockutil): close restore split-brain and unify RemoveLockFile RestoreLockFile could fail with errors other than os.ErrExist (hardlink- incapable filesystems such as FAT or some FUSE/network mounts, ENOSPC, Windows sharing violations). Callers only clean up on os.ErrExist, so any other failure left the lock path missing with the live holder's lock stranded in the sidelined file; the next O_EXCL create then succeeded and two processes held the lock. RestoreLockFile now falls back to an O_EXCL copy in that case, which preserves the no-overwrite guarantee, and a successful restore with a failed sidelined-file cleanup reports nil instead of a failed restore. RemoveLockFile now has one cross-platform contract: removing an already missing file reports nil on POSIX too, matching Windows, and daemon release() drops its caller-side ErrNotExist filter. Tests: TestRestoreByCopy and TestRemoveLockFile run on all platforms; Windows-tagged tests pin the ERROR_ALREADY_EXISTS mapping to os.ErrExist and exercise the RemoveLockFile retry loop to success and to bounded exhaustion. The package doc comment documents the shared contract. * refactor(swarm): extract reclaimStaleLock to mirror other lock callers The stale-lock reclaim in acquireLock was inline, so its restore path (a holder rotated a fresh lock in the gap between the stale check and the rename) could not be regression-tested the way cron, daemon, hooks, and oauth test theirs. Extract it into reclaimStaleLock with the same shape as the other callers, behavior unchanged, and add the mirrored regression test. * test(hooks,oauth): add stale lock reclaim regression tests Mirror the cron reclaim test in the two packages that lacked coverage for reclaimStaleLock: a genuinely stale lock is reclaimed, a fresh lock (reacquired in the gap) is restored intact rather than stolen, and a missing lock reports no reclaim. daemon already had equivalent tests in lock_test.go. * fix(lock): fail closed and drop sidelined file when restore fails A non-os.ErrExist error from RestoreLockFile means both the no-replace restore and its O_EXCL copy fallback failed, so the lock path may be missing while the sidelined holder is live. Callers previously swallowed that error and their next O_EXCL create would succeed, breaking mutual exclusion. reclaimStaleLock in cron, daemon, hooks, oauth, and swarm now returns an error for that case and the acquire loops fail closed instead of re-acquiring. The sidelined .stale.* file is now removed on every restore failure, not only os.ErrExist: once the restore has failed it has no protocol function (release only consults the lock path), so keeping it only leaked files. Also replace the numeric errno literals in the Windows RemoveLockFile retry loop with windows.ERROR_SHARING_VIOLATION and windows.ERROR_ACCESS_DENIED. * refactor(lockutil): extract shared ReclaimStaleLock helper Consolidate the duplicated reclaimStaleLock functions from cron, hooks, oauth, and swarm into lockutil.ReclaimStaleLock. Each caller now passes an mtime-based isLive predicate to the shared helper, which owns the rename-aside, restore-if-live, cleanup, ErrExist handling, and fail-closed error propagation. Daemon keeps its own reclaimStaleLock because it uses PID-based liveness (different predicate shape and no staleAfter duration). The per-package reclaim regression tests are removed since the shared helper is thoroughly tested in lockutil/reclaim_test.go (dead lock reclaimed, live lock restored, missing lock, fail-closed on restore error, sidelined file dropped when new holder wins). * fix(lockutil): propagate non-contention rename failures from ReclaimStaleLock A rename-aside failure that is not a lost race previously mapped to the benign lost-race result, so acquirers spun until their deadline and reported a timeout instead of the real failure. Keep only a missing source and the Windows sharing-violation/access-denied contention errnos benign (neither Windows errno maps to os.ErrExist, and ERROR_SHARING_VIOLATION does not map to os.ErrPermission either, so the classification compares raw errnos) and return everything else so callers fail closed. Also convert daemon's private reclaimStaleLock to the shared helper so it picks up the same contract, and reword the caller error messages from "restore reclaimed" to "reclaim stale" since the error can now come from the rename aside as well. * fix(lockutil): publish restore-by-copy atomically and honor lost mailbox reclaims restoreByCopy created the destination lock file empty before copying into it, so a crash mid-copy could leave a live holder's lock permanently unrepresented at the lock path. Stage the copy under a private name and publish it with the platform's no-replace primitive instead. swarm's mailbox lock discarded the reclaimed boolean and always retried immediately, so a lost reclaim race (or Windows sharing-violation contention) could hot-spin past the lock timeout instead of falling through to the bounded wait like the other lock callers. * fix(lockutil): shrink the reclaim/restore window with a fast replacing rename Restoring a lock that turned out live went through the no-replace path (plus its copy fallback on some failures) first, which is slow enough that a fourth process's unrelated O_EXCL create could win lockPath while it was absent for the whole call. RestoreLockFile's no-replace contract would then report os.ErrExist and the caller silently dropped the sidelined lock, orphaning the still-running original holder. restoreLiveLock now tries a single replacing rename straight back first, falling back to the no-replace path only if that fails. It shrinks the window from a whole restore call down to roughly one syscall; it does not close it, since the fast path can silently overwrite a competing lock rather than detect it. A fully race-free reclaim would need an OS-level advisory lock checked non-destructively instead of by moving the file, which is a larger change than this.
117 lines
4.5 KiB
Go
117 lines
4.5 KiB
Go
package daemon
|
|
|
|
import (
|
|
"errors"
|
|
"fmt"
|
|
"io/fs"
|
|
"os"
|
|
"strconv"
|
|
"strings"
|
|
"sync/atomic"
|
|
|
|
"github.com/Gitlawb/zero/internal/lockutil"
|
|
)
|
|
|
|
// Single-instance lock. Mirrors reference-daemon-code-agent-js/supervisor.js's
|
|
// lock file: a PID file created with O_EXCL. A second start fails; a STALE lock
|
|
// left by a dead daemon (the recorded PID is no longer alive) is reclaimed so the
|
|
// daemon recovers from an unclean shutdown without manual cleanup.
|
|
|
|
// ErrAlreadyRunning is returned when a live daemon already holds the lock.
|
|
var ErrAlreadyRunning = errors.New("daemon: another instance is already running")
|
|
|
|
// fileLock is an acquired single-instance lock.
|
|
type fileLock struct {
|
|
path string
|
|
}
|
|
|
|
// processAlive reports whether pid is a live process. Implemented per-platform
|
|
// (lock_posix.go / lock_windows.go). It is a package var so tests can stub it.
|
|
var processAlive = osProcessAlive
|
|
|
|
// acquireLock takes the single-instance lock at path, reclaiming a stale lock
|
|
// whose recorded PID is dead. isAlive overrides the liveness check (tests pass a
|
|
// stub); nil uses the real processAlive.
|
|
func acquireLock(path string, isAlive func(pid int) bool) (*fileLock, error) {
|
|
if isAlive == nil {
|
|
isAlive = processAlive
|
|
}
|
|
// At most two passes: create, or detect-stale-then-retry once.
|
|
for attempt := 0; attempt < 2; attempt++ {
|
|
f, err := os.OpenFile(path, os.O_CREATE|os.O_EXCL|os.O_WRONLY, 0o600)
|
|
if err == nil {
|
|
// A failed PID write would leave a malformed lock file that another
|
|
// process reads as stale (unparsable PID) and wrongly reclaims, breaking
|
|
// the single-instance guarantee — so on write failure, remove it and fail.
|
|
if _, werr := fmt.Fprintf(f, "%d\n", os.Getpid()); werr != nil {
|
|
_ = f.Close()
|
|
_ = lockutil.RemoveLockFile(path)
|
|
return nil, werr
|
|
}
|
|
if cerr := f.Close(); cerr != nil {
|
|
_ = lockutil.RemoveLockFile(path)
|
|
return nil, cerr
|
|
}
|
|
return &fileLock{path: path}, nil
|
|
}
|
|
if !errors.Is(err, fs.ErrExist) && !errors.Is(err, os.ErrPermission) {
|
|
return nil, err
|
|
}
|
|
pid, perr := readPidFile(path)
|
|
if perr == nil && pid > 0 && isAlive(pid) {
|
|
return nil, fmt.Errorf("%w (pid %d)", ErrAlreadyRunning, pid)
|
|
}
|
|
// Stale lock (dead PID or unreadable) — reclaim it atomically, then retry the
|
|
// O_EXCL create. A blind Remove here races: two daemons starting at once could
|
|
// both read the stale PID, both Remove, and then one Removes the OTHER's
|
|
// freshly-created lock — leaving both "holding" the single-instance lock.
|
|
// reclaimStaleLock renames the file aside so only one racer wins the rename,
|
|
// and restores it if a live holder reacquired in the gap (D6).
|
|
if _, rerr := reclaimStaleLock(path, isAlive); rerr != nil {
|
|
// Reclaim hit a hard failure: the rename aside failed outright, or a
|
|
// live holder's lock could not be put back (the lock path may be
|
|
// missing, so re-acquiring would break the single-instance guarantee).
|
|
// Fail closed instead of spinning to the deadline.
|
|
return nil, fmt.Errorf("daemon: reclaim stale lock: %w", rerr)
|
|
}
|
|
}
|
|
return nil, ErrAlreadyRunning
|
|
}
|
|
|
|
// daemonLockSeq makes each reclaim attempt's sidelined filename unique per process.
|
|
var daemonLockSeq atomic.Uint64
|
|
|
|
// reclaimStaleLock atomically reclaims a single-instance lock whose recorded
|
|
// PID is dead, via lockutil.ReclaimStaleLock: the lock file is renamed aside
|
|
// (only one racer can win the rename) and stolen only if the moved file's PID
|
|
// is still dead; a holder that reacquired the lock in the gap between the
|
|
// stale check and the rename carries a LIVE pid and is restored rather than
|
|
// stolen. Returns true only when a genuinely stale lock was removed. A
|
|
// non-nil error means the caller must fail closed instead of re-acquiring
|
|
// (see lockutil.ReclaimStaleLock).
|
|
func reclaimStaleLock(path string, isAlive func(pid int) bool) (bool, error) {
|
|
suffix := fmt.Sprintf("%d-%d", os.Getpid(), daemonLockSeq.Add(1))
|
|
return lockutil.ReclaimStaleLock(path, suffix, func(reclaimedPath string) bool {
|
|
pid, err := readPidFile(reclaimedPath)
|
|
return err == nil && pid > 0 && isAlive(pid)
|
|
})
|
|
}
|
|
|
|
// release removes the lock file. Safe to call once. An already-missing lock
|
|
// file is not an error (lockutil.RemoveLockFile swallows it on every platform).
|
|
func (l *fileLock) release() error {
|
|
if l == nil || l.path == "" {
|
|
return nil
|
|
}
|
|
return lockutil.RemoveLockFile(l.path)
|
|
}
|
|
|
|
// readPidFile reads and parses the PID recorded in a lock file.
|
|
func readPidFile(path string) (int, error) {
|
|
data, err := os.ReadFile(path)
|
|
if err != nil {
|
|
return 0, err
|
|
}
|
|
return strconv.Atoi(strings.TrimSpace(string(data)))
|
|
}
|