refactor(storage): consolidate IO reader wrappers#2570
Draft
Conversation
…ning - Use header.HugepageSize for uncompressed fetch alignment (semantically correct) - Stream NFS cache hits directly into ReadFrame instead of buffering in memory - Fix timer placement to cover full GetFrame (read + decompression) - Fix onRead callback: nil for compressed inner calls (prevents double-invoke), pass through for uncompressed (bytes are final) - Remove panic recovery from runFetch (never in main) - Remove low-value chunker tests subsumed by ConcurrentStress - Remove 4MB frame configs from benchmarks (targeting 2MB only) - Remove unused readCacheFile function Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ble NFS cache
- Remove dead flagsClient chain through chunker/build/template layers (~15 files)
- Delete ChunkerConfigFlag (unused after flagsClient removal)
- Delete mock_flagsclient_test.go
- Simplify GetUploadOptions: remove redundant intOr/strOr fallbacks (flags have defaults)
- Add GetCompressionType helper to frame_table.go, deduplicate compression type extraction
- Replace [16]byte{} with uuid.Nil and "rootfs.ext4" with storage.RootfsName in inspect-build
- Simplify UploadV4Header return pattern
- Remove onRead callback from legacy fullFetchChunker (FullFetch should not use progressive reads)
- Re-enable NFS cache in template cache.go
- Remove all fmt.Printf debug instrumentation from orchestrator
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…sionType threading
Add per-build file size and SHA-256 checksum to V4 headers, eliminating
the redundant Size() network call when opening upstream data files on
the read path. Checksums are computed for free by piggybacking on
CompressStream's existing frame iteration.
Remove the separate compressionType parameter threaded through
getBuild → newStorageDiff → NewChunker; the read path now derives
compression state from the per-mapping FrameTable directly.
V4 binary format change (not yet deployed):
[Metadata] [LZ4: numBuilds, builds(uuid+size+checksum),
numMappings, mappings...]
V3 path unchanged — falls back to Size() call when size is unknown.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Merge writeFrameToCache and writeChunkToCache into unified writeToCache with lock + atomic rename, used by all three cache write paths - Fix file descriptor leak in cache hit paths: defer f.Close() and wrap in NopCloser so ReadFrame's close doesn't double-close the fd - Add defer uploader.Close() in CompressStream so PartUploader file handles are released on error paths between Start() and Complete() - Make Close() idempotent via sync.Once on fsPartUploader and filePartWriter Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… lev-compression-final
The SHA-256 checksum in BuildFileInfo now covers uncompressed data, making it useful for end-to-end integrity verification of the original content. Updated inspect-build to use SHA-256 (replacing MD5) and verify checksums against the header. Fixed early-return lint warnings. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
GetUploadOptions now accepts fileType and useCase parameters, enriching the LD evaluation context so dashboard targeting rules can differentiate (e.g. compress memfile but not rootfs, or builds but not pauses). TemplateBuild accepts per-file opts directly instead of holding an ff reference. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Progressive path: write compressed chunks to NFS via AtomicImmutableFile concurrently with decompression instead of waiting until after it completes. Simple path: remove unnecessary copy in cacheFrameAsync since compressedBuf is allocated fresh per call and never modified after. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- block: lock fetchSession.advance so bytesReady.Store + cond.Broadcast happen under s.mu, closing a wait/broadcast race where waiters could miss a wakeup after observing stale bytesReady. - storage/compress: clamp maxUploadConcurrency to >=1 in compressStream (errgroup SetLimit(n+1) with n=0 leaves only the read-loop slot, deadlocking uploaders) and clamp cfg.FrameEncodeWorkers to >=1 in readLoop (per-part errgroup with SetLimit(0) rejects all Go() calls and hangs addFrame). Default COMPRESS_FRAME_ENCODE_WORKERS is 0, so this is a real deadlock path. - storage/compress: drop CompressBytes concurrency from 4 to 1. It's a test helper over an in-memory uploader — parallelism there adds nothing and invites questions. - storage/header: consolidate the two local `const = 4` declarations in serializeV4 / deserializeV4 into a single package-level v4SizePrefixLen at the top of serialization_v4.go.
… lev-compression-final
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Nil-safe FrameTable size accessors; strip compression suffix in peerserver LookupDiff; thread putOpts.Metadata through compressed GCS upload; defer fs sidecar write until after compressStream; revert toothless NotNil tests.
… lev-compression-final
Mirrors V4's collectAncestorBuilds gate so layered V3 builds can't make their hash→BuildID lookup globally visible before ancestor data files land in storage. Restores the invariant the deleted UploadTracker held.
… lev-compression-final
- peerserver/seekable + localDiff: use logical cache.Size(), not on-disk FileSize() (8x-inflated formula broke V3/V4 P2P resume). - storage_google.StoreFile: one outer WriteFromFileSystem timer covering all branches; drop the shadowed OneShot; tag with compression.type/level. - Privatize ResolveCompressConfig into sandbox; validate frame size against the real per-file block size from the snapshot diff header. - CI: add lz4 matrix; propagate COMPRESS_* to the base-template build via .env.test so the fixture matches the services.
Unify the scattered io.ReadCloser wrapper types into a single io_wrappers.go file with interface assertions as a TOC: offsetReader, instrumentedReader, cancelReader, sectionReader.
❌ 9 Tests Failed:
View the full list of 11 ❄️ flaky test(s)
To view more test analytics, go to the Test Analytics Dashboard |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The error recording logic in the instrumented reader's Close method is mutually exclusive, which may cause read errors to be suppressed if a close error also occurs. A data race exists on the byte count and error fields because they are accessed concurrently by Read and Close without synchronization. Passing a nil error to the completion check in the cache writeback reader creates a regression that prevents the proper caching of short final chunks.
Contributor
|
@cla-bot check |
|
The cla-bot has been summoned, and re-checked this pull request! |
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
4 participants
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.
Unify the scattered io.ReadCloser wrapper types into a single
io_wrappers.go file with interface assertions as a TOC:
offsetReader, instrumentedReader, cancelReader, sectionReader.
Pre-cursor to #2431, taking all non-functional refactoring into a separate PR