pgw#1002/#1003/#1004/#1005: a publish failure costs the UPLOAD, not the CAST (th#1653 client half) - #529
Merged
Merged
Conversation
…he CAST
The client half of the th#1653 upload audit. Four defects on one exit path
which, together, turned a recoverable transient at the end of a 2h16m fp8
cast into total loss — and which hid each other, so fixing any one alone
would have measured as no improvement.
pgw#1002 — HONOR THE HUB'S OWN CLASSIFICATION.
`HubPublishError` carries the hub's th#1301 `retryable` bit and
`_map_exception` had no branch for the type at all (`grep -n HubPublishError
src/gen_worker/executor.py` was empty), so a failure the hub explicitly asked
us to retry reported JOB_STATUS_FATAL. Only RETRYABLE is requeued, so the
orchestrator's five-attempt budget was never spent on the final artifact of a
two-hour job — while intermediate checkpoints, which raise
ArtifactTransferError with an honest flag, WERE requeued. That asymmetry is
what named the bug. Now: retryable=True -> RETRYABLE; False (a repudiation)
and None ("the hub named nothing") stay FATAL. Provenance decides the class,
never our reading of the message (th#1259), and the hub's `code` LEADS the
detail so refusals group by a stable token instead of by prose.
And the abort. `DELETE /publishes/:id` is not a tidy-up: hub-side it runs
`cleanupCASPublishV2Staging` and deletes every staged chunk. The handler sent
it on ANY exception, so a blip on chunk 590 of 590 threw away 37 GB to keep a
session row from looking in-flight. It now fires ONLY for a refusal the hub
itself classified terminal. The default is KEEP — an exception we cannot
classify is not evidence that 37 GB is worthless.
pgw#1003 — THE ARTIFACT SURVIVES.
`IncrementalSafetensorsWriter.close()` closed a handle: no fsync, no atomic
rename, on the path that produces multi-GB cast outputs. It now writes to a
same-directory temp and reaches its real name only via fsync -> os.replace ->
fsync(dir) — the shape the download side has used since gw#408 — and an
incomplete tensor set, or a body that raised, is discarded rather than left
under the real name for the publisher to hash and ship.
New `convert/publish_journal.py`: the `publish_id` used to exist only as a
local variable inside `publish_v2`, so no successor could name the session
whose staged bytes it should resume. The journal records it — plus the
declared object set, the declared paths, and an opaque producer state —
beside the produced bytes, BEFORE the first PUT, and clears it only on
promotion. `publish_v2` re-adopts a journalled session instead of declaring a
fresh one (the staging prefix is session-scoped, so reusing the id is the only
way to reach what a predecessor moved); a session the hub no longer accepts
costs one round trip and falls through to a fresh declare, because resuming is
an optimization and must never be a way to fail.
`clone.py` closes the loop. Its `finally` rmtree'd the produced tree on every
exit path, with "only the downloaded source is resumable" stated outright in
the code. It now retains a workdir that still holds a resumable session — a
machine-checkable condition, not a mode, and bounded by the scratch sweeper
that already exists — and a tree a predecessor finished CASTING AND DECLARING
is re-published rather than re-cast. Identity is checked twice: the declared
path set on disk, then `publish_v2` re-hashing every file, so a
same-shape-but-corrupted tree publishes fresh instead of resuming wrong.
SCOPE, STATED: this is a SAME-POD durability record. The disk dies with the
pod. It covers the case that motivated the issue — a blip, a hub restart, a
completion timeout, a requeue onto the same worker — and deliberately not
cross-pod resume, which needs the id to travel through the hub (th#1654's own
open question). What it does NOT depend on is th#1654 landing: the client is
built to exploit the staging-aware re-plan the moment it exists, and costs one
extra /grants round trip before then.
pgw#1004 — THE CHUNK DATA PLANE GROWS UP.
`_put_one` had five attempts and no `time.sleep` anywhere in the loop: four
threads hammering a store that had just answered 429. It now classifies before
charging an attempt (a terminal status raises on the spot and spends nothing)
and backs off on the repo's one decorrelated-jitter helper, which is promoted
to `_upload_transport.backoff_sleep_s` rather than reimplemented a fourth
time.
`UploadGrant` carries `expires_at` — on the wire since th#1303 and dropped on
the floor. A grant that is expired, or too close to it to start a 64 MiB PUT,
now RE-PLANS (that is the CAS path's presign re-mint; th#1655 explicitly does
not apply here) on its own budget rather than consuming a re-upload pass, and
a 403 on an already-expired grant is finally distinguishable from a
substituted claim.
Every completed object feeds `upload:bytes` and `note_progress()`, so a
healthy multi-GB publish stops being the same silence a wedge is to the
orchestrator's 10-minute stall window; the beat lives IN the data plane, not
in a callback a caller can forget. `convert/publish.py` — every quantize /
fuse / cast job — emits per-leg `convert_publish` events, where it previously
emitted none at all.
Concurrency 4 -> 8 under a 16-PUT process ceiling, anchored to the two
measured in-repo reference points (the hub's 16-window ranged reader, the SDK
path's 10 workers), and the slot is taken BEFORE the span is read so it bounds
buffer residency rather than socket count. `_concurrent_upload.py`'s claim to
own a `parallel_map_uploads` that has never existed is deleted rather than
reasserted.
pgw#1005 — VERIFIED DIGESTS AND REAL TESTS.
`upload_file_with_grant` returned the caller's claim having verified nothing,
and forwarded it verbatim into `/complete`; its download twin was fixed and
documented long ago. It now proves the digest from the bytes, refuses an
expired scoped credential (`expires_at`: parsed, never read), and stops
multiplying four outer attempts by botocore's ten. The docstring says what the
check is and is not: this path is SDK multipart, where R2 does not enforce
x-amz-checksum-sha256 on UploadPart, which is exactly why the CAS path keeps
per-chunk presigns and the platform's strongest integrity property.
`_upload_transport` gets its first functional coverage: re-open-per-attempt at
the part's true offset (the entire reason its reader class exists), fresh pool
per retry and discard_connections on failure (the R2 SSLV3_ALERT_BAD_RECORD_MAC
fix), every classifier branch, the 2xx-without-ETag refusal.
The four adversarial injectors `tests/convert/fake_hub.py` has shipped since
the day it was written, which no test ever switched on, are wired to the v2
chunk surface and driven: mid-PUT reset, 5xx-then-success, expired presign,
terminal repudiation. Plus one row at the REAL 64 MiB chunk size, which no
upload test had ever allocated — every existing one monkeypatches it to 4096.
NO WALL-CLOCK ASSERTIONS anywhere: backoff is proven by injecting the sleep
and reading what the loop asked for.
One existing row flips with the abort rule:
`test_corrupt_local_bytes_are_refused_BEFORE_the_transfer` asserted
`hub.aborts` and now asserts the opposite, with the reason inline.
No version bump — this rides the next train.
PaulFidika
force-pushed
the
1002-upload-client
branch
from
August 7, 2026 08:28
7bb2e81 to
9665315
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The client half of the th#1653 upload audit. Four defects on one exit path
which, together, turned a recoverable transient at the end of a 2h16m fp8 cast
into total loss — and which hid each other, so fixing any one alone would have
measured as no improvement.
pgw#1002 (P0) — honor the hub's own classification
HubPublishErrorcarries the hub's th#1301retryablebit and_map_exceptionhad no branch for the type at all, so a failure the hub explicitly asked us
to retry reported
JOB_STATUS_FATAL. OnlyRETRYABLEis requeued, so theorchestrator's five-attempt budget was never spent on the final artifact of a
two-hour job — while intermediate checkpoints, which raise
ArtifactTransferErrorwith an honest flag, were requeued. That asymmetryis what named the bug.
retryable=True→ RETRYABLE;False(a repudiation) andNone("the hub namednothing") stay FATAL. Provenance decides the class, never our reading of the
message (th#1259); the hub's
codeleads the detail so refusals group by astable token.
And the abort.
DELETE /publishes/:idis not a tidy-up — hub-side it runscleanupCASPublishV2Stagingand deletes every staged chunk. The handler sent iton any exception, so a blip on chunk 590 of 590 threw away 37 GB to keep a
session row from looking in-flight. It now fires only for a refusal the hub
itself classified terminal. The default is KEEP.
pgw#1003 (P0) — the artifact survives
IncrementalSafetensorsWriter.close()closed a handle: no fsync, no atomicrename, on the path that produces multi-GB cast outputs. Now temp → fsync →
os.replace→ fsync(dir), matching the download side since gw#408. Anincomplete tensor set, or a body that raised, is discarded rather than left
under the real name for the publisher to hash and ship.
convert/publish_journal.py.publish_idexisted only as a localvariable, so no successor could name the session whose staged bytes it should
resume. The journal records it — plus the declared object set, the declared
paths, and an opaque producer state — beside the produced bytes, before the
first PUT, and clears it only on promotion.
publish_v2re-adopts ajournalled session instead of declaring a fresh one; a session the hub no
longer accepts costs one round trip and falls through to a fresh declare.
clone.pycloses the loop: itsfinallyrmtree'd the produced tree on everyexit path ("only the downloaded source is resumable", stated outright in the
code). It now retains a workdir that still holds a resumable session — a
machine-checkable condition, not a mode, bounded by the scratch sweeper that
already exists — and a tree a predecessor finished casting and declaring
is re-published rather than re-cast. Identity is checked twice: the declared
path set on disk, then
publish_v2re-hashing every file.Resume-exploitation design, and the hub-lane dependency
Scope is stated rather than implied: this is a same-pod durability record.
The disk dies with the pod. It covers the case that motivated the issue — a
blip, a hub restart, a completion timeout, an orchestrator requeue onto the same
worker — and deliberately not cross-pod resume, which needs the id to travel
through the hub (th#1654's own open question).
It does not block on th#1654. The client re-plans the adopted session
through
POST .../publishes/:id/grantsand uploads only what the need set stillnames; th#1654 is what makes that set come back smaller (staged-but-unpromoted
objects are invisible to the planner until it lands). Before then the client
costs one extra round trip; the day it lands, a blip costs a chunk. Not aborting
the session on a transport failure is the precondition that makes th#1654 worth
anything at all — staging that is deleted on the way out can never be resumed.
th#1655 does not apply to this lane. th#1655 says so itself: "the repo-CAS
v2 path is NOT affected by this one — it has
/grants." The CAS path's presignre-mint is that existing route, so pgw#1004 §C is implemented against it with no
cross-lane gate.
pgw#1004 (P1) — the chunk data plane grows up
_put_onehad five attempts and notime.sleepanywhere: four threadshammering a store that had just answered 429. Now classify-before-charging (a
terminal status raises on the spot and spends nothing) plus the repo's one
decorrelated-jitter helper, promoted to
_upload_transport.backoff_sleep_srather than reimplemented a fourth time.
UploadGrantcarriesexpires_at— on the wire since th#1303, dropped on thefloor. Expired (or inside the margin) re-plans on its own budget instead of
consuming a re-upload pass, and a 403 on an already-expired grant is finally
distinguishable from a substituted claim.
upload:bytes+note_progress(), in the dataplane itself rather than in a callback a caller can forget.
convert/publish.pyemits per-legconvert_publishevents where itpreviously emitted none.
reference points (the hub's 16-window ranged reader; the SDK path's 10
workers). The slot is taken before the span is read, so it bounds buffer
residency rather than socket count.
_concurrent_upload.py's claim to own aparallel_map_uploadsthat has neverexisted is deleted rather than reasserted.
pgw#1005 (P2) — verified digests and real tests
upload_file_with_grantreturned the caller's unverified claim and forwardedit into
/complete. It now proves the digest from the bytes, refuses anexpired scoped credential (
expires_at: parsed, never read), and stopsmultiplying four outer attempts by botocore's ten. The docstring states what
the check is and is not — this path is SDK multipart, where R2 does not
enforce
x-amz-checksum-sha256onUploadPart, which is exactly why the CASpath keeps per-chunk presigns and the platform's strongest integrity property.
That property is untouched.
_upload_transportgets its first functional coverage: re-open-per-attempt atthe part's true offset, fresh pool per retry +
discard_connections, everyclassifier branch, the 2xx-without-ETag refusal.
tests/convert/fake_hub.pyhas shipped sincethe day it was written, which no test ever switched on, are wired to the
v2 chunk surface and driven: mid-PUT reset, 5xx-then-success, expired presign,
terminal repudiation.
monkeypatches it to 4096.
Tests
New:
tests/test_chunk_upload_robustness_pgw1004.py(11),tests/test_upload_transport_pgw1005.py(30),tests/test_grant_upload_digest_pgw1005.py(7),tests/convert/test_publish_survival_pgw1003.py(12),tests/convert/test_publish_status_mapping_pgw1002.py(8).All integration-grade: real sockets, real files, real threads, the real
publish_v2and the real_map_exception. No wall-clock assertions —backoff is proven by injecting the sleep and reading what the loop asked for.
One existing row flips with the abort rule:
test_corrupt_local_bytes_are_refused_BEFORE_the_transferassertedhub.abortsand now asserts the opposite, with the reason inline.Gates:
mypyclean (236 files),ruffclean,lint_http_timeouts,lint_unreached_surface,lint_config_readsall green.No version bump — rides the next train.