Skip to content

fix(storage): compression upload & cache correctness fixes#3231

Merged
dobrac merged 7 commits into
mainfrom
dobrac/storage-compression-fixes
Jul 9, 2026
Merged

fix(storage): compression upload & cache correctness fixes#3231
dobrac merged 7 commits into
mainfrom
dobrac/storage-compression-fixes

Conversation

@dobrac

@dobrac dobrac commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Split out from #3113. Stacked on #3230 (units refactor).

Correctness fixes to the existing compressed-upload / cache / chunker code (all independent of the AWS feature):

  • verify frame checksum on decompress reader Close — an exact-size read never pulled the zstd footer through, so a footer-corrupt/truncated frame decoded as success. Close now drain-verifies.
  • reject unaligned compressed frame offsets — LocateCompressed returned the containing frame for a mid-frame offset, silently serving data from the frame start.
  • don't cache frames that fail to decompress — a right-sized-but-corrupt fetch passed the size-only writeback guard and poisoned the cache; now gated on decode success + evicted on failure.
  • explicit Content-Length on GCS multipart parts — ReaderFunc bodies went out chunked (411 on S3-compatible backends); multiSliceReader now reports Len.
  • reject GCS complete-upload 200 with error body — a 200 carrying an body was recorded as success.
  • Content-Length on empty GCS part — zero-length parts were chunked.
  • verify compressed frame before releasing chunk waiters — the chunker advanced waiters and cached before the CRC was checked.

Two GCS fix-tests (empty-part, Content-Length) that require the MinIO/fake-GCS testcontainer harness are exercised in the stacked tests PR, since that harness lives there.

Verified: shared storage suite green; orchestrator block suite green (Linux container).

@cla-bot cla-bot Bot added the cla-signed label Jul 8, 2026
@cursor

cursor Bot commented Jul 8, 2026

Copy link
Copy Markdown

PR Summary

Medium Risk
Changes sit on hot read/cache and upload commit paths; mis-handling could cause extra refetches or failed uploads, but the intent is to fail closed on integrity and commit errors rather than serve bad data.

Overview
This PR closes several correctness gaps in compressed storage reads, NFS frame caching, sandbox chunk fetching, and GCS XML multipart commits so corrupt or uncommitted data is not served, cached, or recorded as success.

Decompression and chunker: NewDecompressReader now drains the frame on Close so zstd/LZ4 footer checksums run even when the caller read exactly the uncompressed size; failures propagate. The chunker treats reader Close errors as fetch failures, skips progressive advance for compressed frames until verification completes, and no longer marks corrupt frames cached.

Frame lookup and cache: LocateCompressed rejects mid-frame uncompressed offsets (callers must align via LocateUncompressed). Compressed NFS writeback runs only after a successful decode; bad cache hits are evicted on Close via closeHookReader.

GCS multipart: Part uploads use explicit Content-Length (including empty parts) via multiSliceReader.Len; complete-upload responses with HTTP 200 but an XML <Error> body or truncated body are retried and ultimately rejected instead of treated as committed.

Reviewed by Cursor Bugbot for commit 16bc458. Bugbot is set up for automated code reviews on this repo. Configure here.

@codecov

codecov Bot commented Jul 8, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 80.59701% with 26 lines in your changes missing coverage. Please review.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...d/pkg/storage/storage_cache_seekable_compressed.go 70.45% 12 Missing and 1 partial ⚠️
packages/shared/pkg/storage/gcp_multipart.go 86.79% 4 Missing and 3 partials ⚠️
packages/shared/pkg/storage/compress_decode.go 75.00% 2 Missing and 1 partial ⚠️
packages/shared/pkg/storage/io_wrappers.go 62.50% 2 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces critical integrity and reliability fixes to the storage and caching mechanisms. It ensures that decompression and CRC errors are properly surfaced during reader closure, preventing corrupt or truncated frames from being cached or served. It also resolves S3/GCS multipart upload issues, such as handling HTTP 200 responses containing XML error payloads and avoiding chunked transfer encoding for empty parts. Feedback on the changes suggests making the new integrityReader's Close method idempotent by clearing the onFail callback after its first invocation.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread packages/shared/pkg/storage/storage_cache_seekable_compressed.go Outdated

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 22e613c139

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread packages/shared/pkg/storage/compress_frame_table.go
@dobrac dobrac force-pushed the dobrac/storage-compression-fixes branch from 22e613c to 91721cc Compare July 8, 2026 21:49
@dobrac dobrac changed the base branch from dobrac/refactor-units-to-shared to dobrac/storage-compress-framework July 8, 2026 21:49
@dobrac dobrac force-pushed the dobrac/storage-compress-framework branch from 744251a to 871235f Compare July 8, 2026 22:03
@dobrac dobrac force-pushed the dobrac/storage-compression-fixes branch from 91721cc to 3ad30bb Compare July 8, 2026 22:04
@dobrac dobrac force-pushed the dobrac/storage-compress-framework branch from 871235f to d0c5642 Compare July 8, 2026 22:06
@dobrac dobrac force-pushed the dobrac/storage-compression-fixes branch from 3ad30bb to 6220dcf Compare July 8, 2026 22:06
@dobrac dobrac force-pushed the dobrac/storage-compress-framework branch from d0c5642 to 3f7e772 Compare July 8, 2026 22:08
@dobrac dobrac force-pushed the dobrac/storage-compression-fixes branch from 6220dcf to afc4287 Compare July 8, 2026 22:08
Comment thread packages/shared/pkg/storage/compress_frame_table.go

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit afc4287. Configure here.

Comment thread packages/shared/pkg/storage/storage_cache_seekable_compressed.go Outdated
Base automatically changed from dobrac/storage-compress-framework to main July 8, 2026 22:31
@dobrac dobrac force-pushed the dobrac/storage-compression-fixes branch from afc4287 to d11cd84 Compare July 8, 2026 22:43
dobrac added 2 commits July 8, 2026 15:45
An exact-size read of a compressed frame never pulls the zstd footer
through the decoder, so a footer-corrupt or truncated frame decoded as
success. Close now drains to EOF (bounded to one frame) and surfaces the
codec's CRC/truncation error.
LocateCompressed returned the containing frame for a mid-frame offset,
so a decode silently served data from the frame start instead of the
requested position. Reject non-frame-aligned offsets; callers align via
LocateUncompressed.
@dobrac dobrac force-pushed the dobrac/storage-compression-fixes branch 3 times, most recently from f3070b8 to 14ac0e1 Compare July 8, 2026 23:07

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 14ac0e11bd

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread packages/shared/pkg/storage/gcp_multipart.go
A fetch returning corrupt bytes at the expected length passed the
size-only writeback guard and was cached, so every later read served
the corruption. Gate the writeback on decode success and evict a cached
frame that fails to decode, so reads recover once the source heals.
@dobrac dobrac force-pushed the dobrac/storage-compression-fixes branch 2 times, most recently from 01bc002 to a842e0f Compare July 8, 2026 23:33

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a842e0f793

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread packages/shared/pkg/storage/gcp_multipart.go Outdated
dobrac added 2 commits July 8, 2026 16:42
…ters

progressiveFetch advanced waiters as bytes streamed and only recorded
the reader Close error in metrics, so a corrupt compressed frame could
be served to guests and marked cached before its CRC was checked. Defer
waiter release for compressed chunks until after verification, and fail
the fetch on a Close/CRC error.
retryablehttp only sets ContentLength for ReaderFunc bodies that
implement LenReader; io.SectionReader does not, so compressed parts
went out chunked-encoded with no Content-Length. Real GCS tolerates
chunked uploads, but S3-compatible XML backends reject them with 411,
and an explicit length lets the server validate the payload up front.

Give multiSliceReader a Len method (picked up by retryablehttp and
persisted across retries) and send no body at all for empty parts — a
zero-length reader still counts as unknown length to net/http and
would be chunked. The MinIO-backed TestGCSXML* tests, which reject
chunked parts, now guard this against regression.
@dobrac dobrac force-pushed the dobrac/storage-compression-fixes branch from a842e0f to 35a5f0a Compare July 8, 2026 23:42
dobrac added 2 commits July 8, 2026 16:47
The GCS XML API (like S3) can return HTTP 200 with an <Error> body when
a commit fails server-side. completeUpload only checked the status code,
so a failed commit was recorded as success — storing a frame table for
an object that was never written. Parse the body and fail on an <Error>
root.
A zero-length *bytes.Reader is "unknown length" to net/http and is sent
chunked, which S3-compatible XML backends reject with 411. Pass no body
for empty parts so Content-Length: 0 is sent instead.
@dobrac dobrac force-pushed the dobrac/storage-compression-fixes branch from 35a5f0a to 16bc458 Compare July 8, 2026 23:47
@dobrac dobrac merged commit 980748f into main Jul 9, 2026
43 checks passed
@dobrac dobrac deleted the dobrac/storage-compression-fixes branch July 9, 2026 00:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants