Add hydrate and persist endpoints. - #216
Conversation
Signed-off-by: Mohammad <mohammad.nassar@ibm.com>
| .route("/health", get(handler::health)) | ||
| .route("/ready", get(handler::ready)) | ||
| .route("/internal/hydrate", post(handler::internal_hydrate)) | ||
| .route("/internal/persist", post(handler::internal_persist)) |
There was a problem hiding this comment.
these routes have no workload authentication or object-level authorization. once this listener is reachable by the coordinator, any reachable workload with a known response ID can hydrate its full stored history or write a turn. we should authenticate both endpoints and scope every read and write to the authenticated tenant.
| #[derive(Debug, Clone, Serialize, Deserialize)] | ||
| pub struct SplitContext { | ||
| /// Response id reserved for this turn (`resp_` prefix). | ||
| pub response_id: String, |
There was a problem hiding this comment.
SplitContext is accepted as caller-authored authority: persist never proves it came from hydrate and directly trusts its response_id, original request, continuation link, and effective configuration. a caller can skip hydrate and forge durable records; an empty response_id also returns success because persistence silently skips the write. we should use a signed, expiring context or a one-time server-side handle, and reject malformed reserved IDs.
| exec_ctx: &ExecutionContext, | ||
| ) -> ExecutorResult<ResponsePayload> { | ||
| let ctx = RequestContext::from(context); | ||
| let payload = payload_from_upstream(&ctx, upstream)?; |
There was a problem hiding this comment.
at the new HTTP persistence boundary, a response containing only {"id":"x"} defaults to Completed with empty output, while malformed output entries are silently dropped. this lets an invalid model response persist a completed turn with no valid output. we should deserialize a strict terminal response here and reject missing status, missing output, and malformed output items.
| ))); | ||
| } | ||
|
|
||
| persist_if_needed( |
There was a problem hiding this comment.
this insert makes /internal/persist non-idempotent. if the transaction commits but the response is lost, retrying the exact request hits the same primary key and returns HTTP 500 even though the turn was stored. we should use the reserved response ID as an idempotency key: return the existing result for an identical retry and 409 for mismatched reuse.
|
|
||
| // The in-process flow silently skips non-terminal statuses; here that would | ||
| // return an envelope whose id can never be continued, so reject it. | ||
| if !matches!( |
There was a problem hiding this comment.
response.failed is a terminal upstream result, but it becomes ResponseStatus::Error and is rejected here as InvalidRequest, so /internal/persist reports a model failure as HTTP 400. we should return the normalized failed response or a dedicated upstream-error status, and reserve 400 for malformed or non-terminal input.
| let shutdown = CancellationToken::new(); | ||
| let on_signal = shutdown.clone(); | ||
| tokio::spawn(async move { | ||
| if tokio::signal::ctrl_c().await.is_ok() { |
There was a problem hiding this comment.
this only handles Ctrl-C, while Kubernetes terminates containers with SIGTERM. the default SIGTERM action bypasses the graceful-shutdown future and can cut off in-flight hydrate or persist requests. we should reuse the gateway's SIGTERM-aware shutdown path and bounded drain timeout.
|
|
||
| #[derive(Parser)] | ||
| #[command(name = "agentic-llm-d", about = "agentic-api backend mode for the llm-d coordinator")] | ||
| struct Cli { |
There was a problem hiding this comment.
Could we reuse agentic-server’s existing common CLI/configuration instead of defining another Cli here? agentic-server already supports --gateway-host, --gateway-port, and --db-url. Duplicating these options with different names and defaults risks configuration drift. Could the llm-d backend be exposed as an agentic-server subcommand, or could both binaries share the same common argument structure?
|
|
||
| ## Composition | ||
|
|
||
| Neither step reimplements the flow. `hydrate` calls `rehydrate_conversation` and `upstream_request_json`, and `persist` |
There was a problem hiding this comment.
can we reconsider the boundary between split execution and upstream handling?
The llm-d integration is intended to expose only hydration and persistence while the coordinator owns inference. However, implementing it currently requires changing the existing agentic-server-core upstream path by introducing UpstreamBody, payload_from_upstream, absorb_line, and ResponseAccumulator::saw_terminal_frame. This couples the split-execution API to the current OpenAI Responses JSON/SSE representation and increases the regression surface for the normal agentic-server inference path.
Could core instead expose a transport-neutral prepare/commit API built around the existing rehydrate_conversation and persist_response operations? The commit operation would also handle the missing shared behavior—context reconstruction, terminal-state validation, reserved-ID injection, and conditional persistence.
Upstream-specific decoding could then live behind an adapter interface, with the existing OpenAI JSON/SSE implementation as one adapter. The llm-d handlers would decode their input through that adapter and pass a normalized ResponsePayload to the core commit operation. This would keep persistence independent of upstream wire formats and make support for future upstream protocols additive rather than requiring further changes to upstream.rs and the accumulator.
I do not think the accumulator itself should move entirely into agentic-llm-d, because the regular server path already depends on it. The concern is that response decoding, upstream transport, and persistence are currently joined in the same abstraction. Separating those responsibilities would give us a cleaner and more extensible integration boundary.
| /// A complete upstream response, in whichever form the caller received it. | ||
| #[derive(Debug, Clone, Copy)] | ||
| pub enum UpstreamBody<'a> { | ||
| Json(&'a str), |
There was a problem hiding this comment.
I left a design-level comment in docs/design/agentic-llm-d.md. This abstraction currently couples split persistence to the OpenAI JSON/SSE wire format; I think response decoding should sit behind an upstream adapter boundary.
|
|
||
| ## Discussion points | ||
|
|
||
| The `/internal` endpoints carry no credential and trust their caller, so restricting them is a network-layer concern |
There was a problem hiding this comment.
relying solely on network isolation does not seem sufficient for endpoints with this level of authority. /internal/hydrate can return complete persisted conversation history, while /internal/persist accepts an unsigned, caller-controlled SplitContext and writes to storage. Any compromised pod, SSRF path, or misconfigured network policy could potentially read history, forge context fields, inject turns, replay requests, or exhaust storage.
agentic-server supports optional inbound OIDC bearer-token authentication, but this binary has no equivalent application-layer authentication or authorization. Before merging, could we define and enforce the service-to-service trust model—for example, mTLS/workload identity or bearer-token authentication?
We should also consider making SplitContext tamper-evident, such as an HMAC-signed opaque token with an expiry and audience, or storing the context server-side behind a short-lived, one-time identifier. Replay/idempotency behavior for persist should also be defined. Network policy can remain defense in depth, but it should not be the only security control.
| /// Absent on purpose: `enriched_request` (the conversation, already in flight as | ||
| /// the request — rebuilt on return) and `new_input_items` (derived). | ||
| #[derive(Debug, Clone, Serialize, Deserialize)] | ||
| pub struct SplitContext { |
There was a problem hiding this comment.
Core should not model the private protocol of one consuming crate.
SplitContext appears to be specific to the llm-d HTTP boundary rather than part of the executor’s general domain model. It exists to serialize state between /internal/hydrate and /internal/persist, and its wire representation is consumed only by the llm-d integration.
Could we move SplitContext, its conversions, and the associated wire-level tests into agentic-llm-d? The same likely applies to Hydration, UpstreamBody, and the split-specific orchestration. agentic-server-core should expose only the narrow, transport-neutral operations needed for rehydration, response normalization, ID assignment, validation, and persistence.
This preserves the intended dependency direction—agentic-llm-d depends on core—without making core aware of a specific consumer’s private protocol. The split integration tests currently under agentic-server-core/tests should move with that boundary, while tests for the generic core operations can remain in core.
3 participants
Summary
Exposes the two halves of a stateful Responses turn as separate endpoints, so an external orchestrator (the llm-d coordinator) can make the inference call itself:
POST /internal/hydrate: expandsprevious_response_idinto a stateless upstream request, plus an opaque context.POST /internal/persist: takes that context and the model's response (a JSON body, or the SSE frames a streaming caller relayed), stores the turn, and returns the envelope carrying the storedresp_id.Both compose existing in-process steps (
rehydrate_conversation,payload_from_upstream,persist_if_needed). Sosplit.rsadds no parsing, storage, or request building of its own.RequestContextis unchanged for in-process callers; it now serializes through a reduced wire form,SplitContext.The endpoints ship as a new crate and binary,
agentic-llm-d, depending onagentic-server-corealone.Test Plan
cargo test- newsplit_execution_integration.rscovers the two-turn replay, the error paths, and the context round-trip. Existing suites pass unchanged.Verified on a cluster with a multi-turn conversation.
Closes #215.