From cc969aa187c407671ef93a716a9108a437c333fe Mon Sep 17 00:00:00 2001 From: retoor Date: Thu, 3 Sep 2026 08:16:17 +0000 Subject: [PATCH] Fix a tunnel-hostname race that could publish duplicate rows tunnels.create() checked for an existing row by hostname and then inserted/updated in a separate statement, with no lock between the two. Concurrent publish calls for the same port (e.g. the editor's Ports extension re-POSTing on every onDidChangeTunnels event) could race past the check together and each insert their own row for the identical hostname. Since routers/tunnel.py resolves a tunnel by a plain find_one(hostname=...) with no ordering, whichever duplicate it happened to return decided whether a request reached the app or got a generic "no tunnel is published at this address" 404 - even while a sibling row for the same hostname was active and serving traffic. Replaced the check-then-write with a single atomic INSERT ... ON CONFLICT(hostname) DO UPDATE, which needs hostname to actually be unique: idx_tunnels_hostname is now a UNIQUE index instead of a plain one. Since existing databases likely already carry duplicate rows from this race, init_db() now runs a one-time _dedupe_tunnel_hostnames() pass (keeps the best row per hostname - active > provisioning > pending > failed, then newest) before dropping and recreating the index as unique - CREATE UNIQUE INDEX IF NOT EXISTS silently no-ops when an index with that name already exists, so the old non-unique index has to be dropped first or the constraint would never actually upgrade. Co-Authored-By: Claude Sonnet 5 --- devplacepy/database/schema.py | 52 ++++++++++- .../services/containers/workspace/tunnels.py | 88 +++++++++---------- 2 files changed, 94 insertions(+), 46 deletions(-) diff --git a/devplacepy/database/schema.py b/devplacepy/database/schema.py index 8c9db8c..8aa464c 100644 --- a/devplacepy/database/schema.py +++ b/devplacepy/database/schema.py @@ -30,6 +30,54 @@ def migrate_bug_tables_to_issue_tables() -> None: logger.info("Dropped table %s after migration", source_name) +_TUNNEL_STATUS_RANK = { + "active": 4, + "provisioning": 3, + "pending": 2, + "failed": 1, + "suspended": 0, +} + + +def _dedupe_tunnel_hostnames() -> None: + if "tunnels" not in db.tables: + return + table = get_table("tunnels") + if not table.has_column("hostname") or not table.has_column("uid"): + return + groups = list( + db.query( + "SELECT hostname FROM tunnels " + "WHERE hostname IS NOT NULL AND hostname != '' " + "GROUP BY hostname HAVING COUNT(*) > 1" + ) + ) + for group in groups: + hostname = group["hostname"] + dupes = list(table.find(hostname=hostname)) + dupes.sort( + key=lambda r: ( + _TUNNEL_STATUS_RANK.get(r.get("status") or "", -1), + r.get("deleted_at") is None, + r.get("created_at") or "", + r.get("id") or 0, + ), + reverse=True, + ) + losers = [r["uid"] for r in dupes[1:]] + if not losers: + continue + with db: + for uid in losers: + db.query("DELETE FROM tunnels WHERE uid = :uid", uid=uid) + logger.warning( + "Removed %d duplicate tunnel row(s) for hostname %s, kept %s", + len(losers), + hostname, + dupes[0]["uid"], + ) + + def init_db(): tables = db.tables _index(db, "users", "idx_users_username", ["username"]) @@ -785,7 +833,9 @@ def init_db(): _index(db, "instances", "idx_instances_workspace", ["is_workspace", "status"]) _index(db, "instances", "idx_instances_workspace_owner", ["workspace_owner_uid"]) _index(db, "instances", "idx_instances_tunnel_name", ["tunnel_name"]) - _index(db, "tunnels", "idx_tunnels_hostname", ["hostname"]) + _dedupe_tunnel_hostnames() + _drop_index(db, "idx_tunnels_hostname") + _index(db, "tunnels", "idx_tunnels_hostname", ["hostname"], unique=True) _index(db, "tunnels", "idx_tunnels_instance", ["instance_uid"]) _index(db, "tunnels", "idx_tunnels_user", ["user_uid"]) _index(db, "tunnels", "idx_tunnels_state", ["desired_state", "status"]) diff --git a/devplacepy/services/containers/workspace/tunnels.py b/devplacepy/services/containers/workspace/tunnels.py index 2d97b71..b9a10c1 100644 --- a/devplacepy/services/containers/workspace/tunnels.py +++ b/devplacepy/services/containers/workspace/tunnels.py @@ -69,52 +69,50 @@ def create( return None hostname = naming.hostname_for(name, container_port) table = _table() - revived = table.find_one(hostname=hostname) stamp = _now() - if revived: - changes = { - "uid": revived["uid"], - "instance_uid": instance["uid"], - "project_uid": instance.get("project_uid", ""), - "user_uid": user_uid, - "label": label or f"port {container_port}", - "container_port": container_port, - "desired_state": "present", - "updated_at": stamp, - "deleted_at": None, - "deleted_by": None, - } - if not keeps_certificate(revived): - changes["status"] = STATUS_PENDING - changes["last_error"] = "" - table.update(changes, ["uid"]) - return table.find_one(uid=revived["uid"]) - uid = generate_uid() - table.insert( - { - "uid": uid, - "instance_uid": instance["uid"], - "project_uid": instance.get("project_uid", ""), - "user_uid": user_uid, - "hostname": hostname, - "label": label or f"port {container_port}", - "container_port": container_port, - "desired_state": "present", - "status": STATUS_PENDING, - "cert_status": "", - "cert_checked_at": "", - "request_count": 0, - "bytes_out": 0, - "last_request_at": "", - "last_error": "", - "last_synced_at": "", - "created_at": stamp, - "updated_at": stamp, - "deleted_at": None, - "deleted_by": None, - } - ) - return table.find_one(uid=uid) + with db: + db.query( + """ + INSERT INTO tunnels ( + uid, instance_uid, project_uid, user_uid, hostname, label, + container_port, desired_state, status, cert_status, + cert_checked_at, request_count, bytes_out, last_request_at, + last_error, last_synced_at, created_at, updated_at, + deleted_at, deleted_by + ) VALUES ( + :uid, :instance_uid, :project_uid, :user_uid, :hostname, :label, + :container_port, 'present', :status_pending, '', '', 0, 0, '', '', + '', :stamp, :stamp, NULL, NULL + ) + ON CONFLICT(hostname) DO UPDATE SET + instance_uid = excluded.instance_uid, + project_uid = excluded.project_uid, + user_uid = excluded.user_uid, + label = excluded.label, + container_port = excluded.container_port, + desired_state = 'present', + updated_at = excluded.updated_at, + deleted_at = NULL, + deleted_by = NULL, + status = CASE + WHEN tunnels.deleted_at IS NULL AND tunnels.status = :status_active + THEN tunnels.status ELSE :status_pending END, + last_error = CASE + WHEN tunnels.deleted_at IS NULL AND tunnels.status = :status_active + THEN tunnels.last_error ELSE '' END + """, + uid=generate_uid(), + instance_uid=instance["uid"], + project_uid=instance.get("project_uid", ""), + user_uid=user_uid, + hostname=hostname, + label=label or f"port {container_port}", + container_port=container_port, + stamp=stamp, + status_pending=STATUS_PENDING, + status_active=STATUS_ACTIVE, + ) + return table.find_one(hostname=hostname) def awaiting_certificate() -> list[dict]: