Repository navigation
[Database] Enable PRAGMA foreign_keys = ON for SQLite connections - #84
Open
LetMeSleep8h wants to merge 1 commit into
Open
LetMeSleep8h wants to merge 1 commit into
LetMeSleep8h wants to merge 1 commit into
Conversation
SQLite enforces foreign keys only when asked, per connection: without this the cascade deletes between workspaces, keys and requests never fire. Also sets synchronous=NORMAL, the documented pairing with WAL. The pragma assertions join the existing connection test and a new cascade test proves ON DELETE CASCADE now deletes. Fixes PicadoLabs#80
Kaap10
requested changes
Oct 25, 2026
Kaap10
left a comment
Kaap10
left a comment
Collaborator
There was a problem hiding this comment.
Thanks for your contribution.
While the intent of enabling PRAGMA foreign_keys=ON and PRAGMA synchronous=NORMAL is correct, this change causes 21 test failures across test_redis_lifecycle.py and test_traffic_sse.py because child tables referencing requests.request_id do not have ondelete="CASCADE" configured on their foreign key columns in backend/app/storage/models.py:
sqlalchemy.exc.IntegrityError: (sqlite3.IntegrityError) FOREIGN KEY constraint failed [SQL: DELETE FROM requests]
Required Fixes:
- In backend/app/storage/models.py, add ondelete="CASCADE" to the foreign key columns referencing requests.request_id:
- RoutingDecisionRecord: ForeignKey("requests.request_id", ondelete="CASCADE")
- ResponseRecord: ForeignKey("requests.request_id", ondelete="CASCADE")
- FeedbackRecord: ForeignKey("requests.request_id", ondelete="CASCADE")
- ExperimentRunRecord: ForeignKey("requests.request_id", ondelete="CASCADE")
- In backend/tests/test_redis_lifecycle.py and backend/tests/test_traffic_sse.py, ensure test cleanup cleans child records or correctly exercises cascade deletes.
- Run the full test suite (pytest backend/tests) to verify all 144 tests pass before requesting re-review.
Collaborator
|
@LetMeSleep8h just checking in to see the progress. Let me know any blockers you are facing. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #80.
configure_sqlite_connectionnow also executesPRAGMA foreign_keys=ONandPRAGMA synchronous=NORMAL(the documented pairing with WAL).test_sqlite_connections_configure_journal_and_timeoutnow asserts both pragmas read back on every connection (foreign_keys=1,synchronous=1), in-memory and file-backed.test_sqlite_foreign_keys_enforce_cascade: creates parent/child tables withON DELETE CASCADE, deletes the parent, and asserts the child row went with it — which is exactly what silently did not happen before.pytest backend/tests/test_database_sqlite.py: 4 passed.