Repository navigation
Conversation
Preview:
|
| if (!await this.host.flush(lease)) continue; | ||
| const version = this.host.version(lease); | ||
| const remote = this.#changes(lease, batch.baseCursor, batch.files); |
There was a problem hiding this comment.
[P1] Fence the revision actually persisted by the preliminary flush
A file tool can buffer another edit after flushReplica() drains stepBuffer, while its commitAgentStep() is awaiting prefetches. Sampling version() afterwards includes that newer in-memory revision, but #changes() includes only durable rows, so the final version check passes without reconciling the buffered edit. For example, with a.txt = abc, a buffered tool edit c → X and a replica edit a → R produce a receipt whose accepted hash is for Rbc and whose corrections are empty, while the live barrier actually persists RbX. Positional collisions also bypass the normal OT mapping. Return/fence the revision drained by the flush and retry if additional edits were buffered before sampling the canonical changes.
| }, | ||
| }; | ||
|
|
||
| hooks.registerWorktreeReplicaTurn?.(chatId, worktreeTurnAccess, author, () => chatBindings); |
There was a problem hiding this comment.
[P1] Reconcile journal writes before replacing the live pass
During an automatic-compaction reload or transient-model retry, runAgent() snapshots loadChatHistory() before the new pass awaits prepareChatBindings() and history replay. The previous pass remains in #worktreeReplicaTurns until this registration replaces it. A submitBatch() during those awaits can therefore update the previous pass and ACK a durable edit that is absent from the new pass's history snapshot. This line then publishes a live file view missing that acknowledged edit; subsequent model reads and edits use stale content and can overwrite the edit or apply an OT at the wrong positions. Fence the pass handoff and revalidate/replay journal additions before publishing the replacement access. A regression should submit a batch while the second pass's replay is held behind an await and verify the replacement file view includes it.
|
Review: 2 findings. Posted both high-priority findings inline. Reviewed against |
|
Since last review: 0 resolved, 2 still open, 0 new. Verified both previously posted P1 findings still apply at |
| content = applyCodeChange(content, {[workpieceId]: entries}); | ||
| } | ||
| for (let {workpieceId, entries, pin} of prepared) { | ||
| appendAgentEdit(workpieceId, {[workpieceId]: entries}, pin); |
There was a problem hiding this comment.
[P1] Fence in-flight gadget tools against replica pinning
A replica can establish and persist a gadget's first pin here while an ordinary writeFile/editFile is awaiting its own first-write readCommitFiles(head) or anchorKnowledgeToPin(). The tool retains the pin prepared when the gadget was unpinned; when it resumes, appendAgentEdit(..., pin) resets the entire gadget's session content to pin.baseFiles, discarding acknowledged replica changes to other paths. I reproduced this with the current agent functions: pause writeFile(a.txt) at its base read, accept and flush a replica write to b.txt, then resume the tool. Durable b.txt retains the replica write, but the live file view returns the old base content. Subsequent model reads and edits therefore use stale content. Revalidate the pin/revision after the tool's awaited setup, or serialize that setup with replica ingestion, so an already-pinned gadget's overlay cannot be reset by the in-flight tool.
|
Since last review: 0 resolved, 2 still open, 1 new. Both existing P1 findings still apply. Posted one additional P1 finding inline, verified with a deterministic reproduction using the current agent functions. Reviewed against |
|
Since last review: 0 resolved, 3 still open, 0 new. Verified all three previously posted P1 findings still apply at |
deregtd
left a comment
There was a problem hiding this comment.
Reviewed the full 30-file diff at this revision. One additional reproducible storage regression is inline; the three existing concurrency findings remain reproducible.
| } else { | ||
| const combined = composeCodeChange({[id]: [[path, parts.at(-1)!]]}, incoming); | ||
| if (codeChangeSerializedSize(combined) <= MAX_CODE_CHANGE_SIZE) { | ||
| parts[parts.length - 1] = combined[id][0][1]; |
There was a problem hiding this comment.
[P2] Split accepted prefixes at a storage-safe record size
The 8_000_000 change budget lets this compose a prefix larger than SQLite KV can store. In an ordinary worktree with no replica grant, I accepted a new file containing 600,000 界 characters, then appended and accepted another 600,000 characters per epoch. Each individual edit is about 1.2 MB serialized, but the fourth accept tries to store the cumulative 2.4-million-code-unit prefix here and fails with SQLITE_TOOBIG, leaving the proposed changes unmergeable. The identical eight-accept Workers probe passes on base 21984f5; this head fails on accept four, and both new cumulative-prefix regression cases also fail. Please split prefix records using a storage-safe byte bound, including record framing, rather than the larger CodeChange budget.
There was a problem hiding this comment.
Verified fixed at ef76e2dbc16908ae14ad209be3aa53b2dd74cf6b. I reran both cumulative-prefix regression cases against the current source in workerd after generating the browser runtime/bundled blueprints and rebuilding typed-storage. Both pass: eight ordinary accepts and the single-epoch variant retain the full file after DO eviction, with the prefix split into two bounded records.
My previous re-review metadata had carried forward an older failing result; this fresh run supersedes it for this finding. I’m clearing the storage-size issue. The separate replica concurrency and early-flush replay findings still need resolution. This used local test storage and installed dependencies, with no live provider/container/deployment test.
| if (await hooks.commitAgentStep(chatId, author, [], { | ||
| changes, createdGadgets, createdWorktrees, addedBindings, worktreeCommits, | ||
| blueprintMerges, replicaCommit, | ||
| })) ++nextChangeId; |
There was a problem hiding this comment.
[P2] Record which tool edits an early replica flush has already covered
This saves ordinary buffered file-tool edits with msgs = [], so their changes message can precede the assistant message containing those tool calls. On history replay, the editFile branch then adds the already-applied edit to pendingReplayEdits; if the normal step barrier has no remaining writes, it emits no trailing changes message to discharge it. A subsequent session-served readFile therefore applies that edit again: starting with abc, editing a → aX, flushing through the replica, and then reading produces aXbc live but aXXbc on replay (or a replay error when the old match no longer exists). Preserve the association between an early flush and its tool effects so replay does not apply them twice, and cover replay after a mid-step flush in a regression.
There was a problem hiding this comment.
Confirmed on ef76e2dbc16908ae14ad209be3aa53b2dd74cf6b with an independent workerd probe using the real agent file tools and replica checkpoint path. Within one assistant step: read abc, edit a → aX, let a replica checkpoint() run during the next asynchronous source read, then read the edited file. The live result is aXbc; a fresh runAgent() reconstructs that historical read as aXXbc, while the persisted file remains aXbc.
A checkpoint alone can therefore trigger this, without a replica write or a DO reset. Please include this mid-step checkpoint/replay sequence in the regression for the fix. The probe passed its assertions demonstrating the discrepancy; it used a scripted model and no live provider or container.
|
Since last review: 0 resolved, 3 still open, 1 new. The three existing P1 findings still apply. Posted one additional P2 finding inline. The separately reported accepted-prefix storage issue also remains present. Reviewed |
|
Since last review: 0 resolved, 5 still open, 0 new. Reviewed Static review only; tests were not run because dependencies are not installed. |
Eval resultsVerdict: ⚪ Unchanged. No task moved beyond what 10 runs can tell apart from noise.
Failed checks
|
🔬 Eval runs reviewPerformanceWith 10 comparable runs per task, main → PR pass rates were change-calendar 90% → 100%, chess 80% → 70%, incident-desk 90% → 90%, and worker-logs 100% → 80%; none moved beyond noise. Mean costs were mixed: calendar $0.0203 → $0.0229, incidents $0.0228 → $0.0246, chess $0.0506 → $0.0455, and logs $0.0202 → $0.0167; shorter chess/logs times partly reflect early failures, while calendar’s significant cache-hit gain accompanied more steps rather than a changed prompt. Chess lost the most candidate runs, 3/10, and produced the largest detour: trial 4 passed after 120 steps, 14 tool errors and $0.0971, versus a baseline maximum of 81 steps and $0.0735, with repeated unmatched regex edits consuming much of the extra work. ⚪ VERDICT: NO REGRESSION FROM THIS PRThe diff adds replica synchronization and serialized persistence barriers, but the observed failures arise in agent-written gadget code under unchanged prompts and tool contracts, with no attributable performance improvement or regression established. TriageFailure modes
Tool errors
Prompt cache
What to do
|
User scenario
An agent is editing a Git-backed workspace in its DO, then runs a build or test command in a Linux container. Both sides may continue editing. Container edits must become ordinary canonical workspace changes, DO edits must reach the running container, and acknowledged changes must survive a disconnected bridge or a DO reset.
When a container is restored from a snapshot, it needs to catch up from a known point rather than become the new owner of an old workspace copy. A local commit made in the DO must also appear as the same real Git commit in the container.
The existing DO worktree/file tools do not provide an external integration with that checkpoint, replay, submission, and durable-acknowledgement protocol. This PR adds that generic protocol; it does not implement the container itself.
What changes
The DO remains canonical throughout. Ordinary agent file operations stay ordinary DO operations; there is no exclusive container ownership mode or runtime stop-the-world synchronization policy here.
Kernel/gatekeeper boundary
The kernel owns canonical worktrees, OT ordering, authority, and durable receipts. The integration owns filesystem-change detection and OT calculation, initial hydration/catch-up policy, container images, FUSE/native engines, snapshots, commands, and network approvals. Those Linux-specific pieces remain in gadgets-internal.
This revision transfers binary base files but accepts text mutations only. Whole-file binary replica mutations remain the separate #759 follow-up; this PR does not replace text deltas with full-file changes or add binary diffs.
Dependencies and merge order
This draft targets
ddr/split-replica-prerequisites, a merge-only review base, so prerequisite changes do not drown out this diff:The prerequisite PRs target main independently. Do not merge this PR into the temporary base as a release step. Reconcile the final prerequisite heads, then rebase/retarget this PR onto main after they land. In particular, this review base pins #770's initial text-attachment contribution; #770 has since expanded to all file formats and that newer foundation still needs integration during restacking.
Verification and preservation
This reorganization leaves production source/configuration byte-identical to the previous #774 head (
82186a4d). It moves the persistence queue and shared permission extraction into this PR's diff, while #776 owns the generic timing instrumentation. The timing test is relocated, not dropped or duplicated.Full lint, generated-config checks, and workspace build/type checks passed. The initial broad backend run passed 1,550 tests but hit a 20-second transfer-test timeout; that transfer suite then passed all 3 tests in isolation. The bounded-concurrency rerun passed 1,547 tests / 81 files but exited with one workerd-pool runner startup timeout before all files ran, so neither broad run is claimed fully green. The relocated timing/validation suites also passed all 3 tests in isolation. The five previously reproduced failures in unchanged main's ai-models suite are excluded from these runs. Prior backend integration verification passed 3 tests, with 4 intentionally skipped.
Extracted from #729. No deployment, feature enablement, new container performance run, or exactly-once promise for unjournalled shell effects.