kami
219e2c762e
New core:artifacts-store interface + infrastructure/artifacts-cas implementation: segment files + SQLite index, Blake3 hashing, group-commit fsync via flushBefore, recovery tail-scan, manual compactor, and oldest-first disk-cap evictor. Inference events now carry promptArtifactId / responseArtifactId; orchestrators put bytes before emitting. SqliteEventStore wraps its txn in artifactStore.flushBefore so segment data is fsynced before the event commit, making the crash window non-corrupting (TailScanner re-indexes orphan tail records on reopen). Compactor and evictor are mutually exclusive via maintenanceMutex. Step 9 (cloud sync) deferred to a later epic. See docs/reviews/2026-05-18-cas-steps-1-8-review.md for the final review.
63 lines
4.7 KiB
Markdown
63 lines
4.7 KiB
Markdown
# CAS Artifact Store — Steps 1–8 Final Review
|
||
|
||
**Plan:** `docs/plans/2026-05-17-cas-artifact-store.md`
|
||
**Date:** 2026-05-18
|
||
**Scope:** Steps 1–8 (step 9 cloud sync deferred to a later epic)
|
||
**Build status:** `:infrastructure:artifacts-cas:check` green, 21/21 tests, detekt + kover clean
|
||
|
||
## Summary
|
||
|
||
Content-Addressed Store added as a new core interface (`core:artifacts-store`) plus a SQLite+segment-file implementation (`infrastructure/artifacts-cas`). Inference events now carry `promptArtifactId` / `responseArtifactId`; orchestrators `put(...)` bytes before emitting events. `SqliteEventStore.append` wraps its txn in `artifactStore.flushBefore { ... }` so segment data is fsynced before the event commit. Recovery tail-scan re-indexes orphan tail records on reopen, making the crash window between artifact write and event commit non-corrupting. A manual compactor + oldest-first disk-cap evictor round out the lifecycle; both gated by a `maintenanceMutex` so they cannot race each other.
|
||
|
||
## CLAUDE.md invariants
|
||
|
||
All hold:
|
||
|
||
- Event log remains sole source of truth; segment + index are derivable via `TailScanner.recover()`.
|
||
- No new `EventPayload` subclasses introduced; `InferenceStartedEvent` / `InferenceCompletedEvent` already registered in `core/events/.../serialization/Serialization.kt`.
|
||
- No cross-core impl deps. `core:artifacts-store` depends only on `core:events` (for the `ArtifactId = TypeId` typealias).
|
||
- Apps → core → infrastructure DAG preserved.
|
||
- Polymorphic registration verified for the modified inference event classes.
|
||
|
||
## Findings
|
||
|
||
### MUST FIX
|
||
None.
|
||
|
||
### SHOULD FIX
|
||
|
||
1. **`CasArtifactStore.open()` discards the `TailScanReport`** (`infrastructure/artifacts-cas/.../CasArtifactStore.kt:87-91`). If recovery truncates a corrupt tail or re-indexes orphan records, the caller has no signal. Log at INFO when `truncatedAt != null` or `recoveredRecords > 0`.
|
||
|
||
2. **No end-to-end orchestrator→put→append→replay→get test.** `SessionOrchestratorIntegrationTest` uses `NoopArtifactStore` plus a `RecordingArtifactStore` to assert put-then-emit ordering, but nothing wires the real `CasArtifactStore` + `SqliteEventStore` together to verify the crash-window invariant in situ. Add one test: put via real CAS, append an inference event, reopen, replay, assert `get(responseArtifactId)` returns the original bytes.
|
||
|
||
3. **`SqliteEventStore.allEvents()` (new in this diff) lacks `withContext(Dispatchers.IO)`.** Called from `LivenessScanner` which already wraps the iteration, so currently safe — but for consistency the read methods on the store itself should dispatch internally.
|
||
|
||
4. **`ReplayOrchestrator.runInference` super path still calls `artifactStore.put`** for the prompt before checking strategy (`core/kernel/.../ReplayOrchestrator.kt:178`). For non-`SkipInference` strategies during replay this writes a new artifact for an event being re-derived, bloating the store with duplicates. Verify intent; if unintentional, gate `put` on a replay flag.
|
||
|
||
### NICE TO HAVE
|
||
|
||
- `SegmentWriter.append` does not fsync per-record; durability comes from `flushBefore`. Document this contract in `ArtifactStore.kt` KDoc.
|
||
- `Compactor.readLiveEntries` builds `liveHashHex` from `liveIds.value` (hex) and compares to `hash.toHex()` per entry. A `ByteArrayKey` wrapper would avoid per-entry hex allocation.
|
||
- `CasArtifactStore.evict` does `listDirectoryEntries` under `mutex` to determine `activeId`. `SegmentWriter` already tracks `currentSegmentId` — expose a getter and skip the filesystem call.
|
||
|
||
## Categories with no findings
|
||
|
||
- Polymorphic serialization registration
|
||
- Cross-core deps
|
||
- Resource hygiene (`AutoCloseable`, `.use { }` chains)
|
||
- Detekt antipatterns (`Thread.sleep`, bare try/catch, new `@Suppress`)
|
||
|
||
## Per-step inventory
|
||
|
||
| Step | Subject | Status |
|
||
|------|----------------------------------------------------------------------|--------|
|
||
| 1 | `ArtifactId` value class in `core:events` | reused existing `TypeId` typealias |
|
||
| 2 | `core:artifacts-store` interface module | done |
|
||
| 3 | `infrastructure/artifacts-cas` MVP (writer/reader/index/store) | done |
|
||
| 4 | DI wiring + `flushBefore` around SQLite commit | done |
|
||
| 5 | Event schema (`promptArtifactId` / `responseArtifactId`) | done |
|
||
| 6 | Recovery tail-scan | done |
|
||
| 7 | Compactor + LivenessScanner | done |
|
||
| 8 | Disk-cap eviction policy | done |
|
||
| 9 | Cloud sync | deferred (later epic) |
|