retoorandClaude Sonnet 5
0c0ae4f747
CI / build-and-test (push) Failing after 23s
The real Gitea Actions run on this project's own registered runner just failed exactly as CONTRIBUTING.md already anticipated it might: FATAL: ThreadSanitizer: unexpected memory mapping, the same signature already documented as a sandbox/container seccomp restriction blocking personality(ADDR_NO_RANDOMIZE), which TSan needs to start at all. Build, test suite, and ASan/UBSan all passed -- only the TSan step failed, on an environment issue, not a code issue. Fixed the CI step itself rather than just noting the failure: it now classifies each binary's TSan run as PASS, a known flake (that exact FATAL signature and nothing indicating an actual race was found), or a real failure (anything else -- a genuine data race, a crash, any other error). Only a real failure fails the build. Verified the classification logic locally against four cases (a synthetic real race report, a plain assertion failure, the known flake signature alone, and a clean pass) -- each classified correctly -- and against this sandbox's own six test binaries, all six of which hit the real flake (this sandbox has never been able to run TSan either) and correctly did not fail the build. This does NOT mean TSan verification is happening in CI -- it means CI no longer conflates "TSan couldn't start" with "the build is broken." Updated CONTRIBUTING.md and CLAUDE.md from "whether the runner can execute TSan is unverified" (a hedge) to the now-confirmed fact that it can't, and corrected an overclaim in README.md that the suite is "regularly run under ThreadSanitizer" -- as far as this project has been able to confirm, TSan has not actually completed a run in any environment it's been built in yet, local sandbox or CI. ASan/UBSan remain the real, run, load-bearing sanitizer coverage. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UqJpkdJ6Njnt1pw3CbghzB
130 lines
7.5 KiB
Markdown
130 lines
7.5 KiB
Markdown
# Contributing to PackFS
|
||
|
||
## Before changing anything
|
||
|
||
Read [`concept.md`](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`](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`](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
|
||
|
||
```sh
|
||
make test # must pass before any PR
|
||
cc ... -fsanitize=address,undefined # ASan/UBSan: see .gitea/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 (`.gitea/workflows/ci.yml`, Gitea Actions — this project is
|
||
hosted on Gitea, not GitHub) runs on this project's own self-hosted
|
||
runner. **Confirmed, not just anticipated: that runner cannot run TSan
|
||
either.** The first real CI run on it failed the ThreadSanitizer step with
|
||
the exact same signature described above (`FATAL: ThreadSanitizer:
|
||
unexpected memory mapping`), consistent with the runner's job containers
|
||
using a default seccomp profile that blocks `personality()` the same way
|
||
some local sandboxes do. The CI step now detects this specific failure
|
||
signature and does not fail the build over it (while still failing hard on
|
||
any *other* TSan outcome — a real race, a crash, anything without that
|
||
exact signature) — see the step's own comment in `ci.yml` for the
|
||
detection logic. This means **CI's ThreadSanitizer step passing is not
|
||
evidence that TSan actually ran** on a given push; it may just mean the
|
||
runner couldn't start it and the step correctly didn't treat that as a
|
||
failure. Treat a change as TSan-verified only once it has actually passed
|
||
on a machine confirmed able to run it (a local machine or container with
|
||
`personality(ADDR_NO_RANDOMIZE)` available), not merely because CI is
|
||
green.
|
||
|
||
**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.
|