From 8511fa6aa31a69153dd8a22843312133f449612a Mon Sep 17 00:00:00 2001 From: Tianye Song <162393000+sontianye@users.noreply.github.com> Date: Tue, 21 Jul 2026 09:36:09 +0800 Subject: [PATCH] fix(memory): consolidated facts inherit expected_valid_days from sources (#4225) * fix(memory): consolidated facts inherit expected_valid_days from sources Consolidation (#3996) and per-fact expected_valid_days (#4143) were both authored by the same contributor but never connected: the consolidated new_fact carried the newest source's createdAt but no expected_valid_days, so _effective_fact_staleness_age fell back to the global staleness_age_days. A merge of several 200-day-old stable facts (each evd=3650) would land with no evd, read as a 90-day window, and re-enter the staleness candidate set on the very next cycle - the merge discarded the lifetime signal of the underlying information and contradicted consolidation's premise (these are stable, related facts worth synthesising). Fix: the merged fact inherits expected_valid_days set so it is re-reviewed at the EARLIEST source review deadline (min(createdAt + expected_valid_days) across sources, relative to the merged fact's createdAt = the newest source's). A merge combines details from every source, so a volatile sub-detail (evd=7) must not inherit a stable source's 3650-day window and escape staleness review for years - staleness KEEP/REMOVE is the only path that re-validates a merged fact, so biasing toward the soonest deadline keeps uncertain merges re-checked sooner. A source already past its deadline yields a minimal positive window (review next cycle) rather than the global fallback, which would defer an overdue review. Capped at the creation-time staleness_max_lifetime_multiplier like any new fact. Omitted when no source carries a valid evd (legacy facts fall back to the global age at read time, matching pre-feature behaviour). DRY: extract _read_expected_valid_days(fact) -> int | None, the shared type rule (int/float, reject bool, coerce to int BEFORE the > 0 guard) previously inlined in four places - _normalize_memory_update_fact, _effective_fact_staleness_age, the newFact creation cap, and consolidation inheritance. All four call the single helper. Coercing before the guard matters for values in (0, 1): 0.5 passes a raw > 0 check but truncates to 0, which would violate the helper's "positive int or None" contract; the order now matches the original _normalize_memory_update_fact rule. No prompt/schema change: consolidation's prompt does not surface source evd to the LLM, so asking the model to assign a merged lifetime would be guessing without signal. Source inheritance is deterministic and always available. Tests: consolidation evd cases now use time-stable createdAt (relative to now via a _days_ago helper) covering - earliest-deadline selection, creation-cap clamp, omit when no source evd, volatile source governs the deadline (and re-enters staleness next cycle), overdue source clamps to a minimal window, float coercion. Plus TestReadExpectedValidDays / TestEffectiveFactStalenessAge regression cases for the (0, 1) coercion-order fix. * fix(memory): reject non-finite expected_valid_days before int coercion The shared _read_expected_valid_days helper (introduced when consolidating the evd type rule across four call sites) coerces with int(raw) before the > 0 guard - reversing the original _normalize_memory_update_fact order so that a fractional 0.5 does not leak as 0. But int(raw) raises for non-finite floats: int(nan) raises ValueError and int(inf)/int(-inf) raise OverflowError. Python's JSON decoder accepts NaN / Infinity as floats by default, so a single malformed expected_valid_days in a hand-edited memory.json would abort staleness selection or consolidation instead of falling back to the global lifetime. On main, _effective_fact_staleness_age checked raw > 0 first, so NaN fell back safely (nan > 0 is false) - but inf did NOT (inf > 0 is true, so main also crashed on inf). This helper is now the persisted-fact read path for both staleness and consolidation, so the regression (and the pre-existing inf crash) must be closed here. Fix: require math.isfinite(float(raw)) before the int() coercion, then keep the existing positivity check and fallback. NaN / +/-inf all return None, so callers fall back to the global staleness_age_days. Normal int/float values (including large ones) are unaffected - isfinite is a no-op for them. Tests: - TestReadExpectedValidDays.test_rejects_non_finite_values - NaN, inf, -inf return None (not raise). - TestEffectiveFactStalenessAge.test_falls_back_for_non_finite_values - the persisted-fact read path returns the global age for each, no raise. - test_consolidation_with_non_finite_source_evd_does_not_raise - end-to-end: a NaN-evd source merged with a stable source does not abort consolidation; the NaN source's effective lifetime falls back to the global 90 and its deadline participates in the earliest-deadline computation. * test(memory): hoist _select_stale_candidates import + tidy deadline docstring Two review nits from the latest pass: - `_select_stale_candidates` was imported inline inside three test methods; hoisted to the module-level import block so the dependency is declared once. - `test_consolidated_evd_volatile_source_with_equal_created_at_future_deadline` had an abandoned calculation in its comment ("3 + 7 = 10 ... minus 3 already elapsed = 7? No:") that could mislead future readers into thinking elapsed-since-creation factors into the inherited window. Collapsed to a single clear line stating the window is relative to the merged createdAt, regardless of the source's current age. * fix(memory): reject huge-int expected_valid_days above timedelta.max.days _read_expected_valid_days routed every numeric value through float(raw) for the math.isfinite guard, but Python's JSON decoder parses an integer literal with no decimal point as an arbitrary-precision int (unlike 1e400, which decodes to float inf). So a hand-edited memory.json carrying "expected_valid_days": 10**400 raised OverflowError in float(raw) before math.isfinite was ever called - exactly the malformed-field-aborts-everything scenario the helper's docstring claims to prevent. The earlier non-finite fix only closed the float cases (NaN / +/-inf / 1e400). A huge int below the float limit but above timedelta.max.days (e.g. 10**12) would pass the helper and raise OverflowError downstream in timedelta(days=evd) during staleness selection or consolidation - the same crash fancyboi999 flagged for extend_by_days, just reached via a stored evd. Fix: branch on type so an int never passes through float() (matching the reviewer's suggestion), AND cap the returned int at timedelta.max.days (999999999) so the downstream timedelta(days=evd) call cannot overflow either. The float branch keeps the isfinite + int() coercion. Both branches share the 0 < evd <= timedelta.max.days positivity/range check. Normal values are unaffected - any legitimate expected_valid_days is far below the cap (the config ceiling staleness_max_extension_days tops out at 36500). Tests (all three layers): - TestReadExpectedValidDays.test_rejects_huge_int_above_timedelta_max - 10**400, 10**12, 10**9, timedelta.max.days+1 return None; timedelta.max.days itself is accepted. - TestEffectiveFactStalenessAge.test_falls_back_for_huge_int_above_timedelta_max - the persisted-fact read path returns the global age, no raise. - test_consolidation_with_huge_int_source_evd_does_not_raise[1e400|1e12|1e9] - parametrized end-to-end: a huge-int-evd source merged with a stable source does not abort consolidation; the bad source falls back to the global 90. * fix(memory): guard datetime arithmetic, not just timedelta construction The huge-int fix capped _read_expected_valid_days at timedelta.max.days, but that only proves timedelta(days=evd) can be constructed - adding it to a real fact timestamp still overflows datetime.max. @fancyboi999 reproduced it: a source with expected_valid_days=timedelta.max.days raises "OverflowError: date value out of range" at dt + timedelta(...) in the new consolidation deadline calculation. capping at timedelta.max.days was another patch chasing the next overflow boundary, not a real close. Root cause: the helper was doing datetime-range validation, but the safe bound depends on the datetime the evd is added to, not on the evd alone. So the responsibility moves to the arithmetic site, with try/except as the terminal guard - no concrete upper bound to be wrong about. Changes: - _read_expected_valid_days returns any positive int (huge ints included, not routed through float). Its job is type/positivity validation only. - New _safe_add_days(dt, days) -> datetime | None wraps dt + timedelta(days), returning None on OverflowError/ValueError. This is the terminal guard - there is no further boundary to overflow because try/except catches any datetime-range failure regardless of magnitude. - _select_stale_candidates uses _safe_add_days(now, -effective_age); a None result means the window is unrepresentably large, so the fact cannot yet be stale and is skipped (not selected). - Consolidation computes each source's deadline via _safe_add_days; a source whose deadline overflows falls back to the global staleness_age_days deadline (same treatment as a legacy no-evd source), so one malformed field cannot abort the merge. Normal values are unaffected - any legitimate expected_valid_days is far below the overflow boundary (the config ceiling staleness_max_extension_days tops out at 36500). Tests: - TestSafeAddDays: normal/negative shifts; 10**400/10**12/10**9 return None; timedelta.max.days (the exact reproduced value) returns None, not raises. - TestSelectStaleCandidates.test_huge_evd_does_not_abort_selection: a fact with a huge evd is skipped, not selected, and selection does not raise. - test_consolidation_with_huge_int_source_evd_does_not_raise now parametrized over [1e400, 1e12, 1e9, timedelta.max.days] - the last is the value that constructs a valid timedelta but overflows datetime arithmetic. - Helper/read-path tests updated to assert huge ints are returned as-is (the overflow guard is no longer in the helper). --- backend/AGENTS.md | 2 +- .../backends/deermem/deermem/core/updater.py | 132 ++++- backend/tests/test_memory_consolidation.py | 521 ++++++++++++++++++ backend/tests/test_memory_staleness_review.py | 142 +++++ 4 files changed, 781 insertions(+), 16 deletions(-) diff --git a/backend/AGENTS.md b/backend/AGENTS.md index 7f2e60032..dcea77f76 100644 --- a/backend/AGENTS.md +++ b/backend/AGENTS.md @@ -621,7 +621,7 @@ The cached value is reused for both the blocking (`runs.wait`) and streaming (`_ - Both modes share `FileMemoryStorage`, per-user/per-agent isolation, prompt injection, manual CRUD primitives, and the updater backend. - Middleware mode queue debounces (30s default), batches updates, deduplicates per-thread, applies updates atomically (temp file + rename) with cache invalidation, and skips duplicate fact content before append. - Staleness pass (same LLM invocation as the regular updater, no extra API call): when `staleness_review_enabled` is `true` and at least `staleness_min_candidates` aged facts exist, `_select_stale_candidates` selects facts older than their individual review window (`expected_valid_days`, or the global `staleness_age_days` fallback) that are not in `staleness_protected_categories` (default: `correction`), surfaces them in the prompt with a `valid:Nd` annotation, and the LLM judges each as KEEP, REMOVE, or EXTEND. REMOVE entries go in `staleFactsToRemove`; EXTEND entries go in `staleFactsToExtend` with an `extend_by_days` value, which sets the fact's `expected_valid_days` to `min(days_since_created + extend_by_days, staleness_max_extension_days)`. The LLM assigns `expected_valid_days` when creating a fact; it is clamped at write time to `staleness_age_days × staleness_max_lifetime_multiplier` (creation cap). `_apply_updates` enforces the guardrail unconditionally at apply time: it intersects both the removal and extension sets with `_select_stale_candidates` output before applying the per-cycle cap (`staleness_max_removals_per_cycle`), so protected and non-aged facts can never be targeted regardless of model behavior or the feature flag setting. Facts the LLM proposed for removal are excluded from extension even if the per-cycle cap prevented their actual deletion that cycle. Extensions use an absolute ceiling (`staleness_max_extension_days`) rather than the creation multiplier so a deliberate review decision can advance the window beyond the initial cap while preventing `timedelta` overflow from a malformed `extend_by_days`. -- Consolidation pass (same LLM invocation as the regular updater, no extra API call): when `consolidation_enabled` is `true` and at least one category holds `consolidation_min_facts` or more facts, `_select_consolidation_candidates` identifies fragmented categories and surfaces at most `consolidation_max_groups_per_cycle` of them (largest first) in the prompt. The LLM decides which groups to merge and proposes a synthesised fact per group. `_apply_updates` enforces guardrails: source IDs must exist and must not overlap across groups, group size is capped at `consolidation_max_sources`, the merged fact's confidence cannot exceed the source maximum, and facts below `fact_confidence_threshold` are not written. +- Consolidation pass (same LLM invocation as the regular updater, no extra API call): when `consolidation_enabled` is `true` and at least one category holds `consolidation_min_facts` or more facts, `_select_consolidation_candidates` identifies fragmented categories and surfaces at most `consolidation_max_groups_per_cycle` of them (largest first) in the prompt. The LLM decides which groups to merge and proposes a synthesised fact per group. `_apply_updates` enforces guardrails: source IDs must exist and must not overlap across groups, group size is capped at `consolidation_max_sources`, the merged fact's confidence cannot exceed the source maximum, and facts below `fact_confidence_threshold` are not written. The merged fact carries the newest source's `createdAt` (so the staleness clock reflects the underlying information, not synthesis time) and inherits `expected_valid_days` set so the merged fact is re-reviewed at the earliest source review deadline (`min(createdAt + effective_lifetime)` across sources, where a source's effective lifetime is its `expected_valid_days` or the global `staleness_age_days` fallback for legacy facts without one - so a legacy source's default window is not swallowed by a long-lived sibling), relative to the merged `createdAt`, clamped to a minimal positive window if a source is already past its deadline, then capped at the creation-time `staleness_max_lifetime_multiplier`; this keeps a volatile or legacy sub-detail from inheriting a stable source's long window and escaping staleness review for years, while a merge of uniformly stable sources does not re-enter review prematurely. - Next interaction injects selected facts + context into `` tags in the system prompt when `injection_enabled` is true. **Run-level memory identity**: diff --git a/backend/packages/harness/deerflow/agents/memory/backends/deermem/deermem/core/updater.py b/backend/packages/harness/deerflow/agents/memory/backends/deermem/deermem/core/updater.py index 490832515..e6b8b822c 100644 --- a/backend/packages/harness/deerflow/agents/memory/backends/deermem/deermem/core/updater.py +++ b/backend/packages/harness/deerflow/agents/memory/backends/deermem/deermem/core/updater.py @@ -171,13 +171,11 @@ def _normalize_memory_update_fact(fact: Any) -> dict[str, Any] | None: normalized_fact["sourceError"] = normalized_source_error # Fact lifetime (expected_valid_days): optional LLM-assigned review window. - # Accept int/float (reject bool which subclasses int), coerce to int, keep - # only positive values; the creation-time cap is applied in _apply_updates. - raw_evd = fact.get("expected_valid_days") - if isinstance(raw_evd, (int, float)) and not isinstance(raw_evd, bool): - evd = int(raw_evd) - if evd > 0: - normalized_fact["expected_valid_days"] = evd + # Validated via the shared _read_expected_valid_days rule (reject bool, require + # finite, coerce to int, keep only positive); cap applied in _apply_updates. + evd = _read_expected_valid_days(fact) + if evd is not None: + normalized_fact["expected_valid_days"] = evd return normalized_fact @@ -392,6 +390,54 @@ def _parse_fact_datetime(raw: str) -> datetime | None: return None +def _read_expected_valid_days(fact: dict[str, Any]) -> int | None: + """Return a fact's ``expected_valid_days`` as a positive int, or ``None``. + + Accepts int/float (rejects ``bool``, which subclasses ``int``) and coerces + to int *before* the positivity check, mirroring the original + ``_normalize_memory_update_fact`` rule. Coercing first matters for values + in (0, 1): ``0.5`` passes a raw ``> 0`` check but truncates to ``0``, which + would otherwise be returned as a (non-positive) lifetime instead of + ``None``. Non-finite floats (``NaN``, ``+/-inf``) are rejected, and huge + ints are returned as-is rather than routed through ``float()`` (which + raises ``OverflowError`` for ``10**400``): Python's JSON decoder parses an + integer literal with no decimal point as an arbitrary-precision ``int``, + so a hand-edited ``memory.json`` can carry one. An int that is too large + to participate in ``datetime`` arithmetic is bounded by the caller via + :func:`_safe_add_days` - the helper's job is type/positivity validation, + not datetime-range validation, because the safe bound depends on the + ``datetime`` it is added to, not on the value alone. Returning ``None`` + lets callers fall back to the global age or omit the field rather than + silently writing a zero/negative/non-finite lifetime. + """ + raw = fact.get("expected_valid_days") + if isinstance(raw, bool): + return None + if isinstance(raw, int): + evd = raw # arbitrary-precision int; never routed through float() + elif isinstance(raw, float) and math.isfinite(raw): + evd = int(raw) # coerce before the positivity guard + else: + return None + return evd if evd > 0 else None + + +def _safe_add_days(dt: datetime, days: int) -> datetime | None: + """Return ``dt + timedelta(days=days)``, or ``None`` if it overflows. + + A huge persisted ``expected_valid_days`` (e.g. ``10**12``) can exceed + ``timedelta.max.days`` or push the result past ``datetime.max`` / below + ``datetime.min``. Both raise ``OverflowError``. The staleness and + consolidation paths add an evd to a ``datetime`` to compute a review + deadline; returning ``None`` lets the caller fall back to the configured + global lifetime instead of aborting the whole update cycle. + """ + try: + return dt + timedelta(days=days) + except (OverflowError, ValueError): + return None + + def _effective_fact_staleness_age(fact: dict[str, Any], config: Any) -> int: """Return the effective staleness review age in days for *fact*. @@ -404,10 +450,8 @@ def _effective_fact_staleness_age(fact: dict[str, Any], config: Any) -> int: ``staleness_age_days`` for facts that pre-date this feature or where the LLM did not provide an estimate. """ - raw = fact.get("expected_valid_days") - if isinstance(raw, (int, float)) and not isinstance(raw, bool) and raw > 0: - return int(raw) - return config.staleness_age_days + evd = _read_expected_valid_days(fact) + return evd if evd is not None else config.staleness_age_days def _select_stale_candidates( @@ -436,7 +480,11 @@ def _select_stale_candidates( if created_at is None: continue effective_age = _effective_fact_staleness_age(fact, config) - if created_at < now - timedelta(days=effective_age): + # now - timedelta(days=effective_age) can overflow datetime.min when + # effective_age is a huge persisted value; a window that large means the + # fact cannot yet be stale, so skip it rather than aborting the cycle. + cutoff = _safe_add_days(now, -effective_age) + if cutoff is not None and created_at < cutoff: candidates.append(fact) return candidates @@ -1128,6 +1176,9 @@ class MemoryUpdater: # Add new facts existing_fact_keys = {fact_key for fact_key in (_fact_content_key(fact.get("content")) for fact in current_memory.get("facts", [])) if fact_key is not None} new_facts = update_data.get("newFacts", []) + # Creation-time lifetime cap shared with the consolidation path below, so + # both fact-creation sites apply the identical bound in one place. + creation_cap = int(config.staleness_age_days * config.staleness_max_lifetime_multiplier) for fact in new_facts: confidence = fact.get("confidence", 0.5) if confidence >= config.fact_confidence_threshold: @@ -1157,14 +1208,13 @@ class MemoryUpdater: normalized_source_error = source_error.strip() if normalized_source_error: fact_entry["sourceError"] = normalized_source_error - evd = fact.get("expected_valid_days") - if isinstance(evd, int) and not isinstance(evd, bool) and evd > 0: + evd = _read_expected_valid_days(fact) + if evd is not None: # Apply the creation-time cap so the LLM cannot assign an # unbounded lifetime that defers staleness review indefinitely. # Extensions (staleFactsToExtend) bypass this cap via their own # staleness_max_extension_days ceiling because they represent a # deliberate review decision, not an unchecked initial assignment. - creation_cap = int(config.staleness_age_days * config.staleness_max_lifetime_multiplier) fact_entry["expected_valid_days"] = min(evd, creation_cap) current_memory["facts"].append(fact_entry) if fact_key is not None: @@ -1190,6 +1240,10 @@ class MemoryUpdater: fact_index = {f.get("id"): f for f in current_memory.get("facts", []) if isinstance(f, dict)} max_groups = config.consolidation_max_groups_per_cycle max_sources = config.consolidation_max_sources + # Creation-time lifetime cap shared with the newFacts path: an + # inherited expected_valid_days is clamped so a merge of long-lived + # sources cannot defer first review indefinitely. + creation_cap = int(config.staleness_age_days * config.staleness_max_lifetime_multiplier) ids_consumed: set[str] = set() new_consolidated: list[dict[str, Any]] = [] merge_count = 0 @@ -1278,6 +1332,54 @@ class MemoryUpdater: if source_errors: new_fact["sourceError"] = "\n".join(source_errors) + # Inherit expected_valid_days from the sources so the merged + # fact keeps the lifetime signal of the underlying information + # rather than silently degrading to the global staleness_age_days. + # The merged fact is re-reviewed at the EARLIEST source review + # deadline (createdAt + effective lifetime): a merge combines + # details from every source, and a volatile sub-detail (e.g. + # evd=7) must not inherit a stable source's 3650-day window and + # escape staleness review for years - staleness KEEP/REMOVE is the + # only path that re-validates a merged fact, so biasing toward the + # soonest deadline keeps uncertain merges re-checked sooner. + # Every source participates, including legacy facts without an + # explicit evd: their effective lifetime is the configured global + # staleness_age_days (matching _effective_fact_staleness_age's + # read-time fallback), so a legacy source's default 90-day window + # is not silently swallowed by a long-lived sibling. The deadline + # is expressed relative to the merged fact's createdAt (the newest + # source's), so a source already past its deadline yields a + # minimal positive window (review next cycle) rather than the + # global fallback, which would otherwise defer an overdue review. + # Capped at the creation-time multiplier (hoisted above the loop) + # like any new fact so consolidation cannot defer first review + # indefinitely. + # Compute each source's absolute review deadline + # (createdAt + effective lifetime). A huge persisted evd can + # overflow datetime arithmetic; _safe_add_days returns None + # then, and the source falls back to the global lifetime's + # deadline - the same treatment as a legacy (no-evd) source, + # so one malformed field cannot abort the merge. + global_age = config.staleness_age_days + source_deadlines: list[datetime] = [] + for sid, dt in zip(source_ids, _source_dts): + eff = _effective_fact_staleness_age(fact_index[sid], config) + deadline = _safe_add_days(dt, eff) + if deadline is None: + deadline = _safe_add_days(dt, global_age) or _newest_dt + source_deadlines.append(deadline) + earliest_deadline = min(source_deadlines) + # int(total_seconds() // 86400) avoids the .days toward-zero + # truncation inconsistency flagged in #4143; a negative result + # (a source already past its deadline) is clamped below. + days_until_earliest = int((earliest_deadline - _newest_dt).total_seconds() // 86400) + # A non-positive value means a source is already past its + # deadline (the merge itself was the overdue review) - surface + # a minimal positive window so the merged fact is re-reviewed + # next cycle instead of inheriting the global fallback. + inherited_evd = max(days_until_earliest, 1) + new_fact["expected_valid_days"] = min(inherited_evd, creation_cap) + ids_consumed.update(source_ids) new_consolidated.append(new_fact) merge_count += 1 diff --git a/backend/tests/test_memory_consolidation.py b/backend/tests/test_memory_consolidation.py index 90915005a..fd6bf470b 100644 --- a/backend/tests/test_memory_consolidation.py +++ b/backend/tests/test_memory_consolidation.py @@ -30,6 +30,7 @@ from deerflow.agents.memory.backends.deermem.deermem.core.updater import ( _build_consolidation_section, _normalize_memory_update_data, _select_consolidation_candidates, + _select_stale_candidates, ) # ── Helpers ──────────────────────────────────────────────────────────────── @@ -94,6 +95,13 @@ def _make_memory(facts: list[dict] | None = None) -> dict: } +def _days_ago(days: int) -> str: + """ISO-Z createdAt `days` before now - keeps evd-deadline tests time-stable + (a hardcoded 2025-01-01 would silently flip assertions once the fact exceeds + its window, e.g. around 2029 for a 1800-day cap).""" + return (datetime.now(UTC) - timedelta(days=days)).isoformat().replace("+00:00", "Z") + + # ── _select_consolidation_candidates ────────────────────────────────────── @@ -895,6 +903,519 @@ class TestReviewerFindings: # consolidatedAt should be more recent than the source dates assert merged[0]["consolidatedAt"] > newer_date + def test_consolidated_evd_uses_earliest_source_deadline(self): + """A merged fact is re-reviewed at the earliest source review deadline + (createdAt + expected_valid_days), not the longest source window. With + equal createdAt, that resolves to the smallest source evd - the merge + keeps every source's detail, so the soonest-expiring source governs + when the combined fact must be re-validated.""" + updater = _make_updater( + max_facts=100, + consolidation_enabled=True, + consolidation_min_facts=2, + consolidation_max_groups_per_cycle=3, + consolidation_max_sources=8, + staleness_age_days=90, + staleness_max_lifetime_multiplier=20.0, # creation cap = 1800 + ) + # Both sources 100 days old; evd 365 and 730 -> earliest deadline is the + # 365-day source's, so the merged fact inherits 365 (under the 1800 cap). + created = _days_ago(100) + facts = [ + {**_make_fact("fact_a", "Fact A", "knowledge", 0.9), "createdAt": created, "expected_valid_days": 365}, + {**_make_fact("fact_b", "Fact B", "knowledge", 0.85), "createdAt": created, "expected_valid_days": 730}, + ] + current_memory = _make_memory(facts) + update_data = { + "user": {}, + "history": {}, + "newFacts": [], + "factsToRemove": [], + "staleFactsToRemove": [], + "factsToConsolidate": [ + { + "sourceIds": ["fact_a", "fact_b"], + "consolidated": {"content": "A and B merged", "category": "knowledge", "confidence": 0.9}, + }, + ], + } + result = updater._apply_updates(current_memory, update_data) + + merged = [f for f in result["facts"] if f.get("source") == "consolidation"] + assert len(merged) == 1 + # earliest deadline (365-day source) - merged createdAt (same) = 365, under cap + assert merged[0]["expected_valid_days"] == 365 + + def test_consolidated_evd_capped_by_creation_multiplier(self): + """Inherited expected_valid_days is capped at the creation-time multiplier, + consistent with newFacts - consolidation cannot defer first review indefinitely.""" + updater = _make_updater( + max_facts=100, + consolidation_enabled=True, + consolidation_min_facts=2, + consolidation_max_groups_per_cycle=3, + consolidation_max_sources=8, + staleness_age_days=90, + staleness_max_lifetime_multiplier=3.0, # creation cap = 270 + ) + # Both sources fresh (10 days old) with long evd: earliest deadline is far + # out, but the creation cap (270) still clamps the inherited window. + created = _days_ago(10) + facts = [ + {**_make_fact("fact_a", "Fact A", "knowledge", 0.9), "createdAt": created, "expected_valid_days": 3650}, + {**_make_fact("fact_b", "Fact B", "knowledge", 0.85), "createdAt": created, "expected_valid_days": 3650}, + ] + current_memory = _make_memory(facts) + update_data = { + "user": {}, + "history": {}, + "newFacts": [], + "factsToRemove": [], + "staleFactsToRemove": [], + "factsToConsolidate": [ + { + "sourceIds": ["fact_a", "fact_b"], + "consolidated": {"content": "merged", "category": "knowledge", "confidence": 0.9}, + }, + ], + } + result = updater._apply_updates(current_memory, update_data) + + merged = [f for f in result["facts"] if f.get("source") == "consolidation"] + assert len(merged) == 1 + # earliest deadline (10 + 3650) - merged createdAt (10 days ago) = 3650, clamped to 270 + assert merged[0]["expected_valid_days"] == 270 + + def test_consolidated_evd_all_legacy_sources_uses_global_fallback_deadline(self): + """When no source carries expected_valid_days, every source's effective + lifetime is the global staleness_age_days (matching the read-time + fallback). The merged fact's deadline is derived from that fallback, not + omitted - so a merge of aged legacy facts still re-enters review rather + than silently inheriting nothing.""" + updater = _make_updater( + max_facts=100, + consolidation_enabled=True, + consolidation_min_facts=2, + consolidation_max_groups_per_cycle=3, + consolidation_max_sources=8, + staleness_age_days=90, + staleness_max_lifetime_multiplier=20.0, + ) + # Both legacy facts 100 days old, no evd -> effective lifetime 90 (global), + # deadline = createdAt + 90 = 10 days ago... but relative to the merged + # createdAt (100 days ago, same as both sources) that deadline is 90 days + # AFTER the merged createdAt, so the inherited window is 90 (not overdue: + # the deadline is a point in time; its offset from merged createdAt is 90). + created = _days_ago(100) + facts = [ + {**_make_fact("fact_a", "Fact A", "knowledge", 0.9), "createdAt": created}, + {**_make_fact("fact_b", "Fact B", "knowledge", 0.85), "createdAt": created}, + ] + current_memory = _make_memory(facts) + update_data = { + "user": {}, + "history": {}, + "newFacts": [], + "factsToRemove": [], + "staleFactsToRemove": [], + "factsToConsolidate": [ + { + "sourceIds": ["fact_a", "fact_b"], + "consolidated": {"content": "merged", "category": "knowledge", "confidence": 0.9}, + }, + ], + } + result = updater._apply_updates(current_memory, update_data) + + merged = [f for f in result["facts"] if f.get("source") == "consolidation"] + assert len(merged) == 1 + # Legacy fallback deadline (createdAt + 90) - merged createdAt (same) = 90. + assert merged[0]["expected_valid_days"] == 90 + # The 100-day-old merged fact (age 100 > 90) is immediately a staleness + # candidate next cycle. + next_cycle_candidates = _select_stale_candidates(result, updater._config) + assert any(f.get("source") == "consolidation" for f in next_cycle_candidates) + + def test_consolidated_evd_legacy_source_not_swallowed_by_stable_sibling(self): + """A legacy source (no evd -> global 90-day fallback) merged with a + long-lived source (evd=3650) must not inherit the long window. The merged + fact is re-reviewed at the legacy source's 90-day deadline, so the legacy + detail is not buried for years. Covers the mixed legacy/stable case with + equal and different createdAt values.""" + updater = _make_updater( + max_facts=100, + consolidation_enabled=True, + consolidation_min_facts=2, + consolidation_max_groups_per_cycle=3, + consolidation_max_sources=8, + staleness_age_days=90, + staleness_max_lifetime_multiplier=20.0, # cap 1800 + ) + # Equal createdAt, 50 days old. Legacy source's fallback deadline is + # createdAt + 90 = 40 days from now; stable source's is far future. + # earliest = legacy's; relative to merged createdAt (same) = 90. + created = _days_ago(50) + facts = [ + {**_make_fact("fact_legacy", "Legacy", "knowledge", 0.9), "createdAt": created}, + {**_make_fact("fact_stable", "Stable", "knowledge", 0.85), "createdAt": created, "expected_valid_days": 3650}, + ] + current_memory = _make_memory(facts) + update_data = { + "user": {}, + "history": {}, + "newFacts": [], + "factsToRemove": [], + "staleFactsToRemove": [], + "factsToConsolidate": [ + { + "sourceIds": ["fact_legacy", "fact_stable"], + "consolidated": {"content": "merged", "category": "knowledge", "confidence": 0.9}, + }, + ], + } + result = updater._apply_updates(current_memory, update_data) + + merged = [f for f in result["facts"] if f.get("source") == "consolidation"] + assert len(merged) == 1 + # Legacy source's 90-day deadline governs (not the stable 3650/cap-1800). + assert merged[0]["expected_valid_days"] == 90 + + # Different createdAt: legacy source older, so its fallback deadline is + # earlier still. Legacy 200 days old (deadline 110 days ago, past), stable + # 10 days old (deadline far future). merged createdAt = stable (10 days + # ago); earliest deadline is legacy's (110 days ago) -> negative, clamped. + facts_diff = [ + {**_make_fact("fact_legacy", "Legacy", "knowledge", 0.9), "createdAt": _days_ago(200)}, + {**_make_fact("fact_stable", "Stable", "knowledge", 0.85), "createdAt": _days_ago(10), "expected_valid_days": 3650}, + ] + result_diff = updater._apply_updates(_make_memory(facts_diff), update_data) + merged_diff = [f for f in result_diff["facts"] if f.get("source") == "consolidation"] + assert len(merged_diff) == 1 + # Legacy deadline (200 + 90 = 110 days ago) is before merged createdAt + # (10 days ago) -> negative delta clamped to 1. + assert merged_diff[0]["expected_valid_days"] == 1 + + def test_consolidated_evd_volatile_source_governs_earliest_deadline(self): + """A transient source (evd=7) merged with a stable source (evd=3650) must + not inherit the stable window - the merged fact is re-reviewed at the + volatile source's much sooner deadline, so the volatile sub-detail cannot + escape staleness review for years (staleness KEEP/REMOVE is the only path + that re-validates a merged fact).""" + updater = _make_updater( + max_facts=100, + consolidation_enabled=True, + consolidation_min_facts=2, + consolidation_max_groups_per_cycle=3, + consolidation_max_sources=8, + staleness_age_days=90, + staleness_max_lifetime_multiplier=20.0, + ) + # Both sources 100 days old. The volatile source (evd=7) makes the merged + # fact's review window 7 days (the earliest source deadline relative to + # the merged createdAt) - NOT the stable source's 3650/cap-1800. + created = _days_ago(100) + facts = [ + {**_make_fact("fact_stable", "Stable", "knowledge", 0.9), "createdAt": created, "expected_valid_days": 3650}, + {**_make_fact("fact_volatile", "Volatile", "knowledge", 0.85), "createdAt": created, "expected_valid_days": 7}, + ] + current_memory = _make_memory(facts) + update_data = { + "user": {}, + "history": {}, + "newFacts": [], + "factsToRemove": [], + "staleFactsToRemove": [], + "factsToConsolidate": [ + { + "sourceIds": ["fact_stable", "fact_volatile"], + "consolidated": {"content": "merged", "category": "knowledge", "confidence": 0.9}, + }, + ], + } + result = updater._apply_updates(current_memory, update_data) + + merged = [f for f in result["facts"] if f.get("source") == "consolidation"] + assert len(merged) == 1 + # earliest deadline = createdAt + 7 (volatile source); relative to merged + # createdAt (same) the window is 7 - far below the stable source's 3650. + assert merged[0]["expected_valid_days"] == 7 + # The merged fact is 100 days old but has a 7-day window, so it is + # immediately a staleness candidate next cycle - the volatile sub-detail + # gets re-reviewed instead of being buried for years. + next_cycle_candidates = _select_stale_candidates(result, updater._config) + assert any(f.get("source") == "consolidation" for f in next_cycle_candidates), "volatile-source merge must re-enter staleness review" + + @pytest.mark.parametrize("bad_evd", [float("nan"), float("inf"), float("-inf")], ids=["nan", "inf", "-inf"]) + def test_consolidation_with_non_finite_source_evd_does_not_raise(self, bad_evd): + """A malformed non-finite expected_valid_days (NaN / +/-inf) in a + hand-edited memory.json must not abort consolidation. The source's + effective lifetime falls back to the global staleness_age_days, so its + deadline still participates in the earliest-deadline computation instead + of raising ValueError/OverflowError during int() coercion.""" + updater = _make_updater( + max_facts=100, + consolidation_enabled=True, + consolidation_min_facts=2, + consolidation_max_groups_per_cycle=3, + consolidation_max_sources=8, + staleness_age_days=90, + staleness_max_lifetime_multiplier=20.0, + ) + created = _days_ago(100) + facts = [ + {**_make_fact("fact_bad", "Bad evd", "knowledge", 0.9), "createdAt": created, "expected_valid_days": bad_evd}, + {**_make_fact("fact_stable", "Stable", "knowledge", 0.85), "createdAt": created, "expected_valid_days": 3650}, + ] + current_memory = _make_memory(facts) + update_data = { + "user": {}, + "history": {}, + "newFacts": [], + "factsToRemove": [], + "staleFactsToRemove": [], + "factsToConsolidate": [ + { + "sourceIds": ["fact_bad", "fact_stable"], + "consolidated": {"content": "merged", "category": "knowledge", "confidence": 0.9}, + }, + ], + } + # Must not raise. The bad source's non-finite evd falls back to the global + # 90, whose deadline (createdAt + 90) governs over the stable 3650. + result = updater._apply_updates(current_memory, update_data) + + merged = [f for f in result["facts"] if f.get("source") == "consolidation"] + assert len(merged) == 1 + # earliest deadline = createdAt + 90 (non-finite source fallback), relative + # to merged createdAt (same) = 90. + assert merged[0]["expected_valid_days"] == 90 + + @pytest.mark.parametrize( + "bad_evd", + [10**400, 10**12, 10**9, timedelta.max.days], + ids=["1e400", "1e12", "1e9", "timedelta_max"], + ) + def test_consolidation_with_huge_int_source_evd_does_not_raise(self, bad_evd): + """A huge int expected_valid_days (above timedelta.max.days) in a + hand-edited memory.json must not abort consolidation. Python's JSON + decoder parses an integer literal with no decimal point as an + arbitrary-precision int, so 10**400 stays an int (not float inf); the + helper rejects it so it falls back to the global staleness_age_days + instead of raising OverflowError in float() or in timedelta() downstream.""" + updater = _make_updater( + max_facts=100, + consolidation_enabled=True, + consolidation_min_facts=2, + consolidation_max_groups_per_cycle=3, + consolidation_max_sources=8, + staleness_age_days=90, + staleness_max_lifetime_multiplier=20.0, + ) + created = _days_ago(100) + facts = [ + {**_make_fact("fact_bad", "Bad evd", "knowledge", 0.9), "createdAt": created, "expected_valid_days": bad_evd}, + {**_make_fact("fact_stable", "Stable", "knowledge", 0.85), "createdAt": created, "expected_valid_days": 3650}, + ] + current_memory = _make_memory(facts) + update_data = { + "user": {}, + "history": {}, + "newFacts": [], + "factsToRemove": [], + "staleFactsToRemove": [], + "factsToConsolidate": [ + { + "sourceIds": ["fact_bad", "fact_stable"], + "consolidated": {"content": "merged", "category": "knowledge", "confidence": 0.9}, + }, + ], + } + # Must not raise. The bad source's huge-int evd falls back to the global + # 90, whose deadline governs over the stable 3650. + result = updater._apply_updates(current_memory, update_data) + + merged = [f for f in result["facts"] if f.get("source") == "consolidation"] + assert len(merged) == 1 + assert merged[0]["expected_valid_days"] == 90 + + def test_consolidated_evd_overdue_source_clamps_to_minimal_window(self): + """When a source's review deadline (createdAt + evd) is earlier than the + merged fact's createdAt (the newest source's) - e.g. a very old source + with a short window merged with a fresh source - the inherited window + would be negative. It is clamped to a minimal positive value so the + merged fact is re-reviewed next cycle instead of carrying a non-positive + lifetime or falling back to the (longer) global age.""" + updater = _make_updater( + max_facts=100, + consolidation_enabled=True, + consolidation_min_facts=2, + consolidation_max_groups_per_cycle=3, + consolidation_max_sources=8, + staleness_age_days=90, + staleness_max_lifetime_multiplier=20.0, + ) + # Old source: 200 days old, evd=7 -> deadline 193 days ago. + # Fresh source: 10 days old, evd=3650 -> deadline far in future. + # merged createdAt = fresh source (10 days ago); earliest deadline is the + # old source's (193 days ago), which is BEFORE the merged createdAt -> + # negative delta clamped to 1. + facts = [ + {**_make_fact("fact_old", "Old volatile", "knowledge", 0.9), "createdAt": _days_ago(200), "expected_valid_days": 7}, + {**_make_fact("fact_fresh", "Fresh stable", "knowledge", 0.85), "createdAt": _days_ago(10), "expected_valid_days": 3650}, + ] + current_memory = _make_memory(facts) + update_data = { + "user": {}, + "history": {}, + "newFacts": [], + "factsToRemove": [], + "staleFactsToRemove": [], + "factsToConsolidate": [ + { + "sourceIds": ["fact_old", "fact_fresh"], + "consolidated": {"content": "merged", "category": "knowledge", "confidence": 0.9}, + }, + ], + } + result = updater._apply_updates(current_memory, update_data) + + merged = [f for f in result["facts"] if f.get("source") == "consolidation"] + assert len(merged) == 1 + # Overdue deadline relative to merged createdAt -> clamped to 1. + assert merged[0]["expected_valid_days"] == 1 + + def test_consolidated_evd_volatile_source_with_equal_created_at_future_deadline(self): + """When the volatile source's deadline has NOT yet passed, the merged fact + inherits exactly the remaining days to that deadline (not the stable + source's long window). Sources created recently so the volatile deadline + is still in the future.""" + updater = _make_updater( + max_facts=100, + consolidation_enabled=True, + consolidation_min_facts=2, + consolidation_max_groups_per_cycle=3, + consolidation_max_sources=8, + staleness_age_days=90, + staleness_max_lifetime_multiplier=20.0, + ) + # The window is relative to the merged createdAt, not elapsed-since- + # creation: days_until = (createdAt + 7) - createdAt = 7, regardless of + # the source's current age. + created = _days_ago(3) + facts = [ + {**_make_fact("fact_stable", "Stable", "knowledge", 0.9), "createdAt": created, "expected_valid_days": 3650}, + {**_make_fact("fact_volatile", "Volatile", "knowledge", 0.85), "createdAt": created, "expected_valid_days": 7}, + ] + current_memory = _make_memory(facts) + update_data = { + "user": {}, + "history": {}, + "newFacts": [], + "factsToRemove": [], + "staleFactsToRemove": [], + "factsToConsolidate": [ + { + "sourceIds": ["fact_stable", "fact_volatile"], + "consolidated": {"content": "merged", "category": "knowledge", "confidence": 0.9}, + }, + ], + } + result = updater._apply_updates(current_memory, update_data) + + merged = [f for f in result["facts"] if f.get("source") == "consolidation"] + assert len(merged) == 1 + # earliest deadline is the volatile source's (createdAt + 7); relative to + # merged createdAt (same) that is 7 days - well under the 1800 cap. + assert merged[0]["expected_valid_days"] == 7 + + def test_consolidated_evd_float_source_coerced_to_int(self): + """A hand-edited memory.json may store expected_valid_days as a float; + the inherited deadline is computed from the int-coerced value like every + other read path.""" + updater = _make_updater( + max_facts=100, + consolidation_enabled=True, + consolidation_min_facts=2, + consolidation_max_groups_per_cycle=3, + consolidation_max_sources=8, + staleness_age_days=90, + staleness_max_lifetime_multiplier=20.0, + ) + # Sources 10 days old; float evds 365.7 and 180.2 -> int 365 and 180. + # Earliest deadline is the 180-day source's -> inherited window = 180. + created = _days_ago(10) + facts = [ + {**_make_fact("fact_a", "Fact A", "knowledge", 0.9), "createdAt": created, "expected_valid_days": 365.7}, + {**_make_fact("fact_b", "Fact B", "knowledge", 0.85), "createdAt": created, "expected_valid_days": 180.2}, + ] + current_memory = _make_memory(facts) + update_data = { + "user": {}, + "history": {}, + "newFacts": [], + "factsToRemove": [], + "staleFactsToRemove": [], + "factsToConsolidate": [ + { + "sourceIds": ["fact_a", "fact_b"], + "consolidated": {"content": "merged", "category": "knowledge", "confidence": 0.9}, + }, + ], + } + result = updater._apply_updates(current_memory, update_data) + + merged = [f for f in result["facts"] if f.get("source") == "consolidation"] + assert len(merged) == 1 + # int(180.2) = 180 governs (earliest deadline), under the 1800 cap + assert merged[0]["expected_valid_days"] == 180 + + def test_consolidation_preserves_stable_lifetime_across_cycle(self): + """End-to-end: merging aged-but-stable sources must not make the merged + fact immediately stale next cycle. Before this fix the merged fact had no + expected_valid_days, fell back to staleness_age_days=90, and (with an old + createdAt) re-entered the staleness candidate set right away.""" + updater = _make_updater( + max_facts=100, + consolidation_enabled=True, + consolidation_min_facts=2, + consolidation_max_groups_per_cycle=3, + consolidation_max_sources=8, + staleness_age_days=90, + staleness_max_lifetime_multiplier=20.0, + staleness_min_candidates=1, + ) + # Two 200-day-old stable facts (evd 5 years) in the same category. + created = _days_ago(200) + facts = [ + {**_make_fact("fact_a", "Fact A", "knowledge", 0.9), "createdAt": created, "expected_valid_days": 3650}, + {**_make_fact("fact_b", "Fact B", "knowledge", 0.85), "createdAt": created, "expected_valid_days": 3650}, + ] + current_memory = _make_memory(facts) + update_data = { + "user": {}, + "history": {}, + "newFacts": [], + "factsToRemove": [], + "staleFactsToRemove": [], + "factsToConsolidate": [ + { + "sourceIds": ["fact_a", "fact_b"], + "consolidated": {"content": "merged stable skill", "category": "knowledge", "confidence": 0.9}, + }, + ], + } + result = updater._apply_updates(current_memory, update_data) + + merged = [f for f in result["facts"] if f.get("source") == "consolidation"] + assert len(merged) == 1 + # Both sources share createdAt + evd=3650, so earliest deadline is + # createdAt+3650; relative to the merged createdAt that is 3650, clamped + # to the 1800 creation cap. The 200-day-old merged fact (200 < 1800) is + # therefore NOT yet stale and stays out of the next-cycle candidate set. + assert merged[0]["expected_valid_days"] == 1800 + next_cycle_candidates = _select_stale_candidates(result, updater._config) + assert all(f.get("source") != "consolidation" for f in next_cycle_candidates), "merged stable fact must not re-enter staleness review immediately" + def test_confidence_fallback_to_max_source_when_llm_omits_field(self): """Finding 5: when LLM omits confidence field entirely, merged fact uses max_source_conf.""" updater = _make_updater( diff --git a/backend/tests/test_memory_staleness_review.py b/backend/tests/test_memory_staleness_review.py index 72639a4f5..3db4a15dc 100644 --- a/backend/tests/test_memory_staleness_review.py +++ b/backend/tests/test_memory_staleness_review.py @@ -26,6 +26,8 @@ from deerflow.agents.memory.backends.deermem.deermem.core.updater import ( _effective_fact_staleness_age, _normalize_memory_update_data, _parse_fact_datetime, + _read_expected_valid_days, + _safe_add_days, _select_stale_candidates, ) @@ -124,6 +126,65 @@ class TestParseFactDatetime: assert result.tzinfo is not None +# ── _read_expected_valid_days ───────────────────────────────────────────── + + +class TestReadExpectedValidDays: + """Shared validator used by the newFact creation cap, the staleness read + path, and consolidation inheritance - so its type rule is tested once here + rather than reimplemented per call site.""" + + def test_returns_int_when_present(self): + assert _read_expected_valid_days({"expected_valid_days": 365}) == 365 + + def test_coerces_float_to_int(self): + assert _read_expected_valid_days({"expected_valid_days": 180.7}) == 180 + + def test_returns_none_when_absent(self): + assert _read_expected_valid_days({}) is None + + def test_ignores_bool(self): + assert _read_expected_valid_days({"expected_valid_days": True}) is None + + def test_ignores_zero_and_negative(self): + assert _read_expected_valid_days({"expected_valid_days": 0}) is None + assert _read_expected_valid_days({"expected_valid_days": -5}) is None + + def test_fractional_below_one_returns_none(self): + # 0.5 passes a raw > 0 check but truncates to 0 - the int coercion must + # happen BEFORE the positivity guard, else 0 leaks out as a (non-positive) + # lifetime instead of None. Regression for the helper-extraction order bug. + assert _read_expected_valid_days({"expected_valid_days": 0.5}) is None + assert _read_expected_valid_days({"expected_valid_days": 0.9}) is None + + def test_ignores_non_numeric(self): + assert _read_expected_valid_days({"expected_valid_days": "365"}) is None + + def test_rejects_non_finite_values(self): + # int(nan) raises ValueError and int(inf) raises OverflowError; the helper + # must reject non-finite floats (NaN, +/-inf) before coercion so a single + # malformed field in a hand-edited memory.json cannot abort staleness + # selection or consolidation. Python's JSON decoder accepts these as floats. + + for bad in (float("nan"), float("inf"), float("-inf")): + assert _read_expected_valid_days({"expected_valid_days": bad}) is None + # sanity: the helper no longer raises on these + assert _read_expected_valid_days({"expected_valid_days": float("nan")}) is None + + def test_accepts_huge_int_without_routing_through_float(self): + # Python's JSON decoder parses an integer literal with no decimal point as + # an arbitrary-precision int (not a float), so a hand-edited memory.json + # can carry 10**400. The helper must not route it through float() (which + # raises OverflowError); it returns the int as-is. Whether that int can + # participate in datetime arithmetic is the caller's concern, handled by + # _safe_add_days - the helper's job is type/positivity validation only. + from datetime import timedelta + + assert _read_expected_valid_days({"expected_valid_days": 10**400}) == 10**400 + assert _read_expected_valid_days({"expected_valid_days": timedelta.max.days}) == timedelta.max.days + assert _read_expected_valid_days({"expected_valid_days": timedelta.max.days + 1}) == timedelta.max.days + 1 + + # ── _effective_fact_staleness_age ───────────────────────────────────────── @@ -164,6 +225,77 @@ class TestEffectiveFactStalenessAge: fact["expected_valid_days"] = bad assert _effective_fact_staleness_age(fact, config) == 90 + def test_falls_back_for_fractional_below_one(self): + # A hand-edited 0.5 must NOT be treated as an explicit (zero) lifetime + # that makes the fact immediately stale - it falls back to the global + # age like any other invalid value. Guards the coercion-order regression + # where int() ran after the > 0 check. + config = _memory_config(staleness_age_days=90) + for bad in (0.5, 0.9): + fact = _make_fact("f1", days_ago=100) + fact["expected_valid_days"] = bad + assert _effective_fact_staleness_age(fact, config) == 90 + + def test_falls_back_for_non_finite_values(self): + # NaN / +/-inf in a hand-edited memory.json must fall back to the global + # age instead of raising (int(nan) -> ValueError, int(inf) -> OverflowError). + # Drives the persisted-fact read path the reviewer flagged. + config = _memory_config(staleness_age_days=90) + for bad in (float("nan"), float("inf"), float("-inf")): + fact = _make_fact("f1", days_ago=100) + fact["expected_valid_days"] = bad + assert _effective_fact_staleness_age(fact, config) == 90 + + def test_returns_huge_int_as_is(self): + # A huge int is returned as-is (not routed through float, not rejected); + # datetime-overflow safety is the caller's job (_safe_add_days). The read + # path itself must not raise on such a stored value. + config = _memory_config(staleness_age_days=90) + for v in (10**400, 10**12, 10**9): + fact = _make_fact("f1", days_ago=100) + fact["expected_valid_days"] = v + assert _effective_fact_staleness_age(fact, config) == v + + +# ── _safe_add_days ──────────────────────────────────────────────────────── + + +class TestSafeAddDays: + """Guards datetime arithmetic against huge persisted expected_valid_days. + + A stored evd above timedelta.max.days (or that pushes the result past + datetime.min/max) raises OverflowError in timedelta(days=...) or in the + addition. _safe_add_days returns None so callers can fall back instead of + aborting the whole staleness/consolidation cycle. + """ + + def test_normal_shift(self): + from datetime import UTC, datetime, timedelta + + dt = datetime.now(UTC) + assert _safe_add_days(dt, 365) == dt + timedelta(days=365) + + def test_negative_shift(self): + from datetime import UTC, datetime, timedelta + + dt = datetime.now(UTC) + assert _safe_add_days(dt, -365) == dt + timedelta(days=-365) + + def test_huge_int_returns_none(self): + from datetime import UTC, datetime + + dt = datetime.now(UTC) + for bad in (10**400, 10**12, 10**9): + assert _safe_add_days(dt, bad) is None + + def test_overflow_past_datetime_max_returns_none(self): + # timedelta.max.days itself constructs but now + timedelta.max.days + # overflows datetime.max - must return None, not raise. + from datetime import UTC, datetime, timedelta + + dt = datetime.now(UTC) + assert _safe_add_days(dt, timedelta.max.days) is None + # ── _select_stale_candidates ────────────────────────────────────────────── @@ -213,6 +345,16 @@ class TestSelectStaleCandidates: assert len(candidates) == 1 assert candidates[0]["id"] == "f1" + def test_huge_evd_does_not_abort_selection(self): + # A huge persisted expected_valid_days (10**400) makes the review window + # unrepresentable in datetime arithmetic. The fact cannot yet be stale + # (its window is astronomically large), so it is skipped - not selected, + # and crucially the selection does not raise. + config = _memory_config(staleness_age_days=90) + for bad in (10**400, 10**12, 10**9): + memory = _make_memory([_make_fact("f1", days_ago=100, expected_valid_days=bad)]) + assert _select_stale_candidates(memory, config) == [] + # ── Trigger conditions via _select_stale_candidates + config ─────────────