perf: bound what one model call costs to store - #4722
Conversation
462207f to
f65a47f
Compare
Astro-Han
changed the title
5b059f8 to
b80b903
Compare
Each dispatched provider request was serialized whole and written to the artifact store. The request is built from the conversation the run already holds, so every capture was another copy of the same messages, and each one grew with the conversation: one session here reached 772 MB of captures carrying 4.8 MB of distinct content. The bytes were the smaller cost. Captures were 87% of the artifact population, and every artifact write paid for the whole population, so the capture sink is what turned a growing conversation into quadratic write amplification. Nothing read them. The reader shipped with the capture in apache#1277 and was deleted by apache#2605; the producer stayed. What the panels and diagnostics actually read is the bounded observation on the canonical ModelCallAttempt, which is unchanged. The request is still serialized in memory to size and identify it, and is then dropped. `PreparedRequestMaterial` collapses into the observation it wrapped, and the tracker's per-step capture memo goes with it: its key was the digest, so it never saved the work it appeared to cache. Decoders stay. `captureArtifactId`, the `provider_request_captured` event and the `provider_request_capture` artifact source all still resolve, so attempts and sessions already on disk keep decoding and keep copying. Removing them would fail exactly the records this change is meant to stop producing more of. Tests that used the sink as a hook now use the dispatch gate, and the ones that used it to inspect the outgoing request assert against the provider request bodies instead — the stronger evidence of the two. Closes apache#4082 Generated-by: Claude Code
…s made from Every completed call stored a `PreparedRequestObservation`: up to 256 ordered segments, each with an index, a cacheable flag, a comparison mode, a sha256 digest, a byte count and a role. One reader existed, and it did one thing with all of it — `foldPromptComposition`, into four byte totals and a capped tool list. The other four fields per segment had no reader anywhere. So the fold moves to where the request is prepared, and the attempt carries its result. `PromptComposition` lives in core, next to the record that stores it, and the diagnostics types are now aliases of it rather than a second spelling kept in step by hand. What this stops doing per model call: serializing the entire request payload to hash it, hashing each of up to 256 segments, and writing that array into the run's event log. What it still answers is exactly what the panel and `/context` asked before. Attempts recorded before this still carry their segments, and folding them on read is the only way to say what those requests were made of, so that path stays. It is the same shape `readPromptCompositionEvent` already had for the generation before it. The 256-segment cap goes with the array. The fold's output was always the bound that mattered — four kinds and 64 named tools — and that constant now has one definition the producer and the decoder share. `hasRequestObservation` on the metering anchor becomes redundant once the compat fold happens inside it: it only ever meant "this anchor has no composition", which the composition itself now says. Closes apache#4082 Generated-by: Claude Code
…ation left Diagnostics declared its own segment, tool and composition types over the ones a ModelCallAttempt durably carries. Folding them onto the record left those as aliases with no consumer outside the package, which is two spellings of one fact kept in step by hand. Prepared-value normalization also tracked whether a value could be compared exactly. That fed the retired observation's per-segment comparison mode; the one caller left takes the normalized value and sizes it, so the flag was accumulated through every branch and read nowhere. Refs apache#4082 Generated-by: Claude Code
…'s buckets The snapshot validator spelled the four segment kinds twice and capped the tool list with a literal 64 beside the constant that sets it. Both now read from one list and one constant, so a change to the fold's buckets cannot leave the validator agreeing with a stale copy of itself. Also drops a sweep assertion that could not fail: the batch size is a module constant, so asserting it is a positive integer pinned nothing. Generated-by: Claude Code
`observe` was the seam the capture machinery hung on. With that gone it named nothing: two calls, one line, two call sites. Generated-by: Claude Code
Every load and every mutation rebuilt a map holding one sealed snapshot per session, and a snapshot's revision hashes every record in its session — so a store with 400 sessions sorted and hashed all 400 to answer a question about one. The map never earned that: each of the five readers reloads the whole store from the database first, so a kept snapshot never survived to be read. Sealing on the way out instead deletes the map, the two methods that maintained it, and the per-mutation bookkeeping that told them what changed. At 6,000 records across 400 sessions, same machine, same run: one listPage 13.45 to 11.54 ms, one create 38.32 to 37.42 ms. Refs apache#4037 Generated-by: Claude Code
b80b903 to
8c35544
Compare
The test stopped the Host as soon as `turn.start` returned, which only says the Turn was admitted -- which state the drain then found was left to how fast the machine was, and it asserts the drain records `cancelled`. It failed twice while this branch was verified with builds and benchmarks running beside it. It now waits for the question the scenario is about to ask before stopping, the same checkpoint its sibling test one screen below already uses. That is what makes the Turn active, which is what this test is about. Generated-by: Claude Code
The decoder and the projection validator each spelled the same four segment kinds in the same order, so a change to what a composition is made of could leave one of them agreeing with a stale copy of the other. The list moves to the owner of the shape and says there that its order is contract, not presentation. The two validators stay separate on purpose: a record that fails the decoder loses its usage and cost with it, while a derived projection row that fails is rebuilt from the ledger -- so the projection can afford to check the fold's ordering and byte conservation, and the durable record cannot. Generated-by: Claude Code
jackwener
left a comment
jackwener
left a comment
There was a problem hiding this comment.
Approving at exact head 2f69ee4bb3790f9c23cbe02eff4cd73139817846. No P0 or P1. One P2 that does not block, and it is about test coverage rather than the change itself.
Three independent reviews ran on this head — different model lineages, each sealing its conclusion before reading the others. All three reached approve. Two of them independently landed on the same coverage gap, from different directions, which is the finding worth your attention.
P2 — the 64-tool persistence bound is not pinned at the production seam
This is the contract the PR is built on, and the only test that crosses the real seam does not exercise it.
latest-context-commit.test.ts:157-161 is the one SessionManager → tracker → AgentRun/SQLite → reopen test. It runs with zero tools and asserts only tools.length <= 64. The 64+5 case in request-shape.test.ts:126-147 hits the pure helper, never the persisted path.
Reverse mutation makes the gap concrete: inserting a 65th named row after the helper's correct fold, without touching the helper, leaves 102/102 relevant Core/Runtime tests green — including the helper's own boundary test and the real send/reopen test. A production boundary probe fails immediately at decodeModelCallAttempt() with Invalid ModelCallAttempt schema.
The real-world consequence is the part that matters: the provider turn still succeeds, and provider-request-telemetry.ts:604-609 swallows the accounting failure. Usage, cost and latest-context silently do not persist. A registry with 65 or more tools is ordinary rather than exotic once MCP servers are in play, so this is reachable in normal use, and the failure is silent in both the product and the test suite.
Smallest fix: give the existing real send/reopen test 65 or 69 actual tools and assert 64 named, the exact remainder count, byte conservation, and a consistent warm/cold SQLite read-back.
Graded P2 rather than P3 because the failure is silent data loss on a common configuration while every related test stays green — not because it blocks this merge. It does not.
The current implementation is correct where it is tested
Run through the real SessionManager / AI SDK backend / tracker / AgentRun SQLite chain at 63, 64, 65 and 69 tools, all pass and read back consistently after restart: 63 → 63 with no remainder; 64 → 64 with no remainder; 65 → 64+1; 69 → 64+5. Named plus remainder bytes always equal tool_definitions. Records land at roughly 2,698–2,772 B. The over-limit semantics are a summary, not a rejection and not a silent drop — bytes stay accounted for.
The deletion is safe, including for data already on disk
The premise checks out in the history rather than only in the description. 151844286 (#2605) deleted packages/headless/src/provider-request-trace.ts (566 lines) with its 1,073-line test — that was the reader — while the producer changed by 22 lines and survived. 35998f234 (#1277) introduced the capture. No importer of the deleted helper remains.
The legacy handling deserves specific mention because it is easy to get wrong and this got it right. Keeping the provider_request_capture source policy is not a courtesy; it is load-bearing. The policy table ends in satisfies Record<ArtifactSource, ArtifactSourcePolicy>, which binds it to the enum: removing the value from ARTIFACT_SOURCES alone fails to compile with TS2353. And removing both would matter more than it first appears — artifact-metadata-codec.ts:120 throws invalidMetadataRecord for an unknown source, and decodeArtifactRecordJsons is fail-fast: one bad record aborts the whole batch rather than skipping it. Against the 379 capture records in the workspace measured in the description, that is the entire artifact metadata decode failing to open, not a few unreadable rows. Probed on this head: the source is still enumerated, canUserDeleteArtifact is true, visibility and shared-read are false — exactly what the comment claims.
Old requestObservation records still decode and fold, and old captureArtifactId values are still rewritten in conversation copy.
The snapshot change is safe for pagination
Dropping the sessionSnapshots map in favor of computing one on demand preserves the pagination contract, because revision is a pure function of content: sha256(JSON.stringify(records)) over records sorted by createdAt descending with id as tiebreak — a total order. Computed-on-read and cached therefore agree. The cost moves from every write to each read, which is the right direction here, and the read side gets cheaper anyway once captures stop being 87% of the population. The two halves are genuinely coupled, as the description says.
On the numbers
Recomputed from the tables: 26.3× / 58.1× / 102.9× per call, 29.5× / 59.3× / 102.9× per Session, 2.039 MB average per capture, 379/436 = 86.9%, 99.38% duplication, 772.7 + 3.3 = 776.0 MB, 379 + 57 = 436. Every one of them lands.
One qualifier on the shape argument, offered only for accuracy: "4× the turns costs 8× the bytes" holds for the pair it cites (50 → 200 turns is 7.93×), but 200 → 500 is 4.35× where a pure quadratic would be 6.25×. The "after" side is exactly linear in both intervals (3.95× and 2.51×). The central claim does not rest on that generalization — the flat 3,942 B per call across 40, 200 and 600-message conversations demonstrates it directly and far more cleanly.
Scope
test, package and windows_recovery are green on this head, and the merge tree against current main is clean. Reclaiming the captures already on disk is scoped to a separate change; this one stops producing them, as stated. This is a perf refactor, so the merge decision remains a human's.
简体中文
在 2f69ee4bb3790f9c23cbe02eff4cd73139817846 上批准。没有 P0/P1。 有一条不阻塞的 P2,而且它针对的是测试覆盖,不是这次改动本身。
这个 head 上跑了三次独立评审——不同模型谱系,各自在读到别人的结论之前先把自己的封存。三次都是批准。其中两次从不同方向独立命中了同一个覆盖缺口,那是值得你注意的发现。
P2:64 工具的持久化上界没有在生产接缝上被钉住
这正是这个 PR 立足的合同,而唯一穿过真实接缝的那条测试并没有触碰它。
latest-context-commit.test.ts:157-161 是唯一一条 SessionManager → tracker → AgentRun/SQLite → 重开 的测试。它跑的是 0 个工具,而且只断言 tools.length <= 64。request-shape.test.ts:126-147 里那个 64+5 打的是纯 helper,从不经过持久化路径。
反向变异让这个缺口变得具体:在 helper 完成正确折叠之后额外塞入第 65 个 named row、helper 本身不动,相关 Core/Runtime 测试 102/102 仍然全绿——包括 helper 自己的边界测试和真实的 send/reopen 测试。而生产边界探针立刻在 decodeModelCallAttempt() 以 Invalid ModelCallAttempt schema 失败。
真正要紧的是现实后果:provider turn 仍然成功,而 provider-request-telemetry.ts:604-609 会吞掉记账失败。于是 usage、cost 和 latest-context 静默地没有落盘。 一旦接入 MCP server,65 个以上工具的注册表是寻常配置而不是极端情形,所以这在正常使用中可达;而失败在产品里和测试套件里都是静默的。
最小补法:让现有那条真实 send/reopen 测试用 65 或 69 个实际工具,断言 64 个 named、精确的 remainder 计数、字节守恒,以及冷热 SQLite 读回一致。
定 P2 而不是 P3,是因为它在常见配置下是静默的数据丢失,而所有相关测试都还是绿的——不是因为它拦这次合并。它不拦。
在被测到的地方,当前实现是正确的
用真实的 SessionManager / AI SDK backend / tracker / AgentRun SQLite 链分别跑 63、64、65、69 个工具,全部通过,重启后冷读一致:63 → 63 无 remainder;64 → 64 无 remainder;65 → 64+1;69 → 64+5。named 加 remainder 的字节始终等于 tool_definitions。记录约 2,698–2,772 B。超限语义是摘要,不是拒绝,也不是默默丢弃——字节仍然入账。
这次删除是安全的,包括对已经躺在磁盘上的数据
前提能在历史里查证,而不只是写在描述里。151844286(#2605)删掉了 packages/headless/src/provider-request-trace.ts(566 行)连同它 1,073 行的测试——那就是 reader;同一个 commit 里 producer 只改了 22 行,活了下来。35998f234(#1277)引入了 capture。被删 helper 没有任何残留 importer。
遗留数据的处理值得单独说,因为这件事很容易做错,而这次做对了。保留 provider_request_capture 的 source policy 不是客气,它是承重的。policy 表以 satisfies Record<ArtifactSource, ArtifactSourcePolicy> 结尾,把它和枚举绑死了:只从 ARTIFACT_SOURCES 删掉那个值会以 TS2353 编译失败。而两处都删的后果比乍看更重——artifact-metadata-codec.ts:120 对未知 source 会 throw invalidMetadataRecord,而 decodeArtifactRecordJsons 是 fail-fast:一条坏记录会中止整批,而不是跳过它。对照描述里实测工作区中的 379 条 capture 记录,那等于整个 artifact 元数据解码打不开,而不是几行读不出来。在这个 head 上探针核过:该 source 仍在枚举中,canUserDeleteArtifact 为 true,可见性与共享读为 false——与注释所述完全一致。
旧的 requestObservation 仍能解码与折叠,旧的 captureArtifactId 仍在 conversation copy 中被重写。
快照改动对分页是安全的
去掉 sessionSnapshots 这个 map、改为按需计算,保住了分页合同,因为 revision 是内容的纯函数:sha256(JSON.stringify(records)),而记录按 createdAt 降序、id 作决胜键排序——这是一个全序。所以「读时计算」与「缓存」结果一致。成本从每次写移到了每次读,在这里方向是对的;而且一旦 capture 不再占到 87%,读侧本身也变便宜了。两半确实是耦合的,正如描述所说。
关于那些数字
从表里重算:每次调用 26.3× / 58.1× / 102.9×,单 Session 29.5× / 59.3× / 102.9×,每个 capture 均值 2.039 MB,379/436 = 86.9%,99.38% 重复率,772.7 + 3.3 = 776.0 MB,379 + 57 = 436。逐项都对。
对曲线论证有一处限定,只为准确起见:「4× 的 turn 花 8× 的字节」在它引用的那一对上成立(50 → 200 turns 是 7.93×),但 200 → 500 是 4.35×,而纯二次应为 6.25×。「after」那一侧在两个区间都精确线性(3.95× 和 2.51×)。核心主张并不依赖这个概括——40、200、600 条会话下每次调用恒为 3,942 B,那直接、而且干净得多地证明了它。
范围
这个 head 上 test、package、windows_recovery 均为绿,与当前 main 的合并树干净。回收磁盘上已有的 capture 属于另一个改动;本 PR 只是停止生产它们,如其所述。这是一次 perf 重构,合并与否仍由人决定。
Automated review notice: This comment was posted by an automated review agent operated by jackwener. It is not an independent human review and does not replace one.
3 participants
Summary
Every model call stored a copy of the conversation. The prepared provider request was serialized whole into an Artifact, and the record beside it carried up to 256 per-segment rows — so the cost of storing one call grew with the Session it belonged to. Nothing read either: the capture's reader shipped in #1277 and was deleted in #2605, which kept the producer; the per-segment detail's only consumer folded it into four byte totals.
flowchart LR REQ(["one provider request"]) subgraph B["before"] direction TB CAP["full request body<br/>private Artifact<br/>grows with the conversation"] OBS["up to 256 segment rows<br/>index · cacheable · comparison<br/>digest · bytes · role"] end subgraph A["after"] direction TB FOLD["4 byte totals<br/>+ capped tool list<br/>1,971 B, flat"] end REQ --> B REQ --> A CAP -.- X1["reader deleted · PR 2605"] OBS -.- X2["folded by its one reader"] classDef dead fill:#fcebeb,stroke:#e24b4a,color:#a32d2d classDef live fill:#e1f5ee,stroke:#1d9e75,color:#0f6e56 class CAP,OBS,X1,X2 dead class FOLD liveThis removes both producers, and seals an Artifact session snapshot when a reader asks for one instead of keeping a map of all of them. #4716 landed the persistence half of #4037 first; this is rebased on it and built on its
applyChangesshape. Reclaiming the captures already on disk is #4738, stacked on this.Closes #4082
Refs #4037
Refs #4704
📉 Before / after
Against
780dc4b4, same machine.Durable bytes per model call — 60 tools, conversation of N messages. Two different costs, worth keeping apart: the record goes in the database and is written twice (AgentRun event log,
usage_model_call_attempts); the capture is one file on disk holding the request whole.The rows stop growing at 200 messages because the retired observation already capped itself at 256 segments — the database cost was a large constant, not an unbounded one. What grew without a bound is the capture file, and it is the whole of the 772.7 MB below.
One Session, every call summed
The curve is the point: before, 4× the turns costs 8× the bytes, because each call copies a conversation that is itself growing. After, 4× the turns costs 4×.
One real workspace — 814 MB installation of mine, before this change:
Deduplicating messages by content within their own Session across all 379 capture files: 4.8 MB unique, 763.3 MB re-serialized duplicates (99.4%). Captures are 87% of the Artifact population, which is also what made the metadata write path expensive — the two problems were never independent.
Artifact store — 6,000 records across 400 Sessions, same machine, one run:
listPage, mean of 50create, mean of 20A snapshot's revision hashes every record in its Session, and the store kept one snapshot per Session, rebuilt on every load and every mutation — so 400 Sessions were sorted and hashed to answer a question about one. The map never earned that: each of the five readers reloads the whole store from the database first, so a kept snapshot never survived to be read. Sealing on the way out deletes the map, the two methods that maintained it, and the per-mutation bookkeeping. The
createfigure is small because the rest of it is ~24 ms of filesystem durability and the fullreadAllbelow.🚧 Still open
Step 1 of #4037. Every mutation and every read begins with
readAll(): fullSELECTplus a JSON decode of every row, scaling with the store. The reseal that used to sit beside it is gone with this PR; the reload is what is left.That reload dates from the
metadata.jsonlera, where re-reading before a mutation was how a writer stayed correct against another process. Making it cheap needs a way to ask whether anything changed since the last read, and SQLite'sdata_versionis not it: the operational-state database is one shared connection per process, so it does not move for a sibling store's writes. Who may invalidate the in-memory mirror is a design question, not a tidy-up, so it belongs in its own change.🧹 What came out
Removed
ContextDiagnosticsSegmentKind/Segment/Tool/Composition— aliases of theModelCallAttempttypes with no consumer outside the package.opaqueflag threaded through every branch of prepared-value normalization. It fed the retired observation's per-segmentcomparisonmode; the one caller left sizes the value and never reads it.request-shape.ts: 430 → 369 lines.64beside the constant that sets it. The remaining list lives on the shape's owner in@maka/coreand both validators read it.Kept, every one —
captureArtifactId, theprovider_request_capturesource, andPreparedRequestObservationwith its validator.hasExactShaperejects a record carrying an unknown key, so removing any of these fails exactly the records this PR exists to stop producing more of.Tried and kept — the two
isPromptCompositionvalidators look like one concept twice, and they are not. A record that fails the decoder in@maka/coreloses its usage and cost with it; a derived projection row that fails is rebuilt from the ledger. So the projection can afford to check the fold's ordering and byte conservation and the durable record cannot, and collapsing them would either strand records or let a broken projection row answer as if it were good.secretFreeParamsalso stays: its redaction is dead weight now that only byte counts leave the function, but removing it would change those counts and break the one property the upgrade rests on — that a Session's numbers stay comparable across it.One test given a checkpoint —
graceful Host shutdown stops and drains an active Turnstopped the Host as soon asturn.startreturned, which only says the Turn was admitted; which state the drain then found was left to how fast the machine was. It failed twice while this branch was verified with builds and benchmarks running beside it. It now waits for the question the scenario is about to ask, the same checkpoint its sibling test one screen below already uses. Honest limit: I could not reproduce the failure under controlled load either before or after — 3 runs each with two other suites running concurrently were green both ways, as were 4 isolated runs on this branch and 4 onmain. So this is not a demonstrated fix; it replaces a timing race with a defined point.✅ Verification
format,lintclean.test:dist:@maka/core807,@maka/runtime3,216,@maka/storage1,117,@maka/runtime-host1,687 — 0 failures.typecheckalso on@maka/desktop,@maka/ui,@maka/mcp,@maka/eval,maka-agent./contextanswer what they answered before, from the same fold, and byte accounting is unchanged —sizedSegmentserializes a given value to the same bytes the retired path did, so a Session's numbers stay comparable across the upgrade.One model-visible change: a sub-agent's
spawntool result listed the capture Artifact inartifactIds/artifactCount. A child turn now stores nothing of its own, so that list is empty. Nothing but the private capture ever appeared there.🔍 Review focus
The compatibility boundary: the producers are gone but every decoder stays, because real stores hold these records today.
hasExactShapefails a whole record on an unknown key, so a decoder removed here is a record that stops decoding — including its usage and cost.AI use
Select exactly one:
Tool(s) and scope: Claude Code — traced the demand chains, wrote the implementation and tests, ran the measurements. Reviewed and verified by me.
Checklist
Does this PR entail a change in behavior?