fix: resolve e2e bugs — double POST, SSE disconnect, orphan containers, PR link

- Prevent StrictMode double container creation with AbortController
- Poll backend status on SSE disconnect instead of marking as failed
- Add periodic container cleanup reaper task (every 5min)
- Fix PR comment link to match frontend route format
- Fix pre-existing ruff E402 (import os placement in service.py)

Author: mariuspruvot

apps/api/src/helprs/main.py

@@ -22,6 +22,7 @@
 _REPLAY_DISCOVERY_TIMEOUT_SECONDS = 10.0
 _REPLAY_CONCURRENCY = 10
 _REAPER_INTERVAL_SECONDS = 300
+_CONTAINER_CLEANUP_INTERVAL_SECONDS = 300
 
 
 async def _replay_pending_webhook_events(app: FastAPI) -> None:
@@ -100,6 +101,35 @@ async def _run_webhook_reaper(app: FastAPI, *, interval_seconds: int = _REAPER_I
         raise
 
 
+async def _run_container_cleanup(
+    app: FastAPI,
+    *,
+    interval_seconds: int = _CONTAINER_CLEANUP_INTERVAL_SECONDS,
+) -> None:
+    """Periodic cleanup of expired container sessions.
+
+    Finds sessions past their TTL and destroys their Docker containers.
+    Cancelled cleanly by the lifespan teardown via ``task.cancel()``.
+    """
+    from helprs.modules.container.service import AioDockerClient, cleanup_expired
+
+    try:
+        while True:
+            await asyncio.sleep(interval_seconds)
+            try:
+                docker = AioDockerClient()
+                async with app.state.session_factory() as db:
+                    cleaned = await cleanup_expired(db, docker)
+                    await db.commit()
+                    if cleaned:
+                        logger.info("container_cleanup_cycle", cleaned=cleaned)
+            except Exception:
+                logger.exception("container_cleanup_cycle_failed")
+    except asyncio.CancelledError:
+        logger.info("container_cleanup_stopped")
+        raise
+
+
 def create_app() -> FastAPI:
     settings = get_settings()
 
@@ -112,6 +142,7 @@ async def lifespan(app: FastAPI):
         """Manage database engine lifecycle, admin setup, and webhook reaper."""
         engine = create_engine()
         reaper_task: asyncio.Task | None = None
+        cleanup_task: asyncio.Task | None = None
         try:
             session_factory = create_session_factory(engine)
             app.state.engine = engine
@@ -137,9 +168,15 @@ async def lifespan(app: FastAPI):
             # mark_processed commit failure) without waiting for the next
             # restart.
             reaper_task = asyncio.create_task(_run_webhook_reaper(app))
+            cleanup_task = asyncio.create_task(_run_container_cleanup(app))
 
             yield
         finally:
+            if cleanup_task is not None:
+                cleanup_task.cancel()
+                with contextlib.suppress(asyncio.CancelledError, Exception):
+                    await cleanup_task
+
             if reaper_task is not None:
                 reaper_task.cancel()
                 with contextlib.suppress(asyncio.CancelledError, Exception):

apps/api/src/helprs/modules/container/service.py

@@ -13,6 +13,7 @@
 import asyncio
 import contextlib
 import json
+import os
 from datetime import UTC, datetime
 from pathlib import Path
 from typing import TYPE_CHECKING, Protocol
@@ -46,8 +47,6 @@
 # Host path for skills — needed for Docker-in-Docker volume mounts.
 # The API container can't use its own /app/skills path as a bind mount source
 # for child containers; Docker needs the HOST filesystem path.
-import os
-
 SKILLS_HOST_PATH = os.environ.get("SKILLS_HOST_PATH", str(SKILLS_BASE_PATH))
 
 
@@ -316,10 +315,12 @@ async def send_message(
     if cs.status != ContainerStatus.RUNNING or not cs.container_id:
         raise ExternalServiceError("Container is not running")
 
-    message = json.dumps({
-        "type": "user",
-        "message": {"role": "user", "content": content},
-    })
+    message = json.dumps(
+        {
+            "type": "user",
+            "message": {"role": "user", "content": content},
+        }
+    )
 
     try:
         await docker.write_to_container(cs.container_id, message)

apps/api/src/helprs/modules/webhook/handlers.py

@@ -129,7 +129,7 @@ async def handle_pull_request_opened(payload: dict, session: AsyncSession) -> No
         comment_body = (
             f"**helPRs** session created for this PR.\n\n"
             f"Skill: `challenge-me` | "
-            f"[Open session]({settings.APP_BASE_URL}/session/{cs.id})"
+            f"[Open session]({settings.APP_BASE_URL}/session/{installation.id}/{repo_full_name}/{pr_number})"
         )
         await post_pr_comment_with_retry(
             owner=owner,

apps/web/src/features/session/ContainerSession.test.tsx

@@ -129,12 +129,15 @@ describe('ContainerSession', () => {
     expect(screen.getAllByText(/challenge-me/).length).toBeGreaterThan(0)
 
     await waitFor(() => {
-      expect(mockedCreate).toHaveBeenCalledWith({
-        installation_id: defaultProps.installationId,
-        pr_number: 42,
-        repo_full_name: 'acme/helprs',
-        skill_name: 'challenge-me',
-      })
+      expect(mockedCreate).toHaveBeenCalledWith(
+        {
+          installation_id: defaultProps.installationId,
+          pr_number: 42,
+          repo_full_name: 'acme/helprs',
+          skill_name: 'challenge-me',
+        },
+        expect.any(AbortSignal),
+      )
     })
   })
 

apps/web/src/features/session/ContainerSession.tsx

@@ -12,6 +12,7 @@ import { useAuthStore } from '../auth/store'
 import {
   buildStreamUrl,
   createContainerSession,
+  getContainerSession,
   sendMessage,
   stopContainerSession,
 } from './containerApi'
@@ -68,21 +69,24 @@ export default function ContainerSession({
   // Create session on mount
   useEffect(() => {
     mountedRef.current = true
-    let cancelled = false
+    const abortController = new AbortController()
 
     async function init() {
       try {
         setStatus('starting')
         appendLine(`Starting ${skillName} for ${repoFullName}#${prNumber}...`)
 
-        const created = await createContainerSession({
-          installation_id: installationId,
-          pr_number: prNumber,
-          repo_full_name: repoFullName,
-          skill_name: skillName,
-        })
+        const created = await createContainerSession(
+          {
+            installation_id: installationId,
+            pr_number: prNumber,
+            repo_full_name: repoFullName,
+            skill_name: skillName,
+          },
+          abortController.signal,
+        )
 
-        if (cancelled) return
+        if (abortController.signal.aborted) return
 
         setSession(created)
         setStatus(created.status)
@@ -94,14 +98,36 @@ export default function ContainerSession({
           appendLine('[error] Container failed to start.')
         }
       } catch (err) {
-        if (cancelled) return
+        if (abortController.signal.aborted) return
         const msg = err instanceof Error ? err.message : 'Unknown error'
         setError(msg)
         setStatus('failed')
         appendLine(`[error] ${msg}`)
       }
     }
 
+    async function pollSessionStatus(sessionId: string) {
+      try {
+        const fresh = await getContainerSession(sessionId)
+        if (!mountedRef.current) return
+        setStatus(fresh.status)
+        if (fresh.status === 'completed') {
+          appendLine('Session completed.')
+        } else if (fresh.status === 'failed') {
+          setError('Container failed')
+          appendLine('[error] Container failed.')
+        } else if (fresh.status === 'running') {
+          appendLine('[reconnecting] Stream dropped, container still running...')
+          connectStream(sessionId)
+        }
+      } catch {
+        if (!mountedRef.current) return
+        setStatus('failed')
+        setError('Lost connection to session')
+        appendLine('[error] Lost connection to session.')
+      }
+    }
+
     function connectStream(sessionId: string) {
       const accessToken = useAuthStore.getState().accessToken
       if (!accessToken) {
@@ -176,19 +202,19 @@ export default function ContainerSession({
           return
         }
 
-        // Native error — connection dropped
+        // Native error — connection dropped. Poll backend for actual status
+        // instead of immediately marking as failed.
         if (source.readyState === EventSource.CLOSED) {
           eventSourceRef.current = null
-          // Only mark as failed if we haven't already completed
-          setStatus((prev) => (prev === 'completed' ? prev : 'failed'))
+          pollSessionStatus(sessionId)
         }
       })
     }
 
     init()
 
     return () => {
-      cancelled = true
+      abortController.abort()
       mountedRef.current = false
       if (eventSourceRef.current) {
         eventSourceRef.current.close()

apps/web/src/features/session/containerApi.ts

@@ -21,11 +21,13 @@ export class ContainerSessionError extends Error {
 
 export async function createContainerSession(
   body: ContainerSessionRequest,
+  signal?: AbortSignal,
 ): Promise<ContainerSessionResponse> {
   const resp = await apiFetch('/api/v1/containers/sessions', {
     method: 'POST',
     headers: { 'Content-Type': 'application/json' },
     body: JSON.stringify(body),
+    signal,
   })
   if (!resp.ok) {
     throw new ContainerSessionError(resp.status, `Failed to create session: ${resp.status}`)