phase: 190_app_name_settings_fix
All gates re-verified green. Report: - **Verified (all 4 tasks already complete; this was the final pass):** `effective_settings` resolves the 3 brand strings panel-row → tab-row → env (`applied_override_keys`, presence-not-value); `PUT /api/ui-settings` ignores string body fields; Theme-tab form is colors-only (17 pickers, no `#theme-app-name`/`#theme-placeholder`/`#theme-footer`); the ONE shared `applyBrandFromConfig` re-applies title/`.brand-text`/footer/placeholder after a panel save-reset and a tab save-reset; boot path byte-identical (single `/api/config`). - **Completion criteria:** - strings order panel→tab→env, both branches — ✅ `tests/unit/test_theming.py::test_effective_app_name_prefers_settings_panel_row_over_ui_settings_row`, `..._fall_back_to_ui_settings_row_without_panel_row`, `..._per_field_panel_then_tab_then_env`, `..._ignore_panel_rows_when_the_overlay_is_off` - PUT ignores string fields + no 3 inputs in the tab — ✅ `tests/integration/test_ui_settings_api.py::test_put_ignores_string_fields_in_body` (+2) and the E2E `test_theme_tab_dom_is_colors_only` - live apply without reload + incident regression — ✅ `tests/e2e/test_app_name_settings_fix.py` tests 1–4 - byte-identical boot, no double-apply — ✅ `tests/unit/test_brand_live.py` markers + E2E "exactly one /api/config" - suite green, coverage >90%, ruff+pyright clean — ✅ - no behavior change in prior phases — ✅ (palette, phase-168 panel, env fallback suites all green) - atomic commit — **left to the harness** (pipeline rule overrides per-phase committing; no git write run) - **Gates:** `uv run pytest --cov=app --cov-report=term-missing` → **4630 passed, TOTAL 6941 16 99%**, exit 0 · `uv run pytest tests/e2e/test_app_name_settings_fix.py -v --no-cov` → **6 passed** in isolation · regression E2E (isolated, one process each, all exit 0): `test_smoke` `test_configurable_brand` `test_admin_theme_tab` `test_theme_save_and_coverage` `test_theme_semantic_completion` `test_themed_favicon` `test_dark_tech_theme` `test_admin_settings_panel` `test_admin_settings_layout` `test_settings_mobile_value_width` · `uv run ruff check .` → All checks passed · `uv run pyright` → 0 errors. - **Documentation:** README — fixed "saving one repaints *every* open page" (only the open document repaints; other pages on next load) and a broken paragraph wrap in *Customizing the look*; AGENTS.md rule 16 — `test_chunks_hnsw_recall.py` now described accurately (index paths forced with `enable_seqscan`/`indexonlyscan`/`bitmapscan` off, unforced plan asserted only on a pristine relation); `tests/unit/test_theme_frontend.py` — two stale section headers ("20 form fields"/"20 inputs" → 17 color). No AGENTS.md settings-overlay/resolution-order clause exists, so per the phase none was added. Full env-var cross-check (`BOR_*` in `app/config.py` vs README) → 0 gaps; 0 missing file references. - **Notable:** the working tree also carries this phase's flake hardening of the phase-188 `tests/integration/test_chunks_hnsw_recall.py` (verified legit and green). Live infra untouched; shared DB left clean (0 `app_settings`, 0 `ui_settings`). **Flagged, not changed:** `settings_overlay.applied_override_keys` lets a `ProgrammingError` (missing `app_settings`) propagate from public `GET /api/config`, where `load_overrides` deliberately fails soft — only reachable on an un-migrated (0027) DB, outside this phase's pinned A1 semantics, so left for the owner to rule on rather than silently deviating. - **Next pending phase:** `191_source_name_labels_tags`.
This commit is contained in:
@@ -44,6 +44,28 @@ database returned them, so the comparison measures the index. Setup stays inside
|
||||
generate ≈ 0.5 s, seed ≈ 2 s, ``CREATE INDEX CONCURRENTLY`` ≈ 1.4 s, read-back +
|
||||
exact top-100 ≈ 3.5 s — ≈ 8 s against the ~60 s target.
|
||||
|
||||
**The determinism premise (measured while fixing the 2026-10-18 flake).** A
|
||||
plan is a cost decision, and two things outside this module's control move it
|
||||
on a shared dev DB: the statistics of BOTH relations the tier's SELECT joins
|
||||
(the fixture used to ``ANALYZE`` only ``chunks``, so the join side could carry
|
||||
an estimate from whatever ran before it) and the ``embedding IS NULL`` fraction
|
||||
of ``chunks`` — a NULL-embedding row is a candidate the index scan must skip
|
||||
before the LIMIT fills, so ambient NULL rows inflate the estimated index cost.
|
||||
Measured on this corpus with ambient NULL-embedding rows inserted alongside the
|
||||
5,000 fixture rows: at 4,000 of them the UNFORCED planner leaves
|
||||
``ix_chunks_embedding_hnsw`` for ``Sort → Hash Join → Seq Scan on chunks`` with
|
||||
the index present and valid (at 2,000 and at 18,000 it uses it again — the band
|
||||
moves with the statistics, which is exactly what makes it a flake rather than a
|
||||
finding). Therefore: the fixture analyzes both joined relations, asserts the
|
||||
index is present and VALID before any plan is read, and the index assertions run
|
||||
with the alternatives that can disguise a scan refused
|
||||
(``enable_seqscan`` AND ``enable_bitmapscan`` — verified across the pollution
|
||||
levels above, and verified to still report ``False`` with the index dropped, so
|
||||
the pin cannot pass on a missing index). The UNFORCED preference — the shape of
|
||||
the 2026-10-15 incident — is asserted when the relation holds nothing but the
|
||||
fixture corpus, which is the case a clean CI DB and this file's own teardown
|
||||
leave.
|
||||
|
||||
**What 5,000 rows proves and what it does not.** It is enough for a PLAN pin
|
||||
(the planner needs a populated relation to prefer an ordered index scan over a
|
||||
seq scan + top-N sort) and enough for a deterministic index-vs-exact
|
||||
@@ -326,6 +348,23 @@ def _plan(session: Session, sql: str) -> list[dict[str, Any]]:
|
||||
return nodes
|
||||
|
||||
|
||||
def _index_validity(session: Session) -> bool | None:
|
||||
"""``pg_index.indisvalid`` for :data:`INDEX_NAME` (``None`` = absent) — the
|
||||
catalog fact behind every plan assertion here, so a failure names the
|
||||
missing/INVALID index instead of a plan shape."""
|
||||
value = session.execute(
|
||||
text(
|
||||
"SELECT i.indisvalid"
|
||||
" FROM pg_index i"
|
||||
" JOIN pg_class c ON c.oid = i.indexrelid"
|
||||
" WHERE c.relname = :name"
|
||||
),
|
||||
{"name": INDEX_NAME},
|
||||
).scalar()
|
||||
session.rollback()
|
||||
return None if value is None else bool(value)
|
||||
|
||||
|
||||
def _describe(nodes: list[dict[str, Any]]) -> str:
|
||||
return ", ".join(
|
||||
f"{node.get('Node Type')}[{node.get('Index Name') or node.get('Relation Name') or '?'}]"
|
||||
@@ -369,7 +408,12 @@ def hnsw_corpus() -> Iterator[dict[str, Any]]:
|
||||
config = Config() # no alembic.ini — env.py reads the URL from app config
|
||||
config.set_main_option("script_location", "alembic")
|
||||
try:
|
||||
preexisting = session.execute(text(f"SELECT count(*) FROM {RELATION}")).scalar() or 0
|
||||
# `preexisting` is the relation's own state (the skip guard below);
|
||||
# its EMBEDDED half is what the exact reference can score, and the
|
||||
# pristine flag is the premise of the unforced-plan arm of pin 1.
|
||||
preexisting, preexisting_embedded = session.execute(
|
||||
text(f"SELECT count(*), count(embedding) FROM {RELATION}")
|
||||
).one()
|
||||
if preexisting > MAX_RELATION_ROWS:
|
||||
pytest.skip(
|
||||
f"{RELATION} holds {preexisting} rows: the exact reference is a Python "
|
||||
@@ -384,15 +428,34 @@ def hnsw_corpus() -> Iterator[dict[str, Any]]:
|
||||
session.rollback()
|
||||
command.upgrade(config, "0029") # the ONE definition of the index
|
||||
session.rollback()
|
||||
# Planner statistics for the plan pin: a freshly loaded relation has
|
||||
# none, and a row estimate of 0 makes "seq scan + top-N sort" look free.
|
||||
# The premise of every pin below, checked where it is established: an
|
||||
# INVALID remnant (phase 188 D2) or a drop by whatever else owns the
|
||||
# schema on this box must report ITSELF, not surface three tests later
|
||||
# as "the plan did not name the index".
|
||||
validity = _index_validity(session)
|
||||
assert validity is True, (
|
||||
f"{INDEX_NAME} is {validity!r} after migration 0029 "
|
||||
"(None = absent, False = the INVALID remnant D2 describes) — every "
|
||||
"plan assertion below would be measuring a scan"
|
||||
)
|
||||
# Planner statistics for the plan pins — BOTH relations the tier's
|
||||
# SELECT joins, because the plan is a JOIN plan and a stale estimate on
|
||||
# either side flips it (see the module docstring). A freshly loaded
|
||||
# relation has none, and a row estimate of 0 makes
|
||||
# "seq scan + top-N sort" look free.
|
||||
session.execute(text(f"ANALYZE {RELATION}"))
|
||||
session.execute(text("ANALYZE documents"))
|
||||
session.commit()
|
||||
rows = session.execute(select(Chunk.id, Chunk.embedding)).all()
|
||||
# The SAME predicate the tier applies: a NULL-embedding row is in
|
||||
# neither the index nor the exact reference (a zero vector would score
|
||||
# as a real candidate and land in no result set).
|
||||
rows = session.execute(
|
||||
select(Chunk.id, Chunk.embedding).where(Chunk.embedding.is_not(None))
|
||||
).all()
|
||||
stored: dict[Any, list[float]] = {row[0]: row[1] or [] for row in rows}
|
||||
assert len(stored) >= preexisting + CORPUS_CHUNKS, (
|
||||
f"expected {CORPUS_CHUNKS} fixture rows on top of {preexisting}, "
|
||||
f"the relation holds {len(stored)}"
|
||||
assert len(stored) >= preexisting_embedded + CORPUS_CHUNKS, (
|
||||
f"expected {CORPUS_CHUNKS} fixture rows on top of "
|
||||
f"{preexisting_embedded} embedded rows, the relation holds {len(stored)}"
|
||||
)
|
||||
# The exact sets are the expensive half (30 × N dot products); the two
|
||||
# recall pins differ only in the ef_search they ask the index for.
|
||||
@@ -401,6 +464,9 @@ def hnsw_corpus() -> Iterator[dict[str, Any]]:
|
||||
"exact": [_exact_top(stored, query) for query in queries],
|
||||
"chunk_ids": set(stored),
|
||||
"plan_sql": _vector_statement_sql(),
|
||||
# The unforced-planner assertion (pin 1) is meaningful only on a
|
||||
# relation this module owns — see the determinism premise above.
|
||||
"pristine": preexisting == 0,
|
||||
}
|
||||
finally:
|
||||
session.rollback()
|
||||
@@ -437,14 +503,27 @@ def _force_index_scan(session: Session) -> None:
|
||||
caller's transaction instead of reaching the tier. Every caller resets it in
|
||||
a ``finally`` — a leaked ``off`` would slow every later test on this pooled
|
||||
connection — and the assertion beside it proves the lever took effect.
|
||||
|
||||
``enable_bitmapscan`` goes off with it because refusing sequential scans
|
||||
alone does not refuse a scan: measured on the 2026-10-18 flake, the planner
|
||||
answered ``enable_seqscan = off`` with
|
||||
``Sort → Nested Loop → Index Scan on documents → Bitmap Heap Scan on chunks
|
||||
(ix_chunks_document_id)`` — every chunk row read and sorted through the
|
||||
btree on ``document_id``, the very full read these pins exist to rule out,
|
||||
wearing an index's name. With both levers off the only access path left for
|
||||
``chunks`` that can serve the ORDER BY is the HNSW graph; with the index
|
||||
dropped the same arm reports ``False`` (verified), so the guard cannot pass
|
||||
on a missing index.
|
||||
"""
|
||||
session.rollback()
|
||||
session.execute(text("SET enable_seqscan = off"))
|
||||
session.execute(text("SET enable_bitmapscan = off"))
|
||||
|
||||
|
||||
def _release_index_scan(session: Session) -> None:
|
||||
session.rollback()
|
||||
session.execute(text("RESET enable_seqscan"))
|
||||
session.execute(text("RESET enable_bitmapscan"))
|
||||
session.rollback()
|
||||
|
||||
|
||||
@@ -467,34 +546,66 @@ def test_plan_uses_the_hnsw_index(db: Session, hnsw_corpus: dict[str, Any]) -> N
|
||||
Explaining the captured statement is also what proves D1's "no query
|
||||
rewrite": the ``<=>`` the retriever already emits is exactly the operator
|
||||
``vector_cosine_ops`` serves.
|
||||
|
||||
Two arms, because the unforced one is a cost decision this file does not
|
||||
own alone (module docstring, "The determinism premise"): the DEFAULT plan is
|
||||
asserted while the relation holds nothing but the fixture corpus (a clean CI
|
||||
DB, and this module's own teardown), and the FORCED plan — sequential and
|
||||
bitmap scans refused, so nothing that reads every row can be chosen — is
|
||||
asserted always.
|
||||
"""
|
||||
nodes = _plan(db, hnsw_corpus["plan_sql"])
|
||||
assert _uses_index(nodes, INDEX_NAME), (
|
||||
f"{INDEX_NAME} is not in the plan — the incident is back: [{_describe(nodes)}]"
|
||||
)
|
||||
assert not _seq_scans(nodes, RELATION), (
|
||||
f"a sequential scan of {RELATION} survived the index: [{_describe(nodes)}]"
|
||||
)
|
||||
db.rollback()
|
||||
sql = hnsw_corpus["plan_sql"]
|
||||
if hnsw_corpus["pristine"]:
|
||||
nodes = _plan(db, sql)
|
||||
assert _uses_index(nodes, INDEX_NAME), (
|
||||
f"{INDEX_NAME} is not in the plan — the incident is back "
|
||||
f"(index validity: {_index_validity(db)}): [{_describe(nodes)}]"
|
||||
)
|
||||
assert not _seq_scans(nodes, RELATION), (
|
||||
f"a sequential scan of {RELATION} survived the index: [{_describe(nodes)}]"
|
||||
)
|
||||
_force_index_scan(db)
|
||||
try:
|
||||
forced = _plan(db, sql)
|
||||
assert _uses_index(forced, INDEX_NAME), (
|
||||
f"{INDEX_NAME} is not in the plan even with sequential and bitmap "
|
||||
f"scans refused (index validity: {_index_validity(db)}): "
|
||||
f"[{_describe(forced)}]"
|
||||
)
|
||||
assert not _seq_scans(forced, RELATION), (
|
||||
f"a sequential scan of {RELATION} survived the index: "
|
||||
f"[{_describe(forced)}]"
|
||||
)
|
||||
finally:
|
||||
_release_index_scan(db)
|
||||
db.rollback()
|
||||
|
||||
|
||||
def test_plan_without_the_index_seq_scans_the_relation(
|
||||
db: Session, hnsw_corpus: dict[str, Any]
|
||||
) -> None:
|
||||
"""Vacuity guard: the same statement, same session, same statistics, with
|
||||
index scans refused, MUST fall back to ``Seq Scan on chunks``.
|
||||
the index access paths refused, MUST fall back to ``Seq Scan on chunks``.
|
||||
|
||||
``SET LOCAL enable_indexscan = off`` rather than dropping the index: a
|
||||
``DROP INDEX CONCURRENTLY`` + rebuild would cost a second graph build and
|
||||
leave the corpus unindexed if the rebuild failed, while this GUC is
|
||||
transaction-scoped (it dies with the rollback) and removes exactly the
|
||||
access path under test. Without this test the plan pin would pass vacuously
|
||||
on any plan that merely mentions some index.
|
||||
``SET LOCAL enable_indexscan/indexonlyscan/bitmapscan = off`` rather than
|
||||
dropping the index: a ``DROP INDEX CONCURRENTLY`` + rebuild would cost a
|
||||
second graph build and leave the corpus unindexed if the rebuild failed,
|
||||
while these GUCs are transaction-scoped (they die with the rollback) and
|
||||
together remove every access path that reaches an index — including the
|
||||
bitmap path into ``ix_chunks_document_id``, which alone would let this guard
|
||||
answer with a disguised full read instead of the seq scan it asserts.
|
||||
Without this test the plan pin would pass vacuously on any plan that merely
|
||||
mentions some index.
|
||||
"""
|
||||
sql = hnsw_corpus["plan_sql"]
|
||||
db.rollback()
|
||||
db.execute(text("SET LOCAL enable_indexscan = off"))
|
||||
db.execute(text("SET LOCAL enable_indexonlyscan = off"))
|
||||
# Bitmap scans go too: they are the other way into ``ix_chunks_document_id``,
|
||||
# so leaving them on let this guard depend on the planner's mood instead of
|
||||
# on the access path it removes (the 2026-10-18 flake answer was a Bitmap
|
||||
# Heap Scan of chunks and no Seq Scan at all).
|
||||
db.execute(text("SET LOCAL enable_bitmapscan = off"))
|
||||
nodes = _plan(db, sql)
|
||||
assert _seq_scans(nodes, RELATION), (
|
||||
f"the vacuity guard itself did not seq-scan {RELATION}: [{_describe(nodes)}]"
|
||||
@@ -534,8 +645,9 @@ def test_top100_recall_at_the_shipped_ef_search(
|
||||
depth_nodes = _plan(db, hnsw_corpus["plan_sql"])
|
||||
assert _uses_index(depth_nodes, INDEX_NAME), (
|
||||
f"this recall run at ef_search={EF_PIN} did not use {INDEX_NAME} even "
|
||||
"with sequential scans refused — the pin would be measuring a scan "
|
||||
f"and calling it parity: [{_describe(depth_nodes)}]"
|
||||
"with sequential AND bitmap scans refused — the pin would be "
|
||||
"measuring a scan and calling it parity (index validity: "
|
||||
f"{_index_validity(db)}): [{_describe(depth_nodes)}]"
|
||||
)
|
||||
|
||||
overlaps: list[float] = []
|
||||
|
||||
Reference in New Issue
Block a user