Conversation
Closes compscidr/sair-ochestrator#582 on the client side: a new int32 priority on AcquireLockRequest (higher first, FIFO within a value, 0 is today's behaviour), plumbed from sair-acquire through the proxy's HTTP API to the orchestrator. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
The new priority plumbing lacks a success-path HTTP API unit test verifying a valid ?priority= value is forwarded into the acquire call.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds a priority concept to SAIR lock acquisition so automated/bot jobs can wait behind human-driven jobs while devices are busy, by plumbing the value from sair-acquire → proxy HTTP API → proxy internals → orchestrator gRPC request.
Changes:
- Add
AcquireLockRequest.priority(int32) to the orchestrator proto and forward it from the proxy’s gRPC request. - Add
sair-acquire --priority Nwith 32-bit integer validation and send it as apriorityquery parameter. - Update docs and add/extend unit tests for priority validation/forwarding in the proxy layer.
File summaries
| File | Description |
|---|---|
| tools/sair-acquire | Adds --priority flag, validates int32 range, and forwards as priority query parameter |
| README.md | Documents --priority usage and a suggested GitHub Actions expression for bots |
| proto/orchestrator/orchestrator.proto | Adds AcquireLockRequest.priority field (int32, field 7) |
| internal/proxy/scoped_port.go | Extends ScopedPortManager.Acquire to accept/forward priority |
| internal/proxy/http_api.go | Parses priority query param (int32 range) and forwards into acquire call |
| internal/proxy/http_api_test.go | Adds validation test cases for invalid/oversized priority |
| internal/proxy/command_router.go | Extends AcquireLock to accept priority and set it on gRPC request |
| internal/proxy/command_router_test.go | Asserts priority is forwarded into AcquireLockRequest |
| internal/proxy/adb_connection_remote_test.go | Updates AcquireLock call sites for new signature |
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
75
to
80
| {"non-numeric count", "?count=abc", "count parameter must be a non-negative integer"}, | ||
| {"negative count", "?count=-1", "count parameter must be a non-negative integer"}, | ||
| {"count with serial", "?count=1&serial=DEVICE_A", "count and serial parameters are mutually exclusive"}, | ||
| {"non-numeric priority", "?priority=high", "priority parameter must be a 32-bit integer"}, | ||
| {"oversized priority", "?priority=2147483648", "priority parameter must be a 32-bit integer"}, | ||
| } |
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.
Client side of compscidr/sair-ochestrator#582.
AcquireLockRequest.priority(int32, field 7): higher is served first, ties are FIFO,0is exactly today's behaviour so existing callers are unaffected.sair-acquire --priority N, validated as a 32-bit integer, sent as thepriorityquery param.ScopedPortManager.Acquire→CommandRouter.AcquireLock→ gRPC, same shape asrun_url.Typical use in a workflow:
sair-acquire --count 1 --priority ${{ github.actor == 'dependabot[bot]' && -1 || 0 }}.The orchestrator side (queue ordering in
DeviceLockManager) follows in a separate PR that pulls this proto from main.🤖 Generated with Claude Code