Skip to content

feat(runtime-host): write Read images to Session context - #4184

Merged
likun666661 merged 5 commits into
mainfrom
refactor/read-image-context-writer
Sep 2, 2026
Merged

feat(runtime-host): write Read images to Session context#4184
likun666661 merged 5 commits into
mainfrom
refactor/read-image-context-writer

Conversation

@likun666661

@likun666661 likun666661 commented Aug 29, 2026

Copy link
Copy Markdown
Member

Part of #4071. The storage reader foundation in #4182 is merged.

Summary

  • Write production Read image snapshots to the typed Session context Store instead of ArtifactStore.
  • Use Runtime's durable tool operation id as snapshot ownership and compensate known T2 result-commit failures best-effort.
  • Copy only typed context refs in the selected branch/revision projection and retire refs through durable Session cleanup.
  • Keep preparing-copy cleanup retryable while the context Store is unavailable.
  • Publish compatibility epoch 88 because Read tool results can now carry session_context refs.

Architecture and Key Decisions

  • SQLite owns context metadata and reference lifecycle; managed bytes remain KV-separated and content-addressed.
  • Read uses one canonical writer with 5 MiB per-image, 1 GiB per-Session logical, and 20 GiB per-workspace physical limits.
  • Snapshot uniqueness is keyed by Runtime's durable operation identity, never the provider-scoped call id.
  • Conversation copy traverses only the typed attachment/image fields it rewrites; opaque tool payloads are not interpreted as storage refs.
  • Known T2 failure invokes a tool-owned compensation hook; compensation failure never replaces the persistence error.

Risks and Remaining Work

  • Hard-crash windows cannot be perfectly compensated; Session retirement remains the durable cleanup backstop.
  • Ordinary branch/revision copies reject archived source-owned image refs rather than migrating them in this slice.

Breaking Changes

None. Mixed Runtime Host/Client builds are rejected by compatibility epoch 88.

Database Migrations

None.

Verification

  • Built @maka/core, @maka/storage, @maka/runtime, and @maka/runtime-host in dependency order.
  • 99 focused Runtime tests passed for Read image wiring, filesystem authority, conversation copy, and T1/T2 durability.
  • 105 focused Runtime Host tests passed for production recovery, protocol compatibility, and Session retirement.
  • Two-client UDS Session branch/revision restart integration test passed.
  • git diff --check origin/main and publication secret scans passed.
  • Repository has no make lint-fix target.

Merge Order

None.

Related PRs

@likun666661
likun666661 force-pushed the refactor/session-context-ref-reader branch from 91817d6 to 407a687 Compare August 29, 2026 14:58
@likun666661
likun666661 force-pushed the refactor/read-image-context-writer branch from 15e4434 to a003af7 Compare August 29, 2026 14:59
Base automatically changed from refactor/session-context-ref-reader to main August 29, 2026 15:16
@likun666661
likun666661 force-pushed the refactor/read-image-context-writer branch from a003af7 to a6b6e8d Compare August 29, 2026 15:19
Comment thread packages/runtime-host/src/server/session-revision-coordinator.ts
Comment thread packages/runtime-host/src/server/session-revision-coordinator.ts
Comment thread packages/runtime/src/conversation-copy.ts Outdated
Comment thread packages/runtime-host/src/server/execution-composition.ts

@me2seeks me2seeks left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes for four product-reachable ownership/lifecycle boundaries:

  1. Snapshot ownership must use a Runtime-owned durable operation identity, not a provider-scoped toolCallId.
  2. Revision/branch collection must follow the selected copy projection and must not retain refs from excluded later turns.
  3. A known T2/result-commit failure needs best-effort snapshot compensation (existing inline thread).
  4. 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.

Comment thread packages/runtime/src/builtin-tools.ts Outdated
Comment thread packages/runtime/src/conversation-copy.ts Outdated
@github-actions github-actions Bot added the effort/M Under 500 readable lines label Aug 30, 2026
@likun666661
likun666661 force-pushed the refactor/read-image-context-writer branch from a6b6e8d to 34f2783 Compare September 1, 2026 14:37
@likun666661
likun666661 requested a review from me2seeks September 1, 2026 14:37

@me2seeks me2seeks left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 gonevisit() was deleted outright and collection now runs through five typed sites. The regression test plants opaqueStorageRefShape in Tool args and a kind: 'json' result and both are ignored.
  • Archived results are genuinely in scope now, and I checked that archivePreflight is built from plan.runs events + slice.messages + copyTurnIds — the selected projection, not the whole Session.
  • ownerId: ctx.operationId with fail-closed on absence, and ctx.operationId really is always present in production (runtimeCommitSink is injected unconditionally at execution-composition.ts:741, invocationId = this.input.invocationId ?? runId).
  • The compensation hook is safe in the direction I was worried about: commitToolOutcome bottoms out in a single this.transaction(...) in sqlite-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 #discard reaches a recovery loop that was written assuming it can't throw (P1, inline)
  • the owner change from a content hash to operationId silently 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:

  • createReadImageSnapshotter is now dead. Its only non-test caller was main:execution-composition.ts:404, which this PR replaces. What's left is the re-export at artifact-stores.ts:45 and its own test. createReadImageSnapshotPlanner is 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 main the artifact id was sha256(sessionId, turnId, name, mimeType, bytes), so re-reading one image in a Turn produced one artifact. With ownerId = operationId every Read creates a ref row and context_session_usage.logical_bytes adds up again. Physical bytes are fine — content addressing plus the if (!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:428 says Read image snapshots are not available in this toolset, while the copy and retirement paths get a stub error carrying the real cause. 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.

Comment thread packages/runtime-host/src/server/execution-composition.ts
Comment thread packages/runtime-host/src/server/execution-composition.ts
Comment thread packages/runtime/src/conversation-copy.ts Outdated
Comment thread docs/changelogs/2026-09-01-read-image-context-writer-review-fixes.md Outdated
@Astro-Han

Copy link
Copy Markdown
Contributor

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.

main currently carries this:

// 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 #discard now has a way to fail that its caller was written without.

Neither was reachable before this PR. The zero quotas made the whole path inert, and #discard couldn't throw because the store wasn't wired into it. So they aren't pre-existing gaps this PR happens to sit next to — they're both created by it, which I think is what makes them belong here rather than in a follow-up.

#4071's own Goals list has GC in it, with the constraint being off Host Ready rather than later. Hanging collectGarbage off the Session-retirement drain satisfies that reading directly, and session-sidecar-purge.ts already holds contextOffload, so it's a widened Pick<> and a call.

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.

@likun666661
likun666661 force-pushed the refactor/read-image-context-writer branch from f515912 to e1fcd3b Compare September 1, 2026 16:37
@likun666661
likun666661 force-pushed the refactor/read-image-context-writer branch from e1fcd3b to b378980 Compare September 2, 2026 02:02
@github-actions github-actions Bot added effort/L Under 1000 readable lines and removed effort/M Under 500 readable lines labels Sep 2, 2026
@likun666661
likun666661 force-pushed the refactor/read-image-context-writer branch from b378980 to 34011d3 Compare September 2, 2026 06:54
@likun666661
likun666661 force-pushed the refactor/read-image-context-writer branch from 34011d3 to 634b2a1 Compare September 2, 2026 15:07
@likun666661
likun666661 merged commit b714a39 into main Sep 2, 2026
2 checks passed
@likun666661
likun666661 deleted the refactor/read-image-context-writer branch September 2, 2026 15:37
@Joob1n

Joob1n commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Heads-up: since this merged, the Desktop e2e transcript-scroll.spec.ts:251 ("switching Sessions restores a Turn anchor while a tail Session follows background growth") fails deterministically, and main has not shown it because every push since dies earlier at the CLI production audit (#4578 fixes that and is the run that exposed it).

Bisect, each on a clean checkout with npm ci and npm --workspace @maka/desktop run e2e -- transcript-scroll:

Commit Result
8cdd61e2d (the parent of this merge) 11/11 pass
6c8e749d3 (main tip; the only other commit in between, #4469, touches one CLI test file) 10 pass, 1 fail

The failing step is right after the tail Session's prompt is sent:

Locator: locator('.maka-user-message').filter({ hasText: '第 1 行' })
Expected: visible   Timeout: 10000ms

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.

@Joob1n

Joob1n commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/L Under 1000 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants