Fix SQLite concurrency risk in create_or_get method - #8
krishna3554 wants to merge 3 commits into
Conversation
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| time.sleep(0.1 * (2 ** attempt)) | ||
| continue | ||
| raise | ||
| raise RuntimeError("Unreachable code reached") |
There was a problem hiding this comment.
🔴 Leftover duplicated code block after the rewritten save routine breaks the memory service entirely
An old copy of the save logic was left behind after the new retry loop's final error (raise RuntimeError("Unreachable code reached") at services/dma-api/src/dma_api/repository.py:221) at a deeper indentation than the surrounding code, so the memory service can no longer start or store anything.
Impact: The API fails to load, so every request — remembering, recalling, listing, deleting — stops working.
Incomplete refactor leaves an orphaned, unreachable copy of the transaction body
The PR moved the body of create_or_get into a for attempt in range(max_retries) / try / with self._connect() block (lines 139-221), but the pre-existing body was not deleted. Lines 223-281 remain at 12-space indentation directly after the 8-space-indented raise RuntimeError(...) on line 221, with no enclosing block. This is an indentation/parse failure for the whole repository.py module, so dma_api.main (which imports SQLiteMemoryRepository, see services/dma-api/src/dma_api/main.py:69) cannot be imported. Even ignoring parsing, the block is dead code referencing an undefined connection.
Prompt for agents
In services/dma-api/src/dma_api/repository.py, the refactor of create_or_get into a retry loop (lines 139-221) duplicated the original method body: the old copy still exists at lines 223-281 at an indentation level that no longer belongs to any enclosing block, immediately after the `raise RuntimeError("Unreachable code reached")`. Remove the stale duplicated block so create_or_get contains only the new retry-wrapped implementation, and verify the module imports and the existing test suite passes.
Was this helpful? React with 👍 or 👎 to provide feedback.
| with self._connect() as connection: | ||
| connection.execute("BEGIN IMMEDIATE") |
There was a problem hiding this comment.
🟡 Failed write attempts leave database connections open, and repeated retries multiply the leak
Each attempt opens a new database connection that is never closed (with self._connect() as connection at services/dma-api/src/dma_api/repository.py:142), so retrying a locked write accumulates several open handles per request instead of one.
Impact: Under sustained concurrent writes the service can exhaust file handles and lock the database for longer, making writes fail more often.
sqlite3 connection context manager commits/rolls back but does not close
SQLiteMemoryRepository._connect (services/dma-api/src/dma_api/repository.py:403-407) returns a raw sqlite3.Connection. Using it as a context manager only manages the transaction — on exit it commits or rolls back but leaves the connection (and its file lock, until garbage collection) open. Previously one connection leaked per call; with the new loop up to max_retries connections leak per call, and the leaked connection from a failed attempt may still hold a reserved lock while the next attempt runs BEGIN IMMEDIATE, which can make the retry itself hit "database is locked". Wrapping with contextlib.closing(...) (or a try/finally close) fixes it.
Prompt for agents
services/dma-api/src/dma_api/repository.py:_connect returns a bare sqlite3.Connection and all call sites use `with self._connect() as connection`, which manages the transaction but never closes the connection. The new retry loop in create_or_get makes this worse because a failed attempt's connection stays open (potentially still holding locks) while the next attempt begins. Consider making _connect a contextmanager that closes the connection on exit (or wrap with contextlib.closing at each call site), and confirm the retry path releases resources between attempts.
Was this helpful? React with 👍 or 👎 to provide feedback.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 69522744ed
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Addresses SQLite locking issues that could cause data corruption under concurrent multi-tenant writes.\n\n- Added retry logic with exponential backoff for 'database is locked' errors in create_or_get\n- Improves data integrity under concurrent load from multiple tenants\n\nFixes issue identified during code review for scalability before beta release.