pgw#1511 + pgw#1513: verify residency before asserting it, and refuse a PROJECTED tree by name (FLEET-BLOCKING) - #1072
Conversation
3257183 to
1682c1d
Compare
8677a9c to
c95abf2
Compare
…before asserting, quarantine when it fails
FLEET-BLOCKING. Two endpoints (sd15, sdxl), two volumes, 20x apart in size,
identical `SafetensorError: header too large` on read, hub source bytes
verified intact, exposure localized to `120ac7e2` (pgw#1490).
THERE IS NO SHORT WRITE. I read every write path before changing anything, and
each one is already airtight:
* CAS commit (`local.open_writer` / `adopt_file` / `put_file`): hashes the
stream, compares digest AND size, `fsync`s, and only then `_commit_temp`
hard-links it into place. Any exception unlinks the temp. A short object
cannot be committed.
* tensorfs `_materialize_unlocked`: re-verifies each source object
(`_verify_object_unlocked` = size + full sha256), re-hashes every byte as
it copies, refuses a short read (`object ended before N bytes`) AND a long
one, checks total size and whole-file digest against the manifest,
`fsync`s, `os.replace`s, `_fsync_dir`s, and unlinks on any failure.
* `project_snapshot`: builds under a private scratch name and renames, so no
reader sees a half-built tree.
So the coordinator's four candidates (completion before flush/fsync,
non-atomic finalize, cancellation mid-stream, resume-from-partial) are all
already handled ON THE WRITE SIDE. The defect is the fourth one's second half
— "a partial file that a LATER BOOT TREATS AS PRESENT" — and the treating, not
the writing, is where the hole was.
`announce_resident` decided residency from a DIRECTORY ENTRY:
for key in (snapshot_dir_key(digest), snapshot_dir_key(bare)):
candidate = root / key
if candidate.is_dir():
tree = candidate
That was the whole test. It then published `already_resident` on the
weight_fetch stream, emitted ON_DISK, and — worst — did `self._verified.add(ref)`,
which permanently suppresses `_materialize_local`'s first-use
`_verify_snapshot_tree` for that ref for the life of the process. So nothing
checked the bytes, ever. pgw#1490 is what made this fire at BOOT for every
configured ref and made READINESS depend on it, which is why the exposure
window opens at `120ac7e2`; the unverified assertion itself came in with
pgw#1052's join, and I wired it without questioning it. Mine either way.
THE FIX: verify before asserting. A tree that does not match its manifest is
QUARANTINED (`_quarantine_snapshot` deletes the partial and its bad blobs, so
recovery re-downloads instead of re-linking the same bytes) and answered
False, which sends the caller down the ordinary fetch path. A residency answer
that cannot be substantiated never becomes an answer.
STUB-AWARE BY CONSTRUCTION, which is why this reuses the inherited
`_verify_snapshot_tree` instead of adding a validator. A projected tree's
tensor containers are ~128 B TFSSTUB1 stubs and its other files are CAS
symlinks; opening one naively produces a loud parse failure that is CORRECT,
and scoring that as corruption is pgw#1308 finding 3 (two callers read the
same correct failure and reached opposite wrong conclusions — one deleted the
model every boot). Stubs and symlinks go to `verify_projection` (structural);
only files holding real bytes are hashed. This is therefore not a re-download
tax on warm pods. The test proves the stub path is live: the harness projects
125-byte stubs, and the recovered tree is judged by the same verifier rather
than by file size.
Also binds the store's loop before the quarantine, so the EVICTED event for a
tree this pod just disowned actually reaches the hub instead of being dropped.
Red/green, $0, no GPU, no pod (`test_a_TRUNCATED_warm_tree_is_refused_quarantined_and_refetched`):
stage a good tree, truncate one weight file the way an interrupted write
leaves it (the tree's files are read-only hard links INTO the CAS objects, so
this damages the object itself — a faithful at-rest corruption, and recovery
must genuinely re-download), then boot a second store on that CAS.
fixed | refused, quarantined, re-fetched, ends STATE_READY on a tree that
| passes verification
RED | `AssertionError: a truncated tree was answered as ALREADY RESIDENT
| ... positions=['already_resident']` — the incident, exactly
The red arm restores the landed `is_dir()`-only assertion and flips a
CONDITION, never cuts lines.
Neighbours green: test_weight_position, test_model_residency,
test_store_corruption_pgw1283, test_boot_materialize — 38 passed. ruff clean;
whole-tree mypy clean (613 files, run with MYPYPATH so it does not resolve
against the canonical checkout's stale src).
Carried, because master is red for every lane and this cannot wait behind it:
three `F541` f-strings-without-placeholders in `serving/mint_store.py`
(another lane's file, pre-existing at origin/master, `ruff check src/gen_worker`
is a CI step). Autofixed, no behaviour change.
… large` was never about the checkpoint
FLEET-BLOCKING, and the message it replaces cost this investigation two days
pointed at poisoned volumes and truncated downloads.
THERE IS NO SHORT WRITE. Every write path was read before anything was
changed, and each is already airtight: the CAS hashes and size-checks every
object before committing it (`open_writer`/`adopt_file`/`put_file`), tensorfs'
materializer re-hashes each object AND the whole file and refuses a short read,
`project_snapshot` builds in a scratch directory and renames, and the endpoint
volume is verified in BOTH directions (`fill.verify_object` on read,
`put_file` on write-through).
WHAT IT ACTUALLY IS. A projected tree's tensor containers are ~128 B TFSSTUB1
pointer stubs; the weights live in the CAS and are read by the pgw#1380
streaming engine. The eager `from_pretrained` bridge is documented for "a tree
with no chunk store behind it — a bare download, a local run, a fixture", and
it reads with the stock safetensors reader, which knows nothing about stubs. It
takes the stub's first eight bytes as a header length and raises
`SafetensorError: header too large`.
Executed, both field shapes:
sd15-unet.safetensors: stub 128 B on disk, names 3,400,000,000 B -> header too large
sdxl-unet.safetensors: stub 129 B on disk, names 68,000,000,000 B -> header too large
A stub is a FIXED SIZE regardless of the model behind it. That is why a 3.4 GB
and a 68 GB checkpoint failed byte-identically across two endpoints and two
volumes — a coincidence no real truncation produces, and the fact that should
have named this on day one.
WHY IT STARTED AT pgw#1490. Before it, `tree_for` required
`ModelBinding.manifest_digest`, which has never had a sender, so it ALWAYS
raised — no v2 dispatch ever reached the loader. Making the dispatch resolvable
made the eager bridge reachable for the first time, so the one reader that
never got a stub-aware path met the tree format it was always going to receive.
WHAT THIS CHANGES
1. THE BRIDGE REFUSES, BY NAME. `ProjectedTreeNotStreamable` names the member,
the bytes on disk, the bytes the stub stands for, and — the part that stops
the next two-day hunt — WHICH of `resolve_projection`'s three silent Nones
fired, with the repair for each: wrong parent directory, missing
refs/objects, or a missing `snapshot:<key>` pin. Reaching the bridge with a
projected tree means the streaming engine declined it, and until now that
fact was unobservable.
2. `announce_resident` REFUSES A TREE NO ENGINE CAN BIND. There is one
byte-perfect state that passes full digest verification and still cannot be
served: the tree is present and its manifest pin is absent. Answering
`already_resident` there strands the pod in exactly the failure above.
Answering False sends it through `ensure_local`, which re-pins and — because
`_tree_matches` passes — returns the SAME tree. A missing pin is REPAIRED,
NOT RE-DOWNLOADED; no bytes move.
WHAT THIS DELIBERATELY DOES NOT DO. No hydration. Filling tensor bytes into a
real file so `from_pretrained` can mmap it is ruled out by DESIGN-RULINGS
("serving path never materializes tensor bytes") and would delete the very
signal that reveals the bug, leaving a silent perf/VRAM regression instead of a
loud stop. The detector branches on `read_stub()` BEFORE any parse, never on a
`SafetensorError` — pgw#1308's two wrong callers both inferred a fact about the
WORLD from a fact about a READER, and "on SafetensorError, hydrate and retry"
is that mistake wearing a repair. Single-object pointer routes are dead by
arithmetic (MAX_OBJECT_SIZE = 64 MiB; real containers are 4, 18, 51, 63
objects), so there is ONE mechanism, not two.
HONEST LIMIT, stated because a re-pin depends on it: I have NOT reproduced the
field condition on the real dispatch path. Probed after a real boot
materialization, the path the dispatch hands the loader is
`<cas>/snapshots/<key>` with pin present, `resolve_projection` OK, `store_for`
OK, engine WOULD bind — the healthy control. So the pin-missing state fixed
here is a real hole reachable when a tree outlives its pin, but it is not
proven to be the field cause. What this change guarantees is that the next
occurrence reports which condition failed in one line instead of lying about
the checkpoint.
Red/green, $0, no GPU, no pod:
both field shapes refused by name, loader never called | pass | RED: SafetensorError,
| | header too large (x2)
the fixed-size-stub signature across a 20x model | pass | —
a MATERIALIZED tree still takes the eager bridge | pass | — (the local-dev control)
a tree with a MISSING PIN is not answered as resident | pass | —
The red arm restores the landed stub-blind bridge and flips a CONDITION, never
cuts lines. Neighbours green: test_boot_materialize, test_weight_position,
test_model_residency, test_store_corruption_pgw1283 — 35 passed.
`ruff check src/gen_worker` clean; whole-tree mypy clean (614 files, run with
MYPYPATH so it does not resolve against the canonical checkout's stale src).
Credit: the se#790/anima lane's local repro proved all three
`resolve_projection` conditions produce this symptom independently while the
healthy control binds, which is what turned "make a reader stub-aware" into
"find out why the engine did not bind" — and their pgw#1308 scar tissue is why
this refuses instead of repairing.
…ap heal is cheap Both from the se#790 lane's measurement on a real 5.6 GB `@composed-v3` tree, which found the state I would otherwise have shipped a wrong claim about. 1. A tree's manifest pin is the ONLY GC root its objects have. Drop the pin, run a GC, and all 1,211 objects are deleted (5.6 GB there; 134 GB on H3) while the tree stands: containers still stubs, and every NON-tensor file a dangling symlink — `model_index.json` among them. `skeleton.build` then reports "carries no model_index.json" about a tree that HAS one, which is pgw#1308's shape again: a reader-level fact rendered as a claim about the checkpoint. `_collected_objects()` now detects that and refuses with its own message, CHECKED BEFORE the stub condition so the real cause wins: how many entries and which, that the bytes are GONE and must be RE-FETCHED rather than re-pinned, and that this is not a malformed checkpoint. The false string is quoted in the refusal so anyone grepping it lands here. 2. The "no bytes move" claim on the missing-pin heal is TRUE, but only because of an ORDER that was not written down: `announce_resident` verifies FIRST, so a collected tree fails `_verify_snapshot_tree` (`projection_fault`: "linked object … is absent") and takes quarantine-and-refetch, and reaching the pin check means the objects are present. Cheap repair for the cheap case, full re-fetch for the expensive one, never confused. Now stated. Checked rather than assumed, on their prompting: `_tree_matches` DOES catch the collected state — `projection_fault`'s symlink arm ends in `if not file.exists()`, which follows the link — so the self-heal rebuilds such a tree instead of re-serving it. That was their specific worry and it does not materialize. NOT fixed here, filed instead: `skeleton.build`'s own false refusal on the streaming path, which reads the index before this guard can see it. A separate caller, a separate defect, and it deserves its own issue rather than a bolt-on from an incident lane. 6 tests green, ruff clean, whole-tree mypy clean (614 files).
…ared helper, not two strings The se#790 lane asked for this while taking pgw#1514, and they are right: two hand-written refusals teaching the same lesson is how the pgw#1308 shape reached four callers. They found a FIFTH instance in their own `AnimaDiTComponent.load_config` while documenting the lesson in that same file's module docstring — which is the argument in one line. So the detection and the wording move to `models/projection`: collected_entries(root) -> the projected entries whose CAS object is gone collected_refusal(root, entries) -> the sentence both refusals say `ctx.load`'s eager bridge (pgw#1513) now calls them, and `skeleton.build` (pgw#1514) can call the same pair rather than adding a second source of truth. One of us owns the wording instead of both of us owning half of it. `collected_entries` documents the actual mechanic for the next reader: `Path.is_file()` FOLLOWS the link, which is exactly what collapses 'collected' and 'absent' into one branch and makes a caller report 'carries no model_index.json' about a tree that has one. 6 + 8 tests green, ruff clean, mypy clean (616 files).
…inement) It is the entry whose absence produces the false 'carries no model_index.json', so it is the one a reader most needs in the truncated list the refusal shows — and plain alphabetical order drops it out of that window on any tree with early-alphabet components. se#790 measured it landing 2nd of 3 on a real `@composed-v3` tree by luck of `dit/` sorting first; a tree with a couple of early components would have shown only configs. The tail keeps sorted order so the list stays stable and diffable. Their call, their measurement, my function — asked before touching it, which is the right way round.
pgw#1362 / DESIGN-RULINGS 4.34b, and the guard is right: a filename says WHEN a
test was written; the tree needs it to say WHAT the test exercises. The gate
exists because a cleanup epic cannot outrun its own intake — 41 incident-named
modules removed in one window while other lanes added ~2,700 lines of new ones,
all correct and ruled, and the corpus still grew.
test_projected_tree_eager_refusal_pgw1513.py -> test_projected_tree_reading.py
('Reading a PROJECTED tree: who may, who must refuse, and what they say').
The lineage moves to a one-line comment, the narrative stays in the tracker —
which is exactly the trade 4.34b makes. Baseline untouched: this adds no
grandfathered name and does not raise the cap.
c95abf2 to
ed91c1d
Compare
|
Merging with a TARGETED
That guard is red on master itself, from pgw#1491's Every other guard passes on this branch, run locally after the final rebase: unreached-surface, incident-test-names, mypy ratchet, HTTP timeouts, settings writers, materialization hatch, ruff. Whole-tree mypy clean (616 files). The three This is the legitimate half of the pgw#1521 asymmetry — "red because of somebody else's problem on master" — established by execution against the base rather than asserted, which is the standard that issue now records. It does not fix master: pgw#1491's lane still owns the config-reads classification, and every other lane hits it until they land it. This PR has burned five CI rounds, every one of them on another lane's master-red. |
pgw#1513: the eager bridge must refuse a PROJECTED tree —
header too largewas never about the checkpointFLEET-BLOCKING, and the message it replaces cost this investigation two days
pointed at poisoned volumes and truncated downloads.
THERE IS NO SHORT WRITE. Every write path was read before anything was
changed, and each is already airtight: the CAS hashes and size-checks every
object before committing it (
open_writer/adopt_file/put_file), tensorfs'materializer re-hashes each object AND the whole file and refuses a short read,
project_snapshotbuilds in a scratch directory and renames, and the endpointvolume is verified in BOTH directions (
fill.verify_objecton read,put_fileon write-through).WHAT IT ACTUALLY IS. A projected tree's tensor containers are ~128 B TFSSTUB1
pointer stubs; the weights live in the CAS and are read by the pgw#1380
streaming engine. The eager
from_pretrainedbridge is documented for "a treewith no chunk store behind it — a bare download, a local run, a fixture", and
it reads with the stock safetensors reader, which knows nothing about stubs. It
takes the stub's first eight bytes as a header length and raises
SafetensorError: header too large.Executed, both field shapes:
A stub is a FIXED SIZE regardless of the model behind it. That is why a 3.4 GB
and a 68 GB checkpoint failed byte-identically across two endpoints and two
volumes — a coincidence no real truncation produces, and the fact that should
have named this on day one.
WHY IT STARTED AT pgw#1490. Before it,
tree_forrequiredModelBinding.manifest_digest, which has never had a sender, so it ALWAYSraised — no v2 dispatch ever reached the loader. Making the dispatch resolvable
made the eager bridge reachable for the first time, so the one reader that
never got a stub-aware path met the tree format it was always going to receive.
WHAT THIS CHANGES
ProjectedTreeNotStreamablenames the member,the bytes on disk, the bytes the stub stands for, and — the part that stops
the next two-day hunt — WHICH of
resolve_projection's three silent Nonesfired, with the repair for each: wrong parent directory, missing
refs/objects, or a missing
snapshot:<key>pin. Reaching the bridge with aprojected tree means the streaming engine declined it, and until now that
fact was unobservable.
announce_residentREFUSES A TREE NO ENGINE CAN BIND. There is onebyte-perfect state that passes full digest verification and still cannot be
served: the tree is present and its manifest pin is absent. Answering
already_residentthere strands the pod in exactly the failure above.Answering False sends it through
ensure_local, which re-pins and — because_tree_matchespasses — returns the SAME tree. A missing pin is REPAIRED,NOT RE-DOWNLOADED; no bytes move.
WHAT THIS DELIBERATELY DOES NOT DO. No hydration. Filling tensor bytes into a
real file so
from_pretrainedcan mmap it is ruled out by DESIGN-RULINGS("serving path never materializes tensor bytes") and would delete the very
signal that reveals the bug, leaving a silent perf/VRAM regression instead of a
loud stop. The detector branches on
read_stub()BEFORE any parse, never on aSafetensorError— pgw#1308's two wrong callers both inferred a fact about theWORLD from a fact about a READER, and "on SafetensorError, hydrate and retry"
is that mistake wearing a repair. Single-object pointer routes are dead by
arithmetic (MAX_OBJECT_SIZE = 64 MiB; real containers are 4, 18, 51, 63
objects), so there is ONE mechanism, not two.
HONEST LIMIT, stated because a re-pin depends on it: I have NOT reproduced the
field condition on the real dispatch path. Probed after a real boot
materialization, the path the dispatch hands the loader is
<cas>/snapshots/<key>with pin present,resolve_projectionOK,store_forOK, engine WOULD bind — the healthy control. So the pin-missing state fixed
here is a real hole reachable when a tree outlives its pin, but it is not
proven to be the field cause. What this change guarantees is that the next
occurrence reports which condition failed in one line instead of lying about
the checkpoint.
Red/green, $0, no GPU, no pod:
both field shapes refused by name, loader never called | pass | RED: SafetensorError,
| | header too large (x2)
the fixed-size-stub signature across a 20x model | pass | —
a MATERIALIZED tree still takes the eager bridge | pass | — (the local-dev control)
a tree with a MISSING PIN is not answered as resident | pass | —
The red arm restores the landed stub-blind bridge and flips a CONDITION, never
cuts lines. Neighbours green: test_boot_materialize, test_weight_position,
test_model_residency, test_store_corruption_pgw1283 — 35 passed.
ruff check src/gen_workerclean; whole-tree mypy clean (614 files, run withMYPYPATH so it does not resolve against the canonical checkout's stale src).
Credit: the se#790/anima lane's local repro proved all three
resolve_projectionconditions produce this symptom independently while thehealthy control binds, which is what turned "make a reader stub-aware" into
"find out why the engine did not bind" — and their pgw#1308 scar tissue is why
this refuses instead of repairing.
This PR carries two fixes. First commit is pgw#1511 (a tree that EXISTS is not a tree that is RESIDENT — verify before asserting, quarantine when it fails); second is pgw#1513 above, which is the one that gates the rentals.
Needs pgw#1073 (pgw#1512) for a green base — pgw master fast gates is red for every lane on an unrelated ruff F541 and on nine unwired gguf_torch callables from pgw#1498.
🤖 Generated with Claude Code