Skip to content

fix(core): own startup index tasks and remove flaky test causes - #1967

Merged
phernandez merged 2 commits into
mainfrom
fix/1955-flaky-tests
Oct 29, 2026
Merged

phernandez merged 2 commits into
mainfrom
fix/1955-flaky-tests

Conversation

@phernandez

@phernandez phernandez commented Oct 29, 2026 •

Copy link
Copy Markdown
Member

Refs #1955. The teardown hang is still open there; see the last section.

This PR fixes the flakes reported in #1955 plus two the coordinator added: the linear-scaling tokenizer tests and the MCP container leak. Each section gives the cause, the evidence and the change. Verification is at the end.

1. vtable constructor failed: search_index in the watcher lifecycle test (test bug, real SQLite behavior)

Cause. The SQLite engine_factory fixture in test-int/conftest.py created search_index but not the vector storage. So the first embedding of the new note ran _ensure_vector_tables, which created search_vector_chunks, its indexes and the vec0 table while bm_search polled on another pooled connection. A schema change makes other connections reload the schema and reconnect the FTS5 table. A reconnect that overlaps the DDL fails with this error.

Evidence.

  • CI logs for jobs 123581698132 (main, ab6a059) and 123623036006 (refactor(mcp): port context and recent-activity tests to bm_context and bm_project #1962) both fail in the narwhal loop with this error.
  • I reproduced it locally on main with a DDL tracer, a pytest plugin that logs every DDL and search statement with timestamps. The failing SELECT COUNT(*) FROM search_index is interleaved with CREATE INDEX ... search_vector_chunks, issued from _ensure_vector_tables <- sync_entity_vectors_internal, about 1 ms earlier.
  • A standalone sqlite3 script gives the same vtable constructor failed: search_index. It commits schema changes from a second connection while the first prepares an FTS5 query.

Not a product bug. Production opens the database through get_or_create_db -> run_migrations -> init_search_index, which creates search_index, search_vector_chunks and search_vector_embeddings before any request. I confirmed this on a fresh database. The _ensure_vector_tables DDL that runs per repository instance afterwards uses IF NOT EXISTS. Re-running it leaves PRAGMA schema_version unchanged (10 -> 10), so it cannot trigger the reconnect.

Fix. The SQLite fixture now calls create_search_repository(...).init_search_index(), as the Postgres branch already did. After the fix, the trace shows no schema-changing DDL during the search loops.

The TimeoutError symptom (job 123284407567, #1953) times out in the same narwhal loop, right where the first embedding creates the vector tables. A watcher batch that fails is logged and dropped until the next change, so the same race on the indexer's write would leave narwhal unindexed. CI lost the loguru output ("I/O operation on closed file"), so this link is inferred, not shown.

2. Module-global _initial_index_tasks (product lifecycle fix)

Cause. Startup project indexes were fire-and-forget tasks held in a module-level set. Nothing owned them:

  • WatchCoordinator.stop() cancelled only the watcher.
  • The lifespan could close the database while they still ran.
  • The set outlived event loops. A _CapturedTask fake from tests/services/test_initialization.py was later gathered by the lifecycle test (TypeError).

Fix. initialize_file_indexing runs the startup indexes and watch_service.run() in one asyncio.TaskGroup. Cancelling the watcher now cancels and awaits the indexes. The global and the test's gather(*_initial_index_tasks) are gone.

Each index task still catches and logs its own Exception, so one failing project does not cancel the watcher. CancelledError still propagates. A new test, test_a_failed_startup_index_leaves_the_watcher_running, uses a project whose root is a regular file, so its real index fails, and asserts the watcher keeps running. The test fails if the handler re-raises.

The teardown hang (loop close stuck in _cancel_all_tasks) reproduced once locally in about 40 loaded runs, with the same stack as CI job 123279312418. It did not reproduce again in 25 more runs with a pending-task dump installed, so I cannot name the task that ignored cancellation. This change removes the startup indexes as unowned tasks that outlived the lifespan.

3. test_file_sqlite_concurrent_writes_commit_and_dispose (test design)

Cause. The failure is on Windows, main 5b1d843, run 40839542939, job 123332360458. It was not in the ab6a059 run. SQLite's busy handler polls with growing sleeps of up to 100 ms instead of queueing waiters. When 64 writers each ran 40 transactions back to back, the other pooled connections could pass the write lock between themselves for the whole run while one waiter slept through every free moment. The run took 19.97 s on Windows against about 3 s on Linux, and one waiter passed busy_timeout (10 s): database is locked on the first INSERT of a transaction.

Fix. Same 2560 writes, run in 40 rounds of 64 concurrent writers. A waiting connection now competes only with one round's work, well under a second, never with the rest of the run. Contention within each round is unchanged.

Note, not changed here. On Windows, connect_args={"timeout": 30.0} is overridden by the per-connection PRAGMA busy_timeout=10000, so the 30 s setting never takes effect.

4. Postgres refresh negative controls (deleted)

Cause. These controls assert that overlapping refreshes produce search_index_pkey. The upsert's ON CONFLICT absorbs any conflict it sees in its arbiter index; it waits for the other transaction, then updates. The pkey error happens only when two inserts pass the arbiter check in the same instant. The unit engine uses NullPool, so on a loaded runner one refresh could commit before the last one had deleted anything, and the round did not collide (job 123284407396).

Why deleted. I first replaced the sleep with an asyncio.Barrier between delete and insert. With every delete done before any insert, the saves control still missed 1 run in 5. Nothing outside the INSERT can force that instant. The tolerance is already pinned deterministically:

  • tests/services/test_search_refresh_atomicity.py: an injected lost race is logged and the previous projection kept; any other database error still raises.
  • tests/repository/test_search_refresh_race.py: which errors count as a lost race.

The positive concurrency tests still assert that overlapping refreshes never fail.

5. Linear-scaling tokenizer tests (robust measurement)

Cause. The tests compared wall-clock time (perf_counter), which includes time the test spends descheduled. On a loaded machine, the old version failed 8 of 10 runs with 24 CPU burners.

Fix. The tests now measure time.thread_time() (this thread's CPU only) with gc paused, best of 5, and keep the ×2.5 bound. There is no operation count to assert instead: the quadratic versions make the same calls and inspect the same characters, and only the bytes each slice copies grow, inside C.

  • Under the same load the new version passed 20 of 20 runs.
  • The quadratic implementations still fail it (×2.96 and ×3.3), checked by temporarily reverting them.

6. MCP container leak (product fix)

Cause. The lifespan called set_container() and never reset it. After an in-process Client(mcp) session ended, later code in the same process, such as picking the default search mode, read that session's config.

Fix. A new basic_memory.mcp.container.installed_container installs the container for the lifespan only and restores the previous one on exit, mirroring the API's installed_container. Regression test: test_mcp_lifespan_uninstalls_its_container_on_exit.

Verification

  • just fast-check: clean.
  • just test-int-sqlite, full suite: 720 passed, 14 skipped.
  • just test-int-postgres -k "watch_lifecycle or lifespan or mcp_read_cache or sqlite_lifecycle": passed.
  • just test-unit-sqlite and just test-unit-postgres on the touched modules: passed.
  • Coverage of the touched src: mcp/container.py 100%, services/initialization.py 100%. mcp/server.py misses only lines this PR did not change.
  • Repro loops: the lifecycle and SQLite contention tests passed 14 of 15 runs under 8 burners, with no vtable errors; the one failure was the teardown hang described in section 2. Then 25 of 25 under 2 burners. On main, 1 of about 30 traced or loaded runs hit the vtable error.

🤖 Generated with Claude Code

https://claude.ai/code/session_01CMYVL1x91B8kJxy7nn2GU8

- SQLite integration fixture now initializes search storage (FTS5 and
  vector tables) at database open, as production does. Lazy vector-table
  DDL during the first embedding raced concurrent searches and failed
  them with "vtable constructor failed: search_index".
- initialize_file_indexing runs startup indexes and the watcher in one
  TaskGroup, replacing the module-level _initial_index_tasks set. Shutdown
  cancels and awaits them; a failed index is logged and the watcher keeps
  running.
- MCP lifespan installs its container for the lifespan only and restores
  the previous one on exit, so a finished in-process session no longer
  leaves a stale config behind.
- SQLite contention test writes in rounds so a busy-handler waiter cannot
  be starved for the whole run.
- Drop the Postgres refresh negative controls: the pkey collision needs two
  upserts to pass the arbiter check in the same instant, which no barrier
  can force; the tolerance is covered deterministically elsewhere.
- Linear-scaling tokenizer tests measure thread CPU time with gc paused.

Closes #1955

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CMYVL1x91B8kJxy7nn2GU8
Signed-off-by: phernandez <paul@basicmachines.co>
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 29, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-29T19:42:24.745785Z c4af332 Manual request
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@phernandez

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3e475aa867

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread src/basic_memory/mcp/container.py Resolved
Overlapping lifespans that exit out of order restored an exited container
with the save/restore slot. Each lifespan now adds its container to a list
and removes that same entry on exit; the newest running one is current.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CMYVL1x91B8kJxy7nn2GU8
Signed-off-by: phernandez <paul@basicmachines.co>
@phernandez

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. What shall we delve into next?

Reviewed commit: c4af332b7f

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

@phernandez
phernandez merged commit 04026ad into main Oct 29, 2026
56 checks passed
@phernandez
phernandez deleted the fix/1955-flaky-tests branch October 29, 2026 20:07
phernandez added a commit that referenced this pull request Oct 29, 2026
## Why

`just release v1.0.0` pre-flight-checks for a `## v1.0.0` heading on
`main`, so the changelog has to land first.

## What changed

`CHANGELOG.md`: the Unreleased section becomes `## v1.0.0 (2026-10-29)`.
Its existing items are kept, and the 60 or so commits since #1871 are
added:

- **Breaking:**
- the MCP surface is only `bm_*`, with the old tools deleted (#1944,
#1947, #1959, #1962–#1964)
  - `project` is required (#1934)
  - the ChatGPT adapters moved to Cloud
  - `bm tool` mirrors the tools (#1943)
  - the POSIX verbs are rebuilt and `bm tail` is removed (#1960)
  - cloud sync/bisync are removed (#1953)
  - `bm schema` project resolution (#1959)
  - LiteLLM is an optional extra
- **Features:** text-file line ranges in `bm_read`; chat imports return
their file paths.
- **Bug fixes:** including the three found in the 1.0 live test (#1971,
#1972, #1974), with contributor credits for @Flynn-Kawolsky (#1957),
@SWSAmor (#1951) and @MbappeWU (#1948).
- **Security:** the CodeRabbit deep-scan hardening batch.

I checked the entries against PR bodies rather than titles, and
corrected #1954, #1948 and #1967 to describe what they actually fixed.

The date is today; change it if the tag lands tomorrow.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

https://claude.ai/code/session_01CMYVL1x91B8kJxy7nn2GU8

Signed-off-by: phernandez <paul@basicmachines.co>
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant