Repository navigation
perf(storage): artifact metadata rewrites the full record table on every mutation (O(M×N) during startup) #4037
Description
Problem
Every artifact metadata mutation rewrites the entire artifact_records table, so a store with N records pays O(N) JSON + SQL + hashing work per mutation — and startup performs M serialized mutations (session-retirement purges, recovery adoptions), making cold start O(M×N).
Mechanism in packages/storage/src/artifact-store.ts + sqlite-artifact-metadata.ts:
- Every mutation begins with
reloadForMutationUnlocked()→metadataRepository.readAll():SELECT record_json FROM artifact_records+decodeArtifactRecordJsons()over all N rows. - Every mutation ends with
metadataRepository.replaceAll(records):DELETE FROM artifact_records, then re-INSERT all N rows, each with a freshJSON.stringify(record)and a fresh sha256artifactIdentityKey(record.id).
Measured impact
On a real store with ~11.7k artifact records and hundreds of accumulated Sessions (details in this #4027 comment), a windowed CPU profile of Runtime Host cold start shows the first ~90 s of the ~130 s total saturated by exactly this churn: ~34% of samples in the per-record INSERT, ~11% in the DELETE, ~6.5% in JSON decode of the full read-back, ~5.4% in the SELECT ... all(), and ~5.6% in sha256 (identity keys are re-hashed for every record on every rewrite). After ~105 s the Host is idle; the metadata phase is the residual cold-start bottleneck.
Steady state pays the same O(N) cost on every single-artifact mutation (create / purge / adopt), just without the M multiplier.
Proposed direction
Change-tracked write-back in replaceAll's place: within the same single write transaction, apply only the delta the mutation actually made — targeted INSERT ... ON CONFLICT(storage_key) DO UPDATE / DELETE WHERE storage_key IN (...) — instead of deleting and re-inserting the full snapshot. Additionally, compute the sha256 artifactIdentityKey once per record (it is a pure function of the record id) rather than once per record per rewrite.
Combined with a batched multi-Session purge (separate proposal), this reduces a retirement batch of M Sessions from M full-table rewrites to one O(changed) commit.
Questions for maintainers
- Crash-atomicity contract: today
replaceAllpersists the post-mutation snapshot as one atomic transaction. Targeted writes would run inside the same single write transaction, so atomicity should be equivalent — is there any consumer that relies on the table being a full rewrite (e.g. for recovery or consistency checking) rather than just "the persisted state after transaction commit"? - Source of truth: the in-memory record map is authoritative during a mutation and the table is its persistence. Any objection to keeping that model with delta persistence, or is a deeper redesign (e.g. querying SQLite instead of holding the map) preferred?
- Is a prototype welcome once the direction is confirmed?
Related: #4027 (cold-start investigation), #4031 (bounded purge resolution; review thread identified the O(M×N) structure).
This issue was prepared with AI assistance (Kimi k3-256k), including profiling and analysis.
Activity
me2seeks commentedon Aug 27, 2026
Companion proposal: #4038 (batched multi-Session purge). Batching removes the M multiplier on metadata commits for the retirement path; this issue's change tracking removes the O(N) cost of each remaining commit.
me2seeks commentedon Aug 28, 2026
Heads-up: this proposal is likely superseded by the retirement direction in discussion #4030. That discussion has converged on retiring the Artifact authority entirely, and its stated cutover invariants already subsume this issue ("surviving metadata stores must use indexed row operations rather than
readAll/replaceAll"). If retirement proceeds on that timeline, a contract-level change to the currentArtifactStoremetadata layer is probably not worth the review investment. I'm repurposing this issue as a fallback stopgap option in case the retirement takes longer than expected — happy to close it if maintainers prefer to track everything under the retirement work.This comment was prepared with AI assistance (Kimi k3-256k).
perf(storage): reseal only the sessions a mutation touched
feat(storage): reclaim the prepared-request captures already on disk
test(storage): hold a create to the rows it is about
fix(runtime): let a copy outlive the capture it points at
fix(runtime-host): start the capture sweep once the store can be written
perf(storage): drop the snapshots a mutation invalidates instead of r…
perf(storage): reseal only the sessions a mutation touched
perf(storage): drop the snapshots a mutation invalidates instead of r…
perf(storage): seal a session's artifact snapshot when it is asked for
12 remaining items
fix(runtime-host): start the capture sweep once the store can be written
github-actions commentedon Sep 5, 2026
Astro-Han commentedon Sep 5, 2026
Reopening: #4716 landed only half of this issue.
The Mechanism section above describes two costs per mutation. #4716 replaced step 2 (
replaceAll→ change-trackedINSERT ... ON CONFLICT/DELETE), andartifactIdentityKeyis no longer re-hashed for unchanged rows. That part is done and should stay closed.Step 1 is untouched.
ArtifactStore.load()andreloadForMutationUnlocked()both still callmetadataRepository.readAll()— a fullSELECT record_jsonplusdecodeArtifactRecordJsons()over all N rows — on every read path and again before every mutation. In the profile above that is the ~6.5% decode + ~5.4% SELECT share, and unlike the write side it has no M multiplier to remove: it is paid per operation in steady state too.The open design question is Question 2 in the description, which was never answered: does the in-memory record map stay authoritative (and something has to define when it is invalidated), or does
ArtifactStorequery SQLite by session/id through the indexes and stop holding the full set? The second reading matches thereadAll/replaceAllretirement invariant from #4030. This issue is the tracker for that decision — #4030 is closed and lists #4037 as one of its four execution threads, so the "likely superseded" note above no longer applies.lhpqaq commentedon Sep 5, 2026
take
Astro-Han commentedon Sep 5, 2026
@lhpqaq thanks for taking this — two things that may save you a round trip.
Question 2 in the body has an answer already on record. #4030 states the cutover invariant as "surviving metadata stores must use indexed row operations rather than
readAll/replaceAll". That settles the direction: query SQLite by index, stop holding the full table in memory. The indexes are already there (artifact_idprimary key, plus(session_id, created_at, artifact_id)).One seam worth planning for before you start. Revision is computed from the whole session.
sessionSnapshot()(packages/storage/src/artifact-store.ts:851) filters, sorts, and hashes every record in the session, andgetInSessionandlistPageboth return it. So replacingreadAll()with a per-Session query, on its own, still decodes an entire Session to answer a single-record read — the N just changes what it counts. Either revision becomes a value maintained inside the metadata transaction, or its cost needs to be stated explicitly.A useful acceptance check: adding records to other Sessions must not change the work done by a read, and adding records to the target Session must not change the work done by a single-record read — including obtaining its revision.
perf(storage): persist transactional artifact Session revisions
test(storage): publish Session revision growth evidence
docs(storage): report bounded Session revision verification
test(storage): trim redundant artifact growth cases
test(storage): reduce artifact regression coverage
Merge upstream/main into codex/issue-4037-targeted-artifact-reads
lhpqaq commentedon Sep 6, 2026
@Astro-Han Thanks for the clarification. I've used indexed queries and transaction-maintained Session revisions, with tests covering both growth checks. The implementation and verification details are in #4874 — feedback welcome.
Merge upstream/main into codex/issue-4037-targeted-artifact-reads