🌳 Retain and restore immutable Workspace roots - #393
Conversation
PR #393: 🌳 Retain and restore immutable Workspace roots19 files, +2834 / -190 Scope🔴 PR has 3024 lines changed. Split into focused PRs. 🟡 3024 lines changed. PRs under 400 receive more thorough review. Structural✅ No structural bloat detected. Slop
Static Analysis✅ Oxlint found no issues. CorrectnessNo extraneous code patterns detected. |
fe00594 to
02a7363
Compare
84998e3 to
cbe78a1
Compare
System/software architecture reviewVerdict: REQUEST CHANGES. The root model is coherent, but two correctness defects prevent this layer from satisfying its own durability contract. The architecture direction is right:
The following must be corrected before merge. 1. Clear authoritative DOFS caches when an outer Workspace transaction rolls back
The failing sequence is:
Restoration itself clears caches, but ordinary private Workspace rollback does not. This is exactly the raw-outer-transaction hazard called out in the #391 architecture review. Make rollback of any caller-owned transaction that may have reached DOFS clear both the resolution and blob caches on the authoritative wrapper. The cleanup must run for thrown failure, returned validation failure, cancellation/teardown, and commit failure; it cannot rely on code after a Add a discriminating test:
Cover cancellation as well as ordinary failure if the common cleanup path is not already directly proven. 2. Validate every historical file entry's declared size against its DOFS manifest
The live/current comparison catches this only for the current root. A historical root can therefore be changed coherently at the XMD-table level—canonical manifest bytes, recomputed root ID, exact root reference rows—so that a file entry declares a different size from the referenced DOFS manifest. Lookup accepts that historical root, but When deriving/validating a root, require every file entry's Add a read-only corruption test where:
3. Add the new WRR conformance tier to the executable specificationThe tests introduce Add a Tier WRR table covering canonical identity, mutation-derived roots, restoration, rollback/cache coherence, read-only corruption recognition, bigint redaction, child teardown, and no-GC closure. Include the two missing cases above in that mapping. This keeps specification, mechanics, and the executable test inventory moving together. After these changes, rerun the focused root/restoration suites and the four repository gates. The currently green CI demonstrates the existing test set passes; it does not exercise the two counterexamples above. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| yield* until(filesystem.rm(path, options)); | ||
| }, | ||
|
|
||
| // deno-lint-ignore require-yield |
There was a problem hiding this comment.
Redundant comment — restates what the code does.
| // deno-lint-ignore require-yield |
| yield* until(filesystem.symlink(target, path)); | ||
| }, | ||
|
|
||
| // deno-lint-ignore require-yield |
There was a problem hiding this comment.
Redundant comment — restates what the code does.
| // deno-lint-ignore require-yield |
| const workspace: PrivateWorkspaceTransaction = { | ||
| filesystem: createDenoWorkspaceFilesystem(connection), | ||
|
|
||
| // deno-lint-ignore require-yield |
There was a problem hiding this comment.
Redundant comment — restates what the code does.
| // deno-lint-ignore require-yield |
| return currentWorkspaceRoot(connection.database, connection.path); | ||
| }, | ||
|
|
||
| // deno-lint-ignore require-yield |
There was a problem hiding this comment.
Redundant comment — restates what the code does.
| // deno-lint-ignore require-yield |
| return captureWorkspaceRoot(connection, options); | ||
| }, | ||
|
|
||
| // deno-lint-ignore require-yield |
There was a problem hiding this comment.
Redundant comment — restates what the code does.
| // deno-lint-ignore require-yield |
1 participant
Why
A WorkflowRun database already owns one authoritative SQLite/DOFS connection and the complete version-1 schema, but it only recognizes the canonical empty Workspace. A retained root must name the complete filesystem deterministically and remain sufficient to rebuild it after the live frontier changes.
What changes
Before:
Complete schema v1 stores the empty root and refuses any nonempty live Workspace.
After:
The Deno adapter captures arbitrary live DOFS frontiers as immutable, content-addressed Workspace roots, retains their exact manifest/blob reachability, validates arbitrary current and historical roots without repair, and privately restores a selected root through a nested savepoint.
Opening an existing run reads structure, retained content, the live/current comparison, and the run row through one SQLite snapshot. Retained-root integer reads opt into
bigintconversion so corrupt 64-bit values reach redacting parsers instead of escaping throughnode:sqlite. Private Workspace transaction bodies finish child teardown before final live/current validation.How it works
Files remain solely in DOFS blobs. Root manifests record paths, topology, kind, mode, observable mtime, file size and DOFS manifest identity, symlink targets, and canonical hardlink groups; they exclude mutable inode/revision/cache bookkeeping.
Review guide
Start with:
packages/workflow/src/deno/workspace/manifest.tsThen review:
packages/workflow/src/deno/workspace/root.ts— live traversal, bigint-safe DOFS reads, content validation, retention, and read-only recognitionpackages/workflow/src/deno/workspace/restore.ts— savepoint materialization, cache coherence, and identity proofpackages/workflow/src/deno/workspace/private.ts— adapter-private transaction boundary and child-teardown orderingpackages/workflow/src/deno/provider.tsandreading.ts— single-snapshot recognition and redacted integer readspackages/workflow/tests/workspace-root*.test.ts— identity, retention, restoration, corruption, teardown, and rollback proofarchitecture.mdandspecs/workflow-spec.md— the present-tense storage contractLook carefully at:
What must stay true
sha256("xmd-workspace-root\0v1\0" || canonical_manifest_utf8)— enforced by fixed-order encoding and checked by independent-state and reopen tests.WorkflowDatabaseCorruptErrorvalues, never rawRangeErrorvalues containing stored data.How to verify it
deno task lint— passes with 0 errors.deno task check— passes.deno task test— 403 tests / 2,755 steps pass in 4m57s.deno task check:jsr— passes withSuccess Dry run complete.pnpm exec tsc --project tsconfig.node.json --noEmit— passes.pnpm test:nodeunder the repository's Node 22 toolchain — 2,543 tests pass with 0 failures.deno task verifyfrom the committed, prepared, clean worktree — all 9 applicable commands pass concurrently: vendor 2.1s, lint 2s, check 0.8s, test 446.8s, check:jsr 0.9s, tsc 18.3s, test:node 208.8s, test:bun 288.8s, docs 12.7s. The tracked tree remains unchanged.git diff --check origin/agent/issue-365-2-connection-schema...HEAD— passes.The earlier apparent 20-minute Deno timeout was caused by the restricted local sandbox denying the verifier's
/bin/pschild-process cleanup. Running the unchanged battery in its required host environment completed and exposed two test portability errors; those were corrected before the successful final run. The deadline, concurrency, and clean/composability checks are unchanged.Scope
Included
Intentionally unchanged
<File>, or workflow start/resumeNew abstractions
setReadBigInts(true)and consistent SQLite read transactions without crossing a shared production boundary.Risks and limitations
Scope confirmation