Found by the chaos harness extension (#221), which now bounds and reports the leak rather
than reddening a merge gate over an unfixed cost issue. This is the follow-up fix.
The defect
upload_stage_resumable in driven-s3 calls open_resumable_session UNCONDITIONALLY on
every attempt. So a retry mints a FRESH multipart upload id and simply abandons the
previous one - along with every part already uploaded under it.
S3 and R2 bill for the parts of an incomplete multipart upload until the upload is aborted
or expires. One abandoned upload per failed attempt, indefinitely.
Not data loss - a cost and hygiene bug
Nothing is lost or corrupted: the successful attempt completes normally and the manifest
points at the right object. The stranded parts are invisible to list_folder, so they do
not confuse the remote-existence audit or the new integrity scrub (#203) either. The cost
is money and clutter, which is why #221 chose to bound it (a chaos row fails only if MORE
parts strand than its injected faults account for) instead of blocking merges on it.
Why driven-localfs does not have this
The local-folder backend sweeps its equivalent (abandoned temp files) at startup. S3Store
has no such sweep - that asymmetry is the fix.
Suggested fix
Two parts, either or both:
- Reuse the session across retries where the failure is transient and the upload id is
still valid. This is the real fix: retrying a part should not restart the upload. Note
NoSuchUpload (already injectable in the harness) is the signal that the id has genuinely
expired and a fresh session IS required, so the retry logic needs to distinguish that from
an ordinary transient.
- Abort on give-up, and sweep on startup. Call
AbortMultipartUpload when a staged
upload is finally abandoned, and add a ListMultipartUploads + abort sweep at
S3Store::new for anything Driven left behind previously, mirroring what
driven-localfs already does for temp files. A prefix/age guard is wise so Driven never
aborts an unrelated application's in-flight upload sharing the bucket.
Also worth documenting for users: providers can expire incomplete multipart uploads with a
lifecycle rule, which is a good belt-and-braces recommendation regardless of the code fix.
Test hook already exists
#221 added NoSuchUpload, connection-drop-mid-UploadPart, and connection-drop-at-
CompleteMultipartUpload faults to the in-process FaultyS3Server, so a regression test
asserting "a retried upload strands zero extra parts" can be written directly against them.
Found by the chaos harness extension (#221), which now bounds and reports the leak rather
than reddening a merge gate over an unfixed cost issue. This is the follow-up fix.
The defect
upload_stage_resumableindriven-s3callsopen_resumable_sessionUNCONDITIONALLY onevery attempt. So a retry mints a FRESH multipart upload id and simply abandons the
previous one - along with every part already uploaded under it.
S3 and R2 bill for the parts of an incomplete multipart upload until the upload is aborted
or expires. One abandoned upload per failed attempt, indefinitely.
Not data loss - a cost and hygiene bug
Nothing is lost or corrupted: the successful attempt completes normally and the manifest
points at the right object. The stranded parts are invisible to
list_folder, so they donot confuse the remote-existence audit or the new integrity scrub (#203) either. The cost
is money and clutter, which is why #221 chose to bound it (a chaos row fails only if MORE
parts strand than its injected faults account for) instead of blocking merges on it.
Why
driven-localfsdoes not have thisThe local-folder backend sweeps its equivalent (abandoned temp files) at startup.
S3Storehas no such sweep - that asymmetry is the fix.
Suggested fix
Two parts, either or both:
still valid. This is the real fix: retrying a part should not restart the upload. Note
NoSuchUpload(already injectable in the harness) is the signal that the id has genuinelyexpired and a fresh session IS required, so the retry logic needs to distinguish that from
an ordinary transient.
AbortMultipartUploadwhen a stagedupload is finally abandoned, and add a
ListMultipartUploads+ abort sweep atS3Store::newfor anything Driven left behind previously, mirroring whatdriven-localfsalready does for temp files. A prefix/age guard is wise so Driven neveraborts an unrelated application's in-flight upload sharing the bucket.
Also worth documenting for users: providers can expire incomplete multipart uploads with a
lifecycle rule, which is a good belt-and-braces recommendation regardless of the code fix.
Test hook already exists
#221 added
NoSuchUpload, connection-drop-mid-UploadPart, and connection-drop-at-CompleteMultipartUploadfaults to the in-processFaultyS3Server, so a regression testasserting "a retried upload strands zero extra parts" can be written directly against them.