Skip to content

Preserve stable params lock ownership - #116

Open
FrogAi wants to merge 1 commit into
pfeiferj:mainfrom
FrogAi:codex/stabilize-params-locking
Open

Preserve stable params lock ownership#116
FrogAi wants to merge 1 commit into
pfeiferj:mainfrom
FrogAi:codex/stabilize-params-locking

Conversation

@FrogAi

@FrogAi FrogAi commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Developer summary

PutParam and RemoveParam currently unlink the shared .lock pathname while its inode may still be locked. Another process can then create and lock a different inode at the same path, entering the protected section concurrently. The same paths also leave temporary-file and directory descriptors to garbage collection.

This keeps the lock pathname stable, preserves the existing bounded retry policy, and closes each owned descriptor deterministically. The change stays inside params/params.go and does not alter exported APIs or normal parameter contents.

Verification

  • A separate-process lock holder reproduces the replacement-inode failure on current main; this branch returns the existing lock-acquisition error without changing the parameter.
  • A 64-put/64-remove lifecycle grows current-main descriptors by 192 and this branch by 0 with GC disabled.
  • The focused race matrix passes 10 consecutive runs; go test, race, vet, and build pass repository-wide on Linux/Go 1.25.1.

Compatibility

Parameter paths, payload bytes, atomic rename ordering, file/directory fsync ordering, retry count, retry delay, and exported APIs are unchanged. The lock file now persists and requests the same creation mode as FrogPilot's C++ owner.


Engineering record and audit trail

Root cause

Both persistence operations repeatedly call TryLock on the shared parent .lock. After the 31st failed attempt, current code removes that pathname and retries. It also removes the pathname after a successful acquisition, before the deferred unlock runs.

On Linux, unlinking a pathname does not release an advisory lock held through an open file descriptor. A later opener can create a new file at the same pathname, lock its different inode, and enter the critical section while the original lock remains held.

PutParam also leaves its temporary-file descriptor open after fsync, and both operations leave their synced directory descriptors open. Error paths after temporary-file creation have the same ownership gap.

Implementation

  • Stop unlinking the shared lock pathname during contention and after successful acquisition.
  • Keep the existing 51-attempt loop and 1 ms delay.
  • Request mode 0775 when the lock file is first created, matching the C++ owner of the same path.
  • Close the temporary file after its existing fsync and before lock acquisition/rename.
  • Close both containing-directory descriptors after their existing directory fsync.
  • Retain deferred cleanup for earlier error returns and propagate explicit close errors with context.

The pinned gofrs/flock implementation releases and closes its held descriptor when Unlock succeeds. Existing lock files are opened without truncation.

Red/green behavior

Case Main 1daead9 Rebased candidate
Put/replace/remove Deletes the stable lock pathname Retains the same pathname and inode
Separate process holds .lock through the retry window Put and remove succeed through a replacement inode Both return could not obtain lock; target is unchanged
64 puts + 64 removes, GC disabled Descriptors 12 -> 204 (+192) Descriptors 7 -> 7 (+0)
Lock-open error after temp creation Deleted temporary file remains open Temporary pathname and descriptor are released
Normal payload lifecycle Works Unchanged

The 192-descriptor increase is the exact ownership total for the exercised sequence: each put retained one temporary-file descriptor and one directory descriptor, and each remove retained one directory descriptor.

Final validation

The external matrix exercises the exported PutParam and RemoveParam functions in temporary Linux parameter trees. The final rebased production tree passed:

Check Result
Current-main focused matrix Expected failure in stable-path, exclusion, and descriptor cases
Candidate focused matrix Pass
Candidate focused race matrix, -count=10 Pass
go test ./... Pass
go test -race ./... Pass
go vet ./... Pass
go build ./... Pass
git diff --check Pass

Scope limits

The runtime matrix establishes Linux advisory-lock exclusion with a separate helper process and normal descriptor ownership through /proc/self/fd. It does not establish identical semantics on every OS, inject kernel close/fsync/LOCK_UN failures, redesign contention timing, or measure production contention frequency. Existing RemoveParam unlink-error handling and settings-save ordering are outside this change.

@FrogAi
FrogAi force-pushed the codex/stabilize-params-locking branch from edaa76f to 4a8942f Compare August 10, 2026 02:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant