Skip to content

Require aiosqlite 0.22.1 - #1898

Open
TheJulianJES wants to merge 2 commits into
devfrom
zigpy-bot/aiosqlite-0.22
Open

TheJulianJES wants to merge 2 commits into
devfrom
zigpy-bot/aiosqlite-0.22

Conversation

@TheJulianJES

@TheJulianJES TheJulianJES commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

This PR adds support for aiosqlite 0.22.x and also requires 0.22.1 (or newer – currently latest).
See their changelog: https://github.com/omnilib/aiosqlite/blob/HEAD/CHANGELOG.md

Also closes #1762

AI-generated PR description

Lifts the <0.22.0 cap on aiosqlite added in #1729. aiosqlite 0.22 is a breaking release for zigpy: Connection is no longer a Thread subclass. The worker thread is now Connection._thread, and _stop_running() became the public stop() in 0.22.1. On dev with 0.22.1 installed, PersistingListener.shutdown() fails with AttributeError: 'Connection' object has no attribute 'join'.

Changes

  • Require aiosqlite>=0.22.1. That release (Improve stop semantics for connections omnilib/aiosqlite#370) adds the public stop(), which also works without a running event loop; the test fixture relies on that. Nothing else in the dependency tree caps aiosqlite: zha and the radio libraries don't depend on it, and the only other requirement in HA Core's tree, knx-telegram-store's aiosqlite>=0.20, is a lower bound.
  • shutdown() joins the worker thread through self._db._thread instead of self._db. The join now sits in its own finally, so it also runs if close() raises; aiosqlite's close() stops the thread in that case too.
  • auto_kill_aiosqlite now records the connections created during the test, by wrapping Connection.__init__, and calls stop() on any whose thread is still alive. It used to scan threading.enumerate() for Connection threads, which no longer exist.
  • verify_cleanup drops its aiosqlite special case, since isinstance(thread, aiosqlite.Connection) can no longer be true.
  • Three shutdown tests check listener._db._thread.is_alive() instead of listener._db.is_alive().

About the race from #1729: since 0.22.0, which includes omnilib/aiosqlite#305, await close() returns only after the worker has processed the stop request. The worker schedules that future's result just before it leaves its loop, though, so the thread can still be running when close() returns. Without extra load this is rare: 0 of 600 close/check cycles in one run, 2 of 600 in another on a busy machine. With two CPU-bound threads competing for the GIL and a short switch interval, the thread was still alive right after close() in 10 to 15 of 200 cycles across three runs. So the explicit join stays.

About _thread: aiosqlite has no public way to wait for the worker thread. omnilib/aiosqlite#387 asks for a join() method and has no response yet. Awaiting the future returned by stop() doesn't help either. close() already awaits the future from its own stop() call, and calling stop() again afterwards returns a new future that never resolves, because the worker has already exited. SQLAlchemy's aiosqlite dialect reaches into connection._thread too since 0.22 (sqlalchemy/sqlalchemy#13039), to mark it as a daemon thread. If a future aiosqlite release renames the attribute, shutdown() raises AttributeError only after the connection is closed, ControllerApplication.shutdown() logs it as a warning, and the shutdown tests that check _thread fail in CI once it rebuilds its environment. #1762 skipped the join when join was missing; that would silently bring back the race, and this PR supersedes it.

Testing: the full suite passes on aiosqlite 0.22.1 (1350 passed). test_shutdown_joins_thread_when_close_fails makes aiosqlite's own close step raise, spies on the thread's join(), and fails if the join is skipped. With an open connection deliberately left behind by a test, auto_kill_aiosqlite stops its thread after the test.

aiosqlite 0.22 no longer makes `Connection` a `Thread`: the worker thread is
now `Connection._thread`, `_stop_running()` became `stop()`, and `close()`
waits for the worker to process the stop request. Require 0.22.1, which has
the public `stop()`, and join the worker thread through `_thread` on shutdown,
since `close()` can still return just before the thread exits.

`auto_kill_aiosqlite` now tracks the connections created during a test instead
of looking for `Connection` threads, and `verify_cleanup` drops its aiosqlite
special case, which can no longer match.
@codecov

codecov Bot commented Oct 6, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.51%. Comparing base (b4543ff) to head (2b752da).

Additional details and impacted files
@@           Coverage Diff           @@
##              dev    #1898   +/-   ##
=======================================
  Coverage   99.51%   99.51%           
=======================================
  Files          61       61           
  Lines       12773    12774    +1     
=======================================
+ Hits        12711    12712    +1     
  Misses         62       62           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment thread zigpy/appdb.py Outdated
Comment on lines +278 to +281
# `close()` can return before aiosqlite's worker thread has exited
await asyncio.get_running_loop().run_in_executor(
None, self._db._thread.join
)

@TheJulianJES TheJulianJES Oct 6, 2026 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still not happy about reaching into internals here but SQLAlchemy's dialect seems to do something similar still: https://github.com/sqlalchemy/sqlalchemy/blob/529fa3520bcc59ffc884ffe5bcb46d7a9961ebd5/lib/sqlalchemy/dialects/sqlite/aiosqlite.py#L293-L299

I'll look into this a bit more. Upstream issue seems to be omnilib/aiosqlite#387

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was working on an SQLAlchemy port last year, we could also try it: #1637

@TheJulianJES TheJulianJES Oct 6, 2026 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. I was looking at this just because of the previous batching PR and wanting to do some other improvements before playing around with the generic storage stuff. But if we can also move to SQLAlchemy before, that might be better(?)

@zigpy-review-bot zigpy-review-bot Oct 6, 2026 •

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked into it a bit more, and _thread still looks like the least bad option:

Why (CLICK TO EXPAND)

  • There's no public way to wait for the thread. Add a join() method omnilib/aiosqlite#387 has had no response since July, and nothing has landed upstream since 0.22.1 (Dec 2025).
  • Awaiting the future that stop() returns doesn't help either. close() already awaits the one from its own stop() call, and the worker schedules its result just before leaving its loop. Calling stop() again afterwards returns a new future that never resolves, because the worker is gone.
  • The join is still needed. Without extra load the thread is rarely still alive when close() returns: 0 of 600 cycles in one run, 2 of 600 in another on a busy machine. With two CPU-bound threads competing for the GIL and a short switch interval, it was still alive in 10 to 15 of 200.
  • SQLAlchemy reaches into connection._thread for 0.22 too (SqlAlchemy aiosqlite wrapper doesn't close the aiosqlite connection object (hanging thread) sqlalchemy/sqlalchemy#13039, to make it a daemon thread). zzzeek wasn't happy about it either, but merged it.
  • If aiosqlite ever renames it, the failure is mild: the AttributeError comes after the connection is already closed, ControllerApplication.shutdown() logs it as a warning, and our shutdown tests that use _thread fail in CI once it rebuilds its environment, so we'd notice.

I pushed 2b752da: the join now runs in its own finally, so it also happens if close() raises. The comment there points to omnilib/aiosqlite#387, and a new test makes aiosqlite's close step fail and checks that join() still runs. Once omnilib/aiosqlite#387 lands, we can switch to the public method.

On the SQLAlchemy port (CLICK TO EXPAND)

On the SQLAlchemy port (#1637, closed as stale): SQLAlchemy still runs on aiosqlite underneath. Its dialect closes connections with a plain await connection.close() and no join, and since 0.22 it avoids hangs at exit by setting connection._thread.daemon = True. So a port moves the private access into SQLAlchemy rather than removing it; the window after close() stays, but the process can still exit because the thread is a daemon. On the ordering question: I don't think the port has to come first. The storage-seam work for the generic store needs a key/value interface routed through the worker, a migration strategy, and lifetimes that don't hang off foreign keys. SQLAlchemy doesn't provide any of those by itself. Where it would help is as a statement builder, or if we want other backends later. If we do port, two things have to be redone: the #1835 commit batching (the timer and forced commits drive commit() on the aiosqlite connection), and the migration rollback that #1637's description says fails. Either way, this PR is small and independent of that decision, and it unblocks 0.22 now.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zigpy-review-bot With SQLAlchemy setting connection._thread.daemon = True, would that still trigger HA warnings about leaking threads on shutdown? (Unlike this approach?) If so, we'd also need to work around that if we used SQLAlchemy directly?

@zigpy-review-bot zigpy-review-bot Oct 6, 2026 •

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, the daemon flag wouldn't cause HA warnings. HA has no leaked-thread warning at runtime, and the daemon flag only makes HA's shutdown skip the thread. With this PR it doesn't matter either way, since the thread is joined before ControllerApplication.shutdown() returns.

For a SQLAlchemy port, HA itself wouldn't need a workaround. Our own test suite isn't affected today either: zigpy's verify_cleanup is currently a no-op, because it's a plain sync @pytest.fixture that calls asyncio.get_running_loop(), which raises there, so it returns before checking anything. HA's version is a @pytest_asyncio.fixture using asyncio.get_event_loop(), and zha's is an async fixture. Once ours is fixed the same way, SQLAlchemy's await connection.close() without a join could make the rare window after close() described in the PR body show up as an occasional "Lingering thread" failure, unless a fixture still joins the thread (through _thread again) or verify_cleanup skips aiosqlite's threads.

What I checked (CLICK TO EXPAND)

  • HA runtime: runner.py replaces threading._shutdown with deadlock_safe_shutdown() (homeassistant/util/thread.py). That collects threads that are alive and not daemon, joins them within a shared 10 s budget, and only logs if join() itself raises. A daemon thread is skipped; a non-daemon one that is still finishing just gets joined. Neither logs anything.
  • HA's and zha's test suites: their verify_cleanup fixtures fail on any new thread after a test except _DummyThread and waitpid threads (and, in HA, _run_safe_shutdown_loop), so the daemon flag wouldn't exempt anything there. Neither suite opens a zigpy database, though: both set CONF_DATABASE: None for the app controller, so no aiosqlite thread is ever started in them.
  • Python itself (aiosqlite 0.22.1, Python 3.14): an unclosed connection only lets the process exit if it gets garbage-collected first, because aiosqlite's __del__ then stops the thread. If something still holds a reference at exit, as SQLAlchemy's pool does, plain Python waits on the non-daemon worker thread forever. With _thread.daemon = True set before the connection is awaited, it exits, with aiosqlite's ResourceWarning. Under HA, deadlock_safe_shutdown() would cap that wait at 10 s.
  • What SQLAlchemy's flag does: it keeps an open (or still-finishing) worker thread from holding up interpreter exit. It doesn't make the thread finish any sooner after close(). It is also only set when the dialect calls aiosqlite.connect() itself, not when an async_creator_fn is passed.

aiosqlite's `close()` always stops its worker thread, even when closing the
connection raises, so the join is now in a `finally` and still runs in that
case. The comment also points to omnilib/aiosqlite#387, which asks for a
public way to wait for the thread.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟢 Approval recommended

The dependency, shutdown implementation, cleanup fixture, and tests consistently support aiosqlite 0.22.1.

Review effort: Balanced
Findings: None

What changed in this PR

Adds compatibility with aiosqlite 0.22.1’s revised worker-thread lifecycle.

Changes:

  • Requires aiosqlite 0.22.1 or newer.
  • Joins the internal worker thread during shutdown.
  • Updates cleanup fixtures and shutdown tests for the new API.
File Description
zigpy/​appdb.py Joins the new worker thread after closing.
tests/​test_appdb.py Updates and extends shutdown coverage.
tests/​conftest.py Tracks and stops leaked connections.
pyproject.toml Raises the minimum aiosqlite version.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@TheJulianJES
TheJulianJES marked this pull request as ready for review October 6, 2026 17:42
@TheJulianJES

Copy link
Copy Markdown
Contributor Author

Even if we port to SQLAlchemy next, I think it still makes sense to get this upgrade in before then. I’ll look at everything else.

(And note: Both the previous batching PR and this one are not something for the current HA cycle. If we need zigpy bugfixes, we can always release that separately from this.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

code quality Improvement to code quality

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants