fix(test): resolve parallel E2E database locks and timeouts#271
Conversation
|
Warning Review limit reached
More reviews will be available in 31 minutes and 50 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughAdds SQLite WAL and timeout settings in the app, refreshes the stored default-admin password hash when needed, removes explicit 301 redirect tuples, and reworks E2E SQLite test database setup, cleanup, and helper transactions for isolated parallel runs. ChangesApp: SQLite pragmas and redirect fixes
E2E: parallel-safe test database lifecycle
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
DogukanUrker
force-pushed
the
fix/e2e-parallel-concurrency
branch
from
80a297a to
f4fd21f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/e2e/conftest.py`:
- Around line 218-231: The non-xdist teardown in conftest leaves stale setup
state because it removes the database files but does not clear the
DB_BACKUP_DONE sentinel or the .locks artifacts. Update the teardown path that
handles the session DB cleanup to also delete DB_BACKUP_DONE and any lock files,
so the setup logic in the DB_BACKUP_DONE.exists() / shutil.copy2 block can
recreate a fresh database on the next local run.
- Around line 42-45: The e2e test setup still points all xdist workers at the
same sqlite file, which makes parallel runs interfere with each other. Update
the database setup in conftest.py to derive the test DB filename from the
current worker id and create a separate copy per worker, and make sure any
db_path/reset flow in tests/e2e/helpers/database_helpers.py uses that
worker-specific path instead of the shared TEST_DB_NAME. Keep the logic
centralized around the existing SQLALCHEMY_DATABASE_URI setup and reset_database
helper so each worker gets isolated fixtures.
- Around line 225-230: The WAL initialization in the SQLite setup block is
swallowing all exceptions, which hides failures in `sqlite3.connect`/`PRAGMA
journal_mode=WAL` and disables the intended lock-timeout behavior. Update the
setup around the `conn = sqlite3.connect(...)` and `conn.execute("PRAGMA
journal_mode=WAL;")` flow to fail fast or at minimum log/raise the exception
instead of using a bare `except Exception: pass`, so any WAL setup problem is
surfaced during test startup.
In `@tests/e2e/helpers/database_helpers.py`:
- Around line 348-353: The INSERT in database_helpers.py is writing time_stamp
with int(time.time()) instead of the shared timestamp helper, which violates the
Python timestamp contract. Update the comments insert path to use the
Unix-integer helper from utils/time.py rather than calling time.time() directly,
keeping the change localized to the database_helpers helper that builds the
comments row.
- Around line 89-90: Move the slow password hashing in the database helper
before the sqlite write lock is acquired: in the helper around
conn.execute("BEGIN IMMEDIATE") and cursor = conn.cursor(), compute the
sha512_crypt.hash() result first, then begin the transaction and write the row.
This keeps parallel workers from holding the write lock during CPU-bound work
and reduces busy-timeout failures.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 54339a55-b717-4c8a-946f-12a147a95d2f
📒 Files selected for processing (7)
.gitignoreapp/database.pyapp/routes/login.pyapp/routes/post.pytests/e2e/conftest.pytests/e2e/helpers/database_helpers.pytests/e2e/post/test_post.py
| # Force tests to use a temporary test database copy to keep git workspace clean | ||
| TEST_DB_NAME = "flaskblog_test.db" | ||
| os.environ["SQLALCHEMY_DATABASE_URI"] = f"sqlite:///{TEST_DB_NAME}" | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Use a per-worker test database.
TEST_DB_NAME/db_path still sends every xdist worker to the same sqlite file. Cross-file, reset_database() in tests/e2e/helpers/database_helpers.py wipes shared tables, so one worker can erase another worker's fixtures mid-test and parallel runs stay nondeterministic. Please derive the DB filename from the worker id and provision one copy per worker instead of one shared file.
Also applies to: 82-82
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/e2e/conftest.py` around lines 42 - 45, The e2e test setup still points
all xdist workers at the same sqlite file, which makes parallel runs interfere
with each other. Update the database setup in conftest.py to derive the test DB
filename from the current worker id and create a separate copy per worker, and
make sure any db_path/reset flow in tests/e2e/helpers/database_helpers.py uses
that worker-specific path instead of the shared TEST_DB_NAME. Keep the logic
centralized around the existing SQLALCHEMY_DATABASE_URI setup and reset_database
helper so each worker gets isolated fixtures.
| if not DB_BACKUP_DONE.exists(): | ||
| # First worker to acquire lock does the backup | ||
| backup_path = db_path.with_suffix(".db.bak") | ||
| if db_path.exists(): | ||
| shutil.copy2(db_path, backup_path) | ||
| # Copy original database to the test database file | ||
| if orig_db_path.exists(): | ||
| shutil.copy2(orig_db_path, db_path) | ||
| # Ensure the test database has WAL mode enabled once | ||
| import sqlite3 | ||
|
|
||
| try: | ||
| conn = sqlite3.connect(str(db_path)) | ||
| conn.execute("PRAGMA journal_mode=WAL;") | ||
| conn.close() | ||
| except Exception: | ||
| pass | ||
| DB_BACKUP_DONE.touch() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Non-xdist teardown leaves stale setup state behind.
This branch deletes the DB files but never clears the DB_BACKUP_DONE sentinel or .locks. On the next local run, Line 218 skips the copy step because the sentinel still exists, so the session can start from a missing or stale database.
Suggested fix
if not _is_xdist_worker(request.config):
for suffix in ["", "-wal", "-shm"]:
path = Path(str(db_path) + suffix)
path.unlink(missing_ok=True)
+ DB_BACKUP_DONE.unlink(missing_ok=True)
+ shutil.rmtree(LOCK_DIR, ignore_errors=True)Also applies to: 235-239
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/e2e/conftest.py` around lines 218 - 231, The non-xdist teardown in
conftest leaves stale setup state because it removes the database files but does
not clear the DB_BACKUP_DONE sentinel or the .locks artifacts. Update the
teardown path that handles the session DB cleanup to also delete DB_BACKUP_DONE
and any lock files, so the setup logic in the DB_BACKUP_DONE.exists() /
shutil.copy2 block can recreate a fresh database on the next local run.
| try: | ||
| conn = sqlite3.connect(str(db_path)) | ||
| conn.execute("PRAGMA journal_mode=WAL;") | ||
| conn.close() | ||
| except Exception: | ||
| pass |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Don't swallow WAL setup failures.
If PRAGMA journal_mode=WAL fails here, the suite silently falls back to default journaling and the lock-timeout fix is effectively disabled. This should fail fast or at least surface the error.
Suggested fix
- try:
- conn = sqlite3.connect(str(db_path))
- conn.execute("PRAGMA journal_mode=WAL;")
- conn.close()
- except Exception:
- pass
+ with sqlite3.connect(str(db_path)) as conn:
+ mode = conn.execute("PRAGMA journal_mode=WAL;").fetchone()[0]
+ if str(mode).lower() != "wal":
+ raise RuntimeError(
+ f"Failed to enable WAL for {db_path}: got {mode!r}"
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try: | |
| conn = sqlite3.connect(str(db_path)) | |
| conn.execute("PRAGMA journal_mode=WAL;") | |
| conn.close() | |
| except Exception: | |
| pass | |
| with sqlite3.connect(str(db_path)) as conn: | |
| mode = conn.execute("PRAGMA journal_mode=WAL;").fetchone()[0] | |
| if str(mode).lower() != "wal": | |
| raise RuntimeError( | |
| f"Failed to enable WAL for {db_path}: got {mode!r}" | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/e2e/conftest.py` around lines 225 - 230, The WAL initialization in the
SQLite setup block is swallowing all exceptions, which hides failures in
`sqlite3.connect`/`PRAGMA journal_mode=WAL` and disables the intended
lock-timeout behavior. Update the setup around the `conn = sqlite3.connect(...)`
and `conn.execute("PRAGMA journal_mode=WAL;")` flow to fail fast or at minimum
log/raise the exception instead of using a bare `except Exception: pass`, so any
WAL setup problem is surfaced during test startup.
| cursor.execute( | ||
| """ | ||
| INSERT INTO comments (post_id, comment, username, time_stamp) | ||
| VALUES (?, ?, ?, ?) | ||
| """, | ||
| (post_id, comment, username, int(time.time())), |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use utils/time.py for this timestamp write.
This stores time_stamp via int(time.time()) instead of the shared helper, which breaks the repository's timestamp contract for Python files. As per coding guidelines, **/*.py: Timestamps must be stored as Unix integers via utils/time.py.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/e2e/helpers/database_helpers.py` around lines 348 - 353, The INSERT in
database_helpers.py is writing time_stamp with int(time.time()) instead of the
shared timestamp helper, which violates the Python timestamp contract. Update
the comments insert path to use the Unix-integer helper from utils/time.py
rather than calling time.time() directly, keeping the change localized to the
database_helpers helper that builds the comments row.
Source: Coding guidelines
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/routes/post.py`:
- Line 84: The redirect after a successful comment submit should go directly to
the canonical post URL instead of the non-canonical url_id-only route. In the
post.post flow, use the already available post_slug together with url_id when
building the url_for target so the redirect lands on the final slugged URL
without an extra round trip.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a914d2b7-3a26-4a20-b6c2-8a6a43e4e63d
📒 Files selected for processing (7)
.gitignoreapp/database.pyapp/routes/login.pyapp/routes/post.pytests/e2e/conftest.pytests/e2e/helpers/database_helpers.pytests/e2e/post/test_post.py
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/e2e/post/test_post.py
- app/routes/login.py
- tests/e2e/conftest.py
- tests/e2e/helpers/database_helpers.py
- app/database.py
| ) | ||
|
|
||
| return redirect(url_for("post.post", url_id=url_id)), 301 | ||
| return redirect(url_for("post.post", url_id=url_id)) |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
Redirect straight to the canonical post URL.
Line 84 drops slug even though post_slug is already available, so every successful comment submit now incurs an extra /post/<url_id> → /post/<slug>-<url_id> round trip before the page reloads. This still works, but it adds avoidable latency on a flow this PR is trying to make less timeout-prone.
Suggested fix
- return redirect(url_for("post.post", url_id=url_id))
+ return redirect(url_for("post.post", url_id=url_id, slug=post_slug))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return redirect(url_for("post.post", url_id=url_id)) | |
| return redirect(url_for("post.post", url_id=url_id, slug=post_slug)) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/routes/post.py` at line 84, The redirect after a successful comment
submit should go directly to the canonical post URL instead of the non-canonical
url_id-only route. In the post.post flow, use the already available post_slug
together with url_id when building the url_for target so the redirect lands on
the final slugged URL without an extra round trip.
36c3718 to
5948467
Compare
DogukanUrker
force-pushed
the
fix/e2e-parallel-concurrency
branch
from
5948467 to
1ceb798
Compare
1 participant
This PR fixes database locking and transient timeouts in parallel E2E test execution.
Summary by CodeRabbit
New Features
Bug Fixes
Chores