Skip to content

refactor(posixage/flock): tidy tryLock structure and test name#527

Merged
Benehiko merged 1 commit into
mainfrom
posixage/lock-cleanup
May 26, 2026
Merged

refactor(posixage/flock): tidy tryLock structure and test name#527
Benehiko merged 1 commit into
mainfrom
posixage/lock-cleanup

Conversation

@Benehiko
Copy link
Copy Markdown
Member

Clean up follow-ups to the ctx-driven lock-wait refactor (#523):

  • Collapse the two sequential ctx.Err() checks into a single early-return gate before stale recovery. Equivalent behavior with a more linear read; the ctx is also checked again inside retryLock via backoff.Retry so the explicit re-check after recovery was redundant.

  • Clear fl immediately after calling recoverStaleLock so the outer close-on-error defer does not double-close the file handle (which recoverStaleLock already closes via its own defer).

  • Rename the test "caller context can wait past former default timeout" to "acquires lock when holder releases before ctx deadline" — the former-default-timeout reference no longer applies.

Clean up follow-ups to the ctx-driven lock-wait refactor (#523):

- Collapse the two sequential ctx.Err() checks into a single early-return
  gate before stale recovery. Equivalent behavior with a more linear
  read; the ctx is also checked again inside retryLock via backoff.Retry
  so the explicit re-check after recovery was redundant.

- Clear fl immediately after calling recoverStaleLock so the outer
  close-on-error defer does not double-close the file handle (which
  recoverStaleLock already closes via its own defer).

- Rename the test "caller context can wait past former default timeout"
  to "acquires lock when holder releases before ctx deadline" — the
  former-default-timeout reference no longer applies.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@Benehiko Benehiko linked an issue May 26, 2026 that may be closed by this pull request
Copy link
Copy Markdown

@docker-agent docker-agent Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assessment: 🟢 APPROVE

The refactoring is clean and correct:

  • Early ctx check: Moving ctx.Err() before recoverStaleLock correctly skips unnecessary file I/O when the context is already cancelled.
  • Double-close fix: Setting fl = nil unconditionally after recoverStaleLock(root, fl) correctly prevents the deferred close-on-error from double-closing a handle that recoverStaleLock already closes via its own defer. This is a genuine improvement over the previous code.
  • Removed second ctx check: Safe — retryLock respects context cancellation internally via backoff.Retry(ctx, …). The only subtle behaviour change is that a cancellation occurring in the narrow window between the early-exit check and retryLock now returns an error wrapping ErrLockUnsuccessful (via retryLock), whereas the old code returned a bare ctx.Err() for that specific path. This is arguably more consistent, and all existing tests already assert errors.Is(err, ErrLockUnsuccessful).
  • Test rename: Cosmetic improvement — the new name better describes the scenario being tested.

No resource leaks, nil dereferences, or logic errors found.

@Benehiko Benehiko merged commit edf93b4 into main May 26, 2026
28 of 29 checks passed
@Benehiko Benehiko deleted the posixage/lock-cleanup branch May 26, 2026 13:27
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.

Simplify posixage lock acquisition API

2 participants