perf(ingest): bound memory under sustained packet load — actor recycling, bounded event stream, DM badge throttle#2106
Open
garthvh wants to merge 3 commits into
Open
perf(ingest): bound memory under sustained packet load — actor recycling, bounded event stream, DM badge throttle#2106garthvh wants to merge 3 commits into
garthvh wants to merge 3 commits into
Conversation
…e DM unread recount
The ingest actor's ModelContext registers every entity it inserts or
faults and never releases them. recreateShared() exists for exactly
this ('call ... periodically to release accumulated memory') but was
only invoked at connect time, so a session that stays connected under
sustained traffic accumulates registered objects without bound — a
long TCP stress-replay run reached millions of live model objects and
multi-gigabyte RSS, with the SwiftData observation machinery alone
holding hundreds of megabytes.
didReceive now flushes pending debounced saves and recycles the actor
every 20,000 packets while subscribed — between packets, where no
in-flight handler holds the retiring instance, and never during the
node-DB retrieval phase. On a busy mesh that's every few minutes; on a
quiet mesh it may never fire, which is fine because accumulation is
proportional to packets processed.
Also rate-limits the per-DM unreadMessages recount to ~1/sec, matching
the throttle the channel-badge path already has: the recount is
O(unread), so under a DM burst the per-message recompute turned the
badge update into quadratic work.
…-connect hook The transport event streams used AsyncStream's default unbounded buffering, so a producer faster than the main-actor consumer (a TCP radio or replay can sustain >100 packets/s) queued every unprocessed frame indefinitely: under a sustained stress replay the backlog grew to effectively every packet of the session — hundreds of thousands of live protobufs — while the app fell minutes behind real time. All three transports now use .bufferingNewest(4096): under sustained overload the OLDEST frames drop first (stale mesh traffic, exactly what a saturated real radio sheds), a node-DB dump fits well inside the bound, and error/teardown events are always the newest when they occur. The TCP reader counts and logs drops so backpressure is visible instead of silent. Also drops the ingest-recycle interval to 5,000 packets — at the consumer's real throughput 20,000 meant many minutes between recycles, letting hundreds of MB of registered objects pile up between them. Adds a DEBUG-only launch argument for automated stress testing: -meshtastic-connect-tcp <host[:port]> connects straight to a TCP radio at startup, so perf runs are fully scriptable (simctl launch + replay server) with no UI automation.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI 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)
📝 WalkthroughWalkthroughThe changes bound BLE, serial, and TCP event streams, add TCP backpressure logging, periodically recycle packet ingestion state, throttle direct unread recomputation, and add a DEBUG-only TCP connection launch path. ChangesPacket ingest and transport backpressure
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant LaunchArguments
participant MeshtasticAppleApp
participant AccessoryManager
participant TCPConnection
LaunchArguments->>MeshtasticAppleApp: provide -meshtastic-connect-tcp host[:port]
MeshtasticAppleApp->>AccessoryManager: resolve TCP device
MeshtasticAppleApp->>AccessoryManager: connect(to:) after startup delay
AccessoryManager->>TCPConnection: establish packet stream
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 `@Meshtastic/Accessory/Transports/TCP/TCPConnection.swift`:
- Around line 212-219: Update yieldDataEvent in yieldDataEvent(_:) to inspect
the continuation.yield result as .enqueued(let remaining), treating remaining ==
0 as the backpressure/drop signal; retain the existing counter increment and
periodic warning behavior within that condition, and remove the ineffective
.dropped handling.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: cc53971c-e03c-4666-9fa5-e812f89e4d5a
📒 Files selected for processing (7)
Meshtastic/Accessory/Accessory Manager/AccessoryManager+Connect.swiftMeshtastic/Accessory/Accessory Manager/AccessoryManager.swiftMeshtastic/Accessory/Transports/Bluetooth Low Energy/BLEConnection.swiftMeshtastic/Accessory/Transports/Serial/SerialConnection.swiftMeshtastic/Accessory/Transports/TCP/TCPConnection.swiftMeshtastic/Helpers/MeshPackets.swiftMeshtastic/MeshtasticApp.swift
…ropped With .bufferingNewest the new element is always enqueued and the OLDEST buffered frame is evicted, so yield never returns .dropped and the backpressure warning was dead code. Count yields that report zero remaining capacity instead — each one evicts an oldest frame (to within the one yield that exactly fills the buffer). Verified live: the warning now fires under a saturating replay.
📄 Docs staleness warningThis PR modifies user-facing Swift source files but does not update any page under Changed source files: What to check:
If this PR does not require a doc update (e.g., internal refactor, bug fix, test change), add the After updating |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #2105 — together they're the two halves of the stress-replay findings: #2105 fixed the main-thread view storms; this bounds the ingestion side's memory.
The problem
Two unbounded accumulators, found by heap analysis of a long session against a 1,600-node DEF-CON-shaped TCP replay at ~140 packets/second:
MeshPackets.recreateShared()exists for exactly this — its doc comment says to call it periodically — but it was only invoked at connect time, so a session that stays connected accumulates without bound. A long run reached over a million live model objects, with the SwiftData observation machinery alone holding hundreds of MB.AsyncStream's default unbounded policy let the TCP reader queue every frame the main-actor consumer hadn't gotten to. Under sustained overload the backlog grew to essentially the whole session's packets — hundreds of thousands of live protobufs — while the app processed traffic minutes old.Plus a small quadratic: the per-DM unread-badge recount is O(unread) and ran for every incoming DM (the channel path was already throttled).
The fixes
didReceiveflushes pending debounced saves and recycles the MeshPackets actor every 5,000 packets while subscribed — between packets (no in-flight handler holds the retiring instance) and never during node-DB retrieval. Proportional to traffic: a quiet mesh may never recycle, which is fine..bufferingNewest(4096). Under sustained overload the oldest frames drop first — stale mesh traffic, what a saturated real radio would shed. A node-DB dump fits well within the bound; error/teardown events are the newest when they occur so they can't be displaced. The TCP reader logs drops so backpressure is visible.-meshtastic-connect-tcp <host[:port]>connects straight to a TCP radio at startup, making stress runs fully scriptable (simctl launch+ replay server) with no UI automation. Same pattern as the existing--marketing-capture/ perf-seed hooks.Verification (same replay, before → after)
Known remainder (future work)
The main context still grows slowly under flood (message-view fetches, notification donations) and the consumer saturates around half the replay's rate, so a sustained >100pps source sheds oldest traffic by design. Both are follow-up material; neither affects real-mesh rates.
Summary by CodeRabbit
Performance
Reliability