Skip to content

dropbox: detach empty-upload cleanup from the request context - #28

Merged
acoshift merged 1 commit into
mainfrom
dropbox-empty-cleanup-ctx
Jun 28, 2026
Merged

dropbox: detach empty-upload cleanup from the request context#28
acoshift merged 1 commit into
mainfrom
dropbox-empty-cleanup-ctx

Conversation

@acoshift

Copy link
Copy Markdown
Member

What

In uploadHandler, when a chunked (unknown-length) request carries an empty body, the handler finalizes a 0-byte object and then deletes it before returning body empty. That delete used the request context:

if n == 0 {
    if err := a.Bucket.Delete(r.Context(), fn); err != nil && gcerrors.Code(err) != gcerrors.NotFound {
        slog.Error("delete empty upload", "fn", fn, "error", err)
    }
    ...
}

r.Context() is canceled the instant the client disconnects — and a client that streams an empty body and drops the connection is exactly what reaches this branch. A canceled-context delete fails, stranding a rowless 0-byte object that the DB-driven GC can never reclaim (it only deletes objects that have an expired DB row).

The mid-stream io.Copy error path right above (handler.go:119) and the signed-upload rejection paths already guard against this by deleting on a detached context. This applies the same fix to the empty-upload path via the existing helper:

if n == 0 {
    a.deleteObject(r, fn) // detaches via context.WithoutCancel + 10s timeout
    ...
}

Why

It's a (narrow) resource leak and an inconsistency: every other "delete a just-written object" site in this service already detaches from the request context for precisely this reason; the empty-upload site was the one that didn't.

Verification

  • go build ./... and go vet ./... clean; gofmt clean.
  • Existing TestUpload_EmptyChunkedBody (asserts no bucket object and no DB row after an empty chunked upload) and the full TestUpload* suite stay green.
  • No behavior change on the success path; the only change is the cleanup context (and the log line now reads delete rejected upload, shared with the other rejection paths).

Note

The exact client-disconnect race isn't unit-reproducible with httptest (the test context isn't canceled mid-handler), so this reuses the already-tested deleteObject helper rather than adding a contrived test. The io.Copy error path at line 119 still inlines the equivalent detached-cleanup logic with a more specific log message; folding it onto the helper too is a reasonable follow-up, left out here to keep this a focused fix.

When a chunked (unknown-length) request carries an empty body, the handler
finalizes a 0-byte object and then deletes it before returning 'body empty'.
That delete used r.Context(), which is canceled the moment the client
disconnects — and a client that sends an empty body and drops the connection
is exactly the case that triggers this path. A canceled-context delete fails,
stranding a rowless 0-byte object that the DB-driven GC can never reclaim.

Use the existing deleteObject helper, which detaches via
context.WithoutCancel + a 10s timeout — the same pattern already used by the
mid-stream io.Copy error path and the signed-upload rejection paths. No
behavior change for the success path.

go build / vet clean; existing TestUpload_EmptyChunkedBody (and the rest of
the upload suite) still green.
@acoshift
acoshift merged commit 510baed into main Jun 28, 2026
2 checks passed
@acoshift
acoshift deleted the dropbox-empty-cleanup-ctx branch June 28, 2026 23:08
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.

1 participant