fix(blockwise): return 4.08 on incomplete Block1 and make receive cache limit configurable#677
fix(blockwise): return 4.08 on incomplete Block1 and make receive cache limit configurable#677jkralik wants to merge 1 commit into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (12)
🚧 Files skipped from review as they are similar to previous changes (10)
📝 WalkthroughWalkthroughThis PR adds a configurable limit for blockwise receiving assemblies, threads it through config and transport setup, and updates blockwise receive-path handling and tests to return the new cache-full and incomplete-request responses. ChangesBounded blockwise receiving cache and validation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant BlockWise
participant ReceivingCache
participant Client
BlockWise->>ReceivingCache: getCachedReceivedMessage(token)
alt cache full on miss
ReceivingCache-->>BlockWise: errReceivingMessagesCacheFull
BlockWise-->>Client: ServiceUnavailable
else incomplete body
BlockWise-->>Client: RequestEntityIncomplete
else complete assembly
BlockWise->>ReceivingCache: store/update cached request
BlockWise-->>Client: Continue or next()
end
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 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. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #677 +/- ##
==========================================
+ Coverage 76.63% 76.72% +0.08%
==========================================
Files 77 77
Lines 6095 6208 +113
==========================================
+ Hits 4671 4763 +92
- Misses 1042 1058 +16
- Partials 382 387 +5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR addresses block-wise (RFC 7959 Block1) conformance gaps from issue #664 (point 2) by returning 4.08 (Request Entity Incomplete) when a Block1 transfer is completed with missing prior blocks, and by adding a configurable limit for the in-flight “receiving messages” blockwise assembly cache to prevent unbounded growth.
Changes:
- Return 4.08 for incomplete Block1 transfers (e.g., final block received while earlier blocks are missing).
- Add
BlockwiseReceivingMessagesCacheMaxEntriesconfig/option and plumb it through UDP/TCP/DTLS clients and servers into the blockwise layer. - Add regression tests (unit + UDP end-to-end) covering missing-previous-block behavior and bounded cache behavior.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| udp/server/server.go | Plumbs the new receiving-cache max-entries setting into server-side blockwise. |
| udp/client.go | Plumbs the new receiving-cache max-entries setting into client-side blockwise. |
| tcp/server/server.go | Plumbs the new receiving-cache max-entries setting into server-side blockwise. |
| tcp/client.go | Plumbs the new receiving-cache max-entries setting into client-side blockwise. |
| dtls/server/server.go | Plumbs the new receiving-cache max-entries setting into server-side blockwise. |
| dtls/client.go | Plumbs the new receiving-cache max-entries setting into client-side blockwise. |
| options/config/common.go | Adds defaulted common config field for receiving-cache max entries. |
| options/commonOptions.go | Introduces WithBlockwiseReceivingMessagesCacheMaxEntries option and applies it across transports. |
| options/commonOptions_test.go | Verifies the new option correctly applies into configs. |
| net/blockwise/blockwise.go | Implements cache limit + incomplete-transfer detection (4.08 path via existing error mapping). |
| net/blockwise/blockwise_test.go | Adds unit tests for missing-previous-block handling and cache bounding. |
| udp/blockwise_test.go | Adds UDP end-to-end regression test for missing-previous-block -> 4.08 behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tcp/client.go (1)
33-42: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWiring is correct.
bwis configured withSetReceivingMessagesCacheMaxEntriesmatching the upstreamBlockWisecontract and theBlockwiseReceivingMessagesCacheMaxEntriesOptconfig wiring shown inoptions/commonOptions.go.One gap: only UDP gets an end-to-end regression test for this feature (per stack layer 5); DTLS/TCP client and server factories have no equivalent test verifying the configured limit is actually applied at runtime (e.g., asserting cache rejection behavior over a real TCP/DTLS connection). Consider adding at least a lightweight test asserting
SetReceivingMessagesCacheMaxEntriesis invoked with the configured value for these transports too.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tcp/client.go` around lines 33 - 42, Add runtime coverage for the TCP/DTLS blockwise receiving cache limit, since only the UDP path is currently tested. Use the client/server factory setup in tcp/client.go (and the matching DTLS factory) to verify that blockwise.New is followed by SetReceivingMessagesCacheMaxEntries with the configured value, ideally through an end-to-end or lightweight transport test that exercises the actual factory wiring and cache rejection behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@net/blockwise/blockwise.go`:
- Line 707: The return in blockwise handling uses fmt.Errorf with a static
string, which triggers the perfsprint lint rule. Update the receiving messages
cache-full error in the relevant blockwise path to use errors.New instead of
fmt.Errorf, keeping the message identical and preserving the surrounding return
signature.
---
Nitpick comments:
In `@tcp/client.go`:
- Around line 33-42: Add runtime coverage for the TCP/DTLS blockwise receiving
cache limit, since only the UDP path is currently tested. Use the client/server
factory setup in tcp/client.go (and the matching DTLS factory) to verify that
blockwise.New is followed by SetReceivingMessagesCacheMaxEntries with the
configured value, ideally through an end-to-end or lightweight transport test
that exercises the actual factory wiring and cache rejection behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8dacf627-65ef-4144-8634-03563c890632
📒 Files selected for processing (12)
dtls/client.godtls/server/server.gonet/blockwise/blockwise.gonet/blockwise/blockwise_test.gooptions/commonOptions.gooptions/commonOptions_test.gooptions/config/common.gotcp/client.gotcp/server/server.goudp/blockwise_test.goudp/client.goudp/server/server.go
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
net/blockwise/blockwise.go (1)
737-767: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRelease the unused pooled message on the
loadedpathWhen
LoadOrStorereturnsloaded, the freshly acquiredmsgis dropped aftermgis reassigned to the cached entry.closeFnonly releases the guard semaphore, so the orphaned pooled message is never returned tob.cc. Addb.cc.ReleaseMessage(msg)on the loaded path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@net/blockwise/blockwise.go` around lines 737 - 767, The loaded path in blockwise.go leaks the freshly acquired pooled message because msg is replaced by the cached guard from LoadOrStore and closeFn only releases the guard. Update the request setup around newCloseStack, AcquireMessage, acquireMessageGuard, and the LoadOrStore loaded branch to return the unused msg to b.cc with ReleaseMessage before exiting that path, while keeping the existing guard release behavior unchanged.
🧹 Nitpick comments (1)
net/blockwise/blockwise.go (1)
866-922: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCI is failing on complexity for
processReceivedMessage.
gocyclo(16 > 15) and SonarCloud cognitive complexity (18 > 15) both fail on this function. Extract the offset/copy/completion logic (Lines 904-920) into a helper (e.g.appendBlockAndMaybeFinalize) so the top-level flow stays under the threshold and the checks pass.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@net/blockwise/blockwise.go` around lines 866 - 922, `processReceivedMessage` is over the cyclomatic/cognitive complexity threshold because the offset/copy/finalization branch is still inline. Extract the block handling from the `off := num * szx.Size()` section through the `continueReceivedRequest` return into a helper such as `appendBlockAndMaybeFinalize`, and have `processReceivedMessage` delegate to it. Keep the helper responsible for the copy, incomplete-body check, optional `finalizeReceivedRequest`, and `getSzx` update so the top-level flow is flattened and the complexity checks pass.Sources: Linters/SAST tools, Pipeline failures
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@net/blockwise/blockwise.go`:
- Around line 737-767: The loaded path in blockwise.go leaks the freshly
acquired pooled message because msg is replaced by the cached guard from
LoadOrStore and closeFn only releases the guard. Update the request setup around
newCloseStack, AcquireMessage, acquireMessageGuard, and the LoadOrStore loaded
branch to return the unused msg to b.cc with ReleaseMessage before exiting that
path, while keeping the existing guard release behavior unchanged.
---
Nitpick comments:
In `@net/blockwise/blockwise.go`:
- Around line 866-922: `processReceivedMessage` is over the cyclomatic/cognitive
complexity threshold because the offset/copy/finalization branch is still
inline. Extract the block handling from the `off := num * szx.Size()` section
through the `continueReceivedRequest` return into a helper such as
`appendBlockAndMaybeFinalize`, and have `processReceivedMessage` delegate to it.
Keep the helper responsible for the copy, incomplete-body check, optional
`finalizeReceivedRequest`, and `getSzx` update so the top-level flow is
flattened and the complexity checks pass.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 851d596e-32c8-4309-93a1-03df21f1f4e6
📒 Files selected for processing (12)
dtls/client.godtls/server/server.gonet/blockwise/blockwise.gonet/blockwise/blockwise_test.gooptions/commonOptions.gooptions/commonOptions_test.gooptions/config/common.gotcp/client.gotcp/server/server.goudp/blockwise_test.goudp/client.goudp/server/server.go
🚧 Files skipped from review as they are similar to previous changes (9)
- dtls/client.go
- tcp/server/server.go
- udp/client.go
- dtls/server/server.go
- tcp/client.go
- options/config/common.go
- options/commonOptions.go
- options/commonOptions_test.go
- udp/server/server.go
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
net/blockwise/blockwise.go (1)
765-790: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winCapture each guard in its own local. On the
loadedpath, bothappendClosecallbacks close over the reassignedmg, so they release the cached guard twice and never release the freshly acquired message. A concurrentLoadOrStorerace can hit this and trigger a semaphore over-release panic while leakingmsg.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@net/blockwise/blockwise.go` around lines 765 - 790, The LoadOrStore handling in the request-guard setup is capturing the reassigned mg in both appendClose callbacks, which can make the loaded path release the cached guard twice and skip releasing the newly acquired one. In the acquire/loaded branch around acquireMessageGuard, appendClose, and the requestGuard/cache.NewElement logic, store the initial guard and the cached guard in separate local variables and bind each close callback to the correct guard instance before returning.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@net/blockwise/blockwise.go`:
- Around line 765-790: The LoadOrStore handling in the request-guard setup is
capturing the reassigned mg in both appendClose callbacks, which can make the
loaded path release the cached guard twice and skip releasing the newly acquired
one. In the acquire/loaded branch around acquireMessageGuard, appendClose, and
the requestGuard/cache.NewElement logic, store the initial guard and the cached
guard in separate local variables and bind each close callback to the correct
guard instance before returning.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6c5d4c06-cc8d-40f1-ae6f-e62a2ee4112a
📒 Files selected for processing (12)
dtls/client.godtls/server/server.gonet/blockwise/blockwise.gonet/blockwise/blockwise_test.gooptions/commonOptions.gooptions/commonOptions_test.gooptions/config/common.gotcp/client.gotcp/server/server.goudp/blockwise_test.goudp/client.goudp/server/server.go
🚧 Files skipped from review as they are similar to previous changes (9)
- tcp/client.go
- dtls/server/server.go
- dtls/client.go
- tcp/server/server.go
- options/commonOptions.go
- udp/client.go
- options/commonOptions_test.go
- udp/server/server.go
- options/config/common.go
…he limit configurable
|
Fixes #664 point 2
Summary by CodeRabbit
New Features
0for unlimited) across UDP, TCP, and DTLS clients/servers.Bug Fixes
Tests