fix(storage): import legacy JSONL session transcripts into SQLite (#2260) - #2263
Conversation
…ache#2260) After the JSONL->SQLite cutover (apache#1994, apache#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 apache#2260). Add a one-time importer (importLegacySessionsOnce) that scans the legacy sessions directory, decodes each schemaVersion:1 transcript with the pre-apache#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.
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.
|
Thanks for this — the migration fills a real gap (#2260: pre-cutover sessions silently invisible), and I verified the decoder line-by-line against the actual pre-#1994 format (source of truth P1 — per-file atomicity is not delivered: three transactions, and the failure mode is a permanent partial session. P2 — marker-mode transcripts are laundered into fabricated sessions. Between #1373 and #1994 the writer stored line 1 as a P2 — legacy fields are silently dropped: subagent lineage, thinkingLevel, lastReadMessageId. The old P2 — the import gate covers only list paths; P2 — import outcomes are unobservable: the result is computed and discarded. The wiring does P2 — steady-state cost: the cheap probe runs after the expensive work, every launch. The latch is in-memory per store instance, so the import re-runs on every process start — every CLI invocation and desktop launch — and each file is fully read + JSON-parsed + strict-decoded before the probe that would skip it (legacy-session-import.ts:98-101). The old store budgeted reads (HEADER_BUDGET 8KB / MAX_HEADER_BYTES 1MB); the importer abandons all of it, with no size guard on whole-file P2 — test gaps (verified by mutation, not inference): (a) the lazy wiring has zero coverage — deleting P3 (optional): no fixture from actual old-writer output and no The decoder core is right and the fixtures are faithful — the fixes are mostly about validation timing, one marker check, one entry point, and pinning what's claimed. Happy to re-review once the P1 and the P2s are addressed (or explicitly deferred with reasons). |
…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.
|
All P1 and P2 items addressed on P1 — per-file atomicity: the decoded header AND the post-create patch are now validated through P2 — marker laundering: P2 — legacy field loss: P2 — resume gate: P2 — observability: P2 — steady-state cost: the probe now runs BEFORE the file read, so every launch of an upgraded install skips known ids without reading or parsing their transcripts. (Durable completion marker deferred — the probe-first order already avoids the per-launch file work the review flagged.) P2 — test gaps: added lazy-list wiring, resume-path, torn-tail, marker fail-closed, subagent fail-closed, and field-preservation tests (11/11 in the spec). Existing tests updated for the new result fields. Verification: |
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.
|
Additional fix after the first CI run: (desktop ) flagged the new sites in session-store.ts as unlisted — the observability change violated the repo's console audit. Allow-listed for legacy import diagnostics (same pattern as the existing entry; no credentials or provider payloads). 432d114. |
… 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)
|
CI is fully green on the retry (1464fae). The previous failure was confirmed flaky: identical headless code passed on the run before it and on this retry, the failing test ('settles background child sessions at the task-run deadline', a mock-timer deadline watchdog) passes 30/30 locally, and this PR touches no headless files. All checks now pass: test, test_workspaces (storage 719/719), test_headless, e2e ×2, typecheck, test_runtime_host, alignment_audit, changes, windows_baseline. |
|
Thanks — this pass closes most of the prior list, and I verified each item: validation now runs before any write (string/missing P1 — the subagent "full fidelity" branch cannot work and leaves a phantom session. The docstring advertises that files with complete spawn metadata ( P2 — torn-tail tolerance is broader than the pre-#1994 strict reader, silently dropping a final corrupt record. The new P2 — the crash window between the three transactions is still undocumented. The P1 core fix is real (all validation failures now happen before any write), but create → append → update remains three transactions; a process death or IO error between them still commits a partial session that the next run counts P2 — payload assertions still under-pin (verified by mutation, not inference). Forcing P2 — load-bearing scenarios still untested. Concurrent first launch (the documented "two processes converge on one winner" — the real topology is a CLI pre-check store racing the TUI/desktop store), id collision ( P3 (optional): The decoder core remains right and the fixtures are faithful — happy to re-review once the P1 is fixed and the P2s are handled or explicitly deferred. |
|
One more suggestion on the shape of the fix, since we're going another round anyway — a single-transaction store-level Today the importer is ~500 lines because The minimal shape: // one SQLite transaction: header row (with original timestamps/flags) +
// all messages in order. Idempotent by primary key (INSERT OR IGNORE /
// rowid probe inside the same tx), so concurrent first launches converge
// on one winner for free.
store.importSession(sessionId, header, messages): 'imported' | 'existing'With that primitive the importer becomes: read file → decode → validate (the pre-write validation you already added stays) → one call. What each current mechanism buys vs. what the single transaction gives for free:
What survives unchanged: the decoder + pre-write validation, marker fail-closed, torn-tail tightened to the legacy strict-reader semantics, and the subagent fail-closed (or full-fidelity using the created id). The result would be roughly 150 lines instead of 500, and most of the findings from both review rounds — phantom rows, crash windows, convergence — disappear rather than get patched. This is a suggestion, not a requirement — the current direction works if you prefer to land it incrementally — but given the review cycle, the single-transaction seam is the Occam-shaped fix and the P1 falls out of it naturally. |
|
Following up on the refactor suggestion — I dug through the current store code to check how much of the machinery is actually load-bearing, and the seams for a much smaller version already exist. The shape below would drop the importer from ~500 lines to roughly 250 and delete the whole classes of bugs from both review rounds by construction rather than by patching: 1.
2. 3. What structurally disappears: the P1 phantom session (header written directly under the legacy id — no 4. The permanent part (small): a durable completion marker in the DB ( We'd genuinely prefer this shape — it's less code, and the findings from both rounds mostly stop existing instead of needing repairs. Happy to review either way; just wanted to make the case clearly since the seams make it cheap. |
…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).
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.
|
Took the single-transaction route from your last two messages — the importer is now roughly a third of its former size and the P1 from round 2 disappears by construction. Store primitive (sqlite-session-metadata-store + session-store): Importer (legacy-session-import): read file → decode →
Deliberately not done (independent decisions, reasons):
Verification: Happy to fold in the durable marker if you'd rather have it anyway. |
|
Merged latest main into the branch to stay mergeable (#2240 moved connection-lock into the metadata layer and shipped schema v22; #2289/#2315/#2316 landed after the branch was cut). The import path was aligned to the new semantics: Local verification on the merged tree: |
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks — this round is exactly what we hoped for. The single-transaction refactor landed precisely on the seams we'd flagged: importSession writes header + messages + catalog projection in one BEGIN IMMEDIATE transaction with rollback on throw (the crash-window P1 is structurally gone), tryInsertHeader carries the real historical timestamps with zero Date.now() in the path, the PK gives concurrent convergence without claims or fingerprints, tombstoned ids are never resurrected, the ensureReady() memoized latch covers every entry point with zero per-caller wiring (and the pre-existing appendMessages/closeAfterReady gaps got fixed along the way), and the phantom path is gone — subagent children import under their own legacy id with lineage intact (no createSubagent, no randomUUID).
Verified independently across three passes, including mutation checks: removing the latch wiring now fails exactly the two dedicated tests (the round-1 "5 tests stay green" gap is closed), zeroing ts / forcing model:'default' / removing the probe / relaxing torn-tail / removing marker fail-closed each fail at least one test, and the torn-tail fixtures check out at the raw-byte level (trailing \n decides skip vs fail exactly as the strict semantics say). The marker shape matches the pre-#1994 createSessionTranscriptMarker byte-for-byte, and the connection-lock derivation matches the v22 backfill rule, so imported rows land in the same state as pre-upgrade rows regardless of migration order. 20/20 green at head (the "19/19" in the commit message is a stale count), storage suite clean (remaining failures reproduce identically at base — environment).
No blockers. Three small things worth doing as follow-ups, none of which need to hold this PR:
- Failed import runs are never retried for the process lifetime —
readyPromise ??= importLegacySessionsOnce()memoizes a failed run (session-store.ts:316-317), so a transient first-access failure (slow volume, antivirus lock) hides all legacy sessions for the rest of that process. Cheap fix: reset the promise in the catch so the next access retries; worth a test (firstlist()withsessions/unreadable → fix → secondlist()imports). - The connection-lock branch is implemented but unpinned — every fixture has
connectionLocked: true, soupdateCatalogProjectionSync(..., lockConnection)at sqlite-session-metadata-store.ts:1313 is dead in tests (deleting the call passes 20/20). One fixture with a user message and noconnectionLockedin the header, asserting the import locks it, would pin the behavior this PR explicitly claims. - Diagnostics aren't regression-pinned — deleting the
console.warn/info/errorblock passes 20/20, andcheck-console.mjsnever flags a stale allow-list entry. Capturingconsole.warnin the whole-run containment test (asserting the[legacy-session-import]prefix + failed id) would keep the feature's only failure signal observable.
Minor notes (also follow-up material): the torn-tail heuristic is a brace-depth counter, not exactly the pre-#1994 classifyJsonRecord parser — the comment claiming exact equivalence overstates (real transcripts never hit the divergence, but {this line is not valid json would be skipped where the old reader failed it); tombstone-collision import is untested despite the PR body claiming it; message_ts is write-only and unpinned; per-message modelId and subagent runtime/spawn/workspace round-trips aren't pinned; and there's no size guard on transcripts (a 200k-line file means one long transaction — parity with the legacy full read, so acceptable, just worth a documented cap).
Merging now — this properly closes #2260. Happy to review any of the follow-ups when they land.
Summary
Fixes #2260. After the JSONL→SQLite cutover (#1994) dropped the JSONL session tree (#2029), sessions created before the switch stayed on disk as
sessions/<id>/session.jsonlbut never appeared in the UI — the new storage layer only reads SQLite and there was no import path. Intact user data became silently invisible.This PR adds a one-time importer,
importLegacySessionsOnce, that migrates legacyschemaVersion: 1transcripts into SQLite under their original session ids.Design
Why not a schema migration. The existing
session_metadatamigration framework (sqlite-session-metadata-schema.ts) is a pure-SQL DDL array run in a transaction — it cannot read files. A data migration belongs in a storage-layer module, following the repository's existingimportLegacyCatalogOncepattern inproject-catalog.ts.Idempotency. The session id is the idempotency key. We probe with
probeStableSessionCreateand create through the stable path with a constant fingerprint, so re-runs (and concurrent first launches) converge on one winner and never duplicate messages.Per-file atomicity. A transcript imports completely (header + all messages) or is skipped and reported as failed. Corrupt records are never laundered into the new authoritative store as synthetic notes, and a malformed file never blocks startup or the import of other sessions.
Compatibility. The decoder mirrors the pre-#1994
decodeSessionHeaderrules (which were documented as "kept public for one-way importers" before #1994 deleted them): missingpermissionMode→ask,collaborationMode→agent,orchestrationMode→default, empty model→default, andclaude/pibackends remapped. Final strict validation is delegated to the metadata store's existingnormalizeSessionHeaderon create.Fidelity.
createStableSessionstamps now-based timestamps and default flags; the importer restores the originalcreatedAt/lastUsedAt/lastMessageAt/statusUpdatedAt,titleIsManual,isFlagged,isArchived,hasUnread, andconnectionLockedfrom the legacy header afterwards.Retention. Legacy files are not deleted, honoring the repository policy that legacy stores are kept as migration evidence.
Wiring.
createSessionStoregains a lazy, once-per-process import awaited bylist/listCatalogPage/listHeaders, so upgraded installs see their pre-cutover sessions on first launch. Desktop, headless, and CLI all share this factory.Verification
tsctypecheck: cleanbiome lint+biome format: cleanturn_statenormalization)list()surfaces it, and a second run reportsimported: 0, skipped: 1.Files
packages/storage/src/legacy-session-import.ts— new importer + legacy header decoderpackages/storage/src/session-store.ts— lazy import wiring increateSessionStorepackages/storage/src/index.ts— exportimportLegacySessionsOncepackages/storage/src/__tests__/legacy-session-import.test.ts— 5 test casespackages/storage/src/__tests__/fixtures/legacy-sessions/— sanitized transcript fixtures (normal / sparse-header / corrupt-line / no-turn-state)