Skip to content

refactor(storage): consolidate IO reader wrappers - #2570

Merged
levb merged 11 commits into
mainfrom
lev-storage-rationalize
Jun 17, 2026
Merged

levb merged 11 commits into
mainfrom
lev-storage-rationalize

Conversation

@levb

@levb levb commented May 5, 2026

Copy link
Copy Markdown
Contributor

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

@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

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.

Comment thread packages/shared/pkg/storage/io_wrappers.go
Comment thread packages/shared/pkg/storage/io_wrappers.go
Comment thread packages/shared/pkg/storage/storage_cache_seekable.go Outdated
@tvi

tvi commented May 5, 2026

Copy link
Copy Markdown
Contributor

@cla-bot check

@cla-bot cla-bot Bot added the cla-signed label May 5, 2026
@cla-bot

cla-bot Bot commented May 5, 2026

Copy link
Copy Markdown

The cla-bot has been summoned, and re-checked this pull request!

Base automatically changed from lev-compression-final to main May 5, 2026 20:50
@levb
levb force-pushed the lev-storage-rationalize branch from 0919279 to b648e63 Compare May 12, 2026 18:02
@levb
levb force-pushed the lev-storage-rationalize branch 2 times, most recently from 244f0e1 to bfaa4cb Compare May 27, 2026 20:03
@levb
levb marked this pull request as ready for review May 27, 2026 22:17
@levb
levb force-pushed the lev-storage-rationalize branch from 57dfa58 to d38984a Compare June 1, 2026 17:46
Lev Brouk added 2 commits June 8, 2026 11:23
Replace the OpenRangeReader return type io.ReadCloser with a context-aware
RangeReader and collapse scattered reader wrappers (offset/section/capture/
observable/decompress) into a single io_wrappers.go. Drops the
cacheWriteThroughReader in favor of a reusable captureReader with optional
drain-on-close for codecs that stop short of EOF on their source.
@levb
levb force-pushed the lev-storage-rationalize branch from d38984a to 082285c Compare June 8, 2026 21:05
Comment thread .vscode/settings.json 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: 6d9b21d691

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread packages/shared/pkg/storage/io_wrappers.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: 5787a00ad0

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread packages/shared/pkg/storage/storage_cache_seekable_compressed.go Outdated
@chatgpt-codex-connector

Copy link
Copy Markdown

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

When NewDecompressingReader fails, closing the captureReader drained the
raw stream into its buffer and persisted those bytes to NFS, poisoning
the .frm cache on a failed miss. Close raw directly on the error path
to bypass the drain+writeback side effect.
@chatgpt-codex-connector

Copy link
Copy Markdown

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

Comment thread packages/orchestrator/pkg/sandbox/block/streaming_chunk.go Outdated
Comment thread packages/orchestrator/pkg/sandbox/template/peerclient/blob.go Outdated
Comment thread tests/integration/internal/tests/api/sandboxes/sandbox_rapid_pause_resume_test.go Outdated
@chatgpt-codex-connector

Copy link
Copy Markdown

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

@levb
levb enabled auto-merge (squash) June 17, 2026 16:22
@levb
levb merged commit b437d18 into main Jun 17, 2026
52 checks passed
@levb
levb deleted the lev-storage-rationalize branch June 17, 2026 16:43
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.

5 participants