From 189d814e1d2f74f54fa3b59ef386170cacb5e5d4 Mon Sep 17 00:00:00 2001
From: Hayden <154503486+groupthinking@users.noreply.github.com>
Date: Sun, 2 Aug 2026 12:23:55 -0500
Subject: [PATCH 1/3] perf: batch project scaffolding disk writes off the event
 loop

ProjectCodeGenerator performed every scaffolding filesystem call inline
inside async def bodies, so a blocked write parked the whole event loop
rather than just the requesting coroutine.

Move all 28 filesystem calls off the loop, batched into one
asyncio.to_thread hop per generator (O(1) hops instead of O(files)).
Content generation is pure in-memory string building and stays on the
loop; only the writes are offloaded.

Output is byte-for-byte identical, verified by loading the pre- and
post-change modules side by side and comparing SHA-256 digests of every
emitted file plus every returned dict across all three generators.

Refs #1250

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
---
 .../backend/code_generator.py                 | 108 ++++++-----
 tests/unit/test_code_generator.py             | 178 ++++++++++++++++++
 2 files changed, 242 insertions(+), 44 deletions(-)

diff --git a/src/youtube_extension/backend/code_generator.py b/src/youtube_extension/backend/code_generator.py
index 62f1640a9..51ecfe9f2 100644
--- a/src/youtube_extension/backend/code_generator.py
+++ b/src/youtube_extension/backend/code_generator.py
@@ -10,6 +10,7 @@
 template-based generation that still produces unique, video-specific output.
 """
 
+import asyncio
 import hashlib
 import json
 import logging
@@ -22,6 +23,34 @@
 logger = logging.getLogger(__name__)
 
 
+#: One scaffolding step. ``(path, None)`` creates a directory; ``(path, text)``
+#: writes a file. Steps are applied in list order.
+WritePlan = list[tuple[Path, Optional[str]]]
+
+
+def _apply_write_plan(plan: WritePlan) -> None:
+    """Apply an ordered scaffolding plan on the calling thread.
+
+    This is the only place the generators touch the filesystem. It is written
+    as a plain synchronous function so callers can hand the whole batch to
+    ``asyncio.to_thread`` in a single hop, rather than paying a hop per file.
+
+    Steps are applied strictly in order, so a directory step always lands
+    before the file steps that depend on it and the on-disk result matches
+    what the equivalent inline sequence produced.
+
+    No exception is suppressed. A failing step raises exactly the error the
+    equivalent inline call would have raised, on the same step, leaving the
+    preceding steps applied -- identical to the previous behaviour.
+    """
+    for path, content in plan:
+        if content is None:
+            path.mkdir(exist_ok=True)
+        else:
+            with open(path, "w") as handle:
+                handle.write(content)
+
+
 def _extract_video_id(video_url: str) -> Optional[str]:
     """Extract the YouTube video ID from a URL, returning None if not found."""
     if not video_url:
@@ -150,7 +179,9 @@ async def generate_project(self, video_analysis: dict[str, Any], project_config:
                 video_analysis["build_plan"] = build_plan
 
             # Create temporary project directory
-            temp_dir = tempfile.mkdtemp(prefix="uvai_project_")
+            temp_dir = await asyncio.to_thread(
+                tempfile.mkdtemp, prefix="uvai_project_"
+            )
             project_path = Path(temp_dir)
 
             # Generate project structure based on type
@@ -234,25 +265,13 @@ async def _generate_react_project(self, project_path: Path, video_analysis: dict
             package_json["dependencies"]["tailwindcss"] = "^3.3.0"
             package_json["devDependencies"] = {"autoprefixer": "^10.4.14", "postcss": "^8.4.24"}
 
-        # Write package.json
-        with open(project_path / "package.json", "w") as f:
-            json.dump(package_json, f, indent=2)
-
-        # Create src directory
         src_dir = project_path / "src"
-        src_dir.mkdir(exist_ok=True)
-
-        # Create public directory
         public_dir = project_path / "public"
-        public_dir.mkdir(exist_ok=True)
-
-        # Generate index.html
-        index_html = self._generate_index_html(title)
-        with open(public_dir / "index.html", "w") as f:
-            f.write(index_html)
 
-        # Generate main App component
+        # Build every artifact in memory first. This is pure string work and
+        # stays on the event loop; only the disk I/O below is offloaded.
         technologies = extracted_info.get("technologies", [])
+        index_html = self._generate_index_html(title)
         app_component = self._generate_react_app_component(
             title,
             technologies,
@@ -261,23 +280,23 @@ async def _generate_react_project(self, project_path: Path, video_analysis: dict
             summary,
             key_concepts
         )
-        with open(src_dir / "App.js", "w") as f:
-            f.write(app_component)
-
-        # Generate index.js
         index_js = self._generate_react_index_js()
-        with open(src_dir / "index.js", "w") as f:
-            f.write(index_js)
-
-        # Generate CSS
         app_css = self._generate_app_css(features)
-        with open(src_dir / "App.css", "w") as f:
-            f.write(app_css)
-
-        # Generate README
         readme = self._generate_readme(title, "React", video_analysis)
-        with open(project_path / "README.md", "w") as f:
-            f.write(readme)
+
+        # ``json.dumps`` produces exactly the bytes ``json.dump`` would have
+        # written for the same arguments; neither appends a trailing newline.
+        plan: WritePlan = [
+            (project_path / "package.json", json.dumps(package_json, indent=2)),
+            (src_dir, None),
+            (public_dir, None),
+            (public_dir / "index.html", index_html),
+            (src_dir / "App.js", app_component),
+            (src_dir / "index.js", index_js),
+            (src_dir / "App.css", app_css),
+            (project_path / "README.md", readme),
+        ]
+        await asyncio.to_thread(_apply_write_plan, plan)
 
         return {
             "framework": "react",
@@ -313,25 +332,25 @@ async def _generate_vanilla_js_project(self, project_path: Path, video_analysis:
             summary,
             key_concepts
         )
-        with open(project_path / "index.html", "w") as f:
-            f.write(index_html)
 
         # Generate main.js — NOW uses video-specific content
         main_js = self._generate_vanilla_main_js(
             title, tutorial_steps, features, key_concepts, fingerprint
         )
-        with open(project_path / "main.js", "w") as f:
-            f.write(main_js)
 
         # Generate styles.css — NOW uses video-derived accent color
         styles_css = self._generate_vanilla_styles_css(title, features, fingerprint)
-        with open(project_path / "styles.css", "w") as f:
-            f.write(styles_css)
 
         # Generate README
         readme = self._generate_readme(title, "Vanilla JavaScript", video_analysis)
-        with open(project_path / "README.md", "w") as f:
-            f.write(readme)
+
+        plan: WritePlan = [
+            (project_path / "index.html", index_html),
+            (project_path / "main.js", main_js),
+            (project_path / "styles.css", styles_css),
+            (project_path / "README.md", readme),
+        ]
+        await asyncio.to_thread(_apply_write_plan, plan)
 
         return {
             "framework": "vanilla",
@@ -369,18 +388,19 @@ async def _generate_python_api(self, project_path: Path, video_analysis: dict, f
             requirements.append("sqlalchemy==2.0.23")
         if "authentication" in features:
             requirements.append("python-jose[cryptography]==3.3.0")
-        with open(project_path / "requirements.txt", "w") as f:
-            f.write("\n".join(requirements))
 
         # Generate main.py
         main_py = self._generate_fastapi_main(title, features)
-        with open(project_path / "main.py", "w") as f:
-            f.write(main_py)
 
         # Generate README
         readme = self._generate_readme(title, "Python FastAPI", video_analysis)
-        with open(project_path / "README.md", "w") as f:
-            f.write(readme)
+
+        plan: WritePlan = [
+            (project_path / "requirements.txt", "\n".join(requirements)),
+            (project_path / "main.py", main_py),
+            (project_path / "README.md", readme),
+        ]
+        await asyncio.to_thread(_apply_write_plan, plan)
 
         return {
             "framework": "fastapi",
diff --git a/tests/unit/test_code_generator.py b/tests/unit/test_code_generator.py
index 62d8c3d60..f47a6fd72 100644
--- a/tests/unit/test_code_generator.py
+++ b/tests/unit/test_code_generator.py
@@ -5,12 +5,14 @@
 import asyncio
 import json
 import tempfile
+import threading
 from pathlib import Path
 
 import pytest
 
 from youtube_extension.backend.code_generator import (
     ProjectCodeGenerator,
+    _apply_write_plan,
     _build_title,
     _extract_video_id,
     get_code_generator,
@@ -800,3 +802,179 @@ def test_returns_same_instance_on_repeated_calls(self):
         gen2 = get_code_generator(use_ai_generation=False)
         assert gen1 is gen2
         cg_module._code_generator = None  # clean up
+
+
+# ===========================================================================
+# Scaffolding disk I/O runs off the event loop (issue #1250)
+# ===========================================================================
+
+
+class TestScaffoldingWritesOffLoop:
+    """Every filesystem call must land on a worker thread, not the loop.
+
+    These assert *thread identity* rather than elapsed time: a wall-clock
+    threshold would be flaky under CI contention and would still pass if the
+    work ran on the loop but happened to be fast.
+    """
+
+    @staticmethod
+    def _recording_open(record: list[str]):
+        """Wrap ``builtins.open`` so each call records its executing thread."""
+        import builtins
+
+        real_open = builtins.open
+
+        def _tracked(*args, **kwargs):
+            record.append(threading.current_thread().name)
+            return real_open(*args, **kwargs)
+
+        return _tracked
+
+    @pytest.mark.parametrize(
+        "generator_name",
+        [
+            "_generate_react_project",
+            "_generate_vanilla_js_project",
+            "_generate_python_api",
+        ],
+    )
+    async def test_generator_writes_never_touch_loop_thread(
+        self, monkeypatch, tmp_path, generator_name
+    ):
+        gen = ProjectCodeGenerator(use_ai_generation=False)
+        analysis = _build_video_analysis(
+            "Dashboard Tutorial", "auJzb1D-fag", "A summary.", ["state", "charts"]
+        )
+        loop_thread = threading.current_thread().name
+
+        seen: list[str] = []
+        monkeypatch.setattr("builtins.open", self._recording_open(seen))
+
+        project = tmp_path / "project"
+        project.mkdir()
+        await getattr(gen, generator_name)(project, analysis, ["database"])
+
+        assert seen, "expected the generator to write at least one file"
+        offenders = [name for name in seen if name == loop_thread]
+        assert not offenders, (
+            f"{generator_name} performed {len(offenders)} of {len(seen)} writes "
+            f"on the event loop thread ({loop_thread})"
+        )
+
+    async def test_mkdtemp_runs_off_loop(self, monkeypatch, tmp_path):
+        """The project directory itself is also created off-loop."""
+        import youtube_extension.backend.code_generator as cg_module
+
+        loop_thread = threading.current_thread().name
+        seen: list[str] = []
+        real_mkdtemp = tempfile.mkdtemp
+
+        def _tracked(*args, **kwargs):
+            seen.append(threading.current_thread().name)
+            return real_mkdtemp(*args, **kwargs)
+
+        monkeypatch.setattr(cg_module.tempfile, "mkdtemp", _tracked)
+
+        gen = ProjectCodeGenerator(use_ai_generation=False)
+        await gen.generate_project(
+            _build_video_analysis("T", "auJzb1D-fag", "S", []),
+            {"project_type": "web", "technologies": ["react"]},
+        )
+
+        assert seen, "expected mkdtemp to be called"
+        assert loop_thread not in seen, (
+            f"tempfile.mkdtemp ran on the event loop thread ({loop_thread})"
+        )
+
+    @pytest.mark.parametrize(
+        ("generator_name", "expected_files"),
+        [
+            ("_generate_react_project", 6),
+            ("_generate_vanilla_js_project", 4),
+            ("_generate_python_api", 3),
+        ],
+    )
+    async def test_batches_into_a_single_thread_hop(
+        self, monkeypatch, tmp_path, generator_name, expected_files
+    ):
+        """Cost is O(1) thread hops per generator, not O(files).
+
+        Guards against a regression that offloads each write individually,
+        which would still pass the thread-identity tests above while paying
+        one context switch per file.
+        """
+        import youtube_extension.backend.code_generator as cg_module
+
+        real_to_thread = asyncio.to_thread
+        hops: list[str] = []
+
+        async def _counting(func, /, *args, **kwargs):
+            hops.append(getattr(func, "__name__", repr(func)))
+            return await real_to_thread(func, *args, **kwargs)
+
+        monkeypatch.setattr(cg_module.asyncio, "to_thread", _counting)
+
+        gen = ProjectCodeGenerator(use_ai_generation=False)
+        project = tmp_path / "project"
+        project.mkdir()
+        await getattr(gen, generator_name)(
+            project,
+            _build_video_analysis("T", "auJzb1D-fag", "S", []),
+            ["database"],
+        )
+
+        assert hops == ["_apply_write_plan"], (
+            f"expected exactly one batched hop, got {hops}"
+        )
+        written = [p for p in project.rglob("*") if p.is_file()]
+        assert len(written) == expected_files
+
+
+class TestApplyWritePlan:
+    """Contract of the batched write helper itself."""
+
+    def test_applies_steps_in_order_so_dirs_precede_their_files(self, tmp_path):
+        nested = tmp_path / "src"
+        plan = [
+            (nested, None),
+            (nested / "App.js", "console.log(1);"),
+        ]
+        _apply_write_plan(plan)
+
+        assert nested.is_dir()
+        assert (nested / "App.js").read_text() == "console.log(1);"
+
+    def test_directory_step_tolerates_an_existing_directory(self, tmp_path):
+        existing = tmp_path / "public"
+        existing.mkdir()
+        _apply_write_plan([(existing, None)])  # must not raise
+        assert existing.is_dir()
+
+    def test_does_not_suppress_errors_and_leaves_earlier_steps_applied(
+        self, tmp_path
+    ):
+        """A failing step raises, exactly as the inline sequence did.
+
+        Uses a real invalid path rather than a mock so the stdlib itself
+        produces the failure.
+        """
+        good = tmp_path / "first.txt"
+        plan = [
+            (good, "written"),
+            (tmp_path / "bad\x00name.txt", "never"),
+            (tmp_path / "third.txt", "unreached"),
+        ]
+
+        with pytest.raises(ValueError, match="null"):
+            _apply_write_plan(plan)
+
+        assert good.read_text() == "written"
+        assert not (tmp_path / "third.txt").exists()
+
+    def test_writes_content_verbatim_without_adding_a_trailing_newline(
+        self, tmp_path
+    ):
+        target = tmp_path / "package.json"
+        payload = json.dumps({"name": "x", "version": "1.0.0"}, indent=2)
+        _apply_write_plan([(target, payload)])
+        assert target.read_bytes() == payload.encode()

From 42ed56f579fc9f7958ea732cd1ce2103cd9c84d5 Mon Sep 17 00:00:00 2001
From: Hayden <154503486+groupthinking@users.noreply.github.com>
Date: Sun, 2 Aug 2026 12:47:08 -0500
Subject: [PATCH 2/3] fix: keep scaffolding atomic under cancellation

Moving the project writes onto worker threads added suspension points the
inline sequence did not have, so a cancelled request could unwind while a
worker thread was still writing into a directory no caller would ever
receive.

Drain the scaffolding task through a shield loop before propagating the
cancellation, then remove the directory that generate_project created.
Mirrors _run_sync_rpc in services/cloud/cloud_tasks_queue.py.

CancelledError is never suppressed. The pre-existing generic-exception
leak is tracked separately in #1254.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
---
 .../backend/code_generator.py                 |  85 ++++++++++++--
 tests/unit/test_code_generator.py             | 108 ++++++++++++++++++
 2 files changed, 184 insertions(+), 9 deletions(-)

diff --git a/src/youtube_extension/backend/code_generator.py b/src/youtube_extension/backend/code_generator.py
index 51ecfe9f2..c8c83a136 100644
--- a/src/youtube_extension/backend/code_generator.py
+++ b/src/youtube_extension/backend/code_generator.py
@@ -11,10 +11,12 @@
 """
 
 import asyncio
+import contextlib
 import hashlib
 import json
 import logging
 import os
+import shutil
 import tempfile
 from pathlib import Path
 from typing import Any, Optional
@@ -51,6 +53,60 @@ def _apply_write_plan(plan: WritePlan) -> None:
                 handle.write(content)
 
 
+async def _run_to_completion(coro) -> tuple[Any, bool]:
+    """Run *coro* as a task that outlives cancellation of the awaiting frame.
+
+    ``asyncio.to_thread`` cannot stop a worker thread that has already started.
+    Returning from the ``await`` while that thread is still writing would leave
+    a live writer touching a directory no caller owns. Shielding the task and
+    draining it keeps the filesystem quiescent before cancellation propagates.
+    Mirrors ``_run_sync_rpc`` in ``services/cloud/cloud_tasks_queue.py``.
+
+    Returns ``(result, cancelled)``. When *cancelled* is true the caller MUST
+    re-raise ``CancelledError`` after releasing whatever it owns; ``result`` is
+    then whatever the task produced, or ``None`` if it failed.
+    """
+    task = asyncio.ensure_future(coro)
+    cancelled = False
+    while not task.done():
+        try:
+            await asyncio.shield(task)
+        except asyncio.CancelledError:
+            cancelled = True
+        except Exception:
+            if not cancelled:
+                raise
+    if cancelled:
+        result = None
+        with contextlib.suppress(Exception):
+            result = task.result()
+        return result, True
+    return task.result(), False
+
+
+async def _discard_project_dir(project_path: Path) -> None:
+    """Remove a scaffolding directory that no caller will ever receive.
+
+    ``generate_project`` creates the directory, so it owns it until it hands
+    the path back. If the request is cancelled first, nothing downstream can
+    ever learn the path, so it is removed here. Removal itself is drained to
+    completion for the same reason the writes are.
+
+    ``shutil.rmtree(..., ignore_errors=True)`` only swallows ``OSError``, so
+    the broader guard is deliberate: cleanup must never mask the cancellation
+    it is running underneath.
+    """
+
+    def _remove() -> None:
+        try:
+            shutil.rmtree(project_path, ignore_errors=True)
+        except Exception as exc:  # pragma: no cover - defensive
+            logger.warning(f"Could not discard {project_path}: {exc}")
+
+    with contextlib.suppress(Exception):
+        await _run_to_completion(asyncio.to_thread(_remove))
+
+
 def _extract_video_id(video_url: str) -> Optional[str]:
     """Extract the YouTube video ID from a URL, returning None if not found."""
     if not video_url:
@@ -178,21 +234,32 @@ async def generate_project(self, video_analysis: dict[str, Any], project_config:
             if build_plan:
                 video_analysis["build_plan"] = build_plan
 
-            # Create temporary project directory
-            temp_dir = await asyncio.to_thread(
-                tempfile.mkdtemp, prefix="uvai_project_"
+            # Create temporary project directory. Draining the worker means a
+            # cancelled request can still learn the path it must clean up.
+            temp_dir, cancelled = await _run_to_completion(
+                asyncio.to_thread(tempfile.mkdtemp, prefix="uvai_project_")
             )
+            if cancelled:
+                if temp_dir is not None:
+                    await _discard_project_dir(Path(temp_dir))
+                raise asyncio.CancelledError
             project_path = Path(temp_dir)
 
             # Generate project structure based on type
-            if project_type == "web":
-                result = await self._generate_web_project(project_path, video_analysis, technologies, features)
-            elif project_type == "api":
-                result = await self._generate_api_project(project_path, video_analysis, technologies, features)
+            if project_type == "api":
+                generation = self._generate_api_project(project_path, video_analysis, technologies, features)
             elif project_type == "mobile":
-                result = await self._generate_mobile_project(project_path, video_analysis, technologies, features)
+                generation = self._generate_mobile_project(project_path, video_analysis, technologies, features)
             else:
-                result = await self._generate_web_project(project_path, video_analysis, technologies, features)
+                generation = self._generate_web_project(project_path, video_analysis, technologies, features)
+
+            # Scaffolding is atomic with respect to cancellation, as it was
+            # before the writes moved off the loop. Draining first guarantees
+            # no worker is still writing when the directory is removed.
+            result, cancelled = await _run_to_completion(generation)
+            if cancelled:
+                await _discard_project_dir(project_path)
+                raise asyncio.CancelledError
 
             result["project_path"] = str(project_path)
             result["project_type"] = project_type
diff --git a/tests/unit/test_code_generator.py b/tests/unit/test_code_generator.py
index f47a6fd72..e4366ffb5 100644
--- a/tests/unit/test_code_generator.py
+++ b/tests/unit/test_code_generator.py
@@ -978,3 +978,111 @@ def test_writes_content_verbatim_without_adding_a_trailing_newline(
         payload = json.dumps({"name": "x", "version": "1.0.0"}, indent=2)
         _apply_write_plan([(target, payload)])
         assert target.read_bytes() == payload.encode()
+
+
+class TestCancellationSafety:
+    """Cancelling a scaffolding request must not strand a directory.
+
+    Moving the writes onto worker threads introduced suspension points that the
+    inline sequence did not have, so cancellation can now land mid-scaffold.
+    A worker thread cannot be cancelled once it has started, so the request has
+    to drain it before removing the directory it alone knows about.
+    """
+
+    @staticmethod
+    def _analysis_and_config():
+        return (
+            {
+                "title": "T",
+                "description": "d",
+                "key_concepts": [],
+                "technologies": ["react"],
+            },
+            {"project_type": "web", "technologies": ["react"], "features": []},
+        )
+
+    async def _cancel_mid_scaffold(self, monkeypatch, tmp_path):
+        """Cancel while a real worker thread is inside the write hop."""
+        monkeypatch.setattr(tempfile, "tempdir", str(tmp_path))
+        from youtube_extension.backend import code_generator as cg
+
+        started = threading.Event()
+        release = threading.Event()
+        finished = threading.Event()
+        real_apply = cg._apply_write_plan
+
+        def blocking_apply(plan):
+            started.set()
+            assert release.wait(timeout=10), "probe deadlocked"
+            real_apply(plan)
+            finished.set()
+
+        monkeypatch.setattr(cg, "_apply_write_plan", blocking_apply)
+
+        analysis, config = self._analysis_and_config()
+        task = asyncio.create_task(
+            ProjectCodeGenerator(use_ai_generation=False).generate_project(
+                analysis, config
+            )
+        )
+        for _ in range(500):
+            if started.is_set():
+                break
+            await asyncio.sleep(0.01)
+        assert started.is_set(), "never reached the write hop"
+
+        task.cancel()
+        for _ in range(5):
+            await asyncio.sleep(0)
+        release.set()
+        return task, finished
+
+    async def test_cancellation_removes_the_project_directory(
+        self, monkeypatch, tmp_path
+    ):
+        task, _ = await self._cancel_mid_scaffold(monkeypatch, tmp_path)
+        with pytest.raises(asyncio.CancelledError):
+            await task
+
+        assert list(tmp_path.glob("uvai_project_*")) == []
+
+    async def test_cancellation_drains_the_writer_before_unwinding(
+        self, monkeypatch, tmp_path
+    ):
+        """The directory is only safe to remove once no thread is writing."""
+        task, finished = await self._cancel_mid_scaffold(monkeypatch, tmp_path)
+        with pytest.raises(asyncio.CancelledError):
+            await task
+
+        assert finished.is_set(), "unwound while a worker was still writing"
+
+    async def test_cancellation_is_reported_as_cancellation(
+        self, monkeypatch, tmp_path
+    ):
+        """CancelledError must never be downgraded to a normal failure."""
+        task, _ = await self._cancel_mid_scaffold(monkeypatch, tmp_path)
+        with pytest.raises(asyncio.CancelledError):
+            await task
+
+        assert task.cancelled()
+
+    async def test_uncancelled_web_request_still_returns_a_project(
+        self, monkeypatch, tmp_path
+    ):
+        """Guards the dispatch: an explicit 'web' type behaves like the default."""
+        monkeypatch.setattr(tempfile, "tempdir", str(tmp_path))
+        analysis, config = self._analysis_and_config()
+        generator = ProjectCodeGenerator(use_ai_generation=False)
+
+        explicit = await generator.generate_project(analysis, dict(config))
+        defaulted = await generator.generate_project(
+            analysis, {**config, "project_type": "unrecognised-type"}
+        )
+
+        def tree(result):
+            root = Path(result["project_path"])
+            return sorted(str(p.relative_to(root)) for p in root.rglob("*"))
+
+        assert explicit["project_type"] == "web"
+        assert Path(explicit["project_path"]).is_dir()
+        assert tree(explicit) == tree(defaulted) != []

From d178e210a89c4c05343c4b4e748696fd7352cbe1 Mon Sep 17 00:00:00 2001
From: Hayden <154503486+groupthinking@users.noreply.github.com>
Date: Sun, 2 Aug 2026 13:01:23 -0500
Subject: [PATCH 3/3] perf: remove scaffolding directory when generation fails

generate_project is the only holder of the scaffold path until it
returns, so an exception on the way out left a directory that no caller
could name, let alone remove. Wrap the dispatch-to-return region in
try/except Exception and discard the directory before re-raising.

CancelledError derives from BaseException, so the explicit cancellation
path is unaffected and cannot double-remove. _discard_project_dir
suppresses everything and drains its worker, so cleanup can never mask
the original exception.

Also close a coverage gap the Copilot reviewer found: the off-loop test
recorded only open(), so moving Path.mkdir back onto the loop still
passed. It now hooks Path.mkdir as well, and a dedicated test covers the
one generator that creates subdirectories.

Resolves #1253, #1254.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
---
 .../backend/code_generator.py                 |  61 ++++----
 tests/unit/test_code_generator.py             | 139 +++++++++++++++++-
 2 files changed, 170 insertions(+), 30 deletions(-)

diff --git a/src/youtube_extension/backend/code_generator.py b/src/youtube_extension/backend/code_generator.py
index c8c83a136..501cfb763 100644
--- a/src/youtube_extension/backend/code_generator.py
+++ b/src/youtube_extension/backend/code_generator.py
@@ -245,33 +245,42 @@ async def generate_project(self, video_analysis: dict[str, Any], project_config:
                 raise asyncio.CancelledError
             project_path = Path(temp_dir)
 
-            # Generate project structure based on type
-            if project_type == "api":
-                generation = self._generate_api_project(project_path, video_analysis, technologies, features)
-            elif project_type == "mobile":
-                generation = self._generate_mobile_project(project_path, video_analysis, technologies, features)
-            else:
-                generation = self._generate_web_project(project_path, video_analysis, technologies, features)
-
-            # Scaffolding is atomic with respect to cancellation, as it was
-            # before the writes moved off the loop. Draining first guarantees
-            # no worker is still writing when the directory is removed.
-            result, cancelled = await _run_to_completion(generation)
-            if cancelled:
+            # From here the path is known, so every exit that does not hand it
+            # back is responsible for removing it.
+            try:
+                # Generate project structure based on type
+                if project_type == "api":
+                    generation = self._generate_api_project(project_path, video_analysis, technologies, features)
+                elif project_type == "mobile":
+                    generation = self._generate_mobile_project(project_path, video_analysis, technologies, features)
+                else:
+                    generation = self._generate_web_project(project_path, video_analysis, technologies, features)
+
+                # Scaffolding is atomic with respect to cancellation, as it was
+                # before the writes moved off the loop. Draining first guarantees
+                # no worker is still writing when the directory is removed.
+                result, cancelled = await _run_to_completion(generation)
+                if cancelled:
+                    await _discard_project_dir(project_path)
+                    raise asyncio.CancelledError
+
+                result["project_path"] = str(project_path)
+                result["project_type"] = project_type
+                result["technologies"] = technologies
+                result["features"] = features
+                # Include the structured BuildPlan artifact in the result so that
+                # callers (e.g. API endpoints and tests) can inspect it.
+                if build_plan is not None:
+                    result["build_plan"] = build_plan
+
+                logger.info(f"✅ Project generated successfully at {project_path}")
+                return result
+            except Exception:
+                # A failed generation never returns the path, so nothing
+                # downstream can clean it up. The writers are already drained
+                # by ``_run_to_completion``, so removal cannot race one.
                 await _discard_project_dir(project_path)
-                raise asyncio.CancelledError
-
-            result["project_path"] = str(project_path)
-            result["project_type"] = project_type
-            result["technologies"] = technologies
-            result["features"] = features
-            # Include the structured BuildPlan artifact in the result so that
-            # callers (e.g. API endpoints and tests) can inspect it.
-            if build_plan is not None:
-                result["build_plan"] = build_plan
-
-            logger.info(f"✅ Project generated successfully at {project_path}")
-            return result
+                raise
 
         except Exception as e:
             logger.error(f"❌ Project generation failed: {e}")
diff --git a/tests/unit/test_code_generator.py b/tests/unit/test_code_generator.py
index e4366ffb5..d5e2e9ee8 100644
--- a/tests/unit/test_code_generator.py
+++ b/tests/unit/test_code_generator.py
@@ -830,6 +830,22 @@ def _tracked(*args, **kwargs):
 
         return _tracked
 
+    @staticmethod
+    def _recording_mkdir(record: list[str]):
+        """Record the thread that ran each ``Path.mkdir``.
+
+        Directory creation is part of the write plan, so an implementation that
+        moved only ``open`` off the loop would still be a regression. Hooking
+        ``mkdir`` closes that gap.
+        """
+        real_mkdir = Path.mkdir
+
+        def _tracked(self, *args, **kwargs):
+            record.append(threading.current_thread().name)
+            return real_mkdir(self, *args, **kwargs)
+
+        return _tracked
+
     @pytest.mark.parametrize(
         "generator_name",
         [
@@ -848,17 +864,52 @@ async def test_generator_writes_never_touch_loop_thread(
         loop_thread = threading.current_thread().name
 
         seen: list[str] = []
-        monkeypatch.setattr("builtins.open", self._recording_open(seen))
+        mkdirs: list[str] = []
 
         project = tmp_path / "project"
         project.mkdir()
+
+        # Patch only after the test root exists, otherwise setup trips the hook.
+        monkeypatch.setattr("builtins.open", self._recording_open(seen))
+        monkeypatch.setattr(Path, "mkdir", self._recording_mkdir(mkdirs))
+
         await getattr(gen, generator_name)(project, analysis, ["database"])
 
         assert seen, "expected the generator to write at least one file"
-        offenders = [name for name in seen if name == loop_thread]
+        offenders = [name for name in seen + mkdirs if name == loop_thread]
         assert not offenders, (
-            f"{generator_name} performed {len(offenders)} of {len(seen)} writes "
-            f"on the event loop thread ({loop_thread})"
+            f"{generator_name} performed {len(offenders)} of "
+            f"{len(seen) + len(mkdirs)} filesystem operations on the event "
+            f"loop thread ({loop_thread})"
+        )
+
+    async def test_directory_creation_never_touches_loop_thread(
+        self, monkeypatch, tmp_path
+    ):
+        """Proves the ``Path.mkdir`` hook above is live, not vacuous.
+
+        ``_generate_react_project`` is the generator that creates
+        subdirectories, so it is the one that can demonstrate the hook fires.
+        Without this the sibling test could pass while every ``mkdir`` still
+        ran on the loop.
+        """
+        gen = ProjectCodeGenerator(use_ai_generation=False)
+        analysis = _build_video_analysis(
+            "Dashboard Tutorial", "auJzb1D-fag", "A summary.", ["state"]
+        )
+        loop_thread = threading.current_thread().name
+
+        project = tmp_path / "project"
+        project.mkdir()
+
+        mkdirs: list[str] = []
+        monkeypatch.setattr(Path, "mkdir", self._recording_mkdir(mkdirs))
+        await gen._generate_react_project(project, analysis, ["database"])
+
+        assert mkdirs, "expected the react generator to create subdirectories"
+        assert loop_thread not in mkdirs, (
+            f"{len([n for n in mkdirs if n == loop_thread])} of {len(mkdirs)} "
+            f"mkdir calls ran on the event loop thread ({loop_thread})"
         )
 
     async def test_mkdtemp_runs_off_loop(self, monkeypatch, tmp_path):
@@ -1086,3 +1137,83 @@ def tree(result):
         assert explicit["project_type"] == "web"
         assert Path(explicit["project_path"]).is_dir()
         assert tree(explicit) == tree(defaulted) != []
+
+
+class TestFailedGenerationCleanup:
+    """A generation that raises must not strand the directory it created.
+
+    ``generate_project`` is the only holder of the scaffold path until it
+    returns, so an exception on the way out leaves a directory that no caller
+    can ever name, let alone remove. The caller in
+    ``video_processing_service.py`` reads ``project_path`` only on the success
+    path, so nothing downstream is deprived by removing it here.
+    """
+
+    @staticmethod
+    def _analysis_and_config():
+        return (
+            {
+                "title": "T",
+                "description": "d",
+                "key_concepts": [],
+                "technologies": ["react"],
+            },
+            {"project_type": "web", "technologies": ["react"], "features": []},
+        )
+
+    async def test_failed_generation_leaves_no_orphan_directory(
+        self, monkeypatch, tmp_path
+    ):
+        monkeypatch.setattr(tempfile, "tempdir", str(tmp_path))
+        from youtube_extension.backend import code_generator as cg
+
+        def boom(plan):
+            raise RuntimeError("disk exploded")
+
+        monkeypatch.setattr(cg, "_apply_write_plan", boom)
+
+        analysis, config = self._analysis_and_config()
+        with pytest.raises(RuntimeError, match="disk exploded"):
+            await ProjectCodeGenerator(use_ai_generation=False).generate_project(
+                analysis, config
+            )
+
+        assert list(tmp_path.glob("uvai_project_*")) == [], (
+            "a failed generation stranded its scaffolding directory"
+        )
+
+    async def test_original_exception_is_not_masked_by_cleanup(
+        self, monkeypatch, tmp_path
+    ):
+        """Cleanup failure must never replace the error that caused it."""
+        monkeypatch.setattr(tempfile, "tempdir", str(tmp_path))
+        from youtube_extension.backend import code_generator as cg
+
+        def boom(plan):
+            raise RuntimeError("original failure")
+
+        def exploding_rmtree(*args, **kwargs):
+            raise OSError("cleanup also failed")
+
+        monkeypatch.setattr(cg, "_apply_write_plan", boom)
+        monkeypatch.setattr(cg.shutil, "rmtree", exploding_rmtree)
+
+        analysis, config = self._analysis_and_config()
+        with pytest.raises(RuntimeError, match="original failure"):
+            await ProjectCodeGenerator(use_ai_generation=False).generate_project(
+                analysis, config
+            )
+
+    async def test_successful_generation_keeps_its_directory(
+        self, monkeypatch, tmp_path
+    ):
+        """Guards against the cleanup firing on the happy path."""
+        monkeypatch.setattr(tempfile, "tempdir", str(tmp_path))
+
+        analysis, config = self._analysis_and_config()
+        result = await ProjectCodeGenerator(use_ai_generation=False).generate_project(
+            analysis, config
+        )
+
+        assert Path(result["project_path"]).is_dir()
+        assert len(list(tmp_path.glob("uvai_project_*"))) == 1