fix(sdk): stop GetManifest from splitting the key under the lock - #3941
Open
dmihalcik-virtru wants to merge 1 commit into
Open
fix(sdk): stop GetManifest from splitting the key under the lock#3941dmihalcik-virtru wants to merge 1 commit into
dmihalcik-virtru wants to merge 1 commit into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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. 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
`chunkedWriter.GetManifest` held `mu.RLock` for its whole body, and that body calls `KeySplitter.Split`. A real splitter resolves KAS public keys over the network, so every `WriteSegment` racing a `GetManifest` sat on the write lock until those round-trips finished. `sync.RWMutex` bars new readers once a writer is queued, so a second `GetManifest` behind that `WriteSegment` blocked as well -- one manifest snapshot could stall the whole writer. Splits the manifest build in two. `snapshotLocked` resolves the emission order and copies each segment's metadata; `buildManifest` then works from that 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. `GetManifest` now releases the read lock as soon as the snapshot is taken. The snapshot copies `Segment` values rather than the `*Segment` pointers `w.segments` holds. `WriteSegment` mutates those in place when the archive accepts a write, so ranging over the pointers after releasing the lock would be a data race, not merely a stale read. `Finalize` deliberately keeps the write lock across the split. It is terminal -- no `WriteSegment` may succeed after it returns -- so there is no 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: `GetManifest` returns a 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 `WriteSegment` issued 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. Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
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-11-chunked-writer
branch
from
September 1, 2026 15:08
63a0a56 to
1f814c2
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:
|
Contributor
|
Contributor
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
1 participant
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.