fix(util): re-evaluate lock state in optimistic locking retry loop

[Norway-02]

fix(util): re-evaluate lock state in optimistic locking retry loop

What happened

The HAMi scheduler and device plugins rely on pkg/util/nodelock to implement a distributed node lock using Kubernetes node annotations (hami.io/mutex.lock).

Currently, SetNodeLock and ReleaseNodeLock correctly use an optimistic locking pattern via retry.OnError, but the condition checking occurs outside the retry function. Inside the retry.OnError block, the code blindly fetches the new resourceVersion and patches the node without evaluating whether the lock is still available (or still owned by the caller).

This leads to a race condition where concurrent scheduler instances blindly overwrite each other's locks upon a patch conflict.

What you expected to happen

The retry.OnError loop should explicitly re-evaluate the lock state after fetching the node (inside the loop). If the node is already locked by another process (for SetNodeLock) or the lock is no longer owned by the current pod (for ReleaseNodeLock), the retry loop should abort rather than blindly applying the patch.

How to reproduce it (as minimally and precisely as possible)

1. Run two instances of SetNodeLock concurrently for the same node.
2. The first instance patches the node successfully.
3. The second instance fails with a 409 Conflict.
4. The second instance's retry.OnError triggers, fetches the node, and unceremoniously overwrites the first instance's lock because it fails to check node.Annotations[NodeLockKey] inside the retry block.

Environment

• HAMi version: master

--
In a perfect world, AI assistance would produce equal or higher quality
work than any human. That isn't the world we live in today, and in many cases
AI-generated code can contain subtle bugs or not adhere to project-specific
best practices. I say this despite being a fan of and using them successfully
myself (with heavy supervision)!

When using AI assistance, we expect contributors to understand the code
that is produced and be able to answer critical questions about it. It
isn't a maintainer's job to review a PR so broken that it requires
significant rework to be acceptable.

Please be respectful to maintainers and disclose AI assistance.

AI Assistance Disclosure: This PR was identified and written with the assistance of an AI agent. The AI was used to identify the concurrency race condition within the optimistic locking retry logic, write the fix, and draft this PR description. The solution was fully reviewed, vetted, and manually verified.

Summary by CodeRabbit

• Bug Fixes
  • Improved node-lock acquisition when ownership changes during retries.
  • Prevented lock releases from removing another pod’s lock.
  • Treats safely aborted releases as successful no-ops.
  • Added handling for lock contention without unnecessary retries.

[hami-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Norway-02
Once this PR has been reviewed and has the lgtm label, please assign archlitchi for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 3e4b0562-c5af-40ff-a6c9-afa22d666229

📥 Commits

Reviewing files that changed from the base of the PR and between 83845fd and 79302a8.

📒 Files selected for processing (2)

• pkg/util/nodelock/nodelock.go
• pkg/util/nodelock/nodelock_test.go

🚧 Files skipped from review as they are similar to previous changes (1)

• pkg/util/nodelock/nodelock.go

---

📝 Walkthrough

Walkthrough

Node lock acquisition and release now refresh lock state during retries. Acquisition accepts an existing lock owned by the requester and stops on contention. Release aborts when ownership changes and treats the abort as successful.

Changes

Node lock retry handling

Layer / File(s)	Summary
Lock state rechecks and abort handling pkg/util/nodelock/nodelock.go, pkg/util/nodelock/nodelock_test.go	SetNodeLock refreshes annotations, preserves a lock owned by the requester, and stops on contention. ReleaseNodeLock rechecks ownership, preserves another pod’s lock, and treats ErrNodeLockAborted as a successful no-op. Tests cover both conflict scenarios.

Estimated code review effort: 2 (Simple) | ~15 minutes

Possibly related PRs

• Project-HAMi/HAMi#2066: Changes the same node-lock acquisition and release contention paths.
• Project-HAMi/HAMi#2197: Changes retry handling for concurrent node-lock updates.
• Project-HAMi/HAMi#2206: Refines node-lock contention errors and retry behavior.

Suggested reviewers: dsfans2014

Poem

A rabbit checks the lock with care,
And leaves another’s claim still there.
Retries pause when owners change,
Safe release keeps the state in range.
Conflicts end without despair.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: re-evaluating lock state during optimistic locking retries.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[Norway-02]

@DSFans2014 Hi! I've opened this PR to fix a critical concurrency race condition in the distributed node locking mechanism (pkg/util/nodelock). The retry.OnError loop was previously bypassing lock state validation upon a patch conflict, which could cause concurrent schedulers to blindly overwrite each other's locks. I've added the lock state check inside the retry block to ensure robust optimistic locking. Could you take a look when you have a chance? Thank you!

[mesutoezdil]

no ai assistance disclosure is present. if any ai tool was used, it must be disclosed per CONTRIBUTING.md: https://github.com/Project-HAMi/HAMi/blob/master/CONTRIBUTING.md#ai-assistance-notice

[Norway-02]

@mesutoezdil Thanks for the review! All feedback has been addressed:

1. Ran gofmt to fix the trailing whitespace.
2. Added a test in nodelock_test.go covering the new retry paths for both Set and Release node locks.
3. Formatted the AI assistance disclosure exactly as requested in CONTRIBUTING.md within the PR description block.
4. Added ErrNodeLockAborted as a sentinel error instead of abusing context.Canceled.
5. Fixed SetNodeLock to return nil if the lock is already owned by this pod to handle lost responses.

PTAL when you have a moment. Thanks!

[mesutoezdil]

closing per the comparison above, #2197 is the earlier and more complete base. pls check existing prs before opening a new one.