refactor(storage): make SQLite the sole operational authority - #1994
Conversation
Astro-Han
left a comment
There was a problem hiding this comment.
Blocking (6 issues):
-
Pricing authority is never seeded on a fresh workspace, so
readPricingSnapshotthrows (sqlite-usage-store.ts:488-491). That breakscreateHostAiSdkBackend(execution-model-composition.ts:282, 413-417), desktopensureUsageReady(boot.ts:706), everyusage:*IPC handler, and the firstsendMessageon a new workspace (session-stream.ts:111).sqlite-usage-schema.ts:50-58creates the table but nothing inserts thesingleton=1row; the deletedimportLegacyUsageStateused to do this, andsqlite-automation-schema.ts:13shows the same pattern for its own authority. Fix:INSERT OR IGNORE INTO usage_pricing_authority(singleton, revision) VALUES (1, 0)in the migration or inload(). -
The new migrations break 14 legacy-schema simulation tests. Migration 20 (
session_messages,sqlite-session-metadata-schema.ts:783) and migration 8 (headless_task_run_events,sqlite-runtime-schema.ts:243) are plainCREATE TABLE. The rewind fixtures drop the tables added after their simulated version but not these two, so re-migration throwstable ... already existsinsqlite-session-metadata-store.test.ts(3),agent-graph-intent-claims.test.ts,agent-graph-supervisor-wakes.test.ts,sqlite-runtime-store.test.ts,sqlite-recovery-concurrency.test.ts,recovery-persistence-authority.test.ts(2), andsqlite-runtime-schema.test.ts(its mock must return version 8, not 7). Fix: addDROP TABLE session_messages/DROP TABLE headless_task_run_eventsto each rewind script. The migration chain is fine for real databases. -
usage-stores.test.ts:195-298still tests the deleted legacy telemetry import: it writestelemetry.jsonand expects an import that no longer exists. Delete or rewrite these 3 tests. -
claimed-agent-graph-root-admission.test.ts:31-39readssessions/.../turn-admissions/turn-next.json, a path that no longer exists. Drop the file read; thereadRootTurnAdmissionround-trip right above it already covers durability. -
SqliteShellRunStoredropped the ENOENT contract:shell-run-store.ts:176throws a plainError, whileagent-run-store.ts:588-590setscode = 'ENOENT'.ShellRunProcessManager.isNotFoundError(shell-run-manager.ts:1904-1906) only maps ENOENT, so unknown background-task refs surface raw storage errors, echo the id back (non-disclosure lost), andruntime-resource-coordinatorclassifies the miss asinternal_failureinstead of not-found. Fix: seterror.code = 'ENOENT'on the miss. -
session-health-notice.spec.ts:53clicks a legacy stale session the fixture no longer seeds (onlystaleFakeSessionandhealthySessionremain), soe2e_shard(2/2)times out. No other spec references the removed scenario. Fix: update the spec.
Needs a decision, not a fix:
- Upgrade drops all existing session message history. Migration 20 creates an empty
session_messageswith no backfill,readMessages(session-store.ts:552-559) reads only SQLite, and nosession.jsonlreferences remain anywhere. Sessions keep their titles but open to empty threads. The PR body says this is intentional; confirm the irreversible loss of conversation history is accepted and documented. validateOperationalStateBackupnow only runsPRAGMA quick_checkplus version rows (operational-state-backup.ts). It no longer verifies SQLite schema/content or that artifact metadata matches payload bytes, so a backup with missing or truncated payloads validates successfully. The fsync discipline andchmod 0600on the DB snapshot were also dropped.- The legacy credential migration is removed with no fallback. It shipped about six weeks ago; workspaces that never converted
credentials.jsonnow fail closed on every read, while README/SECURITY still promise the migration. - Coverage regressions on code that still ships (restore tests or state the deferral): checkpoint event-projection preserve/repair (7 tests,
agent-run-store.ts:366-452); the fail-closed bundle/backup guards (symlink, path escape; ~39 tests cut to 1 each); workflow and interaction stores cut to smoke tests (write-queue races, concurrent winner, cron validation); root-turn admission semantics and catalog keyset pagination (zero coverage);lastMessagePreviewForMessages.
Not this PR:
- The
test_headlessdeadline-watchdog failure does not reproduce locally (1302/1302 pass) andtask-agent-controllerhas no diff. The headless/CLI domain is a clean backend swap;provider-auth-proxy.tsis untouched.
Gate: FAIL. Items 1-6 land before merge; the decision list needs explicit answers.
The migration created usage_pricing_authority but never inserted the singleton row; the deleted importLegacyUsageState used to do it. Every fresh workspace then threw PricingValidationError from readPricingSnapshot, breaking createHostAiSdkBackend, ensureUsageReady, and the first send. Mirror the automation authority seeding (INSERT OR IGNORE revision 0).
The SQLite not-found error was a plain Error, so callers gated on isNotFoundError (code ENOENT) surfaced the raw storage message and echoed the id back. Match agent-run-store's ENOENT-coded error.
…d 20 Migration 20 (session_messages) and migration 8 (headless_task_run_events) are plain CREATE TABLE, so rewind fixtures that rebuild an old version from a current database must drop them; the runtime schema mock also has to report version 8 as current. Fixes the 14 'table already exists' failures in the legacy-schema simulation suite.
usage-stores and the desktop usage-ipc test still seeded telemetry.json and asserted the deleted legacy import; the claimed-graph admission test read a turn-admissions file the SQLite store never writes. Remove the assertions, keep the surviving IPC serialization coverage.
The old mechanism blocked the file store by writing a file at <root>/sessions; the SQLite store ignores it. Fail the first durable ShellRun create through the wrapper store instead, keeping the slot-release assertions.
The composition startup-failure test corrupted task-events.jsonl, which the legacy import read; no JSONL path remains. Reject an invalid host epoch in beginHostEpoch instead: it runs after the long-term memory store is opened, so the fail-closed cleanup assertion still holds.
…ture The stale-sessions fixture no longer seeds the legacy Claude session the spec clicked. Switch the switching assertion to the healthy session (notice must hide) and back to the stale fake session for the settings click-through; the deleted-connection notice variant stays covered by deriveSessionHealthNotice unit tests.
|
@Astro-Han 已处理本轮 review:6 个 blocking 项全部落地;另外补强 operational backup 的 integrity/foreign-key/schema/content 校验、Artifact payload 对账、0600 与 fsync,并在 README/SECURITY/PR body 明确接受会话历史不回填、旧 safeStorage 凭据需重新登录的升级边界。删减的旧兼容/切换覆盖明确作为后续 focused coverage 延后,不恢复已删除的 file-store contract。最新提交:713e2a05。 |
…rage # Conflicts: # packages/runtime-host/src/__tests__/fixtures/execution-host-suite.ts # packages/storage/src/__tests__/execution-stores.test.ts # packages/storage/src/agent-run-store.ts
All six blocking findings are fixed. The decision items are explicitly documented in the PR body and repository docs, backup validation/durability was strengthened, and the maintainer approved direct merge.
…atabase (#2016) * refactor(storage): make the project catalog part of the operational database The project catalog decides how every session is organized, yet it stayed in its own `projects.json` when the rest of the File stores were retired. That left the only copy of every project name, relink alias and archive state outside `createOperationalStateBackup`, which captures `runtime.sqlite` plus the Artifact tree and nothing else — a restore silently dropped all of it, and no test could notice because the state was not in the database the validator checks. Schema 21 adds `projects`, `project_locations` and `project_aliases`, keyed by the random project id the renderer already contracts on, with `identity` as a unique index rather than the primary key so `data-project-id` keeps carrying an opaque id instead of a filesystem path. `session_metadata` gains a `project_id` column lifted out of `payload_json`, which makes grouping by project a SQL query for the first time; the column stays two-valued because `json_type` already distinguishes "no project" from "never resolved" for the one caller that needs it. Only persistence moved. Read-modify-write under a serial queue, whole-catalog validation on every write and each method's semantics are untouched, so all 17 catalog contract tests carry over unchanged. `projects.json` is imported once and renamed rather than deleted, so a failed upgrade stays inspectable; this is not legacy-format support but recovery of state this refactor would otherwise strand. The legacy-schema rewind fixtures are brought back in step with the migration list: #1994 left six `DROP TABLE` statements duplicated inside a single `exec`, which failed with `no such table` on the second one, and the new tables need their own teardown. Backup validation failures now carry their cause in the message — with integrity, foreign-key, ~60 required tables and Artifact reconciliation behind one error, a bare "is invalid" tells an operator nothing once `cause` is dropped from a log. * fix: resolve project membership for sessions that never had it `CreateSessionInput.projectId` is documented as three-valued — an id, an explicit `null` for "no project", and absent meaning "main resolves it automatically" (packages/core/src/runtime-inputs.ts:28-30). #1994 deleted the code that did that resolving along with the File stores, so the third state became permanent: any session whose project was never decided could no longer acquire one, and the sidebar's "group by project" view collapsed every such session into 未归属项目. The e2e case that renames a project failed deterministically as a result, because the ungrouped bucket has no project actions to click. Resolution is restored, scoped by SQL to the sessions that still need it instead of walking every header, and skipping the explicit `null` so a user who detached a session keeps that choice. A session whose directory is gone still resolves — a session outlives the folder it ran in — while a session that fails to resolve is left alone and retried next start rather than frozen into a wrong group. Verified against the failure this fixes: e2e/sidebar-navigation.spec.ts goes from a reproducible 30s timeout to 7/7, with the fixture untouched — the 66 seeded sessions now find their project from `cwd` on their own. * fix(storage): make project resolution atomic, stable and correctly scoped The follow-up review of the SQLite project-authority change found that the rebuilt catalog was correct in structure but wrong in three ways that only show up on a real upgrade, plus one path that never had a consumer. Backfill input. `listSessionsWithUnresolvedProject` returned only id and cwd, so every project was created with "now" as its timestamp and `preferredPath` fell out of session-id order rather than real activity. It also returned subagent sessions, whose disposable Git worktrees then became — and outranked — the user's own project locations. Both are one query: carry the session's last activity and exclude rows that have a subagent parent. Sessions are grouped by directory before resolution so an upgrade pays one Git probe per project instead of one per session. Identity. A directory that no longer exists was canonicalized with `normalize` alone, so on macOS the same folder resolved to `/var/...` after deletion and `/private/var/...` before it, splitting one project in two. The nearest surviving ancestor is now resolved with `realpath` and the missing segments re-appended. Concurrency. Every mutation rewrites the whole catalog, and the read and the write sat in separate transactions, so a second window's rename or archive was replayed away with no error. The synchronous mutations now read, change and rewrite inside one `BEGIN IMMEDIATE`; `select` and `relink` await the filesystem mid-change and re-derive their commit from the state under the lock. Dead weight. `session_metadata.project_id` and its index had no reader — three-valued membership can only be answered by `json_type` on the payload — so the column, the recency index and the unused `databaseLease` dependency are gone rather than kept as a second source of truth. Also: a failed `projects.json` import no longer takes the read path down with it, the set-aside file is timestamped so a second attempt cannot overwrite the first, the catalog releases its database lease, and project resolution runs after session recovery instead of ahead of it. Tests cover what the reviews found untested: the backup/restore round trip that is this change's whole premise, the legacy import in both its success and failure branches, identity stability across deletion, recency order, subagent exclusion, unresolvable directories, and the concurrent lost update. Each was confirmed to fail against the unfixed code. The e2e fixture resolves projects while seeding, so sidebar tests assert on a settled state instead of racing the startup resolver. * fix(storage): fence project assignment and relink against concurrent decisions Two reviews of the previous commit found the same thing from different angles: its concurrency guarantees were asserted in comments but not implemented. The backfill claimed a user detaching a session mid-resolution would win, but read the header and wrote it in two separate transactions, so a detach landing between them was overwritten back to a resolved project. `updateHeaderVersioned` already exists for exactly this; the listing now carries each row's metadata revision and the assignment is fenced by it. The unreliable extra read is gone rather than kept alongside the fence — a version conflict simply means someone decided first, and a session that is still unresolved is retried next start. `relink` re-derived its merge from the catalog as it stood at commit time. That kept the catalog self-consistent while leaving it inconsistent with the world: `beforeCommit` reassigns the sessions of the project being merged away, and those writes cannot be re-derived. If the merge target moved while the callback ran, the sessions had already been handed to the wrong owner. The commit now refuses when the conflict it would merge is not the one the callback was shown. Relink was already retryable — a throwing callback leaves the catalog untouched — so this hands the decision back instead of committing a half-true one. A historical working directory whose ancestor was replaced by a plain file raises ENOTDIR, not ENOENT, and was left permanently unresolved even though walking one level further up canonicalizes it exactly like a deleted directory. Both mean "cannot reach this path"; they are now handled together. Finally, `mutate` made `withQueue` redundant for every fully synchronous mutation: the SQLite write lock does that job now. The queue is kept only where it still earns its place — `select` and `relink`, which await the filesystem or a caller's callback mid-change. Removing it elsewhere deletes five `Failed to …` branches that `mutate` had already made unreachable. Each new test was confirmed to fail against the unfixed code.
- conversation-copy.test.ts: keep canonicalToolArgsHash on subpath, take upstream's createWorkspaceRuntimeStore import - workspace-version-authority-persistence.test.ts: drop dead createRuntimeEventStore import (upstream refactor), keep subpath canonicalToolArgsHash - execution-stores.test.ts: deleted upstream (apache#1994) — PR's import-migration edit is moot, take deletion
…be first, marker/subagent fail-closed, resume gate, observable results Addresses Astro-Han's review (P1 + P2s): - **P1 per-file atomicity**: the decoded header AND the post-create header patch are now validated through normalizeSessionHeader BEFORE any store write. Previously updateHeader (the third of three transactions) could throw after create+append committed, leaving a permanent partial session that later probes would report as skipped forever. - **P2 marker laundering**: a session_transcript marker file with no backing SQLite row (restored backup, copied sessions/, reset DB) now fails closed instead of being fabricated into a fake session — matching the pre-apache#1994 reader's contract. - **P2 legacy field loss**: decodeLegacySessionHeader preserves subagentParent/Runtime/Spawn/Workspace, thinkingLevel, lastReadMessageId. Legacy subagent children route through createSubagent (parent lineage kept); an incomplete spawn identity fails the file instead of flattening the child into a top-level session. - **P2 resume gate**: readHeaderSnapshot/readMessagesSnapshot now await the lazy import, so 'maka --resume <legacy-id>' no longer misses pre-cutover sessions on the first post-upgrade run. - **P2 observability**: ensureLegacyImported retains the result and logs failures/imported counts instead of swallowing them; LegacySessionImportResult now splits skipped into existing vs collision. - **P2 steady-state cost**: the idempotency probe now runs BEFORE the file read, so every launch skips known ids without touching their transcripts. - **P2 torn tail**: an incomplete final line (interrupted append) is skipped like the pre-apache#1994 strict reader, instead of failing the whole file. Tests: 11/11 in legacy-session-import (added lazy-list, resume, torn-tail, marker, subagent fail-closed, field-preservation cases); session-store + sqlite-session-metadata-store + foreign-session-store 70/70; full storage suite 702 pass, 2 pre-existing env failures (dugite git binary + root tsconfig load) verified unrelated via stash.
…n store API Addresses apache#2263 review round 3: collapse the importer's probe -> create -> append -> update choreography (three transactions, constant fingerprint, fidelity patch, in-memory latch, resume gate) into one store-level importSession primitive. - sqlite-session-metadata-store: importSession(header, messages, projection) writes the header row (with historical timestamps/flags) and all messages in one transaction. Idempotent by primary key (INSERT OR IGNORE), so concurrent first launches converge on one winner with no create claims; tombstoned ids are never resurrected; a failure mid-transaction rolls back, so a partial session can never persist (closes the crash-window P1). - legacy-session-import: read -> decode -> normalizeSessionHeader (pre-write validation) -> one importSession call. Subagent children now import under their own legacy id with lineage preserved instead of a fresh UUID (fixes the phantom-session P1). Torn-tail tolerance tightened to the pre-apache#1994 strict-reader semantics: only a final line of a file with no trailing newline whose parse failure is an unclosed bracket is skipped; truncated lines ending in a newline and garbage tails fail the file. - session-store: memoized import latch moves into ensureReady(), which every public method already awaits, so desktop/CLI/headless/--resume are all covered with zero per-caller wiring; appendMessages and closeAfterReady now await ensureReady() (pre-existing gaps). Import diagnostics are kept observable through the existing console allow-list. - tests: payload pins (header model/status, deepEqual messages[0]), concurrent double-import, id collision, header-only, absent sessions/, empty file, garbage tail, truncated-with-newline, whole-run failure containment, and subagent legacy-id round-trip; 19/19 legacy import tests, full storage suite 713 pass (1 pre-existing dugite-binary env failure).
) (#2263) * fix(storage): import legacy JSONL session transcripts into SQLite (#2260) After the JSONL->SQLite cutover (#1994, #2029), sessions created before the switch stayed on disk as sessions/<id>/session.jsonl but never appeared in the UI: the new storage layer only reads SQLite and there was no migration path (issue #2260). Add a one-time importer (importLegacySessionsOnce) that scans the legacy sessions directory, decodes each schemaVersion:1 transcript with the pre-#1994 compatibility rules (backend remapping, missing-field defaults), creates the session under its original id via the idempotent createStableSession path, appends the decoded messages, and restores the original lifecycle timestamps and flags. Design: - Idempotency key is the session id itself (probeStableSessionCreate), so re-runs and concurrent first launches converge without duplicates. - Per-file atomicity: a transcript imports fully or is skipped and reported; corrupt records are never laundered into the authoritative store. Failures never block startup or other files. - Legacy files are retained as migration evidence. - Wired into createSessionStore: list/listCatalogPage/listHeaders await the lazy import so upgraded installs see their pre-cutover sessions. * fix(storage): drop console diagnostic from legacy import wiring The import result logging used console.error, which the repository check-console audit rejects for new call sites. Remove the log entirely and harden the lazy import to swallow unexpected errors (best-effort semantics): a legacy-import failure must never block session listing. * fix(storage): address #2260 review — validate before write, probe first, marker/subagent fail-closed, resume gate, observable results Addresses Astro-Han's review (P1 + P2s): - **P1 per-file atomicity**: the decoded header AND the post-create header patch are now validated through normalizeSessionHeader BEFORE any store write. Previously updateHeader (the third of three transactions) could throw after create+append committed, leaving a permanent partial session that later probes would report as skipped forever. - **P2 marker laundering**: a session_transcript marker file with no backing SQLite row (restored backup, copied sessions/, reset DB) now fails closed instead of being fabricated into a fake session — matching the pre-#1994 reader's contract. - **P2 legacy field loss**: decodeLegacySessionHeader preserves subagentParent/Runtime/Spawn/Workspace, thinkingLevel, lastReadMessageId. Legacy subagent children route through createSubagent (parent lineage kept); an incomplete spawn identity fails the file instead of flattening the child into a top-level session. - **P2 resume gate**: readHeaderSnapshot/readMessagesSnapshot now await the lazy import, so 'maka --resume <legacy-id>' no longer misses pre-cutover sessions on the first post-upgrade run. - **P2 observability**: ensureLegacyImported retains the result and logs failures/imported counts instead of swallowing them; LegacySessionImportResult now splits skipped into existing vs collision. - **P2 steady-state cost**: the idempotency probe now runs BEFORE the file read, so every launch skips known ids without touching their transcripts. - **P2 torn tail**: an incomplete final line (interrupted append) is skipped like the pre-#1994 strict reader, instead of failing the whole file. Tests: 11/11 in legacy-session-import (added lazy-list, resume, torn-tail, marker, subagent fail-closed, field-preservation cases); session-store + sqlite-session-metadata-store + foreign-session-store 70/70; full storage suite 702 pass, 2 pre-existing env failures (dugite git binary + root tsconfig load) verified unrelated via stash. * chore: allow-list session-store.ts for legacy import diagnostics check-console.mjs flagged the new console.error/warn/info sites in session-store.ts (legacy JSONL import outcome diagnostics) as unlisted. Same pattern as the existing automation-store.ts allow-list entry — best-effort import diagnostics, no credentials or provider payloads. * chore: retry CI — test_headless 'settles background child sessions at the task-run deadline' flaked on the previous run (identical headless code passed two runs ago; local 30/30 green; no headless files touched by this PR) * fix(storage): refactor legacy session import onto a single-transaction store API Addresses #2263 review round 3: collapse the importer's probe -> create -> append -> update choreography (three transactions, constant fingerprint, fidelity patch, in-memory latch, resume gate) into one store-level importSession primitive. - sqlite-session-metadata-store: importSession(header, messages, projection) writes the header row (with historical timestamps/flags) and all messages in one transaction. Idempotent by primary key (INSERT OR IGNORE), so concurrent first launches converge on one winner with no create claims; tombstoned ids are never resurrected; a failure mid-transaction rolls back, so a partial session can never persist (closes the crash-window P1). - legacy-session-import: read -> decode -> normalizeSessionHeader (pre-write validation) -> one importSession call. Subagent children now import under their own legacy id with lineage preserved instead of a fresh UUID (fixes the phantom-session P1). Torn-tail tolerance tightened to the pre-#1994 strict-reader semantics: only a final line of a file with no trailing newline whose parse failure is an unclosed bracket is skipped; truncated lines ending in a newline and garbage tails fail the file. - session-store: memoized import latch moves into ensureReady(), which every public method already awaits, so desktop/CLI/headless/--resume are all covered with zero per-caller wiring; appendMessages and closeAfterReady now await ensureReady() (pre-existing gaps). Import diagnostics are kept observable through the existing console allow-list. - tests: payload pins (header model/status, deepEqual messages[0]), concurrent double-import, id collision, header-only, absent sessions/, empty file, garbage tail, truncated-with-newline, whole-run failure containment, and subagent legacy-id round-trip; 19/19 legacy import tests, full storage suite 713 pass (1 pre-existing dugite-binary env failure). * fix(storage): probe legacy session ids before reading transcripts Restores the probe-before-read steady-state cost from review round 2 on the single-transaction design: the importer now asks the store whether a session id already exists (live or tombstoned) before opening or parsing its file, so every launch of an upgraded install pays a directory listing plus per-id SQLite existence checks. importSession remains the idempotency authority — a race between the probe and the write still converges on one winner via the primary key. Adds hasSession to the store surface and a test that corrupts the on-disk transcript between runs to pin that a skipped id is never re-read. --------- Co-authored-by: cat0825 <cat0825@users.noreply.github.com>
Summary
runtime.sqlitethe only operational authority for Sessions, execution ledgers, workflows, usage, Artifacts metadata, Automations, Daily Review, and Headless TaskRunsENOENT, and align the health-notice E2E with the surviving fixtureCompatibility decisions
Backward compatibility and old File/JSONL data migration are intentionally not retained.
session_messages; affected sessions can retain metadata/title but open with an empty thread. This irreversible upgrade boundary is accepted and documented in the README.safeStoragecredential/token files are not imported.credentials.jsonis the sole authority and affected users must re-authenticate; README and SECURITY now state this explicitly.Verification
npm --workspace @maka/storage run typecheckgit diff --checkNo additional local test suite was run for the final review pass; CI is the merge gate.