Skip to content

Layout F: fixes from #61's review - #62

Merged
isTravis merged 3 commits into
mainfrom
layout-f-fixes
Oct 20, 2026
Merged

isTravis merged 3 commits into
mainfrom
layout-f-fixes

Conversation

@isTravis

Copy link
Copy Markdown
Member

Fixes for the correctness findings from the review of #61 (layout F). Each has a test that fails on main and passes here. All ten findings, and the lower-priority lease one, turned out to be real; none skipped.

  1. Reclaim left packs being rewritten unlisted (keyed.ts reclaimSwap). Registry packs chosen for rewrite were dropped from the registry before reclaimRewrite ran, so for that window the index pointed at packs that nothing listed: a reclaim planned then dropped their live entries, forkInto left them out of the fork's list, and mirrorFiles left them out of the mirror. Fix: the swap moves them into the manifest's packs (a new relist option 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, so allPacks, reclaimPlan, forkInto and mirrorFiles now 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.

  2. Mirrors deleted files another placement at the same location still uses (mirror.ts copyRecordFiles). A fork's mirror deleted the parent's files under runs/<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 new KeyedLayout.mirrorDroppable decides 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. On main this fails with "Missing layer directory" or "has no stored record".

  3. Mirrors got delta-stored leaf nodes without the node they're a delta against (sync.ts treeObjects without bodies). 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.

  4. Cross-location forks weren't refused for F (fork.ts). For F the targetRepo !== repo check 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: server layouts.test.ts, "forks › are refused for a collection on another storage location" (a second platform-kind location). On main it fails in the f project.

  5. An assembly retry could fold a commit's runs in twice (kit.ts #withPending, parallel.ts). Assembly calls appendRuns, 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.appendRuns writes 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 with ifAbsent) wasn't used: once the first object has been folded and deleted, the retry's ifAbsent write 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.

  6. Untrusted repositories didn't hash-check every packed read path (repo.ts). Only bodyLines checked records. Fix: a shared #checkPacked (hash, size, id prefix) now runs on linesAt, entryLine and bodyLines. That covers bodyLineStream's packed branch and RepoSource.payloadsAt / withPayload, which go through linesAt / entryLine. Test: "an untrusted repository in layout F" tampers with one stored record and expects every read path to throw IntegrityError.

  7. Sparse merges buffered unbounded look-ahead (merge.ts oldBodies). The look-ahead held every change up to the leaf's last key, and Array.shift made draining O(n²). Fix: Peekable keeps its put-back buffer with a read index instead of shifting, and lookAhead takes a max. 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 one payloadsAt. 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.

  8. LayerReader.range scanned pages linearly (kit.ts). Fix: a shared binary search (pageAt), now used by lookup, range and all'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.

  9. forCollectionRuns cached a Repo per collection forever (stores.ts). Fix: F's repositories now live in their own map used as an LRU, capped at collectionRepos.max (256): a hit moves to the end, and the oldest is evicted. Test: server layouts.test.ts, "layout F's repositories › are kept for the most recently used collections only".

  10. RepoSink's delta base was never cleared (repo.ts). The base set by rewriting() 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, today and f). pnpm fmt, pnpm lint and pnpm typecheck are clean.

- 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
isTravis marked this pull request as ready for review October 20, 2026 17:19
…hich depends on what the last maintain wrote)
@isTravis
isTravis merged commit 6c67a1f into main Oct 20, 2026
1 check passed
@isTravis
isTravis deleted the layout-f-fixes branch October 20, 2026 17:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant