Skip to content

[Storage] Add sdk/storage/internal shared module - #27437

Open
tanyasethi-msft (tanyasethi-msft) wants to merge 10 commits into
mainfrom
storage/internal-module
Open

[Storage] Add sdk/storage/internal shared module#27437
tanyasethi-msft (tanyasethi-msft) wants to merge 10 commits into
mainfrom
storage/internal-module

Conversation

@tanyasethi-msft

@tanyasethi-msft tanyasethi-msft (tanyasethi-msft) commented Aug 21, 2026

Copy link
Copy Markdown
Member

Summary

Add the shared internal module for Azure Storage with:
- structuredmsg package: XSM/1.0 encoder/decoder consolidated from azblob, azfile, and azdatalake
- ValidateSeekableStreamAt0AndGetCount utility function
- Module skeleton files (CHANGELOG.md, LICENSE.txt, README.md, go.mod)
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).
6 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

@tanyasethi-msft

Copy link
Copy Markdown
Member Author

/azp run prepare-pipelines

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

@github-actions github-actions Bot added the Storage Storage Service (Queues, Blobs, Files) label Aug 21, 2026

Copilot AI 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.

Pull request overview

Adds a shared internal Storage module to consolidate structured-message functionality used by azblob, azfile, and azdatalake.

Changes:

  • Adds XSM/1.0 streaming and in-memory encoders/decoders.
  • Adds seekable-stream validation.
  • Adds module metadata, documentation, and tests.

Reviewed changes

Copilot reviewed 9 out of 10 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
sdk/storage/internal/version.go Defines the initial module version.
sdk/storage/internal/util.go Adds seekable-stream validation.
sdk/storage/internal/structuredmsg/structured_message.go Implements structured-message encoding and decoding.
sdk/storage/internal/structuredmsg/structured_message_test.go Tests structured-message behavior and failures.
sdk/storage/internal/README.md Documents the module.
sdk/storage/internal/LICENSE.txt Adds MIT licensing.
sdk/storage/internal/go.sum Records dependency checksums.
sdk/storage/internal/go.mod Defines the Go module and dependencies.
sdk/storage/internal/doc.go Documents the root package.
sdk/storage/internal/CHANGELOG.md Records the initial release.
Suppressed comments (2)

sdk/storage/internal/structuredmsg/structured_message.go:461

  • Replace the corrupted replacement character in this comment.
		// All segments done - emit trailer

sdk/storage/internal/structuredmsg/structured_message_test.go:1324

  • The segment header is 10 bytes (SMSegmentHeaderSize), so this byte-layout comment has the wrong size.
	truncated := encoded[:truncateAt]

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

Comment thread sdk/storage/internal/structuredmsg/structured_message.go
Comment thread sdk/storage/internal/structuredmsg/structured_message.go
Comment thread sdk/storage/internal/structuredmsg/structured_message.go Outdated
Comment thread sdk/storage/internal/structuredmsg/structured_message_test.go
Comment thread sdk/storage/internal/structuredmsg/structured_message.go Outdated
Comment thread sdk/storage/internal/structuredmsg/structured_message_test.go Outdated
Comment thread sdk/storage/internal/structuredmsg/structured_message_test.go Outdated
Copilot AI review requested due to automatic review settings August 21, 2026 07:36
- Add uint16 segment cap to SMEncode (matching streaming encoder)
- Convert io.EOF to io.ErrUnexpectedEOF on truncated segment reads
- Remove msgLen > 0 guard so zero-length headers are always rejected
- Assert io.ErrUnexpectedEOF in truncated decoder test
- Fix incorrect segment header size in test comments (6 -> 10 bytes)

Copilot AI 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.

Pull request overview

Copilot reviewed 9 out of 10 changed files in this pull request and generated 1 comment.

Suppressed comments (7)

sdk/storage/internal/structuredmsg/structured_message.go:54

  • numSegments can exceed the uint16 wire field (for example, 70,000 bytes with segmentSize == 1). The casts in the header and segment numbers then wrap, so this function returns a malformed payload that cannot round-trip. The azfile and azdatalake copies already increase segmentSize in this case; retain that protection in the consolidated implementation.
	numSegments := totalDataLen / segmentSize
	if totalDataLen%segmentSize != 0 {
		numSegments++

sdk/storage/internal/structuredmsg/structured_message.go:312

  • Converting contentLen to int before division corrupts lengths above MaxInt on 32-bit builds. For example, a 3 GiB stream becomes a negative segment count, causing the encoder to emit only framing with a wrapped count instead of the content. Keep length and segment-count arithmetic in int64, converting only the final bounded segment count after enforcing the uint16 limit.
// NewSMEncoder creates a streaming encoder that wraps the given content source.
// contentLen is the total size of the content (must be known upfront for the SM header).
// segmentSize specifies the max segment size; use 0 for the default (4MB).

sdk/storage/internal/structuredmsg/structured_message.go:806

  • A zero message length bypasses validation entirely, so changing an otherwise valid response header's length field to zero is accepted by the streaming decoder. The one-shot decoder correctly rejects the same payload, and zero cannot be a valid encoded length because framing alone is nonempty. Compare unconditionally using unsigned values to preserve the full wire range.
	}
}

sdk/storage/internal/structuredmsg/structured_message.go:820

  • When the source returns the last framing bytes together with a non-EOF error, this branch discards that error and continues decoding. Segment-data reads explicitly propagate this case, but a transport error at a header/footer/trailer boundary can currently be reported as success. Preserve non-EOF errors even when the frame became full.
	d.state = decStateDone
	return nil

sdk/storage/internal/structuredmsg/structured_message_test.go:1198

  • The segment header is 10 bytes (SMSegmentHeaderSize), not 6 bytes. Update the boundary description so it matches the calculation below.
	// Header (13 bytes) + segment header (10 bytes) + segment data (segSize bytes).

sdk/storage/internal/structuredmsg/structured_message_test.go:1301

  • The segment header is 10 bytes, not 6 bytes; the comment currently contradicts SMSegmentHeaderSize used by the test.
	// Truncate after segment data, partway through the segment footer.

sdk/storage/internal/structuredmsg/structured_message_test.go:1321

  • The segment header is 10 bytes, not 6 bytes; update this size breakdown to match the actual XSM framing.
	// Truncate after segment footer, partway through the message trailer.

Comment thread sdk/storage/internal/structuredmsg/structured_message.go Outdated
Copilot AI review requested due to automatic review settings August 21, 2026 07:44

Copilot AI 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.

Pull request overview

Copilot reviewed 9 out of 10 changed files in this pull request and generated 2 comments.

Suppressed comments (3)

sdk/storage/internal/structuredmsg/structured_message.go:206

  • XSM/1.0 requires at least one segment, but the one-shot decoder accepts a crafted 21-byte message with numSegments == 0 and an empty CRC64 trailer. Reject zero here so malformed messages cannot bypass all segment validation.
	numSegments := binary.LittleEndian.Uint16(smData[11:13])

sdk/storage/internal/structuredmsg/structured_message.go:816

  • This length check only compares bytes consumed through the trailer with the embedded length. If a valid encoded message is followed by extra bytes while retaining its original msgLen, the decoder reaches decStateDone without reading those bytes and accepts the body, even though XSM requires message length to match the complete HTTP body. Validate the underlying body's end (or pass its known content length into the decoder) before reporting success.
	// The consumed byte count must match the declared message length, so a stream that declares
	// fewer segments (leaving trailing bytes unvalidated) is rejected rather than silently accepted.
	if d.bytesRead != int64(d.msgLen) {

sdk/storage/internal/structuredmsg/structured_message.go:838

  • The full-frame check runs before handling err, so a source returning the final framing bytes together with a non-EOF error (for example, a transient net.Error) has that error silently discarded. This defeats the retry behavior preserved in the segment-data path. Only suppress EOF when it accompanies a complete frame; propagate other errors first.
		if d.frameHave >= d.frameNeed {
			return true, nil
		}
		if err != nil {

Comment thread sdk/storage/internal/structuredmsg/structured_message.go
Comment thread sdk/storage/internal/structuredmsg/structured_message.go
… GetBlob

Replace the GetProperties (HEAD) call in DownloadBuffer/DownloadFile with an
initial GetBlob (GET) request to determine blob size. For small blobs (<=4MB),
the entire content is returned in the initial response, eliminating an extra
round trip. For larger blobs, the first chunk is consumed from the initial
response and remaining chunks are downloaded in parallel.

This reduces download latency by ~50% for blobs up to 4MB, which covers the
vast majority of blob downloads (average blob size is 16KB).
Use uint32 loop counters in SMDecode and the streaming decoder
to prevent infinite loops when numSegments is 65535 (MaxUint16).
Copilot AI review requested due to automatic review settings August 21, 2026 08:04

Copilot AI 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.

Pull request overview

Copilot reviewed 13 out of 14 changed files in this pull request and generated no new comments.

Suppressed comments (4)

sdk/storage/azblob/blob/client.go:518

  • This newly added parallel path also leaves the retry reader and HTTP response body open when io.Copy fails. Close it before returning so failed range downloads do not leak connections.
			if _, err = io.Copy(shared.NewSectionWriter(writer, chunkStart+writerOffset, count), body); err != nil {
				return err
			}

sdk/storage/azblob/blob/client.go:393

  • This writes the response ETag into the caller-owned AccessConditions. Reusing the same options after the blob changes then sends the stale injected If-Match and can fail unexpectedly; concurrent reuse can also race. Clone the nested condition structs before adding the per-download ETag.
		} else if o.AccessConditions.ModifiedAccessConditions == nil {
			o.AccessConditions.ModifiedAccessConditions = &ModifiedAccessConditions{IfMatch: dr.ETag}
		} else if o.AccessConditions.ModifiedAccessConditions.IfMatch == nil {
			o.AccessConditions.ModifiedAccessConditions.IfMatch = dr.ETag

sdk/storage/azblob/blob/client.go:592

  • Skipping this block when downloaded == 0 leaves existing destination contents intact for an empty blob (or an empty range), despite DownloadFile promising to truncate the file to the downloaded size. Always compare and truncate, including to zero.
	if downloaded > 0 {

sdk/storage/azblob/blob/client.go:474

  • When copying a block fails, this returns without closing the retry reader and its active HTTP response body. With concurrent downloads, repeated read/write failures can leak connections and exhaust transport resources; close the body on this error path.

This issue also appears on line 516 of the same file.

			if _, err = io.Copy(shared.NewSectionWriter(writer, chunkStart, count), body); err != nil {
				return err
			}

- Reject numSegments == 0 in the one-shot decoder since XSM
  requires at least one segment
- Fix fillFrame to propagate non-EOF errors even when the frame
  buffer is full, instead of silently discarding them
Copilot AI review requested due to automatic review settings August 21, 2026 10:39

Copilot AI 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.

Pull request overview

Copilot reviewed 9 out of 10 changed files in this pull request and generated no new comments.

Suppressed comments (2)

sdk/storage/internal/structuredmsg/structured_message.go:745

  • The streaming decoder does not reject a zero segment count, unlike SMDecode. If an otherwise valid one-segment message has bytes 11–12 cleared, this decoder still parses segment 1, treats it as the last segment (1 >= 0), validates the CRCs and length, and accepts malformed metadata. Reject zero here and add the streaming equivalent of the one-shot validation test.
	d.numSegments = binary.LittleEndian.Uint16(buf[11:13])

sdk/storage/internal/structuredmsg/structured_message.go:875

  • After a truncated or otherwise invalid header, Read moves the decoder to decStateError, so this condition no longer holds and DecodeResult returns a non-nil, partially initialized result even though the documented prerequisite was never met. Track successful header parsing explicitly (for example, with a boolean set only after parseHeader validates the header) and return nil until then; add a case using the existing truncated-body test.
	if d.state == decStateHeader {
		return nil

… ETag and data verification tests

- Clone AccessConditions before mutating to avoid modifying caller's struct
- Close body on io.Copy failure in parallelDownload and parallelDownloadFrom
- DownloadFile now always truncates (including zero-byte blobs)
- Fake transport returns deterministic bytes and ETag header
- TestDownloadSmallBlobSkipsParallelRequests verifies actual buffer content
- TestDownloadETagConsistency verifies If-Match header on follow-up requests
Comment thread sdk/storage/internal/streams/streams.go
Keep the root internal package empty of functional code, grouping
related helpers into sub-packages as Joel suggested.
Copilot AI review requested due to automatic review settings August 22, 2026 05:50

Copilot AI 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.

Pull request overview

Copilot reviewed 13 out of 14 changed files in this pull request and generated 2 comments.

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

sdk/storage/internal/structuredmsg/structured_message.go:745

  • Unlike SMDecode, the streaming decoder does not reject a zero segment count. If an otherwise valid one-segment message has bytes 11–12 cleared, this decoder still reads segment 1, treats 1 >= 0 as completion, validates both CRCs, and accepts the malformed message with NumSegments == 0. Reject zero here before entering the segment state.
	d.numSegments = binary.LittleEndian.Uint16(buf[11:13])

Comment on lines +386 to +395
if o.AccessConditions != nil {
clone := *o.AccessConditions
ac = &clone
}
if ac.ModifiedAccessConditions == nil {
ac.ModifiedAccessConditions = &ModifiedAccessConditions{}
}
if ac.ModifiedAccessConditions.IfMatch == nil {
ac.ModifiedAccessConditions.IfMatch = dr.ETag
}
Comment on lines +342 to +343
// It uses an initial GetBlob (GET) request instead of GetProperties (HEAD) to determine the blob size,
// eliminating an extra round trip for small blobs where the entire content is returned in the initial response.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Storage Storage Service (Queues, Blobs, Files)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants