Fix acquire timeout release lock - #108
Open
jessicaochen wants to merge 2 commits into
Open
Conversation
|
Note Reviews pausedUse the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough
ChangesLock cancellation lifecycle
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Acquire as Server.Acquire
participant GroupStore
participant GroupSpec
participant MockWorkQueue
Acquire->>GroupStore: re-read group after context cancellation
GroupStore->>GroupSpec: CancelLockRequest(jobID)
GroupSpec-->>Acquire: report whether lock was released
Acquire->>MockWorkQueue: enqueue group controller work when released
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Collaborator
Author
|
Addresses #80 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/accelerator-orchestrator/server/server_internal_test.go (1)
195-265: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding coverage for the
cancelLockRequestfailure path.These
verifyAftercases cover the success paths of cancellation cleanup well, but there's no case wheregroup.Spec().CancelLockRequest(i.e.,lockStore.Unlock) fails — the scenario flagged in server.go where the lock could be left stuck.MockGroupLockStore.Unlockcould be extended to optionally return an error to exercise this.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/accelerator-orchestrator/server/server_internal_test.go` around lines 195 - 265, The cancellation tests do not cover cleanup when cancelLockRequest fails to unlock the group lock. Extend MockGroupLockStore.Unlock with an optional error path, configure it in a test for the cancellation-after-lock-granted scenario, and assert the expected failure behavior and resulting lock state through the existing verifyAfter checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/accelerator-orchestrator/server/server.go`:
- Around line 103-121: Add a bounded timeout to the detached context in
Server.cancelLockRequest before calling groupStore.Get and
group.Spec().CancelLockRequest. Derive the timeout context from
context.WithoutCancel(ctx), use the established cancellation/defer pattern, and
ensure both store operations receive the timed context so cleanup cannot block
indefinitely.
- Around line 117-127: Update the cancellation flow around CancelLockRequest to
retry transient unlock failures before returning, ensuring a failed
lockStore.Unlock does not leave lockingJob set without an automatic recovery
path. Reuse the existing retry or resync mechanism where available, and preserve
the current logging and EnqueueWork behavior after a successful release.
---
Nitpick comments:
In `@pkg/accelerator-orchestrator/server/server_internal_test.go`:
- Around line 195-265: The cancellation tests do not cover cleanup when
cancelLockRequest fails to unlock the group lock. Extend
MockGroupLockStore.Unlock with an optional error path, configure it in a test
for the cancellation-after-lock-granted scenario, and assert the expected
failure behavior and resulting lock state through the existing verifyAfter
checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: dd6128b3-de0c-4dc5-9f31-d2021edf4822
📒 Files selected for processing (6)
pkg/accelerator-orchestrator/server/server.gopkg/accelerator-orchestrator/server/server_internal_test.gopkg/accelerator-orchestrator/store/group.gopkg/accelerator-orchestrator/store/group_test.gopkg/accelerator-orchestrator/store/waiting_job_queue.gopkg/accelerator-orchestrator/store/waiting_job_queue_test.go
Comment on lines
+103
to
+121
| // cancelLockRequest undoes the lock request made by Acquire when the caller | ||
| // stops waiting, so the group is not left locked (or the job queued) for a | ||
| // caller that believes the acquire failed. | ||
| func (s *Server) cancelLockRequest(ctx context.Context, groupID, jobID string) { | ||
| // The request context is already cancelled; detach so the store updates can proceed. | ||
| ctx = context.WithoutCancel(ctx) | ||
|
|
||
| // Re-read group to get the latest status and spec from the store | ||
| group, err := s.groupStore.Get(ctx, groupID) | ||
| if err != nil { | ||
| slog.ErrorContext(ctx, "Failed to get group to cancel lock request", "error", err) | ||
| return | ||
| } | ||
|
|
||
| released, err := group.Spec().CancelLockRequest(ctx, jobID) | ||
| if err != nil { | ||
| slog.ErrorContext(ctx, "Failed to cancel lock request", "error", err) | ||
| return | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Detached work needs a timeout
context.WithoutCancel removes both cancellation and deadline, so groupStore.Get and CancelLockRequest can block indefinitely on a slow backing store while holding the group mutex. Wrap the detached context in WithTimeout before calling into the store.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/accelerator-orchestrator/server/server.go` around lines 103 - 121, Add a
bounded timeout to the detached context in Server.cancelLockRequest before
calling groupStore.Get and group.Spec().CancelLockRequest. Derive the timeout
context from context.WithoutCancel(ctx), use the established cancellation/defer
pattern, and ensure both store operations receive the timed context so cleanup
cannot block indefinitely.
Comment on lines
+117
to
+127
| released, err := group.Spec().CancelLockRequest(ctx, jobID) | ||
| if err != nil { | ||
| slog.ErrorContext(ctx, "Failed to cancel lock request", "error", err) | ||
| return | ||
| } | ||
| if released { | ||
| slog.InfoContext(ctx, "Released lock held by cancelled acquire") | ||
| if s.ctrl != nil { | ||
| s.ctrl.EnqueueWork(groupID) | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Files mentioning CancelLockRequest / lockingJob / fault ==\n'
rg -n --hidden --glob '!**/.git/**' 'CancelLockRequest|lockingJob|isGroupFaulted|faulted|lease|heartbeat|reconcile|reconciler' pkg
printf '\n== Candidate files ==\n'
git ls-files 'pkg/**' | rg 'accelerator-orchestrator|group\.go|server\.go|recon|fault|lease|heartbeat'
printf '\n== Outline of server.go and likely group file(s) ==\n'
ast-grep outline pkg/accelerator-orchestrator/server/server.go --view expanded || true
fd -a 'group.go' pkg || trueRepository: llm-d-incubation/llm-d-rl-time-slicing
Length of output: 20710
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== store/group.go relevant slice ==\n'
sed -n '150,320p' pkg/accelerator-orchestrator/store/group.go | cat -n
printf '\n== controller/controller.go relevant slice around reconcile and group state ==\n'
sed -n '200,520p' pkg/accelerator-orchestrator/controller/controller.go | cat -n
printf '\n== server/server.go fault handling slice ==\n'
sed -n '130,190p' pkg/accelerator-orchestrator/server/server.go | cat -nRepository: llm-d-incubation/llm-d-rl-time-slicing
Length of output: 21953
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== all unlock / CancelLockRequest call sites ==\n'
rg -n --hidden --glob '!**/.git/**' 'CancelLockRequest\(|\.unlock\(|Yield\(' pkg/accelerator-orchestrator
printf '\n== lock store implementations ==\n'
sed -n '1,220p' pkg/accelerator-orchestrator/store/configmap_lockstore.go | cat -n
printf '\n---\n'
sed -n '1,140p' pkg/accelerator-orchestrator/store/lock_memstore.go | cat -n
printf '\n== server Acquire/cancel path ==\n'
sed -n '59,130p' pkg/accelerator-orchestrator/server/server.go | cat -nRepository: llm-d-incubation/llm-d-rl-time-slicing
Length of output: 12468
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== controller queue / enqueue sources ==\n'
rg -n --hidden --glob '!**/.git/**' 'EnqueueWork|Run\(|NewController|workqueue|ticker|resync|periodic|reconcileAll|List\(\)' pkg/accelerator-orchestrator/controller
printf '\n== controller top-level structure ==\n'
sed -n '1,220p' pkg/accelerator-orchestrator/controller/controller.go | cat -nRepository: llm-d-incubation/llm-d-rl-time-slicing
Length of output: 24740
Retry CancelLockRequest before giving up. The periodic resync only requeues groups; it does not retry the unlock, so a transient lockStore.Unlock error can leave lockingJob set and block the queue with no automatic recovery path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/accelerator-orchestrator/server/server.go` around lines 117 - 127, Update
the cancellation flow around CancelLockRequest to retry transient unlock
failures before returning, ensuring a failed lockStore.Unlock does not leave
lockingJob set without an automatic recovery path. Reuse the existing retry or
resync mechanism where available, and preserve the current logging and
EnqueueWork behavior after a successful release.
Collaborator
Author
|
@coderabbitai ignore |
✅ Action performedReviews paused. |
|
This PR is marked as stale after 21d of inactivity. After an additional 14d of inactivity (7d to become rotten, then 7d more), it will be closed. To prevent this PR from being closed, add a comment or remove the |
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
2 participants
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.
What does this PR do?
Why is this change needed?
How was this tested?
Checklist
git commit -s) per DCOmake test)make lint)Related Issues
Summary by CodeRabbit
Bug Fixes
Tests