Concurrent chunck generation#21
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR introduces asynchronous chunk loading for the Dragonfly game server. Chunk acquisition is refactored from synchronous blocking calls to request-driven async operations with callback completion. The system tracks in-flight loads per chunk position, associates operations with transaction context, and integrates completed chunks back into world state through structured callbacks. ChangesAsynchronous Chunk Loading
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 4 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 8b4d0de. Configure here.
| l.w.addViewer(tx, c, l) | ||
|
|
||
| l.loaded[pos] = c | ||
| } |
There was a problem hiding this comment.
Async callback writes to map without mutex
High Severity
viewChunk writes to l.loaded (a map) without holding l.mu, but Chunk() reads l.loaded under l.mu.RLock(). Previously, the equivalent write happened inside Load which held l.mu.Lock(). Now, when viewChunk is called as an async callback from signal in a later transaction, l.mu is not held. In Go, concurrent map read and write causes a fatal runtime panic. Since Chunk() is a public method with explicit locking for concurrent access, any concurrent call to Chunk() while viewChunk is executing will crash the program.
Reviewed by Cursor Bugbot for commit 8b4d0de. Configure here.
There was a problem hiding this comment.
the finding is still valid pls ask ai
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/world/world.go (1)
1197-1207:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftDon't resolve async chunk requests through
w.currentTx.
World.chunk()is still reachable from non-transaction APIs likeHighestLightBlocker(), but these paths now consultchunkRequeststhroughw.currentTx. Between transactions that field is nil or points at the last closed tx, soreq.doImmediate(tx)can panic once it reachestx.World(). Pass the active*Txexplicitly from transaction-only callers, and keep non-transaction callers on a path that doesn't depend on implicit world-global transaction state.Also applies to: 1227-1240, 1255-1262
🤖 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 `@server/world/world.go` around lines 1197 - 1207, World.chunk() is using the global w.currentTx to resolve async chunk requests which can be nil or stale and causes req.doImmediate(tx) to panic; modify the API so transaction-only callers pass the active *Tx explicitly and non-transaction callers stay on a path that doesn't consult transaction-scoped chunkRequests: change World.chunk(pos ChunkPos) (and the internal call to chunkFromAsyncPool) to accept an explicit tx *Tx (e.g. World.chunkWithTx(tx *Tx, pos ChunkPos)) or add a separate World.chunkNoTx for non-transactional callers, update transaction-only call sites to pass their active tx, and update chunkFromAsyncPool to use the supplied tx instead of w.currentTx (apply same pattern to the other affected blocks around the mentioned ranges 1227-1240 and 1255-1262).
🤖 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 `@server/world/generator.go`:
- Around line 57-61: chunkRequest.load currently always calls w.Exec(r.signal)
after completing, which can enqueue signals onto a stopped world and block; fix
by either tracking these loader goroutines in World shutdown or rejecting them
during shutdown. Concretely: add a sync.WaitGroup field to World, call
world.wg.Add(1) when spawning the chunk request goroutine and defer
world.wg.Done() at the start of chunkRequest.load; then update World.Close to
wait on world.wg (wg.Wait()) before or as part of stopping handleTransactions so
late completions are drained. Alternatively (or additionally), guard the enqueue
by checking a shutdown flag or world.stop channel in chunkRequest.load and skip
calling w.Exec(r.signal) if the world is shutting down.
In `@server/world/loader.go`:
- Around line 103-108: The callback passed to loadChunkAsync is invoked inline
when pos is already loaded, causing a deadlock because Load() still holds l.mu
and the callback calls l.World() and later mutates l.w, l.viewer, and l.loaded
without holding l.mu; fix by capturing any necessary state under l.mu (e.g.,
determine whether to proceed and capture pos/world id) then release l.mu before
calling w.loadChunkAsync, and inside the callback re-acquire l.mu, re-check that
l.World() == captured world id and that the loader is not closed/changed, and
only then call l.viewChunk (or mutate l.w/l.viewer/l.loaded) while holding l.mu;
apply the same pattern to the other block around lines 117-125 so callbacks
never run with l.mu held and mutations always occur after a re-validation under
the lock.
In `@server/world/world.go`:
- Around line 1211-1223: The loadChunk logic in World.loadChunk is turning
non-ErrNotFound provider read failures into fresh empty columns, risking
permanent data loss when those synthetic chunks are later modified/saved;
instead stop fabricating data on hard read errors by changing World.loadChunk
(and its callers in the sync/async load path) to return an error for
non-NotFound cases rather than a synthetic *chunk.Column, so Provider.LoadColumn
errors are bubbled up and handled by the existing load path (and not passed into
addChunk/save). Update code paths that call loadChunk to handle the error, treat
ErrNotFound by creating a new column as before, and ensure addChunk is only
called for legitimately generated or retrieved chunks.
---
Outside diff comments:
In `@server/world/world.go`:
- Around line 1197-1207: World.chunk() is using the global w.currentTx to
resolve async chunk requests which can be nil or stale and causes
req.doImmediate(tx) to panic; modify the API so transaction-only callers pass
the active *Tx explicitly and non-transaction callers stay on a path that
doesn't consult transaction-scoped chunkRequests: change World.chunk(pos
ChunkPos) (and the internal call to chunkFromAsyncPool) to accept an explicit tx
*Tx (e.g. World.chunkWithTx(tx *Tx, pos ChunkPos)) or add a separate
World.chunkNoTx for non-transactional callers, update transaction-only call
sites to pass their active tx, and update chunkFromAsyncPool to use the supplied
tx instead of w.currentTx (apply same pattern to the other affected blocks
around the mentioned ranges 1227-1240 and 1255-1262).
🪄 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: 02eba2a3-1c90-42c6-b8cc-157ec2128834
📒 Files selected for processing (5)
server/world/conf.goserver/world/generator.goserver/world/loader.goserver/world/tx.goserver/world/world.go
2 participants
Note
Medium Risk
Introduces concurrent chunk load/generation and a shared request pool, which changes core world/chunk lifecycle and adds goroutine/transaction coordination that could surface races or ordering issues under load.
Overview
Enables concurrent chunk loading/generation by introducing a per-position
chunkRequestpool that coalesces multiple requests, runs provider/generator work off-thread, then re-enters the world transaction loop to add the chunk and notify waiters.Updates chunk access paths to use this async pool:
Loader.Load()now requests chunks vialoadChunkAsync,World.chunk()can block on an in-flight request, and chunk insertion/light calculation is centralized inaddChunk. Also addsWorld.currentTxtracking and adjusts some reads (e.g.light()) to avoid implicitly loading chunks.Reviewed by Cursor Bugbot for commit 8b4d0de. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
Release Notes
Bug Fixes
Refactor