Skip to content

Review fixes: contact key race, history hot paths, build snapshots, test and doc cleanup - #175

Merged
erikwb merged 9 commits into
mainfrom
fix/recovery-completion-restart
Sep 27, 2026
Merged

erikwb merged 9 commits into
mainfrom
fix/recovery-completion-restart

Conversation

@erikwb

@erikwb erikwb commented Sep 27, 2026

Copy link
Copy Markdown
Owner

A batch of fixes from a whole-project review. Each commit stands alone, and each one passes the full hermetic suite (dbus-run-session + pytest), Ruff, and mypy when checked out by itself. Reviewing commit by commit is easiest.

Bluetooth recovery

  • Keep recovery state with the daemon that owns the bus name (26e0641). A second daemon, such as blueferry run while the service is running, read the recovery journal before claiming the name. If the claim failed, its stop() still ran finish_shutdown(), which could replay or clear restoration belonging to the live daemon. The journal is now reloaded after the claim, and restoration is skipped when the name was never acquired.

Security

  • Give contact pulls a private storage key. The PBAP pull runs on the OBEX worker for up to minutes, using the live StorageSecurity. Relocking, a policy change, or fail_closed zeroes that key buffer in place on the GLib thread. A concurrent encrypt could therefore seal contact rows under a zeroed key, and later reads would fail authentication and put storage into the error state.
    • Each pull now gets its own key snapshot.
    • The new StorageSecurity.revision changes whenever the key or policy does. If it changes during a pull, the result is discarded, the cache cleared, and the download re-queued.

Daemon responsiveness

  • Stop decrypting and rewriting history on every few messages.
    • prune_events ran on the GLib loop every 10 writes. It decrypted the whole archive (up to 10k rows) and UPDATEd every row, even rows already normalized. With secure_delete on, that rewrote nearly the whole file every few incoming messages.
    • The hot path now applies only the count and size ceilings, which need no decryption. The age sweep runs at most hourly, plus at every startup and unlock as before.
    • Mark-read uses an incremental handle→row index instead of decrypting the whole table for each read event.
  • Bound the periodic bond check. GetManagedObjects runs every 2s on the GLib loop with the default 25s timeout. It now uses a 2s timeout.
  • Check daemon compatibility once per bus owner. BackendClient called GetStatus before every operation. Verified daemons are now cached by their unique bus name, which a replacement daemon never reuses.

Packaging

  • Build source snapshots from the Git index only. build.sh and the deb and rpm builders packed every untracked, non-ignored file, including .agents/, .codex/, uv.lock, and a website/ checkout. Uncommitted edits to indexed files are still included, and untracked paths are listed.

Tests and structure

  • Move contact sync out of Daemon. The state machine lived in eight private Daemon fields and now lives in ContactSync, moved without behavior changes.
  • Build real daemons in tests. The new isolated_state / make_daemon fixtures construct a real Daemon against temporary state with no D-Bus or Bluetooth I/O. All 11 Daemon.__new__ + hand-set-private-field constructions in tests are gone.
    • One pairing-policy assertion only held because a stub skipped the step where a manual sync satisfies a deferred pull. It now checks the real behavior.
    • The rule is documented in TESTING.md.
  • Restructure ARCHITECTURE.md around a module map.
    • Every backend, client, and Quickshell file is listed.
    • Invariants are grouped by area, and bug-fix narration is condensed.
    • A "Known duplication" section replaces the claim that Quickshell shares onboarding and roster semantics.

Not in this PR

  • The runtime completion-marker half of the original recovery branch was dropped. It covered a narrow journal-cleanup failure at the cost of a second persistence layer, and regressed the case where the user logs out while a power cycle is in progress.
  • Recovery still refuses to cycle when any other device is paired, not just connected, so for most users it rarely fires. That deserves its own change.

Validation

  • 1444 tests pass on a private bus. Ruff, mypy, and Bandit pass.
  • No live hardware testing was done.

A second daemon can be constructed while the first is finishing a power
cycle. It read the recovery journal at construction and, when it then
failed to claim the bus name, still ran finish_shutdown() and could
replay or clear restoration belonging to the live owner.

Reload the journal only after claiming the name, and skip restoration
during shutdown when the name was never acquired.
The Arch, Debian, and RPM builders packed every untracked, non-ignored
file, so scratch directories, agent state, lock files, and a website
checkout silently shipped in source archives.

Snapshot the working-tree contents of indexed files, which still
includes uncommitted edits, and list any untracked paths that were left
out. Stage a new file with git add to include it.
BackendClient called GetStatus before every operation, doubling D-Bus
round trips. Remember verified daemons by their unique bus name; a bus
never reuses one, so a replacement daemon is still checked before its
first call. The GTK client shares one cache across its per-call private
connections.
Every ten history writes, prune_events decrypted the whole archive and
rewrote every row's metadata on the GLib loop, and each MAP read event
decrypted the whole table to find one handle.

- Enforce the count and size ceilings on the hot path without
  decryption, and run the age sweep at most hourly (plus at every
  startup and unlock, as before).
- Only normalize rows whose metadata actually needs it.
- Index received-message handles incrementally. AUTOINCREMENT ids are
  never reused, so each lookup decrypts only rows appended since the
  last one; a random instance id in meta detects a recreated database.
The saved-target check calls GetManagedObjects every two seconds on the
daemon's GLib loop with dbus-python's default 25 s timeout, so a wedged
bluetoothd could stall every client request. Give it a 2 s timeout; a
timeout reads as "cannot inspect", which the check already treats as
transient.
The contact-sync state machine (MAP grace period, daily refresh, joined
manual requests, storage-generation handling) lived in eight private
Daemon fields. Tests reconstructed Daemon with __new__ and hand-set those
fields, so they tracked the implementation and could assert behavior
the real code never had.

- Move it unchanged into ContactSync with an explicit public surface;
  Daemon wires it up and keeps only the setup-task and invalidation
  follow-up. The worker submit is looked up at call time.
- Add isolated_state and make_daemon fixtures that build a real Daemon
  against temporary state with no D-Bus or Bluetooth I/O.
- Test contact-sync behavior against ContactSync directly, and build
  real daemons in the lifecycle, pairing-policy, and storage-recovery
  tests. One pairing-policy assertion depended on a stub that skipped
  the manual-sync-satisfies-deferred-pull step; it now checks the real
  behavior.
The PBAP pull runs on the OBEX worker for up to minutes and encrypted
with the live StorageSecurity. Relocking, a policy change, or
fail_closed zeroes that key buffer in place on the GLib thread, so a
concurrent encrypt could seal contact rows under a zeroed key; later
reads then fail authentication and put storage into the error state.

- Hand each pull its own snapshot of the key and release it afterwards.
- Add StorageSecurity.revision, which changes with the key or policy.
  If it moved during a pull, discard the result: clear the cache,
  fail waiting callers with StorageChangedDuringSync (not reported as a
  Bluetooth failure), and download again once storage is writable.
Replace the last Daemon.__new__ constructions in the MAP-event, BlueZ
setup, and private-bus Bluetooth setup tests with make_daemon, replacing
only hardware-facing collaborators. Document the rule in TESTING.md.
The document had become one long run of prose mixing contracts with the
details of individual bug fixes. Lead with a module map covering every
backend, client, and Quickshell file, then group the invariants by area:
process boundaries, D-Bus compatibility and roster tokens, clients,
pairing, Bluetooth supervision, storage and privacy, lifecycle, tests.

Replace the claim that every client shares onboarding and roster
semantics with a Known duplication section: Quickshell re-implements
stage derivation, and the shared ConversationLogic.qml re-implements
roster logic in JavaScript.
@erikwb
erikwb merged commit 0b9aada into main Sep 27, 2026
4 checks passed
@erikwb
erikwb deleted the fix/recovery-completion-restart branch September 27, 2026 15:11
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