hosted agents: declare workspace upload size and negotiate before sending - #1074
Open
sdharavath wants to merge 3 commits into
Open
hosted agents: declare workspace upload size and negotiate before sending#1074sdharavath wants to merge 3 commits into
sdharavath wants to merge 3 commits into
Conversation
UploadWorkspace handed its io.Reader straight to http.NewRequest, which derives a Content-Length only for a few in-memory types. The *os.File callers pass fell through, so uploads went out chunked with no declared size. OHS caps workspace uploads at 50 MiB and limits concurrent transfers, but with no length to inspect it cannot apply either verdict until it has read the body. An over-cap upload was therefore only refused mid-stream, after the whole payload had crossed the network -- measured on staging at 176s for a 51 MiB file that was doomed from its first byte. Measure the body when we can (regular *os.File from its current offset, or any reader reporting Len) and let callers declare a size for readers we cannot measure. Above 1 MiB, also ask before sending. With Expect: 100-continue the payload stays on the client until OHS approves, so a rejection at headers costs one round trip instead of a full transfer. A test that counts bytes on the wire pins this down: 4 MiB refused with 503, under 32 KiB actually sent, and the paired control confirms an accepted upload still delivers every byte. The threshold keeps the extra round trip off small uploads, and an intermediary that ignores the handshake only delays a transfer -- the transport falls back to sending the body after ExpectContinueTimeout. Co-authored-by: Cursor <[email protected]>
The withholding test built a bare client, but honouring Expect: 100-continue depends on a non-zero ExpectContinueTimeout, and doctl does not use a bare client -- it wraps oauth2 in retryablehttp, which supplies its own transport and can replay a request. Both are places the guarantee could silently lapse. Assemble the client the way doctl does and confirm a 4 MiB upload refused with 503 still puts under 32 KiB on the wire across the initial attempt and its retry, and that the retry waits out Retry-After rather than the much shorter configured backoff. Co-authored-by: Cursor <[email protected]>
The API is reached over TLS with h2 negotiation enabled, and h2 implements Expect in an entirely separate transport, so the h1 coverage said nothing about the path most uploads actually take. A 4 MiB upload refused with 503 puts under 64 KiB on the wire there too. Two things this pinned down that are easy to get wrong. Honouring the handshake depends on a non-zero ExpectContinueTimeout on the underlying transport, which httptest leaves at zero -- left alone, the test silently sends the whole payload and proves nothing, so it is set to match the production transports. And the handler cannot assert on the Expect header, because net/http lifts it into an internal flag before an h2 handler runs; only the byte count shows it worked. Co-authored-by: Cursor <[email protected]>
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.
Companion to cthulhu #169365 (MARSOHS-626), which makes OHS shed saturated workspace transfers with a 503 and refuse over-cap uploads from their headers. Both of those verdicts need a client that declares a size and, ideally, asks before sending.
Summary
Declare
Content-Length.UploadWorkspacehanded itsio.Readerstraight tohttp.NewRequest, which derives a length only for a few in-memory types. The*os.Filecallers pass falls through, so uploads went out chunked with no declared size.OHS caps uploads at 50 MiB and limits concurrent transfers, but with no length to inspect it cannot apply either verdict until it has read the body. An over-cap upload was therefore only refused mid-stream, after the whole payload had crossed the network — measured on staging at 176s for a 51 MiB file that was doomed from its first byte.
uploadBodyLengthmeasures a regular*os.Filefrom its current offset, or any reader exposingLen(). A new optionalContentLengthfield covers readers we cannot measure.Negotiate above 1 MiB. With
Expect: 100-continuethe payload stays on the client until OHS approves, so a rejection at headers costs one round trip instead of a full transfer. The threshold keeps the extra round trip off small uploads, and an intermediary that ignores the handshake only delays a transfer — the transport falls back to sending the body afterExpectContinueTimeout.Both changes are additive.
doctlneeds no source change: it already passes an*os.Filerewound to the start after hashing.Testing
Tests count bytes on the socket rather than trusting headers. A 4 MiB upload refused with 503 puts under 32 KiB on the wire, and a paired control confirms an accepted upload still delivers every byte. Covered over HTTP/1.1, over HTTP/2, and through the exact stack
doctlbuilds (oauth2 wrapped inretryablehttp), where the payload is withheld across both the initial attempt and its retry, and the retry waits outRetry-Afterrather than its much shorter configured backoff.Two things worth knowing for review, both easy to get wrong:
ExpectContinueTimeouton the underlying transport.httptestleaves it at zero, which silently sends the whole payload and proves nothing, so the tests set it to match production (http.DefaultTransportand cleanhttp both use 1s).Expectheader:net/httplifts it into an internal flag before the handler runs. Only the byte count shows the negotiation happened.go build ./...,go vet ./..., and the full suite pass; the new tests were also run 10x under-race.No CHANGELOG entry, matching the other hosted-agents commits on this branch.
Made with Cursor