fix(sdk): stop GetManifest from splitting the key under the lock - #3941
Merged
Merged
Conversation
|
Warning Review limit reachedNext included review available in 59 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (3)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (8)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe SDK adds a concurrent chunked TDF writer with configurable manifest construction, key splitting, archive recovery, and manifest snapshots. The change also adds archive mutation tracking and broad unit and integration coverage. ChangesChunked TDF writer
Priority: ⬇️ Low Estimated code review effort: 5 (Critical) | ~90 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant Caller
participant ChunkedWriter
participant ArchiveWriter
participant KeySplitter
Caller->>ChunkedWriter: WriteSegment
ChunkedWriter->>ArchiveWriter: encrypt and archive segment
Caller->>ChunkedWriter: GetManifest or Finalize
ChunkedWriter->>KeySplitter: split DEK from snapshot
KeySplitter-->>ChunkedWriter: return shares
ChunkedWriter->>ArchiveWriter: finalize archive
ChunkedWriter-->>Caller: manifest and TDF data
Suggested reviewers: Merge Risk: ⚪ Minimal · up to No actionable merge-blocking risk remains after the documented concurrency and recovery coverage. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
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. I am a rabbit with segments to share Comment |
github-actions
Bot
added
comp:sdk
This was referenced Sep 1, 2026
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
dmihalcik-virtru
force-pushed
the
dspx-2604-12-getmanifest-lock
branch
from
September 1, 2026 03:36
912b24c to
e7d3d2d
Compare
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
dmihalcik-virtru
force-pushed
the
dspx-2604-12-getmanifest-lock
branch
from
September 1, 2026 15:08
e7d3d2d to
41aaa77
Compare
dmihalcik-virtru
force-pushed
the
dspx-2604-12-getmanifest-lock
branch
from
September 15, 2026 23:57
9288bef to
7fffc53
Compare
dmihalcik-virtru
force-pushed
the
dspx-2604-11-chunked-writer
branch
from
September 15, 2026 23:57
4664236 to
78f2ee4
Compare
Contributor
X-Test Failure Report |
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
dmihalcik-virtru
force-pushed
the
dspx-2604-11-chunked-writer
branch
from
September 16, 2026 00:13
78f2ee4 to
546457a
Compare
dmihalcik-virtru
force-pushed
the
dspx-2604-12-getmanifest-lock
branch
from
September 16, 2026 00:14
7fffc53 to
5a27c0e
Compare
Contributor
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
dmihalcik-virtru
force-pushed
the
dspx-2604-11-chunked-writer
branch
from
September 16, 2026 14:50
546457a to
74243e8
Compare
dmihalcik-virtru
force-pushed
the
dspx-2604-12-getmanifest-lock
branch
from
September 16, 2026 15:05
5a27c0e to
c08ed03
Compare
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
|
Contributor
GetManifest held the writer's read lock across KeySplitter.Split. A real splitter resolves KAS keys over the network, and RWMutex bars new readers once a writer is queued, so a single GetManifest stalled every concurrent WriteSegment commit -- and every other GetManifest -- for the duration of that I/O. That defeats the point of a writer built to let segments be encrypted in parallel. Split the manifest build in two. snapshotLocked resolves the emission order and copies each segment's metadata (values, not the *segmentSlot pointers WriteSegment mutates in place) out from under the lock, together with the archive-wide byte totals that read slots outside the emission order. buildManifest then runs entirely unlocked: everything else it touches -- dek, splitter, useHex -- is fixed at construction. GetManifest therefore describes the writer as of the snapshot rather than as of the return; a segment that lands mid-split is absent from that call and present in the next. Its interface doc now says so. Finalize keeps the lock throughout: it is terminal, and ErrChunkedWriteInFlight has already established that no write is outstanding, so there is no concurrency left to preserve and dropping the lock would only reopen the snapshot/archive race. Adds TestChunkedGetManifestDoesNotBlockWriteSegment, which blocks a splitter mid-Split and asserts a concurrent WriteSegment still completes. It deadlocks on the old shape. Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
dmihalcik-virtru
force-pushed
the
dspx-2604-12-getmanifest-lock
branch
from
September 17, 2026 16:47
c08ed03 to
c068a0c
Compare
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
|
Contributor
sujankota
approved these changes
Sep 21, 2026
github-merge-queue Bot
pushed a commit
that referenced
this pull request
Sep 22, 2026
> **Part 14 of 20** in the DSPX-2604 re-cut. Base branch: `dspx-2604-13-unresolved-kas`. > > This stack replaces #3782 / #3865 / #3921, which stay open and untouched > until it lands. Nothing here is a rebase of those branches — the work was > re-cut from the ticket so each PR stands on its own. ### Proposed Changes `sdk/experimental/tdf` carried its own copies of the manifest and assertion types -- `Manifest`, `Segment`, `KeyAccess`, `Assertion`, `Statement`, `AssertionKey` and the rest -- structurally identical to the ones in `sdk` but distinct to the type system, so anything crossing the boundary needed conversion. Two copies of the JWT signing and verification logic also had to be kept in step by hand. Replaces both files' definitions with type aliases. `sdk` owns the definitions; this package re-exports them. Every exported name and every method survives: `Assertion.Sign` / `Verify` / `GetHash`, `Statement.UnmarshalJSON`, `AssertionKey.IsEmpty` / `Algorithm`, `AssertionVerificationKeys.Get` / `IsEmpty` and the five `String()` methods all come along with the aliased types, so importers compile unchanged. A manifest produced here can now be handed to the stable SDK without conversion, which is what the follow-up delegation needs. Two deliberate non-aliases: `Policy`, `PolicyBody` and `PolicyAttribute` stay local. `sdk.PolicyObject` declares `Body` as an anonymous struct over an unexported element type, so there is no nameable sdk equivalent to alias to. Exporting those in `sdk` first would make the alias possible; that is a separate change. `IntegrityAlgorithm` stays a distinct `int` type. `sdk.IntegrityAlgorithm` is itself `= int`, so no method can be attached to it, and aliasing would silently drop `String()` from this package's public API. The underlying values match, so the two convert freely. `kSplitKeyType`, `kPolicyBindingAlg`, `kGMACPayloadLength` and `calculateSignature` are retained verbatim: this package still builds its own manifests and they have callers in `writer.go` and `key_access.go`. The change that removes those callers removes these too. No behavior change. The package's existing tests pass unmodified. ### 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 ./experimental/... -race ``` No behavior change — the point is that the package's existing tests pass unmodified against aliased types. <details> <summary><b>The full DSPX-2604 stack — 20 PRs</b></summary> | # | PR | Based on | |---|----|----------| | 01 | #3930 chore: bump go.work toolchain to go1.25.12 and simplify an rt_test condition | `main` | | 02 | #3931 feat(sdk): make the zipstream clock injectable for deterministic ZIP output | `main` | | 03 | #3932 fix(sdk): reject a zipstream write set that omits segment 0 | #3931 | | 04 | #3933 fix(sdk): map ReadAt plaintext offsets from cumulative segment sizes | `main` | | 05 | #3934 chore(sdk): extract integrityAlgorithmString, createPolicyBinding, signAssertions | `main` | | 06 | #3935 chore(sdk): add direct tests for createKeyAccess, encryptMetadata and tdfSalt | `main` | | 07 | #3936 fix(sdk): fill each segment with io.ReadFull and size the buffer to the input | `main` | | 08 | #3937 chore(cli): move streaming IO helpers into pkg | `main` | | 09 | #3938 fix(cli): stream encrypt instead of buffering the whole payload | #3937 | | 10 | #3939 fix(cli): stream decrypt and inspect instead of buffering | #3938 | | 11 | #3940 feat(sdk): add a chunked segment writer (experimental) | `dspx-2604-base-11` = #3932 + #3934 + #3935 | | 12 | #3941 fix(sdk): stop GetManifest from splitting the key under the lock | #3940 | | 13 | #3942 fix(sdk): reject a chunked split naming a KAS with no resolved public key | #3941 | | 14 | #3943 chore(sdk): alias experimental/tdf manifest and assertion types | #3942 | | 15 | #3944 fix(sdk): emit spec-compliant key access in experimental/tdf and delegate Writer | #3943 | | 16 | #3945 feat(sdk): accept io.Reader in CreateTDF and drop the 64 GB payload cap | #3936 | | 17 | #3946 chore(sdk): rewrite CreateTDF on top of the chunked writer | `dspx-2604-base-17` = #3944 + #3945 | | 18 | #3947 chore(sdk): drop dead TDFConfig fields and deprecate the TDFFormat enum | #3946 | | 19 | #3948 fix(cli): drop the encrypt-side stdin spool | `dspx-2604-base-19` = #3947 + #3939 | | 20 | #3949 feat(sdk): graduate the chunked writer to stable API | #3948 | **Reviewable in parallel right now**, since they sit directly on `main` and depend on nothing else: 01, 02, 04, 05, 06, 07, 08. **Why three PRs have a `dspx-2604-base-*` base.** A GitHub PR takes one base branch, but 11, 17 and 19 each build on more than one parent. The `base-*` branches are empty merge commits that exist only to join those parents so the PR diff shows exactly its own change and nothing else. They contain no code, have no PR of their own, and go away once their parents land — retarget the child onto `main` at that point. **Wants a cross-SDK xtest run before merge:** 15, 17 (and therefore 20). They touch the KAS wire format. **Red checks you may see are network flakes, not this stack.** Four distinct ones hit this batch and all clear on re-run: `golangci-lint config verify` timing out on `https://golangci-lint.run/.../golangci.v2.8.jsonschema.json` (fails the whole `go (<module>)` job and fail-fast cancels its siblings), the bats installer getting a 403, Docker Hub timing out on `keycloak/keycloak:26.4`, and `buf` reporting "the server hosted at that remote is unavailable" while the Java SDK generates sources. The `govulncheck` step also emits `##[error]` annotations against the go1.25.11 stdlib, but it is `continue-on-error: true` and never fails a job — 01 bumps the toolchain and clears those annotations. </details> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Compatibility** * Experimental TDF manifest and assertion types now align with the stable SDK, enabling direct interoperability without type conversion. * Existing assertion and manifest behavior is provided through the stable SDK definitions. * **Documentation** * Clarified the relationship between the experimental TDF package and the stable SDK. * Updated documentation for missing assertion verification keys to reflect current behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
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
2 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.
Proposed Changes
chunkedWriter.GetManifestheldmu.RLockfor its whole body, and thatbody calls
KeySplitter.Split. A real splitter resolves KAS public keysover the network, so every
WriteSegmentracing aGetManifestsat on thewrite lock until those round-trips finished.
sync.RWMutexbars newreaders once a writer is queued, so a second
GetManifestbehind thatWriteSegmentblocked as well -- one manifest snapshot could stall thewhole writer.
Splits the manifest build in two.
snapshotLockedresolves the emissionorder and copies each segment's metadata;
buildManifestthen works fromthat snapshot and touches no mutable writer state, so it needs no lock --
the dek, the splitter, the integrity algorithms and the signature encoding
are all fixed at construction.
GetManifestnow releases the read lock assoon as the snapshot is taken.
The snapshot copies
Segmentvalues rather than the*Segmentpointersw.segmentsholds.WriteSegmentmutates those in place when the archiveaccepts a write, so ranging over the pointers after releasing the lock
would be a data race, not merely a stale read.
Finalizedeliberately keeps the write lock across the split. It isterminal -- no
WriteSegmentmay succeed after it returns -- so there isno concurrency to preserve, and dropping the lock would open a window for a
segment to reach the archive after the snapshot that fixes the manifest.
This makes explicit a semantic that was already true:
GetManifestreturnsa point-in-time view. A segment committed after the snapshot is absent from
that manifest and present in the next one. The new test pins both halves,
along with the property that motivated the change -- a
WriteSegmentissued while a split is in flight completes rather than blocking. It
deadlocks to a 10s timeout against the previous locking and passes against
this one.
Checklist
Testing Instructions
The new test deadlocks to its 10s timeout against the previous locking and
passes against this one.
The full DSPX-2604 stack — 20 PRs
mainmainmainmainmainmainmaindspx-2604-base-11= #3932 + #3934 + #3935dspx-2604-base-17= #3944 + #3945dspx-2604-base-19= #3947 + #3939Reviewable in parallel right now, since they sit directly on
mainand depend onnothing else: 01, 02, 04, 05, 06, 07, 08.
Why three PRs have a
dspx-2604-base-*base. A GitHub PR takes one base branch,but 11, 17 and 19 each build on more than one parent. The
base-*branches are emptymerge commits that exist only to join those parents so the PR diff shows exactly its
own change and nothing else. They contain no code, have no PR of their own, and go
away once their parents land — retarget the child onto
mainat that point.Wants a cross-SDK xtest run before merge: 15, 17 (and therefore 20). They touch
the KAS wire format.
Red checks you may see are network flakes, not this stack. Four distinct ones hit
this batch and all clear on re-run:
golangci-lint config verifytiming out onhttps://golangci-lint.run/.../golangci.v2.8.jsonschema.json(fails the wholego (<module>)job and fail-fast cancels its siblings), the bats installer getting a 403,Docker Hub timing out on
keycloak/keycloak:26.4, andbufreporting "the serverhosted at that remote is unavailable" while the Java SDK generates sources. The
govulncheckstep also emits##[error]annotations against the go1.25.11 stdlib, butit is
continue-on-error: trueand never fails a job — 01 bumps the toolchain andclears those annotations.
Summary by CodeRabbit
New Features
Bug Fixes