Repository navigation
fix(lock): replace the PID lockfile with an OS lock - #241
Merged
ashishkurmi merged 10 commits intoOct 17, 2026
Merged
ashishkurmi merged 10 commits into
ashishkurmi merged 10 commits into
Conversation
The PID-file lock could be wedged by a crashed root run (0600 file in sticky /tmp), a recycled PID, or any user writing a live PID into it, and was never released on os.Exit/SIGKILL. Unix: flock on /tmp/stepsecurity-dev-machine-guard.flock (new name, so legacy PID files are ignored), opened O_RDONLY|O_NOFOLLOW, mode 0644. Windows: Global\StepSecurityDevMachineGuard mutex open to every account, falling back to Local\. The OS releases both on any exit.
The failure report for a refused lock was sent before device.Gather, so device_id was empty and the backend rejected it with 400. Probe the serial before taking the lock, and prefix contention with lock_held:.
Holding the real /tmp lock would refuse an agent run on the same machine.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
🟡 Changes recommended
Unix acquisition can block on a planted FIFO, and the new tests have Windows naming and lock-lifetime defects.
3 open findings
What changed in this PR
Replaces PID lockfiles with kernel-managed locks to prevent stale locks from blocking agent runs.
Changes:
- Uses Unix
flockand Windows named mutexes. - Resolves device identity before locking and identifies contention reports.
- Adds lock lifecycle tests and updates watchdog comments.
| File | Description |
|---|---|
| internal/telemetry/telemetry.go | Resolves device identity before acquisition and labels contention. |
| internal/telemetry/run_lock_test.go | Tests contention reporting with device identity. |
| internal/lock/lock.go | Defines shared acquisition and idempotent release. |
| internal/lock/lock_windows.go | Implements named mutex locking. |
| internal/lock/lock_windows_test.go | Provides isolated test mutex names. |
| internal/lock/lock_unix.go | Implements persistent-file flock locking. |
| internal/lock/lock_unix_test.go | Tests file permissions and legacy-file handling. |
| internal/lock/lock_test.go | Tests contention, release, and process termination. |
| cmd/stepsecurity-dev-machine-guard/watchdog_windows.go | Updates comments about kernel lock cleanup. |
| cmd/stepsecurity-dev-machine-guard/watchdog_unix.go | Updates comments about kernel lock cleanup. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+51
to
+62
| const flags = syscall.O_RDONLY | syscall.O_NOFOLLOW | syscall.O_CLOEXEC | ||
| for range 2 { | ||
| fd, err := syscall.Open(path, flags, 0) | ||
| if errors.Is(err, syscall.ENOENT) { | ||
| fd, err = syscall.Open(path, flags|syscall.O_CREAT|syscall.O_EXCL, lockFileMode) | ||
| if errors.Is(err, syscall.EEXIST) { | ||
| continue // lost a create race; open the winner's file | ||
| } | ||
| } | ||
| if err != nil { | ||
| return nil, fmt.Errorf("opening lock file %s: %w", path, err) | ||
| } |
| // Hold a private instance lock so a real agent on this machine is unaffected. | ||
| target := filepath.Join(t.TempDir(), "test.flock") | ||
| if runtime.GOOS == "windows" { | ||
| target = fmt.Sprintf(`Local\dmg-lock-test-%d`, os.Getpid()) |
Comment on lines
+21
to
+24
| if _, err := Acquire(nil); err != nil { | ||
| fmt.Println("acquire failed:", err) | ||
| os.Exit(1) | ||
| } |
ashishkurmi
approved these changes
Oct 16, 2026
ashishkurmi
approved these changes
Oct 17, 2026
github-merge-queue
Bot
removed this pull request from the merge queue due to no response for status checks
Oct 17, 2026
…ock-flock # Conflicts: # cmd/stepsecurity-dev-machine-guard/watchdog_windows.go
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.
The PID lockfile could block every later run, so devices went inactive and never came back:
/tmp. User runs couldn't read it or delete it, so they were refused forever.%TEMP%survives reboots, so this persisted.1into the file and disable the agent.%TEMP%).Changes:
flockon/tmp/stepsecurity-dev-machine-guard.flock, a new name so legacy PID files are ignored. The file is openedO_RDONLY|O_NOFOLLOWwith mode 0644 so root and user runs can both open it, and it is never unlinked.Global\StepSecurityDevMachineGuardmutex any account can open, held on a locked OS thread.WAIT_ABANDONEDcounts as acquired. Falls back toLocal\.os.Exit.lock_held: ….Known limit: a local user can still hold the lock open and block runs. That is narrower than before, since it needs a live process; fully closing it needs a root-owned lock directory.
Testing:
go test ./...,go vetfor windows/linux/darwin. Linux lock tests also ran in a container, covering a root holder vs a user acquirer and reacquiring after SIGKILL. Not run on Windows.