refactor(responses): retrieval to use data layer directly - #938
Conversation
Signed-off-by: Ziwen Zhao <zzw.mose@gmail.com>
📝 WalkthroughWalkthroughThe PR refactors response retrieval endpoints (GET, DELETE, list input items) by removing protocol-specific router implementations and consolidating shared handler logic into a new dedicated module. Endpoints now directly call handlers backed by Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
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)
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. Comment |
There was a problem hiding this comment.
Code Review
This pull request refactors the response management system by centralizing the logic for retrieving, deleting, and listing response input items into a new responses module. It removes these methods from the RouterTrait and its various implementations (gRPC, HTTP, OpenAI), allowing the server to handle these requests directly via the shared response_storage. This change simplifies the routing architecture and ensures consistent behavior across different router types. I have no feedback to provide as there were no review comments to assess.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b1b6ea7260
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
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)
model_gateway/tests/api/api_endpoints_test.rs (1)
999-1007:⚠️ Potential issue | 🟡 MinorStrengthen shared-storage retrieval assertion beyond status code.
Line 1006 only checks
200 OK, so this test can pass even if the wrong response object is returned.✅ Suggested test hardening
let resp = app.clone().oneshot(req).await.unwrap(); assert_eq!(resp.status(), StatusCode::OK); + let body = axum::body::to_bytes(resp.into_body(), usize::MAX) + .await + .unwrap(); + let get_json: serde_json::Value = serde_json::from_slice(&body).unwrap(); + assert_eq!(get_json["id"], rid); + assert_eq!(get_json["object"], "response");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model_gateway/tests/api/api_endpoints_test.rs` around lines 999 - 1007, The test currently only asserts the HTTP status for the GET /v1/responses/{rid} call; update the assertion to parse the response body (from the Response returned by app.clone().oneshot) as JSON and validate that it contains the expected response object fields (at minimum that the "id" matches rid, and preferably all fields or an exact JSON equality against the originally stored response); modify the test around the Request/resp handling in api_endpoints_test.rs so after asserting StatusCode::OK you read the body bytes, deserialize to the same response struct or serde_json::Value, and assert the id and other fields match the expected values to ensure the correct object was returned.
🤖 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/routers/responses/handlers.rs`:
- Around line 36-37: The current code does a get_response(&id) followed by
delete_response(&id), which has a TOCTOU race if another caller deletes between
those calls; change logic to avoid the separate existence check: call
response_storage.delete_response(&id).await directly and handle its result
idempotently (treat “not found” as success) or make delete_response itself
return success when the record is already absent. Update handlers that reference
get_response and delete_response to rely on delete_response’s idempotent
behavior and remove the pre-check to eliminate the race.
- Around line 72-82: The current items_with_ids creation in
list_response_input_items injects a random ID via generate_id("msg") at read
time causing non-deterministic IDs across requests; to fix, replace the
ephemeral random generation with a deterministic ID (for example compute a
stable hash of the item's JSON plus its index or other stable fields) or persist
the generated ID back into the stored response so subsequent reads return the
same id; locate the items_with_ids mapping and change the id assignment logic
(currently calling generate_id("msg")) to either (a) compute a deterministic id
from item content/position or (b) write the generated id back into the response
storage so future calls see the same id.
---
Outside diff comments:
In `@model_gateway/tests/api/api_endpoints_test.rs`:
- Around line 999-1007: The test currently only asserts the HTTP status for the
GET /v1/responses/{rid} call; update the assertion to parse the response body
(from the Response returned by app.clone().oneshot) as JSON and validate that it
contains the expected response object fields (at minimum that the "id" matches
rid, and preferably all fields or an exact JSON equality against the originally
stored response); modify the test around the Request/resp handling in
api_endpoints_test.rs so after asserting StatusCode::OK you read the body bytes,
deserialize to the same response struct or serde_json::Value, and assert the id
and other fields match the expected values to ensure the correct object was
returned.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 54e8b2e6-abd1-4d1a-85ed-4a7d558201cd
📒 Files selected for processing (13)
crates/protocols/src/responses.rsmodel_gateway/src/routers/grpc/common/responses/handlers.rsmodel_gateway/src/routers/grpc/router.rsmodel_gateway/src/routers/http/router.rsmodel_gateway/src/routers/mod.rsmodel_gateway/src/routers/openai/responses/mod.rsmodel_gateway/src/routers/openai/router.rsmodel_gateway/src/routers/responses/handlers.rsmodel_gateway/src/routers/responses/mod.rsmodel_gateway/src/routers/router_manager.rsmodel_gateway/src/server.rsmodel_gateway/tests/api/api_endpoints_test.rsmodel_gateway/tests/routing/test_openai_routing.rs
💤 Files with no reviewable changes (1)
- crates/protocols/src/responses.rs
…t#938) Signed-off-by: Ziwen Zhao <zzw.mose@gmail.com>
Description
Problem
When running with
--enable-igw, response retrieval APIs such asGET /v1/responses/{response_id}will returnnot implemented.The root cause is that these APIs do not carry a
model_id, so IGW cannot reliably choose the correct router. Before this change, these endpoints were routed through the generic router-selection path, which works for inference requests but is not a good fit for storage-backed response APIs. Depending on which router was selected, the request could land on a router that does not implement the endpoint and return501 Not Implemented.This is different from
/v1/conversations/*, which already operates directly on the shared data layer instead of relying on router dispatch.Solution
Move response retrieval-style APIs to the data layer directly, following the same pattern as conversations.
This PR makes
GET /v1/responses/{response_id},GET /v1/responses/{response_id}/input_items, andDELETE /v1/responses/{response_id}use shared response storage directly from the server layer instead of dispatching throughRouterTrait/RouterManager.It also removes the now-unused router trait methods and related plumbing for response retrieval, so these endpoints are no longer coupled to IGW router selection.
Known limitation: HTTP regular router response persistence is still incomplete. Responses created through that path may not be persisted to shared
response_storage, so this PR does not attempt to change that behavior. That issue is intentionally left out of scope for this change.Changes
GET /v1/responses/{response_id}directly to shared response storageGET /v1/responses/{response_id}/input_itemsdirectly to shared response storageDELETE /v1/responses/{response_id}against shared response storageRouterTraitand router implementationsresponsesmodule aligned with the existingconversationsstructureResponsesGetParamsTest Plan
Before
After
Checklist
cargo +nightly fmtpassescargo clippy --all-targets --all-features -- -D warningspassesSummary by CodeRabbit