Files

T

retoorandClaude Sonnet 5 684c6d95de Fix pack_write's O(n^2) dedup + correctness bug; document mount table O(n^2)

A audit for other instances of the file-index O(n^2) shape (fixed
previously via the persistent treap) found two more real issues:

1. pack_write's Section 9.2 exact-duplicate elimination was a linear scan
   of every previously-seen (hash, size) pair per entry -- O(n^2) total,
   invisible in the existing benchmark because its identical-content test
   files made every scan match on the first comparison. Measured with
   unique content instead: 80,000 entries took 1.74s, with a 20,000->80,000
   step showing 15.6x for a 4x-N step, matching O(n^2)'s 16x prediction.
   The same scan also trusted a (hash, size) match without ever comparing
   actual bytes -- a latent correctness bug, since FNV-1a64 is explicitly
   not collision-resistant. Fixed both at once with an open-addressing hash
   table (load factor 1/2, linear probing) plus a memcmp verification
   before ever reusing a data_off. Post-fix: 80,000 entries in 0.044s
   (39.6x faster), ratio drops to 3.35x (consistent with O(n)).

   Covered permanently by two new/extended tests: a white-box assertion in
   test_pack_overlay.c that duplicate-content entries share one data_off
   and distinct-content entries do not, and a new
   tests/test_pack_write_perf.c regression tripwire against 10,000 unique
   entries.

2. vfs.c's mount table uses the same full-array-copy-per-write pattern the
   file index used to, confirmed O(n^2) via a new bench/bench.c category
   (500/2,000/8,000 mounts, both 4x-N steps showing 15-20x). Deliberately
   NOT rewritten: mount points are created by a program's own source code,
   not workload-driven, so realistic mount counts never reach the scale
   that made the file index's O(n^2) a real problem. Documented with full
   reasoning in BENCH.md and CLAUDE.md rather than silently left as an
   undocumented gap.

Also fixes a real CI gap the new tests exposed: ci.yml's sanitizer-build
steps never passed -D_GNU_SOURCE when compiling test files (only the
library .o's got it), which was harmless while no test included
internal.h and became a link failure once two did (internal.h needs
_GNU_SOURCE for pthread_rwlock_t). And documents, in CONTRIBUTING.md and
CLAUDE.md, a sandbox flake observed directly during this work's own
sanitizer runs: ASan/UBSan test binaries occasionally fail to start with
AddressSanitizer:DEADLYSIGNAL (sometimes looping rather than exiting),
non-deterministically hitting different unrelated binaries across runs --
a startup race, not a memory-safety bug, confirmed by clean passes on
retry; sanitizer runs in such an environment should be timeout-wrapped.

BENCH.md's "After" table and Appendix B are replaced with the current,
complete 54-measurement bench/bench.c run (the original 45 plus the new
mount-scaling category); the pre-fix 45-measurement "Before" table is kept
as the historical record, per this project's documentation standard.

Verified: make test (all 6 binaries, including the 2 new/changed), a clean
make all, and repeated ASan+UBSan runs (0 real findings; the DEADLYSIGNAL
flake above was observed and correctly distinguished from a real finding
by re-running until a clean pass). TSan could not be run in this sandbox
(pre-existing, documented environment limitation).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UqJpkdJ6Njnt1pw3CbghzB

2026-09-14 09:55:47 +00:00

6.5 KiB

Raw Blame History

Contributing to PackFS

Before changing anything

Read concept.md in full. It is the project's specification, not background reading — every backend, lock, and on-disk field in src/ exists because a section of concept.md requires it, and most functions' comments cite the section they implement rather than re-explaining it. concept.md is frozen (see CLAUDE.md): if you believe the design itself needs to change, that belongs in a new document that amends or supersedes it, not in an edit to concept.md.

Then read CLAUDE.md, which states the load-bearing constraints a change must respect (static linking, the write model, the concurrency model, path containment, pack integrity) and the documentation register this project is written in.

Workflow

make test                              # must pass before any PR
cc ... -fsanitize=address,undefined    # ASan/UBSan: see .github/workflows/ci.yml for exact flags
cc ... -fsanitize=thread               # TSan, for anything touching src/upper.c, src/overlay.c, or src/vfs.c

Any change to the concurrency-sensitive files (upper.c, overlay.c, vfs.c) must be run under ThreadSanitizer, not just the plain test suite — a data race there is exactly the class of bug Section 5 of concept.md exists to prevent, and the plain build will not surface it.

Known environment limitation, stated plainly rather than glossed over: ThreadSanitizer cannot run at all in some sandboxed/containerized development environments — including the one this project's own commit history was largely developed in — because TSan requires disabling ASLR for itself via personality(ADDR_NO_RANDOMIZE), and some sandboxes block that syscall outright (confirmed here: personality() returns EPERM, and even a trivial unrelated pthread program fails identically with FATAL: ThreadSanitizer: unexpected memory mapping, not just PackFS code). If your environment can't run TSan, say so rather than silently skipping it or claiming verification that didn't happen — ASan/UBSan still catch real bugs (they found and fixed a genuine heap-use-after-free during this project's development, see the reclaim_gate note in internal.h) but do not do TSan's happens-before race analysis, so they are not a substitute for it. CI (.github/workflows/ci.yml) runs on a normal, unrestricted GitHub Actions VM where TSan is expected to work; treat a change as TSan-verified only once it has actually passed there or on a local machine that can run it, not merely because ASan/UBSan passed.

A second, separate environment quirk, also observed directly rather than assumed: in the same kind of sandboxed environment, an ASan/UBSan-built test binary occasionally (non-deterministically, roughly 1 run in 5–10 in this project's own experience) fails to start at all, printing AddressSanitizer:DEADLYSIGNAL — and, in its worse form, printing that same line in an unbounded loop rather than printing it once and exiting, which can run for minutes and consume unbounded CPU/memory if left unattended. This has been observed hitting different, unrelated test binaries from run to run (in one session: test_dir and test_mem, in another: nothing at all, in another: test_pack_overlay), which — together with the fact that every one of those binaries passes cleanly on a repeat run — indicates a sandbox startup race (plausibly in the same family as the ASLR/mapping restrictions behind the TSan limitation above), not a bug in the binary being tested. Always wrap sanitizer-build test runs in timeout in such an environment (e.g. timeout 30 ./build/san/test_name) so a stuck run fails loudly and bounded instead of hanging; treat a DEADLYSIGNAL exit as inconclusive, re-run, and only treat the run as a real finding if the output contains an actual ERROR: AddressSanitizer or runtime error: string — DEADLYSIGNAL alone, with neither of those strings present, is this flake, not a memory-safety bug in PackFS. As with the TSan limitation, the correct response is to say this plainly and re-run until a clean pass is obtained (or hand off to CI, which runs on an unrestricted VM where this has not been observed), not to suppress or silently ignore a DEADLYSIGNAL exit.

A change to the index structure in upper.c (the TreapNode/treap_*/ node_* functions, snapshot_upsert, snapshot_remove, or the UpperSnapshot/UpperEntry representation) should be run through make bench before and after, not just make test: BENCH.md records a real, measured O(n²) cost the flat array this index used to be had in bulk sequential create/unlink, and the persistent treap that replaced it exists specifically to avoid regressing back into it — a correctness-preserving change to this code can still reintroduce that regression (e.g. an implementation that silently degrades to a linked list under adversarial insertion order) in a way make test alone cannot detect, only make bench plus tests/test_index_stress.c's randomized-order correctness check can.

Scope

Changes that add functionality concept.md Section 10 lists as explicitly out of scope for v0 (full POSIX semantics, enforced permissions/symlinks/hard links, cross-process concurrency, content-defined chunking/delta compression) should discuss the tradeoff with a maintainer first — those exclusions were deliberate design decisions, not gaps waiting to be filled.

zip and tar import/export backends will not be accepted, full stop — not discussed, not behind a flag. concept.md Section 11 recommends them, but that recommendation is permanently superseded; see CLAUDE.md, "Project decisions that supersede concept.md." This is a harder line than the v0 exclusions above, which are open to future discussion — this one is not.

Static-linking constraint

concept.md Section 11.1 is a hard constraint: the core must build with zero required third-party libraries and zero dynamic loading. In practice this project has no optional-dependency boundary at all: concept.md described one at the zip/tar backend for miniz/libarchive, but per the decision above, that backend will never exist, so there is no path by which a third-party dependency enters this codebase — a change proposing one, for any backend, will not be accepted.

Style

Match the register already in the file you're editing — precise, constraint-labeled comments citing the concept.md section they implement, no filler. See the "Documentation standard" section of CLAUDE.md for the full rule; it applies to code comments and commit messages, not only to .md files.

6.5 KiB Raw Blame History Unescape Escape