fix: serve URL resource packs over RakNet, not client-side CDN #86
No reviewers
Labels
No labels
bug
dependencies
documentation
duplicate
enhancement
github_actions
go
good first issue
help wanted
invalid
question
wontfix
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
pokebedrock/gobds!86
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "fix/url-packs-raknet"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Problem
When the hub and a downstream server ship different versions of the same resource pack UUID (routine: bds-manager's
ProxyManager.generateConfigfetches the latest pokebedrock-res release on every proxy start, while the hub only updates on hub restart), players transferring to GOLD/SILVER got stuck on "Loading resource packs" or joined with a broken half-applied pack. Updating the hub to the same version, or installing the pack locally, "fixed" it — because both eliminate the client-side download.Root cause
resource.ReadURLrecords the source URL as the pack'sdownloadURL. The gophertunnel listener then advertises it asTexturePackInfo.DownloadURLinResourcePacksInfo, telling clients to fetch the pack themselves over HTTPS instead of over RakNet chunks.That CDN path only fires on a pack cache miss — i.e. exactly when the client has the hub's older
uuid_versioncached and encounters our newer one. And it fails on GitHub release asset URLs for two independent reasons:objects.githubusercontent.com; several client platforms (notably consoles) don't follow it..mcpackhasmanifest.jsonat the zip root; the client-side CDN loader expects the pack inside a subfolder (the same layoutresource.ReadURL's own docs call out).So: version match → cache hit → no download → looks fine. Version mismatch → cache miss → broken CDN download → stuck/corrupted.
Fix
readURLPack()downloads the archive proxy-side and compiles it viaresource.Read, leavingDownloadURLempty. The proxy now serves URL-configured packs over RakNet chunks — the same delivery path the hub (dragonfly,resource.ReadPath) uses, which works for every client and tolerates hub/downstream version skew.Test
TestReadURLPackHasNoDownloadURLserves a minimal.mcpackfromhttptestand asserts the loaded pack has noDownloadURLand correct content length — guards against regressing toresource.ReadURL.Notes
Summary by CodeRabbit
Bug Fixes
Tests
📝 Walkthrough
Walkthrough
URL resource packs now download through a local HTTP helper, are parsed without a
DownloadURL, and are covered by tests using a generated.mcpackserved from anhttptestserver. Documentation records pack delivery and version-skew behavior.Changes
Resource pack loading
gobds/user_config.go,gobds/user_config_test.go,AGENTS.mdUserConfig.packsusesreadURLPack, which validates HTTP responses and parses response bodies without download URL metadata. Tests verify the emptyDownloadURLand pack length, whileAGENTS.mddocuments resource-pack delivery and version skew.🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
✨ Finishing Touches
🧪 Generate unit tests (beta)
fix/url-packs-raknetThanks 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 helpto get the list of available commands.Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID:
c2a01343-7914-4731-9b1b-72ba7c5cdd4b📥 Commits
Reviewing files that changed from the base of the PR and between
4084560eb7andde0858e2cf.📒 Files selected for processing (3)
AGENTS.mdgobds/user_config.gogobds/user_config_test.go@ -127,6 +128,40 @@ func (c UserConfig) packs(log *slog.Logger) []*resource.Pack {return packs🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
Repository: smell-of-curry/gobds
Length of output: 11001
🏁 Script executed:
Repository: smell-of-curry/gobds
Length of output: 6006
🌐 Web query:
Go net/http http.Get default client timeout resource.Read io.ReadAll blocking body stall💡 Result:
In Go's net/http package, the http.DefaultClient has a Timeout of zero, which means it waits indefinitely [1][2][3]. This applies to the entire request lifecycle, including connection establishment, any redirects, and reading the response body [1][4][3]. If you use http.Get (which uses the DefaultClient) or any custom http.Client without a configured Timeout, a request can stall indefinitely if the server stops sending data or if there is a network partition during the reading of the body [5][6][3]. Key points regarding the behavior: 1. Blocking during io.ReadAll: The io.ReadAll function is a blocking operation [5][6][7]. It will continue to wait for data from the provided io.Reader until it reaches EOF or encounters an error [6][8]. If the connection remains open but the server halts transmission, io.ReadAll has no native knowledge of a "timeout" and will block the executing goroutine forever [5][6][7]. 2. Role of Client.Timeout: If you have configured a non-zero Timeout on your http.Client, that timer remains active even after the initial request (Get, Post, etc.) returns [1][2]. If io.ReadAll is still reading the response body when that total timeout duration is reached, the client will interrupt the reading process by canceling the request context, which causes the Body.Read call to return an error [1][4][3]. 3. Best Practices to Prevent Stalls: - Use a custom http.Client with a defined Timeout [9][3]. - If you need to handle long-running or streaming requests where a single total timeout is inappropriate, use context-aware requests (e.g., http.NewRequestWithContext) [10][9]. - For applications requiring granular control (such as ensuring reads respect a specific context cancellation), do not rely solely on io.ReadAll [7]. Consider using a custom reader that checks for context cancellation or wraps the read operation to ensure it does not hang indefinitely [7]. - Always close the response body (resp.Body.Close) to prevent resource leaks, which can contribute to socket exhaustion and application instability [1][10][5].
Citations:
github.com/golang/go@99d4927f/src/net/http/response.go🏁 Script executed:
Repository: smell-of-curry/gobds
Length of output: 7241
Bound the resource-pack fetch.
http.Gethas no timeout, andresource.Read(resp.Body)blocks on the body; sincelistenerFunccallsc.packs(srv.Log)during listener setup, a stalled URL can hang proxy startup. Use a client or request context with a finite deadline.🤖 Prompt for AI Agents
✅ Confirmed as addressed by @smell-of-curry
@ -130,0 +155,4 @@}defer func() {_ = resp.Body.Close()}()📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
Repository: smell-of-curry/gobds
Length of output: 249
🏁 Script executed:
Repository: smell-of-curry/gobds
Length of output: 9034
🏁 Script executed:
Repository: smell-of-curry/gobds
Length of output: 2379
Make the pack-layout wording consistent.
gobds/user_config.goshould say the zip needsmanifest.jsonat the archive root, andAGENTS.mdshould use the same wording; the current text contradicts the pack test and each other.📍 Affects 2 files
gobds/user_config.go#L131-L141(this comment)AGENTS.md#L13-L14🤖 Prompt for AI Agents
✅ Confirmed as addressed by @smell-of-curry
@ -127,6 +128,40 @@ func (c UserConfig) packs(log *slog.Logger) []*resource.Pack {return packsFixed in
fe37e39— readURLPack now uses an http.Client with a 5-minute Timeout, which bounds the connect and the body read inside resource.Read, so a stalled URL fails startup with an error instead of hanging it.@ -130,0 +155,4 @@}defer func() {_ = resp.Body.Close()}()Reworded in
fe37e39. Note the original text was intentional: the subfolder layout is a requirement of the CLIENT-side CDN download path (per resource.ReadURL's own docs), while server-side compilation (resource.Read here, ReadPath on the hub) accepts manifest.json at the archive root — which is what the test exercises. The comment and AGENTS.md now spell out that distinction explicitly so they no longer read as contradicting the test.@ -127,6 +128,40 @@ func (c UserConfig) packs(log *slog.Logger) []*resource.Pack {return packs@smell-of-curry, confirmed—using a five-minutehttp.Client.Timeoutbounds both the fetch andresource.Readbody consumption, preventing a stalled resource URL from blocking listener startup. Thanks for the fix.If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
@ -130,0 +155,4 @@}defer func() {_ = resp.Body.Close()}()@smell-of-curry, thanks for the detailed clarification. The distinction is correct: the subfolder layout applies only to the client-side CDN path, while proxy/hub-side compilation acceptsmanifest.jsonat the archive root. The rewording resolves the ambiguity with the test.✏️ Learnings added
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!