Commit 25cd913
[log-classifier] Don't panic on Bedrock errors, and bound the LLM prompt by bytes (#8461)
✴️ iz2: (written by Ivan's agent, on his account)
## Summary
The `log_classifier` Lambda panics on every Bedrock error, and the panic
throws away a classification it has **already computed**.
`query_model` unwrapped the Converse result five levels deep:
```rust
let response = make_bedrock_call(input_text, model_name).await;
let (line_num, validation) = validate_output_in_log(
&response.unwrap().output.unwrap().as_message().unwrap().content[0]
.as_text().unwrap().clone(),
&log,
);
```
`make_query` is called from `handle()` *after* `evaluate_ruleset` has
produced `match_json` but *before* `upload_classification_dynamo`. So a
panic anywhere in that chain aborts the invocation with the regex
verdict still in hand and never written — the job then shows on HUD /
Dr. CI with no failure line at all, which looks like "the classifier had
nothing to say" rather than "the classifier crashed".
**Why it started firing.** #8391 widened the LLM context from 100 to 500
lines. `make_query` caps the snippet in *lines*, never in bytes, so a
500-line window of long CI lines (stack traces, embedded base64, long
compiler command lines) can run to megabytes and exceed the model's
context window. Bedrock returns `ValidationException: prompt is too
long`, and the bare `.unwrap()` on line 79 turns that into a panic.
Throttling and timeouts panic identically at the same line.
Two independent defects, both fixed here: the prompt should not be built
over-long, and a Bedrock failure should not be fatal.
## Changes
**1. No `unwrap` on the Bedrock path.** Each attempt now returns an
`Attempt`:
- `Refined` — the model named a line that exists in the log.
- `Unusable` — the model answered but the answer didn't validate. Falls
through to the secondary model, which is what that fallback was always
for.
- `CallFailed` — the call itself failed. Ends the LLM path; `handle()`
proceeds to write the ruleset verdict.
`CallFailed` deliberately does *not* retry the secondary model. The
failures reaching it are either an over-long prompt (which any model
rejects identically) or an outage / timeout / throttle, where a second
multi-second round trip on an already-failing path risks burning the
Lambda deadline before the DynamoDB write. Losing the refinement is
cheap; losing that write is the bug being fixed.
**2. A byte budget on the prompt.** `snippet_around` replaces the
`get_snippets` call at this one site and bounds the window by bytes as
well as lines. It walks outward from `error_line` over the map's real
keys, so:
- the matched line is always retained, including when it sits within
`num_lines / 2` of either end of the log and the window is clipped to
one side (it is *not* reliably centered);
- it anchors on the line **number**, not the line text. The catch-all
rule that sends a log down this path (`^##\[error\](.*)`) matches text
that repeats throughout a log, and the engine scans in reverse and picks
the **last** occurrence — anchoring by text would have built the window
around the first one, potentially thousands of lines away;
- sparse keys (preprocessing drops boilerplate lines) are handled by
walking keys rather than a dense range.
The budget is 100 KiB for the whole rendered prompt, template included.
Even at a pessimistic one token per byte that is well inside the
200k-token context, and a typical 500-line window is a small fraction of
it — so this only clamps pathological logs and does not narrow the
window #8391 widened.
**3. Response parsing.** Text blocks are joined with a newline rather
than taking `content[0]`, so a model that emits a reasoning block ahead
of its answer doesn't read as "no text".
**4. Observability.** Call failures now log at `error!` rather than
vanishing. See the note below.
**5. A test seam for the Bedrock path** (added after review — see
below).
## The test seam
@huydhn asked for more tests before landing, pointing at the mocking
capability Ed recently added. That capability does not exist for this
path: `tests/classify.rs` is a marker-based fixture harness over the
**ruleset**, and nothing in the crate can stand in for Bedrock —
`make_bedrock_call` built its own client inline, so the `Attempt` state
machine had no seam and no coverage. So this builds it:
- `make_bedrock_call` and `query_model` take a `&Client`; the two-model
fallthrough moves into `refine_with_models`. `make_query` builds the
client once and passes it down, so the primary and the secondary now
**share** it rather than constructing one apiece (which also means the
secondary reuses the primary's pooled TLS connection — a little less of
the round trip the `CallFailed` rationale worries about).
- The tests drive a real `aws_sdk_bedrockruntime::Client` over a
~40-line `HttpConnector` that queues canned responses and records the
outgoing request URI and body. They therefore exercise the SDK's own
serialization and deserialization rather than hand-built types.
`StaticReplayClient` would have done this, but smithy's `test-util`
feature pulls `aws-smithy-protocol-test` and hyper-0.14 into a lambda
crate for what is a queue and a `Vec`. The only new dependency is a
**dev**-dependency on `aws-smithy-types` (already in the tree
transitively) to reach `SdkBody`.
Also removed the last `unwrap()` on the path —
`Message::builder().build()`, which cannot fail today but is generated
code an SDK bump can change — so "no panic on the Bedrock path" is now
literally true rather than true-with-a-footnote.
## Test plan
`cargo test` — **76 pass** (57 lib + 18 main + 1 fixture harness; lib
was 38 before this PR). `cargo fmt --check` and `lintrunner` clean.
Bedrock-path tests, over the replayed client:
- `a_bedrock_failure_yields_no_refinement_instead_of_panicking` —
replays the incident's own `ValidationException: prompt is too long`;
asserts no panic, `None`, and that the secondary was **not** tried.
- `a_bedrock_server_error_also_degrades_quietly` — same contract for a
500.
- `a_usable_primary_answer_is_returned_without_asking_the_secondary`
- `an_unusable_primary_answer_falls_through_to_the_secondary` — asserts
the model in the **request URI**, in both directions, so a fallthrough
that asked the same model twice would fail (the emitted rule name alone
can't show this).
- `both_models_unusable_yields_no_refinement`,
`a_secondary_that_fails_after_an_unusable_primary_still_degrades`
- `an_empty_response_body_is_unusable_rather_than_a_panic` — the case
the old `.output.unwrap().as_message().unwrap()` chain died on, through
the real deserializer.
- `the_prompt_that_reaches_bedrock_stays_within_the_byte_budget` — 8 MB
log; asserts the bytes that actually go on the wire are `<=
MAX_PROMPT_BYTES` **and** more than half of it, so a bug that zeroed the
budget can't pass trivially. Asserted for both models.
- `the_prompt_on_the_wire_is_the_rendered_template_around_the_snippet`,
`make_query_bounds_the_snippet_with_the_prompt_budget`
Plus the 18 `snippet_*` / `response_text_*` / budget tests from the
earlier commits.
**Negative control:** reverting `query_model`'s `Err` arm to a panic
makes exactly the `CallFailed` tests fail — so they catch the regression
they are named for rather than passing incidentally.
## Notes for whoever owns the alarms / infra
Two things this PR deliberately does **not** do:
1. **The `Errors` metric stops being an LLM-path signal.** The panic was
loud — it *was* the Lambda `Errors` metric, and that is what made this
visible at all. After this change the same condition is an `error!` log
line and the invocation succeeds, so Errors goes to ~zero. Worth a
CloudWatch metric filter on `bedrock: converse call to` (or a
`BedrockRefinementFailure` counter) so a future Bedrock degradation is
still detectable. That's infra config rather than a change to this repo
— flagging it so the drop isn't read as "problem fully gone".
2. **There is still no operation timeout on the Bedrock client**
(pre-existing; surfaced by a cross-model review of this change).
`load_defaults` sets a connect timeout but no overall operation timeout,
and `Converse` is non-streaming, so time-to-first-byte is the model's
full generation latency. An unbounded call can eat the Lambda deadline
and lose the DynamoDB write — the same failure *class* this PR fixes, by
a different route, and with no catch. Out of scope here, but this PR
creates the single client-construction point that makes it a one-line
`TimeoutConfig` fix.
---------
Co-authored-by: Ivan Zaitsev <izaitsevfb@users.noreply.github.com>
Co-authored-by: Ivan Zaitsev <izaitsevfb@meta.com>1 parent e33a53f commit 25cd913
3 files changed
Lines changed: 1126 additions & 62 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
35 | 35 | | |
36 | 36 | | |
37 | 37 | | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
| 41 | + | |
| 42 | + | |
| 43 | + | |
0 commit comments