feat(runtime-host): write Read images to Session context - #4184
Conversation
91817d6 to
407a687
Compare
15e4434 to
a003af7
Compare
a003af7 to
a6b6e8d
Compare
me2seeks
left a comment
There was a problem hiding this comment.
Requesting changes for four product-reachable ownership/lifecycle boundaries:
- Snapshot ownership must use a Runtime-owned durable operation identity, not a provider-scoped toolCallId.
- Revision/branch collection must follow the selected copy projection and must not retain refs from excluded later turns.
- A known T2/result-commit failure needs best-effort snapshot compensation (existing inline thread).
- Preparing-copy recovery must retain a durable cleanup anchor while the context store is unavailable (existing inline thread).
I am treating archived-result handling under the current pruning thresholds, inactive backup/export integration, and physical GC scheduling as follow-ups rather than blockers for this PR.
a6b6e8d to
34f2783
Compare
me2seeks
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 34f2783. The prior durable-owner, selected-copy projection, known T2 compensation, and unavailable-store recovery gaps are addressed, and I found no remaining product-reachable correctness or ownership issue. Local exact-head builds passed; the focused Runtime, Runtime Host, and two-client integration suites passed 99 + 105 + 1 tests. Hosted windows_recovery is green; the pending main test check remains the merge gate.
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed cccf8a4e. Sorry to arrive after the approval — I went through the six earlier threads rather than taking them as settled, and they all genuinely hold up. Worth saying which, because a couple of them were the kind that get half-fixed:
- The opaque-payload traversal isn't narrowed, it's gone —
visit()was deleted outright and collection now runs through five typed sites. The regression test plantsopaqueStorageRefShapein Tool args and akind: 'json'result and both are ignored. - Archived results are genuinely in scope now, and I checked that
archivePreflightis built fromplan.runsevents +slice.messages+copyTurnIds— the selected projection, not the whole Session. ownerId: ctx.operationIdwith fail-closed on absence, andctx.operationIdreally is always present in production (runtimeCommitSinkis injected unconditionally atexecution-composition.ts:741,invocationId = this.input.invocationId ?? runId).- The compensation hook is safe in the direction I was worried about:
commitToolOutcomebottoms out in a singlethis.transaction(...)insqlite-runtime-store.ts:2469, so a rejection rolls back and there's no "commit actually succeeded but we released the ref" window.
One small thing to write down rather than change: the copy path deliberately uses a non-operation owner (targetOwner: { kind: 'read_image_snapshot', ownerId: sourceRefId }) so retried copies dedupe. That's the right call, but it means "owner is always a durable operation id" only holds for newly created snapshots. A one-line comment there would save the next reader the trip.
Where I think this isn't ready yet, and it's one thing wearing several hats.
main currently holds this feature open with zero quotas and a comment saying why: Zero quotas make accidental non-empty puts fail closed until the writer/lifecycle cutover lands. This PR lands the writer — sessionLogicalBytes: GIBIBYTE, workspacePhysicalBytes: 20 * GIBIBYTE — but the lifecycle half of that sentence didn't come with it. Three of my findings are that same gap seen from different sides:
- nothing ever reclaims physical bytes (P2, inline)
- the new failure source in
#discardreaches a recovery loop that was written assuming it can't throw (P1, inline) - the owner change from a content hash to
operationIdsilently removed same-image dedup, and the logical quota is what pays for it (P3, below)
I'd rather see the collector and the recovery guard land here than as follow-ups, because before this PR neither was reachable — the zero quotas made the whole path inert. That's also why I'm raising the P1 despite the approval: it's a startup failure with no in-product way out, and I'd feel bad staying quiet about it.
Smaller ones, no inline thread needed:
createReadImageSnapshotteris now dead. Its only non-test caller wasmain:execution-composition.ts:404, which this PR replaces. What's left is the re-export atartifact-stores.ts:45and its own test.createReadImageSnapshotPlanneris still live for projection images (execution-model-composition.ts:348) — it's just this wrapper. Deleting it belongs with the change that orphaned it.- Repeated Reads of the same image no longer fold. On
mainthe artifact id wassha256(sessionId, turnId, name, mimeType, bytes), so re-reading one image in a Turn produced one artifact. WithownerId = operationIdevery Read creates a ref row andcontext_session_usage.logical_bytesadds up again. Physical bytes are fine — content addressing plus theif (!existingBlob)guard — but the same 5 MiB image read 200 times fills the 1 GiB Session budget. Recoverable by retiring the Session, so P3, but it's a real trade the summary doesn't mention. - When the context store can't be opened, image Read reports the wrong cause.
builtin-tools.ts:428says Read image snapshots are not available in this toolset, while the copy and retirement paths get a stub error carrying the realcause. Fail-closed is right; it's the diagnosis that's lost, and this is precisely the situation where someone will be reading that message.
Things I specifically checked and found fine: no dangling refs (#markBlobUnreferencedIfEligible has WHERE NOT EXISTS (SELECT 1 FROM context_refs WHERE blob_id = ?), so retiring the source doesn't affect a branch pointing at the same blob); copyReferences is idempotent under retry; retirement cleanup only targets removedSessionIds and stays pending on failure; revision semantics are untouched — the new data only enters ConversationCopyArtifactReferenceMap.contextRefs and never reaches revision numbering or the state machine; epoch 88 is bumped with the inline rationale at protocol/index.ts:266, as protocol-epoch-check.mjs requires.
Evidence boundary: I read pr4184 against origin/main and traced the call graphs by hand — I did not build, run the suites, or reproduce either the startup failure or the 20 GiB ceiling. The 99/105/1 numbers in the summary are yours, not independently confirmed; CI test is still queued as I write this. I also haven't read #4071 or the earlier PRs in the chain, so "physical GC isn't scheduled anywhere" is a statement about the current tree, not about your plans — if it's already queued somewhere, say so and I'll drop that one.
AI-assisted review: drafted with Maka; I verified the six resolved threads, the recovery call chain, the GC call graph, and the main-vs-branch quota change against the branch source myself.
|
One more thought, on the two findings together rather than each separately — I think they're the same thing seen twice, and it's about where the slice boundary fell rather than about either piece of code.
// Zero quotas make accidental non-empty puts fail closed
// until the writer/lifecycle cutover lands.
sessionLogicalBytes: 0, workspacePhysicalBytes: 0,That sentence names one cutover with two halves, and this PR brings the first half. Deleting those zeros is genuinely a good deletion — a "not wired up yet" note living in production code is exactly the kind of thing worth getting rid of. But once they're gone, the second half stops being future work and becomes missing behaviour: bytes now accumulate with nothing that reclaims them, and Neither was reachable before this PR. The zero quotas made the whole path inert, and #4071's own Goals list has GC in it, with the constraint being off Host Ready rather than later. Hanging Concretely, what I'd want to be able to say after merge is one sentence: an image Read is written to the context Store, accounted against the quotas, and reclaimed when the Session goes away. Right now it'd be the first two and not the third, and the missing clause isn't visible from the diff. None of this is a comment on the six-PR split — #4182 landing the reader first was clearly the right call, and the layering in #4071 reads well. It's just that this particular boundary cuts through one cutover instead of around it. Happy to be wrong if reclamation is already queued somewhere I haven't read. |
f515912 to
e1fcd3b
Compare
e1fcd3b to
b378980
Compare
b378980 to
34011d3
Compare
34011d3 to
634b2a1
Compare
|
Heads-up: since this merged, the Desktop e2e Bisect, each on a clean checkout with
The failing step is right after the tail Session's prompt is sent: so the prompt sent into the tail Session no longer renders as a user message within the timeout — consistent with the Session-context / tool-runtime changes here rather than with the scroll logic (the ten other transcript-scroll cases pass). Reproduced twice in CI on #4578 (a lockfile-only PR) at merge commits with this change, and locally on macOS as above. I have not dug into the cause; happy to if you would rather I did. |
|
Correction to the above: this is not a regression from this PR. The failure is the known race in #4573 (an unfenced send racing a 30s admission against a 10s expect), which #4577 / #4579 address; today it is hitting most PRs' Desktop e2e runs (transcript-scroll:316, quote-selection:22, streaming-remount:161) and passed on a later run of a branch that includes this merge. My local bisect was one run per commit, which cannot distinguish a flake from a regression — sorry for the noise. |
Part of #4071. The storage reader foundation in #4182 is merged.
Summary
session_contextrefs.Architecture and Key Decisions
Risks and Remaining Work
Breaking Changes
None. Mixed Runtime Host/Client builds are rejected by compatibility epoch 88.
Database Migrations
None.
Verification
@maka/core,@maka/storage,@maka/runtime, and@maka/runtime-hostin dependency order.git diff --check origin/mainand publication secret scans passed.make lint-fixtarget.Merge Order
None.
Related PRs