Guard the deferred local queue and depth counter - #11284
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRouter updates packet validation, XEdDSA policy verification, encryption-failure handling, and packet-pool cleanup. It also synchronizes nested local delivery and deferred queue draining across concurrent BLE and loop-task paths. ChangesRouter packet handling and deferred delivery
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant BLETask
participant LoopTask
participant Router
participant DeferredLocalQueue
BLETask->>Router: handleReceived(packet)
Router->>Router: Lock deferredLock and increment handleDepth
LoopTask->>Router: deliverLocal(packet)
Router->>Router: Lock deferredLock and inspect handleDepth
Router->>DeferredLocalQueue: Enqueue copied packet when nested
Router->>DeferredLocalQueue: Dequeue deferred packets while outermost
Router->>Router: Dispatch queued packets outside deferredLock
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
⚡ Try this PR in the Web Flasher
Warning This is an automated, unreviewed CI test build. Back up your device configuration Supported boards built by this PR (31)
Build artifacts expire on 2026-08-29. Updated for |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this 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 `@src/mesh/Router.cpp`:
- Around line 1251-1256: Update the deliverLocal final-drain logic around
nested, deferredLock, and handleDepth so the final empty-check and decrement to
zero occur under deferredLock, and only while the deferred ring remains empty.
After allocating the packet copy, re-check handleDepth and append the copy under
the same lock, making the enqueue decision atomic with the final drain check.
Keep dispatchReceived() outside deferredLock.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c2b715cf-b263-4f68-a6db-82466e6e1e89
📒 Files selected for processing (3)
src/mesh/MemoryPool.hsrc/mesh/Router.cppsrc/mesh/Router.h
2e87743 to
d4dc786
Compare
caveman99
changed the title
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/mesh/Router.cpp (1)
1251-1256: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftMake handle-depth admission and exit atomic.
Line 1254 releases the lock before dispatch admission, so two tasks can both observe depth zero and concurrently enter
dispatchReceived(). The prior final-drain race also remains: an enqueuer can observe nonzero depth, allocate a copy, then append it after Lines 1311-1313 drop depth to zero—stranding it until another receive.Reserve the outermost handler under
deferredLock; make the post-allocation enqueue decision atomic with the final empty-check/decrement. Keep dispatch outside the lock and avoid calling the locking queue helpers while already holding it.Also applies to: 1285-1314
🤖 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 `@src/mesh/Router.cpp` around lines 1251 - 1256, Make handler-depth admission and deferred-queue finalization atomic under deferredLock: reserve the outermost handler while holding the lock before dispatchReceived(), and combine post-allocation enqueue selection with the final empty-check/decrement so no item can be stranded. Keep dispatchReceived() outside the lock and avoid locking queue helpers while deferredLock is held; update the surrounding logic using nested, handleDepth, and the deferred queue operations.
🤖 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.
Duplicate comments:
In `@src/mesh/Router.cpp`:
- Around line 1251-1256: Make handler-depth admission and deferred-queue
finalization atomic under deferredLock: reserve the outermost handler while
holding the lock before dispatchReceived(), and combine post-allocation enqueue
selection with the final empty-check/decrement so no item can be stranded. Keep
dispatchReceived() outside the lock and avoid locking queue helpers while
deferredLock is held; update the surrounding logic using nested, handleDepth,
and the deferred queue operations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 98bcef02-f21a-4abf-82b7-0b18a5835597
📒 Files selected for processing (2)
src/mesh/Router.cppsrc/mesh/Router.h
🚧 Files skipped from review as they are similar to previous changes (1)
- src/mesh/Router.h
There was a problem hiding this 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 `@src/mesh/Router.cpp`:
- Around line 1275-1278: Update the fallback branch in handleReceived() so the
copied packet is routed back through handleReceived() rather than calling
dispatchReceived() directly. Preserve packetPool.release(copy) after the
delegated handling, ensuring loopback deliveries remain subject to the existing
depth-based deferral.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 26a075e8-7e75-40d6-8b69-32715c3f7012
📒 Files selected for processing (2)
src/mesh/Router.cppsrc/mesh/Router.h
🚧 Files skipped from review as they are similar to previous changes (1)
- src/mesh/Router.h
cd870fa to
8bd36fa
Compare
There was a problem hiding this 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 `@src/mesh/Router.cpp`:
- Around line 1302-1343: Update Router::handleReceived so the final handleDepth
decrement and determination of whether this frame is the last active frame occur
under the same deferredLock acquisition. If this frame becomes the drain owner,
immediately re-elevate handleDepth to keep it at least 1 throughout
deferred-ring draining, then run the existing drain loop; otherwise return after
the atomic decrement.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2be740ce-ed5b-438b-b656-974dfb0eab9c
📒 Files selected for processing (2)
src/mesh/Router.cppsrc/mesh/Router.h
There was a problem hiding this comment.
Pull request overview
This PR hardens Router’s local loopback deferral path against cross-task races (notably on nRF52 where the BLE task can drive router code concurrently with the main loop task). It introduces a Router-owned lock to make handleDepth and the deferred-local ring updates atomic, preventing lost decrements and double-drains/double-releases that could otherwise wedge loopback delivery or corrupt packet ownership.
Changes:
- Add
deferredLockto guardhandleDepthand the deferred local ring bookkeeping. - Use
concurrency::LockGuardindeliverLocal()andhandleReceived()to make depth updates and ring drain decisions atomic while keepingdispatchReceived()outside the lock. - Update
Router.hdocumentation aroundhandleReceived()ownership semantics.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/mesh/Router.h |
Adds a Router-owned lock for deferred-local coordination and updates API/ownership documentation. |
src/mesh/Router.cpp |
Wraps handleDepth and deferred-ring enqueue/dequeue/drain decisions in a lock to avoid race-induced wedges/double handling. |
handleDepthand the deferred local ring were plainuint8_tread-modify-written from the loop task and, on nRF52, the Bluefruit BLE task. A lost decrement wedges the counter above zero and silently drops all local loopback until reboot; a lost count update can dispatch and release a slot twice.Both are now guarded by a lock owned by Router, which is constructed at runtime (
router = new ReliableRouter()), so the semaphore is created after the scheduler is up, same as the existingcryptLock. No lock is held acrossdispatchReceived(), the deferred lock and the pool are never held at the same time, and no path acquires the lock twice.The static packet pool has the same class of race but is deliberately left out of this PR: those pools are file-scope statics, so adding a lock member would create four binary semaphores before
main(), andconcurrency::Lockaborts if creation fails. It would also break the non-blockingalloc(0)contract, sinceLock::lock()waitsportMAX_DELAY.Summary by CodeRabbit