feat(retain): optional fail-on-extraction-errors flag (issue #2700)#2721
Merged
Conversation
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.
Problem: silent success on dropped facts
When batch fact extraction fails for a chunk during retain, the error is recorded
non-fatally (into a
RetainExtractionErrorsaccumulator, persisted asextraction_errors_count/extraction_errors_samplein the operation'sresult_metadata) but the retain async operation is still markedstatus='completed'unconditionally. Nothing reads those counters to change theterminal status, so a retain that dropped all of its facts still reports a
clean success — the worst failure mode for a memory system, because the caller
has no signal that memories were lost.
The fix: an opt-in escape hatch (default off)
Adds a new static, server-level config flag
HINDSIGHT_API_FAIL_ON_EXTRACTION_ERRORS(defaultFalse, so currentbehavior is 100% preserved). When it is
Trueand the retain operationaccumulated any extraction errors (
extraction_errors_count > 0), the operationis marked
failedwith an informativeerror_message(includes the error countand mentions extraction errors) instead of
completed.The decision is made at the single retain completion point
(
_mark_operation_completed), reading theextraction_errors_countthat_write_retain_outcome_metadataalready persisted before completion. When theflag is off, the code path is unchanged.
Changes
config.py:ENV_FAIL_ON_EXTRACTION_ERRORS,DEFAULT_FAIL_ON_EXTRACTION_ERRORS = False,a static
fail_on_extraction_errors: boolfield wired infrom_env().memory_engine.py:_mark_operation_completednow marksfailed(with aninformative message) when the flag is on and the persisted extraction-error
count is > 0; otherwise unchanged.
.env.example(+ synced embed bundle copy) andconfiguration.mddoc row.behavior (fails when flag on + errors, stays completed when flag off, stays
completed when flag on but zero errors).
Deferred follow-ups (intentionally out of scope)
To keep this change self-contained, the following are left for sibling PRs and
do not touch API response models, generated clients, webhooks, or the DB status
enum:
extraction_errors_countin the operation-status API response.completed_with_errorsterminal status (needs a DB enum value).RetainEventDatafor webhook consumers.Note: the root cause of the parse failures themselves (why extraction fails on
certain chunks) is handled by sibling PRs for #2699 and #2701; this PR is purely
about not silently reporting success when facts were dropped.
Fixes #2700