cloudflare / cloudflare/computer

Prune acknowledged tombstones from vfs_changes so the table stops growing without bound

Open
#67 1 comment 0 reactions 0 assignees View on GitHub
Dominant language
TypeScript
Stars
9.2k
Forks
513
Avg merge
3d 8h
Merged PRs (30d)
24

Description

# Summary

`vfs_changes` is append-only. Nothing in the repository ever deletes from it, so every `rm` a workspace performs is retained for the lifetime of the Durable Object. `docs/03_filesystem_schema.md` already flags this as planned-but-unwired. This proposes wiring it, with a safer predicate than the one the doc sketches.

Filing as an issue because `CONTRIBUTING.md` routes feature requests to Discussions and Discussions are not enabled on this repo, so the documented link 404s (#53). Happy to move this to a Discussion if that gets turned on.

# Background and motivation

`docs/03_filesystem_schema.md:197` states the intent and the current state:

> Pruning (planned; not yet wired). The target behaviour is to delete rows with `rev <= pushRev` in the same transaction that advances `pushRev` [...] Today `writeWatermark` only updates `_vfs_watermark`; there is no `DELETE FROM vfs_changes` anywhere in the package, so the table grows unboundedly with delete activity. Cheap to add once the apply path becomes push-atomic.

Both halves verify against `main` at `76d9e75`:

- Searching `vfs_changes` across `packages/dofs/src` and `packages/computer/src` returns inserts (`sync/changes.ts:10`, `fs/rename.ts:225`) and selects (`sync/coalesce.ts:86,90`, `sync/changes.ts:98`). There is no `DELETE`.
- `writeWatermark` (`packages/dofs/src/sync/watermarks.ts:85-92`) is a single `writeWatermarkValue` call, no transaction, no prune.

There is a second consequence beyond storage size. `packages/dofs/src/sync/changes.ts:96` justifies a per-path lookup with an invariant that does not currently hold:

> an indexed scan by path is cheap because `vfs_changes` is bounded by the watermark window.

Nothing bounds it. That lookup runs on every `materialiseChange`, and the `(path, id DESC)` index at `packages/dofs/src/schema/sync.ts:25` grows with cumulative historical delete count rather than with live state. An agent loop that repeatedly builds and cleans a tree (`npm install`, `rm -rf node_modules`) adds one row per removed path per cycle, permanently. `fs/rename.ts:225` inserts one tombstone per path in a moved subtree via a CTE, so a single recursive delete or directory rename is O(subtree) rows.

## Why the doc's one-line predicate is not safe as written

`rev <= pushRev` is correct for one backend with one consumer. Neither assumption holds today.

Multiple backends. `_vfs_watermark` is keyed `PRIMARY KEY (k, backend)` (`packages/dofs/src/schema/sync.ts:31-36`), and every watermark accessor takes a `backend` parameter. The README says a Workspace may register multiple backends under stable IDs. Pruning at backend A's `pushRev` destroys tombstones backend B has not been told about, and B never learns those paths were deleted.

Served fetches use a cursor that is not ours. `coalesceChanges` has two consumers:

- `packages/rpc/src/sync-driver.ts:300` reads from our own `sincePush` watermark.
- `packages/rpc/src/server.ts:189` serves `fetchChanges({ after })`, where `after` is the peer's cursor arriving over the wire. It is not compared against any local watermark before the tombstone query at `coalesce.ts:86` runs.

So a peer resuming from a cursor below the prune point silently receives no tombstones for the pruned range, keeps files the DO considers deleted, and nothing reports it. Silent divergence is a worse failure than unbounded growth.

# Goals

- Bound `vfs_changes` so it tracks live delete activity rather than cumulative history, making the `changes.ts:96` invariant true rather than aspirational.
- Prune only at a point no consumer can still need. Concretely, a floor of `MIN(v)` across `_vfs_watermark` for both `pushRev` and `fetchRev`, across every backend, with an unknown or never-written backend contributing 0 and stopping the prune entirely.
- Perform the delete in the same transaction that advances the watermark, which is what the doc asks for. `writeFetchCursor` (`watermarks.ts:105-114`) already demonstrates that exact `db.transactionSync` shape eight lines below `writeWatermark`.
- Make an under-served fetch loud instead of silent. If a peer's `after.rev` is below the floor, raise a typed truncation error so the client routes into the existing rev-0 re-baseline in `reconcileWatermarks` (`sync-driver.ts:396-435`) rather than quietly missing deletions. `ELOG_TRUNCATED` is the existing precedent for exactly this trade-off on the exec log, and `assertAppliedPushCursor` is the house style for failing loudly on a cross-side invariant.
- No schema migration and no wire-format change. Tables and indexes are unchanged; only rows are removed.

Out of scope: blob and manifest reclamation, which is a separate gap I am filing alongside this one.

# Example

The shape of the change in `packages/dofs/src/sync/watermarks.ts`:

```ts
// New: the lowest rev any consumer could still resume from.
// Fail closed - a backend with no watermark row contributes 0.
export function prunableRev(db: Database): number { /* MIN over pushRev + fetchRev, all backends */ }

export function writeWatermark(db, key, value, backend = DEFAULT_BACKEND_ID): void {
db.transactionSync(() => {
writeWatermarkValue(db, key, value, backend);
db.run("DELETE FROM vfs_changes WHERE rev <= ?", prunableRev(db));
});
}
```

The test that distinguishes this from the doc's literal predicate: register backends A and B, advance only A, and assert no rows are pruned.

The tests would live in `packages/dofs/src/sync/watermarks.test.ts` and `apply.test.ts`, which already build real databases via `SQLiteTestStorage` and already call `writeWatermark` directly (`apply.test.ts:555-556,616-618,703-712`).

If it helps review, the truncation error (the fourth goal) is a correctness improvement that stands on its own and could land first as a smaller change, since a peer resuming from a stale cursor is already reachable today through `reconcileWatermarks` resets.

Happy to open a PR if the direction works for you, or to adjust the predicate if there is a resumable cursor I have missed.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.