From b5f9bf14df04a5e7ae1c6fc9d507a542ae4bf6be Mon Sep 17 00:00:00 2001
From: Jason Staack <jason@staack.com>
Date: Sat, 14 Mar 2026 16:42:17 -0500
Subject: [PATCH] fix(vpn): commit before sync_wireguard_config to ensure data
 visibility

sync_wireguard_config opens its own AdminAsyncSessionLocal connection
which cannot see uncommitted data from the caller's transaction. Add
_commit_and_sync helper that commits first, then regenerates wg0.conf.

Also removes the unused db parameter from sync_wireguard_config.
---
 backend/app/routers/tenants.py                |  6 ++---
 backend/app/services/vpn_service.py           | 24 ++++++++++++++-----
 .../tests/integration/test_vpn_isolation.py   | 15 ++++++------
 3 files changed, 27 insertions(+), 18 deletions(-)

diff --git a/backend/app/routers/tenants.py b/backend/app/routers/tenants.py
index 7ad5961..4532513 100644
--- a/backend/app/routers/tenants.py
+++ b/backend/app/routers/tenants.py
@@ -232,13 +232,11 @@ async def delete_tenant(
     had_vpn = await get_vpn_config(db, tenant_id)
 
     await db.delete(tenant)
-    await db.flush()
+    await db.commit()
 
     # Regenerate wg0.conf without deleted tenant's peers
     if had_vpn:
-        await sync_wireguard_config(db)
-
-    await db.commit()
+        await sync_wireguard_config()
 
 
 # ---------------------------------------------------------------------------
diff --git a/backend/app/services/vpn_service.py b/backend/app/services/vpn_service.py
index 35349ee..71dd462 100644
--- a/backend/app/services/vpn_service.py
+++ b/backend/app/services/vpn_service.py
@@ -151,10 +151,22 @@ def _get_wg_config_path() -> Path:
     return Path(os.getenv("WIREGUARD_CONFIG_PATH", "/data/wireguard"))
 
 
-async def sync_wireguard_config(db: AsyncSession) -> None:
+async def _commit_and_sync(db: AsyncSession) -> None:
+    """Commit the caller's transaction then regenerate wg0.conf.
+
+    sync_wireguard_config opens its own DB session, so callers must commit
+    first for their changes to be visible. This helper combines both steps
+    and provides a single patch point for tests.
+    """
+    await _commit_and_sync(db)
+
+
+async def sync_wireguard_config() -> None:
     """Regenerate wg0.conf with ALL tenants' peers and write to shared volume.
 
     Uses AdminAsyncSessionLocal to bypass RLS (must see all tenants).
+    Callers MUST commit their transaction before calling this function,
+    since it opens a separate DB session that cannot see uncommitted data.
     Uses a PostgreSQL advisory lock to prevent concurrent writes.
     Writes atomically via temp file + rename.
     """
@@ -323,7 +335,7 @@ async def setup_vpn(
     logger.info("vpn_subnet_allocated", event="vpn_audit",
                 tenant_id=str(tenant_id), subnet_index=subnet_index, subnet=subnet)
 
-    await sync_wireguard_config(db)
+    await _commit_and_sync(db)
     return config
 
 
@@ -341,7 +353,7 @@ async def update_vpn_config(
         config.is_enabled = is_enabled
 
     await db.flush()
-    await sync_wireguard_config(db)
+    await _commit_and_sync(db)
     return config
 
 
@@ -410,7 +422,7 @@ async def add_peer(db: AsyncSession, tenant_id: uuid.UUID, device_id: uuid.UUID,
     db.add(peer)
     await db.flush()
 
-    await sync_wireguard_config(db)
+    await _commit_and_sync(db)
     return peer
 
 
@@ -425,7 +437,7 @@ async def remove_peer(db: AsyncSession, tenant_id: uuid.UUID, peer_id: uuid.UUID
 
     await db.delete(peer)
     await db.flush()
-    await sync_wireguard_config(db)
+    await _commit_and_sync(db)
 
 
 async def get_peer_config(db: AsyncSession, tenant_id: uuid.UUID, peer_id: uuid.UUID) -> dict:
@@ -523,7 +535,7 @@ async def onboard_device(
     db.add(peer)
     await db.flush()
 
-    await sync_wireguard_config(db)
+    await _commit_and_sync(db)
 
     # Generate RouterOS commands
     endpoint = config.endpoint or "YOUR_SERVER_IP:51820"
diff --git a/backend/tests/integration/test_vpn_isolation.py b/backend/tests/integration/test_vpn_isolation.py
index f213169..14e5025 100644
--- a/backend/tests/integration/test_vpn_isolation.py
+++ b/backend/tests/integration/test_vpn_isolation.py
@@ -36,16 +36,15 @@ def wireguard_tmp_dir(tmp_path):
 
 
 @pytest.fixture(autouse=True)
-def _no_sync_wireguard():
-    """Patch sync_wireguard_config to a no-op in service calls.
+def _no_commit_and_sync():
+    """Patch _commit_and_sync to a no-op in service calls.
 
-    sync_wireguard_config opens its own AdminAsyncSessionLocal connection,
-    which cannot see uncommitted test-transaction data. We patch it globally
-    so setup_vpn / add_peer / remove_peer don't fail, and then call the
-    real function explicitly in tests that need to verify wg0.conf content
-    (those tests commit data first or use a dedicated helper).
+    _commit_and_sync commits the transaction then opens a separate DB session
+    to regenerate wg0.conf. In tests, committing breaks transaction rollback
+    isolation, and the separate session can't see test data. Patching this
+    single function prevents both issues.
     """
-    with patch("app.services.vpn_service.sync_wireguard_config", new_callable=AsyncMock):
+    with patch("app.services.vpn_service._commit_and_sync", new_callable=AsyncMock):
         yield