MILAB-6705: disk-persist the tree mirror (middle layer) - #1783
Open
xnacly wants to merge 6 commits into
Open
Conversation
Adds a length-prefixed binary format for a tree mirror, with capture and restore around it. Resource bodies store every reference as a global id and keep signatures in a side table the decoder rejoins, so the corpus outlives the signatures it was written with. The session witness (the root's signature bytes) sits outside the compressed payload, so a rotated session is detected without inflating the file. Restore goes through updateFromResourceData into a throwaway tree, which is what keeps a failed restore from invalidating a working one. A torn, foreign or corrupt file reports a reason instead of throwing, and PlTreeState grows an isValid getter so capturing an invalidated tree is refused rather than silently written.
Wires the snapshot codec into the tree and the middle layer. SynchronizedTreeState gains a restoreFrom option that seeds the tree before its first refresh, a capture method, and a change generation bumped on every cycle that brought something new, which is what the periodic write gates on. On the middle-layer side a filesystem store addresses one file per project by backend, user, root, build stamp and schema version, witnesses the session with the root's signature so a rotated session is a miss that keeps the file, evicts out-of-scope entries and trims to a ceiling at startup, and writes nothing at all for an impersonated client. Writes happen on the existing project maintenance loop, change-gated and once per interval, plus once at closeProject, deliberately not in the shutdown teardown. A restored tree whose first refresh is refused discards its snapshot and reopens cold, once. The build stamp is injected through rolldown's transform.define, so any change to the pruning, field filter, traversal stop or finality rules invalidates every snapshot. A dirty worktree stamps the build time too, so editing those rules locally cannot hit a stale mirror.
Covers warm reopen, project switching, killed process, open and idle, open and computing, rotated signature, poisoned snapshot and the kill switch. Two fixes the scenarios forced out. The tree now reports whether a snapshot was actually applied, via wasRestoredFromSnapshot, and the middle layer reads that instead of assuming a restore happened whenever init did not throw: a snapshot can be handed over and still be refused, and the old flag would then claim the file on disk described the tree we hold. The store gains a restores counter alongside hits, since a hit only means the bytes were read and a warm reopen needs the tree to have accepted them. MiddleLayer exposes treeSnapshotStats so that is observable at all. Verified the reopen assertions bite by breaking restore and watching them fail.
🦋 Changeset detectedLatest commit: f517a5e The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 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 |
Correctness: - The fail-safe now retries cold on any failure of a warm open, not only on the three classified ones. Rethrowing left the snapshot on disk, so the next open restored it and failed identically: a project that never opens again until someone deletes the cache directory, which is the outcome the fail-safe exists to prevent. Deletion stays reserved for failures that implicate the snapshot, so a timeout no longer destroys a good mirror. The classifier walks the cause chain and is now tested. - A failed write was recorded as a successful persist, so both triggers believed the tree was on disk and skipped it forever, close write included. One transient I/O error cost the whole session. write() now reports success. - captureTreeState copied nothing: it handed out the tree's live field objects, which the next update mutates in place. Any await between capture and encode produced a corpus whose fields point at resources it does not carry, i.e. an unrestorable snapshot. Only the synchronous call path made this safe. - The signature side table silently collapsed one global id carrying two signatures, rewriting one resource's references to another's. Unreachable for single-root trees; now refused rather than silently wrong. - changeGeneration missed dynamic-field removals, so a poll that dropped a field and collected its subtree read as idle and skipped the write. - purge() was an unguarded recursive delete of a caller-supplied path. It now removes only this class's files. - The cache key carries the backend instanceId, so a database reset at a fixed address is a miss rather than a hit against reused global ids. Also: eviction no longer stops early on an undeletable file; reads distinguish unreadable from absent; a hit touches the file so the size trim orders by use rather than by write; decode copies byte slices instead of pinning the whole payload; the in-flight guard loops; the first periodic write lands on the first maintenance pass rather than one interval in, so sessions shorter than the interval are covered; and the sources-mode stamp is stable, so running from sources exercises restore instead of guaranteeing a miss. Two build-stamp comments were wrong: the finality predicate lives in pl-client, not here, and release builds always take the dirty path because CI writes version bumps before building.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1783 +/- ##
==========================================
+ Coverage 53.21% 53.94% +0.72%
==========================================
Files 376 379 +3
Lines 20081 20535 +454
Branches 4451 4543 +92
==========================================
+ Hits 10687 11077 +390
- Misses 8087 8127 +40
- Partials 1307 1331 +24 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
closeProject captured and encoded up to ten megabytes before returning, so project switching sat behind the write. Only the capture needs the tree alive, and a capture is a copy rather than a view, so the encode and write can run after the tree is gone. closeProject now starts the write and returns. MiddleLayer keeps the promise so close() can drain it, bounded at five seconds: quitting still starts no snapshot work of its own, it only lets one already in flight finish, and a wedged filesystem cannot hold the quit open. The close write is queued behind any in-flight periodic write rather than racing it, so the same mirror is not encoded twice. Measured first: moving the encode to the middle layer's worker thread was the obvious alternative and is a bad trade, because structured-cloning the snapshot across the boundary costs 32ms of the 54ms it would save on the reference project's shape.
xnacly
marked this pull request as ready for review
August 18, 2026 14:27
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+366
to
+371
| if (inScope) { | ||
| current.push({ file, size, mtimeMs }); | ||
| continue; | ||
| } | ||
|
|
||
| await this.remove(file, size, false); |
Contributor
There was a problem hiding this comment.
If treeSnapshotPath is configured as an existing or shared directory, startup eviction passes every regular file outside the current snapshot scope to remove(), including files this component did not create, causing unrelated user data to be silently deleted.
Suggested change
| if (inScope) { | |
| current.push({ file, size, mtimeMs }); | |
| continue; | |
| } | |
| await this.remove(file, size, false); | |
| if (inScope) { | |
| current.push({ file, size, mtimeMs }); | |
| continue; | |
| } | |
| if (!name.startsWith(FILE_PREFIX)) continue; | |
| await this.remove(file, size, false); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/node/pl-middle-layer/src/middle_layer/tree_snapshot_store.ts
Line: 366-371
Comment:
**Eviction deletes foreign files**
If `treeSnapshotPath` is configured as an existing or shared directory, startup eviction passes every regular file outside the current snapshot scope to `remove()`, including files this component did not create, causing unrelated user data to be silently deleted.
```suggestion
if (inScope) {
current.push({ file, size, mtimeMs });
continue;
}
if (!name.startsWith(FILE_PREFIX)) continue;
await this.remove(file, size, false);
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
| const stored = bytes.subarray(payloadStart, payloadStart + payloadLength); | ||
| if (crc32(stored) !== checksum) return failure("checksum"); | ||
|
|
||
| payload = (flags & FLAG_COMPRESSED) !== 0 ? await inflateAsync(stored) : stored; |
Contributor
There was a problem hiding this comment.
A damaged or locally replaced snapshot with a checksum-consistent, high-ratio compressed payload is fully inflated before structural validation, allowing project open to consume process-scale memory and become unresponsive or terminate. Apply an application-level output limit appropriate for the maximum valid snapshot size.
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/node/pl-tree/src/persisted_tree.ts
Line: 415
Comment:
**Inflation lacks an output bound**
A damaged or locally replaced snapshot with a checksum-consistent, high-ratio compressed payload is fully inflated before structural validation, allowing project open to consume process-scale memory and become unresponsive or terminate. Apply an application-level output limit appropriate for the maximum valid snapshot size.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Startup eviction deleted every file in the snapshot directory that was not addressed to the current scope, so a `treeSnapshotPath` pointed at a shared or pre-existing directory lost unrelated files. It now applies the same ownership rule `purge` already documented, shared as `isOurFile`. Decoding also inflates under a 512 MB ceiling, so a replaced file whose checksum-consistent payload has a huge compression ratio costs a cold open instead of the process's memory.
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.
MILAB-6705: disk-persist the tree mirror (middle layer)
Implements the middle-layer half of MILAB-6705, specified in
docs/text/work/projects/disk-persist-tree/spec.md.Reopening a project should transfer what changed while the user was away, not the tree
again. On the reference project (7,240 resources, 10.4 MB) a cold open moves 10.6 MB in two
stream rounds; a warm reopen should sit near the 25.8 KB steady-state floor in one round.
No backend change:
constructTreeLoadingRequestalready builds its request from tree statealone, seeding the non-final frontier and passing everything final as a skip set.
Derivations from the spec
Four decisions were taken during implementation that refine or contradict what the spec
atoms say. The spec set was deliberately not updated, so they are recorded here.
1. The session witness is the root's signature bytes, not the session id.
work/atoms/150-session-bound-snapshot.mdand250-snapshot-content.mdsay the snapshotstores the session id, compared on load. It stores the root resource's signature bytes
instead, and compares those against the signature of the freshly resolved root.
A signature is an HMAC over the id, the session and the colour, so byte equality means
every other signature in the file is still live, and inequality means they are all dead.
Same guarantee, and it needs no
GetSessionInfoRPC: that call exists onLLPlClientbutPlClientcaches onlyrolefrom it, behind apublicGrants:v1capability gate, so thesession-id route meant a pl-client change plus a fallback for backends predating the RPC.
It is also strictly stronger. A rotated master secret changes the signatures while the
session id is unchanged, so the session-id key would miss it and leave the case to
failsafe-cold-retry. The witness catches it on the key.2. Invalidation keys on a build-time stamp, not a package version.
350-cache-key-and-eviction.mdkeys on the package version. It uses a stamp injected atbuild time through rolldown's
defineinstead: the git sha when the worktree is clean, plusthe build timestamp when it is dirty.
The atom names its own weakness, that local development can edit the pruning rules without
a version change and hit a stale cache. The dirty-worktree component closes that: iterating
on those rules produces a new stamp on every build. Under
USE_SOURCES=1the build neverruns, so the constant falls back to a per-process random value and the cache simply always
misses, which is the safe direction.
3. The stamp lives in pl-middle-layer, not pl-tree.
The atom's rationale for keying on a version is that it stands in for "the pruning function,
the field filter, the traversal stop rules, the finality predicate". All four are ML's:
project.tspassesprojectTreePruning,projectTreeFieldFilterandprojectTreeTraverseStopRulesinto the tree.A stamp in pl-tree would therefore not invalidate on the changes the atom is actually
worried about. A stamp in ML covers both packages, because
updateInternalDependencies: "patch"propagates every pl-tree bump into ML. pl-tree keeps a hand-maintained codec schemaversion in the file header, but for a different job: letting a decoder recognise a format it
cannot read. Invalidation is the stamp's; format rejection is the schema version's.
4. The kill switch is a desktop setting.
The scope atom asks only that a setting exist as an operational kill switch, default on. It
is wired as a settings toggle following
traversalModeend to end, rather than an env var.Notes for review
Startup eviction drops other backends' and users' snapshots.
350-cache-key-and-eviction.mdsays to drop entries whose backend, user, build or schema version is not current, and that is
what it does. The consequence worth being explicit about: someone alternating between a local
backend and a cloud one, or between two accounts, has each connection wipe the other's
snapshots at startup, so those reopens are always cold. Keeping them would mean bounding them
by size alone. The atom's wording is unambiguous, so it is implemented as written.
An idle warm reopen writes nothing, not once.
450-acceptance-scenarios.mdasks forexactly one write on an open-and-idle project. That holds for a cold open. For a warm reopen
where the first refresh changes nothing, the change gate correctly suppresses the write
entirely, since the file on disk already matches the tree. The scenario test asserts at most
one.
PlTreeState.dumpState()deliberately reads through an invalidated tree, which the specflags as a coincidence rather than a contract.
captureTreeStatenow refuses an invalidatedtree via a new
PlTreeState.isValidgetter, so "capture before teardown" is enforced ratherthan assumed.
pl-client's lint rules carry an explicit
no-restricted-syntaxguard whose message reads"resource signatures are ephemeral and change from client session to session (never persist
them)". This change persists them on purpose. The witness and the global-id / signature
split are what make that safe, and the codec goes through the typed
createSignedResourceIdAPI rather than casting, so the guard is respected rather thanworked around.
Testing
Two things the scenario tests forced out, both worth a look:
hitswas not proof of a warm reopen. It counts a snapshot read and accepted by the witnesscheck, but the tree can still refuse to apply it, and
loadProjectTreewas reportingrestored: truewheneverSynchronizedTreeState.initdid not throw. So the flag could claimthe file on disk described the tree in hand when the open had actually been cold.
SynchronizedTreeState.wasRestoredFromSnapshotnow reports what really happened, the middlelayer reads that, and a separate
restorescounter sits next tohits.The reopen assertions were checked by breaking
restore()to return false and confirming theyfail, rather than by trusting that they pass.
The contract test is the one the spec names as first to write and fastest to keep: dump,
encode, decode, restore into a fresh tree, then assert the loading request built from the
restored tree equals the original's, seed for seed and skip for skip. It needs no backend.
Alongside it: finality is proven to be recomputed rather than read from the file (a
predicate that settles nothing yields a tree that skips nothing, from a file that says
otherwise); every truncation point of a valid snapshot is checked to fail cleanly rather
than throw; a damaged payload fails its checksum; an unknown schema version loads as absent;
and a snapshot with a dangling reference is proven to leave the live tree untouched.
Greptile Summary
The PR adds a versioned, compressed disk snapshot codec to pl-tree and integrates snapshot restore, periodic/close writes, eviction, observability, and a kill switch into pl-middle-layer. Important touched terms:
Confidence Score: 4/5
The pull request should not merge until startup eviction is restricted to files owned by the snapshot store, because an overridden path can currently cause unrelated file deletion.
Snapshot persistence is broadly fail-safe, but TreeSnapshotStore startup housekeeping applies its destructive removal path to every foreign regular file in a caller-configurable directory; compressed decoding also needs non-blocking resource-exhaustion hardening.
Files Needing Attention: lib/node/pl-middle-layer/src/middle_layer/tree_snapshot_store.ts; lib/node/pl-tree/src/persisted_tree.ts
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Open project] --> B[TreeSnapshotStore reads keyed file] B --> C{Header and witness valid?} C -- No --> D[Cold SynchronizedTreeState initialization] C -- Yes --> E[Decode and restore PersistedTree] E --> F{Restored tree initializes?} F -- No --> G[Optional discard and cold retry] F -- Yes --> H[Warm incremental backend refresh] D --> I[Live project tree] G --> I H --> I I --> J{Generation changed?} J -- Yes --> K[Periodic or close-boundary capture] K --> L[Atomic snapshot write] J -- No --> M[Skip redundant write]Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "MILAB-6705: do not block a project close..." | Re-trigger Greptile
Context used (3)