feat(consolidation): warn when a run dead-letters a large share of its work - #3372
Open
JoshFunnell wants to merge 1 commit into
Open
Conversation
…s work
Memories that exhaust consolidation retries are stamped consolidation_failed_at
and skipped by every later run. That is the right conservative default -- the
engine cannot know whether the provider refused this content or was simply
unavailable -- but it is silent. A provider outage or quota window can park a
whole batch indefinitely, and the recovery endpoint that already exists
(POST /v1/default/banks/{bank_id}/consolidation/recover) only helps if someone
knows to call it.
This is the observability half of vectorize-io#2973, and deliberately not the other half.
It infers nothing about WHY a memory failed: it reads two counters the job
already keeps (memories_processed, memories_failed) and compares the ratio to a
threshold. No provider exception types, no status codes, no message substrings,
so nothing here rots as providers change their error vocabulary.
At the end of a run, if the dead-lettered share reaches
HINDSIGHT_API_CONSOLIDATION_DEAD_LETTER_WARN_FRACTION (default 0.5), a single
WARNING names the counts, states the rows will not be retried, and gives both
the HTTP and CLI recovery invocations for that bank. Set the fraction to 0 to
silence it. It is read from the bank-resolved config, like the sibling
consolidation knobs, so a per-bank value takes effect rather than only
round-tripping through bank templates.
The denominator is the run's attempted memories, not the bank's whole pending
set: a wiped round warns, a slow bleed of a couple of rows per large round does
not. Mass failure is the shape recovery is for, and anything finer would need an
opinion about why a memory failed.
An absolute floor of 3 dead-lettered memories applies regardless of the
fraction. A bank consolidating one or two memories at a time hits 100%
routinely, and a warning that fires constantly is one operators stop reading.
The floor is a constant rather than a second dial.
The decision is a pure function returning str | None, so the caller logs only on
a real signal and the behaviour is testable without capturing log output.
22 tests cover the firing shapes, the quiet shapes (which decide whether the
warning stays readable), both inclusive boundaries, the message being actionable
on its own, that no provider-error vocabulary appears in the code path, and the
call site itself -- that the fraction comes from the bank-resolved config rather
than the process global, and that a non-None decision is actually logged.
Generated artifacts regenerated for the new bank-template field: the Go and
Python clients and hindsight-clients/go/api/openapi.yaml via the Docker-pinned
openapi-generator, plus openapi.json, bank-template-schema.json and the docs
skill. The TypeScript entry in generated/types.gen.ts is the additive field
only, matching what vectorize-io#3332 and vectorize-io#3223 landed for their bank-template fields: a
full local TS regeneration rewrites ~7k lines across all 16 generated files
even on a pristine main checkout with the pinned generator and the CI-pinned
Node 20, because hindsight-clients/typescript has no package-lock.json and the
generator's transitive dependencies float. That drift is unrelated to this
change and is not carried here.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The observability half of #2973, following the close of #3309.
#3309 was closed because failure-classification on the consolidation path was too brittle and the recovery path already exists. That was right, and this PR does not reintroduce classification. It implements what was suggested instead:
One deliberate divergence, stated up front: you wrote "pending set"; this measures a large fraction of the run's attempted memories —
memories_failed / (memories_processed + memories_failed)— not bank-wide pending. So a wiped round still warns even when the bank's backlog is far larger thanmax_memories_per_round, while a slow bleed of a couple of rows per large round does not. That is the mass-failure shape recover is for, and it needs no opinion about why rows failed. Bank-wide pending would need an extra query and would still not change what this run just stamped.The gap
Memories that exhaust consolidation retries are stamped
consolidation_failed_atand are then skipped by every later run. That default is correct — the engine cannot know whether the provider refused this content or was merely unavailable — but the run itself is quiet.failed_consolidationstays queryable in bank stats; what is missing is a signal at the moment a run mass-stamps failures.POST /v1/default/banks/{bank_id}/consolidation/recoveralready fixes it in one call, but only for someone who knows to look.The change
At the end of a consolidation run: if
memories_failed >= 3andmemories_failed / (memories_processed + memories_failed) >= HINDSIGHT_API_CONSOLIDATION_DEAD_LETTER_WARN_FRACTION(default0.5,0disables), emit one WARNING. No exception types, no status codes, no message substrings — the decision reads two counters the job already keeps.memories_failedcounts only rows stampedconsolidation_failed_atin this run;memories_processedis its success counterpart.Read from the bank-resolved config like the sibling consolidation knobs, so a per-bank value takes effect rather than only round-tripping through bank templates.
The floor of 3 is a constant rather than a second dial: a bank consolidating one or two memories at a time hits 100% routinely, and a warning that fires constantly is one operators stop reading.
Non-goals
No metrics, no alerting integration, no automatic recovery — this assumes WARNING-level logs are somewhere an operator sees, which is the minimum surface that makes recover reachable the same day. Deciding to re-run rows a provider rejected stays with the operator.
Surfaces deliberately not updated
The repo is wider than the engine, so rather than leave these silent:
hindsight-api-slimengine + config +BankTemplateConfigopenapi.json,bank-template-schema.json, docs skilldocs/developer/configuration.mdhindsight-control-plane—bank-config-view.tsxFieldRows with per-locale label/description keys, and the view already omits several per-bank consolidation knobs (consolidation_max_memories_per_round,consolidation_llm_parallelism), so this is consistent with current practice rather than a new gap. Say the word and I'll add the row plus locale strings.hindsight-clibank set-config, and the coverage manifest already skipsupdate_bank_config.updateswholesale as "flattened into per-setting flags". No coverage-manifest entry is required by this change.monitoring/dashboardsTests
22 tests, no database and no LLM — the decision is a pure
str | Nonefunction, so the caller logs only on a real signal. Weighted toward the shapes that must stay quiet (clean run, empty run, below fraction, below floor, disabled, negative counters), with both inclusive boundaries pinned:fraction == thresholdandfailed == 3. Two cover the call site rather than the function — that the fraction comes from the bank-resolved config, and that a non-Nonedecision is actually logged.