Skip to content

Fix cache poisoning from broken-pipe truncated responses - #239

Closed
thavaahariharangit with Copilot wants to merge 1 commit into
mainfrom
copilot/fix-dependabot-mix-job-failure
Closed

Fix cache poisoning from broken-pipe truncated responses#239
thavaahariharangit with Copilot wants to merge 1 commit into
mainfrom
copilot/fix-dependabot-mix-job-failure

Conversation

Copilot AI commented Sep 7, 2026

Copy link
Copy Markdown

What are you trying to accomplish?

Production Hex jobs intermittently fail with Unexpected end of file / CaseClauseError{term: :eof} unpacking tarballs, affecting a different set of public hex.pm packages on every run. Log analysis (per-repo diagnosis in the linked ticket) showed a WARN: Cannot write response from mitm'd client: ... write: broken pipe immediately preceding the corrupted downloads — pointing at the proxy-to-updater path rather than Hex.pm or the private registry.

Root cause: the disk cache's TeeReadCloser tees the response body to a file while it streams to the client. When the write to the mitm'd client fails partway (broken pipe), goproxy stops reading and closes the body early — but teeReader.Close() only checked for cache-file write errors, not whether the upstream body had actually been read to EOF. Since the file write itself hadn't failed, the (truncated) response was still committed to the cache DB as a valid, complete entry. Every subsequent request for that package/version within the same job (dependabot-core repeatedly resolves the same dependency graph) was then served the same truncated bytes, producing the erratic, package-agnostic tarball failures reported.

Anything you want to highlight for special attention from reviewers?

  • teeReader now tracks whether the source reader reached true io.EOF, independent of write success.
  • TeeReadCloser takes two callbacks — onComplete (full read, no write error) and onIncomplete (closed early, e.g. broken pipe) — instead of one. onIncomplete removes the orphaned partial file from disk so it isn't left dangling.
  • No change to the disk-full case: if the write to the cache file itself fails, behavior is unchanged (skip caching, leave existing file handling as-is).
resp.Body = TeeReadCloser(resp.Body, f,
    func() { /* full read: commit cache entry */ },
    func() { /* short read: remove partial file, don't cache */ },
)

How will you know you've accomplished your goal?

  • Added a regression test that closes a TeeReadCloser after a partial read (simulating a broken-pipe write failure) and asserts the entry is never added to cacheDB and no orphaned file remains in the cache directory.
  • Updated existing TestTeeReadCloser cases for the new two-callback signature, confirming onComplete/onIncomplete fire correctly for full reads, write failures, and short reads.

Checklist

  • I have run the complete test suite to ensure all tests and linters pass.
  • I have thoroughly tested my code changes to ensure they work as expected, including adding additional tests for new functionality.
  • I have written clear and descriptive commit messages.
  • I have provided a detailed description of the changes in the pull request, including the problem it addresses, how it fixes the problem, and any relevant details about the implementation.
  • I have ensured that the code is well-documented and easy to understand.

Co-authored-by: thavaahariharangit <164553783+thavaahariharangit@users.noreply.github.com>
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.

2 participants