fix(pfs): refresh_from never clears FAMILIES mid-swap; fix its stale docstring (refs #691 #692)

Minor from the final fix wave: refresh_from used to FAMILIES.clear()
then .update(merged) — a concurrent detect_codes/family_of read
(the chat runs each turn in a threadpool worker) could observe an
empty registry in the window between those two calls. Now
FAMILIES.update(merged) first, then delete whatever key isn't in the
new merge — every in-between state is old UNION new, never empty.

Also fixes the FAMILIES module docstring, which still said "the chat
sees hand families only" — stale since Task 1 (P49) wired
llm.evidence.warm/_connect to call refresh_from on every replica
(re)open, at API startup and again before every detect_codes call.

New tests: a stale key not in the new merge is dropped by the end of
refresh_from; refresh_from never calls FAMILIES.clear() (verified by
swapping in a dict subclass whose clear() raises).
This commit is contained in:
kert
2026-09-10 13:32:05 -04:00
parent c1a199c878
commit aa108797a2
2 changed files with 86 additions and 8 deletions

View File

@@ -111,13 +111,16 @@ HAND_FAMILIES: dict[str, Family] = {
#: Live registry. Starts as the hand list; ``refresh_from(con)`` merges in #: Live registry. Starts as the hand list; ``refresh_from(con)`` merges in
#: the derived families (``pfs.code_family``) in place — hand keys seeded #: the derived families (``pfs.code_family``) in place — hand keys seeded
#: first, derived rows extend/override per key — so every importer sees #: first, derived rows extend/override per key — so every importer sees
#: the same dict object. Only the ``stack pfs`` CLI (``elements``, #: the same dict object. A live DuckDB connection is forbidden at import
#: ``lineage``, ``families``) calls ``refresh_from`` today; the spec's #: (the ``llm`` chat container must import this module with no database
#: §Components asked for an import-time load, but that needs a live #: reachable), so ``FAMILIES`` starts as the hand list only; the ``stack
#: DuckDB connection, which is forbidden at import (the ``llm`` chat #: pfs`` CLI (``elements``, ``lineage``, ``families``) calls
#: container must import this module with no database reachable). Until #: ``refresh_from`` directly, and the chat picks up the derived families
#: something calls ``refresh_from`` in that path too — a controller #: the moment it opens (or reopens) the DuckDB replica — ``llm.evidence
#: follow-up issue — the chat sees hand families only. #: warm``/``_connect`` call ``refresh_from`` on every (re)open (Ruling
#: B3), at API startup and again, defensively, before every
#: ``detect_codes`` call — so in practice the chat sees the full derived
#: registry from its first request onward, not hand families only.
FAMILIES: dict[str, Family] = dict(HAND_FAMILIES) FAMILIES: dict[str, Family] = dict(HAND_FAMILIES)
#: Guards the swap of ``FAMILIES``/``_PHRASE_RE``/``_PHRASE_INDEX``/ #: Guards the swap of ``FAMILIES``/``_PHRASE_RE``/``_PHRASE_INDEX``/
@@ -987,7 +990,14 @@ def refresh_from(con: Any) -> int:
phrase_re, phrase_index, code_index = _build_registry_index(merged) phrase_re, phrase_index, code_index = _build_registry_index(merged)
global _PHRASE_RE, _PHRASE_INDEX, _CODE_INDEX global _PHRASE_RE, _PHRASE_INDEX, _CODE_INDEX
with _REGISTRY_LOCK: with _REGISTRY_LOCK:
FAMILIES.clear() # Minor fix (final fix wave): update in place, THEN drop keys the
# new merge doesn't carry — a bare ``clear()`` first left a window
# (however short) where a concurrent reader iterating FAMILIES
# (not just the swapped index) saw it empty. This way every
# in-between state is old UNION new, never empty.
stale = [k for k in FAMILIES if k not in merged]
FAMILIES.update(merged) FAMILIES.update(merged)
for k in stale:
del FAMILIES[k]
_PHRASE_RE, _PHRASE_INDEX, _CODE_INDEX = phrase_re, phrase_index, code_index _PHRASE_RE, _PHRASE_INDEX, _CODE_INDEX = phrase_re, phrase_index, code_index
return len(FAMILIES) return len(FAMILIES)

View File

@@ -1340,6 +1340,74 @@ class TestLoadAndRefresh:
finally: finally:
con.close() con.close()
def test_stale_key_not_in_the_new_merge_is_dropped(self, restore_families):
# Final fix wave minor: a key that was in FAMILIES before this
# refresh (a previous derivation run's family, say) but isn't in
# the new merge must still end up gone — update-then-delete must
# reach the same end state a clear()-then-update would.
mod = restore_families
mod.FAMILIES["STALE-KEY"] = Family("STALE-KEY", "Stale", ("Z9999",), ())
con = duckdb.connect(":memory:")
try:
ensure_tables(con)
write_families(
con,
[
FamilyRow(
"CCM",
"Chronic Care Management",
"99490",
"base",
None,
None,
"",
0,
)
],
)
refresh_from(con)
assert "STALE-KEY" not in mod.FAMILIES
finally:
con.close()
def test_refresh_never_clears_the_registry(self, restore_families):
# Final fix wave minor: refresh_from must never leave a window
# where FAMILIES is empty — verified by swapping in a dict
# subclass whose clear() raises (the old clear()-then-update()
# implementation this replaces would trip it immediately).
class _NoClearDict(dict):
def clear(self) -> None:
raise AssertionError("refresh_from must not clear() FAMILIES")
mod = restore_families
original = mod.FAMILIES
mod.FAMILIES = _NoClearDict(original)
con = duckdb.connect(":memory:")
try:
ensure_tables(con)
write_families(
con,
[
FamilyRow(
"CCM",
"Chronic Care Management",
"99490",
"base",
None,
None,
"",
0,
)
],
)
refresh_from(con) # must not raise
assert "CCM" in mod.FAMILIES
finally:
con.close()
# Swap the plain dict back before restore_families' own
# teardown (.clear()/.update()) runs on mod.FAMILIES.
mod.FAMILIES = original
def test_hand_key_absent_from_table_survives(self, restore_families): def test_hand_key_absent_from_table_survives(self, restore_families):
# Ruling 17: only CCM appears in the derived table; the other four # Ruling 17: only CCM appears in the derived table; the other four
# hand families (ACP, PCM, TCM, APCM) must come through unchanged. # hand families (ACP, PCM, TCM, APCM) must come through unchanged.