Great Cleanup & Improvements #70

2026-05-08T18:58:13Z

smell-of-curry commented

2026-05-08 18:58:13 +00:00

(Migrated from github.com)

All audit items are now fixed and the project builds cleanly. Summary of what changed:

Critical (race conditions / panics / leaks)

pokebedrock/queue/manager.go — replaced the atomic.Value[PriorityQueue] + RMW pattern with a sync.Mutex-guarded PriorityQueue. All queue mutations now go through the mutex; Update collects entries to remove during a snapshot iteration and removes them by identity (using entry.index with a stale-index fallback) rather than by stale indices, so concurrent AddPlayer/RemovePlayer calls can no longer drop the wrong heap node or corrupt invariants.
pokebedrock/session/ranks.go — collapsed the rankLoadQueue + rankUpdateCh double-queue into a single rankUpdateCh consumed by 3 worker goroutines. Ranks.Load is now fire-and-forget, so the close-channel panic race is structurally impossible. StopRankChannel waits on a WaitGroup for clean shutdown.
pokebedrock/moderation/service.go — eliminated defer resp.Body.Close() inside retry loops by extracting attempt, decodeInflictionsResponse, decodeNoContentResponse, and closeBody helpers; bodies are now closed per attempt. closed is atomic.Bool. All s.log.Debug(fmt.Sprintf(...)) were replaced with structured slog fields, removing the per-request bytes.Buffer allocation.
pokebedrock/rank/service.go — same defer-in-loop fix via fetchRoles helper. closed is atomic.Bool.
pokebedrock/vpn/service.go — same fix via handleResponse.
pokebedrock/restart/service.go — closed is atomic.Bool. cleanupExpiredEntries now selects on a done channel so Stop() shuts the goroutine down promptly instead of waiting up to a full minute.

Significant (performance / correctness)

pokebedrock/pokebedrock.go — replaced the switch statement in startTicking with independent if blocks so srv.UpdateAll, slapper.UpdateAll, and doAFKCheck actually run on every cadence they're supposed to.
pokebedrock/vpn/cache.go — rewritten with a debounced flusher goroutine: Set marks the cache dirty and signals; the flusher writes at most once per flushInterval (5 s), using a tmp+rename for atomic disk replacement. Stop() flushes pending writes deterministically. Service Stop() now calls cache.Stop().
pokebedrock/session/inflictions.go — inflictionWorker and inflictionLoadWorker now track in-flight requests with a sync.WaitGroup instead of a fragile fixed-capacity channel; StopInflictionWorker blocks on a real wait group with a 3-second cap rather than always sleeping the full timer.
pokebedrock/form/moderate.go — formatExpiry helper formats *int64 expiry timestamps correctly; the previous %d printed the pointer address.
pokebedrock/command/kick.go — o.Print now logs the actual victim name instead of the entire target list per iteration.

Build, vet, and tests pass clean. The only remaining lint warnings are pre-existing cognitive-complexity / exhaustive-switch flags on CreateInfliction.Submit and RemoveInfliction.Submit, which were untouched by this change set.

Summary by CodeRabbit

New Features
- Improved moderation messaging: per-player kick feedback and clearer “permanent vs expiry” formatting for inflictions.
- Rank updates gained a worker-pool flow plus extra rank-status helpers; parkour countdown handling is more reliable.
Bug Fixes
- Prevented HTTP connection/resource leaks and strengthened shutdown coordination across moderation, VPN, inflictions, ranks, and restart flows.
- Improved queue/transfer behavior and made AFK checks run consistently.
Performance
- Faster, more centralized HTTP request/retry handling; debounced VPN cache persistence.
UX
- More consistent queue boss-bar positions and queue-full/rank/infliction user feedback.

All audit items are now fixed and the project builds cleanly. Summary of what changed: ### Critical (race conditions / panics / leaks) - **`pokebedrock/queue/manager.go`** — replaced the `atomic.Value[PriorityQueue]` + RMW pattern with a `sync.Mutex`-guarded `PriorityQueue`. All queue mutations now go through the mutex; `Update` collects entries to remove during a snapshot iteration and removes them by identity (using `entry.index` with a stale-index fallback) rather than by stale indices, so concurrent `AddPlayer`/`RemovePlayer` calls can no longer drop the wrong heap node or corrupt invariants. - **`pokebedrock/session/ranks.go`** — collapsed the `rankLoadQueue` + `rankUpdateCh` double-queue into a single `rankUpdateCh` consumed by 3 worker goroutines. `Ranks.Load` is now fire-and-forget, so the close-channel panic race is structurally impossible. `StopRankChannel` waits on a `WaitGroup` for clean shutdown. - **`pokebedrock/moderation/service.go`** — eliminated `defer resp.Body.Close()` inside retry loops by extracting `attempt`, `decodeInflictionsResponse`, `decodeNoContentResponse`, and `closeBody` helpers; bodies are now closed per attempt. `closed` is `atomic.Bool`. All `s.log.Debug(fmt.Sprintf(...))` were replaced with structured `slog` fields, removing the per-request `bytes.Buffer` allocation. - **`pokebedrock/rank/service.go`** — same `defer`-in-loop fix via `fetchRoles` helper. `closed` is `atomic.Bool`. - **`pokebedrock/vpn/service.go`** — same fix via `handleResponse`. - **`pokebedrock/restart/service.go`** — `closed` is `atomic.Bool`. `cleanupExpiredEntries` now `select`s on a `done` channel so `Stop()` shuts the goroutine down promptly instead of waiting up to a full minute. ### Significant (performance / correctness) - **`pokebedrock/pokebedrock.go`** — replaced the `switch` statement in `startTicking` with independent `if` blocks so `srv.UpdateAll`, `slapper.UpdateAll`, and `doAFKCheck` actually run on every cadence they're supposed to. - **`pokebedrock/vpn/cache.go`** — rewritten with a debounced flusher goroutine: `Set` marks the cache dirty and signals; the flusher writes at most once per `flushInterval` (5 s), using a `tmp+rename` for atomic disk replacement. `Stop()` flushes pending writes deterministically. Service `Stop()` now calls `cache.Stop()`. - **`pokebedrock/session/inflictions.go`** — `inflictionWorker` and `inflictionLoadWorker` now track in-flight requests with a `sync.WaitGroup` instead of a fragile fixed-capacity channel; `StopInflictionWorker` blocks on a real wait group with a 3-second cap rather than always sleeping the full timer. - **`pokebedrock/form/moderate.go`** — `formatExpiry` helper formats `*int64` expiry timestamps correctly; the previous `%d` printed the pointer address. - **`pokebedrock/command/kick.go`** — `o.Print` now logs the actual victim name instead of the entire target list per iteration. Build, vet, and tests pass clean. The only remaining lint warnings are pre-existing cognitive-complexity / exhaustive-switch flags on `CreateInfliction.Submit` and `RemoveInfliction.Submit`, which were untouched by this change set.  ## Summary by CodeRabbit * **New Features** * Improved moderation messaging: per-player kick feedback and clearer “permanent vs expiry” formatting for inflictions. * Rank updates gained a worker-pool flow plus extra rank-status helpers; parkour countdown handling is more reliable. * **Bug Fixes** * Prevented HTTP connection/resource leaks and strengthened shutdown coordination across moderation, VPN, inflictions, ranks, and restart flows. * Improved queue/transfer behavior and made AFK checks run consistently. * **Performance** * Faster, more centralized HTTP request/retry handling; debounced VPN cache persistence. * **UX** * More consistent queue boss-bar positions and queue-full/rank/infliction user feedback.

glancist (Migrated from github.com) reviewed

2026-05-08 18:58:13 +00:00

coderabbitai[bot] commented

2026-05-08 18:58:29 +00:00

(Migrated from github.com)

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 9f2bde8e-721b-4400-b64c-7a743347c598

📥 Commits

Reviewing files that changed from the base of the PR and between b4bc3199c5 and fc24b35f33.

⛔ Files ignored due to path filters (1)

go.sum is excluded by !**/*.sum

📒 Files selected for processing (1)

go.mod

📝 Walkthrough

Walkthrough

Moves blocking I/O out of world transaction callbacks, centralizes HTTP handling, introduces atomic shutdown flags and deterministic worker shutdown, debounces VPN cache writes, and refactors queue and session worker pools. Parkour countdown callbacks receive player context inside ExecWorld to avoid entity lookups. Go toolchain and dependencies updated.

Changes

Concurrency Refactoring and Service I/O Isolation

Layer / File(s)	Summary
Documentation: ExecWorld I/O Pattern Rule `.cursor/rules/no-blocking-io-in-execworld.mdc`	Cursor rule documenting that `ExecWorld` and `cmd.Command.Run` callbacks execute on the world transaction goroutine and must not perform blocking I/O; shows fetch-offthread-apply examples and references `rankWorker` and `processInflictionRequest`.
Atomic Shutdown State Management `pokebedrock/moderation/service.go`, `pokebedrock/rank/service.go`, `pokebedrock/restart/service.go`	Services switch from plain `bool closed` to `atomic.Bool` and add done/shutdown channels where needed to enable prompt worker/goroutine termination.
Moderation Service HTTP Infrastructure `pokebedrock/moderation/service.go`	Centralizes HTTP request execution with `s.attempt()`, adds `ctxCloser`/`closeBody` helpers, and consolidates response decoding plus player-details worker/shutdown/send logic.
Moderation Form and Kick Integration `pokebedrock/form/moderate.go`, `pokebedrock/command/kick.go`	Create/RemoveInfliction now call moderation service outside ExecWorld; ExecWorld closures use safe type assertions; `formatExpiry()` added; kick spawns async per-target sync goroutines and logs failures with slog.
VPN Cache Debouncing and Service `pokebedrock/vpn/cache.go`, `pokebedrock/vpn/service.go`	Implements background flusher coalescing writes, adds `Stop()` to flush on shutdown and re-mark dirty on write error; CheckIP delegates per-attempt response handling to helper that ensures body closure.
Queue Manager Heap-Based Refactoring `pokebedrock/queue/manager.go`	Replaces atomic snapshot/queue-value with a mutex-protected heap owned by Manager; adds `snapshot()`, locked removal helpers, `positionFor()`, and rewrites `Update`/`NextPlayer`/`GetQueuePosition`.
Infliction Worker Shutdown Coordination `pokebedrock/session/inflictions.go`	Use shared `sync.WaitGroup` and `sync.Once` for deterministic `StopInflictionWorker()`; replace counting channel with per-request waitgroups and semaphore; centralize fetch+apply in `processInflictionRequest` using ExecWorld read then re-enter ExecWorld to apply.
Rank Worker Pool Coordination `pokebedrock/session/ranks.go`	Replace single worker + loader with fixed 3-worker pool consuming `rankUpdateCh`; add `rankShutdown` and `rankWorkerWG`; consolidate `processRankUpdate` and update Ranks enqueue/accessor APIs (`Ranks()`, `HasRank`, `HasRankOrHigher`, `LastRankFetch`).
Rank Service HTTP & Retry Restructure `pokebedrock/rank/service.go`	Extract `fetchRoles` single-attempt helper, drain/close response bodies per attempt, reorganize status handling, and use retry loop honoring atomic closed flag.
Restart Service Deterministic Shutdown `pokebedrock/restart/service.go`	Stop uses `sync.Once` to close done channel and set atomic closed flag; cleanup loop exits by selecting on done closure.
Main Tick Logic Restructuring `pokebedrock/pokebedrock.go`	Run server and slapper updates independently and execute AFK checks every tick after queue updates.
Parkour Session and Countdown ExecWorld Callbacks `pokebedrock/parkour/session.go`, `pokebedrock/parkour/manager.go`	Session captures `world.EntityHandle`; `beginCountdown` callbacks accept `player.Player` (and `*world.Tx` on done) and are invoked inside `handle.ExecWorld`; StartCourse/restartFromCheckpoint updated accordingly.
Go Toolchain and Dependency Updates `go.mod`	Bump toolchain to `go1.26.3`, update direct dependencies for dragonfly, gophertunnel, and golang.org/x/text, and remove the dragonfly replace directive.

Sequence Diagram(s)

The following diagram visualizes the core pattern introduced across multiple services: moving blocking I/O outside of world transaction callbacks and coordinating clean shutdown via atomic flags and channels.

sequenceDiagram
  participant App as Application
  participant ExecWorld as World Goroutine
  participant Service as Remote Service
  participant Infliction as inflictionWorker
  
  App->>ExecWorld: Call entity.ExecWorld(callback)
  ExecWorld->>ExecWorld: Read cheap state (XUID)
  ExecWorld->>Infliction: Send fetch request
  Infliction->>Service: Blocking HTTP call
  Service->>Infliction: Return inflictions
  Infliction->>ExecWorld: Re-enter via ExecWorld
  ExecWorld->>ExecWorld: Apply state changes
  Infliction->>Infliction: WaitGroup.Done()

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~65 minutes

smell-of-curry/pokebedrock-hub#45: Overlaps changes to the parkour subsystem and session/manager wiring with respect to countdown callback signatures and ExecWorld execution guarantees.
smell-of-curry/pokebedrock-hub#22: Modifies core gameplay services (moderation/service.go, queue/manager.go) that intersect with configuration and concurrency logic in this PR.

Poem

🐰 I hopped from ExecWorld's tight thread,
Sent I/O to fields instead,
Atomic flags guard the way,
Workers finish, queues hold sway,
A tidy burrowed code ahead.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 78.79% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check	❓ Inconclusive	The title 'Great Cleanup & Improvements' is vague and generic, using non-descriptive terms that do not convey meaningful information about the specific changes in this substantial multi-file refactoring.	Revise the title to be more specific and descriptive. Consider focusing on the main change, such as 'Fix race conditions with mutex-protected queue and atomic flags' or 'Refactor service concurrency: mutex queues, atomic flags, and defer-in-loop fixes'.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch great-improvements-and-cleanup

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 golangci-lint (2.12.2)

Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions
The command is terminated due to an error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[![Review Change Stack](https://storage.googleapis.com/coderabbit_public_assets/review-stack-in-coderabbit-ui.svg)](https://app.coderabbit.ai/change-stack/smell-of-curry/pokebedrock-hub/pull/70?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack)   > [!CAUTION] > ## Review failed > > The pull request is closed.  <details> <summary>ℹ️ Recent review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: defaults **Review profile**: CHILL **Plan**: Pro **Run ID**: `9f2bde8e-721b-4400-b64c-7a743347c598` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between b4bc3199c5000dafc3a612885ea12470e85c9a0d and fc24b35f339f08684239f5f47c598bd918022a23. </details> <details> <summary>⛔ Files ignored due to path filters (1)</summary> * `go.sum` is excluded by `!**/*.sum` </details> <details> <summary>📒 Files selected for processing (1)</summary> * `go.mod` </details> </details> ---  <details> <summary>📝 Walkthrough</summary> ## Walkthrough Moves blocking I/O out of world transaction callbacks, centralizes HTTP handling, introduces atomic shutdown flags and deterministic worker shutdown, debounces VPN cache writes, and refactors queue and session worker pools. Parkour countdown callbacks receive player context inside ExecWorld to avoid entity lookups. Go toolchain and dependencies updated. ## Changes **Concurrency Refactoring and Service I/O Isolation** |Layer / File(s)|Summary| |---|---| |**Documentation: ExecWorld I/O Pattern Rule** `.cursor/rules/no-blocking-io-in-execworld.mdc`|Cursor rule documenting that `ExecWorld` and `cmd.Command.Run` callbacks execute on the world transaction goroutine and must not perform blocking I/O; shows fetch-offthread-apply examples and references `rankWorker` and `processInflictionRequest`.| |**Atomic Shutdown State Management** `pokebedrock/moderation/service.go`, `pokebedrock/rank/service.go`, `pokebedrock/restart/service.go`|Services switch from plain `bool closed` to `atomic.Bool` and add done/shutdown channels where needed to enable prompt worker/goroutine termination.| |**Moderation Service HTTP Infrastructure** `pokebedrock/moderation/service.go`|Centralizes HTTP request execution with `s.attempt()`, adds `ctxCloser`/`closeBody` helpers, and consolidates response decoding plus player-details worker/shutdown/send logic.| |**Moderation Form and Kick Integration** `pokebedrock/form/moderate.go`, `pokebedrock/command/kick.go`|Create/RemoveInfliction now call moderation service outside ExecWorld; ExecWorld closures use safe type assertions; `formatExpiry()` added; kick spawns async per-target sync goroutines and logs failures with slog.| |**VPN Cache Debouncing and Service** `pokebedrock/vpn/cache.go`, `pokebedrock/vpn/service.go`|Implements background flusher coalescing writes, adds `Stop()` to flush on shutdown and re-mark dirty on write error; CheckIP delegates per-attempt response handling to helper that ensures body closure.| |**Queue Manager Heap-Based Refactoring** `pokebedrock/queue/manager.go`|Replaces atomic snapshot/queue-value with a mutex-protected heap owned by Manager; adds `snapshot()`, locked removal helpers, `positionFor()`, and rewrites `Update`/`NextPlayer`/`GetQueuePosition`.| |**Infliction Worker Shutdown Coordination** `pokebedrock/session/inflictions.go`|Use shared `sync.WaitGroup` and `sync.Once` for deterministic `StopInflictionWorker()`; replace counting channel with per-request waitgroups and semaphore; centralize fetch+apply in `processInflictionRequest` using ExecWorld read then re-enter ExecWorld to apply.| |**Rank Worker Pool Coordination** `pokebedrock/session/ranks.go`|Replace single worker + loader with fixed 3-worker pool consuming `rankUpdateCh`; add `rankShutdown` and `rankWorkerWG`; consolidate `processRankUpdate` and update Ranks enqueue/accessor APIs (`Ranks()`, `HasRank`, `HasRankOrHigher`, `LastRankFetch`).| |**Rank Service HTTP & Retry Restructure** `pokebedrock/rank/service.go`|Extract `fetchRoles` single-attempt helper, drain/close response bodies per attempt, reorganize status handling, and use retry loop honoring atomic closed flag.| |**Restart Service Deterministic Shutdown** `pokebedrock/restart/service.go`|Stop uses `sync.Once` to close done channel and set atomic closed flag; cleanup loop exits by selecting on done closure.| |**Main Tick Logic Restructuring** `pokebedrock/pokebedrock.go`|Run server and slapper updates independently and execute AFK checks every tick after queue updates.| |**Parkour Session and Countdown ExecWorld Callbacks** `pokebedrock/parkour/session.go`, `pokebedrock/parkour/manager.go`|Session captures `*world.EntityHandle`; `beginCountdown` callbacks accept `*player.Player` (and `*world.Tx` on done) and are invoked inside `handle.ExecWorld`; StartCourse/restartFromCheckpoint updated accordingly.| |**Go Toolchain and Dependency Updates** `go.mod`|Bump toolchain to `go1.26.3`, update direct dependencies for dragonfly, gophertunnel, and golang.org/x/text, and remove the dragonfly replace directive.| ## Sequence Diagram(s) The following diagram visualizes the core pattern introduced across multiple services: moving blocking I/O outside of world transaction callbacks and coordinating clean shutdown via atomic flags and channels. ```mermaid sequenceDiagram participant App as Application participant ExecWorld as World Goroutine participant Service as Remote Service participant Infliction as inflictionWorker App->>ExecWorld: Call entity.ExecWorld(callback) ExecWorld->>ExecWorld: Read cheap state (XUID) ExecWorld->>Infliction: Send fetch request Infliction->>Service: Blocking HTTP call Service->>Infliction: Return inflictions Infliction->>ExecWorld: Re-enter via ExecWorld ExecWorld->>ExecWorld: Apply state changes Infliction->>Infliction: WaitGroup.Done() ``` ## Estimated code review effort 🎯 4 (Complex) | ⏱️ ~65 minutes ## Possibly related PRs - [smell-of-curry/pokebedrock-hub#45](https://github.com/smell-of-curry/pokebedrock-hub/pull/45): Overlaps changes to the parkour subsystem and session/manager wiring with respect to countdown callback signatures and ExecWorld execution guarantees. - [smell-of-curry/pokebedrock-hub#22](https://github.com/smell-of-curry/pokebedrock-hub/pull/22): Modifies core gameplay services (moderation/service.go, queue/manager.go) that intersect with configuration and concurrency logic in this PR. ## Poem > 🐰 I hopped from ExecWorld's tight thread, > Sent I/O to fields instead, > Atomic flags guard the way, > Workers finish, queues hold sway, > A tidy burrowed code ahead. </details>   <details> <summary>🚥 Pre-merge checks | ✅ 3 | ❌ 2</summary> ### ❌ Failed checks (1 warning, 1 inconclusive) | Check name | Status | Explanation | Resolution | | :----------------: | :------------- | :---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | :------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | | Docstring Coverage | ⚠️ Warning | Docstring coverage is 78.79% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. | | Title check | ❓ Inconclusive | The title 'Great Cleanup & Improvements' is vague and generic, using non-descriptive terms that do not convey meaningful information about the specific changes in this substantial multi-file refactoring. | Revise the title to be more specific and descriptive. Consider focusing on the main change, such as 'Fix race conditions with mutex-protected queue and atomic flags' or 'Refactor service concurrency: mutex queues, atomic flags, and defer-in-loop fixes'. | <details> <summary>✅ Passed checks (3 passed)</summary> | Check name | Status | Explanation | | :------------------------: | :------- | :----------------------------------------------------------------------- | | Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. | | Linked Issues check | ✅ Passed | Check skipped because no linked issues were found for this pull request. | | Out of Scope Changes check | ✅ Passed | Check skipped because no linked issues were found for this pull request. | </details> ✏️ Tip: You can configure your own custom pre-merge checks in the settings. </details>   <details> <summary>✨ Finishing Touches</summary> <details> <summary>🧪 Generate unit tests (beta)</summary> - [ ]  Create PR with unit tests - [ ]  Commit unit tests in branch `great-improvements-and-cleanup` </details> </details>   > [!WARNING] > There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. > > <details> > <summary>🔧 golangci-lint (2.12.2)</summary> > > Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions > The command is terminated due to an error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions > > > > > </details>   --- Thanks for using [CodeRabbit](https://coderabbit.ai?utm_source=oss&utm_medium=github&utm_campaign=smell-of-curry/pokebedrock-hub&utm_content=70)! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. <details> <summary>❤️ Share</summary> - [X](https://twitter.com/intent/tweet?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A&url=https%3A//coderabbit.ai) - [Mastodon](https://mastodon.social/share?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A%20https%3A%2F%2Fcoderabbit.ai) - [Reddit](https://www.reddit.com/submit?title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&text=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code.%20Check%20it%20out%3A%20https%3A//coderabbit.ai) - [LinkedIn](https://www.linkedin.com/sharing/share-offsite/?url=https%3A%2F%2Fcoderabbit.ai&mini=true&title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&summary=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code) </details> Comment `@coderabbitai help` to get the list of available commands and usage tips.

smell-of-curry commented

2026-05-08 18:59:17 +00:00

(Migrated from github.com)

@bugbot review

copilot-pull-request-reviewer[bot] (Migrated from github.com) reviewed

2026-05-08 19:05:53 +00:00

copilot-pull-request-reviewer[bot] (Migrated from github.com) left a comment

Pull request overview

This PR focuses on eliminating shutdown races/resource leaks and improving correctness/performance in several background services (queueing, ranks/inflictions workers, moderation/rank/VPN HTTP clients), while also tightening a few gameplay/admin UX details.

Changes:

Reworked several services to avoid races/leaks (atomic closed flags, per-attempt HTTP body closing, deterministic worker shutdown patterns).
Fixed correctness/performance issues in ticking/queue management and VPN cache persistence (debounced flush + atomic rename).
Improved moderation/kick/form UX correctness (expiry formatting, off-thread HTTP sync from transaction goroutine).

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
pokebedrock/vpn/service.go	Refactors response handling to close bodies per attempt; Stop now flushes cache.
pokebedrock/vpn/cache.go	Rewrites cache persistence with a debounced flusher and atomic tmp+rename writes.
pokebedrock/session/ranks.go	Simplifies rank fetching into a single work queue with a worker pool + WaitGroup shutdown.
pokebedrock/session/inflictions.go	Replaces fixed-capacity “in-flight” tracking with WaitGroups for safer shutdown.
pokebedrock/restart/service.go	Makes shutdown signalling more responsive (done channel) and closed flag atomic.
pokebedrock/rank/service.go	Moves per-attempt HTTP logic into a helper and makes `closed` atomic.
pokebedrock/queue/manager.go	Replaces atomic RMW queue pattern with mutex-guarded heap and snapshot-based iteration.
pokebedrock/pokebedrock.go	Fixes ticking cadence so multiple periodic tasks can run in the same tick.
pokebedrock/moderation/service.go	Removes defer-in-retry-loop body leaks; adds helpers and structured logging; atomic closed.
pokebedrock/form/moderate.go	Fixes expiry formatting for `*int64` and moves moderation calls off transaction goroutine.
pokebedrock/command/kick.go	Moves moderation sync off transaction goroutine and fixes logging target output.
.cursor/rules/no-blocking-io-in-execworld.mdc	Adds repo guidance on avoiding blocking I/O in ExecWorld/command Run contexts.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

## Pull request overview This PR focuses on eliminating shutdown races/resource leaks and improving correctness/performance in several background services (queueing, ranks/inflictions workers, moderation/rank/VPN HTTP clients), while also tightening a few gameplay/admin UX details. **Changes:** - Reworked several services to avoid races/leaks (atomic closed flags, per-attempt HTTP body closing, deterministic worker shutdown patterns). - Fixed correctness/performance issues in ticking/queue management and VPN cache persistence (debounced flush + atomic rename). - Improved moderation/kick/form UX correctness (expiry formatting, off-thread HTTP sync from transaction goroutine). ### Reviewed changes Copilot reviewed 12 out of 12 changed files in this pull request and generated 5 comments. <details> <summary>Show a summary per file</summary> | File | Description | | ---- | ----------- | | pokebedrock/vpn/service.go | Refactors response handling to close bodies per attempt; Stop now flushes cache. | | pokebedrock/vpn/cache.go | Rewrites cache persistence with a debounced flusher and atomic tmp+rename writes. | | pokebedrock/session/ranks.go | Simplifies rank fetching into a single work queue with a worker pool + WaitGroup shutdown. | | pokebedrock/session/inflictions.go | Replaces fixed-capacity “in-flight” tracking with WaitGroups for safer shutdown. | | pokebedrock/restart/service.go | Makes shutdown signalling more responsive (done channel) and closed flag atomic. | | pokebedrock/rank/service.go | Moves per-attempt HTTP logic into a helper and makes `closed` atomic. | | pokebedrock/queue/manager.go | Replaces atomic RMW queue pattern with mutex-guarded heap and snapshot-based iteration. | | pokebedrock/pokebedrock.go | Fixes ticking cadence so multiple periodic tasks can run in the same tick. | | pokebedrock/moderation/service.go | Removes defer-in-retry-loop body leaks; adds helpers and structured logging; atomic closed. | | pokebedrock/form/moderate.go | Fixes expiry formatting for `*int64` and moves moderation calls off transaction goroutine. | | pokebedrock/command/kick.go | Moves moderation sync off transaction goroutine and fixes logging target output. | | .cursor/rules/no-blocking-io-in-execworld.mdc | Adds repo guidance on avoiding blocking I/O in ExecWorld/command Run contexts. | </details> --- 💡 <a href="/smell-of-curry/pokebedrock-hub/new/main?filename=.github/instructions/*.instructions.md" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Add Copilot custom instructions</a> for smarter, more guided reviews. <a href="https://docs.github.com/en/copilot/customizing-copilot/adding-repository-custom-instructions-for-github-copilot" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Learn how to get started</a>.

pokebedrock/moderation/service.go

The shutdown path drains activeRequests in the worker (for range len(activeRequests) { <-activeRequests }), but each in-flight goroutine also does <-activeRequests in its deferred cleanup. This can deadlock/leak goroutines: the worker may consume the tokens first (or block waiting for tokens that goroutines already consumed), and then the goroutines block forever on <-activeRequests. Use a sync.WaitGroup (as done in session/inflictions.go) or another mechanism where only one side is responsible for decrementing, and have Stop() wait on that with a timeout if desired.

The shutdown path drains `activeRequests` in the worker (`for range len(activeRequests) { <-activeRequests }`), but each in-flight goroutine also does `<-activeRequests` in its deferred cleanup. This can deadlock/leak goroutines: the worker may consume the tokens first (or block waiting for tokens that goroutines already consumed), and then the goroutines block forever on `<-activeRequests`. Use a `sync.WaitGroup` (as done in `session/inflictions.go`) or another mechanism where only one side is responsible for decrementing, and have `Stop()` wait on that with a timeout if desired.

pokebedrock/rank/service.go

For 5xx responses, fetchRoles returns an error that does not wrap ErrServer (fmt.Errorf("unexpected status code: %d", ...)). RolesError relies on errors.Is(err, ErrServer) to map server-side failures to a user-friendly message, so 5xx responses will now fall through to the generic default case. Consider wrapping ErrServer for >=500 responses as well (while still marking it retryable) so error classification remains consistent.

For 5xx responses, `fetchRoles` returns an error that does not wrap `ErrServer` (`fmt.Errorf("unexpected status code: %d", ...)`). `RolesError` relies on `errors.Is(err, ErrServer)` to map server-side failures to a user-friendly message, so 5xx responses will now fall through to the generic default case. Consider wrapping `ErrServer` for >=500 responses as well (while still marking it retryable) so error classification remains consistent.

pokebedrock/restart/service.go

stopOnce is declared at package scope, but it guards closing s.done, which is per-Service instance. If NewService is called more than once in-process (tests, reloads, multiple hubs), only the first instance will ever close its done channel; subsequent instances' cleanup goroutines will never be signalled to stop. Make the sync.Once a field on Service (or reuse s.closed/s.done with a non-global guard) so shutdown is correct per instance.

`stopOnce` is declared at package scope, but it guards closing `s.done`, which is per-Service instance. If `NewService` is called more than once in-process (tests, reloads, multiple hubs), only the first instance will ever close its `done` channel; subsequent instances' cleanup goroutines will never be signalled to stop. Make the `sync.Once` a field on `Service` (or reuse `s.closed`/`s.done` with a non-global guard) so shutdown is correct per instance.

pokebedrock/session/ranks.go

					
				@ -72,0 +123,4 @@

					if err != nil {

						update.ranks.SetRanks([]rank.Rank{rank.UnLinked})

						msg := text.Colourf("<red>%s</red>", rank.RolesError(err))

StopRankChannel closes rankShutdown and then waits for the workers to exit, but closing rankShutdown makes workers return immediately without draining rankUpdateCh (the select will prefer the shutdown case once it's closed). If the intent is to drain pending rank updates before exit, consider instead closing rankUpdateCh (to stop new work) and having workers range until the channel is drained, or update the comment/behaviour so callers don't assume queued updates will be processed during shutdown.

`StopRankChannel` closes `rankShutdown` and then waits for the workers to exit, but closing `rankShutdown` makes workers return immediately without draining `rankUpdateCh` (the select will prefer the shutdown case once it's closed). If the intent is to drain pending rank updates before exit, consider instead closing `rankUpdateCh` (to stop new work) and having workers range until the channel is drained, or update the comment/behaviour so callers don't assume queued updates will be processed during shutdown.

pokebedrock/vpn/service.go

handleResponse closes the response body without draining it. In the 429/default cases you don't read from the body at all, which prevents HTTP connection reuse in Go's transport and can increase connection churn under load. Consider draining to io.Discard before closing (similar to rank/service.go's fetchRoles and moderation/service.go's closeBody) to keep keep-alives effective.

`handleResponse` closes the response body without draining it. In the 429/default cases you don't read from the body at all, which prevents HTTP connection reuse in Go's transport and can increase connection churn under load. Consider draining to `io.Discard` before closing (similar to `rank/service.go`'s `fetchRoles` and `moderation/service.go`'s `closeBody`) to keep keep-alives effective.

coderabbitai[bot] (Migrated from github.com) reviewed

2026-05-08 19:15:06 +00:00

coderabbitai[bot] (Migrated from github.com) left a comment

Actionable comments posted: 2

🧹 Nitpick comments (5)

pokebedrock/queue/manager.go (1)

215-261: ⚡ Quick win

Avoid the _ = p discard and re-fetch round-trip for invalid entries.

In the snapshot loop you've already cast ent to p := ent.(*player.Player) (line 216). On the s == nil path you discard it with _ = p (line 223), then later at lines 253–261 you re-resolve entry.handle.Entity(tx) and re-cast just to call Message. Since the same *world.Tx is in scope, the original p reference is still valid — capture it instead of round-tripping through the handle a second time.

♻️ Proposed simplification

 	var (
 		toRemove        []*Entry
 		toTransfer      *Transfer
-		invalidEntries  []*Entry
-		invalidMessages []string
+		invalidPlayers  []*player.Player
+		invalidMessages []string
 	)

 	for _, entry := range queueSnap {
@@
 		s := entry.srv
 		if s == nil {
-			invalidEntries = append(invalidEntries, entry)
+			invalidPlayers = append(invalidPlayers, p)
 			invalidMessages = append(invalidMessages, "queue.destination.invalid")
 			toRemove = append(toRemove, entry)
-			_ = p

 			continue
 		}
@@
-	for i, entry := range invalidEntries {
-		ent, ok := entry.handle.Entity(tx)
-		if !ok {
-			continue
-		}
-		if p, ok := ent.(*player.Player); ok {
-			p.Message(locale.Translate(invalidMessages[i]))
-		}
-	}
+	for i, p := range invalidPlayers {
+		p.Message(locale.Translate(invalidMessages[i]))
+	}

🤖 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 `@pokebedrock/queue/manager.go` around lines 215 - 261, The code currently
discards the already-cast player (p := ent.(*player.Player)) on the s == nil
path and later re-resolves entry.handle.Entity(tx) to message the player;
instead, keep the player reference when marking an entry invalid (e.g., add p to
a new invalidPlayers slice or change invalidEntries to carry the
*player.Player), remove the `_ = p` discard, and then use that saved
*player.Player to call Message(locale.Translate(...)) instead of re-calling
entry.handle.Entity(tx); adjust the invalidEntries processing loop to use the
saved player pointer and keep calls to m.removeEntryLocked and Transfer creation
unchanged.

pokebedrock/moderation/service.go (2)

269-310: ⚡ Quick win

Asymmetric body-close contract between the two decoders is a maintenance footgun.

decodeInflictionsResponse closes the body internally via defer closeBody(resp) (line 281), but decodeNoContentResponse leaves it to the caller, requiring an explicit closeBody(resp) after each call (lines 169-170, 210-211). Future callers (or new decoder helpers patterned after these) are likely to forget — the lifetime contract is invisible at the call site. Make both helpers own the body close, then drop the trailing closeBody(resp) calls.
♻️ Proposed refactor
 // decodeNoContentResponse asserts the response has 204 No Content,
 // returning a descriptive error otherwise.
 func decodeNoContentResponse(resp *http.Response, what string) error {
+	defer closeBody(resp)
+
 	if resp.StatusCode == http.StatusNoContent {
 		return nil
 	}

 	body, _ := io.ReadAll(resp.Body)

 	return fmt.Errorf("failed to %s: %s", what, string(body))
 }
Then drop the explicit closeBody(resp) at lines 170 and 211.
🤖 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 `@pokebedrock/moderation/service.go` around lines 269 - 310, The two response
decoders must own closing the response body to avoid a hidden caller contract:
add a deferred closeBody(resp) at the start of decodeNoContentResponse
(mirroring decodeInflictionsResponse), remove any explicit closeBody(resp) calls
at the decodeNoContentResponse call sites (the trailing closeBody calls
currently after calls that expect the caller to close), and keep
decodeInflictionsResponse as-is; update only decodeNoContentResponse and delete
the redundant closeBody invocations in callers so both decoders consistently
close the body themselves.
414-425: ⚡ Quick win

Stop() always sleeps 3 seconds; the doc says "up to" — replace with an actual drain signal.

<-time.After(3 * time.Second) has no early-exit path, so the comment ("blocks for up to 3 seconds while in-flight requests drain") doesn't reflect behavior — every shutdown pays the full 3s, regardless of whether the player-details worker has actually finished. A sync.WaitGroup or a "worker exited" channel (analogous to cache.stopped in vpn/cache.go) would let Stop() exit immediately once dispatched goroutines complete, with a 3s cap as the upper bound.
♻️ Sketch of the change
+// detailsWorkerDone is closed by playerDetailsWorker when it has drained
+// in-flight requests and exited.
+var detailsWorkerDone = make(chan struct{})
+
 func playerDetailsWorker() {
+	defer close(detailsWorkerDone)
 	semaphore := make(chan struct{}, maxConcurrentRequests)
 	...
 }
@@
 func (s *Service) Stop() {
 	s.log.Debug("Stopping moderation service and workers...")
 	s.closed.Store(true)

 	detailsWorkerShutdownOnce.Do(func() {
 		close(detailsWorkerShutdown)
 	})

-	<-time.After(3 * time.Second)
+	select {
+	case <-detailsWorkerDone:
+	case <-time.After(3 * time.Second):
+		s.log.Warn("moderation worker did not drain within 3s")
+	}
 }
🤖 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 `@pokebedrock/moderation/service.go` around lines 414 - 425, The Stop() method
sleeps unconditionally; change it to wait for the worker to actually finish with
a 3s timeout: add a worker-exited signal (e.g. detailsWorkerDone channel or a
sync.WaitGroup) that the player-details worker closes/signals when it exits,
then in Service.Stop() call detailsWorkerShutdownOnce.Do(func(){
close(detailsWorkerShutdown) }) and replace the unconditional <-time.After(3 *
time.Second) with a select that waits on either the worker-exited signal
(immediate return) or time.After(3 * time.Second) (upper bound); update the
player-details worker to send/close that signal when it terminates so Stop() can
exit early.

pokebedrock/restart/service.go (1)

345-356: ⚡ Quick win

Move stopOnce onto the Service struct alongside done.

done is per-instance (line 31) but stopOnce is package-level (line 347). If NewService is ever called more than once (tests, restart flows, future multi-instance use), the package-level Once is already consumed by the prior Stop(), so the new instance's done channel will never be closed and its cleanup goroutine will leak. Even today, the encapsulation split is awkward — both fields describe the same shutdown lifecycle and should live together on Service.
♻️ Proposed refactor
 type Service struct {
 	log    *slog.Logger
 	config Config
 	closed atomic.Bool
 	done   chan struct{}
+	stopOnce sync.Once
 	mu     sync.RWMutex

 	state ServerState
 }
@@
-// stopOnce protects the done channel from being closed twice if Stop is
-// called more than once.
-var stopOnce sync.Once
-
 // Stop stops the restart manager service.
 func (s *Service) Stop() {
 	s.closed.Store(true)
-	stopOnce.Do(func() {
+	s.stopOnce.Do(func() {
 		close(s.done)
 	})
 	s.log.Debug("Restart Manager service stopped")
 }
🤖 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 `@pokebedrock/restart/service.go` around lines 345 - 356, The package-level
stopOnce should be moved onto the Service struct so each Service instance has
its own sync.Once to protect closing its done channel; remove the package var
stopOnce, add a stopOnce field to the Service struct (e.g. stopOnce sync.Once),
update Service.Stop() to call s.stopOnce.Do(...) instead of stopOnce.Do(...),
and ensure any constructors like NewService continue to create the per-instance
done channel without relying on a package-level Once so each Service can shut
down independently.

pokebedrock/session/ranks.go (1)

215-223: ⚡ Quick win

Clone the incoming slice before storing/sorting it.

SetRanks currently takes ownership of the caller's backing array and sorts it in place. Any caller that reuses or mutates that slice later can silently corrupt the session state.

Suggested fix

 func (r *Ranks) SetRanks(ranks []rank.Rank) {
 	r.rankMu.Lock()
 	defer r.rankMu.Unlock()
 
-	r.ranks = ranks
+	r.ranks = slices.Clone(ranks)
 	sort.SliceStable(r.ranks, func(i, j int) bool {
 		return r.ranks[i] < r.ranks[j]
 	})
 }

🤖 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 `@pokebedrock/session/ranks.go` around lines 215 - 223, SetRanks currently
stores and sorts the caller's slice in place which lets external mutation
corrupt session state; clone the incoming []rank.Rank before assigning and
sorting. Inside Ranks.SetRanks, create a new slice with the incoming length,
copy the elements from the parameter slice into the new slice, assign r.ranks =
the new slice and then run sort.SliceStable on r.ranks (still holding r.rankMu).
This ensures callers keep their backing array while the session owns an
independent copy.

🤖 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 `@pokebedrock/rank/service.go`:
- Around line 143-149: The 5xx branch currently returns retry=true but produces
a plain error that doesn't wrap ErrServer so RolesError can't detect it; update
the default case where resp.StatusCode >= internal.InternalServerError to return
the error wrapped with ErrServer (e.g., use fmt.Errorf with %w to wrap
ErrServer) while preserving the retry=true behavior so that after retries the
returned lastErr still matches ErrServer; keep the non-5xx default return
unchanged.

In `@pokebedrock/session/inflictions.go`:
- Around line 172-190: Reset the local infliction state before applying the full
resp.CurrentInflictions list: explicitly clear inflictions.muted and
inflictions.frozen (Store(false)) and reset inflictions.muteDuration (Store(0)
or the zero value you use to represent "no mute") prior to the for loop over
resp.CurrentInflictions so lifted inflictions don't persist; then keep the
existing logic that sets those stores when matching
moderation.InflictionMuted/InflictionFrozen and continue calling
handle.ExecWorld to apply side effects via inflictions.handleActiveInflictions.

---

Nitpick comments:
In `@pokebedrock/moderation/service.go`:
- Around line 269-310: The two response decoders must own closing the response
body to avoid a hidden caller contract: add a deferred closeBody(resp) at the
start of decodeNoContentResponse (mirroring decodeInflictionsResponse), remove
any explicit closeBody(resp) calls at the decodeNoContentResponse call sites
(the trailing closeBody calls currently after calls that expect the caller to
close), and keep decodeInflictionsResponse as-is; update only
decodeNoContentResponse and delete the redundant closeBody invocations in
callers so both decoders consistently close the body themselves.
- Around line 414-425: The Stop() method sleeps unconditionally; change it to
wait for the worker to actually finish with a 3s timeout: add a worker-exited
signal (e.g. detailsWorkerDone channel or a sync.WaitGroup) that the
player-details worker closes/signals when it exits, then in Service.Stop() call
detailsWorkerShutdownOnce.Do(func(){ close(detailsWorkerShutdown) }) and replace
the unconditional <-time.After(3 * time.Second) with a select that waits on
either the worker-exited signal (immediate return) or time.After(3 *
time.Second) (upper bound); update the player-details worker to send/close that
signal when it terminates so Stop() can exit early.

In `@pokebedrock/queue/manager.go`:
- Around line 215-261: The code currently discards the already-cast player (p :=
ent.(*player.Player)) on the s == nil path and later re-resolves
entry.handle.Entity(tx) to message the player; instead, keep the player
reference when marking an entry invalid (e.g., add p to a new invalidPlayers
slice or change invalidEntries to carry the *player.Player), remove the `_ = p`
discard, and then use that saved *player.Player to call
Message(locale.Translate(...)) instead of re-calling entry.handle.Entity(tx);
adjust the invalidEntries processing loop to use the saved player pointer and
keep calls to m.removeEntryLocked and Transfer creation unchanged.

In `@pokebedrock/restart/service.go`:
- Around line 345-356: The package-level stopOnce should be moved onto the
Service struct so each Service instance has its own sync.Once to protect closing
its done channel; remove the package var stopOnce, add a stopOnce field to the
Service struct (e.g. stopOnce sync.Once), update Service.Stop() to call
s.stopOnce.Do(...) instead of stopOnce.Do(...), and ensure any constructors like
NewService continue to create the per-instance done channel without relying on a
package-level Once so each Service can shut down independently.

In `@pokebedrock/session/ranks.go`:
- Around line 215-223: SetRanks currently stores and sorts the caller's slice in
place which lets external mutation corrupt session state; clone the incoming
[]rank.Rank before assigning and sorting. Inside Ranks.SetRanks, create a new
slice with the incoming length, copy the elements from the parameter slice into
the new slice, assign r.ranks = the new slice and then run sort.SliceStable on
r.ranks (still holding r.rankMu). This ensures callers keep their backing array
while the session owns an independent copy.

🪄 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: fb155b71-08b1-422a-a62b-c80b2b9509e2

📥 Commits

Reviewing files that changed from the base of the PR and between 4c3e26bd56 and 323ed9c319.

📒 Files selected for processing (12)

.cursor/rules/no-blocking-io-in-execworld.mdc
pokebedrock/command/kick.go
pokebedrock/form/moderate.go
pokebedrock/moderation/service.go
pokebedrock/pokebedrock.go
pokebedrock/queue/manager.go
pokebedrock/rank/service.go
pokebedrock/restart/service.go
pokebedrock/session/inflictions.go
pokebedrock/session/ranks.go
pokebedrock/vpn/cache.go
pokebedrock/vpn/service.go

**Actionable comments posted: 2** <details> <summary>🧹 Nitpick comments (5)</summary><blockquote> <details> <summary>pokebedrock/queue/manager.go (1)</summary><blockquote> `215-261`: _⚡ Quick win_ **Avoid the `_ = p` discard and re-fetch round-trip for invalid entries.** In the snapshot loop you've already cast `ent` to `p := ent.(*player.Player)` (line 216). On the `s == nil` path you discard it with `_ = p` (line 223), then later at lines 253–261 you re-resolve `entry.handle.Entity(tx)` and re-cast just to call `Message`. Since the same `*world.Tx` is in scope, the original `p` reference is still valid — capture it instead of round-tripping through the handle a second time. <details> <summary>♻️ Proposed simplification</summary> ```diff var ( toRemove []*Entry toTransfer *Transfer - invalidEntries []*Entry - invalidMessages []string + invalidPlayers []*player.Player + invalidMessages []string ) for _, entry := range queueSnap { @@ s := entry.srv if s == nil { - invalidEntries = append(invalidEntries, entry) + invalidPlayers = append(invalidPlayers, p) invalidMessages = append(invalidMessages, "queue.destination.invalid") toRemove = append(toRemove, entry) - _ = p continue } @@ - for i, entry := range invalidEntries { - ent, ok := entry.handle.Entity(tx) - if !ok { - continue - } - if p, ok := ent.(*player.Player); ok { - p.Message(locale.Translate(invalidMessages[i])) - } - } + for i, p := range invalidPlayers { + p.Message(locale.Translate(invalidMessages[i])) + } ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pokebedrock/queue/manager.go` around lines 215 - 261, The code currently discards the already-cast player (p := ent.(*player.Player)) on the s == nil path and later re-resolves entry.handle.Entity(tx) to message the player; instead, keep the player reference when marking an entry invalid (e.g., add p to a new invalidPlayers slice or change invalidEntries to carry the *player.Player), remove the `_ = p` discard, and then use that saved *player.Player to call Message(locale.Translate(...)) instead of re-calling entry.handle.Entity(tx); adjust the invalidEntries processing loop to use the saved player pointer and keep calls to m.removeEntryLocked and Transfer creation unchanged. ``` </details> </blockquote></details> <details> <summary>pokebedrock/moderation/service.go (2)</summary><blockquote> `269-310`: _⚡ Quick win_ **Asymmetric body-close contract between the two decoders is a maintenance footgun.** `decodeInflictionsResponse` closes the body internally via `defer closeBody(resp)` (line 281), but `decodeNoContentResponse` leaves it to the caller, requiring an explicit `closeBody(resp)` after each call (lines 169-170, 210-211). Future callers (or new decoder helpers patterned after these) are likely to forget — the lifetime contract is invisible at the call site. Make both helpers own the body close, then drop the trailing `closeBody(resp)` calls. <details> <summary>♻️ Proposed refactor</summary> ```diff // decodeNoContentResponse asserts the response has 204 No Content, // returning a descriptive error otherwise. func decodeNoContentResponse(resp *http.Response, what string) error { + defer closeBody(resp) + if resp.StatusCode == http.StatusNoContent { return nil } body, _ := io.ReadAll(resp.Body) return fmt.Errorf("failed to %s: %s", what, string(body)) } ``` Then drop the explicit `closeBody(resp)` at lines 170 and 211. </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pokebedrock/moderation/service.go` around lines 269 - 310, The two response decoders must own closing the response body to avoid a hidden caller contract: add a deferred closeBody(resp) at the start of decodeNoContentResponse (mirroring decodeInflictionsResponse), remove any explicit closeBody(resp) calls at the decodeNoContentResponse call sites (the trailing closeBody calls currently after calls that expect the caller to close), and keep decodeInflictionsResponse as-is; update only decodeNoContentResponse and delete the redundant closeBody invocations in callers so both decoders consistently close the body themselves. ``` </details> --- `414-425`: _⚡ Quick win_ **`Stop()` always sleeps 3 seconds; the doc says "up to" — replace with an actual drain signal.** `<-time.After(3 * time.Second)` has no early-exit path, so the comment ("blocks for up to 3 seconds while in-flight requests drain") doesn't reflect behavior — every shutdown pays the full 3s, regardless of whether the player-details worker has actually finished. A `sync.WaitGroup` or a "worker exited" channel (analogous to `cache.stopped` in `vpn/cache.go`) would let `Stop()` exit immediately once dispatched goroutines complete, with a 3s cap as the upper bound. <details> <summary>♻️ Sketch of the change</summary> ```diff +// detailsWorkerDone is closed by playerDetailsWorker when it has drained +// in-flight requests and exited. +var detailsWorkerDone = make(chan struct{}) + func playerDetailsWorker() { + defer close(detailsWorkerDone) semaphore := make(chan struct{}, maxConcurrentRequests) ... } @@ func (s *Service) Stop() { s.log.Debug("Stopping moderation service and workers...") s.closed.Store(true) detailsWorkerShutdownOnce.Do(func() { close(detailsWorkerShutdown) }) - <-time.After(3 * time.Second) + select { + case <-detailsWorkerDone: + case <-time.After(3 * time.Second): + s.log.Warn("moderation worker did not drain within 3s") + } } ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pokebedrock/moderation/service.go` around lines 414 - 425, The Stop() method sleeps unconditionally; change it to wait for the worker to actually finish with a 3s timeout: add a worker-exited signal (e.g. detailsWorkerDone channel or a sync.WaitGroup) that the player-details worker closes/signals when it exits, then in Service.Stop() call detailsWorkerShutdownOnce.Do(func(){ close(detailsWorkerShutdown) }) and replace the unconditional <-time.After(3 * time.Second) with a select that waits on either the worker-exited signal (immediate return) or time.After(3 * time.Second) (upper bound); update the player-details worker to send/close that signal when it terminates so Stop() can exit early. ``` </details> </blockquote></details> <details> <summary>pokebedrock/restart/service.go (1)</summary><blockquote> `345-356`: _⚡ Quick win_ **Move `stopOnce` onto the `Service` struct alongside `done`.** `done` is per-instance (line 31) but `stopOnce` is package-level (line 347). If `NewService` is ever called more than once (tests, restart flows, future multi-instance use), the package-level `Once` is already consumed by the prior `Stop()`, so the new instance's `done` channel will never be closed and its cleanup goroutine will leak. Even today, the encapsulation split is awkward — both fields describe the same shutdown lifecycle and should live together on `Service`. <details> <summary>♻️ Proposed refactor</summary> ```diff type Service struct { log *slog.Logger config Config closed atomic.Bool done chan struct{} + stopOnce sync.Once mu sync.RWMutex state ServerState } @@ -// stopOnce protects the done channel from being closed twice if Stop is -// called more than once. -var stopOnce sync.Once - // Stop stops the restart manager service. func (s *Service) Stop() { s.closed.Store(true) - stopOnce.Do(func() { + s.stopOnce.Do(func() { close(s.done) }) s.log.Debug("Restart Manager service stopped") } ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pokebedrock/restart/service.go` around lines 345 - 356, The package-level stopOnce should be moved onto the Service struct so each Service instance has its own sync.Once to protect closing its done channel; remove the package var stopOnce, add a stopOnce field to the Service struct (e.g. stopOnce sync.Once), update Service.Stop() to call s.stopOnce.Do(...) instead of stopOnce.Do(...), and ensure any constructors like NewService continue to create the per-instance done channel without relying on a package-level Once so each Service can shut down independently. ``` </details> </blockquote></details> <details> <summary>pokebedrock/session/ranks.go (1)</summary><blockquote> `215-223`: _⚡ Quick win_ **Clone the incoming slice before storing/sorting it.** `SetRanks` currently takes ownership of the caller's backing array and sorts it in place. Any caller that reuses or mutates that slice later can silently corrupt the session state. <details> <summary>Suggested fix</summary> ```diff func (r *Ranks) SetRanks(ranks []rank.Rank) { r.rankMu.Lock() defer r.rankMu.Unlock() - r.ranks = ranks + r.ranks = slices.Clone(ranks) sort.SliceStable(r.ranks, func(i, j int) bool { return r.ranks[i] < r.ranks[j] }) } ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pokebedrock/session/ranks.go` around lines 215 - 223, SetRanks currently stores and sorts the caller's slice in place which lets external mutation corrupt session state; clone the incoming []rank.Rank before assigning and sorting. Inside Ranks.SetRanks, create a new slice with the incoming length, copy the elements from the parameter slice into the new slice, assign r.ranks = the new slice and then run sort.SliceStable on r.ranks (still holding r.rankMu). This ensures callers keep their backing array while the session owns an independent copy. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary> ``` 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 `@pokebedrock/rank/service.go`: - Around line 143-149: The 5xx branch currently returns retry=true but produces a plain error that doesn't wrap ErrServer so RolesError can't detect it; update the default case where resp.StatusCode >= internal.InternalServerError to return the error wrapped with ErrServer (e.g., use fmt.Errorf with %w to wrap ErrServer) while preserving the retry=true behavior so that after retries the returned lastErr still matches ErrServer; keep the non-5xx default return unchanged. In `@pokebedrock/session/inflictions.go`: - Around line 172-190: Reset the local infliction state before applying the full resp.CurrentInflictions list: explicitly clear inflictions.muted and inflictions.frozen (Store(false)) and reset inflictions.muteDuration (Store(0) or the zero value you use to represent "no mute") prior to the for loop over resp.CurrentInflictions so lifted inflictions don't persist; then keep the existing logic that sets those stores when matching moderation.InflictionMuted/InflictionFrozen and continue calling handle.ExecWorld to apply side effects via inflictions.handleActiveInflictions. --- Nitpick comments: In `@pokebedrock/moderation/service.go`: - Around line 269-310: The two response decoders must own closing the response body to avoid a hidden caller contract: add a deferred closeBody(resp) at the start of decodeNoContentResponse (mirroring decodeInflictionsResponse), remove any explicit closeBody(resp) calls at the decodeNoContentResponse call sites (the trailing closeBody calls currently after calls that expect the caller to close), and keep decodeInflictionsResponse as-is; update only decodeNoContentResponse and delete the redundant closeBody invocations in callers so both decoders consistently close the body themselves. - Around line 414-425: The Stop() method sleeps unconditionally; change it to wait for the worker to actually finish with a 3s timeout: add a worker-exited signal (e.g. detailsWorkerDone channel or a sync.WaitGroup) that the player-details worker closes/signals when it exits, then in Service.Stop() call detailsWorkerShutdownOnce.Do(func(){ close(detailsWorkerShutdown) }) and replace the unconditional <-time.After(3 * time.Second) with a select that waits on either the worker-exited signal (immediate return) or time.After(3 * time.Second) (upper bound); update the player-details worker to send/close that signal when it terminates so Stop() can exit early. In `@pokebedrock/queue/manager.go`: - Around line 215-261: The code currently discards the already-cast player (p := ent.(*player.Player)) on the s == nil path and later re-resolves entry.handle.Entity(tx) to message the player; instead, keep the player reference when marking an entry invalid (e.g., add p to a new invalidPlayers slice or change invalidEntries to carry the *player.Player), remove the `_ = p` discard, and then use that saved *player.Player to call Message(locale.Translate(...)) instead of re-calling entry.handle.Entity(tx); adjust the invalidEntries processing loop to use the saved player pointer and keep calls to m.removeEntryLocked and Transfer creation unchanged. In `@pokebedrock/restart/service.go`: - Around line 345-356: The package-level stopOnce should be moved onto the Service struct so each Service instance has its own sync.Once to protect closing its done channel; remove the package var stopOnce, add a stopOnce field to the Service struct (e.g. stopOnce sync.Once), update Service.Stop() to call s.stopOnce.Do(...) instead of stopOnce.Do(...), and ensure any constructors like NewService continue to create the per-instance done channel without relying on a package-level Once so each Service can shut down independently. In `@pokebedrock/session/ranks.go`: - Around line 215-223: SetRanks currently stores and sorts the caller's slice in place which lets external mutation corrupt session state; clone the incoming []rank.Rank before assigning and sorting. Inside Ranks.SetRanks, create a new slice with the incoming length, copy the elements from the parameter slice into the new slice, assign r.ranks = the new slice and then run sort.SliceStable on r.ranks (still holding r.rankMu). This ensures callers keep their backing array while the session owns an independent copy. ``` </details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ]  Push a commit to this branch (recommended) - [ ]  Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: defaults **Review profile**: CHILL **Plan**: Pro **Run ID**: `fb155b71-08b1-422a-a62b-c80b2b9509e2` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 4c3e26bd56beb536e58bbf4a5f96d11ced9fabd2 and 323ed9c319f9fe4b0e85c867b7e5ca1e9cdf79cb. </details> <details> <summary>📒 Files selected for processing (12)</summary> * `.cursor/rules/no-blocking-io-in-execworld.mdc` * `pokebedrock/command/kick.go` * `pokebedrock/form/moderate.go` * `pokebedrock/moderation/service.go` * `pokebedrock/pokebedrock.go` * `pokebedrock/queue/manager.go` * `pokebedrock/rank/service.go` * `pokebedrock/restart/service.go` * `pokebedrock/session/inflictions.go` * `pokebedrock/session/ranks.go` * `pokebedrock/vpn/cache.go` * `pokebedrock/vpn/service.go` </details> </details>

pokebedrock/rank/service.go

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

5xx errors lose the ErrServer sentinel, so RolesError mis-classifies them after retries are exhausted.

Line 145 returns retry=true with a plain fmt.Errorf("unexpected status code: %d", ...) that doesn't wrap ErrServer. When all retries are exhausted, that error becomes lastErr and bubbles up to RolesError, which only matches ErrServer for the localized error.server_error_fetching_roles message. 5xx exhaustion will instead hit the default branch ("Failed to fetch roles %s"), regressing the user-facing error message that the non-5xx default branch correctly preserves.

🔧 Proposed fix to wrap 5xx errors with ErrServer

 	default:
 		if resp.StatusCode >= internal.InternalServerError {
-			return nil, true, fmt.Errorf("unexpected status code: %d", resp.StatusCode)
+			return nil, true, fmt.Errorf("unexpected status code %d: %w", resp.StatusCode, ErrServer)
 		}

 		return nil, false, fmt.Errorf("server returned %d: %w", resp.StatusCode, ErrServer)
 	}

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

	default:
		if resp.StatusCode >= internal.InternalServerError {
			return nil, true, fmt.Errorf("unexpected status code %d: %w", resp.StatusCode, ErrServer)
		}

		return nil, false, fmt.Errorf("server returned %d: %w", resp.StatusCode, ErrServer)
	}

🤖 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 `@pokebedrock/rank/service.go` around lines 143 - 149, The 5xx branch currently
returns retry=true but produces a plain error that doesn't wrap ErrServer so
RolesError can't detect it; update the default case where resp.StatusCode >=
internal.InternalServerError to return the error wrapped with ErrServer (e.g.,
use fmt.Errorf with %w to wrap ErrServer) while preserving the retry=true
behavior so that after retries the returned lastErr still matches ErrServer;
keep the non-5xx default return unchanged.

_⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_ **5xx errors lose the `ErrServer` sentinel, so `RolesError` mis-classifies them after retries are exhausted.** Line 145 returns `retry=true` with a plain `fmt.Errorf("unexpected status code: %d", ...)` that doesn't wrap `ErrServer`. When all retries are exhausted, that error becomes `lastErr` and bubbles up to `RolesError`, which only matches `ErrServer` for the localized `error.server_error_fetching_roles` message. 5xx exhaustion will instead hit the default branch (`"Failed to fetch roles %s"`), regressing the user-facing error message that the non-5xx default branch correctly preserves. <details> <summary>🔧 Proposed fix to wrap 5xx errors with ErrServer</summary> ```diff default: if resp.StatusCode >= internal.InternalServerError { - return nil, true, fmt.Errorf("unexpected status code: %d", resp.StatusCode) + return nil, true, fmt.Errorf("unexpected status code %d: %w", resp.StatusCode, ErrServer) } return nil, false, fmt.Errorf("server returned %d: %w", resp.StatusCode, ErrServer) } ``` </details>  <details> <summary>📝 Committable suggestion</summary> > ‼️ **IMPORTANT** > Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements. ```suggestion default: if resp.StatusCode >= internal.InternalServerError { return nil, true, fmt.Errorf("unexpected status code %d: %w", resp.StatusCode, ErrServer) } return nil, false, fmt.Errorf("server returned %d: %w", resp.StatusCode, ErrServer) } ``` </details>  <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pokebedrock/rank/service.go` around lines 143 - 149, The 5xx branch currently returns retry=true but produces a plain error that doesn't wrap ErrServer so RolesError can't detect it; update the default case where resp.StatusCode >= internal.InternalServerError to return the error wrapped with ErrServer (e.g., use fmt.Errorf with %w to wrap ErrServer) while preserving the retry=true behavior so that after retries the returned lastErr still matches ErrServer; keep the non-5xx default return unchanged. ``` </details>

pokebedrock/session/inflictions.go

					
				@ -252,4 +279,3 @@

								}

								// Use the original Load method directly

								inf.Load(handle)

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Reset infliction state from the full response.

This path only ever sets muted, frozen, and muteDuration when an infliction exists. If the moderation API later returns no mute/freeze, the old local state survives, so lifted inflictions can remain applied locally.

Suggested fix

-	for _, inf := range resp.CurrentInflictions {
+	muted := false
+	frozen := false
+	var muteDuration int64
+
+	for _, inf := range resp.CurrentInflictions {
 		switch inf.Type {
 		case moderation.InflictionMuted:
+			muted = true
 			if inf.ExpiryDate != nil && *inf.ExpiryDate != 0 {
-				inflictions.muteDuration.Store(*inf.ExpiryDate)
+				muteDuration = *inf.ExpiryDate
 			}
-
-			inflictions.muted.Store(true)
 		case moderation.InflictionFrozen:
-			inflictions.frozen.Store(true)
+			frozen = true
 		}
 	}
+
+	inflictions.muteDuration.Store(muteDuration)
+	inflictions.muted.Store(muted)
+	inflictions.frozen.Store(frozen)

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

	muted := false
	frozen := false
	var muteDuration int64

	for _, inf := range resp.CurrentInflictions {
		switch inf.Type {
		case moderation.InflictionMuted:
			muted = true
			if inf.ExpiryDate != nil && *inf.ExpiryDate != 0 {
				muteDuration = *inf.ExpiryDate
			}
		case moderation.InflictionFrozen:
			frozen = true
		}
	}

	inflictions.muteDuration.Store(muteDuration)
	inflictions.muted.Store(muted)
	inflictions.frozen.Store(frozen)

	// Apply side effects (e.g. SetImmobile) back on the world goroutine.
	handle.ExecWorld(func(_ *world.Tx, e world.Entity) {
		if p, ok := e.(*player.Player); ok {
			inflictions.handleActiveInflictions(p)
		}
	})

🤖 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 `@pokebedrock/session/inflictions.go` around lines 172 - 190, Reset the local
infliction state before applying the full resp.CurrentInflictions list:
explicitly clear inflictions.muted and inflictions.frozen (Store(false)) and
reset inflictions.muteDuration (Store(0) or the zero value you use to represent
"no mute") prior to the for loop over resp.CurrentInflictions so lifted
inflictions don't persist; then keep the existing logic that sets those stores
when matching moderation.InflictionMuted/InflictionFrozen and continue calling
handle.ExecWorld to apply side effects via inflictions.handleActiveInflictions.

_⚠️ Potential issue_ | _🟠 Major_ | _⚡ Quick win_ **Reset infliction state from the full response.** This path only ever sets `muted`, `frozen`, and `muteDuration` when an infliction exists. If the moderation API later returns no mute/freeze, the old local state survives, so lifted inflictions can remain applied locally. <details> <summary>Suggested fix</summary> ```diff - for _, inf := range resp.CurrentInflictions { + muted := false + frozen := false + var muteDuration int64 + + for _, inf := range resp.CurrentInflictions { switch inf.Type { case moderation.InflictionMuted: + muted = true if inf.ExpiryDate != nil && *inf.ExpiryDate != 0 { - inflictions.muteDuration.Store(*inf.ExpiryDate) + muteDuration = *inf.ExpiryDate } - - inflictions.muted.Store(true) case moderation.InflictionFrozen: - inflictions.frozen.Store(true) + frozen = true } } + + inflictions.muteDuration.Store(muteDuration) + inflictions.muted.Store(muted) + inflictions.frozen.Store(frozen) ``` </details>  <details> <summary>📝 Committable suggestion</summary> > ‼️ **IMPORTANT** > Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements. ```suggestion muted := false frozen := false var muteDuration int64 for _, inf := range resp.CurrentInflictions { switch inf.Type { case moderation.InflictionMuted: muted = true if inf.ExpiryDate != nil && *inf.ExpiryDate != 0 { muteDuration = *inf.ExpiryDate } case moderation.InflictionFrozen: frozen = true } } inflictions.muteDuration.Store(muteDuration) inflictions.muted.Store(muted) inflictions.frozen.Store(frozen) // Apply side effects (e.g. SetImmobile) back on the world goroutine. handle.ExecWorld(func(_ *world.Tx, e world.Entity) { if p, ok := e.(*player.Player); ok { inflictions.handleActiveInflictions(p) } }) ``` </details>  <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pokebedrock/session/inflictions.go` around lines 172 - 190, Reset the local infliction state before applying the full resp.CurrentInflictions list: explicitly clear inflictions.muted and inflictions.frozen (Store(false)) and reset inflictions.muteDuration (Store(0) or the zero value you use to represent "no mute") prior to the for loop over resp.CurrentInflictions so lifted inflictions don't persist; then keep the existing logic that sets those stores when matching moderation.InflictionMuted/InflictionFrozen and continue calling handle.ExecWorld to apply side effects via inflictions.handleActiveInflictions. ``` </details>

coderabbitai[bot] (Migrated from github.com) reviewed

2026-05-12 10:35:15 +00:00

coderabbitai[bot] (Migrated from github.com) left a comment

Actionable comments posted: 1

🤖 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 `@pokebedrock/parkour/session.go`:
- Around line 117-127: The current window allows onDone and tick callbacks to
run after stopCountdown() closes because stop is only checked before scheduling
handle.ExecWorld; modify the anonymous callbacks passed to handle.ExecWorld (the
func(tx *world.Tx, e world.Entity) closures used when invoking onDone and when
scheduling tick) to re-check the stop channel inside the closure before calling
user callbacks: perform a non-blocking select on stop (case <-stop: return;
default:) at the top of each ExecWorld callback and only invoke onDone(tx, p) or
tick handlers if stop has not been closed. Ensure you apply the same guard in
both the onDone closure and the tick closure to eliminate the late-cancellation
race.

🪄 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: ab367934-0be7-4eb2-8f5b-61179b1b0e81

📥 Commits

Reviewing files that changed from the base of the PR and between 323ed9c319 and b4bc3199c5.

⛔ Files ignored due to path filters (1)

go.sum is excluded by !**/*.sum

📒 Files selected for processing (3)

go.mod
pokebedrock/parkour/manager.go
pokebedrock/parkour/session.go

**Actionable comments posted: 1** <details> <summary>🤖 Prompt for all review comments with AI agents</summary> ``` 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 `@pokebedrock/parkour/session.go`: - Around line 117-127: The current window allows onDone and tick callbacks to run after stopCountdown() closes because stop is only checked before scheduling handle.ExecWorld; modify the anonymous callbacks passed to handle.ExecWorld (the func(tx *world.Tx, e world.Entity) closures used when invoking onDone and when scheduling tick) to re-check the stop channel inside the closure before calling user callbacks: perform a non-blocking select on stop (case <-stop: return; default:) at the top of each ExecWorld callback and only invoke onDone(tx, p) or tick handlers if stop has not been closed. Ensure you apply the same guard in both the onDone closure and the tick closure to eliminate the late-cancellation race. ``` </details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ]  Push a commit to this branch (recommended) - [ ]  Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: defaults **Review profile**: CHILL **Plan**: Pro **Run ID**: `ab367934-0be7-4eb2-8f5b-61179b1b0e81` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 323ed9c319f9fe4b0e85c867b7e5ca1e9cdf79cb and b4bc3199c5000dafc3a612885ea12470e85c9a0d. </details> <details> <summary>⛔ Files ignored due to path filters (1)</summary> * `go.sum` is excluded by `!**/*.sum` </details> <details> <summary>📒 Files selected for processing (3)</summary> * `go.mod` * `pokebedrock/parkour/manager.go` * `pokebedrock/parkour/session.go` </details> </details>

pokebedrock/parkour/session.go

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Guard callbacks against late cancellation windows.

Line 117 checks stop only before scheduling onDone; if stopCountdown() closes right after that check, onDone can still run. The same window exists for tick scheduling. Re-check stop inside each ExecWorld callback before invoking user callbacks.

Suggested fix

 			if tick != nil {
 				remaining := i
 				handle.ExecWorld(func(_ *world.Tx, e world.Entity) {
+					select {
+					case <-stop:
+						return
+					default:
+					}
 					if p, ok := e.(*player.Player); ok {
 						tick(p, remaining)
 					}
 				})
 			}
@@
 		default:
 			if onDone != nil {
 				handle.ExecWorld(func(tx *world.Tx, e world.Entity) {
+					select {
+					case <-stop:
+						return
+					default:
+					}
 					if p, ok := e.(*player.Player); ok {
 						onDone(tx, p)
 					}
 				})
 			}
 		}

🤖 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 `@pokebedrock/parkour/session.go` around lines 117 - 127, The current window
allows onDone and tick callbacks to run after stopCountdown() closes because
stop is only checked before scheduling handle.ExecWorld; modify the anonymous
callbacks passed to handle.ExecWorld (the func(tx *world.Tx, e world.Entity)
closures used when invoking onDone and when scheduling tick) to re-check the
stop channel inside the closure before calling user callbacks: perform a
non-blocking select on stop (case <-stop: return; default:) at the top of each
ExecWorld callback and only invoke onDone(tx, p) or tick handlers if stop has
not been closed. Ensure you apply the same guard in both the onDone closure and
the tick closure to eliminate the late-cancellation race.

_⚠️ Potential issue_ | _🟠 Major_ | _⚡ Quick win_ **Guard callbacks against late cancellation windows.** Line 117 checks `stop` only before scheduling `onDone`; if `stopCountdown()` closes right after that check, `onDone` can still run. The same window exists for `tick` scheduling. Re-check `stop` inside each `ExecWorld` callback before invoking user callbacks. <details> <summary>Suggested fix</summary> ```diff if tick != nil { remaining := i handle.ExecWorld(func(_ *world.Tx, e world.Entity) { + select { + case <-stop: + return + default: + } if p, ok := e.(*player.Player); ok { tick(p, remaining) } }) } @@ default: if onDone != nil { handle.ExecWorld(func(tx *world.Tx, e world.Entity) { + select { + case <-stop: + return + default: + } if p, ok := e.(*player.Player); ok { onDone(tx, p) } }) } } ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pokebedrock/parkour/session.go` around lines 117 - 127, The current window allows onDone and tick callbacks to run after stopCountdown() closes because stop is only checked before scheduling handle.ExecWorld; modify the anonymous callbacks passed to handle.ExecWorld (the func(tx *world.Tx, e world.Entity) closures used when invoking onDone and when scheduling tick) to re-check the stop channel inside the closure before calling user callbacks: perform a non-blocking select on stop (case <-stop: return; default:) at the top of each ExecWorld callback and only invoke onDone(tx, p) or tick handlers if stop has not been closed. Ensure you apply the same guard in both the onDone closure and the tick closure to eliminate the late-cancellation race. ``` </details>

Sign in to join this conversation.

No reviewers

glancist