Repository navigation
Conversation
|
Updated for review. The PR description now answers each of your points under its own heading, so I will only summarise here. On the integration test: I tried to make one that fails without the fix and could not. bluetoothd only forwards a client's stop to the kernel while its own The code comment now names the bluetoothd path as well as the bug. On your question about the next scan: yes, it is affected, and the description spells out how; that part is bluetoothd's and I will send a patch to linux-bluetooth after this. Marking ready for review. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #2022 +/- ##
===========================================
- Coverage 65.34% 65.33% -0.01%
===========================================
Files 53 53
Lines 5026 5048 +22
Branches 608 610 +2
===========================================
+ Hits 3284 3298 +14
- Misses 1519 1522 +3
- Partials 223 228 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
Hmm... then ignoring this error might will just lead to a silent scan not working problem later, correct? I'm tempted to say, let's keep the error just to let people know bluetoothd is broken and probably needs to be restarted. |
|
Good question, and the answer is that there are two different situations here that produce the same
The point I would make is that raising the error does not help with case 2. bleak sees the identical If a caller does want to detect the stuck state, the reliable signal is not a single |
Does this mean that any scan that a user requests in Bleak longer than this gets cut short and the rest of the time it is not actually scanning?
Can we look at the
This sounds like the best we can do if the "Discovering" property can't be used. So info is probably good enough. And we can add something to the docs about the known bug in BlueZ. If the "Discovering" property is false at this point though, we could probably demote it from a warning to a debug since that likely means that the next scan will work. Would still want it to be warning in other case though. |
…overy Prod hci7, 2026-09-04 18:00:42Z to 09-05 15:12Z: bluetoothd believed discovery was enabled while the kernel was stopped. Every window was accepted at StartDiscovery, heard nothing, and was rejected at stop; the controller received zero bytes across nine measured windows; Discovering read true with zero clients. No trigger saw it for 29 hours - the strike counter climbed to ~99 with nothing reading it, because e8322ff (rightly) took recovery off the InProgress path after 08-26 charged healthy cards for overlapping discoveries. Sourced in BlueZ 5.72 adapter.c: adapter->discovering is set true only by client discovery (1873, 1925; passive scanning sets discovery_enable and never discovering); stop_discovery_complete returns on non-success without clearing it, and every later start takes update_discovery_filter's "queue up a stop" branch whose completion is discarded, so no MGMT start is ever sent again and the property stays true until the adapter object is reset. On the healthy MGMT-stop path the reply is sent BEFORE the property change is emitted (2153 vs 2165), so a reader needs a grace period. Silent peers cannot produce this: after the kernel's own 10.24s LE timeout, discovery_stop answers the last client from its idle phase without touching the kernel (192 clean stops on hci7 with the same peers off, then 99 rejected ones). So the signal is the PROPERTY, not the exception. After our stop returns - InProgress raised or swallowed, either way - if the session delivered no advertisement and Discovering stays true through a 1.5s settle window, that is the desync: one strike toward recovery under the existing gates (three over a minute, daemon check, empty card, probation, kill switch). Read from bleak's BlueZManager property cache (it mirrors every PropertiesChanged under /org/bluez, Adapter1 included), no bus round trip; None when there is no manager on the loop, never a guess. A bare InProgress with Discovering cleared afterwards stays a ranking hint: that is the one-off #2021 race. A session that heard an advertisement is not charged whatever the property says: it WAS scanning; the next quiet one carries it. Independent of hbldh/bleak#2022, which swallows the exception (bleak-fix, 2026-09-06); BCM needs nothing from bleak for this. Negative controls: with the check disabled the stuck and the InProgress-plus-stuck tests fail; with the heard-nothing lock removed the heard-something test fails. 348 tests. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Not cut short, but not continuous either. When the kernel's The kernel timeout is per adapter, not per client. All clients share one kernel discovery, so it ends for all of them at once and bluetoothd restarts it for all of them together as long as any remain.
I checked the path in What does separate them is what happens on the next stop, and that gives us a clean test that needs no property read. In the one-off case the kernel's In the stuck case So two
That gives you the split you asked for, INFO for the harmless case and a raised error for the broken one, and keeps your original point that a broken bluetoothd should not be hidden. Two limits, both stated in the new troubleshooting section: the memory is per process, so a process that scans once never escalates; and if another scanner is active on the same adapter, bluetoothd answers our stop without asking the kernel, which counts as a success, so the detection only works while we are the last client. The tests cover the first rejection, the second consecutive one raising with the warning, and a clean stop between two rejections resetting the count. The one empty scan in the harmless case is bluetoothd's bug and stays. Clearing |
dlech
left a comment
dlech
left a comment
There was a problem hiding this comment.
This seems like the best we can do given what we know about the bug.
| # then relays MGMT_STATUS_REJECTED as InProgress. | ||
| # See https://github.com/hbldh/bleak/issues/2021 | ||
| # and https://github.com/bluez/bluez/issues/807. | ||
| if ex.dbus_error not in ( |
There was a problem hiding this comment.
I would change this to a match statement.
Indent is getting pretty deep here too, so maybe put everything in the except in a function to help with readability?
| Fixed | ||
| ----- | ||
| * Fixed handling empty notification payloads in BlueZ backend when using "AcquireNotify". Fixes #1982. | ||
| * Fixed ``BleakScanner.stop()`` raising ``BleakDBusError`` with ``org.bluez.Error.InProgress`` in BlueZ backend when the kernel had already stopped scanning. A single rejection is now logged and tolerated; two in a row on the same adapter still raise, since that means bluetoothd's discovery state is stuck. Fixes #2021. |
There was a problem hiding this comment.
I like to keep these short and let people go to the issue for the details.
There was a problem hiding this comment.
🟡 Changes recommended
The second-response escalation contradicts the stated behavior and can reintroduce the teardown exception.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Updates BlueZ scan shutdown handling for InProgress responses.
Changes:
- Tolerates the first
InProgressresponse. - Tracks repeated responses and raises on the second.
- Adds regression tests, diagnostics, and troubleshooting guidance.
File summaries
| File | Description |
|---|---|
bleak/backends/bluezdbus/manager.py |
Adds tolerance, tracking, and logging. |
tests/backends/bluezdbus/test_issue_2021.py |
Tests shutdown response handling. |
docs/troubleshooting.rst |
Documents symptoms and recovery. |
CHANGELOG.rst |
Records the behavioral change. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| "adapter, or restart bluetoothd)", | ||
| adapter_path, | ||
| ) | ||
| raise |
|
I let Fable review this locally and it thinks the heuristic of failing twice is a bit fragile. And it offered the suggestion of rather trying to start a scan again as the test. I.e. If we get the InProgress error, call StartDiscovery and StopDiscovery again right then. If that succeeds, then it should be the harmless case and if it fails, we know it is the stuck case. That eliminates needing to track all of the things that happened between the first and second tries. |
e6392d1 to
8cf1896
Compare
BlueZ answers StopDiscovery with InProgress when the kernel has already stopped scanning on its own. bluetoothd has removed our discovery session by the time it replies, so the stop has taken effect; swallow the error like NotReady. Squash of the review history of hbldh#2022, rebased onto develop. Fixes hbldh#2021. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…nting strikes Review feedback on hbldh#2022. An InProgress reply to StopDiscovery is usually a race with the kernel's own scan timeout, but bluetoothd can also be stuck with every stop rejected. Rather than remembering the last outcome per adapter, start and stop a probe scan right away: bluetoothd answers the probe's stop from its idle state in the harmless case and the kernel rejects it again in the stuck case. The harmless case is logged at INFO and the filters are cleared as usual; the stuck case logs a WARNING with the remedy and raises the original error. Also moves the stop path into helper methods with a match statement, as requested, and drops the per-manager marker that would have outlived an adapter re-plug. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Done as suggested. The branch is rebased onto develop (it had drifted into conflict) and squashed to two commits: the original fix, and the probe. On Why the probe is a clean discriminator, from Other review points:
|
|
Did you see #2033? I prefer the way the tests there use the same "fake" BlueZ D-bus service rather than monkey-patching I didn't compare the way we implemented the actual code fix yet though. |
Move the private dbus-daemon and fake BlueZ objects from test_manager_real_dbus.py to a shared fake_bluez module and the start_daemon fixture to conftest.py so that other tests can use them. The fake adapter also gets a Roles property and scriptable SetDiscoveryFilter, StartDiscovery and StopDiscovery methods so that scanning can be tested through the public BleakScanner API.
BlueZ answers StopDiscovery with InProgress when the kernel has already stopped scanning on its own. bluetoothd has removed our discovery session by then, but it leaves its own discovering flag set, so the next scan on the adapter would be accepted without a kernel scan and find nothing (bluez/bluez#807). Start and stop discovery once more right away: that clears the flag in the harmless case and the stop then completes normally. If the second stop is rejected too, bluetoothd has lost track of the kernel's scan state for good; log a warning naming the remedy and raise the original error so the caller knows the adapter is not scanning. If the extra StartDiscovery fails, warn and return, since scanning is stopped as requested. NotReady is still tolerated without probing. Tests run BleakScanner against the fake BlueZ service on a private dbus-daemon from the shared fixture. Fixes hbldh#2021. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
8cf1896 to
7b53226
Compare
|
Yes, seen now, thanks. I have rebuilt this branch on top of your fixture commit from #2033 (it is the first commit here, unchanged) and the tests now run a real While I was at it I compared the two code versions and took your two refinements, since your reasoning on both is right: the extra Tests: five cases, all green against dbus-daemon locally and on the earlier CI runs' Linux matrix. One real-hardware data point too: on a Cerbo GX running BlueZ 5.72, a 10.3 s scan window hit the race, the second start and stop cleared it, and the scanner's stop returned normally with the INFO line. Branch is rebased on current develop. If you would rather land #2033 and close this one, that is fine by me too; the two are now the same change with the same tests, and I am happy either way. |
Fixes #2021.
The change
In the BlueZ backend,
BleakScanner.stop()no longer fails onorg.bluez.Error.InProgressfromStopDiscoverywhen the stop has in fact taken effect. On that reply it callsStartDiscoveryandStopDiscoveryonce more on the same adapter, right then and under the same bus lock, without touching the discovery filter in between (our filter is still registered with bluetoothd, so the extra start is answered without any kernel traffic; clearing the filter first would start a real unfiltered discovery). If the second stop succeeds, bluetoothd's stale discovering flag has been reset, the original stop is treated as complete, logged at INFO, and the filters are cleared as usual. If the second stop is rejected as well, bluetoothd's discovery state is stuck (see point 6), a WARNING names the adapter and the remedy, and the originalBleakDBusErroris raised so the caller learns the adapter is not scanning. If the extraStartDiscoveryitself fails, a WARNING is logged andstop()returns, since scanning is stopped as requested and the failure is from a call the user did not make.NotReadyis still swallowed without probing, and every other error still raises.The stop path lives in three small helpers (
_stop_discovery,_probe_stuck_discovery,_call_adapter) with amatchon the error name. No state is kept between calls.Why swallowing is correct
InProgressonStopDiscoveryis bluetoothd relayingMGMT_STATUS_REJECTEDfrom the kernel, which the kernel returns whenever discovery is not in theFINDINGorRESOLVINGstate (net/bluetooth/mgmt.c: stop_discovery,hci_core.c: hci_discovery_active). Before it replies, bluetoothd has already rundiscovery_remove(client)insrc/adapter.c: stop_discovery_complete(), so our session is gone when the error arrives. The stop we asked for has happened in every sense that matters to the caller. Raising turns a completed scan into an exception, and the caller cannot retry because the scanner nulls_stopbefore awaiting it.The points raised in the issue thread, one at a time.
1. "I would rather see an integration test that does it for real"
I tried, and I could not make one that fails without the fix, so I have not included one. The reason is in how the two halves interact. bluetoothd forwards a client's
StopDiscoveryto the kernel only while its owndiscovery_enableflag is set (discovery_stop, whose idle branch returns without any MGMT round trip otherwise), and it clears that flag when it processes the kernel'sDiscovering(false)event (discovering_callback). So the kernel's rejection needs a stop to be forwarded after the kernel has leftFINDING(for an LE-only scan, at its ownDISCOV_LE_TIMEOUTof 10.24 s) but before bluetoothd has processed that event. That is a scheduling race inside bluetoothd, not a time offset a test can aim at. Measured on BlueZ 5.72: stops at 10, 12, 15, 15.2, 15.4 and 16 s after start were accepted 18 of 18 times, and fivefind_device_by_address(..., timeout=15.0)teardowns stopped cleanly. I also tried inducing it from the vhci controller by holding the reply to bluetoothd's automatic restart so the kernel sits inSTARTINGwhilestop()runs; it does not work, because bluetoothd only setsdiscovery_enableinstart_discovery_complete, so during its own restart it still reads idle and stops cleanly without asking the kernel. What is included instead is a unit test that fakes the bus reply (see Test below). It pins bleak's handling; it does not exercise BlueZ.2. "We should just add some comments to it that link to the BlueZ bug or code that causes the problem"
Done. The comment above the condition names the bluetoothd path (
stop_discovery_complete()callsdiscovery_remove()before checking the status, then relaysMGMT_STATUS_REJECTEDasInProgress) and links this issue and bluez/bluez#807.3. "It sounds like we should make a patch to BlueZ to fix this"
Agreed, and I intend to send one to linux-bluetooth after this lands. The shape: in
stop_discovery_complete(), treatMGMT_STATUS_REJECTEDas "already stopped" rather than returning early, sodiscovery_enable,discoveringand theDiscoveringproperty are reset the same way they are on success. bleak's change stands on its own regardless, since every 5.x user gets the error as things are.4. "I guess this means we can't listen for a property change to tell us that the scanning stopped early"
Correct, twice over. When the kernel stops an LE scan on its own timer,
discovering_callbackonly flips bluetoothd's internaldiscovery_enable;adapter->discoveringstays true and noPropertiesChangedis emitted, so the early kernel stop is invisible over D-Bus. And after a failedStopDiscovery, the early return leavesDiscoveringtrue with no clients (bluez/bluez#807), so there is nothing to subscribe to for this case either.5. "Also see #1146 which is related to catching that scanning stopped before we requested it"
Related but a different case. From bleak's side the scan never stopped early: bluetoothd kept our session and cycled the kernel scan internally, and the error arises at our own requested stop. An early-stop hook would not fire (no property change, per point 4) and is not needed for this fix. The two are complementary.
6. "It seems like it could affect starting the next scan since the discovering property is now wrong?"
Yes, in two different ways, and the probe tells them apart. In the harmless case the kernel's
discovering = 0event has reached bluetoothd before it relays the rejected stop, sodiscovery_enableis already 0: the nextStartDiscoverywith equal filters is accepted but skipped byupdate_discovery_filter()becauseadapter->discoveringis still set, and that scan sees nothing once; itsStopDiscoverythen takes the idle branch ofdiscovery_stop(), succeeds, and clearsdiscovering. That same idle branch is what answers the probe's stop, so the probe succeeds. In the stuck casediscovery_enablestays 1, every start queues a stop whose completion callback is NULL, every last-client stop is rejected, and the probe's stop is rejected too. TheDiscoveringproperty cannot separate the two at the moment of the error: bluetoothd sends the reply before touching it and never clears it on the error path (bluez/bluez#807). The one empty scan in the harmless case is bluetoothd's bug; the patch in point 3 fixes it.What we measured
24 occurrences on one Cerbo GX (BlueZ 5.72) in a single day, every one on the
find_device_by_address(..., timeout=15.0)teardown for an absent device, none on a scan that found its device early. bluetoothd was crash-looping that day for an unrelated reason, which is consistent with the race widening under load; in the seven days since that was fixed there have been none. #958 and #1689 show other users hitting it. So the condition is real and provable from source, intermittent in practice, and not reproducible on demand.Test
tests/backends/bluezdbus/test_issue_2021.pyruns a realBleakScanneragainst the fake BlueZ service on a private dbus-daemon from the shared fixture in #2033 (that fixture commit is the first commit here). The fake adapter answersStopDiscoveryandStartDiscoveryfrom scripted reply lists. Five cases: oneInProgressis recovered (asserts the exact call sequence including the final filter reset, an INFO record and no WARNING); the second stop is rejected too (the originalInProgresserror is raised, one WARNING, no filter reset); the extraStartDiscoveryfails (returns, one WARNING);NotReadyis swallowed without probing;Failedstill raises. Also observed once on real hardware: on a Cerbo GX with BlueZ 5.72, a 10.3 s scan window hit the race, the second start and stop cleared it, and the scanner's stop returned normally with the INFO line.🤖 Generated with Claude Code