Skip to content

fix(test): resolve parallel E2E database locks and timeouts #271

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
DogukanUrker merged 1 commit into from
Jun 27, 2026
Merged

fix(test): resolve parallel E2E database locks and timeouts #271

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,3 +175,13 @@ tests/e2e/.locks/

# .DS_Store files
.DS_Store

# Test databases
*_test.db
*_test.db-shm
*_test.db-wal

# SQLite temporary journal/WAL files
*.db-shm
*.db-wal
*.db-journal
29 changes: 28 additions & 1 deletion app/database.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
from flask_sqlalchemy import SQLAlchemy
from passlib.hash import sha512_crypt as encryption
from sqlalchemy import event
from sqlalchemy.engine import Engine
import sqlite3

from settings import Settings
from utils.log import Log
Expand All @@ -8,15 +11,29 @@
db = SQLAlchemy()


@event.listens_for(Engine, "connect")
def set_sqlite_pragma(dbapi_connection, connection_record):
if isinstance(dbapi_connection, sqlite3.Connection):
cursor = dbapi_connection.cursor()
cursor.execute("PRAGMA busy_timeout=30000;")
cursor.close()


def init_db(app):
app.config["SQLALCHEMY_DATABASE_URI"] = Settings.SQLALCHEMY_DATABASE_URI
app.config["SQLALCHEMY_TRACK_MODIFICATIONS"] = (
Settings.SQLALCHEMY_TRACK_MODIFICATIONS
)
if "sqlite" in Settings.SQLALCHEMY_DATABASE_URI:
app.config["SQLALCHEMY_ENGINE_OPTIONS"] = {"connect_args": {"timeout": 30}}

db.init_app(app)

with app.app_context():
# Set journal mode to WAL on the database once at startup
if "sqlite" in Settings.SQLALCHEMY_DATABASE_URI:
with db.engine.connect() as connection:
connection.exec_driver_sql("PRAGMA journal_mode=WAL;")
db.create_all()
Log.success("Database tables created/verified")
_create_default_admin()
Expand All @@ -33,7 +50,17 @@ def _create_default_admin():
).first()

if existing_admin:
Log.info(f'Admin: "{Settings.DEFAULT_ADMIN_USERNAME}" already exists')
# Update the password hash if it doesn't match the current environment setting
if not encryption.verify(
Settings.DEFAULT_ADMIN_PASSWORD, existing_admin.password
):
existing_admin.password = encryption.hash(Settings.DEFAULT_ADMIN_PASSWORD)
db.session.commit()
Log.success(
"Admin password updated in database to match DEFAULT_ADMIN_PASSWORD"
)
else:
Log.info(f'Admin: "{Settings.DEFAULT_ADMIN_USERNAME}" already exists')
return

admin = User(
Expand Down
15 changes: 3 additions & 12 deletions app/routes/login.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,10 +43,7 @@ def login(direct):
if Settings.LOG_IN:
if "username" in session:
Log.error(f'User: "{session["username"]}" already logged in')
return (
redirect(direct),
301,
)
return redirect(direct)
else:
form = LoginForm(request.form)
if request.method == "POST":
Expand Down Expand Up @@ -79,10 +76,7 @@ def login(direct):
language=session.get("language", "en"),
)

return (
redirect(direct),
301,
)
return redirect(direct)

else:
Log.error("Wrong password")
Expand All @@ -99,7 +93,4 @@ def login(direct):
hide_login=True,
)
else:
return (
redirect(direct),
301,
)
return redirect(direct)
6 changes: 2 additions & 4 deletions app/routes/post.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,9 +55,7 @@ def post(url_id=None, slug=None):

if "comment_delete_button" in request.form:
delete_comment(request.form["comment_id"], session.get("username"))
return redirect(
url_for("post.post", url_id=url_id, slug=post_slug)
), 301
return redirect(url_for("post.post", url_id=url_id, slug=post_slug))

comment_text = escape(request.form["comment"])

Expand All @@ -83,7 +81,7 @@ def post(url_id=None, slug=None):
language=session.get("language", "en"),
)

return redirect(url_for("post.post", url_id=url_id)), 301
return redirect(url_for("post.post", url_id=url_id))

@coderabbitai coderabbitai Bot Jun 27, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🚀 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.

Suggested change
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.


comments = (
Comment.query.filter_by(post_id=post.id)
Expand Down
91 changes: 65 additions & 26 deletions tests/e2e/conftest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,36 @@
from filelock import FileLock

import pytest
from playwright.sync_api import sync_playwright
from playwright.sync_api import sync_playwright, Page, Frame

# Add app directory to path for imports
APP_DIR = Path(__file__).parent.parent.parent / "app"
sys.path.insert(0, str(APP_DIR))

# Monkey-patch Playwright wait_for_url to resiliently upgrade any strict 5s timeouts to 10s under parallel test load
_orig_page_wait_for_url = Page.wait_for_url
_orig_frame_wait_for_url = Frame.wait_for_url


def patched_page_wait_for_url(self, url, *args, **kwargs):
if "timeout" in kwargs and kwargs["timeout"] <= 5000:
kwargs["timeout"] = 10000
return _orig_page_wait_for_url(self, url, *args, **kwargs)


def patched_frame_wait_for_url(self, url, *args, **kwargs):
if "timeout" in kwargs and kwargs["timeout"] <= 5000:
kwargs["timeout"] = 10000
return _orig_frame_wait_for_url(self, url, *args, **kwargs)


Page.wait_for_url = patched_page_wait_for_url
Frame.wait_for_url = patched_frame_wait_for_url

# 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}"

Comment on lines +42 to +45

@coderabbitai coderabbitai Bot Jun 27, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🩺 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.

# Shared state file paths for parallel execution
LOCK_DIR = Path(__file__).parent / ".locks"
SERVER_LOCK = LOCK_DIR / "server.lock"
Expand Down Expand Up @@ -55,7 +79,7 @@ def app_dir():
@pytest.fixture(scope="session")
def db_path(app_dir):
"""Return the database file path."""
return app_dir / "instance" / "flaskblog.db"
return app_dir / "instance" / "flaskblog_test.db"


@pytest.fixture(scope="session")
Expand Down Expand Up @@ -166,6 +190,14 @@ def context(browser_instance, flask_server):
viewport={"width": 1280, "height": 720},
base_url=flask_server["base_url"],
)

# Abort image/favicon and Dicebear API requests to speed up E2E tests and avoid external CDN/API hangs
context.route("**/*.{png,jpg,jpeg,gif,svg,ico}*", lambda route: route.abort())
context.route(
lambda url: "dicebear.com" in url or "githubusercontent.com" in url,
lambda route: route.abort(),
)

yield context
context.close()

Expand All @@ -179,32 +211,40 @@ def page(context):


@pytest.fixture(scope="session", autouse=True)
def backup_and_restore_db(request, db_path):
def setup_and_teardown_test_db(request, db_path, app_dir):
"""
Session-scoped fixture that backs up the database before tests.
Session-scoped fixture that sets up the temporary test database copy.

With pytest-xdist, coordinates to ensure only one backup happens.
Restore is handled by pytest_sessionfinish after ALL workers complete.
With pytest-xdist, coordinates to ensure only one copy setup happens.
Cleanup is handled by pytest_sessionfinish after ALL workers complete.
"""
LOCK_DIR.mkdir(exist_ok=True)
orig_db_path = app_dir / "instance" / "flaskblog.db"

# Use file lock to coordinate backup across workers
# Use file lock to coordinate setup across workers
with FileLock(str(DB_BACKUP_LOCK)):
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
Comment on lines +233 to +238

@coderabbitai coderabbitai Bot Jun 27, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🩺 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.

Suggested change
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.

DB_BACKUP_DONE.touch()
Comment on lines 226 to 239

@coderabbitai coderabbitai Bot Jun 27, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🩺 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.


yield

# In non-parallel mode, restore immediately
# In non-parallel mode, clean up immediately
if not _is_xdist_worker(request.config):
backup_path = db_path.with_suffix(".db.bak")
if backup_path.exists():
shutil.copy2(backup_path, db_path)
backup_path.unlink(missing_ok=True)
for suffix in ["", "-wal", "-shm"]:
path = Path(str(db_path) + suffix)
path.unlink(missing_ok=True)


def pytest_sessionfinish(session, exitstatus):
Expand All @@ -213,20 +253,19 @@ def pytest_sessionfinish(session, exitstatus):
In xdist mode, this runs on the master after all workers finish.
"""
if _is_xdist_master(session.config):
# Restore database from backup
db_path = APP_DIR / "instance" / "flaskblog.db"
backup_path = db_path.with_suffix(".db.bak")
if backup_path.exists():
shutil.copy2(backup_path, db_path)
backup_path.unlink(missing_ok=True)
# Delete temporary test database files
db_path = APP_DIR / "instance" / "flaskblog_test.db"
for suffix in ["", "-wal", "-shm"]:
path = Path(str(db_path) + suffix)
path.unlink(missing_ok=True)

# Clean up lock directory
if LOCK_DIR.exists():
shutil.rmtree(LOCK_DIR, ignore_errors=True)


@pytest.fixture(scope="session")
def clean_db(db_path):
@pytest.fixture(scope="session", autouse=True)
def clean_db(db_path, setup_and_teardown_test_db):
"""
Session-scoped fixture that resets database once at the start.
Removes test users from previous runs but keeps the admin.
Expand Down Expand Up @@ -260,8 +299,8 @@ def logged_in_page(page, flask_server, app_settings):
app_settings["default_admin"]["password"],
)

# Wait for redirect after login
page.wait_for_url("**/", timeout=5000)
# Wait for redirect after login (using resilient 10s timeout)
page.wait_for_url("**/", timeout=10000)

yield page

Expand Down
Loading