build: bump lampshade to fix session desync that hung TestRoundTrip - #684
build: bump lampshade to fix session desync that hung TestRoundTrip#684reflog wants to merge 3 commits into
Conversation
An ACK frame arriving for an already-closed stream left its 4-byte payload unconsumed in the session-frame parser, which then misread those zero bytes as a padding header and silently discarded every remaining frame coalesced in the same session frame — permanently stalling unrelated live streams. With AckOnFirst (which our Wrap sets) this is easy to hit under concurrency and caused the rare 10-minute TestRoundTrip timeouts in CI. Fixed upstream in getlantern/lampshade#53. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 43 minutes Limit 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. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe pull request updates the direct ChangesDependency update
Estimated code review effort: 1 (Trivial) | ~2 minutes Merge Risk: ⚪ Minimal · up to This localized dependency update applies the upstream fix for the session desynchronization hang, with the affected tests passing; no actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
There was a problem hiding this comment.
Pull request overview
This PR updates the github.com/getlantern/lampshade dependency to a newer pseudo-version that includes an upstream fix intended to prevent rare TestRoundTrip session desynchronization hangs in CI.
Changes:
- Bump
github.com/getlantern/lampshadetov0.0.0-20260821104346-0863c5c50ba1. - Update
go.sumchecksums to match the new lampshade version.
Reviewed changes
Copilot reviewed 1 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| go.mod | Updates the required lampshade version to the fix commit. |
| go.sum | Updates module checksums for the new lampshade pseudo-version. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
getlantern/lampshade#53 is merged; point at the merge commit on master instead of the PR branch head. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Problem
CI on main occasionally fails with
TestRoundTriphanging for the full 10-minute test timeout (e.g. this run): 198/200 streams complete, one client goroutine stuck inio.ReadFullwaiting for exactly one 1443-byte (MaxDataLen) frame, the matching server echo stuck waiting for more data, and every send/recv loop healthy and idle.Root cause
A frame-parsing bug in lampshade's
recvLoop: an ACK frame arriving for an already-closed stream was skipped without consuming its 4-byte acked-frames payload. The parser then misread those zero bytes as a padding header and silently discarded every remaining frame coalesced in the same session frame — permanently stalling whichever live streams' frames followed. WithAckOnFirst: true(which ourlampshade.Wrapsets), the server sends an empty ACK for the first data frame of every inbound session frame, so any stream that writes and quickly closes leaves such a stale ACK in flight; underTestRoundTrip's 100 concurrent streams on one session this occasionally kills a live stream's echo frame.Fixed upstream in getlantern/lampshade#53 (with a regression test that reproduces the hang in ~15s before the fix). This PR bumps the dependency to that fix commit.
Testing
go test ./lampshade/ -count=10 -racepasses against the fix (previously the flake).-count=5); full lampshade suite passes.🤖 Generated with Claude Code
Summary by CodeRabbit