Skip to content

feat(inference): a weights integrity check that re-reads every byte and purges damage - #475

Merged
JArmandoAnaya merged 5 commits into
mainfrom
feat/weights-integrity
Aug 9, 2026
Merged

feat(inference): a weights integrity check that re-reads every byte and purges damage#475
JArmandoAnaya merged 5 commits into
mainfrom
feat/weights-integrity

Conversation

@JArmandoAnaya

Copy link
Copy Markdown
Contributor

download_weights at ready proves a cached snapshot is complete, and it proves that by
consulting the download library's index without opening a file. So a copy that is present and
damaged — truncated by a filesystem error, rotted on a failing disk, edited in place — passes it
for ever, and surfaces much later inside a model load, in a sentence about tensors, on a connection
the row still calls Ready. check_integrity is the second action over the same files, and it
answers by reading every byte.

Implements the Decision (Armando, 2026-08-09) on the issue in full: digest re-read, purge then
not_set_up, separate wire action. Naming was left to the implementer; see below.

Step 0 — what the hub actually publishes, verified before anything was designed

Read off the locked huggingface_hub 1.26.0 (uv.lock; note the ambient conda python on this
box is 1.11.0 and is not what ships) and confirmed against the hub itself on two real repositories.

There is no single digest per file. The hub publishes what its own storage keeps, and that
differs by how the file is stored:

File kind Field What it is
LFS-tracked (model.safetensors, *.pt) lfs.sha256 plain SHA-256 over the contents
everything else (config.json, tokenizer JSON, .gitattributes) blob_id a git object id — SHA-1 over b"blob <len>\0" then the contents

Both kinds appear side by side inside one revision, so the selection is per file and never per
repository. Hashing a config with plain SHA-256 and comparing it to a git OID would fail on a
perfectly intact file, every time, for as long as the check existed — and the remedy it names
deletes the weights. That is the trap the dispatch called out, and it is why PublishedDigest
carries which kind it is rather than just a string.

Measured on facebook/sam2.1-hiera-tiny and hf-internal-testing/tiny-random-gpt2: every file
carries one of the two, so nothing is skipped. A file the hub gives neither for is refused, not
passed
published_digests raises, because reporting "intact" about a snapshot nine files out of
ten were checked in is a guarantee made out of a gap.

Both computations were then reproduced against real downloaded bytes: 3 LFS files by sha256 and 7
non-LFS by git OID, all ten matching.

A second finding, which changed a test. An LFS file's blob_id is the object id of its
pointer text, not of its contents — git only ever stored the pointer. Verified on all three LFS
files of the tiny repo. The first version of the fixture reused the content's OID for both fields,
which would have let the wrong-digest mutation survive; 01c1a03 fixes it, and the mutation is
lethal afterwards (see the table).

Cache mechanics, and what a purge has to reach

The cache is content-addressed: blobs/<digest> with snapshots/<commit>/<name> symlinking into
it. Verified end to end, in this order:

  1. A cache hit is returned unread. Corrupt a blob in place, re-run snapshot_download — the
    corrupt bytes come back. This is the premise the whole decision rests on, now measured rather
    than inferred.
  2. Removing only the symlink is not a purge. Leave the corrupt blob, delete the snapshot entry,
    re-download — the corrupt bytes come back again, because the library re-links a surviving blob
    rather than re-fetching. So a purge must remove the blob and the entry.
  3. A real purge forces a real transfer. Remove both, re-download — correct bytes, and the stale
    trees/<commit>.json the 1.x cache also keeps does not resurrect anything.

Finding 2 is modelled in the test fake (9f6c6d1), which is what makes "purge the symlink only"
a red test rather than a green one.

Naming

check_integrity. The table's grammar is verb + object (download_weights,
create_correction, edit_membership, submit_for_review), and this fits it. verify_weights was
the obvious parallel and is the one thing it could not be called: Verify weights is the existing
UI label for download_weights at ready, so the new action would have taken the old one's name
while meaning something stronger. Naming the property is what separates them.

Kernel

  • ConnectionAction.CHECK_INTEGRITY, declared between the download and the housekeeping.
  • CONNECTION_GATES stops being total in state, which is the notable part. Every connection gate
    was EVERY_SETUP_STATE until now; this one is CHECKABLE_STATESready only — because at
    not_set_up there is no snapshot to read, so the action is not merely useless there but
    unanswerable. WEIGHT_HOLDING_TYPES is the kind half, now named once and shared with
    download_weights rather than spelled out twice.
  • require_checkable (the gate, derived from connection_actions, never from a second reading) and
    record_weights_missing (the ready -> not_set_up edge).
  • InferenceConnectionNotCheckable gets its own code rather than reusing
    NOT_DOWNLOADABLE, because the two arrivals have different remedies: an http connection is told
    to stop asking; a local one at not_set_up is told to download first, which makes the identical
    request succeed. One code for two next steps would be the wrong economy.
  • Still no CONNECTION_MOVES and no transition table, and that survives the second edge
    arriving. Neither not_set_up -> ready nor ready -> not_set_up is a move somebody performs
    nobody asks to make a connection not set up, they ask for the files to be checked. Both are what
    an operation over the cache leaves behind, written in the same transaction.

Purge before the state write — and the one place this PR does not do what the dispatch asked

The decision says purge first, and the mechanics above say why: a cache hit is returned unread, so a
connection sent back to not_set_up with the damage still on disk would be "repaired" by a download
that hands the same file back and lands at Ready carrying it. The remedy would launder the fault.

The dispatch also asked that "purge and state commit together" and that "no path leaves a purged
blob with a ready connection". That is not available, and I did not pretend otherwise.
One write
is a filesystem unlink and the other is a SQLite commit; nothing brackets them. The ordering only
chooses which way the crash window falls:

  • Purge, then write (what ships) — a crash between them leaves ready with a missing file.
    That is an incomplete snapshot, exactly the condition download_weights already repairs, and the
    next run of either action fixes it for real.
  • Write, then purge — a crash between them leaves not_set_up with the corrupt file cached,
    and the download somebody then runs restores it to Ready. Silent, and unrecoverable by any
    control this product offers.

Gate 1 says the issue wins on conflicts, and the issue is explicit about purge-first; the second
ordering would also defeat the feature's whole purpose. So the invariant tested is the one that
actually holds and actually matters: no window leaves corrupt bytes reachable by a connection a
download would return to ready
. test_the_purge_happens_before_the_connection_is_stood_down
observes the ordering rather than assuming it. Never-half-ready is untouched either way —
setup_state has two values, the failure path writes one of them.

A crash before the verdict changes nothing at all: every read happens before the first unlink.

Not a verdict

A metadata fetch needs the network, and a network that is not there is not evidence about the files.
The lookup happens before anything is read, so a failure raises with prose, purges nothing and moves
no state. Answering "damaged" because a laptop was on a train would destroy a healthy cache;
answering "intact" would be a guarantee made out of nothing.

Wire, CLI, MCP

  • POST /inference/connections/{id}/check-integrity → 202 + Location, the launch-and-poll
    contract the download route already answers with. Its job reports processed/total as files
    — a real total, which the download handler cannot give because it hands its transfer to a library
    that owns the progress bar. A succeeded job's result carries files_checked and bytes_read.
  • openapi.json and the TS client regenerated, both committed.
  • visionset inference check-integrity — blocks, reports phases on stderr, --json. Its --json
    document and the job's result are the same two keys because both call IntegrityReport.counts();
    it is not a visionset.wire projection because that module pairs with a server model and no route
    publishes this shape (the route publishes a job).
  • MCP is untouched and deliberately so. There is no inference surface there at HEAD, so adding
    one action would be the odd asymmetry rather than parity.

UI

Verify weights covered two readings of one action and could only ever be honest about one. The
overflow now carries both, each named by what it proves:

Label Action Proves Cost
Check for missing files download_weights at ready nothing is missing seconds; opens no file
Check files are undamaged check_integrity nothing is damaged reads every byte

docs/inference.md carries the same table, so the page and the menu cannot drift.

useWeightsRun now takes its mutation as a parameter, so the row runs two independent polls.
One shared run state would light both controls and make the slow check read as a stalled download.

The settled-job invalidation from #470 covers the new transition, which is the direction it had not
been asked to handle: a failed check moves the row Ready -> Not set up, and the row re-reads on
settle rather than on a reload. A failed integrity check is the one refusal on this screen that has
already acted — the files are purged and the connection stood down before the job row says so —
so its prose describes a state the workspace is already in and names the remedy the row now offers.

Tests

tests/inference/test_integrity.py (new, 17): the hub is faked and the cache is real. That
split is the point — the thing being purged is a directory of blobs with symlinks into it, and a
purge that deleted only the link would pass any test that faked the filesystem too. The git-object
rule is checked against two published constants (the empty blob and b"hello\n") rather than
against a second copy of the rule written in the test file, which would only prove that two
identical mistakes agree.

Covered: per-file digest selection; the config-hashed-as-weights failure, pinned directly; an
intact snapshot leaving ready untouched with its counts; one wrong byte → named, purged, stood
down; the purge proving a re-download re-fetches; a missing file as damage; an unreadable file as
damage; purge idempotence; metadata failure changing nothing; a file the hub will not digest;
ordering; and the three refusal squares.

Elsewhere: capability contract tests gain check_integrity's invoke/refuse closures and the
now-conditional gate assertion; the pinned action set updated by hand (test_the_actions_a_ connection_cannot_yet_be_asked_for_are_not_declared), which is what makes arriving in it a
decision; route tests for 202, the damage transition, the three 409s and no-job-on-refusal; CLI
tests; SHIPPED job types; the error-code table; and five ui-core tests.

Mutation verification

Each applied to the committed tree, anchor asserted present before and replacement after, run, then
reverted by its exact edit with a clean tree confirmed.

Mutation Result
The state write moves before the purge 1 redthe purge happens before the connection is stood down
The per-file digest selection goes — everything checked as a git OID 3 red, and the failure is the production one exactly: an intact snapshot is condemned and purged
digest_of stops hashing the blob <len>\0 header 4 red, led by a config hashed the weights way would never match
purge removes the symlink and leaves the blob 4 red, incl. the purge removes the blob so the next download really re-fetches
CHECK_INTEGRITY gated on EVERY_SETUP_STATE 7 red across kernel, inference and server

The two the dispatch asked to be named are the first two rows. The fourth is the one that found a
gap: it initially survived, because the fake was kinder than the library. Verifying finding 2 above
against the real snapshot_download and modelling it in the fake is what turned it red.

Gate

Run in stages under the harness's ~10-minute ceiling; pytest split by the directories ls tests/
reported at run time. The machine was free (load average 2.07 on 20 cores), so this is the full
gate split and not the saturation fallback. Every exit code verbatim.

Stage Exit
pytest tests/architecture 0
pytest tests/cli 0
pytest tests/examples 0
pytest tests/formats 0
pytest tests/inference 0
pytest tests/jobs 0
pytest tests/kernel 0
pytest tests/mcp 0
pytest tests/packaging 0
pytest tests/server tests/test_versioning.py 0
ruff check . 0
ruff format --check . (344 files) 0
mypy src/visionset (152 files) 0
lint-imports (4 kept, 0 broken) 0
pnpm -r build 0
pnpm test 0 — annotator 914, ui-core 849, 77 node gates
pnpm -r lint (incl. all three annotator boundary gates) 0
check.sh generated (openapi drift, client drift, mcp reference, version sync) 0
check.sh browser — annotator + app e2e, then the real-server cycle 0 — both passed, 114s

tests/scripts holds only .mjs node gates, which run under pnpm test, so it is absent above
rather than reported as exit 5. No baseline-proof exception was used or needed.

One red on the way, worth recording: pnpm -r lint caught a string[] where the wire's action
union was wanted, in a test file. vitest does not typecheck, so the tests had been green over it —
aa3df7c types the pinned list off Connection instead.

Boundaries

visionset.kernel gained no import of visionset.inference, visionset.jobs or a delivery
package; visionset.inference imports no delivery package. Nothing in
frontend/annotator/src/core/ was touched. No route, tab, screen, nav entry or entry point moves,
so the sitemap is unchanged. openapi.json and the generated client are regenerated rather than
hand-edited, and the drift gates agree.

Found, not fixed

  • The download handler still reports processed=1, total=1 for a transfer of gigabytes. The
    integrity handler shows a real total is reachable when the loop is ours; the download's is not,
    because the library owns the transfer. Recorded rather than changed — the bounds put the download
    job out of scope beyond what the purge interaction required, and it required nothing.
  • #466 (the four inference request models do not forbid unknown fields) is untouched.

Closes #471.

…es damage

`download_weights` at `ready` proves a snapshot is complete and answers from an
index without opening a file, so a copy that is present and wrong passes it for
ever. `check_integrity` is the second action over the same files: it reads every
byte and compares it against what the hub published for the pinned revision.

Per file, and that is the trap this could have shipped with. The hub publishes
`lfs.sha256` for LFS-tracked files and only a git object id — SHA-1 over
`blob <len>\0` and the contents — for small ones, side by side inside one
revision. Hashing a config the weights' way would report damage over an intact
snapshot for ever, and the remedy names deletion.

Damage purges the offending blobs and *then* stands the connection down. Only
that order works: a cache hit is returned unread, so leaving the bytes in place
would let the next download hand back the same damaged file. A metadata fetch
that fails is not a verdict — no purge, no state change.

Own capability row, narrow in state as well as in kind: the first connection
gate that is not total in `setup_state`, because at `not_set_up` there is no
snapshot to read. Route answers 202; `visionset inference check-integrity`
blocks and reports phases. MCP has no inference surface at HEAD and gains none
here.
…it proves

**Verify weights** covered two readings of `download_weights` and could only be
honest about one. It becomes **Check for missing files**, beside the new **Check
files are undamaged** — completeness against integrity, fast against every byte.
`docs/inference.md` carries the same two sentences in a table, so the page and
the menu cannot drift.

The row runs two independent polls rather than one shared run state, which is
what keeps a slow check from reading as a stalled download. A failed check is
the one refusal on this screen that has already acted: the damaged files are
gone and the connection is back to Not set up before the job row says so, so the
settle-invalidation lands the row there and the prose names the download that is
now a real transfer.
@JArmandoAnaya
JArmandoAnaya merged commit 4916e64 into main Aug 9, 2026
13 checks passed
@JArmandoAnaya
JArmandoAnaya deleted the feat/weights-integrity branch August 9, 2026 23:50
JArmandoAnaya added a commit that referenced this pull request Aug 10, 2026
…496)

The check still held a client-side job id, so a reload lost one in flight and a
check started from a terminal was invisible to every browser — the failure #492
removed from the download, still present on the other action over the same files.

The connection now carries `integrity_check` beside `download`, through the same
discovery rather than a copy of it: one job type constant and one payload key in
the domain, one `ConnectionJob.of` body that identifies a run, and one queue read
that answers both kinds for every row on a screen's poll path.

The two shapes name different units because they count different things. A
transfer measures bytes off the disk; a check owns its loop and knows how many
files the revision holds before it opens the first one, so it reports files —
determinate, from the counts `jobs/integrity.py` was already reporting. Neither
borrows the other's name at any layer, which is why the wire carries two shapes
rather than one with a discriminator.

No new `ConnectionSetupState` member, for the download's reason verbatim: the
state flip is the last statement, so a third member reopens the crash window that
ordering closes. #475's semantics are untouched — purge-first stands, and a hub
that cannot be reached is still a failure with prose, no verdict, no purge and no
state change. What changes is that the sentence outlives the request.

`allowed_actions` is unchanged and pinned. A connection with a check in flight is
still `ready`, so it still declares `check_integrity` and `download_weights`;
nothing in the kernel refuses a second concurrent run. Making the declaration
job-aware would give `connection_actions` a third dimension, which is a design
decision rather than a consequence of this one.
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.

Weights integrity: a check that a cached snapshot is intact, not only complete

1 participant