Migrate to v0.11.0 of Dragonfly #88
No reviewers
Labels
No labels
blocked
bug
dependencies
documentation
duplicate
enhancement
github_actions
go
good first issue
help wanted
in-progress
invalid
question
ready-to-fix
source:discord
source:sentry
source:server-logs
triage-needed
wontfix
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
pokebedrock/pokebedrock-hub!88
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "v0.11.0-migration"
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?
New Features
Bug Fixes
Reliability
📝 Walkthrough
Walkthrough
The PR updates world-owner execution patterns, adds NPC and skin-loading infrastructure, refactors slapper and parkour lifecycles, removes obsolete APIs, adds release checks, and updates server parkour coordinates.
Changes
World-owner execution and asynchronous player flows
.cursor/rules/*,pokebedrock/form/*,pokebedrock/session/*,pokebedrock/handler/*,pokebedrock/moderation/*pokebedrock/pokebedrock.go,pokebedrock/hider/*,pokebedrock/watchdog/*NPC and slapper asset pipeline
pokebedrock/npc/*pokebedrock/slapper/*,pokebedrock/pokebedrock.go,go.mod,.golangci.ymlParkour transaction and persistence flow
pokebedrock/parkour/leaderboard.go,pokebedrock/parkour/manager.go,pokebedrock/command/parkour_reset.gopokebedrock/parkour/session.goresources/servers/*.jsonAPI cleanup and release validation
pokebedrock/queue/manager.go,pokebedrock/rank/service.go,pokebedrock/moderation/service.go,pokebedrock/kit/lobby.go.github/workflows/release.yml,config.example.toml,pokebedrock/config.gogo vet ./...andgo test ./...; watchdog configuration comments are updated for world-owner task semantics.Estimated code review effort: 4 (Complex) | ~60 minutes
Possibly related PRs
Suggested reviewers:
glancist🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches
🧪 Generate unit tests (beta)
v0.11.0-migrationThanks 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: 6
🧹 Nitpick comments (3)
🤖 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:
3a26861c-ddfe-4f6e-833d-182487df310b📥 Commits
Reviewing files that changed from the base of the PR and between
0a4483e3caand260e1bb378.⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum📒 Files selected for processing (44)
.cursor/rules/no-blocking-io-in-execworld.mdc.github/workflows/release.yml.golangci.ymlconfig.example.tomlgo.modpokebedrock/command/list.gopokebedrock/command/parkour_reset.gopokebedrock/config.gopokebedrock/form/moderate.gopokebedrock/handler/player.gopokebedrock/hider/hider.gopokebedrock/hider/hider_test.gopokebedrock/kit/lobby.gopokebedrock/moderation/service.gopokebedrock/npc/handler.gopokebedrock/npc/npc.gopokebedrock/npc/settings.gopokebedrock/npc/skin.gopokebedrock/npc/skin_test.gopokebedrock/parkour/leaderboard.gopokebedrock/parkour/leaderboard_test.gopokebedrock/parkour/manager.gopokebedrock/parkour/session.gopokebedrock/pokebedrock.gopokebedrock/queue/manager.gopokebedrock/rank/service.gopokebedrock/session/inflictions.gopokebedrock/session/ranks.gopokebedrock/slapper/manager.gopokebedrock/slapper/slapper.gopokebedrock/slapper/slapper_test.gopokebedrock/slapper/util.gopokebedrock/watchdog/watchdog.gopokebedrock/watchdog/watchdog_test.goresources/servers/black.jsonresources/servers/diamond.jsonresources/servers/gold.jsonresources/servers/moon.jsonresources/servers/pearl.jsonresources/servers/ruby.jsonresources/servers/sapphire.jsonresources/servers/silver.jsonresources/servers/sun.jsonresources/servers/white.json💤 Files with no reviewable changes (3)
@ -86,7 +87,6 @@ func (k Kick) Run(src cmd.Source, o *cmd.Output, _ *world.Tx) {## Reference implementations- `pokebedrock/session/ranks.go` (`rankWorker`) — HTTP runs on the worker🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the “GOOD” example self-contained.
Lines 43-44 use
pandhandlewithout defining them, so the example does not show how data is captured on-owner and cannot be copied as written. Wrap the capture in an explicitplayer.Docallback or state that these values come from an existing callback.🤖 Prompt for AI Agents
✅ Addressed in commit
8fd91c3@ -0,0 +21,4 @@if handles := NewManager().exemptedPlayers(); len(handles) != 0 {t.Fatalf("expected no exempted handles, got %d", len(handles))}}🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
Repository: smell-of-curry/pokebedrock-hub
Length of output: 291
🏁 Script executed:
Repository: smell-of-curry/pokebedrock-hub
Length of output: 6341
🏁 Script executed:
Repository: smell-of-curry/pokebedrock-hub
Length of output: 780
Clean up the test server registration
srv.Register(...)writes to the package-global registry inpokebedrock/srv/manager.go, and there’s no unregister/reset path. Add a test cleanup/reset helper or isolate the registry so this test doesn’t leak state into later ones.🤖 Prompt for AI Agents
✅ Addressed in commit
8fd91c3🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
Repository: smell-of-curry/pokebedrock-hub
Length of output: 11380
🏁 Script executed:
Repository: smell-of-curry/pokebedrock-hub
Length of output: 3243
🏁 Script executed:
Repository: smell-of-curry/pokebedrock-hub
Length of output: 20013
🏁 Script executed:
Repository: smell-of-curry/pokebedrock-hub
Length of output: 5287
Use
net.SplitHostPortfor the player IP.strings.Split(p.Addr().String(), ":")[0]breaks on bracketed IPv6 addresses like[::1]:19132and sends a malformedipvalue to the moderation API.net.SplitHostPorthandles both IPv4 and IPv6.🤖 Prompt for AI Agents
✅ Addressed in commit
8fd91c3@ -57,41 +59,48 @@ func NewManager(log *slog.Logger, w *world.World, cfg Config) *Manager {for _, course := range cfg.Courses {🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Blocking
world.Callduring startup has no timeout.world.Call(context.Background(), w, ...)will blockNewManagerindefinitely if the world owner never processes the transaction (e.g., world already shutting down or stalled). Since this runs on what's presumably a request/startup thread (not a world-owner callback), it should carry a bounded context so a stuck owner can't hang server initialization forever.🛡️ Suggested fix
📝 Committable suggestion
🤖 Prompt for AI Agents
✅ Confirmed as addressed by @smell-of-curry
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
Repository: smell-of-curry/pokebedrock-hub
Length of output: 1539
🏁 Script executed:
Repository: smell-of-curry/pokebedrock-hub
Length of output: 22229
🏁 Script executed:
Repository: smell-of-curry/pokebedrock-hub
Length of output: 9739
Resolve the transfer address before
Updatepokebedrock/queue/manager.go:263—Player.Transfercallsnet.ResolveUDPAddrsynchronously, sotransferServer.Address()can block this*world.Txcallback on DNS/lookup work. Pre-resolve the endpoint when the server config is loaded, or use a non-resolving transfer path.🤖 Prompt for AI Agents
Sources: Coding guidelines, MCP tools
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
Repository: smell-of-curry/pokebedrock-hub
Length of output: 792
🏁 Script executed:
Repository: smell-of-curry/pokebedrock-hub
Length of output: 14635
🌐 Web query:
Dragonfly v0.11.0 player.Transfer HandleTransfer nil cancel semantics💡 Result:
In the context of the Dragonfly project (specifically the Minecraft Bedrock Edition server software by df-mc), event handlers such as
HandleTransferuse anevent.Contextto manage the lifecycle of an event [1][2]. When an event handler is triggered, it receives a context that provides the ability to cancel the event. Callingctx.Cancel()on the provided*event.Contextstops the associated action from occurring [1]. In Dragonfly's event handling system, this is the standard mechanism for preventing an action [1]. While some Go APIs usenilto represent a lack of context or a default state,HandleTransferspecifically expects a valid*event.Context(or equivalent context type) to function correctly [1][3]. Providing anilcontext or attempting to interact with a null context in this handler is generally unsupported and will likely lead to a runtime panic when the server attempts to access the context's methods [1][2]. To cancel a transfer, you must use theCancel()method on a non-nil context object provided by the event system [1]. If you are experiencing issues with transfer semantics in Dragonfly, ensure you are utilizing theevent.Contextprovided by the function signature rather than passingnil[1]. If a transfer is not being cancelled as expected, confirm that the handler is correctly registered and thatctx.Cancel()is being invoked before the event completes [1].Citations:
🌐 Web query:
df-mc/dragonfly v0.11.0 player.Transfer HandleTransfer return nil canceled source💡 Result:
In the Dragonfly server software for Minecraft: Bedrock Edition, event cancellation is handled consistently across its
player.Handlerinterface [1]. When a handler method, such as a hypothetical transfer-related event, receives a*event.Contextor similar context type, you can cancel the action by callingctx.Cancel()[1][2]. Dragonfly does not typically use a "return nil" pattern to indicate cancellation in its event handler signatures [1][2]; instead, the idiomatic approach is to use the context provided as an argument to the method [1][3]. If you are encountering a situation where you intend to cancel a player transfer, you must: 1. Ensure your handler method accepts the appropriate context (e.g.,*player.Contextor*event.Context) [1][3]. 2. Invokectx.Cancel()within the body of that method to prevent the event from proceeding [1][2]. If you are referencing a specific API signature involvingHandleTransfer, verify that the context object passed to your function supports theCancel()method, as Dragonfly's event system is built upon thisevent.Contextpattern [1][4]. Returningnilgenerally does not trigger cancellation in Dragonfly's event architecture [1].Citations:
🏁 Script executed:
Repository: smell-of-curry/pokebedrock-hub
Length of output: 572
🏁 Script executed:
Repository: smell-of-curry/pokebedrock-hub
Length of output: 50387
🏁 Script executed:
Repository: smell-of-curry/pokebedrock-hub
Length of output: 11598
Cancelled transfers still consume the queue entry
pokebedrock/queue/manager.go:261-269player.TransferreturnsnilwhenHandleTransfercancels, so this path treats a cancelled transfer as success: the entry is already removed, it is not requeued, and auth is updated. Requeue or gate the success branch on the cancellation case.🤖 Prompt for AI Agents
Source: MCP tools
@ -0,0 +21,4 @@if handles := NewManager().exemptedPlayers(); len(handles) != 0 {t.Fatalf("expected no exempted handles, got %d", len(handles))}}Fixed in
8fd91c3. Added srv.Unregister and registered t.Cleanup so this test cannot leak its server into later tests.@ -86,7 +87,6 @@ func (k Kick) Run(src cmd.Source, o *cmd.Output, _ *world.Tx) {## Reference implementations- `pokebedrock/session/ranks.go` (`rankWorker`) — HTTP runs on the workerFixed in
8fd91c3. The example now explicitly states that it runs inside an existing owner callback where p is defined.Fixed in
8fd91c3. Player addresses now use net.SplitHostPort, with IPv4 and bracketed IPv6 regression coverage.Address-resolution concern fixed in
8fd91c3: server endpoints are resolved and canonicalized during config loading, off the world owner. The cancellation concern does not apply to this hub: PlayerHandler embeds NopHandler and does not override HandleTransfer, so no current path cancels Transfer.@ -57,41 +59,48 @@ func NewManager(log *slog.Logger, w *world.World, cfg Config) *Manager {for _, course := range cfg.Courses {Fixed in
8fd91c3. NewManager now receives a caller-bounded context using the configured world-owner timeout.@smell-of-curry, thanks for confirming — glad the fix and regression tests are in. Closing this one out.(=^･ω･^=)
@ -0,0 +21,4 @@if handles := NewManager().exemptedPlayers(); len(handles) != 0 {t.Fatalf("expected no exempted handles, got %d", len(handles))}}@smell-of-curry, thanks for confirming — that resolves the leaked global registry state concern. 🐇✨@ -57,41 +59,48 @@ func NewManager(log *slog.Logger, w *world.World, cfg Config) *Manager {for _, course := range cfg.Courses {@smell-of-curry, thanks for confirming! Using a caller-bounded context tied to the configured world-owner timeout is a solid approach — it ensures the timeout stays consistent with the rest of the watchdog/world-owner timeout configuration rather than a hardcoded value. Appreciate the quick fix.@ -86,7 +87,6 @@ func (k Kick) Run(src cmd.Source, o *cmd.Output, _ *world.Tx) {## Reference implementations- `pokebedrock/session/ranks.go` (`rankWorker`) — HTTP runs on the worker@smell-of-curry, confirmed — the updated example now clearly states it runs inside an existing owner callback wherepandhandleare already defined, which resolves the ambiguity. Thanks for the quick fix!Addressed the review-body nitpicks in
8fd91c3: slapper fallback now logs the original missing-asset error, loadServers uses the configured bounded world-owner timeout, and leaderboard save failures are logged. Full tests, vet, build, module verification, and diff checks pass.🧩 Analysis chain
🏁 Script executed:
Length of output: 12663
@smell-of-curry, confirmed both points:Address resolution — commit
8fd91c3updatesparseConfiginpokebedrock/srv/config.goto callnet.ResolveUDPAddr("udp", cfg.Address)and store the canonicalized address back intocfg.Addressat config-load time, with a new test (TestParseConfigRejectsInvalidAddress) covering invalid addresses. This moves the resolution off the world owner, soManager.Update'sworld.Txcallback no longer performs a blocking DNS lookup. Good fix.Cancellation concern — verified that
PlayerHandler(pokebedrock/handler/player.go) embedsplayer.NopHandlerand there's noHandleTransferoverride anywhere in the codebase (checkednpc/handler.goandhandler/inventory.gotoo, neither is relevant here). Since nothing cancels the transfer,Transferwon't returnnildue to cancellation in this hub today. Agreed this doesn't currently apply.Thanks for the clarification on both.
🧬 Code Graph Analysis Results
Relevant snippets
pokebedrock/locale/locale.go(lines 61-63)pokebedrock/authentication/factory.go(lines 60-68)pokebedrock/authentication/factory.go(lines 21-23)pokebedrock/authentication/factory.go(lines 80-84)pokebedrock/authentication/factory.go(line 14)pokebedrock/internal/constants.go(line 15)