Skip to content

Conversation

@dan-fernandes
Copy link
Contributor

Fixes #1223

@danielballan
Copy link
Member

danielballan commented Nov 19, 2025

Thanks so much @dan-fernandes! This is great, and very appreciated.

We observed a related recent regression in #1228. I suspect this will fix that as well.

I pushed some additional commits to your PR branch here fixing these failures:

FAILED tiled/_tests/test_sync.py::test_copy_internal - tiled.client.utils.ClientError: 409: /a http://local-tiled-app/api/v1/metadata/
FAILED tiled/_tests/test_sync.py::test_copy_skip_conflict - tiled.client.utils.ClientError: 409: /a http://local-tiled-app/api/v1/metadata/
FAILED tiled/_tests/test_sync.py::test_copy_warn_conflict - tiled.client.utils.ClientError: 409: /a http://local-tiled-app/api/v1/metadata/
FAILED tiled/_tests/test_sync.py::test_copy_error_conflict - tiled.client.utils.ClientError: 409: /a http://local-tiled-app/api/v1/metadata/
FAILED tiled/_tests/test_sync.py::test_copy_external - tiled.client.utils.ClientError: 409: /group http://local-tiled-app/api/v1/register/
FAILED tiled/_tests/test_sync.py::test_copy_search_results - tiled.client.utils.ClientError: 409: /a http://local-tiled-app/api/v1/metadata/
FAILED tiled/_tests/test_sync.py::test_copy_items - tiled.client.utils.ClientError: 409: /a http://local-tiled-app/api/v1/metadata/
FAILED tiled/_tests/test_sync.py::test_copy_dict - tiled.client.utils.ClientError: 409: /a http://local-tiled-app/api/v1/metadata/

I think your last commit, introducing use_cached_database, attempted to fix this issue but in doing so created a new issue, as a distinct in-memory database would be created on every request. My approach reverts that commit and instead uses two distinct (file-based) databases in the test. (If we prefer, we could specify distinct in-memory databases using a URI like sqlite:///file:SOME_UNIQUE_NAME?mode=memory&cache=shared&uri=true.)

Edit: In fact, I did add a commit to use this "named memory" approach. It's a lighter-touch change than introducing file-based databases, and it may run a little faster on CI, which has very slow disk.

Copy link
Member

@danielballan danielballan left a comment

Choose a reason for hiding this comment

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

Minor items

@danielballan
Copy link
Member

I introduced new utility function, is_memory_sqlite, with tests. This is an improvement—more robust and consistent—but I am still seeing this failing test:

FAILED tiled/_tests/test_validation.py::test_unknown_spec_permissive - sqlalchemy.exc.OperationalError: (sqlite3.OperationalError) no such table: nodes

It appears to be flaky, involving a race condition because locally it will sometimes pass.

@danielballan
Copy link
Member

I think those last commits did the trick. My train is almost home, so I'll hand off to 🇬🇧 here.

@dan-fernandes
Copy link
Contributor Author

As mentioned in #1223, I've gone back on caching all in-memory databases, and instead given the authn database a unique in-memory, cached URI. This more closely resembles the expected behaviour of SQLAlchemy and avoids weird workarounds.

Also, minor changes.

@danielballan danielballan merged commit f7da02e into main Nov 21, 2025
12 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.

Broken create_catalog.py

3 participants