afx-team / afx-team/hebb-mind

fix(storage): give background tasks their own connection + process-level write lock + explicit transactions

Offen
#36 0 Kommentare 0 Reaktionen 0 zugewiesene Personen Auf GitHub ansehen
area: reliability effort: hard
Vorherrschende Sprache
Python
Sterne
52
Forks
18
PR-Merge-Kennzahlen
Keine gemergten PRs in 30 T.

Beschreibung

## Context

Goal axis: **reliability (可靠)**. Under real multi-agent load the storage layer is the concurrency/atomicity ceiling. Every FastAPI handler and the background `SchedulerManager` (consolidation + forgetting) share **one** aiosqlite connection, and the `KnowledgeGraph` is a single in-process `nx.Graph`. This issue tracks the SQLite-correctness work that closes audit defect **C1**.

PR #24 already landed a large share of the original C1 remediation. This issue is deliberately **narrowed** to what is still open after that PR. The longer-term scale story (Postgres/pgvector, pluggable vector store) is a **separate** discussion — roadmap **D4** — not part of this issue.

## Already landed in PR #24 (verified, not in scope here)

- Process-wide write lock + explicit transactions in the SQLite store: `SQLiteMemoryStore._write_lock` (`src/hebb/storage/sqlite_store.py:68`); `_begin()` issues `BEGIN IMMEDIATE` (`src/hebb/storage/sqlite_store.py:75-83`); every multi-statement write (`create`, `update`, `delete`, `delete_expired`, `update_access`, `update_access_batch`, `update_embedding`, `update_expiry`, `update_expiry_batch`) wraps the body in `try: … commit() except BaseException: rollback()` (e.g. `src/hebb/storage/sqlite_store.py:117-153`, `247-262`, `544-578`). The audit's orphan-row scenario (write F2) and "commit another coroutine's half-finished transaction" (write F1) are addressed for the store's own writes.
- Unified KG lock: `KnowledgeGraph.lock` (`src/hebb/graph/knowledge_graph.py:37`). The previously-unsynchronised session consolidation graph write (audit 遗忘F4) is now under the lock — `src/hebb/agents/consolidation_agent.py:374-375` and `393-405`; standalone `_consolidate_one` under the same lock at `src/hebb/agents/consolidation_agent.py:637-646`; the forgetting sweep purges + saves under `self.knowledge_graph.lock` at `src/hebb/scheduler/manager.py:189-194`.
- Consolidation runs are serialised against the admin route via `SchedulerManager._consolidation_lock` (`src/hebb/scheduler/manager.py:50`, taken at `120`).
- Orphan reconciliation routine: `KnowledgeGraph.reconcile()` (`src/hebb/graph/knowledge_graph.py:285-324`).
- A first concurrency test exists: `test_concurrent_creates_do_not_interleave` (`tests/unit/test_audit_storage.py:90-106`).

## Current state — what remains (verified file:line)

- **Single shared connection is still the design.** `_create_sqlite()` opens one `aiosqlite` connection and hands the same object to both the `MemoryStore` and the `PartitionStore`; the in-code comment is explicit: *"Resilient pragmas on the single shared connection … Serialization itself is handled by the store's in-process write lock (INT-2), not by re-architecting to a pool."* — `src/hebb/storage/factory.py:46-57`. PR #24 added `busy_timeout=5000` + reaffirmed WAL (`factory.py:52-53`) but did not give background tasks their own connection. The same store/connection is shared by the FastAPI handlers and the `SchedulerManager` (constructed with the shared `memory_store`/`partition_store`/`knowledge_graph` — `src/hebb/scheduler/manager.py:32-50`).
- **Serialization is split across three independent locks, not one.** `SQLiteMemoryStore._write_lock` (`src/hebb/storage/sqlite_store.py:68`), `KnowledgeGraph.lock` (`src/hebb/graph/knowledge_graph.py:37`), and `SchedulerManager._consolidation_lock` (`src/hebb/scheduler/manager.py:50`) are distinct `asyncio.Lock` instances. A consolidation step that creates a memory then mutates the graph holds the write lock for the SQL insert and the KG lock for the graph write **as two separate critical sections** (`src/hebb/agents/consolidation_agent.py:362-375`) — the SQL row and its graph reference are not committed atomically, so a crash between them still produces a graph orphan (the very class `reconcile()` exists to clean up after).
- **No combined write/consolidate/forget stress test.** `tests/unit/test_audit_storage.py:90` only exercises concurrent `create`s; there is no test driving interactive writes, a consolidation batch, and a forgetting sweep concurrently and asserting zero orphan / zero lost rows.

## Proposed approach

1. **Give background tasks their own connection.** Have `create_stores()` provide a second connection (or a small dedicated handle) for `SchedulerManager` so consolidation/forgetting do not share the request connection. The eval harness already runs consolidation against an isolated workdir db (`eval/cli.py`, per-scenario isolated `hebb.db`) — reuse that "own connection" capability rather than inventing a new path. *Alternatively*, if a single connection is kept, route **all** writes (store + graph) through one process-level lock so the split-lock gap above is closed.
2. **Make multi-statement operations explicitly transactional with rollback.** Mostly done in the store; audit the remaining batch paths in `manager.py` (forgetting batches) and the consolidation agent so each logical unit either commits as a whole or rolls back.
3. **Bring every KG read-modify-write + `save()` under one lock with consistent discipline**, and ensure the SQL mutation and its corresponding graph mutation share a single critical section (or are otherwise crash-consistent) so an interrupted consolidation cannot leave a graph orphan.

## Acceptance criteria

- [ ] Background tasks (consolidation, forgetting) no longer share the FastAPI request connection — they use their own connection — **or** all writes (SQL + graph) are serialised behind one process-level lock.
- [ ] Every multi-statement operation across the store, scheduler, and consolidation agent is wrapped in an explicit `BEGIN IMMEDIATE … COMMIT` with `rollback` on failure.
- [ ] KG read-modify-write + `save()` is consistently guarded, and a SQL write plus its graph mutation form a single crash-consistent unit (no new orphan on an interrupted consolidation step).
- [ ] A concurrent write/consolidate/forget stress test produces **no orphan and no lost rows** (extends `tests/unit/test_audit_storage.py` beyond the create-only case at line 90).

## Scope / out of scope

- **In scope:** SQLite-correctness — connection ownership for background tasks, explicit transactions, and consistent KG lock discipline.
- **Out of scope:** The storage-backend scale strategy (SQLite single-connection ceiling vs Postgres/pgvector vs pluggable vector store) — that is roadmap **D4** and decides the *direction*, not the correctness fix here.

## References

- `reports/audit/core-system-audit-2026-06-07.md` — defect **C1** (单一共享 sqlite 连接 + 无锁共享 graph) and its 统一修复方向.
- `reports/audit/newuser-experience-audit-2026-06-08.md`
- `reports/design/capability-gap-roadmap-2026-06-11.md` — **H4** (this issue) and **D4** (storage-backend strategy).

Filed from the capability-gap roadmap (reports/design/capability-gap-roadmap-2026-06-11.md).

Beitragsleitfaden

Beitragsleitfaden öffnen

Bewertung

Dieses Issue wurde noch nicht bewertet.

Neue Issues direkt in Ihr Postfach

Eine kurze Übersicht über anfängerfreundliche GitHub-Issues.