Repository navigation
Layout F: fixes from #61's review - #62
Merged
Merged
Conversation
- reclaim: packs being rewritten stay listed (moved to the manifest) until their rewrite swaps them out - a commit's held runs are named once: a retry of assembly doesn't append them again - untrusted repositories check every packed record read (linesAt, entryLine, streams) - sparse merges read old records a batch of changes ahead, not to the leaf's last key - layer range reads find pages by binary search - a sink's delta base is for the rewritten leaf alone - mirrors get delta-stored nodes in full - the maintain lease is released only by its holder - KeyedLayout.mirrorDroppable: which files a mirror may delete
- mirrors delete only their own collection's files, and none a live fork reads - forks of a collection on another storage location are refused for F too - F's repositories are kept for the 256 most recently used collections
isTravis
marked this pull request as ready for review
October 20, 2026 17:19
…hich depends on what the last maintain wrote)
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
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 for the correctness findings from the review of #61 (layout F). Each has a test that fails on
mainand passes here. All ten findings, and the lower-priority lease one, turned out to be real; none skipped.Reclaim left packs being rewritten unlisted (
keyed.tsreclaimSwap). Registry packs chosen for rewrite were dropped from the registry beforereclaimRewriteran, so for that window the index pointed at packs that nothing listed: a reclaim planned then dropped their live entries,forkIntoleft them out of the fork's list, andmirrorFilesleft them out of the mirror. Fix: the swap moves them into the manifest's packs (a newrelistoption on#swap) and only then rewrites the registry without them. They stay listed until their rewrite swaps them out as ordinary listed packs. During the brief overlap a pack can be in both the registry and the manifest, soallPacks,reclaimPlan,forkIntoandmirrorFilesnow read the registry through#registered(m), which drops packs the manifest already lists. Test: the existing reclaim test (registry-packs case) now checks, between the swap and the rewrites, that every pack being rewritten is listed exactly once.Mirrors deleted files another placement at the same location still uses (
mirror.tscopyRecordFiles). A fork's mirror deleted the parent's files underruns/<parent>/once the fork merged them away, even though the parent's mirror at that location still used them. The reverse also happened: the parent's mirror deleted files the fork's mirror still read. Fix: a newKeyedLayout.mirrorDroppabledecides what a mirror may delete. Keys under another collection's prefix are forgotten rather than deleted, since they belong to that collection. The collection's own files that a live fork reads (forkHeld) are kept and looked at again on the next run. Everything else is deleted as before. Test:mirrors.test.ts, "keeps the files a fork and its parent share, mirrored to one location, when the fork / parent merges them away". It mirrors a parent and its fork to one location, has one side commit until a pack merge takes the shared packs, then reads both mirrors from the bucket alone. Onmainthis fails with "Missing layer directory" or "has no stored record".Mirrors got delta-stored leaf nodes without the node they're a delta against (
sync.tstreeObjectswithoutbodies). The base node can be another version's leaf, a fork parent's leaf, or a private leaf that a public mirror never receives. Fix: a leaf node stored as a delta is always sent in full (expanded and re-gzipped), whatever the path. Copying the base node instead was rejected because, for a type that moved from private to public, it would hand a private leaf's entries to a public mirror. Cost: a mirror stores rewritten leaves in full, roughly 4× the delta (a delta is kept only while under a quarter of the full node), plus one gunzip per leaf node sent. Test: "node deltas › go to a mirror in full": a delta leaf sent with no base comes out matching its hash.Cross-location forks weren't refused for F (
fork.ts). For F thetargetRepo !== repocheck was skipped, since F opens one Repo per collection, yet the fork is always created on the platform location. Fix: before any rows are written, look up the source's primary placement and refuse the fork unless that location is the platform location. This replaces the instance comparison, so it covers both layouts and no longer leaves orphan rows behind. Test: serverlayouts.test.ts, "forks › are refused for a collection on another storage location" (a second platform-kind location). Onmainit fails in thefproject.An assembly retry could fold a commit's runs in twice (
kit.ts#withPending,parallel.ts). Assembly callsappendRuns, then saves the tree. If it dies in between, the retry wrote a second pending object. If a merge had already folded the first copy in and retired its layers, the second copy was folded in again as the newest run, pointing at files swept since. Fix:ManifestStore.appendRunswrites nothing if the runs are already named. They count as named when the manifest view (pending objects included) lists any of their layers or has retired one, or when the first layer's directory file is gone (swept: a unit's layer files are deleted no other way). This holds because a tree's runs always go into one object, all or none. Pending keys are still time-ordered, as before. The suggested fix (a deterministic key written withifAbsent) wasn't used: once the first object has been folded and deleted, the retry'sifAbsentwrite succeeds again, so it doesn't close the hole. Cost: one manifest read and one HEAD per tree appended. Test: "a commit's held runs › are named once, however often a retry appends them" retries before folding, then again after the run is merged away and swept.Untrusted repositories didn't hash-check every packed read path (
repo.ts). OnlybodyLineschecked records. Fix: a shared#checkPacked(hash, size, id prefix) now runs onlinesAt,entryLineandbodyLines. That coversbodyLineStream's packed branch andRepoSource.payloadsAt/withPayload, which go throughlinesAt/entryLine. Test: "an untrusted repository in layout F" tampers with one stored record and expects every read path to throwIntegrityError.Sparse merges buffered unbounded look-ahead (
merge.tsoldBodies). The look-ahead held every change up to the leaf's last key, andArray.shiftmade draining O(n²). Fix:Peekablekeeps its put-back buffer with a read index instead of shifting, andlookAheadtakes amax. A sparse leaf now fetches old records lazily: when a change needs one that hasn't been fetched yet, it fetches that record together with those for the next 1,024 changes (OLD_BATCH) in onepayloadsAt. Test: "a sparse merge" sends 20,000 inserts into one leaf's range. Before the fix the stream ran 20,000 changes ahead of what was applied; now it stays under 2,000.LayerReader.rangescanned pages linearly (kit.ts). Fix: a shared binary search (pageAt), now used bylookup,rangeandall's start page. Test: "a layer range read" counts reads of the directory's first keys on a 266-page layer: 266 before, under 40 now.forCollectionRunscached a Repo per collection forever (stores.ts). Fix: F's repositories now live in their own map used as an LRU, capped atcollectionRepos.max(256): a hit moves to the end, and the oldest is evicted. Test: serverlayouts.test.ts, "layout F's repositories › are kept for the most recently used collections only".RepoSink's delta base was never cleared (repo.ts). The base set byrewriting()stuck around, so later leaves computed a useless delta against an unrelated leaf. Fix:leaf()clears the base after using it. Test: "node deltas › diff a rewritten leaf against the leaf it replaces, and no later leaf".Lower priority: releasing the maintain lease (
keyed.ts). A job whose lease had been taken over deleted the new holder's lease when it released. Fix: release reads the lease and deletes it only if it still holds this job's token. Taking over a stale lease does the same with the stale token. The Store API has no conditional delete, so a takeover between that read and the delete can still lose the lease, but the window is now one request rather than the rest of a job. Test: "the maintain lease › is left alone when another job took it over".Tests: protocol 253 (was 246); server 566 (was 558; the suite runs once per layout,
todayandf).pnpm fmt,pnpm lintandpnpm typecheckare clean.