Fix issue with file sync post container restart - #103
Merged
Conversation
Mark container start, health, callback, and WebSocket upgrade failures as transport errors while preserving their causes. Separate locally disposed capnweb stubs from ambiguous session failures so command recovery only retries requests that were never dispatched.
Run push and pull through one bounded reconnect attempt. Close stale or unpublished handles before reconnecting, wait for teardown across concurrent callers, and reconcile replacement watermarks before replaying idempotent sync work.
Require the pre-exec push to finish successfully before dispatching a command. This prevents execution against stale or incomplete container contents instead of silently reporting a zero-entry push.
Resolve the active shell handle after pre-exec sync and for every process lifecycle call. Retry command spawn only when the local RPC layer proves dispatch never started. Ambiguous and mid-stream failures invalidate the session without replaying command side effects.
Describe the one-retry boundary for replay-safe computerd operations, the mandatory pre-exec push, and the rule against replaying an ambiguous command spawn. Record the container-local durability limit and add a patch changeset for the published behavior.
Classify outbound interception failures as transport errors alongside the other container connection stages. A replacement container can re-enter the full connection path when the platform is not ready during egress setup.
The two arms of `BackendRetryMode` described different things. The `idempotent` value named a property of the operation, while `pre-dispatch` named the evidence the reconnect loop needs before it replays. At the call sites it was not clear that the two were alternatives for the same decision. Rename the type to `BackendRetryPolicy` and its values to `always` and `pre-dispatch`, so both name what the reconnect loop is allowed to do. The parameter on `#runWithReconnect` and `#runShellEnvelope` becomes `policy` to match. Behavior is unchanged: `always` replays any transport failure, `pre-dispatch` replays only when the failure proves no frame reached the peer, and `shell.exec` remains the sole caller that asks for the narrower policy.
Keep the package README focused on its public surface and rely on the design documentation for reconnect details. Rewrite the changeset as a user-facing outcome and link it to the command synchronization contract.
🦋 Changeset detectedLatest commit: d0f04be The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
commit: |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Run the pre-exec push and command spawn as one reconnect attempt on one backend handle. If that handle fails before dispatch, repeat both steps on the replacement so it receives the workspace before the command starts.
Persist a UUID in the container-owning Durable Object for each running container process. Preserve it across connection changes, rotate it on process replacement, clear it on exit, and attach it to backend handles so process-local operations can detect a changed runtime.
Stamp container executions with the UUID of the process that accepted them. Retry get, kill, and dispose across connection changes only when the UUID still matches, and return EEXEC_LOST before an old execution id can reach a replacement process table.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Keep runtime ownership for the 1024 most recently used execution ids. Returned execution handles retain their UUID directly, while older by-id lookups degrade to no generation expectation instead of growing memory for the lifetime of a busy Durable Object.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Carry the execution runtime UUID through post-command pull and durable retry intents. If reconnect reaches an empty replacement container, keep sync pending instead of reporting a successful zero-entry pull that would hide unrecoverable command writes.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Treat a pending post-command pull as unrecoverable when its runtime UUID no longer matches the live container. Clear that stale intent, report it as lost, and let a later failure from the current runtime replace an older intent so new sync work cannot be blocked forever.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Store the latest runtime UUID for each backend and execution id in Workspace SQLite, using the bounded LRU only as a cache. Direct get, kill, and dispose calls now retain their process fence across Durable Object incarnations and cache eviction.
Comment on lines
+281
to
+283
| // Last known container runtime for recent backend/execution ids. | ||
| // Returned handles carry their own id; the bounded LRU supports | ||
| // direct by-id lifecycle calls without growing for the DO lifetime. |
There was a problem hiding this comment.
🟡 New code comment uses an abbreviation the project's writing rules forbid
A newly added comment abbreviates "durable object" as "DO" (packages/computer/src/workspace.ts:283), which the repository's prose rules explicitly forbid for comments.
Impact: The comment does not follow the project's mandatory writing style for code comments.
Rule reference
AGENTS.md requires loading .agents/skills/prose/SKILL.md when writing code comments, and that file states: "IMPORTANT! Avoid acronyms. Use the full word, e.g. "durable object" instead of "DO"". The rest of this change consistently spells out "Durable Object" (for example packages/computer/src/execution-runtime-tracker.ts:3-4), so this line is inconsistent as well as non-conforming.
Suggested change
| // Last known container runtime for recent backend/execution ids. | |
| // Returned handles carry their own id; the bounded LRU supports | |
| // direct by-id lifecycle calls without growing for the DO lifetime. | |
| // Last known container runtime for recent backend/execution ids. | |
| // Returned handles carry their own id; the bounded LRU supports | |
| // direct by-id lifecycle calls without growing for the whole | |
| // Durable Object lifetime. |
Was this helpful? React with 👍 or 👎 to provide feedback.
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #101
A container command uses the workspace in four steps:
After
computerdstopped, the next command could reuse its dead connection or reach the replacement container before it was ready. The command failed immediately. A failed copy into the container was also ignored, so a command could run against an empty or outdated workspace.The change fixes that flow in these steps:
Verification passed with:
npm run check npm run typecheck --workspace @cloudflare/computer npm run build npm test --workspace @cloudflare/computer npm run test:harness --workspace @cloudflare/computer SOAK_SYNC_TICKS=10 SOAK_FETCH_CALLS=10 SOAK_EXEC_CALLS=20 node script/computerd-stub-soak.mjsThe deployed Model Context Protocol example was also tested by saving host and container-written files, stopping PID 1, and immediately running another
container-shellcommand. A new container became ready after about 33 seconds, its temporary marker was gone, and both files saved in Durable Object storage were present. The test files were then removed.The documentation explains the retry rules and the remaining limit: files written only inside a container can still be lost until they are copied back. A patch changeset is included. The shared protocol and
computerdimage are unchanged, so consumers only need to update@cloudflare/computerand redeploy their Worker and Durable Object code.