feat(api): enforce REST semantics for worker endpoints - #875
Conversation
- POST /workers now returns 409 Conflict if a worker with the same
URL already exists, with a message directing to PUT/PATCH
- PUT /workers/{id} does full replace (re-runs registration workflow
with model discovery via register_or_replace)
- PATCH /workers/{id} does partial update (moved from PUT, same
WorkerUpdateRequest behavior)
- DELETE /workers/{id} unchanged
- Add Conflict variant to WorkerServiceError
- Add replace_worker() to WorkerService
Signed-off-by: Chang Su <chang.s.su@oracle.com>
The 409 check now happens in the service layer (create_worker). Signed-off-by: Chang Su <chang.s.su@oracle.com>
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the worker API to align with standard REST principles, ensuring that Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds REST semantics: POST now returns 409 for duplicate worker URLs, PUT /workers/{id} is a full replace (validates worker-id ↔ URL) that enqueues AddWorker, PATCH remains for partial updates, and a docstring TODO was removed from WorkerRegistry::reserve_id_for_url. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server as "Server (HTTP)"
participant Service as "WorkerService"
participant Registry as "WorkerRegistry"
participant Queue as "Job Queue"
Client->>Server: PUT /workers/{worker_id} (WorkerSpec)
Server->>Service: replace_worker(worker_id, config)
Service->>Service: parse & normalize worker_id
Service->>Registry: get_worker_by_id(worker_id)
Registry-->>Service: existing_worker_url / NotFound
alt NotFound
Service-->>Server: 404 Not Found
Server-->>Client: 404
else Found
Service->>Service: validate config.url == existing_worker_url
alt URL mismatch
Service-->>Server: BadRequest (400)
Server-->>Client: 400
else Match
Service->>Queue: submit Job::AddWorker(config)
Queue-->>Service: accepted
Service-->>Server: UpdateWorkerResult{worker_id, url}
Server-->>Client: 200/202 OK
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Check for active worker using the reserved ID instead of calling get_by_url then reserve_id_for_url separately. Signed-off-by: Chang Su <chang.s.su@oracle.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the worker API endpoints to align with REST semantics, introducing PUT for full replacement and PATCH for partial updates, and making POST create-only. The changes are well-structured, but I've identified two high-severity issues. First, there's a race condition in the create_worker function that could lead to an upsert on POST, defeating the create-only goal. Second, the new replace_worker function for PUT requests has a bug where it doesn't validate the worker URL, potentially creating a new worker instead of replacing the intended one. I've provided detailed comments and a code suggestion for the replace_worker bug.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@model_gateway/src/core/worker_service.rs`:
- Around line 277-305: In replace_worker, ensure the URL used for the
replacement is consistent with the registry: after retrieving the existing url
via worker_registry.get_url_by_id(&worker_id) and before creating Job::AddWorker
(which uses config.url during register_or_replace), either validate that
config.url == existing url and return a BadRequest/WorkerServiceError if they
differ, or overwrite config.url with the existing url so the job will target the
current worker; update the code around the Job::AddWorker creation and ensure
UpdateWorkerResult still returns the existing url for clarity.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 165e207f-ebd1-4120-881c-5042ad138ab7
📒 Files selected for processing (3)
model_gateway/src/core/worker_registry.rsmodel_gateway/src/core/worker_service.rsmodel_gateway/src/server.rs
💤 Files with no reviewable changes (1)
- model_gateway/src/core/worker_registry.rs
Reject PUT /workers/{id} if the URL in the request body doesn't match
the existing worker's URL. URL changes are not supported via replace
— use DELETE + POST instead.
Signed-off-by: Chang Su <chang.s.su@oracle.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@model_gateway/src/core/worker_service.rs`:
- Around line 295-302: Replace the incorrect use of
WorkerServiceError::InvalidId when the parsed worker_id_raw matches but the
request body URL differs: add a new error variant (e.g.,
WorkerServiceError::UrlMismatch or a generic BadRequest { message }) and return
that variant in the branch that currently constructs InvalidId, using a clear
message about the URL mismatch (reusing the existing formatted string). Update
the code that constructs the Err(...) in the branch (the expression that
currently builds WorkerServiceError::InvalidId with raw:
worker_id_raw.to_string()) to instead construct the new UrlMismatch/BadRequest
variant so the HTTP 400 response reflects a URL mismatch rather than an invalid
UUID.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7ec96cfd-4215-4794-bf0b-a265c92b4aa9
📒 Files selected for processing (1)
model_gateway/src/core/worker_service.rs
Add TestWorkerAPIRestSemantics e2e test class: - test_post_duplicate_url_returns_409: POST same URL twice, verify 409 - test_patch_partial_update: PATCH updates priority/labels - test_put_full_replace: PUT replaces worker with model re-discovery - test_put_url_mismatch_returns_400: PUT with different URL rejected Signed-off-by: Chang Su <chang.s.su@oracle.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
e2e_test/router/test_worker_api.py (1)
3-6:⚠️ Potential issue | 🟡 MinorStale file docstring: update to reflect new REST endpoints.
The file docstring still references legacy endpoints (
POST /add_worker,POST /remove_worker) but the newTestWorkerAPIRestSemanticsclass testsPOST /workers,PUT /workers/{id}, andPATCH /workers/{id}. Consider updating the docstring to document all tested endpoints.📝 Suggested docstring update
"""Tests for gateway worker management APIs. Tests the gateway's worker management endpoints: - GET /workers - List all workers -- POST /add_worker - Add a worker dynamically -- POST /remove_worker - Remove a worker dynamically +- POST /workers - Add a worker (returns 409 on duplicate URL) +- PUT /workers/{id} - Full replace of a worker +- PATCH /workers/{id} - Partial update of a worker +- DELETE /workers/{id} - Remove a worker dynamically - GET /v1/models - List available models🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e_test/router/test_worker_api.py` around lines 3 - 6, The module-level docstring is stale and still lists legacy endpoints; update it to reflect the endpoints exercised by the TestWorkerAPIRestSemantics test class by replacing references to POST /add_worker and POST /remove_worker with the current REST endpoints: POST /workers (create), PUT /workers/{id} (replace/update), and PATCH /workers/{id} (partial update), and describe that GET /workers lists workers so the docstring accurately documents the tested API surface and behavior in the file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e_test/router/test_worker_api.py`:
- Around line 552-589: The test test_put_full_replace only checks the worker
still exists after PUT; update it to assert that model re-discovery occurred by
either (A) waiting for the worker to become healthy/ready after the PUT (e.g.,
poll the gateway until the worker's status is healthy) to ensure
register_or_replace() completed, or (B) inspecting the PUT response or a
subsequent GET /workers/{worker_id} (via Gateway.list_workers() or a dedicated
Gateway.get_worker() call) for fields that change on re-registration (model
metadata, version, or a re-registration timestamp) and asserting those changed;
locate the HTTP PUT call in test_put_full_replace and add the polling/assertion
against gateway.list_workers() or the PUT response to confirm re-discovery.
- Around line 519-550: Update test_patch_partial_update to actually verify the
PATCH changes: after calling gateway.add_worker and performing the httpx.patch
to /workers/{worker_id}, issue a GET to
f"{gateway.base_url}/workers/{worker_id}" (or use gateway.get_worker if
available) and assert the returned worker JSON contains the updated "priority"
== 100 and "labels" contains {"env": "test"}; if you intend to test "cost" also,
include it in the PATCH payload and assert it on the GET result. Also update the
test's docstring to match the fields you're modifying (remove "cost" if not
testing it, or include it if you add it to the payload). Ensure assertions
reference the test function name test_patch_partial_update and the Gateway
add_worker flow so the change is easy to locate.
---
Outside diff comments:
In `@e2e_test/router/test_worker_api.py`:
- Around line 3-6: The module-level docstring is stale and still lists legacy
endpoints; update it to reflect the endpoints exercised by the
TestWorkerAPIRestSemantics test class by replacing references to POST
/add_worker and POST /remove_worker with the current REST endpoints: POST
/workers (create), PUT /workers/{id} (replace/update), and PATCH /workers/{id}
(partial update), and describe that GET /workers lists workers so the docstring
accurately documents the tested API surface and behavior in the file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0e2f38ff-9e2d-4cd8-b320-6884b5c14e87
📒 Files selected for processing (1)
e2e_test/router/test_worker_api.py
Add WorkerServiceError::BadRequest variant for general bad request errors. Use it for URL mismatch in replace_worker() instead of reusing InvalidId which misleadingly claims the UUID is malformed. Signed-off-by: Chang Su <chang.s.su@oracle.com>
Poll GET /workers/{id} after PATCH to verify priority, cost, and
labels were actually applied, not just accepted.
Signed-off-by: Chang Su <chang.s.su@oracle.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
e2e_test/router/test_worker_api.py (1)
577-614: 🧹 Nitpick | 🔵 TrivialConsider verifying model re-discovery occurred.
The docstring claims the test verifies "full replace with model re-discovery," but the test only confirms the worker still exists after PUT. To strengthen this test, consider waiting for the worker to become healthy again after the PUT, which would confirm the
register_or_replace()workflow completed successfully.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e_test/router/test_worker_api.py` around lines 577 - 614, The test_put_full_replace currently only asserts the worker still exists after PUT; update it to verify model re-discovery completed by polling the Gateway for the worker's health/model metadata after the PUT. After calling httpx.put on /workers/{worker_id}, repeatedly call Gateway.list_workers() (or a Gateway.get_worker(worker_id) helper) and wait until the worker's health/status indicates healthy and/or its model info reflects re-discovery (i.e., compare model fields or a last_registered timestamp), failing the test if a timeout elapses; reference Gateway, add_worker, list_workers, and the register_or_replace workflow when implementing the polling check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@e2e_test/router/test_worker_api.py`:
- Around line 577-614: The test_put_full_replace currently only asserts the
worker still exists after PUT; update it to verify model re-discovery completed
by polling the Gateway for the worker's health/model metadata after the PUT.
After calling httpx.put on /workers/{worker_id}, repeatedly call
Gateway.list_workers() (or a Gateway.get_worker(worker_id) helper) and wait
until the worker's health/status indicates healthy and/or its model info
reflects re-discovery (i.e., compare model fields or a last_registered
timestamp), failing the test if a timeout elapses; reference Gateway,
add_worker, list_workers, and the register_or_replace workflow when implementing
the polling check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f1c59094-d21a-415e-bc5b-e6dd821f595a
📒 Files selected for processing (1)
e2e_test/router/test_worker_api.py
1 participant
Description
Part of the worker API REST fix series: #836 (registry refactor) → this PR (REST endpoints).
Problem
POST /workerssilently upserts when the same URL is registered twice.PUT /workers/{id}only does partial updates. There's no way to do a full worker replacement (e.g., new API key with model re-discovery).Solution
Enforce proper REST semantics:
POST /workersPUT /workers/{id}WorkerSpec, re-runs registration workflow (model discovery, etc.) viaregister_or_replace().PATCH /workers/{id}WorkerUpdateRequestas before (moved from PUT).DELETE /workers/{id}Breaking change
Clients using
PUT /workers/{id}withWorkerUpdateRequestmust switch toPATCH /workers/{id}.PUTnow expects a fullWorkerSpec.Changes
WorkerService::create_worker()— rejects duplicate URLs with actionable error messageWorkerServiceError::ConflictvariantWorkerService::replace_worker()— submitsAddWorkerjob (same workflow, usesregister_or_replace()internally)replace_workerhandler inserver.rsPUT /workers/{id}toreplace_worker,PATCH /workers/{id}toupdate_workerreserve_id_for_urlTest Plan
cargo test -p smg --lib— all 437 tests pass, 0 failuresPOST /workerswith duplicate URL returns 409Checklist
cargo +nightly fmtpassescargo clippy --all-targets --all-features -- -D warningspassesSummary by CodeRabbit
New Features
API Changes
Tests
Summary by CodeRabbit