Clear a central's partial write buffer when it unsubscribes - #1677
Clear a central's partial write buffer when it unsubscribes#1677krishrathi1 wants to merge 1 commit into
Conversation
BLEInboundWriteBuffer.buffersByCentralID accumulates chunked ATT writes keyed by centralID, evicted only on successful decode or on exceeding the oversized cap. A central that disconnects mid-write (out of range, backgrounded) never reaches either exit, so its entry -- and every subsequent one from any central that disconnects mid-transfer -- stays in the dictionary for the life of the process. Over a day of ordinary background operation with many transient peers this is a real, unbounded leak, and if the same physical peer later reconnects and starts a new write at offset 0, replaceSubrange only overwrites the front of the stale buffer -- leftover trailing bytes from the abandoned transfer stay appended, which can corrupt the new decode attempt. peripheralManager(_:central:didUnsubscribeFrom:) is CoreBluetooth's only peripheral-role signal that a central is gone -- it already retires that central from pendingNotifications and linkStateStore, just not from pendingWriteBuffers. Adds BLEInboundWriteBuffer.removeValue(forCentralID:) and calls it from didUnsubscribeFrom, alongside the existing per-central cleanup. No Xcode/Swift toolchain available here. BLEInboundWriteBuffer itself is a plain struct with no CoreBluetooth dependency, so removeValueClearsOnlyTheTargetedCentralsPartialWrite is a real, compilable-by-inspection regression test (mirrors the existing removeAllClearsPartialWrites test's pattern exactly): two centrals each mid-write, remove one, confirm its abandoned half is gone (completing it no longer decodes) while the other central's unrelated in-flight write is untouched. Confirmed via git apply -R / git apply that reverting just the source change leaves the test calling a method that does not exist -- a genuine fail-without-the-fix state for a new method, not something I could exercise as a runtime pass/fail without the compiler. The BLEService+LinkLayerPeripheralRole.swift call site itself has no existing test coverage (didUnsubscribeFrom needs a CBCentral, which has no public initializer), verified by reading only.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dcbe3c4b3f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // never reaches .decoded or .oversized in BLEInboundWriteBuffer.append, so | ||
| // its partial buffer would otherwise sit forever -- this is the only | ||
| // peripheral-role signal CoreBluetooth gives us that a central is gone. | ||
| pendingWriteBuffers.removeValue(forCentralID: centralID) |
There was a problem hiding this comment.
When a connected central disables notifications with setNotifyValue(false) and then continues a fragmented write, this callback fires even though the central has not disconnected, so clearing its buffer here discards the earlier chunks and prevents the completed frame from decoding. This flow is supported by processDecodedCentralWrite, which explicitly treats a writer as live whether or not it subscribed; use inactivity-based eviction rather than treating every notification unsubscribe as a definitive link end.
Useful? React with 👍 / 👎.
Chessing234
left a comment
Chessing234
left a comment
There was a problem hiding this comment.
heads up on a collision: #1666 touches the same three files as this — BLEInboundWriteBuffer.swift, BLEService+LinkLayerPeripheralRole.swift and BLEInboundWriteBufferTests.swift — and you both add your new cases at the same spot in the test file. whichever lands second will conflict.
might be worth one of you rebasing onto the other, or splitting so the buffer struct only gets touched once. they're solving different problems (offset validation vs eviction on unsubscribe) so both look wanted, just not independently.
2 participants
The bug
BLEInboundWriteBuffer.buffersByCentralID(bitchat/Services/BLE/BLEInboundWriteBuffer.swift) accumulates chunked ATT writes keyed bycentralID, evicted only on a successful decode (.decoded) or on exceeding the oversized cap (.oversized). A central that disconnects mid-write -- walked out of range, backgrounded, anything short of finishing the transfer -- lands in.waitingand never reaches either exit. Its entry just stays.peripheralManager(_:central:didUnsubscribeFrom:)(BLEService+LinkLayerPeripheralRole.swift) is CoreBluetooth's only peripheral-role signal that a central is gone, and already retires that central frompendingNotificationsandlinkStateStorethere -- just not frompendingWriteBuffers.Impact
Dataentry for the life of the process. Over a day of ordinary background operation with many transient peers (a train, a conference, a crowded room) this adds up for real.CBCentral.identifieris stable per (device, this app). If the same physical peer reconnects and starts a new, shorter write at offset 0,append'scombined.replaceSubrangeonly overwrites the front of the stale buffer -- leftover trailing bytes from the abandoned transfer stay appended past the new write's end, which can corrupt the new decode attempt in cases where the lengths don't happen to align.The fix
Adds
BLEInboundWriteBuffer.removeValue(forCentralID:)and calls it fromdidUnsubscribeFrom, alongside the cleanup that already happens there for the same central.Test
removeValueClearsOnlyTheTargetedCentralsPartialWritemirrors the existingremoveAllClearsPartialWritestest's pattern: two centrals each mid-write (first half of a frame sent, second half pending), remove one by ID, confirm its abandoned half is actually gone (completing it with only the second half no longer decodes -- there's nothing to decode against) while the other central's unrelated in-flight write is untouched and completes normally.Verification
No Xcode/Swift toolchain available here.
BLEInboundWriteBufferis a plain struct with no CoreBluetooth dependency, so this test is real and compilable-by-inspection (matches the file's existing test style exactly). Confirmed viagit apply -R(revert only the source change) that the test then calls a method that doesn't exist -- a genuine fail-without-the-fix state for a new method, since there's no meaningful "runs but returns the wrong thing" state to demonstrate without a compiler for something this small. Reapplied withgit applyto restore the fix.The
didUnsubscribeFromcall site itself has no existing or new test coverage -- it needs a realCBCentral, which has no public initializer -- verified by reading only, same limitation noted on #1676 (a separate BLE fix open right now, same toolchain wall).