Skip to content

fix(sdk): skip the segment ending at the cursor in Reader.WriteTo - #4233

Open
sujankota wants to merge 1 commit into
mainfrom
fix/sdk-writeto-boundary-segment
Open

sujankota wants to merge 1 commit into
mainfrom
fix/sdk-writeto-boundary-segment

Conversation

@sujankota

Copy link
Copy Markdown
Contributor

Proposed Changes

Part of DSPX-4886 (found while working on DSPX-5112/5114).

Reader.WriteTo skipped segments before the cursor with decryptedDataOffset+segSize < r.cursor. When the cursor sits exactly on a segment boundary, the segment ending there was not skipped: it was read, hash-checked and decrypted, then written as zero bytes. Output was correct, but over an HTTP range reader (LoadTDFFromURL, #4221) every partial decrypt starting on a boundary costs an extra segment fetch (2 MiB by default) plus a decrypt. ReadAt already uses <=; this aligns WriteTo.

Checklist

  • I have added or updated unit tests
  • I have added or updated integration tests (if appropriate)
  • I have added or updated documentation

Testing Instructions

cd sdk && go test -run 'TestTDF/Test_TDFReaderWriteToSkipsSegmentBeforeBoundary' . loads a 4-segment TDF through a byte-counting reader, seeks to the first boundary and asserts WriteTo reads exactly the encrypted bytes of segments 2–4. Before the fix it read one extra segment (49401 vs 32989 bytes). Full go test -race ./... in sdk passes, including the existing Test_TDFReaderMultiSegmentSeekReadWriteTo boundary cases.

WriteTo compared with < when skipping segments before the cursor, so a
start offset exactly on a segment boundary read, verified and decrypted
the preceding segment only to write zero bytes. Over an HTTP range
reader that is a wasted segment fetch per partial decrypt. ReadAt
already uses <=.

Part of DSPX-4886.

Signed-off-by: sujan kota <sujankota@gmail.com>
@sujankota
sujankota requested review from a team as code owners October 15, 2026 21:12
@github-actions github-actions Bot added the comp:sdk A software development kit, including library, for client applications and inter-service communicati label Oct 15, 2026
@coderabbitai

coderabbitai Bot commented Oct 15, 2026

Copy link
Copy Markdown

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 50 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration
  • Configuration used: Repository UI
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: 02871129-46c3-42f1-b53a-af99933384d7
📥 Commits

Reviewing files that changed from the base of the PR and between b048476 and b86c632.

📒 Files selected for processing: 2
  • sdk/tdf.go
  • sdk/tdf_readat_test.go
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

Copy link
Copy Markdown
Contributor
Benchmark results, click to expand

Bulk Benchmark Results

Metric Value
Total Decrypts 100
Successful Decrypts 100
Failed Decrypts 0
Total Time 348.150213ms
Throughput 287.23 requests/second

TDF3 Benchmark Results

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 35.82435316s
Average Latency 357.375526ms
Throughput 139.57 requests/second

@github-actions

Copy link
Copy Markdown
Contributor

⚠️ Govulncheck found vulnerabilities ⚠️

The following modules have known vulnerabilities:

  • otdfctl
  • sdk
  • service
  • lib/fixtures
  • tests-bdd

See the workflow run for details.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp:sdk A software development kit, including library, for client applications and inter-service communicati size/xs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants