Skip to content

perf: verify_project blocks the event loop for up to 7 minutes on three subprocess.run calls #1239

Description

@groupthinking

Problem

DeploymentManager.verify_project() is declared async def but runs three blocking
subprocess.run() calls directly on the event loop. Each carries a long timeout, so a
single request can freeze the entire process — and every other concurrent request with it.

Method Line Blocking call Timeout
verify_project 149 subprocess.run([npm, "install", ...]) 180 s
verify_project 174 subprocess.run([npm, "run", "build"]) 180 s
verify_project 230 subprocess.run([npx, "tsc", "--noEmit"]) 60 s

Worst case is 420 s (7 minutes) of total event-loop blockage per call. Because
retry_verification() invokes verify_project() inside a for attempt in range(max_retries + 1)
loop (deployment_manager.py:286-289), that ceiling is multiplied by the retry count.

While the loop is blocked the process cannot service health checks, other API requests, or
even cancel the in-flight request.

Reachability evidence

Unlike the library-surface fixes in #1227 and #1232, this defect sits on a fully reachable
production request path
, verified at call level (not merely import level):

POST /api/v1/video-to-software        router.py:777   (router mounted at main.py:192)
  -> video_to_software_v1             router.py:783
  -> process_video_to_software        video_processing_service.py:317
  -> deploy_project                   video_processing_service.py:389
  -> verify_project                   deployment_manager.py:287
  -> subprocess.run x3                deployment_manager.py:149, 174, 230

An import closure from youtube_extension.main (60 modules) confirms
youtube_extension.backend.deployment_manager is reachable.

Acceptance criteria

  • All three subprocess.run() calls in verify_project() run off the event loop.
  • The existing timeout= values and subprocess.TimeoutExpired behaviour are preserved.
  • capture_output=True / text=True result handling (returncode, stdout, stderr)
    is unchanged.
  • Regression tests assert the subprocess call executes on a non-event-loop thread,
    using thread identity rather than wall-clock timing.
  • No new dependencies.

Proposed fix

Wrap each call with asyncio.to_thread(...), matching the pattern already merged in
#1194, #1203, #1205, #1228 and #1233. import asyncio is already present at module
level (deployment_manager.py:12), so no import change is required.

install_result = await asyncio.to_thread(
    subprocess.run,
    [npm_path, "install", "--legacy-peer-deps", "--ignore-scripts"],
    cwd=str(resolved_path),
    capture_output=True,
    text=True,
    timeout=180,
)

Note on timeouts (contrast with #1234): subprocess.run's own timeout= argument keeps
working inside the worker thread and kills the child process, so the worker is genuinely
released. This is unlike the unbounded filesystem reads in #1234, where a thread blocked in
the kernel cannot be reclaimed. No asyncio.wait_for wrapper is needed or wanted here.

Note on the file header

deployment_manager.py begins with # LOCKED FILE: SYSTEM AGENT ONLY - DO NOT EDIT MANUALLY.
This appears to be a stale advisory rather than an enforced gate:

Flagging explicitly so a reviewer can object if the lock is meant to be honoured.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions