Conversation
b94d617 to
75bb73e
Compare
…et event limit Address the maintainer review on vllm-project#304. - Align the Responses WebSocket event ceiling with the configured `max_stream_event_bytes` instead of a fixed 1 MiB, and pass the effective transport limit (minus the exact `stream_id` routing member plus slack) into the executor via `ExecuteRequest::with_max_stream_event_bytes`, so the terminal `response.completed` frame is validated against what the socket can deliver before the response is persisted or a checkpoint is published. - Make completed-item measurement comprehensive through one `RetainedSize` trait per type: message annotations, compaction `encrypted_content`, nested tool-search arguments, shell `extra` maps, MCP error content, and list-tools annotations are now counted, recursively for JSON values. - Charge one container per new content part or streamed reasoning index before the entry is inserted, so empty deltas exhaust the budget promptly. - Move per-kind active state into `accumulator/active.rs`. Each kind owns its buffers and update logic; growth is charged before a delta is appended and measured around the single `ApplyDone`/`MergeDone` call for completions, so accounting observes the completion policy instead of re-deriving it. One `RetainedAccount` primitive (`charge`/`grow`/`reconcile`) is used from open, update, completion, and finalization; `SlotMap` keeps identity and lifecycle. - Add accounting tests for delta/done-only equivalence, repeated completion snapshots, empty multipart entries, metadata supplied at completion, rejection before the buffer grows, and oversized annotations, compaction, and nested arguments on both ingestion paths; add WebSocket regressions for a terminal event above the old fixed limit and for an undeliverable terminal event that must not be persisted. Claude-Session: https://claude.ai/code/session_01EsbCF1Smauv41qnpnqYvfz Signed-off-by: Zheng Lu <Lz429671594@gmail.com>
|
@Zheng-Lu please update the |
| Value::Null => 4, | ||
| Value::Bool(_) => 5, | ||
| Value::Number(_) => 8, | ||
| Value::String(text) => text.len(), |
There was a problem hiding this comment.
Value::String("") currently contributes zero bytes, while Value::Array charges only one container plus its children’s measurements. An array containing 100,000 empty strings therefore measures just 32 bytes, despite retaining 100,000 Value entries.
this can be reproduced through both JSON and streaming: an annotation containing that array completed successfully with max_retained_bytes = 4096. The returned output was approximately 300 KB.
Please charge structural overhead per retained JSON value or collection entry, including empty strings, and add regression tests for this nested-array case on both ingestion paths. Final reconciliation currently uses the same measurement, so it cannot catch the bypass.
| } | ||
| } | ||
|
|
||
| impl RetainedSize for OutputTextContent { |
There was a problem hiding this comment.
The RetainedSize documentation excludes type, role, and status because they are supposedly bounded by enums. However, fields such as OutputMessage.role and OutputTextContent.type_ are unrestricted Strings, and the live executor’s lenient ingestion accepts and retains arbitrary values.
this can be reproduced through both JSON and streaming: a message with a 100,000-byte role, and separately a content part with a 100,000-byte type, each completed successfully with max_retained_bytes = 4096.
Please include unrestricted strings in retained measurements, or enforce a bounded vocabulary during deserialization. Audit the other excluded fields for the same assumption and add regression tests on both ingestion paths. Accounting should only omit fields whose bounds are enforced by their owning types.
| delta, summary_index, .. | ||
| } => count_streamed(&mut self.summary_streamed, *summary_index, delta, account, budget), | ||
| EventPayload::ReasoningTextDone { content_index, .. } => { | ||
| account.grow(budget, self, Self::text_retained_bytes, |state| { |
There was a problem hiding this comment.
Each ReasoningTextDone calls RetainedAccount::grow with text_retained_bytes, which scans all accumulated content, summaries, and tracking maps before and after the mutation. ReasoningSummaryTextDone follows the same pattern.
Completing N separate parts therefore performs O(N²) accounting work, even when each completion simply appends a small part. This processing happens synchronously in ingestion.
A synthetic reproduction with sequential one-byte reasoning parts showed:
- 4,000 parts: 98 ms previously → 302 ms after this change.
- 8,000 parts: 197 ms previously → 956 ms after this change.
- 16,000 parts: 394 ms previously → 3,421 ms after this change.
These are debug-build measurements, including parsing; they demonstrate the scaling regression rather than production latency.
Please measure only the affected part, or maintain cached totals updated through the existing completion operation. Keep completion behavior in one place so accounting does not duplicate its reconciliation rules, and reserve full-item measurement for final reconciliation.
… bytes in response budget (vllm-project#288) - Remove raw wire-byte charging per SSE line in upstream streaming. - Decouple and independently configure responses resource limits: - max_retained_bytes (default 8 MiB) - max_upstream_json_bytes (default 16 MiB) - max_upstream_sse_line_bytes (default 16 MiB) - max_stream_event_bytes (default 16 MiB) - Account for logical retained output items and container overhead across rounds. - Reconcile retained bytes in slot draining and completion candidates. - Introduce typed ExecutorError::ResourceLimitExceeded. - Ensure chunk-invariant retained accounting across 1-byte, 1024-byte, and JSON responses. Signed-off-by: Zheng Lu <Lz429671594@gmail.com>
…ing, and part tracking Signed-off-by: Zheng Lu <Lz429671594@gmail.com>
…d document response limits Signed-off-by: Zheng Lu <Lz429671594@gmail.com>
… ordering, and error assertions Signed-off-by: Zheng Lu <Lz429671594@gmail.com>
…et event limit Address the maintainer review on vllm-project#304. - Align the Responses WebSocket event ceiling with the configured `max_stream_event_bytes` instead of a fixed 1 MiB, and pass the effective transport limit (minus the exact `stream_id` routing member plus slack) into the executor via `ExecuteRequest::with_max_stream_event_bytes`, so the terminal `response.completed` frame is validated against what the socket can deliver before the response is persisted or a checkpoint is published. - Make completed-item measurement comprehensive through one `RetainedSize` trait per type: message annotations, compaction `encrypted_content`, nested tool-search arguments, shell `extra` maps, MCP error content, and list-tools annotations are now counted, recursively for JSON values. - Charge one container per new content part or streamed reasoning index before the entry is inserted, so empty deltas exhaust the budget promptly. - Move per-kind active state into `accumulator/active.rs`. Each kind owns its buffers and update logic; growth is charged before a delta is appended and measured around the single `ApplyDone`/`MergeDone` call for completions, so accounting observes the completion policy instead of re-deriving it. One `RetainedAccount` primitive (`charge`/`grow`/`reconcile`) is used from open, update, completion, and finalization; `SlotMap` keeps identity and lifecycle. - Add accounting tests for delta/done-only equivalence, repeated completion snapshots, empty multipart entries, metadata supplied at completion, rejection before the buffer grows, and oversized annotations, compaction, and nested arguments on both ingestion paths; add WebSocket regressions for a terminal event above the old fixed limit and for an undeliverable terminal event that must not be persisted. Claude-Session: https://claude.ai/code/session_01EsbCF1Smauv41qnpnqYvfz Signed-off-by: Zheng Lu <Lz429671594@gmail.com>
b33431e to
ea3c6c1
Compare
Summary
Fixes #288.
Problem
Previously in
crates/agentic-server-core/src/executor/upstream.rs,fetch_stream_payloadcharged raw wire bytes of every upstream SSE line (response_budget.consume(line.len())) before semantic accumulation. Because each line includes transport framing (data:, JSON delimiters, repeated IDs, and redundant completion snapshots), fine-grained chunking (e.g. 1-byte deltas) exhausted the default 1 MiB cumulative response budget after only ~7,655 bytes of answer text. In contrast, coarser deltas or non-streaming JSON succeeded with the identical answer text. Furthermore, the 256 KiB SSE line cap, 1 MiB JSON body limit, and 1 MiB stream event cap were tightly coupled and could cause late stream failures at the terminal boundary.Solution
Logical Retained Accounting (Chunk-Invariant):
fetch_stream_payload.SlotMap::apply/ActiveItem::apply_event) and container overhead across rounds.TextDone,ReasoningTextDone,FunctionCallArgsDone, etc.), charged unstreamed bytes immediately upon reception.drain_output_with_budgetas a final reconciliation safety net.Decoupled & Configurable Resource Limits:
max_retained_bytes(AGENTIC_MAX_RETAINED_RESPONSE_BYTES, default 8 MiB): cumulative logical retained output across rounds.max_upstream_json_bytes(AGENTIC_MAX_UPSTREAM_JSON_BYTES, default 16 MiB): bounds a single upstream JSON response body.max_upstream_sse_line_bytes(AGENTIC_MAX_UPSTREAM_SSE_LINE_BYTES, default 16 MiB): bounds a single upstream SSE line buffer, scanned in linearmax_stream_event_bytes(AGENTIC_MAX_STREAM_EVENT_BYTES, default 16 MiB): bounds a single serialized client stream event (accommodating fulloutput_item.doneandresponse.completedframes).max_retained_bytesby proportional wire headroom (max(64 KiB, max_retained_bytes / 4)) to account for JSON escaping, schemas, and message envelopes.Synchronous Ingestion & State Machine Hardening:
pub(super) struct StreamedPart { text: String, streamed: bool }inslot.rsandMergeDoneincompletion.rs, cleanly separating multi-part stream tracking without data loss.stream_lifecycle = AwaitingCreatedon empty-IDresponse.createdevents to guard strict-mode transitions.WebSearchCall.action,McpCall.output/error,McpListTools.toolsschemas, andReasoningOutput.encrypted_content.ExecutorError::ResourceLimitExceeded { limit: ResourceLimit, max_bytes: usize }.[responses]configuration and streaming in-flight memory sizing guide inREADME.md.Test Plan
Reproduction Script & Live Binary E2E:
public_repro.pyagainst the compiledagentic-serverbinary and mock upstream:size=8000, chunk=1(1-byte deltas): received all 8,000 bytes withresponse.completedand 0 errors (previously failed after 7,655 bytes with HTTP 500budget exceeded).size=8000, chunk=1024: passed with 8,000 bytes andresponse.completed.size=220000, chunk=1024: received all 220,000 bytes withresponse.completed(previously rejected by terminal snapshot inflation).size=270000, chunk=1024: passed under the new 16 MiB line limit (previously exceeded 256 KiB line cap).completed.Unit & Invariance Tests:
small_and_large_sse_chunking_and_json_produce_identical_retained_bytes: asserts byte-identicalbudget.used()across 1-byte chunks, 1024-byte chunks, and JSON.repeated_completion_snapshots_do_not_double_count_retained_bytes: verifies terminal snapshots do not inflate budget.lenient_done_without_output_item_done_exceeding_budget_fails_promptly: asserts prompt failure directly onoutput_text.done.two_part_done_only_stream_preserves_both_parts: asserts both parts are preserved and matches delta-stream budget.responses_config_validation: tests valid configurations, boundary rejection, and proportional headroom limits.retained_accounting_for_web_search_mcp_and_reasoning: asserts accurate estimation for extended items.Workspace Suite & Linters:
cargo test --workspace(100% pass: 697 core library tests + 91 server tests + all replay cassettes).cargo clippy --workspace --all-targets -- -D warnings(clean, 0 warnings).cargo fmt --all -- --check(clean).