Skip to content

fix(sessions): honor Windows sqlite session URIs - #7098

Open
sbguangha wants to merge 1 commit into
google:mainfrom
sbguangha:fix/windows-sqlite-session-uri
Open

sbguangha wants to merge 1 commit into
google:mainfrom
sbguangha:fix/windows-sqlite-session-uri

Conversation

@sbguangha

Copy link
Copy Markdown

Please ensure you have read the contribution guide before creating a pull request.

Link to Issue or Description of Change

1. Link to an existing issue (if applicable):

  • N/A (found while running the documented unit tests on Windows)

2. Or, if no issue exists, describe the change:

Problem:
On Windows, SqliteSessionService and the CLI sqlite:// factory do not treat drive-letter session URIs the way file:// artifact URIs already do.

urlparse("sqlite:///C:/tmp/adk%20sessions.db").path stays '/C:/tmp/adk%20sessions.db' (percent-encoding is not decoded by urlparse). The factory only stripped a leading /, so --session_service_uri created a database whose filename still contained literal %20. The same helper also rejected the documented SQLAlchemy Windows form when the official unit test built sqlite+aiosqlite://// + C:\... (lstrip('/') does nothing on a drive path), so test_sqlite_session_service_accepts_absolute_sqlite_urls failed on Windows.

Steps to Reproduce:

  1. On Windows, uv venv --python 3.11 .venv and uv sync.
  2. pytest tests/unittests/sessions/test_session_service.py::test_sqlite_session_service_accepts_absolute_sqlite_urls.
  3. Or pass --session_service_uri sqlite:///C:/Users/Someone/My%20Docs/sessions.db to adk web.

Expected Behavior:
sqlite:///C:/path/to.db (and a percent-encoded space) opens that filesystem path, matching SQLAlchemy and the existing file:// artifact factory.

Observed Behavior:
The absolute-URL unit test fails because the constructed URI is not a valid Windows sqlite URL. A percent-encoded CLI URI creates adk%20sessions.db instead of adk sessions.db.

Environment Details:

  • ADK checkout: main @ 7b246e0
  • Desktop OS: Windows 10
  • Python Version: 3.11.15

Solution:
Reuse unquote + Windows url2pathname for drive-letter sqlite URL paths inside _parse_db_path (including the four-slash //C:/... shape). Point the CLI sqlite factory at that helper so it cannot drift from SqliteSessionService. Leave relative URIs (sqlite:///test.db) as relative paths so they do not become \test.db. Build the official absolute-URL test with Path.as_posix() so Unix stays ////tmp/... and Windows uses sqlite:///C:/path/to.db.

User impact

Windows users can persist sessions under a drive-letter path, including directories that contain spaces, without getting a differently named database file. The Unix relative and four-slash absolute contracts are unchanged.

Testing Plan

Unit Tests:

  • I have added or updated unit tests for my change.
  • All focused unit tests pass locally.
pytest tests/unittests/cli/test_service_registry.py::test_create_session_service_sqlite \
  tests/unittests/cli/test_service_registry.py::test_create_session_service_sqlite_ignores_unsupported_kwargs \
  tests/unittests/cli/test_service_registry.py::test_sqlite_session_factory_normalizes_windows_sqlite_uri \
  tests/unittests/cli/test_service_registry.py::test_file_artifact_factory_normalizes_windows_file_uri \
  tests/unittests/sessions/test_session_service.py::test_sqlite_session_service_accepts_sqlite_urls \
  tests/unittests/sessions/test_session_service.py::test_sqlite_session_service_preserves_uri_query_parameters \
  tests/unittests/sessions/test_session_service.py::test_sqlite_session_service_accepts_absolute_sqlite_urls \
  tests/unittests/sessions/test_session_service.py::test_sqlite_session_service_decodes_windows_percent_encoded_path \
  tests/unittests/sessions/test_session_service.py::test_parse_db_path_keeps_relative_sqlite_url_on_windows \
  tests/unittests/sessions/test_session_service.py::test_parse_db_path_windows_drive_and_percent_encoding
10 passed

Manual End-to-End (E2E) Tests:

N/A for this parser fix. The new Windows test creates a real sqlite file at tmp_path / 'adk sessions.db' from sqlite+aiosqlite:///C:/.../adk%20sessions.db and asserts the percent-encoded filename is not created.

Checklist

  • I have read the CONTRIBUTING.md document.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have added tests that prove my fix is effective or that my feature works.
  • New and existing focused unit tests pass locally with my changes.
  • I have manually tested my changes end-to-end (covered by the Windows sqlite file-creation unit test).
  • Any dependent changes have been merged and published in downstream modules.

Additional context

file:// artifact URIs already call unquote + url2pathname on Windows. This change brings sqlite session URIs to the same rule. I have signed / will sign the Google CLA as required by CONTRIBUTING.

Percent-encoded sqlite:// paths were stored as literal %20 filenames on Windows, unlike file:// artifacts. Parse drive-letter URIs with url2pathname so session DBs land on the intended path.
@google-cla

google-cla Bot commented Sep 12, 2026

Copy link
Copy Markdown

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

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.

2 participants