fix(host-transfer): emit Content-Length and X-Transfer-SHA256 on GET content#29449
Merged
Merged
Conversation
…content
The HTTP adapter calls 'r.handler' BEFORE 'resolveResponseHeaders'
(http-adapter.ts:107-125), so when 'handleTransferContentGet' consumes
the entry via 'getTransferContent', the entry is gone by the time
'resolveTransferContentGetHeaders' runs. The resolver silently fell
through to the default '{ Content-Type: application/octet-stream }' —
the documented 'Content-Length' and 'X-Transfer-SHA256' response
headers were never sent.
Fix: HostTransferProxy stashes size/sha256 in a 'justConsumedMetadata'
Map synchronously inside 'getTransferContent' (before the entry is
deleted), and exposes 'takeJustConsumedTransferMetadata' which the
header resolver reads + clears on response. A 30s 'unref()'d timer
clears stale entries if the resolver never runs (e.g., handler error
after consume, abort).
This is pre-existing — Devin flagged it on PR #29440 as r3178602466.
The Swift client's 'pullTransferContent' doesn't currently inspect
either header, so there's no observable functional change today; the
fix restores the documented response contract.
Tests: 4 new cases in 'takeJustConsumedTransferMetadata' covering
post-consume read, single-use semantics, never-consumed transferIds,
and unknown ids in the presence of a different consumed transfer.
Contributor
Author
|
@codexbot review |
Contributor
Author
|
@devin-ai-integration[bot] please review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
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
1 participant
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.
Summary
Pre-existing bug Devin flagged on PR #29440 (discussion r3178602466): the GET
/v1/transfers/:transferId/contentendpoint never actually sends its documentedContent-LengthorX-Transfer-SHA256response headers. They silently fall through to the default{ Content-Type: application/octet-stream }.Root cause
The HTTP adapter calls
r.handlerBEFOREresolveResponseHeaders(http-adapter.ts:107-125).handleTransferContentGetconsumes the entry viagetTransferContent(single-use — deletes it from the transfers map). By the timeresolveTransferContentGetHeadersruns, the entry is gone, so the in-resolvergetTransferContentcall returns null and the resolver falls through to the default-only return.Fix
HostTransferProxy.getTransferContentnow stashes size/sha256 in ajustConsumedMetadataMap synchronously, before deleting the entry. A newtakeJustConsumedTransferMetadata(transferId)returns + clears that metadata; the header resolver reads from it instead of trying to access the already-consumed entry.A
setTimeout(..., 30_000).unref?.()clears stale entries if the resolver never runs (handler error after consume, request abort), so the cache can't grow unbounded.unref()keeps the timer from holding the process open.Why a separate PR
Devin flagged this as pre-existing on both PR #29434 and PR #29440 (the Phase 3 final). Out of scope for the Phase 3 feature work — both reviewers agreed it was orthogonal. Filing as a focused follow-up.
Observable impact today
None — the macOS Swift client's
pullTransferContentdoesn't currently inspect either response header. This restores the documented response contract so future consumers (and any clients verifying transfer integrity over the wire) get correct values.Tests
4 new cases in
host-transfer-proxy.test.tsunder atakeJustConsumedTransferMetadatadescribe block:getTransferContenttakereturns nullgetTransferContentwas never calledAll 30 proxy tests + 24 routes-targeted tests pass locally. Type check + lint clean.
Files
assistant/src/daemon/host-transfer-proxy.ts— addsjustConsumedMetadataMap, populates it insidegetTransferContent, exposestakeJustConsumedTransferMetadataassistant/src/runtime/routes/host-transfer-routes.ts—resolveTransferContentGetHeadersreads from the new take methodassistant/src/__tests__/host-transfer-proxy.test.ts— 4 new test cases