afx-team / afx-team/hebb-mind

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

Đang mở
#36 0 bình luận 0 reaction 0 người được giao Xem trên GitHub
area: reliability effort: hard
Ngôn ngữ chính
Python
Star
52
Fork
18
Chỉ số merge pull request
Không có pull request nào được merge trong 30 ngày

Mô tả

## 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).

Hướng dẫn đóng góp

Mở hướng dẫn đóng góp

Đánh giá

Issue này chưa được đánh giá.

Nhận issue mới trong hộp thư của bạn

Bản tóm tắt ngắn những issue GitHub phù hợp với người mới.