fix(search): stop extreme relative date offsets from crashing recall (#3217) - #3413
Merged
Merged
Conversation
…3217) Consolidation recalls with stored fact text as the query, so a phrase like "十万年前" (100,000 years ago) hit unguarded offset arithmetic in extract_period — which runs BEFORE analyze()'s dateparser guard — and escaped as ValueError('year -97974 is out of range'), deterministically failing every recall and consolidation touching the bank. The three years observed in #3217 (-534, -974, -97974) are exactly now.year - {2560, 3000, 100000}: query-time arithmetic, not stored rows (Python datetimes can't represent them, so no bad date can reach the DB through asyncpg in the first place). Three layers, mirroring the #2636 add_years fix: - extract_temporal_constraint (the recall choke point) degrades any analyzer failure to 'no temporal signal' with a warning; analyze() itself stays strict so parser bugs still surface in tests. - chinese_temporal_periods: add_months/subtract_months are now bounds-checked like add_years (returning None, plumbed through every call site), and day/week offsets go through the overflow-guarded add_days instead of raw timedelta addition. - temporal_periods: an explicit month + year 0000 match returns NO_TEMPORAL_CONSTRAINT instead of crashing datetime().
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.
Fixes #3217.
Root cause — query-time arithmetic, not stored rows
The three observed failures (
year -534,year -974,year -97974) are exactlynow.year − {2560, 3000, 100000}. They come from "N years ago" offset arithmetic at query-analysis time, not from dates stored in the DB:十万年前(100,000 years ago) is re-analyzed on every recall, so the failure is deterministic per bank and retries never help.extract_period()runs beforeanalyze()'s dateparser guard (fix(query_analyzer): handle dateparser internal crashes gracefully #893), soValueError('year N is out of range')/OverflowErrorescaped straight out of recall asFailed to search memories (...), which is what killed consolidation.datetimecannot represent pre-year-1 dates, so nothing outside 1–9999 can reach PostgreSQL through asyncpg in the first place (asyncpg decodes BC values with a different error —OverflowError: date value out of range— and-97974is outside even PostgreSQL's representable range). This confirms @koriyoshi2041's analysis that the retain-time clamp proposed in the issue targets a non-existent ingress — no retain changes here.#2636 already fixed this exact class for
add_years(which is why the年前trio no longer reproduces on main), but sibling paths were left unguarded. All of these crash recall on current main:Fix — three layers
extract_temporal_constraint()(the recall choke point) now degrades any analyzer failure to "no temporal signal" with a warning.analyze()itself stays strict, so parser bugs still surface in tests and to direct callers (test_query_analyzer_period_valueerror_still_surfacesis unchanged). This is the guarantee that one pathological phrase can never poison a bank's recall/consolidation again.chinese_temporal_periods:add_months/subtract_monthsare bounds-checked exactly like fix(search): avoid year-0 crashes in Chinese rolling-window temporal extraction #2636'sadd_years(returnNone, plumbed through every call site —tyenforces completeness), and day/week offsets go through the overflow-guardedadd_daysinstead of rawtimedeltaaddition.subtract_monthscollapses intoadd_months(reference_date, -months)(they were the same formula).temporal_periods: an explicit month + year0000match returnsNO_TEMPORAL_CONSTRAINTinstead of crashingdatetime()(and instead of letting the dateparser fallback invent a different date).Tests
2560年前/3000年前/十万年前) pinned as regressions.question_date), and the year-0000 month pattern.extract_temporal_constraintsurvives an analyzer that raises.tests/test_query_analyzer.py: 450 passed (409 existing + new).Note on #3403
#3403 attributes the crash to
datetime.fromisoformat()on stored ISO strings, butfromisoformatraisesInvalid isoformat string(notyear N is out of range) for negative-year input,_as_dtruns after recall so its errors can't produce the observedFailed to search memories (...)wrapper, and clamping to naivedatetime(1, 1, 1)would introduce naive-vs-awareTypeErrors in_merge_min/_merge_max. That path isn't the reported bug.