From cb33787c0827cde47c4a7d1cd4272759c5ff2722 Mon Sep 17 00:00:00 2001 From: chihlasm Date: Wed, 1 Apr 2026 05:09:42 +0000 Subject: [PATCH] fix: close race conditions in script builder session and slug creation - script_builder endpoint: pg_advisory_xact_lock on user_id before session count check, preventing concurrent creates from both passing the MAX_SESSIONS_PER_USER guard - script_builder_service send_message: pg_advisory_xact_lock on session_id before message count check, preventing concurrent sends from both passing the MAX_MESSAGES_PER_SESSION guard - script_builder_service save_to_library: replace check-then-insert slug logic with IntegrityError retry loop (3 attempts with fresh UUID suffix); add unique constraint on script_templates.slug (migration 070) - ScriptBuilderPage: add creatingSessionRef to serialize concurrent handleSend calls that would otherwise both call createSession() while session is still null Co-Authored-By: Claude Sonnet 4.6 --- ..._unique_constraint_script_template_slug.py | 29 +++++++ backend/app/api/endpoints/script_builder.py | 7 ++ backend/app/models/script_template.py | 2 +- .../app/services/script_builder_service.py | 79 ++++++++++++------- frontend/src/pages/ScriptBuilderPage.tsx | 22 +++++- 5 files changed, 105 insertions(+), 34 deletions(-) create mode 100644 backend/alembic/versions/070_add_unique_constraint_script_template_slug.py diff --git a/backend/alembic/versions/070_add_unique_constraint_script_template_slug.py b/backend/alembic/versions/070_add_unique_constraint_script_template_slug.py new file mode 100644 index 00000000..2fad3b09 --- /dev/null +++ b/backend/alembic/versions/070_add_unique_constraint_script_template_slug.py @@ -0,0 +1,29 @@ +"""add unique constraint to script_templates.slug + +Revision ID: 070 +Revises: 069 +Create Date: 2026-04-01 +""" +from alembic import op + + +revision = "070" +down_revision = "069" +branch_labels = None +depends_on = None + + +def upgrade() -> None: + op.create_unique_constraint( + "uq_script_templates_slug", + "script_templates", + ["slug"], + ) + + +def downgrade() -> None: + op.drop_constraint( + "uq_script_templates_slug", + "script_templates", + type_="unique", + ) diff --git a/backend/app/api/endpoints/script_builder.py b/backend/app/api/endpoints/script_builder.py index b328477c..f56c595c 100644 --- a/backend/app/api/endpoints/script_builder.py +++ b/backend/app/api/endpoints/script_builder.py @@ -3,6 +3,7 @@ from typing import Annotated from uuid import UUID from fastapi import APIRouter, Depends, HTTPException, Request +from sqlalchemy import text from sqlalchemy.ext.asyncio import AsyncSession from app.core.database import get_db @@ -67,6 +68,12 @@ async def create_session( current_user: Annotated[User, Depends(get_current_active_user)], ) -> ScriptBuilderSessionDetail: """Start a new Script Builder session.""" + # Acquire per-user advisory lock so concurrent create requests are serialized. + # Without this, two simultaneous requests both read count < limit and both + # insert, exceeding MAX_SESSIONS_PER_USER. + user_lock_key = hash(str(current_user.id)) % (2**62) + await db.execute(text("SELECT pg_advisory_xact_lock(:key)"), {"key": user_lock_key}) + # Enforce max concurrent sessions count = await script_builder_service.count_user_sessions(db, current_user.id) if count >= MAX_SESSIONS_PER_USER: diff --git a/backend/app/models/script_template.py b/backend/app/models/script_template.py index 0ea71ea8..838d2f3c 100644 --- a/backend/app/models/script_template.py +++ b/backend/app/models/script_template.py @@ -48,7 +48,7 @@ class ScriptTemplate(Base): UUID(as_uuid=True), ForeignKey("users.id", ondelete="SET NULL"), nullable=True ) name: Mapped[str] = mapped_column(String(200), nullable=False) - slug: Mapped[str] = mapped_column(String(200), nullable=False, index=True) + slug: Mapped[str] = mapped_column(String(200), nullable=False, unique=True, index=True) description: Mapped[Optional[str]] = mapped_column(Text, nullable=True) use_case: Mapped[Optional[str]] = mapped_column(Text, nullable=True) script_body: Mapped[str] = mapped_column(Text, nullable=False) diff --git a/backend/app/services/script_builder_service.py b/backend/app/services/script_builder_service.py index b483f39e..a1a7647e 100644 --- a/backend/app/services/script_builder_service.py +++ b/backend/app/services/script_builder_service.py @@ -5,7 +5,8 @@ from datetime import datetime, timezone from typing import Optional from uuid import UUID -from sqlalchemy import select, func +from sqlalchemy import select, func, text +from sqlalchemy.exc import IntegrityError from sqlalchemy.ext.asyncio import AsyncSession from sqlalchemy.orm import selectinload @@ -169,6 +170,12 @@ async def send_message( user_content: str, ) -> ScriptBuilderMessageResponse: """Send a user message and get AI response with generated script.""" + # Acquire per-session advisory lock to prevent concurrent message count races. + # Two simultaneous sends to the same session would otherwise both read the same + # count, both pass the limit check, and both insert — exceeding the cap. + session_lock_key = hash(str(session.id)) % (2**62) + await db.execute(text("SELECT pg_advisory_xact_lock(:key)"), {"key": session_lock_key}) + # Count existing messages for the session msg_count_result = await db.execute( select(func.count(ScriptBuilderMessage.id)).where( @@ -344,36 +351,48 @@ async def save_to_library( raise ValueError("Default 'AI Generated' category not found. Run migrations.") resolved_category_id = default_cat - # Generate unique slug + # Generate slug. Use a UUID suffix on first attempt to prevent concurrent + # saves with the same name from hitting the unique constraint on slug. base_slug = name.lower().replace(" ", "-").replace("_", "-")[:80] base_slug = re.sub(r"[^a-z0-9\-]", "", base_slug) - slug = base_slug - # Check uniqueness - existing = await db.execute( - select(ScriptTemplate.id).where(ScriptTemplate.slug == slug) - ) - if existing.scalar_one_or_none(): - slug = f"{base_slug}-{uuid_mod.uuid4().hex[:6]}" - template = ScriptTemplate( - id=uuid_mod.uuid4(), - category_id=resolved_category_id, - created_by=user_id, - team_id=team_id if share_with_team else None, - name=name, - slug=slug, - description=description, - script_body=script_body or session.latest_script, - parameters_schema=parameters_schema or {"parameters": []}, - default_values={}, - validation_rules={}, - tags=[session.language, "ai-generated"], - complexity="intermediate", - is_verified=False, - is_active=True, - version=1, - usage_count=0, + # Check if the base slug is already taken; if not, use it clean (no suffix). + # If taken, or if the insert races with a concurrent request, retry with a + # fresh UUID suffix. The unique constraint on script_templates.slug is the + # authoritative guard — the application check just avoids unnecessary retries. + existing = await db.execute( + select(ScriptTemplate.id).where(ScriptTemplate.slug == base_slug) ) - db.add(template) - await db.flush() - return template + slug = base_slug if not existing.scalar_one_or_none() else f"{base_slug}-{uuid_mod.uuid4().hex[:6]}" + + for attempt in range(3): + template = ScriptTemplate( + id=uuid_mod.uuid4(), + category_id=resolved_category_id, + created_by=user_id, + team_id=team_id if share_with_team else None, + name=name, + slug=slug, + description=description, + script_body=script_body or session.latest_script, + parameters_schema=parameters_schema or {"parameters": []}, + default_values={}, + validation_rules={}, + tags=[session.language, "ai-generated"], + complexity="intermediate", + is_verified=False, + is_active=True, + version=1, + usage_count=0, + ) + db.add(template) + try: + await db.flush() + return template + except IntegrityError as exc: + if "uq_script_templates_slug" not in str(exc.orig) or attempt == 2: + raise + await db.rollback() + slug = f"{base_slug}-{uuid_mod.uuid4().hex[:8]}" + + raise RuntimeError("Failed to generate a unique slug after 3 attempts") diff --git a/frontend/src/pages/ScriptBuilderPage.tsx b/frontend/src/pages/ScriptBuilderPage.tsx index 078210a9..9ea47059 100644 --- a/frontend/src/pages/ScriptBuilderPage.tsx +++ b/frontend/src/pages/ScriptBuilderPage.tsx @@ -1,4 +1,4 @@ -import { useState, useEffect } from 'react' +import { useState, useEffect, useRef } from 'react' import { useSearchParams } from 'react-router-dom' import { Terminal } from 'lucide-react' import { cn } from '@/lib/utils' @@ -21,6 +21,11 @@ export default function ScriptBuilderPage() { const [messages, setMessages] = useState([]) const [language, setLanguage] = useState('powershell') const [isLoading, setIsLoading] = useState(false) + // Ref-based lock: prevents two concurrent handleSend calls (e.g. FlowPilot + // handoff useEffect + user keystroke) from each calling createSession() and + // creating two orphaned sessions. React state updates are async so isLoading + // alone can't guard across two calls in the same render cycle. + const creatingSessionRef = useRef(false) const [previewScript, setPreviewScript] = useState<{ script: string; filename: string | null } | null>(null) const [showSaveDialog, setShowSaveDialog] = useState(false) const [handoffProcessed, setHandoffProcessed] = useState(false) @@ -75,8 +80,19 @@ export default function ScriptBuilderPage() { // Create session if needed let currentSession = session if (!currentSession) { - currentSession = await scriptBuilderApi.createSession(effectiveLanguage) - setSession(currentSession) + if (creatingSessionRef.current) { + // Another concurrent call is already creating the session; drop this send. + setIsLoading(false) + setMessages((prev) => prev.slice(0, -1)) + return + } + creatingSessionRef.current = true + try { + currentSession = await scriptBuilderApi.createSession(effectiveLanguage) + setSession(currentSession) + } finally { + creatingSessionRef.current = false + } } // Send message