Skip to content

Fix roster deletion, bond-check timeout, stale contact pulls, and a flaky test - #180

Merged
erikwb merged 5 commits into
mainfrom
fix/review-followups
Oct 4, 2026
Merged

erikwb merged 5 commits into
mainfrom
fix/review-followups

Conversation

@erikwb

@erikwb erikwb commented Oct 4, 2026 •

Copy link
Copy Markdown
Owner

Follow-up fixes from a review of #175, #176 and #179, plus the cause of the intermittent test failures on push runs.

Deleted group roster comes back (#176). A roster adopted from history keeps its legacy key. When two spellings of a name share that key ("Golf"/"golf"), neither thread lists it as an alias, so deleting both conversations left the roster in settings.json, and the next message revived the thread as reply-ready with the deleted recipients. GroupRoutesStore.discard now also matches a roster by the current key for its recorded name, the rule the old history path used.

Roster store filling up (#176). Rosters whose conversation has aged out cannot be selected for deletion but still count toward MAX_GROUP_ROUTES, where every new save failed. At the limit, saving now evicts the oldest roster whose conversation no longer exists. A roster for a conversation that still exists is never evicted; if all 200 are in use, the save fails as before. "Exists" means present in the conversation list, so a group outside the conversation window can lose its roster at the cap.

Bond-check timeout (#175). The 2 s timeout applied only to GetManagedObjects; dbus-python first blocked on its own Introspect with the default 25 s timeout, so a wedged bluetoothd still stalled the GLib loop for about 27 s per check. _object_manager() now builds its proxy with introspect=False. Every caller only calls GetManagedObjects, which takes no arguments.

Contact pull under an abandoned policy (#175). The pull's worker holds a private copy of the storage policy and key, so it committed the phonebook under the old policy after the live one changed, and the rows were erased only when the completion reached GLib. If the daemon stopped mid-pull, they stayed on disk. The worker's snapshot now follows the live revision and refuses to seal or commit once it changes, so the transaction rolls back and the previous cache is kept. The cleanup in _pulled remains for a change that lands after the commit. A pull discarded this way is marked as owed and downloads again as soon as storage is usable.

Flaky private D-Bus tests. Two tests in tests/test_ancs_client.py left real GLib timers on the default main context. On a slow runner the 15 s ANCS request timeout was still pending when the private D-Bus tests pumped that context; the stale client then made blocking calls on the test bus, each stalling 25 s. Both tests now use fake timers.

Validation:

  • Full private-D-Bus suite: 1,480 passed. Ruff, Bandit, and mypy passed.
  • Each new regression test fails with its fix removed.
  • The flake reproduces on the old tests with a forced 16 s pause before the first D-Bus test (about 2m37 stall, one F, abrupt exit), and the full suite passes with the same pause after the fix.
  • New private-D-Bus test: with a never-dispatched owner of org.bluez, a 0.5 s bond check returns in under 5 s.

Not addressed: orphaned rosters still cannot be deleted individually below the cap, and the bond check's service-activation wait when org.bluez has no owner is still not covered by the timeout.

erikwb added 5 commits October 4, 2026 09:32
test_initial_subscription_waits_for_a_settled_le_bearer patched
GLib.timeout_add_seconds, but AncsClient's schedule default was already
bound to the real function, and
test_le_reconnect_starts_notify_only_when_bluez_dropped_ccc never
patched the request timer. Both left real sources on the default main
context.

On a slow runner the 15 s request timeout was still pending when the
private D-Bus tests pumped that context. The stale client then made
blocking calls on the real test bus, each stalling 25 s, which failed
those tests intermittently.
A roster adopted from history keeps its legacy key. When two spellings
share that key, neither thread lists it as an alias, so deleting the
conversations left the roster in settings and the next message revived
the thread with the deleted recipients. Match a stored roster by the
current key for its recorded name as well, as the history path did.

Rosters whose conversation has aged out cannot be selected for deletion
but still count toward the limit, where every new save failed. At the
limit, evict the oldest roster whose conversation no longer exists. A
roster that is still in use is never evicted.
The 2 s timeout applied only to GetManagedObjects. Each call builds a
fresh proxy, and dbus-python first blocks on its own Introspect with
the default 25 s timeout, so a wedged bluetoothd still stalled the GLib
loop for about 27 s per check.

GetManagedObjects takes no arguments, so introspection adds nothing.
Build the ObjectManager proxy without it.
The pull's worker holds a private copy of the storage policy and key,
so it committed the phonebook under the old policy even after the live
one changed. The rows were erased only when the completion reached
GLib, which never happens if the daemon stops during the pull.

Let the worker's snapshot follow the live revision and refuse to seal
or commit once it changes, so the transaction rolls back and the
previous cache is kept. The existing cleanup still covers a change
that lands after the commit.
A pull rolled back because storage changed keeps the previous cache, so
nothing marked a replacement download as needed and none was queued
until the next daily refresh. Mark it as owed; it starts as soon as
storage is usable.

Say in the roster store's docstrings that eviction judges a
conversation by whether the caller can see it. Make the eviction test
distinguish the oldest roster from the first one saved, and have the
bond-check test assert that it owns org.bluez.
@erikwb
erikwb merged commit 7276309 into main Oct 4, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant