Prune acknowledged tombstones from vfs_changes so the table stops growing without bound
まだ誰も着手していません。
評価
- 難易度
- 5/5
- 見積もり時間
- 1週間以上
- 初心者へのやさしさ
- 45/100
- issue の種類
- 機能追加
- 明瞭さ
- 明確に書かれている
- 活発さ
- 静か
- 技術スタック
- sqlite, typescript
調査の方向性
packages/dofs/src/sync/watermarks.ts、packages/dofs/src/sync/coalesce.ts、および参照されているRPC同期パスから始めて、watermarkとcursorの処理を追跡します。SQLiteTestStorageを使用して、既存のwatermarks.test.tsとapply.test.tsのケースを実行します。完了の条件は、pruningがすべてのバックエンドでトランザクショナルかつfail-closedであり、古いfetch cursorが既存の型付きtruncationパスを発生させ、新しいマルチバックエンドテストがスキーマやwire formatを変更せずにパスすることです。
索引モデルが issue の本文から書いたものです。
説明
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 <= pushRevin the same transaction that advancespushRev[...] TodaywriteWatermarkonly updates_vfs_watermark; there is noDELETE FROM vfs_changesanywhere 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_changesacrosspackages/dofs/srcandpackages/computer/srcreturns inserts (sync/changes.ts:10,fs/rename.ts:225) and selects (sync/coalesce.ts:86,90,sync/changes.ts:98). There is noDELETE. writeWatermark(packages/dofs/src/sync/watermarks.ts:85-92) is a singlewriteWatermarkValuecall, 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_changesis 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:300reads from our ownsincePushwatermark.packages/rpc/src/server.ts:189servesfetchChanges({ after }), whereafteris the peer's cursor arriving over the wire. It is not compared against any local watermark before the tombstone query atcoalesce.ts:86runs.
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_changesso it tracks live delete activity rather than cumulative history, making thechanges.ts:96invariant true rather than aspirational. - Prune only at a point no consumer can still need. Concretely, a floor of
MIN(v)across_vfs_watermarkfor bothpushRevandfetchRev, 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 exactdb.transactionSyncshape eight lines belowwriteWatermark. - Make an under-served fetch loud instead of silent. If a peer's
after.revis below the floor, raise a typed truncation error so the client routes into the existing rev-0 re-baseline inreconcileWatermarks(sync-driver.ts:396-435) rather than quietly missing deletions.ELOG_TRUNCATEDis the existing precedent for exactly this trade-off on the exec log, andassertAppliedPushCursoris 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:
// 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.
- 主要言語
- TypeScript
- スター
- 9.2k
- フォーク
- 525
- 平均マージ
- 3日 12時間
- マージ済み PR(30日)
- 18
コントリビューションガイド
はじめの一歩
- issue を最後まで読み、次にプロジェクトのコントリビューションガイドを読みます。
- 着手することを issue にコメントします — 二人が同じ作業をするのを防げます。
- リポジトリをフォークし、ブランチを切って変更します。
- issue 番号を参照したプルリクエストを送ります。
cloudflare/computer のほかの issue
-
enhancement
cloudflare/computer#129 · 担当者 1 名 ·
-
bug
難易度 3/5 1〜2日 初心者へのやさしさ 65/100
cloudflare/computer#106 · コメント 3 件 ·
-
enhancement
難易度 5/5 1週間以上 初心者へのやさしさ 45/100
cloudflare/computer#68 · コメント 1 件 ·
-
bug
難易度 4/5 3〜5日 初心者へのやさしさ 48/100
cloudflare/computer#52 · コメント 3 件 ·
-
enhancement
難易度 5/5 1週間以上 初心者へのやさしさ 35/100
cloudflare/computer#50 · コメント 5 件 ·
cloudflare/computer の issue をすべて見る
似ている issue
-
bug(cli): hapi doctor inline-media prints a fabricated B:\ helper-script path in packaged installs オープン
難易度 2/5 1〜3時間 初心者へのやさしさ 70/100
-
Crush オープン
難易度 1/5 1時間未満 初心者へのやさしさ 85/100
catppuccin/catppuccin#3125 ·
-
Add a SECURITY.md オープン
難易度 1/5 1時間未満 初心者へのやさしさ 90/100
ElementsProject/cln-application#167 · コメント 1 件 · リアクション 1 件 ·
-
難易度 2/5 1〜3時間 初心者へのやさしさ 75/100
Quantco/pnpm-licenses#17 ·
-
難易度 2/5 1〜3時間 初心者へのやさしさ 75/100