Retry chunked transfers, add jitter, stop retrying hopeless errors by kmatzen · Pull Request #100 · kmatzen/s3lfs

kmatzen · 2026-07-19T05:47:41Z

Fixes #89. Stacked on #98.

The chunked pipeline had no retry at all

@retry was applied at exactly two call sites. The chunked pipeline — which handles the largest files, where transient failures are most likely — had none, so a single blip aborted the whole file.

_upload_chunk, _download_chunk, and _discover_chunks_for_file are now retried.

`_upload_chunk` needed restructuring first

Simply decorating it would not have worked. It removed its chunk file in a finally that runs on the first failure, so the retry would find nothing to read and fail with FileNotFoundError — which isn't in the retried exception set. The decorator would have looked correct and done nothing.

The PUT is now a separate retried call, with cleanup once after all attempts. _download_chunk reopens its target with "wb" each attempt and discovery is a list call, so both were already idempotent.

Jitter

A fixed 2/4/8 schedule means every worker that failed at the same moment retries at the same moment — a transient blip becomes a synchronised stampede against the endpoint that just failed. Now full jitter: each delay is drawn from [0, 2**(attempt+1)].

Hopeless errors are no longer retried

Permission failures, missing buckets, and 4xx responses other than throttling and request timeout now fail immediately. A 403 previously took three attempts and two sleeps before surfacing the real cause.

Measured

A chunk upload whose first PUT returns a 500:

	Attempts	Result
Before	1	`ClientError: InternalError`
After	2	uploaded

61 failed / 577 passed → 61 failed / 589 passed, failure set identical.

A test I changed deliberately

test_exponential_backoff_delays asserted the exact delays 2, 4, 8. It now asserts each delay lies within [0, ceiling]. That test encoded the fixed schedule as intended behaviour, so this is a deliberate reversal — flagging it rather than burying it.

Not addressed

Still open in #89, and the issue stays open for them:

No abort_multipart_upload anywhere. A killed process leaks incomplete multipart uploads that are billed and invisible to cleanup_s3, which lists only completed objects. A bucket lifecycle rule is the usual mitigation and deserves documenting.
The skip-if-exists path compares a local MD5 against the S3 ETag, which can never match for multipart objects, so large files re-upload every time.
A retry still re-compresses and re-splits the whole file at the upload level, since that path deletes its inputs in a finally. Only the chunk-level retry avoids redoing work.

🤖 Generated with Claude Code

https://claude.ai/code/session_019UrDtdFKGwQZp8oYGwL2rj

All three hooks ended in `|| echo`, which forces exit status zero. For the post-* hooks that is defensible: git has already committed its ref and index updates, so failing helps nobody. They now report loudly on stderr and still exit zero. For pre-push it is not defensible. That hook uploads the content the manifest being pushed refers to, and it is the last point at which a bad push can be stopped. Masking a failed upload publishes a manifest whose hashes have no objects behind them, and every collaborator's checkout 404s. It now aborts the push and points at --no-verify for the case where the user knows better. This contradicts an existing test that asserted `|| echo` for all three hooks, which has been updated. That test encoded the current behaviour as intended, so the change is deliberate rather than incidental: reviewers who disagree should look here first. The risk is that a push is now blocked when `s3lfs track --modified` fails for an unrelated reason, such as absent credentials, on a branch that touches no large files. Hook install and uninstall also wrote through write_text, which truncates before writing: an interrupt part-way through leaves a user's pre-existing hook truncated and executable. Both now write to a sibling temp file and rename. Three of the eight new tests fail on the parent commit. Test suite: 61 failed / 568 passed before, 61 failed / 577 passed after. Failure set identical. Fixes #88 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

@Retry

The @Retry decorator was applied at exactly two call sites. The chunked pipeline -- which handles the largest files, where transient failures are most likely -- had none, so a single blip aborted the whole file. _upload_chunk, _download_chunk, and _discover_chunks_for_file are now retried. _upload_chunk needed restructuring first: it removed its chunk file in a finally that ran on the first failure, so a retry would have found nothing to read and failed with FileNotFoundError. The PUT is now a separate retried call and the cleanup happens once, after all attempts. _download_chunk reopens its target with "wb" each attempt and discovery is a list call, so both were already idempotent. Backoff gains full jitter. A fixed 2/4/8 schedule means every worker that failed at the same moment retries at the same moment, turning a transient blip into a synchronised stampede against the endpoint that just failed. Errors that cannot succeed on a second attempt are no longer retried: permission failures, missing buckets, and 4xx responses other than throttling and request timeout. Previously a 403 took three attempts and two sleeps before surfacing. Measured on a chunk upload whose first PUT fails with a 500: before, one attempt and a hard failure; after, two attempts and success. An existing test asserted the exact fixed delays 2/4/8 and now asserts each delay falls within [0, ceiling]. That test encoded the old schedule as intended behaviour, so the change is deliberate. Test suite: 61 failed / 577 passed before, 61 failed / 589 passed after. Failure set identical; the 61 are the missing-"python"-binary failures. Fixes #89 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

@Retry

) * Let pre-push abort the push, and install hooks atomically All three hooks ended in `|| echo`, which forces exit status zero. For the post-* hooks that is defensible: git has already committed its ref and index updates, so failing helps nobody. They now report loudly on stderr and still exit zero. For pre-push it is not defensible. That hook uploads the content the manifest being pushed refers to, and it is the last point at which a bad push can be stopped. Masking a failed upload publishes a manifest whose hashes have no objects behind them, and every collaborator's checkout 404s. It now aborts the push and points at --no-verify for the case where the user knows better. This contradicts an existing test that asserted `|| echo` for all three hooks, which has been updated. That test encoded the current behaviour as intended, so the change is deliberate rather than incidental: reviewers who disagree should look here first. The risk is that a push is now blocked when `s3lfs track --modified` fails for an unrelated reason, such as absent credentials, on a branch that touches no large files. Hook install and uninstall also wrote through write_text, which truncates before writing: an interrupt part-way through leaves a user's pre-existing hook truncated and executable. Both now write to a sibling temp file and rename. Three of the eight new tests fail on the parent commit. Test suite: 61 failed / 568 passed before, 61 failed / 577 passed after. Failure set identical. Fixes #88 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Retry chunked transfers, add jitter, stop retrying hopeless errors The @Retry decorator was applied at exactly two call sites. The chunked pipeline -- which handles the largest files, where transient failures are most likely -- had none, so a single blip aborted the whole file. _upload_chunk, _download_chunk, and _discover_chunks_for_file are now retried. _upload_chunk needed restructuring first: it removed its chunk file in a finally that ran on the first failure, so a retry would have found nothing to read and failed with FileNotFoundError. The PUT is now a separate retried call and the cleanup happens once, after all attempts. _download_chunk reopens its target with "wb" each attempt and discovery is a list call, so both were already idempotent. Backoff gains full jitter. A fixed 2/4/8 schedule means every worker that failed at the same moment retries at the same moment, turning a transient blip into a synchronised stampede against the endpoint that just failed. Errors that cannot succeed on a second attempt are no longer retried: permission failures, missing buckets, and 4xx responses other than throttling and request timeout. Previously a 403 took three attempts and two sleeps before surfacing. Measured on a chunk upload whose first PUT fails with a 500: before, one attempt and a hard failure; after, two attempts and success. An existing test asserted the exact fixed delays 2/4/8 and now asserts each delay falls within [0, ceiling]. That test encoded the old schedule as intended behaviour, so the change is deliberate. Test suite: 61 failed / 577 passed before, 61 failed / 589 passed after. Failure set identical; the 61 are the missing-"python"-binary failures. Fixes #89 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>

@Retry

* Make garbage collection reachability path-aware Objects are stored at assets/{hash}/{manifest_key}.gz, so the storage unit is hash plus path. Garbage collection judged reachability on the hash alone, which leaked in both directions: an object stayed reachable as long as any path shared its content, so removing one of two identical files never freed the removed path's object, and renaming a file left its old object reachable forever though nothing could address it. Reachability is now computed as the set of base keys the manifest implies, and a key is live if it is one of those or a .chunkN belonging to one. The in-flight registry moves from hashes to the same base keys, so a claim on one path no longer protects a different path that happens to share content. remove_file and remove_subtree deleted only the base .gz key, orphaning every chunk of a large file. They now delete the chunks too. This keeps the existing layout rather than moving to content-addressed storage, so no migration is needed. It does mean deduplication remains per-path: two identical files at different paths still occupy two objects, contrary to what the README implies. That claim should be corrected or the layout changed; both are out of scope here. An existing test caught a regression in an earlier draft: keys under the prefix that do not have the shape of an asset were being collected. They are now explicitly skipped, since s3lfs did not put them there in a form it recognises and they are not ours to delete. Five of the seven new tests fail on the parent commit. Three tests from the in-flight registry change were updated for the key-based API. Test suite: 61 failed / 561 passed before, 61 failed / 568 passed after. Failure set identical. Fixes #87 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Let pre-push abort the push, and install hooks atomically (#98) All three hooks ended in `|| echo`, which forces exit status zero. For the post-* hooks that is defensible: git has already committed its ref and index updates, so failing helps nobody. They now report loudly on stderr and still exit zero. For pre-push it is not defensible. That hook uploads the content the manifest being pushed refers to, and it is the last point at which a bad push can be stopped. Masking a failed upload publishes a manifest whose hashes have no objects behind them, and every collaborator's checkout 404s. It now aborts the push and points at --no-verify for the case where the user knows better. This contradicts an existing test that asserted `|| echo` for all three hooks, which has been updated. That test encoded the current behaviour as intended, so the change is deliberate rather than incidental: reviewers who disagree should look here first. The risk is that a push is now blocked when `s3lfs track --modified` fails for an unrelated reason, such as absent credentials, on a branch that touches no large files. Hook install and uninstall also wrote through write_text, which truncates before writing: an interrupt part-way through leaves a user's pre-existing hook truncated and executable. Both now write to a sibling temp file and rename. Three of the eight new tests fail on the parent commit. Test suite: 61 failed / 568 passed before, 61 failed / 577 passed after. Failure set identical. Fixes #88 Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> * Retry chunked transfers, add jitter, stop retrying hopeless errors (#100) * Let pre-push abort the push, and install hooks atomically All three hooks ended in `|| echo`, which forces exit status zero. For the post-* hooks that is defensible: git has already committed its ref and index updates, so failing helps nobody. They now report loudly on stderr and still exit zero. For pre-push it is not defensible. That hook uploads the content the manifest being pushed refers to, and it is the last point at which a bad push can be stopped. Masking a failed upload publishes a manifest whose hashes have no objects behind them, and every collaborator's checkout 404s. It now aborts the push and points at --no-verify for the case where the user knows better. This contradicts an existing test that asserted `|| echo` for all three hooks, which has been updated. That test encoded the current behaviour as intended, so the change is deliberate rather than incidental: reviewers who disagree should look here first. The risk is that a push is now blocked when `s3lfs track --modified` fails for an unrelated reason, such as absent credentials, on a branch that touches no large files. Hook install and uninstall also wrote through write_text, which truncates before writing: an interrupt part-way through leaves a user's pre-existing hook truncated and executable. Both now write to a sibling temp file and rename. Three of the eight new tests fail on the parent commit. Test suite: 61 failed / 568 passed before, 61 failed / 577 passed after. Failure set identical. Fixes #88 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Retry chunked transfers, add jitter, stop retrying hopeless errors The @Retry decorator was applied at exactly two call sites. The chunked pipeline -- which handles the largest files, where transient failures are most likely -- had none, so a single blip aborted the whole file. _upload_chunk, _download_chunk, and _discover_chunks_for_file are now retried. _upload_chunk needed restructuring first: it removed its chunk file in a finally that ran on the first failure, so a retry would have found nothing to read and failed with FileNotFoundError. The PUT is now a separate retried call and the cleanup happens once, after all attempts. _download_chunk reopens its target with "wb" each attempt and discovery is a list call, so both were already idempotent. Backoff gains full jitter. A fixed 2/4/8 schedule means every worker that failed at the same moment retries at the same moment, turning a transient blip into a synchronised stampede against the endpoint that just failed. Errors that cannot succeed on a second attempt are no longer retried: permission failures, missing buckets, and 4xx responses other than throttling and request timeout. Previously a 403 took three attempts and two sleeps before surfacing. Measured on a chunk upload whose first PUT fails with a 500: before, one attempt and a hard failure; after, two attempts and success. An existing test asserted the exact fixed delays 2/4/8 and now asserts each delay falls within [0, ceiling]. That test encoded the old schedule as intended behaviour, so the change is deliberate. Test suite: 61 failed / 577 passed before, 61 failed / 589 passed after. Failure set identical; the 61 are the missing-"python"-binary failures. Fixes #89 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> * Stop queued transfers after an interrupt (#101) * Let pre-push abort the push, and install hooks atomically All three hooks ended in `|| echo`, which forces exit status zero. For the post-* hooks that is defensible: git has already committed its ref and index updates, so failing helps nobody. They now report loudly on stderr and still exit zero. For pre-push it is not defensible. That hook uploads the content the manifest being pushed refers to, and it is the last point at which a bad push can be stopped. Masking a failed upload publishes a manifest whose hashes have no objects behind them, and every collaborator's checkout 404s. It now aborts the push and points at --no-verify for the case where the user knows better. This contradicts an existing test that asserted `|| echo` for all three hooks, which has been updated. That test encoded the current behaviour as intended, so the change is deliberate rather than incidental: reviewers who disagree should look here first. The risk is that a push is now blocked when `s3lfs track --modified` fails for an unrelated reason, such as absent credentials, on a branch that touches no large files. Hook install and uninstall also wrote through write_text, which truncates before writing: an interrupt part-way through leaves a user's pre-existing hook truncated and executable. Both now write to a sibling temp file and rename. Three of the eight new tests fail on the parent commit. Test suite: 61 failed / 568 passed before, 61 failed / 577 passed after. Failure set identical. Fixes #88 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Retry chunked transfers, add jitter, stop retrying hopeless errors The @Retry decorator was applied at exactly two call sites. The chunked pipeline -- which handles the largest files, where transient failures are most likely -- had none, so a single blip aborted the whole file. _upload_chunk, _download_chunk, and _discover_chunks_for_file are now retried. _upload_chunk needed restructuring first: it removed its chunk file in a finally that ran on the first failure, so a retry would have found nothing to read and failed with FileNotFoundError. The PUT is now a separate retried call and the cleanup happens once, after all attempts. _download_chunk reopens its target with "wb" each attempt and discovery is a list call, so both were already idempotent. Backoff gains full jitter. A fixed 2/4/8 schedule means every worker that failed at the same moment retries at the same moment, turning a transient blip into a synchronised stampede against the endpoint that just failed. Errors that cannot succeed on a second attempt are no longer retried: permission failures, missing buckets, and 4xx responses other than throttling and request timeout. Previously a 403 took three attempts and two sleeps before surfacing. Measured on a chunk upload whose first PUT fails with a 500: before, one attempt and a hard failure; after, two attempts and success. An existing test asserted the exact fixed delays 2/4/8 and now asserts each delay falls within [0, ceiling]. That test encoded the old schedule as intended behaviour, so the change is deliberate. Test suite: 61 failed / 577 passed before, 61 failed / 589 passed after. Failure set identical; the 61 are the missing-"python"-binary failures. Fixes #89 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Stop queued transfers after an interrupt Only the drain loops checked _shutdown_requested, so every chunk already submitted to the thread pool ran to completion after Ctrl-C. On a large transfer that makes the interrupt look like a hang. _upload_chunk and _download_chunk now decline to start once shutdown has been requested, raising ShutdownRequested so the drain loops can tell cancelled work from work that genuinely failed and stay quiet about the former rather than printing an error for every queued task. A cancelled upload still removes its temp chunk. The max_concurrency item from the same issue is NOT changed. boto3 spawns max_concurrency threads per transfer while s3lfs runs self.workers transfers at once, so in the worst case they multiply -- but dividing the budget across the pool would leave a single large asset with no multipart parallelism at all, which is the case s3lfs exists for. An existing test documents that intent. I had listed this as a defect in the issue; it is a deliberate trade-off and I was wrong to call it out. A comment now records the reasoning. Test suite: 61 failed / 589 passed before, 61 failed / 593 passed after. Failure set identical. Refs #91 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> * Compose the manifest and GC specs, model termination, check larger sizes (#102) * Let pre-push abort the push, and install hooks atomically All three hooks ended in `|| echo`, which forces exit status zero. For the post-* hooks that is defensible: git has already committed its ref and index updates, so failing helps nobody. They now report loudly on stderr and still exit zero. For pre-push it is not defensible. That hook uploads the content the manifest being pushed refers to, and it is the last point at which a bad push can be stopped. Masking a failed upload publishes a manifest whose hashes have no objects behind them, and every collaborator's checkout 404s. It now aborts the push and points at --no-verify for the case where the user knows better. This contradicts an existing test that asserted `|| echo` for all three hooks, which has been updated. That test encoded the current behaviour as intended, so the change is deliberate rather than incidental: reviewers who disagree should look here first. The risk is that a push is now blocked when `s3lfs track --modified` fails for an unrelated reason, such as absent credentials, on a branch that touches no large files. Hook install and uninstall also wrote through write_text, which truncates before writing: an interrupt part-way through leaves a user's pre-existing hook truncated and executable. Both now write to a sibling temp file and rename. Three of the eight new tests fail on the parent commit. Test suite: 61 failed / 568 passed before, 61 failed / 577 passed after. Failure set identical. Fixes #88 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Retry chunked transfers, add jitter, stop retrying hopeless errors The @Retry decorator was applied at exactly two call sites. The chunked pipeline -- which handles the largest files, where transient failures are most likely -- had none, so a single blip aborted the whole file. _upload_chunk, _download_chunk, and _discover_chunks_for_file are now retried. _upload_chunk needed restructuring first: it removed its chunk file in a finally that ran on the first failure, so a retry would have found nothing to read and failed with FileNotFoundError. The PUT is now a separate retried call and the cleanup happens once, after all attempts. _download_chunk reopens its target with "wb" each attempt and discovery is a list call, so both were already idempotent. Backoff gains full jitter. A fixed 2/4/8 schedule means every worker that failed at the same moment retries at the same moment, turning a transient blip into a synchronised stampede against the endpoint that just failed. Errors that cannot succeed on a second attempt are no longer retried: permission failures, missing buckets, and 4xx responses other than throttling and request timeout. Previously a 403 took three attempts and two sleeps before surfacing. Measured on a chunk upload whose first PUT fails with a 500: before, one attempt and a hard failure; after, two attempts and success. An existing test asserted the exact fixed delays 2/4/8 and now asserts each delay falls within [0, ceiling]. That test encoded the old schedule as intended behaviour, so the change is deliberate. Test suite: 61 failed / 577 passed before, 61 failed / 589 passed after. Failure set identical; the 61 are the missing-"python"-binary failures. Fixes #89 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Stop queued transfers after an interrupt Only the drain loops checked _shutdown_requested, so every chunk already submitted to the thread pool ran to completion after Ctrl-C. On a large transfer that makes the interrupt look like a hang. _upload_chunk and _download_chunk now decline to start once shutdown has been requested, raising ShutdownRequested so the drain loops can tell cancelled work from work that genuinely failed and stay quiet about the former rather than printing an error for every queued task. A cancelled upload still removes its temp chunk. The max_concurrency item from the same issue is NOT changed. boto3 spawns max_concurrency threads per transfer while s3lfs runs self.workers transfers at once, so in the worst case they multiply -- but dividing the budget across the pool would leave a single large asset with no multipart parallelism at all, which is the case s3lfs exists for. An existing test documents that intent. I had listed this as a defect in the issue; it is a deliberate trade-off and I was wrong to call it out. A comment now records the reasoning. Test suite: 61 failed / 589 passed before, 61 failed / 593 passed after. Failure set identical. Refs #91 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Compose the manifest and GC specs, model termination, check larger sizes Three modelling gaps, all raised when the specs were first reviewed. The specs were per-protocol silos, and each assumed the others' properties held: S3lfsGC assumed mutual exclusion, which was false until the lock path was fixed, and S3lfsManifest assumed paths did not matter, which hid the namespace defect. S3lfsCombined puts an uploader, a remover and a collector against one manifest so both safety properties are checked against the same behaviours. It reproduces both single-protocol findings and adds NoOrphanedObjectSurvivesGC, which neither spec could state alone: a lost update orphans an object and the collector then deletes it, so a manifest bug becomes data loss only when GC is also in play. Reachability is keyed on hash and path, tracking the path-aware GC the code now implements. Every configuration previously set CHECK_DEADLOCK FALSE, because a finished behaviour looks like a deadlock to TLC. That also switched off detection of real deadlocks, which mattered: _lock_context is not reentrant, and the question of whether widening a critical section self-deadlocks ended up being answered by instrumenting the lock rather than by the model. Each spec now has an explicit Terminating stutter and CHECK_DEADLOCK is TRUE everywhere; all clean configurations pass with it enabled. The verified-good configurations were also re-run larger -- 3 paths and 3 hashes for the combined model, 3 processes for the manifest model, 6 chunks for the chunk model. Nothing new appears. That is not a proof, but it removes the "only ever checked at the smallest interesting size" caveat. No production code changes; the test suite is untouched at 61 failed / 592 passed. Fixes #92 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>

kmatzen and others added 2 commits July 18, 2026 22:29

kmatzen force-pushed the retry-chunked-transfers branch from e25d246 to c1ecd31 Compare July 19, 2026 05:50

kmatzen mentioned this pull request Jul 19, 2026

Stop queued transfers after an interrupt #101

Merged

Base automatically changed from hook-exit-status to path-aware-gc July 19, 2026 06:16

kmatzen merged commit 76c913b into path-aware-gc Jul 19, 2026
3 checks passed

kmatzen deleted the retry-chunked-transfers branch July 19, 2026 06:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Retry chunked transfers, add jitter, stop retrying hopeless errors#100

Retry chunked transfers, add jitter, stop retrying hopeless errors#100
kmatzen merged 2 commits into
path-aware-gcfrom
retry-chunked-transfers

kmatzen commented Jul 19, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

kmatzen commented Jul 19, 2026

The chunked pipeline had no retry at all

_upload_chunk needed restructuring first

Jitter

Hopeless errors are no longer retried

Measured

A test I changed deliberately

Not addressed

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

`_upload_chunk` needed restructuring first