Repository navigation
fix(sqlite): drain failed connection workers - #2468
Conversation
PR oceanbase#2004 exhausted the 25-minute step after passing individual native cases. Spread all 39 nodes over three shards for each supported CI Python version; retain the preliminary checks and baseline stability repetitions. Constraint: Keep the 180-second node, 25-minute step and 60-minute job budgets. Scope-risk: CI scheduling only; no test selection or runtime behavior changes. Tested: pytest tests/test_ci_changes.py tests/test_ci_pipeline.py -q (125 passed); exact 39-node partition; full Python 3.11 shard 0 replay including prelude and baseline repetitions passed in 1041.98 seconds.
Install the aiosqlite 0.22.1 stop waiter before the initial connection, which may fail before SQLAlchemy emits its connect event. Await the worker's final callback before exposing failed opens or cancellation to callers that may immediately close their event loop. Reuse that waiter after pool admission and report thread-start failure without waiting. Constraint: Keep the workaround restricted to aiosqlite 0.22.1 and preserve cancellation. Scope-risk: Connection lifecycle only; no stored schema, migration or release-policy changes. Refresh the managed-input compatibility digest. Tested: Python 3.12 focused regressions 396 passed with thread warnings as errors; Python 3.11 and 3.14 profile/CLI suites 65 passed each; failed-connect regressions red on original code; thread-start hang reproduced before guard; full Linux ty and changed-file hooks passed.
There was a problem hiding this comment.
🟢 Approval recommended
No active review issues remain for the SQLite connection-cleanup changes.
No open issues 🎉
What changed in this PR
This PR hardens SQLite connection cleanup so failed opens drain aiosqlite workers before callers can close the event loop. It addresses failures before pool admission without changing public APIs or stored schemas.
Changes:
- Install the aiosqlite 0.22.1 stop guard before initial connection, with cancellation-protected cleanup and immediate reporting of thread-start failures.
- Add regressions for failed writable/read-only opens and worker startup failure.
- Refresh the persistence digest and compatibility rationale while retaining the
upgrade-requiredpolicy.
| File | Description |
|---|---|
tests/builtin/persistence/test_sqlite_profile.py |
Add failed-open cleanup and thread-start failure regressions. |
src/powercontext/builtin/persistence/sqlite/profile.py |
Guard initial connections and reuse the stop waiter after pool admission. |
release/upgrade-support.json |
Update the persistence digest and document lifecycle-only compatibility changes. |
Review effort: Balanced
hidb4ai
left a comment
hidb4ai
left a comment
There was a problem hiding this comment.
No blocking correctness findings at 97b0e6c. Installing the stop guard before the initial connection covers the failure path that never reaches pool admission. The failed-open regressions fail against the parent implementation and pass here; repeated cancellation during initial connection and failed-open cleanup also drains the real workers. No new database tables or public interfaces.
CI is still incomplete. The Windows resource-candidate failure occurs in curl while downloading a dependency, before SQLite execution; that job still needs a successful run. This approval does not establish full installed-release upgrade acceptance.
The SQLite cleanup PR still used two migration shards and its Python 3.11 shard 0 exhausted the 25-minute unified-migration step. Merge the companion CI commit from oceanbase#2458 so this PR validates the three-shard layout itself. Constraint: Preserve all native cases, stability repeats and existing deadlines. Scope-risk: Merge only the two-line workflow change; SQLite source and managed policy bytes are unchanged. Tested: Combined CI classification/pipeline and SQLite profile/CLI suites: 190 passed with thread exceptions treated as errors; changed-file hooks and full Linux ty check passed.
|
CI follow-up at
The latest review contains no requested source changes or open inline threads. CI for the new Head is still pending. |
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
4 open issues
% 3) and also implicitly duplicated by the matrix values. To… · Newasync_creator, which means SQLAlchemy… · Newraise interrupted from error) can produce confusing exception… · Newaiosqlite.__version__ != '0.22.1'(the… · New
Review effort: Lite
| for index in "${!seekdb_runner_nodes[@]}"; do | ||
| if [ "$((index % 2))" -ne "$SEEKDB_MIGRATION_SHARD" ]; then | ||
| if [ "$((index % 3))" -ne "$SEEKDB_MIGRATION_SHARD" ]; then | ||
| continue | ||
| fi |
There was a problem hiding this comment.
Treating this as a non-blocking design suggestion. The workflow uses a fixed three-way partition, and changing that partition requires reviewing both the runner matrix and node assignment against the native test budget. The final #2458 run exercised all six Python/shard combinations successfully. I am keeping the explicit matrix and modulus rather than adding another configuration source solely for a possible future partition change.
| def _create_sqlite_engine(url: str | URL, **options: Any) -> AsyncEngine: | ||
| if aiosqlite.__version__ != "0.22.1": | ||
| return create_async_engine(url, **options) | ||
| parsed = make_url(url) | ||
| args, kwargs = parsed.get_dialect()().create_connect_args(parsed) | ||
|
|
||
| async def connect() -> Connection: | ||
| driver = aiosqlite.connect(*args, **kwargs) | ||
| stop = _make_sqlite_stop_idempotent(driver) | ||
| try: |
There was a problem hiding this comment.
The async_creator limitation is real, but this private helper has only two supported callers: SQLiteProfile.open passes echo/hide_parameters and, for nonpersistent storage, poolclass; open_readonly passes echo/hide_parameters. Neither accepts or forwards connect_args. URL driver settings still come from the dialect's create_connect_args. There is no current supported input that loses driver overrides, so I am not adding unused connect_args support in this fix. If a caller later exposes driver overrides, those must be merged into the creator's kwargs at that boundary.
| except BaseException as error: | ||
| if driver._thread.ident is None: | ||
| raise | ||
| # Failed connects never reach SQLAlchemy's connect event. Drain the | ||
| # driver's final callback before its caller can close the event loop. | ||
| stopped = stop() | ||
| interrupted = None | ||
| if stopped is not None: | ||
| while not stopped.done(): | ||
| try: | ||
| await asyncio.shield(stopped) | ||
| except asyncio.CancelledError as cancellation: | ||
| interrupted = cancellation | ||
| stopped.result() | ||
| if interrupted is not None: | ||
| raise interrupted from error | ||
| raise |
There was a problem hiding this comment.
Rechecked this with actual aiosqlite workers for both an initial connection error and an initial cancellation, with two further cancellation requests while shutdown is blocked. In both cases the operation stays pending until the worker exits, then the task is cancelled (task.cancelled() is true) and all stop futures are complete. Raising CancelledError already preserves task cancellation; re-cancelling would add a synthetic cancellation request. Chaining to the interrupted error affects diagnostic presentation but does not demonstrate a cancellation-contract failure, so I am retaining the current behavior.
| @pytest.mark.parametrize("read_only", [False, True]) | ||
| def test_failed_sqlite_open_drains_its_worker_before_reporting_the_error(tmp_path, monkeypatch, read_only) -> None: | ||
| path = tmp_path / "unavailable.db" | ||
| if not read_only: | ||
| path.mkdir() | ||
| config = SQLiteConfig(url=f"sqlite+aiosqlite:///{path}") | ||
| release = ThreadEvent() | ||
| stopping = [] | ||
| original_stop = aiosqlite.Connection.stop |
There was a problem hiding this comment.
Addressed in #2496: the existing real-worker regression now requires a nonempty shutdown-future list before checking that every future is complete. It does not skip the check based on a driver version. A local mutation that removes only future recording passes both cases without the new assertion and fails both with it, confirming that the guard catches the vacuous-success path.
Which issue or RFC does this PR close?
Follow-up acceptance for #2084, found while validating the integrated master after #2004.
Rationale for this change
A missing read-only database or an invalid writable database path can fail before SQLAlchemy's connect event. With the locked aiosqlite 0.22.1, its initial-connect exception handler queues a worker shutdown callback without awaiting it. A CLI caller may then close its event loop before that callback runs, producing an unhandled worker-thread
RuntimeError: Event loop is closed.The strict acceptance replay on unmodified master reproduced this as 348 passing tests and one thread-exception error. Installing the existing stop guard only at successful pool admission cannot protect this path.
What changes are included in this PR?
Are there any user-facing changes?
Failed SQLite opens finish worker cleanup before returning the original error or cancellation. No public API, stored schema, registered revision checksum, migration resources, or upgrade-source policy changes. The workaround remains restricted to aiosqlite 0.22.1. The upgrade policy remains
upgrade-required.How was this change tested?
pytest.PytestUnhandledThreadExceptionWarningtreated as an error.ty check, and managed upgrade-policy validation passed.1190dcbbb; those results are retained as parent evidence and are not relabeled as acceptance of this new commit.97b0e6c261bcdb9e278fde183fd663dfdc62dbf6, with actual VCS version1.5.1.dev69+g97b0e6c26; verified the sdist rebuild and froze a new acceptance plan. The wheel SHA-256 is621cbbae587b317841b685e453dc4d774f7edd2fdfda5117a26f8631de9cb6d2.3825d7a15718ffe41a40c9c8a8640213cfc39f53, combined CI classification/pipeline and SQLite profile/CLI regressions passed all 190 tests, with thread exceptions treated as errors; changed-file hooks and full Linux type checking passed. This merge changes only the workflow relative to97b0e6c2; runtime, tests and managed policy bytes are unchanged. Current-Head GitHub CI is a new run.AI usage statement
OpenAI Codex assisted with investigation, implementation, and validation.