[glamr-agent]

Summary

The request-trace audit record could claim to be a faithful copy of a request and its response even when the response had been thrown away. RequestPayloadHandle::emit wrote the literals payload_complete: true and payload_drop_reason: None into every record it published, including records published with no response at all. A consumer of the audit stream could not tell a request that genuinely produced no content from one whose response we dropped and did not mention. This change makes the record say what it actually knows.

Details:

emit now takes the drop reason as a required second argument and derives payload_complete from the data — response.is_some() && drop_reason.is_none() — instead of hardcoding it. A call site that has no response and states no reason gets the reason unspecified rather than a record that looks good.

The reason had to be carried down from where it was lost. The payload future in lib/llm/src/request_trace/payload_stream.rs resolved to a bare Option, collapsing three outcomes into one None. It now resolves to a PayloadOutcome holding both the response and an optional reason, and each site names itself: empty_response_stream, aggregation_failed:<error>, or response_stream_dropped. The identifier:detail form matches the grammar otel_sink.rs already publishes.

Where a stream delivered content before failing, aggregate_with_partial_recovery re-aggregates the error-free prefix, so the record keeps what the client saw and says why it stops there. DeltaAggregator::apply is unchanged; its short-circuit is load-bearing on the client-facing path.

OTEL records within the size limit now preserve the captured completeness flag and drop reason in the exported attributes, keeping them consistent with the embedded record body. Oversized OTEL markers also retain the original capture reason alongside the size-limit reason. Regression coverage includes successful folds, incomplete OTEL payloads, and capture reasons on both oversized-marker paths.

No wire-schema change: both fields already exist on RequestTracePayload and are already serialized.

Where should the reviewer start?

lib/llm/src/request_trace/payload.rs for the derivation, then payload_stream.rs for the reason plumbing and partial recovery.

Validation

• Current head: 361286827a8b237a631ed12dfe41feac6ad68bbe.
• Rustfmt 1.96.1 on the changed Rust file and whitespace checks passed before committing.
• No local tests, builds, or benchmarks were run for the latest repair; remote CI is pending for this head.
• Regression coverage includes successful non-streaming folds, incomplete capture metadata, partial recovery after backend errors, and OTEL capture reasons both within the size limit and in oversized markers.

Related Issues

🔗 This PR is linked to an issue:

• Closes fix(llm): request-payload audit record loses response and misreports payload_complete on mid-stream/admission error #14134

Summary by CodeRabbit

• Improvements
  • Request tracing now records why response payloads were incomplete or dropped.
  • Partial responses received before backend errors are preserved in the audit record.
  • Empty streams and unspecified failures receive clear fallback reasons.
  • Successfully completed responses continue to be recorded as complete.
  • Incomplete records include available response content alongside the associated drop reason, improving troubleshooting of interrupted requests.

[copy-pr-bot]

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

[github-actions]

👋 Hi glamr-agent! Thank you for contributing to ai-dynamo/dynamo.

Just a reminder: The NVIDIA Test Github Validation CI runs an essential subset of the testing framework to quickly catch errors.Your PR reviewers may elect to test the changes comprehensively before approving your changes.

🚀

[glamr-agent]

Automated evidence record — validation complete

Validation status: complete

Evidence summary: [2/2 validated]

AI review assessment (advisory): sound. An AI agent judged the change logically sound from the code and the reported results. This is not an approval; repository CI and human reviewers decide whether to merge.

Validation result: pass — every command listed below ran in this checkout and exited zero, and the new behaviour is proven by paired positive and negative tests rather than by a green suite alone.

Evidence audit: complete [2/2 validated] — the command report comes from recorded runs.

Commands and results [2/2 validated]

Generated from the commands recorded during this run.

Check 1

Checks the changed files with the repository's fast lint and formatting commands.

Result: Passed (exit 0)

Command:

pre-commit run --files lib/llm/src/preprocessor.rs lib/llm/src/request_trace/payload.rs lib/llm/src/request_trace/payload_stream.rs lib/llm/tests/request_trace_nats_integration.rs --hook-stage manual

Check 2

Checks the changed Rust crates with cargo check and Clippy.

Result: Passed (exit 0)

Command:

cargo test -p dynamo-llm --lib

[glamr-agent]

plan.md

Plan — Audit payload record claims completeness when the response is missing

No existing review request applies. Discovery found no open or merged pull request that
delivers this behavior, so there is nothing to defer to, rebase onto, or supersede. Details and
the exact queries are in Discovery below.

User intent

Issue #14134 reports that the request-trace
audit record lies about itself. RequestPayloadHandle::emit in
lib/llm/src/request_trace/payload.rs hardcodes payload_complete: true and
payload_drop_reason: None on every record it publishes, including the records it publishes when
response is None. response is None on three distinct real paths: the client cancelled or
the gateway timed out mid-stream, DeltaAggregator::apply returned Err, or the response stream
was empty. In all three cases the emitted RequestTracePayload carries the request, omits the
response, and simultaneously asserts payload_complete: true with no drop reason. A consumer of
the audit stream — the JSONL, NATS, OTEL, or S3 sink, or the fixture loader in
lib/data-gen/src/request_trace/load.rs — cannot distinguish "this request genuinely had no
response content" from "we dropped the response and are not telling you". For an auditing
subsystem that is the whole value proposition: the completeness flag is the field a consumer
trusts to decide whether a record can be used as evidence.

The issue additionally asks to consider preserving whatever partial content the aggregator had
accumulated before failing, and to add test coverage for the partial-content-then-error path in
lib/llm/src/request_trace/payload_stream.rs. It also notes in passing that /v1/completions may
have no audit capture at all.

The intent of this work item is to make the record honest: when the response is absent, the record
must say payload_complete: false and carry a machine-parseable payload_drop_reason naming which
of the three paths was taken, and where partial content was available it should be preserved rather
than discarded.

Non-goals

• Changing the public contract of DeltaAggregator::apply. Rejected on evidence; see Rejected
  alternatives. Partial-content preservation is done locally in payload_stream.rs instead.
• Adding audit capture to /v1/completions. The issue raises this as an aside, not as part of
  the defect. It is a separate feature (a new endpoint label, a new handle constructor, a new call
  site in the completions preprocessor path) with its own review surface, and bundling it would
  make this change unreviewable as a bug fix. It should be filed separately.
• Restructuring fold_aggregate_with_future. Open pull request
  #14008 rewrites that function into an
  async_stream::stream! generator and deletes final_response_to_one_chunk_stream. Touching its
  shape here would create a gratuitous conflict. This change adds a field to what travels through
  the existing channel and leaves the control flow alone.
• Changing the wire schema. payload_complete and payload_drop_reason already exist on
  RequestTracePayload in lib/llm/src/request_trace/types.rs and are already serialized. No
  schema version bump, no new field, no consumer migration.
• Changing sink behavior. The OTEL, NATS, JSONL, and S3 sinks already carry these fields
  through unchanged. Nothing downstream needs to learn anything new.

Discovery

All findings below come from reading the checked-out tree at main and from provider metadata
queries. Nothing was built, run, or modified. The issue pins line numbers that no longer match
main, so every location was re-derived from the current source.

The defect, as it exists on main

lib/llm/src/request_trace/payload.rs lines 80–94 is the whole bug:

pub fn emit(self, response: Option<Arc<NvCreateChatCompletionResponse>>) {
    super::record::emit_request_payload(
        super::RequestTracePayload {
            request_id: self.request_id,
            endpoint: "openai.chat_completion".to_string(),
            model: self.model,
            request: Some(self.request),
            response,
            http_request_headers: self.http_request_headers,
            payload_complete: true,
            payload_drop_reason: None,
        },
        unix_time_ms(self.event_time),
    );
}

The doc comment directly above it already describes the failure modes correctly — "response is
None on client cancel / gateway timeout / aggregation failure" — so the intent was understood and
the flags were simply never wired. The two literals are not merely wrong; emit has no parameter
that could carry the truth, because the drop reason is destroyed upstream before it ever reaches
this function.

Where the drop reason is destroyed

lib/llm/src/request_trace/payload_stream.rs line 25 defines the channel that carries the
aggregation outcome from the streaming layer back to the emit site:

type PayloadFuture =
    Pin<Box<dyn std::future::Future<Output = Option<NvCreateChatCompletionResponse>> + Send>>;

The output type is Option<T>. There is no room in it for a reason. Three sites collapse
distinguishable outcomes into that single None:

1. PassThroughWithAgg::poll_next at line 63, on Poll::Ready(None) with an empty chunk buffer:
   logs "request payload: empty response stream, no response to aggregate" and calls drop(tx).
2. The same function at lines 78–80: the spawned DeltaAggregator::apply returned Err(e), which
   is logged as tracing::warn!("request payload: aggregation failed: {e}") and then discarded —
   tx is dropped at the end of the closure and the error text never leaves the log line.
3. fold_aggregate_with_future at lines 135–141: the same aggregation error, logged as
   tracing::warn!("fold aggregation failed: {e}"), followed by an explicit drop(tx) and a
   synthetic empty fallback chunk so the HTTP response shape stays valid.

Both scan_aggregate_with_future (line 101) and fold_aggregate_with_future (line 161) then map
rx.await to Ok(resp) => Some(resp) and Err(_) => None. A fourth outcome — the SSE consumer
dropped the passthrough stream before end-of-stream, i.e. a genuine client cancel — also arrives as
Err(_) on that recv, and the existing comment at lines 104–107 acknowledges that the three
causes are indistinguishable at that point. That indistinguishability is the root cause of the
defect, not the two literals in emit.

Caller enumeration, and what it means for the signature

This was enumerated so the required-versus-optional-parameter decision is grounded rather than
guessed.

RequestPayloadHandle::emit has exactly one production call site: lib/llm/src/preprocessor.rs
lines 6482–6493, inside a tokio::spawn that awaits the payload future and branches
Some(final_resp) => payload.emit(Some(Arc::new(final_resp))) against
None => payload.emit(None). Beyond it there are two #[ignore]-marked callers in
lib/llm/tests/request_trace_nats_integration.rs (manual tests that require a live NATS server) and
two callers in the payload.rs unit test at line 295.

request_trace::payload::create_handle has exactly one production call site,
lib/llm/src/preprocessor.rs line 6337.

Five call sites in total, one of them in production, all inside lib/llm. emit is not a
cross-crate stability surface. The new parameter should therefore be required, not optional.
A defaulted or builder-style parameter would let a future call site silently reintroduce the exact
bug being fixed; a required parameter makes the compiler force every caller to state the outcome.
The blast radius of that choice is five one-line edits, which is affordable.

DeltaAggregator::apply is a different story and the reason its contract is a non-goal. It has two
production callers — payload_stream.rs line 74 and preprocessor.rs line 7185, the latter being
the client-facing non-streaming response path — plus roughly forty call sites across the test module
in lib/llm/src/protocols/openai/chat_completions/aggregator.rs.

The aggregator's error behavior is deliberate, and is load-bearing elsewhere

DeltaAggregator::apply at aggregator.rs line 331 drives the chunk stream through a
futures::TryStreamExt::try_fold over Annotated::into_data, which is defined in
lib/runtime/src/protocols/annotated.rs lines 98–104:

pub fn into_data(self) -> Result<Option<R>, DynamoError> {
    if let Some(error) = self.cloned_error() {
        return Err(error);
    }
    Ok(self.data)
}

try_fold short-circuits on the first Err, so any partial content accumulated before an
error-tagged chunk is dropped along with the accumulator. This is not an oversight. The test
preserves_typed_error_after_partial_output at aggregator.rs line 2645 feeds exactly the
partial-content-then-error sequence this issue is about, and asserts that apply returns
unwrap_err() with ErrorType::Backend(BackendError::InvalidArgument) and the message
"invalid sampling parameter". The client-facing path at preprocessor.rs line 7185 depends on
that: a typed backend error must surface to the client as an error, not as a truncated success.
Changing apply to return partial content would either break that test and that guarantee, or
require a second return channel threaded through roughly forty test call sites.

Crucially, payload_stream.rs does not need apply to change. PassThroughWithAgg has already
buffered every chunk in self.chunks: Vec<Annotated<NvCreateChatCompletionStreamResponse>> before
it calls the aggregator. The error-free prefix is sitting in memory, in a Vec, on the audit side
of the boundary. Preserving partial content is therefore a local operation on that buffer, with
zero aggregator ripple.

Existing precedent for the drop-reason string format

This does not need to be invented. lib/llm/src/request_trace/otel_sink.rs lines 175–239 already
implements a drop marker: when a payload exceeds the OTEL size limit, marker_payload sets
payload.payload_complete = false and
payload.payload_drop_reason = Some(format!("otel_payload_too_large:max_bytes={}:actual_bytes={}", ...)),
and the sink's own tests assert that string at lines 443–447. The convention is a colon-delimited
identifier with optional key=value detail segments, which the new reasons should follow so
consumers can parse one grammar.

Existing test coverage, and the gap

payload_stream.rs has seven tests: test_passthrough_forwards_chunks_unchanged,
test_passthrough_aggregates_reasoning_content_and_tool_calls,
test_final_response_to_one_chunk_preserves_reasoning_and_tool_calls, test_empty_stream_handling,
test_single_chunk_stream, test_chunks_with_metadata_preserved, and test_concurrent_futures.
Not one of them feeds an error-tagged Annotated chunk. The partial-content-then-error path the
issue names is entirely uncovered, which is why the bug survived.

payload.rs has one bus round-trip test,
emit_publishes_one_combined_record_with_and_without_response at line 295. It is a
#[tokio::test] #[serial_test::serial] that emits one record with a response and one with None,
then asserts only request.is_some() and response.is_some() / response.is_none(). It never
looks at payload_complete. It is the natural home for the negative control, because it already
owns the success case.

Host survey

Provider metadata was queried with gh and with git log. All calls were authenticated and
returned results; none were denied.

• gh issue view 14134 — state OPEN, no comments, labels bug, language::rust, dynamo-llm,
  observability.
• gh pr list --state all --search was run for 14134, payload_drop_reason,
  payload_complete in:body, RequestPayloadHandle, request_trace, DeltaAggregator,
  aggregation_failed, and audit record. Searches for payload_drop_reason, 14134, and
  aggregation_failed returned empty. The payload_complete search returned only two pull
  requests: OPEN #14008 and MERGED
  #11386.
• git log over lib/llm/src/request_trace/ confirms that feat(request-trace): sink-agnostic HTTP request-header capture via allowlist #11386 added the HTTP header allowlist
  and did not touch the completeness flags. main still contains the hardcoded literals quoted
  above, so no merged work has delivered this behavior. Nothing has been reverted or superseded.
• git ls-remote --heads origin filtered for audit and payload shows no remote branch matching
  the assigned working branch, so no parallel attempt is in flight.

Four open pull requests touch the same files and are a conflict surface, not competitors:
#14008 rewrites fold_aggregate_with_future and its own description states that the audit record
still emits payload_complete: true and is "unaffected" by that change;
#12571 adds an openai.response endpoint label in
payload.rs; #12511 extends payload capture to the
chat-processor path; #12543 adds media redaction.
The mitigation is scope discipline: keep the diff concentrated on the outcome type, the three
collapse sites, and emit, and do not reformat or restructure anything nearby.

Conclusion: Route: implementation. There is no merged pull request delivering this behavior, so
Disposition: already-resolved does not apply. There is no open pull request implementing it
either, so Template: existing-request does not apply and Template: debug-investigation stands.

Chosen approach

Give the aggregation outcome a place to carry a reason, thread it to the emit site, and derive the
flags from it instead of hardcoding them.

1. Widen the payload future's output. Change PayloadFuture in payload_stream.rs so it
resolves to an outcome carrying both an Option<NvCreateChatCompletionResponse> and an
Option<String> drop reason — a small named struct is preferable to a bare tuple so the two
Options cannot be transposed at a call site. The oneshot channel's item type widens the same
way. The stream side of both scan_aggregate_with_future and fold_aggregate_with_future keeps its
current shape and control flow, so #14008's rewrite of the latter remains a mechanical merge.

2. Populate a reason at each of the three collapse sites, following the otel_sink.rs grammar:

• empty chunk buffer in PassThroughWithAgg::poll_next → empty_response_stream
• aggregation error in PassThroughWithAgg::poll_next and in fold_aggregate_with_future →
  aggregation_failed:<message>, carrying the error text that today only reaches a tracing::warn!
• rx.await resolving to Err(_) with no value sent, i.e. the sender was dropped without ever
  producing an outcome → client_cancelled

The fourth path — the SSE consumer dropping the passthrough stream mid-flight — lands on the
client_cancelled branch, which is the correct label for it and matches the existing comment at
payload_stream.rs lines 104–107.

3. Preserve partial content locally. Where the aggregator returns Err, split the already
buffered Vec<Annotated<NvCreateChatCompletionStreamResponse>> at the first error-bearing chunk and
re-run DeltaAggregator::apply over the error-free prefix. On success the outcome carries both the
partial response and the aggregation_failed:<message> reason, which is exactly the state the flags
now exist to express: content present, completeness false, reason recorded. If the prefix is
empty or itself fails to aggregate, fall back to a reason-only outcome. DeltaAggregator::apply is
not modified, its error contract is untouched, and preserves_typed_error_after_partial_output
keeps passing unchanged.

4. Make emit derive the flags. Change the signature to take the drop reason as a required
second parameter, per the caller enumeration above:

pub fn emit(
    self,
    response: Option<Arc<NvCreateChatCompletionResponse>>,
    drop_reason: Option<String>,
)

Set payload_complete to response.is_some() && drop_reason.is_none() rather than to a literal, so
the flag is a function of the data in the record and cannot drift from it. Add one defensive
invariant: if a caller passes (None, None) — no response and no explanation, which is the exact
shape of today's bug — synthesize a reason such as unspecified rather than publishing another
record that claims completeness while omitting the response. That invariant is itself unit-testable
without any of the streaming machinery.

5. Update the single production call site. preprocessor.rs lines 6482–6493 destructures the
new outcome and passes both parts through. Its existing tracing::debug! on the None branch gains
the reason as a structured field, which is a strict improvement to the operator-visible log.

Why this shape rather than a smaller one: simply flipping the two literals in emit to depend on
response.is_some() would set payload_complete correctly but would leave payload_drop_reason
permanently None, because the reason genuinely does not exist at that point in the program. A
consumer would learn that something was dropped but never which of four causes, which is most of the
diagnostic value the issue is asking for. The channel has to widen for the fix to be worth making.

Rejected alternatives

Change DeltaAggregator::apply to return partial content alongside the error. Rejected on
evidence. preserves_typed_error_after_partial_output at aggregator.rs line 2645 exists
specifically to guarantee that a typed backend error propagates rather than degrading into a
truncated success, and the client-facing non-streaming path at preprocessor.rs line 7185 relies on
that. Changing the return type would touch two production callers and roughly forty test call sites,
and would put a behavior change on the client response path inside what is meant to be an audit
fix. Since payload_stream.rs has already buffered the chunks it needs, the same user-visible
outcome is reachable with a local change and no ripple. This is the issue's second proposal, kept in
substance and rejected only in mechanism.

Add the reason as an optional parameter, or via a setter on the handle. Rejected. With five call
sites, four of them in tests, there is no ergonomic argument for optionality, and a default value
would preserve exactly the failure mode being fixed: a new call site that forgets the reason would
compile and silently publish a dishonest record.

Emit a second, separate record for dropped payloads. Rejected. emit consumes self precisely
to enforce one payload record per request, and the fixture loader in
lib/data-gen/src/request_trace/load.rs and the sinks all assume that invariant. Adding a second
record type would be a schema and consumer change for information that two existing fields already
model.

Fix only payload_complete and leave payload_drop_reason unpopulated. Rejected; reasoning
under Chosen approach.

Include the /v1/completions capture gap. Rejected as a non-goal; reasoning above.

Validation strategy

The change is pure Rust inside lib/llm, with no CUDA, no GPU kernels, no Python surface, no
container, and no deployment topology. Every claim it makes is decidable by compilation and unit
tests, so the validation is entirely CPU-side and the sandbox can execute all of it.

Recipe the validation command — the primary and load-bearing recipe. Its sections run, in order:

• git diff --check and cargo fmt --all --check — proves the diff introduces no whitespace errors
  and matches rustfmt. Cheap, and the repository gates on it.
• cargo check --workspace --all-targets, plus
  cargo check --manifest-path lib/bindings/python/Cargo.toml --all-targets — this is the section
  that proves the caller enumeration in Discovery was complete. Making the emit parameter required
  means every stale call site is a compile error, including the two #[ignore]-marked ones in
  lib/llm/tests/request_trace_nats_integration.rs, which --all-targets compiles even though it
  never runs them. If Discovery missed a caller, this fails loudly instead of silently.
• cargo clippy --workspace --all-targets --no-deps -- -D warnings — proves the new outcome type
  and the prefix-splitting logic introduce no lint regressions, at the repository's deny-warnings
  bar.
• cargo test -p dynamo-llm --lib — runs the new regression tests together with the existing
  request_trace and aggregator suites, so preserves_typed_error_after_partial_output and the
  seven payload_stream.rs tests are proven still green.

The validator must not set CARGO_TARGET_DIR, per that recipe's own warning. Note also that
lib/llm/src/request_trace/AGENTS.md prescribes its own narrower commands for changes in this
folder — cargo fmt --check, cargo check -p dynamo-llm --lib --no-default-features, and
cargo test -p dynamo-llm --lib request_trace --no-default-features. Recipe the validation command
is a strict superset of the first and third, and the --no-default-features check is worth running
additionally because it proves the change does not accidentally depend on the default feature set
(which pulls in Linux/CUDA-oriented NIXL, NUMA, and O_DIRECT code that request_trace has no
business needing). Hardware requirement: none beyond a CPU and the Rust toolchain, both present.

Recipe the validation command — secondary, for its section 2 only. Section 1 is N/A: no Python file
is touched. Section 2 runs pre-commit run --files <changed files> --hook-stage manual, which
applies the repository's file-type-agnostic hooks — SPDX license headers, codespell, end-of-file
and trailing-whitespace — to the changed Rust files. That is a real gate this change can fail
(a new source file without an SPDX header, or a typo in a new drop-reason identifier) and it is not
covered by the validation check. Hardware requirement: none.

Recipes deliberately not nominated. No GPU smoke recipe (the validation command,
the validation command, the validation command) is warranted: the change is library logic behind an env-var
gate, and a GPU smoke would prove only that the frontend still starts — something the unit tests
already prove more precisely and without consuming an A100 for minutes. There is no observable
behavior on a smoke run's happy path, because the flags only differ on the failure paths, which a
smoke run does not deliberately induce. the validation command is not applicable (no Dockerfile is
touched, and the Docker daemon is unavailable in this environment regardless).
the validation command is not nominated for anything the sandbox can execute, which here is
everything. the validation command and the validation command are not applicable: no
Python or binding surface changes.

Regression tests, and why they are not tautological

Three tests, of which two are new failing-then-passing regressions and one is a negative control.

Test 1 — the path the issue names, in payload_stream.rs. Drive
scan_aggregate_with_future with a stream of one content-bearing chunk followed by one
error-tagged Annotated chunk, then await the payload future and assert that the outcome carries a
populated drop reason matching aggregation_failed: and preserves the partial content from the
first chunk.

This is not tautological because it does not assert the shape of the new code — it asserts an
end-to-end property of the module that is unrepresentable today. Against unfixed main this test
does not merely fail an assertion; it does not compile, because PayloadFuture resolves to
Option<NvCreateChatCompletionResponse> and there is no reason to read. Rewritten to compile
against unfixed main (asserting on whatever the future yields), it still fails: the future yields
None, the partial content is gone, and the error text exists only in a tracing::warn!. The test
is therefore pinned to observable behavior a consumer cares about, not to an implementation detail
the fix happens to introduce.

Test 2 — the record itself, in payload.rs. A bus round-trip in the style of the existing
emit_publishes_one_combined_record_with_and_without_response: emit with response = None and a
populated reason, receive the record off the trace bus, and assert
payload_complete == false together with the expected payload_drop_reason. Against unfixed main
this fails on the value, not just the signature: emit writes the literal true and the literal
None regardless of what it is handed. This is the assertion that directly encodes the issue's
complaint, and it is the one a reviewer should look at first.

A third assertion belongs in the same test: pass (None, None) and assert the record still reports
payload_complete == false with a synthesized reason. That pins the defensive invariant from step 4
of the Chosen approach, and it is the test that prevents a future caller from silently
reintroducing this exact bug.

Negative control. The existing success assertions in
emit_publishes_one_combined_record_with_and_without_response must be extended with
payload_complete == true and payload_drop_reason.is_none() for the record that does carry a
response, and must keep passing unchanged in meaning. This is what stops the fix from
over-correcting into "always report incomplete", which would pass Test 2 while destroying the
field's usefulness in the opposite direction. The seven existing payload_stream.rs tests and
preserves_typed_error_after_partial_output serve the same role at module scope: all of them
exercise success paths or the aggregator's error contract, and all of them must remain green
without modification.

Required deliverables

Per debug-investigation, the downstream roles produce:

• plan.md — this document.
• change.md — the implementer's narrative: the outcome type introduced in payload_stream.rs, the
  reason assigned at each of the three collapse sites, the prefix-splitting approach to partial
  content, the widened emit signature and the derivation of payload_complete, and the updated
  call site in preprocessor.rs. It should state the reproducible symptom in one line — an audit
  record with payload_complete: true and no response — and record the smallest distinguishing
  observation, namely that the reason exists in a tracing::warn! at payload_stream.rs line 79
  but has no channel to reach payload.rs line 89.
• change.diff — the patch. Expected to touch lib/llm/src/request_trace/payload.rs,
  lib/llm/src/request_trace/payload_stream.rs, lib/llm/src/preprocessor.rs, and
  lib/llm/tests/request_trace_nats_integration.rs (two mechanical call-site updates). Nothing
  under lib/llm/src/protocols/openai/chat_completions/aggregator.rs, lib/runtime/, or
  lib/data-gen/ should change.
• change-validation.md — the two recipes above, executed, with real command output. A pass
  verdict must be empirical; a section that cannot run is blocked with the failure mode named, not
  pass.
• review.md — reviewer sign-off, with particular attention to whether the negative control still
  asserts payload_complete == true on the success record, and to whether the diff stayed clear of
  fold_aggregate_with_future's control flow given open pull request fix(frontend): observe audit metrics before non-streaming fold #14008.

[glamr-agent]

change.md

Change: record why a captured request payload is incomplete

Issue: ai-dynamo/dynamo#14134
Branch: fix/audit-payload-complete-on-error--1d3133a2347d (from main at 1ca4f3ef5)
Commit: c6f487cb3
MR URL: #14143

The defect

RequestPayloadHandle::emit built every DYN_REQUEST_TRACE payload record with two
hardcoded literals:

payload_complete: true,
payload_drop_reason: None,

So a record emitted after a client cancel, an empty response stream, or a failed
response aggregation asserted that it was a faithful copy of what the client received.
For an audit and compliance surface that is the worst failure shape available: not a
missing record, but a present record that over-claims.

The information needed to say otherwise was destroyed one layer up. The payload future
in lib/llm/src/request_trace/payload_stream.rs resolved to
Option<NvCreateChatCompletionResponse>, and three distinct outcomes all collapsed into
the same None:

• the response stream ended without ever yielding a chunk;
• DeltaAggregator::apply returned Err, with its error text logged and then dropped;
• the sender was dropped without sending, because the SSE consumer let go of the
  pass-through stream before end-of-stream.

By the time the single production call site in lib/llm/src/preprocessor.rs saw None,
the reason no longer existed, so it could not have been recorded even if emit had
accepted one.

I reproduced this before changing anything, by adding a temporary assertion to the
existing bus round-trip test in lib/llm/src/request_trace/payload.rs that a
response-less record must not report itself complete. It failed at
assertion failed: !second_payload.payload_complete, with the other seven tests in that
module passing. The probe was removed before implementation; the permanent assertions
described below replace it.

What changed

Four files, no wire-schema change. payload_complete and payload_drop_reason already
exist on RequestTracePayload and are already serialized; this change populates them
honestly rather than adding fields.

1. The payload future carries a reason (lib/llm/src/request_trace/payload_stream.rs)

PayloadFuture now resolves to a small named struct instead of a bare Option:

pub struct PayloadOutcome {
    pub response: Option<NvCreateChatCompletionResponse>,
    pub drop_reason: Option<String>,
}

The two travel together so the emit site can derive completeness from the data rather
than assuming it. A partial record is Some response and Some reason — a state the
old Option could not express at all.

Each of the three collapse sites now names itself, using the colon-delimited
identifier:detail grammar that lib/llm/src/request_trace/otel_sink.rs already
publishes for its oversize markers, so a consumer parses one grammar across both
producers:

Situation	Reason
Stream ended with no chunks	empty_response_stream
Aggregation returned an error	aggregation_failed:<error>
Sender dropped before reporting	client_cancelled

The aggregation_failed detail is the underlying error's Display output, which for a
DynamoError is itself "{error_type}: {message}". The reason therefore parses as an
identifier plus a free-text remainder — split on the first colon, not on every colon.

2. Partial content is preserved

New helper aggregate_with_partial_recovery finds the first chunk matching
Annotated::is_error(), truncates the already-buffered Vec at that point, and
re-aggregates the error-free prefix. What the client actually saw before the failure is
kept, alongside the reason explaining why the record stops there.

This is sound because Annotated::into_data returns Err for exactly the chunks
is_error() matches, and DeltaAggregator::apply drives the buffer through a try_fold
that short-circuits on the first of them and discards the accumulator. A buffer
containing an error-tagged chunk can therefore only aggregate to Err; aggregating the
prefix is the only way to recover anything.

DeltaAggregator::apply itself is deliberately not modified. Its short-circuit is
load-bearing on the client-facing non-streaming path, where a typed backend error must
surface as an error rather than as a truncated success, and it is guarded by
preserves_typed_error_after_partial_output in
lib/llm/src/protocols/openai/chat_completions/aggregator.rs. The recovery is local to
the capture path.

The implementation avoids cloning the whole buffer — which would double peak memory on
every capture-enabled request. It locates the error index first, clones only that single
chunk to extract its error, then truncates in place.

fold_aggregate_with_future's control flow is left as it was. The only edits there are
the channel's type parameter and replacing a silent drop(tx) with a tx.send(...) that
states the reason. This keeps the conflict surface against the in-flight rewrite of that
function to an async_stream::stream! generator as small as it can be.

3. emit requires the reason (lib/llm/src/request_trace/payload.rs)

pub fn emit(
    self,
    response: Option<Arc<NvCreateChatCompletionResponse>>,
    drop_reason: Option<String>,
)

payload_complete is now derived: response.is_some() && drop_reason.is_none(). Making
the reason a required second parameter rather than a defaulted one is the point — a new
call site cannot quietly re-introduce an over-claiming record, because it has to state
what it knows.

A defensive branch covers (None, None): a record with neither a response nor a reason
asserts nothing about why it is empty, which is exactly the state this signature exists
to prevent. It is marked incomplete with the reason unspecified rather than being
allowed to look complete.

4. Call sites

• lib/llm/src/preprocessor.rs — the single production call site. It now logs the reason
  and whether a partial response survived, then forwards both to emit.
• lib/llm/tests/request_trace_nats_integration.rs — three mechanical updates in
  #[ignore]-marked tests (they require a local NATS server and do not run here).

Tests

Three new tests, plus assertions added to existing ones as the negative control:

• error_chunk_mid_stream_keeps_partial_content_and_names_the_error — the regression
  itself. A stream delivers "Hello ", then an error chunk, then more content. Asserts
  the record keeps "Hello ", that the error chunk still reaches the client unchanged,
  and that the reason starts with aggregation_failed: and names the underlying error.
• error_as_first_chunk_reports_reason_without_a_response — nothing was delivered before
  the error, so there is no partial content to keep, but the record must still say why it
  is empty.
• emit_without_response_or_reason_still_marks_the_record_incomplete — the (None, None)
  defensive invariant, exercised through a real bus round-trip.

Extended:

• test_empty_stream_handling and test_concurrent_futures now assert the specific
  reason, so an empty stream stays distinguishable from a client cancel.
• emit_publishes_one_combined_record_with_and_without_response asserts payload_complete
  and payload_drop_reason in both directions. The success case is the negative
  control: a faithful record must still report itself complete with no reason, so the flag
  does not become vacuously false.
• The three success-path tests in payload_stream.rs assert drop_reason.is_none().

Deviation from the plan, stated plainly

The plan expected the seven existing payload_stream.rs tests to remain green without
modification. That is not achievable: widening the future's output from
Option<NvCreateChatCompletionResponse> to PayloadOutcome makes future.await.is_none()
and future.await.expect(...) fail to compile at the call site. I adapted them
mechanically — .response.is_none(), .response.expect(...) — so every existing
assertion's meaning is preserved, and added the new reason assertions on top. No
existing assertion was weakened or removed. Recording this because a reviewer comparing
against the plan will otherwise see modified test bodies where none were predicted.

Validation

Everything below was run in the work-item checkout. All exit codes were zero.

Two toolchain notes, stated because they affect how much these results prove:

• The repository pins toolchain 1.93.1, which in this sandbox has neither rustfmt nor
  clippy installed. Both are available on the 1.96.1 toolchain that is also present, so
  formatting and lint ran there, via rustup run. CI runs them on the pinned toolchain;
  a pinned-toolchain-only lint would not be caught here.
• cargo fmt -- <file> does not pass the crate edition through to rustfmt, so it
  rejects the edition-2024 let-chains already in lib/llm/src/preprocessor.rs. I formatted
  each changed file with an explicit --edition 2024 and then confirmed the workspace with
  cargo fmt --all --check.

Formatting, per changed file:

rustup run 1.96.1-x86_64-unknown-linux-gnu rustfmt --edition 2024 lib/llm/src/request_trace/payload_stream.rs
rustup run 1.96.1-x86_64-unknown-linux-gnu rustfmt --edition 2024 lib/llm/src/request_trace/payload.rs
rustup run 1.96.1-x86_64-unknown-linux-gnu rustfmt --edition 2024 lib/llm/src/preprocessor.rs
rustup run 1.96.1-x86_64-unknown-linux-gnu rustfmt --edition 2024 lib/llm/tests/request_trace_nats_integration.rs

Whitespace and formatting:

git diff --check
rustup run 1.96.1-x86_64-unknown-linux-gnu cargo fmt --all --check

Both clean, no output.

Folder-local checks required by lib/llm/src/request_trace/AGENTS.md:

cargo check -p dynamo-llm --lib --no-default-features
cargo test -p dynamo-llm --lib --no-default-features request_trace

Finished dev profile ... in 1m 37s, then
test result: ok. 65 passed; 0 failed; 0 ignored; 0 measured; 2251 filtered out; finished in 0.82s.
All three new tests are in that set.

Recipe the validation command:

cargo check --workspace --all-targets
cargo check --manifest-path lib/bindings/python/Cargo.toml --all-targets
rustup run 1.96.1-x86_64-unknown-linux-gnu cargo clippy --workspace --all-targets --no-deps -- -D warnings
cargo test -p dynamo-llm --lib

Finished dev profile ... in 1m 57s; Finished dev profile ... in 1m 48s;
Finished dev profile ... in 2m 40s with no diagnostics emitted; and
test result: ok. 2516 passed; 0 failed; 5 ignored; 0 measured; 0 filtered out; finished in 7.95s.

Recipe the validation command, section 2:

pre-commit run --files lib/llm/src/request_trace/payload_stream.rs lib/llm/src/request_trace/payload.rs lib/llm/src/preprocessor.rs lib/llm/tests/request_trace_nats_integration.rs --hook-stage manual

Every applicable hook passed (codespell, check for case conflicts,
check for merge conflicts, mixed line ending, trim trailing whitespace,
check that scripts with shebangs are executable, Report pytest markers,
Validate AGENTS.md and CLAUDE.md pairs). Python- and docs-specific hooks reported
(no files to check) Skipped, correctly — this change touches no Python or docs.

Not validated here

• The three #[ignore]-marked tests in lib/llm/tests/request_trace_nats_integration.rs
  need a NATS server on localhost and were updated but not executed. Their changes are
  mechanical signature updates.
• No end-to-end run against a live inference backend. The reasons are asserted at the unit
  boundary where they are produced, not observed on a real cancelled request.

[glamr-agent]

change-validation.md

Validation: record why a captured request payload is incomplete

Branch fix/audit-payload-complete-on-error--1d3133a2347d at commit c6f487cb3,
validated in the work-item checkout of this repository.

Target

Everything below ran locally on the validation host: a single-GPU Linux container
(one NVIDIA A100-SXM4-80GB, CUDA 13.0). No GPU, engine, cluster, or model was
exercised, and none is required — the change is confined to four Rust files in the
dynamo-llm crate and its integration test, and every assertion it adds is reachable
from cargo test. There is no Kubernetes or inference-serving surface in this diff.

I re-ran every command myself. Nothing below is carried over from the change
description; where my results differ from it, I say so.

Checks the changed files with the repository's fast lint and formatting commands

No Python file is touched by this change. I confirmed that rather than assuming it:

git diff --name-only HEAD~1..HEAD

returns exactly lib/llm/src/preprocessor.rs,
lib/llm/src/request_trace/payload.rs,
lib/llm/src/request_trace/payload_stream.rs, and
lib/llm/tests/request_trace_nats_integration.rs; the same command filtered with
-- "*.py" returns zero files. The per-file byte-compile step therefore has no
input, and I did not manufacture one.

The repository's hook set ran against the four changed files:

pre-commit run --files lib/llm/src/preprocessor.rs lib/llm/src/request_trace/payload.rs lib/llm/src/request_trace/payload_stream.rs lib/llm/tests/request_trace_nats_integration.rs --hook-stage manual

Exit 0. The hooks with matching inputs all reported Passed — codespell,
check for case conflicts, check for merge conflicts, mixed line ending,
trim trailing whitespace,
check that scripts with shebangs are executable,
Report pytest markers (static + inherited), and
Validate AGENTS.md and CLAUDE.md pairs. The Python-, JSON/YAML/TOML-, and
docs-specific hooks reported (no files to check) Skipped, which is correct for a
Rust-only diff.

Checks the changed Rust crates with cargo check and Clippy

Whitespace and formatting:

git diff --check
rustup run 1.96.1-x86_64-unknown-linux-gnu cargo fmt --all --check

Both exit 0 with no output — no whitespace errors, and no file in the workspace
differs from rustfmt output. (Why the explicit toolchain: see the first finding
below.)

Compilation across the workspace and the Python extension crate:

cargo check --workspace --all-targets
cargo check --manifest-path lib/bindings/python/Cargo.toml --all-targets

Exit 0 each, Finished dev profile [unoptimized + debuginfo] target(s) in 14.14s
and ... in 12.23s. The second matters here because lib/bindings/python is outside
the Cargo workspace, so a signature change like RequestPayloadHandle::emit gaining a
second parameter would not be caught by the workspace check alone.

Lint:

rustup run 1.96.1-x86_64-unknown-linux-gnu cargo clippy --workspace --all-targets --no-deps -- -D warnings

Exit 0, Finished dev profile [unoptimized + debuginfo] target(s) in 33.23s. I
checked that this was not a cache no-op: the log contains
Checking dynamo-llm v1.5.0 (/…/lib/llm), so the changed crate was genuinely
re-linted. The only warning: lines in the output are cargo:warning= messages
emitted by the kvbm-kernels and dynamo-llm build scripts about CUDA fatbin
generation; there are no lint diagnostics, and -D warnings would have failed the run
if there were.

The folder-local commands that lib/llm/src/request_trace/AGENTS.md prescribes, run
additionally to prove the change picks up no default-feature dependency:

cargo check -p dynamo-llm --lib --no-default-features
cargo test -p dynamo-llm --lib --no-default-features request_trace

Exit 0 each: Finished dev profile [unoptimized + debuginfo] target(s) in 7.98s,
then test result: ok. 65 passed; 0 failed; 0 ignored; 0 measured; 2251 filtered out; finished in 0.83s. The new code compiles and its tests pass with the crate's default
features off, so nothing in this change depends on them.

The crate's full library test suite:

cargo test -p dynamo-llm --lib

Exit 0, test result: ok. 2516 passed; 0 failed; 5 ignored; 0 measured; 0 filtered out; finished in 7.93s. The five ignored tests are pre-existing and unrelated to this
change; the three #[ignore]-marked tests updated in
lib/llm/tests/request_trace_nats_integration.rs are in a separate integration target
and need a local NATS server, so they compiled but did not execute here.

Does the change actually do what it claims

A green suite proves the crate still builds and its assertions hold; it does not by
itself prove the new behaviour. I ran the decisive tests individually, in both
polarities, against the real crate binary.

Positive control — a faithful capture must still report itself complete, so the new
payload_complete derivation cannot be vacuously false:

cargo test -p dynamo-llm --lib -- --exact request_trace::payload::tests::emit_publishes_one_combined_record_with_and_without_response --nocapture

Exit 0, 1 passed … 2520 filtered out. This test asserts in both directions across a
real bus round-trip: the with-response record carries payload_complete == true and
payload_drop_reason == None, the response-less one does not.

Negative controls — the three failure shapes the change exists to name:

cargo test -p dynamo-llm --lib -- --exact request_trace::payload_stream::tests::error_chunk_mid_stream_keeps_partial_content_and_names_the_error --nocapture
cargo test -p dynamo-llm --lib -- --exact request_trace::payload_stream::tests::error_as_first_chunk_reports_reason_without_a_response --nocapture
cargo test -p dynamo-llm --lib -- --exact request_trace::payload::tests::emit_without_response_or_reason_still_marks_the_record_incomplete --nocapture

Exit 0 each, 1 passed each. Read together with the positive control, these are the
empirical core of the change: an error mid-stream keeps the content the client actually
saw and labels the record with an aggregation_failed: reason naming the underlying
error; an error as the very first chunk yields no response but still a reason; and the
(None, None) case — neither a response nor a stated reason — is marked incomplete with
unspecified rather than being allowed to look complete. That last one is the invariant
that makes the widened signature worth having, because it is what a careless future call
site would hit.

The whole stream-side module:

cargo test -p dynamo-llm --lib -- request_trace::payload_stream::tests

Exit 0, test result: ok. 9 passed; 0 failed; 0 ignored; 0 measured; 2512 filtered out — the seven that existed before plus the two added here.

Finding 1: the formatting and lint evidence is stronger than the change description states

The change description says the repository pins toolchain 1.93.1 and that rustfmt
and clippy were therefore run on a different toolchain, 1.96.1, that happens to be
present — with the caveat that a lint specific to the pinned toolchain would go
uncaught. I probed this directly and it is backwards.

grep -A2 '\[toolchain\]' rust-toolchain.toml

[toolchain]
channel = "1.96.1"

The repository pins 1.96.1. 1.93.1 is active only because the container sets
RUSTUP_TOOLCHAIN=1.93.1, which overrides the pin file. rustup toolchain list
confirms the split:

1.93.1-x86_64-unknown-linux-gnu (active)
1.96.1-x86_64-unknown-linux-gnu (default)

and the installed components explain the rest — 1.93.1 has only
cargo, rust-std, and rustc, while 1.96.1 additionally has clippy and
rustfmt. So cargo fmt and cargo clippy genuinely cannot run under the active
toolchain, but running them under 1.96.1 ran them on the repository-pinned
toolchain — which is what CI uses. That evidence is therefore stronger than described,
not weaker, and the stated caveat does not apply.

The real gap runs the other way: cargo check and cargo test invoked plainly use the
environment's active 1.93.1, one minor version below the pin. I closed that gap by
re-running the crate's full library suite under the pinned toolchain as well:

rustup run 1.96.1-x86_64-unknown-linux-gnu cargo test -p dynamo-llm --lib

Exit 0, test result: ok. 2516 passed; 0 failed; 5 ignored; 0 measured; 0 filtered out; finished in 7.92s — identical to the run on 1.93.1. The change compiles and
passes on both.

Finding 2: no assertion in the modified tests was weakened or removed

The plan predicted the seven existing tests in
lib/llm/src/request_trace/payload_stream.rs would stay green without modification;
five of them were in fact edited. I read every edit in the diff rather than taking the
change description's word for it. The finding matches its claim.

Two of the seven — test_final_response_to_one_chunk_preserves_reasoning_and_tool_calls
and test_chunks_with_metadata_preserved — are untouched. The other five
(test_passthrough_forwards_chunks_unchanged,
test_passthrough_aggregates_reasoning_content_and_tool_calls,
test_empty_stream_handling, test_single_chunk_stream, test_concurrent_futures)
change only in how they reach the value the future now resolves to. Widening the
future's output from Option<NvCreateChatCompletionResponse> to a PayloadOutcome
struct makes future.await.is_none() and future.await.expect(…) fail to compile, so
each edit is a mechanical field access — outcome.response.is_none(),
outcome.response.expect(…), outcome1.response.is_none(). Every predicate that was
asserted before is still asserted, over the same value, with the same expected result.
Nothing was deleted, relaxed, or converted to a weaker check. Several of the five gained
additional assertions on drop_reason, so on balance they check strictly more than
before: test_empty_stream_handling and test_concurrent_futures now pin the specific
reason, which is what keeps an empty stream distinguishable from a client cancel.

The prediction was simply not achievable — a type widening that a test observes must be
observed differently — and the adaptation is the minimum one. This is worth a
reviewer's glance at the diff, but it is not a loss of coverage.

What is not proven here

• The three #[ignore]-marked tests in
  lib/llm/tests/request_trace_nats_integration.rs need a NATS server on localhost.
  They compile under cargo check --workspace --all-targets but did not execute, so
  their updated call sites are proven to type-check and not to run. Their edits are
  mechanical argument additions.
• No end-to-end run against a live inference backend. The client_cancelled reason in
  particular is asserted at the unit boundary where it is produced, not observed on a
  real cancelled request through the frontend. The production call site in
  lib/llm/src/preprocessor.rs is exercised only by compilation.
• DeltaAggregator::apply is deliberately unchanged, and its existing guard
  preserves_typed_error_after_partial_output in
  lib/llm/src/protocols/openai/chat_completions/aggregator.rs passed as part of the
  full crate suite — so the client-facing behaviour, where a typed backend error must
  surface as an error rather than a truncated success, is confirmed unaltered by the new
  capture-path recovery.

Verdict: pass — every command listed above ran here and exited zero, and the new behaviour is proven by paired positive and negative tests rather than by a green suite alone

[glamr-agent]

review.md

🤖 Automated AI review — advisory. An AI agent's judgment of
whether this change is logically sound based on the code and reported
validation results. This is not an approval. Repository CI and human
reviewers decide whether to merge.

Review: record why a captured request payload is incomplete

Branch fix/audit-payload-complete-on-error--1d3133a2347d, commit c6f487cb3,
inspected in the working clone with a clean tree. The commit touches four files:

lib/llm/src/preprocessor.rs                     |  29 +--
lib/llm/src/request_trace/payload.rs            |  91 +++++++-
lib/llm/src/request_trace/payload_stream.rs     | 293 +++++++++++++++++++-----
lib/llm/tests/request_trace_nats_integration.rs |  12 +-

I read the post-change source in the clone rather than reviewing from the diff
alone, and I checked the claims that the change and validation write-ups make
about code they do not modify — Annotated, DeltaAggregator::apply, and the
OTLP sink's existing drop-reason grammar.

Does the fix address the cause or only the symptom

It addresses the cause. The defect had two halves, and both are fixed at the
place where the information was lost.

The first half is that RequestPayloadHandle::emit wrote the two literals
payload_complete: true and payload_drop_reason: None into every record. That
is now derived, in lib/llm/src/request_trace/payload.rs:97:

let payload_complete = response.is_some() && drop_reason.is_none();

This is a genuine derivation from the record's own data, not a re-hardcoding
behind a new name: both operands come from the call's arguments, neither is a
constant, and both values reach the emitted RequestTracePayload unchanged at
payload.rs:114-115. Making the reason a required second parameter rather than
a defaulted one is what stops a future call site from silently re-introducing an
over-claiming record — the type system now asks the question.

The second half is that the reason did not survive the trip. The payload future
resolved to Option<NvCreateChatCompletionResponse>, which collapsed three
different outcomes into one None. It now resolves to a PayloadOutcome that
carries the response and the reason together, so a partial capture — some
response and a reason — is representable for the first time.

The (None, None) invariant

payload.rs:101-105 normalizes the case where a caller passes neither a
response nor a reason:

let drop_reason = match (payload_complete, drop_reason) {
    (true, reason) => reason,
    (false, Some(reason)) => Some(reason),
    (false, None) => Some(DROP_UNSPECIFIED.to_string()),
};

The invariant holds by construction. payload_complete is false only when the
response is absent or a reason is present; if a reason is present the second arm
keeps it, so the third arm is reachable only when the response is None and no
reason was given — exactly the state it names unspecified. The first arm is
reachable only with reason == None, since payload_complete is true only in
that case, so it cannot pass a reason through on a record that claims to be
complete. There is no input for which a record can carry payload_complete == true alongside a non-empty reason, and none for which an empty record can claim
completeness.

Is the negative control real

Yes — this was the specific risk worth checking, because a fix of this shape can
easily over-correct into "always report incomplete", which would be just as
useless for an audit surface and would still pass every new test.

emit_publishes_one_combined_record_with_and_without_response
(lib/llm/src/request_trace/payload.rs:320) asserts both directions across a
real bus round-trip. On the record that does carry a response
(payload.rs:371-372):

assert!(first_payload.payload_complete);
assert!(first_payload.payload_drop_reason.is_none());

and on the record that does not (payload.rs:383-388), that
payload_complete is false and the reason is exactly client_cancelled. Both
assertions are on records recovered from the bus, keyed by request id, not on
the struct handed to emit. So a regression that flipped completeness to a
constant false would fail here. The success-path tests in
payload_stream.rs similarly assert drop_reason.is_none(), so the reason
field is pinned in the negative direction too.

Partial-content recovery

aggregate_with_partial_recovery (lib/llm/src/request_trace/payload_stream.rs:140-171)
rests on a premise the write-up states but does not prove: that
Annotated::into_data returns Err for exactly the chunks is_error() matches.
I checked it in lib/runtime/src/protocols/annotated.rs — is_error() is
self.event.as_deref() == Some("error"), cloned_error() is
self.is_error().then(...), and into_data() returns Err precisely when
cloned_error() is Some. They are the same predicate, so the premise holds
and the Ok(_) => "unknown error" arm really is unreachable today. Keeping it
as a total match rather than an unwrap is the right call.

I also confirmed the other half of the premise in
lib/llm/src/protocols/openai/chat_completions/aggregator.rs: apply is a
try_fold over Annotated::into_data, which short-circuits on the first Err
and discards the accumulator. A buffer containing an error-tagged chunk can
therefore only aggregate to Err, so aggregating the prefix is the only way to
recover what the client already received. apply itself is untouched, which is
correct: its short-circuit is load-bearing on the client-facing non-streaming
path, where a typed backend error must surface as an error rather than as a
truncated success.

The boundaries are all handled, and none of them double-counts or drops content:

• Error is the first chunk. position returns 0, truncate(0) empties the
  buffer, the chunks.is_empty() guard returns a reason-only outcome. No
  aggregation runs, nothing to double-count. Covered by
  error_as_first_chunk_reports_reason_without_a_response.
• No error chunk present. The let ... else takes the ordinary path: one
  aggregation over the whole buffer, Ok becoming a complete outcome and Err
  becoming an aggregation_failed: reason with no response.
• Prefix itself fails to aggregate. .ok() turns it into None, so the
  record is incomplete with a reason rather than panicking or claiming success.
• Double-counting. Not possible. On the error path the full buffer is never
  aggregated — truncate happens before the only apply call on that path, and
  that call consumes the vector once. The single chunk that is cloned is the
  error chunk, cloned only to read its error text, and it is then truncated away.
  The write-up's claim that the whole buffer is not cloned is accurate.

One narrow behavioural note, not a defect: if the prefix fails to aggregate for
a cause unrelated to the error chunk, the reported reason still names the error
chunk and the partial silently becomes None. The record is correctly marked
incomplete either way, and the aggregator logs the underlying failure, so the
audit record is not misleading — only slightly less specific than it could be.

Drop-reason grammar and leak risk

The three reasons are empty_response_stream, client_cancelled, and
aggregation_failed:<error>. The first two are bare identifiers; the third is
built by payload_stream.rs:63-65:

fn aggregation_failed_reason(error: impl std::fmt::Display) -> String {
    format!("aggregation_failed:{error}")
}

The existing producer in lib/llm/src/request_trace/otel_sink.rs:166-170 emits
otel_payload_too_large:max_bytes={}:actual_bytes={}. So both are
colon-delimited with an identifier first, but the existing one has structured
key=value segments after the identifier while the new one has free text that
may itself contain colons — a DynamoError renders as "{error_type}: {message}". A consumer can only parse both by splitting on the first colon and
treating the remainder as opaque. The change description says this; the code does
not, and the code is what a future maintainer reads. This is a nit, recorded
below.

On leakage: the detail is an inference-engine error message, and the record it
travels in already carries the full request and response bodies, so the reason
adds no category of data that the audit record does not already contain. I do not
see a disclosure concern. The unbounded length is worth a thought, though —
see the second nit.

Scope discipline against the in-flight rewrite

The plan called for fold_aggregate_with_future to keep its control flow because
of the open rewrite of that function to an async_stream::stream! generator. I
compared the function in the clone against the pre-change version. The control
flow is unchanged: same branch structure, same spawn, same ordering. The only
edits are the channel's type parameter, the value sent on success, and the
replacement of a silent drop(tx) with

let _ = tx.send(PayloadOutcome::dropped(None, aggregation_failed_reason(&e)));

Both are forced by the type widening and by the fix itself — the reason has to be
sent from where it is known, and the silent drop was one of the three sites that
destroyed it. That is within the plan's discipline, not a scope violation. It is
a small merge-conflict surface rather than a large one: a rewrite of that function
will have to re-apply a send on the failure path, but the conflict is a handful of
lines in one function rather than a restructured body. I would rather the reason
be sent than deferred to avoid a conflict, since deferring it would leave the
client_cancelled and aggregation_failed cases indistinguishable — the defect
itself.

The other non-goals hold. Nothing under
lib/llm/src/protocols/openai/chat_completions/aggregator.rs, lib/runtime/, or
lib/data-gen/ is touched; no /v1/completions capture is added; and no wire
schema changes — payload_complete and payload_drop_reason already exist on
RequestTracePayload and are already serialized, so this populates fields rather
than adding them. lib/llm/tests/request_trace_nats_integration.rs gains only
argument additions at three call sites.

Tests

Three added tests, and each fails against the unmodified code:

• error_chunk_mid_stream_keeps_partial_content_and_names_the_error
  (payload_stream.rs:824) asserts the record keeps "Hello ", that all three
  chunks still reach the client unchanged, and that the reason names the error.
  Before the change the future resolved to None with no reason at all, so
  every one of those assertions fails on revert.
• error_as_first_chunk_reports_reason_without_a_response
  (payload_stream.rs:871) asserts a reason exists where previously none did.
• emit_without_response_or_reason_still_marks_the_record_incomplete
  (payload.rs:394) asserts payload_complete == false against code that
  hardcoded true.

None is tautological, and none asserts the shape of the new implementation in
place of behaviour: they assert what a consumer of the audit record observes.

The five mechanically modified tests preserve every assertion's meaning. The
widening from Option<NvCreateChatCompletionResponse> to a struct makes
future.await.is_none() and future.await.expect(...) fail to compile, so each
edit is a field access — outcome.response.is_none(),
outcome.response.expect(...), outcome1.response.is_none(). Same predicate,
same value, same expected result; nothing relaxed or deleted. Two of them gained
assertions on the specific reason, so an empty stream stays distinguishable from
a client cancel. The change description flags this as a deviation from the plan's
prediction that the tests would need no edits. The prediction was not achievable —
a type change a test observes must be observed differently — and the adaptation
is the minimum one.

Validation evidence

The verdict is an unqualified pass, not a hedged one. The recorded commands all
exited zero: cargo check --workspace --all-targets;
cargo check --manifest-path lib/bindings/python/Cargo.toml --all-targets
(worth having, since that crate is outside the workspace and would otherwise miss
the emit signature change); cargo clippy --workspace --all-targets --no-deps -- -D warnings; cargo fmt --all --check; git diff --check;
cargo check -p dynamo-llm --lib --no-default-features and the matching
request_trace test run; the full cargo test -p dynamo-llm --lib at
2516 passed; 0 failed; 5 ignored; the four decisive tests run individually with
--exact; and pre-commit over the four changed files.

Two things raise my confidence in that record. The validator ran the positive
control and the three negative controls as separate single-test invocations
rather than resting on the aggregate suite, which is what distinguishes "the
suite is green" from "the new behaviour is proven". And it corrected the change
description's claim about the toolchain rather than repeating it: the repository
pins 1.96.1 in rust-toolchain.toml, so running fmt and clippy under
1.96.1 ran them on the pinned toolchain, and the full suite was additionally
re-run there with an identical result.

Two gaps are stated and neither is load-bearing. The three #[ignore]-marked
tests in lib/llm/tests/request_trace_nats_integration.rs need a local NATS
server; they compile but did not run, and their edits are argument additions. And
client_cancelled is asserted where it is produced rather than observed on a real
cancelled request through the frontend; the production call site in
lib/llm/src/preprocessor.rs is covered by compilation only. That call site is
seven lines of straight-line code — await, log if a reason is present, forward
both values to emit — with no branching worth an end-to-end test.

Findings

Nit — payload_stream.rs:63. The aggregation_failed: reason embeds an
error's Display output verbatim, and that text can itself contain colons, so
the reason parses only if a consumer splits on the first colon. The neighbouring
producer at otel_sink.rs:166 uses structured key=value segments after its
identifier. The distinction is real and is documented only in the pull request
description. A sentence in the doc comment on aggregation_failed_reason stating
that the detail is opaque free text and must be split off at the first colon would
put it where the next maintainer will look.

Nit — payload_stream.rs:63, interaction with the OTLP sink. The reason is
unbounded in length because the error message is. Two consequences worth a
moment's thought. A very long reason makes the serialized record larger, and
otel_sink.rs:144-171 drops a record over otel_max_payload_bytes to a marker;
when it does, otel_sink.rs:203 overwrites payload_drop_reason with
otel_payload_too_large:..., discarding the aggregation_failed: reason
entirely. That overwrite is pre-existing, but it was harmless before this change,
because payload_drop_reason was always None on arrival. Now there are two
producers and the marker silently wins. Truncating the detail to a bounded length
would address the size half; whether the sink should preserve or compose the
upstream reason is a larger question and probably belongs in a separate change.

Neither finding blocks. Both are about how a downstream consumer reads the reason
string, not about whether the record now tells the truth — which it does.

Assessment: sound

[coderabbitai]

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 796063c7-2bcf-42d7-bef6-96bfe0741632

📥 Commits

Reviewing files that changed from the base of the PR and between 4a3943c and a343096.

📒 Files selected for processing (4)

• lib/llm/src/preprocessor.rs
• lib/llm/src/request_trace/payload.rs
• lib/llm/src/request_trace/payload_stream.rs
• lib/llm/tests/request_trace_nats_integration.rs

Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review.

---

Walkthrough

Streaming payload aggregation now returns explicit outcomes for successful, empty, cancelled, and failed streams. Partial responses are preserved when possible. Request-trace records now report completeness and drop reasons.

Changes

Request payload outcome tracking

Layer / File(s)	Summary
Payload outcome aggregation lib/llm/src/request_trace/payload_stream.rs	Aggregation now returns PayloadOutcome values with explicit drop reasons. Empty streams and cancellations report distinct reasons. Mid-stream failures preserve preceding response content. Tests cover successful, empty, cancelled, and failed outcomes.
Request-trace emission lib/llm/src/preprocessor.rs, lib/llm/src/request_trace/payload.rs	Payload emission records incomplete status and drop reasons, including the "unspecified" fallback. The preprocessor emits recovered partial responses with their drop reasons.
Integration emission contract lib/llm/tests/request_trace_nats_integration.rs	Integration tests use the expanded emission API for successful and dropped records.

Priority: ➖ Normal

Estimated code review effort: 4 (Complex) | ~45 minutes

Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to a3430

The payload audit fixes are covered by targeted tests, preserve client behavior, and have no identified merge-blocking risk.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 62.50% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 24 functions across 4 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Issue #14134 requires incomplete records when no response exists, a populated drop reason, partial-response recovery, regression coverage, and unchanged client-facing error behavior. `RequestPayloadHa…
Out of Scope Changes check	✅ Passed	The changes stay within Issue #14134. They update request-payload emission, payload aggregation and outcome plumbing, related call sites, and request-trace tests. The changes do not alter the wire sch…
Title check	✅ Passed	The title clearly and concisely describes the main change: preventing request-trace audit records from claiming completeness when the response was dropped.
Description check	✅ Passed	The description explains the problem, implementation, affected files, validation status, reviewer starting point, and linked issue. It uses a Summary heading instead of the template's Overview heading…

• Fix all pre-merge checks with AI

---

_{Comment @coderabbitai help to get the list of available commands.}

[glamr-agent]

CI passed on head d2781d2f. Every check scheduled for it reached a terminal state: 21 successful, 7 skipped by path filters, none failed and none left pending. The Rust build, clippy, and test jobs are among the successes, as is pre-merge-status-check. What remains needs a maintainer rather than a change: the merge state is BLOCKED on REVIEW_REQUIRED, awaiting an approving review.

[glamr-agent]

Repair round 1 — head 7725214.

Three review findings addressed, one by changing the code and two by answering in-thread with the evidence:

• aggregate_with_partial_recovery doc comment narrated implementation detail. Trimmed to the constraint the code does not show, in 7725214.
• Two-argument RequestPayloadHandle::emit as a source break. Declined: lib/llm/AGENTS.md states the crate makes no stability promise for in-process Rust APIs unless documented, and requires every compatibility shim to name a removal condition, which a permanent lenient overload cannot.
• Removing emit_without_response_or_reason_still_marks_the_record_incomplete. Declined: it is the only coverage of the (false, None) arm that substitutes DROP_UNSPECIFIED, which no other test or production path reaches.

Checks run on the new head, all passing:

cargo fmt --check
cargo check -p dynamo-llm --lib --no-default-features
cargo test -p dynamo-llm --lib request_trace --no-default-features

The last reported 65 passed; 0 failed. The change is comment-only, so no caller signature moved.

[glamr-agent]

/devin review

@coderabbitai full review

[coderabbitai]

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '== Revision and changed files =='
git rev-parse HEAD
git diff --stat HEAD^ HEAD
git diff --name-only HEAD^ HEAD

printf '%s\n' '== Repository instructions =='
fd -HI '^(AGENTS|CLAUDE)\.md$' . -E .git | sort
for f in $(fd -HI '^(AGENTS|CLAUDE)\.md$' . -E .git | sort); do
  echo "--- $f"
  cat "$f"
done

printf '%s\n' '== Changed-file outlines =='
for f in \
  lib/llm/src/preprocessor.rs \
  lib/llm/src/request_trace/payload.rs \
  lib/llm/src/request_trace/payload_stream.rs \
  lib/llm/tests/request_trace_nats_integration.rs
do
  if test -f "$f"; then
    echo "--- $f"
    ast-grep outline "$f" --items all || true
  fi
done

printf '%s\n' '== PR diff =='
git diff --find-renames --find-copies --unified=80 HEAD^ HEAD -- \
  lib/llm/src/preprocessor.rs \
  lib/llm/src/request_trace/payload.rs \
  lib/llm/src/request_trace/payload_stream.rs \
  lib/llm/tests/request_trace_nats_integration.rs

printf '%s\n' '== Relevant contract locations =='
rg -n -C 4 --type rust \
  '\b(struct|type|fn)\s+(PayloadOutcome|PayloadFuture|RequestPayloadHandle|DeltaAggregator)|\b(emit|scan_aggregate_with_future|fold_aggregate_with_future|aggregate_with_partial_recovery|is_error|into_data)\s*\(' \
  lib/llm/src lib/runtime/src

Length of output: 158406

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

base_ref=""
for candidate in origin/main main origin/master master; do
  if git rev-parse --verify -q "$candidate" >/dev/null; then
    base_ref="$candidate"
    break
  fi
done
test -n "$base_ref"
merge_base=$(git merge-base "$base_ref" HEAD)

printf 'base_ref=%s\nmerge_base=%s\nhead=%s\n' "$base_ref" "$merge_base" "$(git rev-parse HEAD)"
printf '%s\n' '== Complete PR file list and statistics =='
git diff --stat "$merge_base" HEAD
git diff --name-status "$merge_base" HEAD

printf '%s\n' '== Complete PR diff =='
git diff --find-renames --find-copies --unified=45 "$merge_base" HEAD -- \
  lib/llm/src/preprocessor.rs \
  lib/llm/src/request_trace/payload.rs \
  lib/llm/src/request_trace/payload_stream.rs \
  lib/llm/tests/request_trace_nats_integration.rs

printf '%s\n' '== Targeted post-change source =='
sed -n '1,235p' lib/llm/src/request_trace/payload_stream.rs
sed -n '50,155p' lib/llm/src/request_trace/payload.rs
sed -n '6440,6510p' lib/llm/src/preprocessor.rs
sed -n '120,190p' lib/llm/src/request_trace/otel_sink.rs
sed -n '130,230p' lib/llm/src/protocols/openai/chat_completions/aggregator.rs
sed -n '1,165p' lib/runtime/src/protocols/annotated.rs

printf '%s\n' '== Full changed API call-site and test coverage search =='
rg -n -C 3 --type rust \
  'RequestPayloadHandle::|\.emit\(|scan_aggregate_with_future\(|fold_aggregate_with_future\(|PayloadOutcome|aggregate_with_partial_recovery|DROP_(EMPTY_RESPONSE_STREAM|CLIENT_CANCELLED|UNSPECIFIED)' \
  lib/llm lib/runtime

printf '%s\n' '== Changed test source =='
sed -n '300,440p' lib/llm/src/request_trace/payload.rs
sed -n '500,910p' lib/llm/src/request_trace/payload_stream.rs
sed -n '1,260p' lib/llm/tests/request_trace_nats_integration.rs

Length of output: 145950

---

@glamr-agent Full review complete.

I found no blocking issue in the current PR head.

The change correctly carries the aggregation outcome to RequestPayloadHandle::emit. It marks incomplete records with a drop reason. It preserves the pre-error response prefix for streaming failures. It keeps the non-streaming fallback behavior unchanged for clients.

The revised tests cover successful capture, empty streams, cancellation, first-chunk errors, mid-stream errors, partial response recovery, and the unspecified fallback. The supplied CI and validation evidence also reports all scheduled checks as passing.

✅ Action performed

Full review finished.

[glamr-agent]

nursery: @dynamo-ops please run full CI for 9285f5a

[MatejKosec]

/ok to test 9285f5a

[dmitry-tokarev-nv]

Review: verified by execution

Approved. I reproduced the old behavior on today's main, confirmed this head fixes it, and mutation-tested the new tests. Two non-blocking items are below. Nothing blocks the merge.

Reproduction on today's main, and the same input on this head

One content chunk the client already received, then a mid-stream engine error, driven through scan_aggregate_with_future and the real record bus.

On c0eb3fcfaf (merge base, today's main):

{"event_type":"request_payload","payload":{"request_id":"probe-req","endpoint":"openai.chat_completion","model":"m","request":{...},"payload_complete":true}}

No response key, no reason, and payload_complete is true. That is the lie.

On 9285f5ae (this head), same input:

{"event_type":"request_payload","payload":{"request_id":"probe-req",...,"response":{"choices":[{"message":{"content":"Hello "}}]},"payload_complete":false,"payload_drop_reason":"aggregation_failed:Internal: engine died mid-stream"}}

The record is now honest, and it keeps the prefix the client saw.

Every way the response can be dropped, and what the record says for each

Drop path	Record written?	Does it tell the truth?
Engine error mid-stream, streaming	Yes	Yes. payload_complete:false, aggregation_failed:<error>, plus the pre-error prefix.
Engine error before any content	Yes	Yes. aggregation_failed:<error>, no response.
Engine error, non-streaming fold	Yes	Yes. Reason plus prefix in the record. The client still gets the empty fallback, same as before.
Client disconnects mid-stream	Yes	Yes about completeness: response_stream_dropped. The delivered prefix is not kept, because the buffered chunks go away with the stream.
Client disconnects before the first chunk	Yes	Yes. Same response_stream_dropped.
Request cancelled or gateway timeout	Yes	Yes. The engine context dies, the stream is dropped, so response_stream_dropped. The comment at payload_stream.rs:47 says these two causes cannot be told apart from here, which matches what I read in the code.
Empty response stream	Yes	Yes. empty_response_stream.
Aggregation fails with no error chunk	Yes	Yes. aggregation_failed:<error>.
A call site passes no response and no reason	Yes	Yes. unspecified, and a test pins it.
Rejected before the stream exists (worker 503, or any of the 6 ? returns between preprocessor.rs:7088 and :7219)	No record at all	Not applicable. The handle is dropped without emit. This is a different defect from a lying record, it is present on main as well, and this PR does not change it. Issue #14134 lists it. Happy to file it separately.
Process shutdown while the request is live	Not guaranteed	The emit runs in a spawned task, so a shutdown can lose it. Unchanged by this PR.

Unverified: I did not drive a real client disconnect through the HTTP frontend. The three rows about disconnect and cancel are read from the code plus the test_concurrent_futures case, which drops the pass-through stream before the first poll and gets response_stream_dropped.

Who sets the field, and whether two writers can disagree

emit consumes the handle, so one handle can produce at most one record. Production has one call site, preprocessor.rs:7240, inside one spawned task. done_tx is a oneshot behind Option::take, so the error-chunk path and the end-of-stream path cannot both fire. There is no race on payload_complete inside the record.

There is a second writer further down. otel_sink.rs rewrites both fields, and it wins for the exported attribute. See the P2 item.

For a consumer: payload_complete is a plain bool with no skip_serializing_if and no serde(default), so the key is always present on the wire and was always present before. An older reader sees the same key and now sees false where it used to see true. payload_drop_reason is skip_serializing_if = "Option::is_none", so it appears only on a dropped record. No schema change, and no field that defaults to complete.

[P2] The OTEL export still says payload_complete: true for a record that says false. lib/llm/src/request_trace/otel_sink.rs:159 returns the literal true for every record under the size limit, so the exported attribute contradicts the record body this PR just corrected. Please derive it from record.payload.

Measured on this head, with the probe output

I set payload_complete = false and a drop reason on a payload record, then called OtelRequestTraceSink::payload_for_limit with a limit the record fits inside.

PROBE otel attribute payload_complete=true payload_drop_reason=None
PROBE body payload_complete=false body drop_reason="aggregation_failed:engine died mid-stream"

otel_sink.rs:159:

if payload.len() <= max_payload_bytes {
    return Some((payload, true, None));
}

Those values become the payload_complete and payload_drop_reason attributes at otel_sink.rs:329. A consumer that filters on the indexed attribute, rather than parsing the embedded body, still reads a dropped response as complete.

This is not a regression. Before this PR the body also said true, so the two agreed and were both wrong. The fix simply does not reach the attribute, and the record now contradicts itself. The file is outside this diff, so this is a review-body note. A follow-up PR is fine if you prefer.

New tests: mutation results, with the control

All 19 tests in request_trace::payload and request_trace::payload_stream pass on this head with cargo test -p dynamo-llm --no-default-features. Three mutations, each caught by the test that claims to cover it, with the other 17 tests unchanged as the control:

Mutation	Result
emit hardcodes payload_complete: true and payload_drop_reason: None again	2 failed, 17 passed. Both new assertions fire: "a record with no response must not claim to be complete".
aggregate_with_partial_recovery drops the pre-error prefix	2 failed, 17 passed. "content delivered before the error must survive the early drop".
The error-chunk early settle in poll_next is disabled	1 failed, 18 passed. Reason falls back to response_stream_dropped, which the test rejects.

The tests read the outcome the code produced, and the two payload.rs tests read the record off the real record bus, not a record they built themselves.

Overlap with #14008

#14008 rewrites the same function, fold_aggregate_with_future in payload_stream.rs, and renames final_response_to_one_chunk_stream. git merge-tree between the two heads produces conflict markers in that file. The two do not depend on each other, but they cannot both merge unchanged.

The order matters for behavior, not only for text. #14008 keeps the current oneshot::channel::<NvCreateChatCompletionResponse>() and still drops the sender on an aggregation error, which is exactly the bare Option this PR replaces. If #14008 lands second and is rebased by taking its own version of the function, the non-streaming path goes back to a record with no reason. Whoever rebases second must keep PayloadOutcome.

State, staleness, and what I did not verify

Head 9285f5ae. The merge base is c0eb3fcfaf, which is from today, so the branch is 3 commits behind main and not stale. The defect still reproduces at that merge base, so the PR is still needed.

The base moved since @nnshah1 approved a343096c, so I compared the author's own change at the two merge bases instead of old-head...new-head. The only difference is comment text, shortened in response to @alec-flowers. No executable line changed after the approval.

I did not run the HTTP frontend or a real engine. Verification is cargo test -p dynamo-llm --no-default-features on macOS ARM64 plus the probes above. 5 unrelated tests in that crate fail locally on this machine on both trees, in networking code this PR does not touch.

[MatejKosec]

/ok to test 6e81ea1

[dmitry-tokarev-nv]

Follow-up review: re-approved at 6e81ea1291

The new commit closes the P2 and the P3 from my last round. I verified both by execution, not by reading. One new P3 is below and it does not block the merge.

What moved since my approval of `9285f5ae`, and how I established it

The push is an append, not a rewrite.

• compare/9285f5ae...6e81ea12 returns status: ahead, ahead_by: 1, behind_by: 0, and the merge base of the two is 9285f5ae itself. The commit I approved is still an ancestor of the live head.
• The merge base with main is c0eb3fcfaf before the push and c0eb3fcfaf after it. The base did not move, so 9285f5ae..6e81ea12 is exactly the author's own change and no diff of diffs is needed.
• The new commit touches two files: otel_sink.rs (the export fix plus one test) and payload_stream.rs (one test only). No production code changed in payload_stream.rs.

I re-read the live head immediately before posting this review. It was still 6e81ea1291.

The drop paths, re-walked against the new code

The new commit changes one production function, payload_for_limit. Its contract is unchanged: same signature, same return type, and emit at otel_sink.rs:297 is still the only caller.

Every drop path still writes a record and still names a reason. The export now repeats that reason instead of dropping it. The one exception is a record that is also over the size limit, which is the new P3 below.

A record with no captured payload still exports payload_complete = true, as it did before. That is not a claim about a response, because there is no payload on such a record.

The reason string reaches an OTEL attribute for the first time. It is not caller-supplied text, and it is a field of the record that the size cap already bounds, so the attribute is bounded too.

Tests: what I ran and what each result proves

Worktree at 6e81ea1291, macOS, --no-default-features, sandbox off so port binding works.

Run	Result
--lib request_trace:: on the clean head	76 passed, 0 failed
Same, with the fold success arm mutated	75 passed, 1 failed: the new fold test
--lib request_trace::otel_sink with the export reverted to the old literal	3 passed, 1 failed: the new sink test
cargo fmt -p dynamo-llm --check	clean

Both new tests fail on the pre-fix code and pass on this head, so each one pins its own arm.

The new sink test builds its own record. I added a separate probe that drives a real errored stream through scan_aggregate_with_future and the record bus, then feeds the record the system produced into payload_for_limit. Body and attribute agree on that path as well. The numbers are on the inline thread.

#14143 and #14008 still conflict, and the order still matters

#14008 is open, head 2b8a915437 from September 1, and its payload_stream.rs change is unchanged since my last round. It rewrites the same fold_aggregate_with_future, still on oneshot::channel::<NvCreateChatCompletionResponse>(), and still sends no drop reason.

The two cannot both merge unchanged. If #14008 lands second and the conflict is resolved by keeping its own function body, the non-streaming record loses its reason again and this PR is silently undone on that path. The safe order is this PR first, then a rebase of #14008 that keeps PayloadOutcome.

I am not reporting this as a finding on this PR. It is a coordination point for whoever merges second.

[P3] An oversized record loses the capture reason it now carries. See the inline comment on otel_sink.rs:169. Non-blocking, and new in the sense that the case did not exist before this PR.

Outstanding: one P3. No P0, no P1. I did not drive a real client disconnect through the HTTP frontend, which was also true last round.

[glamr-agent]

nursery: @coderabbitai full review
/devin review

[glamr-agent]

nursery: @dynamo-ops please run full CI for 3612868

[dynamo-ops]

/ok to test 3612868

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

base_ref=""
for candidate in origin/main main origin/master master; do
  if git rev-parse --verify -q "$candidate" >/dev/null; then
    base_ref="$candidate"
    break
  fi
done
test -n "$base_ref"
merge_base="$(git merge-base "$base_ref" HEAD)"

printf 'head=%s\nbase=%s\nmerge_base=%s\n' "$(git rev-parse HEAD)" "$base_ref" "$merge_base"

printf '%s\n' '== Applicable instructions =='
for file in AGENTS.md lib/AGENTS.md lib/llm/AGENTS.md lib/llm/src/AGENTS.md lib/llm/src/request_trace/AGENTS.md; do
  if test -f "$file"; then
    printf '\n--- %s ---\n' "$file"
    cat "$file"
  fi
done

printf '%s\n' '== Pull request commits and change set =='
git log --oneline --decorate "$merge_base"..HEAD
git diff --stat "$merge_base" HEAD
git diff --name-status "$merge_base" HEAD
git diff --find-renames --find-copies --unified=55 "$merge_base" HEAD

printf '%s\n' '== Changed Rust file outlines =='
git diff --name-only "$merge_base" HEAD | while IFS= read -r file; do
  case "$file" in
    *.rs)
      printf '\n--- %s ---\n' "$file"
      ast-grep outline "$file" --items all || true
      ;;
  esac
done

printf '%s\n' '== Current request-trace implementation =='
sed -n '1,360p' lib/llm/src/request_trace/payload_stream.rs
sed -n '1,250p' lib/llm/src/request_trace/payload.rs
sed -n '6950,7080p' lib/llm/src/preprocessor.rs

printf '%s\n' '== Relevant contracts =='
fd -HI '^annotated\.rs$' lib -x sh -c '
  printf "\n--- %s ---\n" "$1"
  rg -n -C 12 "fn (is_error|cloned_error|into_data)" "$1" || true
' sh {}
fd -HI '^aggregator\.rs$' lib -x sh -c '
  printf "\n--- %s ---\n" "$1"
  rg -n -C 22 "fn apply|try_fold|preserves_typed_error_after_partial_output" "$1" || true
' sh {}
fd -HI '^disconnect\.rs$' lib -x sh -c '
  printf "\n--- %s ---\n" "$1"
  rg -n -C 16 "monitor_for_disconnects_with_timeout_error_and_keep_alive|Some\(Err|break" "$1" || true
' sh {}

printf '%s\n' '== Outcome and emission call sites =='
rg -n -C 6 --type rust \
  'PayloadOutcome|PayloadFuture|aggregate_with_partial_recovery|scan_aggregate_with_future\(|fold_aggregate_with_future\(|RequestPayloadHandle|\.emit\(|response_stream_dropped|aggregation_failed|empty_response_stream|unspecified' \
  lib/llm/src lib/llm/tests lib/runtime/src

printf '%s\n' '== Request trace schema and downstream drop-reason processing =='
rg -n -C 10 --type rust \
  'struct RequestTracePayload|payload_complete|payload_drop_reason|fn marker_payload|marker_payload' \
  lib/llm/src/request_trace

Length of output: 158414

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

log=/tmp/coderabbit-shell-logs/shell-output-EFNcZS
printf '%s\n' '== Inspection metadata from first pass =='
sed -n '1,180p' "$log"

base_ref=""
for candidate in origin/main main origin/master master; do
  if git rev-parse --verify -q "$candidate" >/dev/null; then
    base_ref="$candidate"
    break
  fi
done
merge_base="$(git merge-base "$base_ref" HEAD)"

printf '%s\n' '== PR-only diff, intended files =='
git diff --find-renames --find-copies --unified=30 "$merge_base" HEAD -- \
  lib/llm/src/preprocessor.rs \
  lib/llm/src/request_trace/payload.rs \
  lib/llm/src/request_trace/payload_stream.rs \
  lib/llm/tests/request_trace_nats_integration.rs

printf '%s\n' '== Final fold and preprocessor paths =='
sed -n '210,275p' lib/llm/src/request_trace/payload_stream.rs
sed -n '7200,7250p' lib/llm/src/preprocessor.rs

printf '%s\n' '== Test coverage around error and folding paths =='
sed -n '800,985p' lib/llm/src/request_trace/payload_stream.rs

printf '%s\n' '== All RequestPayloadHandle::emit invocations =='
rg -n -C 4 --type rust '\.emit\s*\(' lib/llm/src lib/llm/tests | rg -C 8 'emit\(|RequestPayloadHandle|payload::create_handle|for_test' || true

printf '%s\n' '== Current tree status =='
git status --short

Length of output: 92135

---

@glamr-agent Full review complete for head 361286827a8b237a631ed12dfe41feac6ad68bbe.

I found no blocking issue.

PayloadOutcome carries the response and drop reason through both aggregation paths. RequestPayloadHandle::emit marks a record complete only when it has a response and no drop reason. The unspecified fallback prevents a response-less record from reporting completion.

The streaming path settles the audit outcome when it receives an error annotation. It does this before the downstream disconnect monitor can drop the pass-through stream. The audit record retains the pre-error prefix and records aggregation_failed:<error>.

The fold path uses the same partial-recovery logic. It preserves partial content for the audit record. It still sends the empty fallback response to the client when aggregation is incomplete.

response_stream_dropped describes the observable early-drop condition without assigning an unverified cause. This condition can result from a client disconnect or a backend inactivity timeout.

The updated tests cover successful aggregation, empty streams, early stream drops, an error as the first chunk, partial recovery after an error, fold aggregation, and the unspecified fallback.

I did not run build or test commands in this read-only review environment.

You are interacting with an AI system.