Merge pull request #17 from depoll/feature/edit-pool-attributes

depoll · web-flow · commit e75294a61cb5 · 2026-01-04T11:46:50.000-08:00
feat: clean up runners when pool is deleted + bulk runner management
diff --git a/backend/src/routes/pools.ts b/backend/src/routes/pools.ts
@@ -5,10 +5,12 @@
 
 import { Router, Request, Response } from 'express';
 import { v4 as uuidv4 } from 'uuid';
-import { db, type RunnerPoolRow, type CredentialRow } from '../db/index.js';
+import { db, type RunnerPoolRow, type CredentialRow, type RunnerRow } from '../db/index.js';
 import { decrypt, generateSecret } from '../utils/index.js';
 import { createGitHubClient, type GitHubScope } from '../services/index.js';
 import { ensureWarmRunners, getPoolById as getPoolRowById } from '../services/autoscaler.js';
+import { removeRunner, stopOrphanedRunner, cleanupRunnerFiles } from '../services/runnerManager.js';
+import { removeDockerRunner } from '../services/dockerRunner.js';
 
 export const poolsRouter = Router();
 
@@ -91,6 +93,8 @@ const updatePoolDockerOptions = db.prepare(`
 
 const deletePoolById = db.prepare('DELETE FROM runner_pools WHERE id = ?');
 
+const deleteRunnerById = db.prepare('DELETE FROM runners WHERE id = ?');
+
 const getCredentialById = db.prepare('SELECT * FROM credentials WHERE id = ?');
 
 const getPoolRunners = db.prepare(`
@@ -335,7 +339,7 @@ poolsRouter.patch('/:id', async (req: Request, res: Response) => {
 });
 
 /**
- * Delete a pool
+ * Delete a pool and all its runners
  */
 poolsRouter.delete('/:id', async (req: Request, res: Response) => {
   try {
@@ -346,9 +350,54 @@ poolsRouter.delete('/:id', async (req: Request, res: Response) => {
       return;
     }
     
-    // Note: Runners belonging to this pool will have pool_id set to NULL
-    // They won't be deleted automatically
+    // Get all runners belonging to this pool and clean them up
+    // Note: There's a small race window where a new runner could be added between this query
+    // and pool deletion. Such runners would have pool_id set to NULL by ON DELETE SET NULL.
+    // The UI's orphan detection feature can help identify and clean up such cases if they occur.
+    const poolRunners = getPoolRunners.all(req.params.id) as RunnerRow[];
+    
+    console.log(`[pools] Deleting pool ${req.params.id} with ${poolRunners.length} runners`);
+    
+    // Clean up each runner in parallel (fire-and-forget pattern for speed)
+    const cleanupPromises = poolRunners.map(async (runner) => {
+      try {
+        console.log(`[pools] Cleaning up runner ${runner.name} (${runner.id})`);
+        
+        if (runner.isolation_type === 'docker') {
+          await removeDockerRunner(runner.id).catch((err) => {
+            console.error(`[pools] Failed to remove Docker runner ${runner.id}:`, err);
+          });
+        } else {
+          // For native runners, try the full removal first
+          // This handles deregistration from GitHub and cleanup
+          await removeRunner(runner.id).catch(async (err) => {
+            // If removeRunner fails (e.g., runner already gone), do manual cleanup
+            console.error(`[pools] removeRunner failed for ${runner.id}, doing manual cleanup:`, err);
+            await stopOrphanedRunner(runner.id, runner.process_id).catch(() => {});
+            await cleanupRunnerFiles(runner.runner_dir).catch(() => {});
+          });
+        }
+        
+        // Delete the database record
+        deleteRunnerById.run(runner.id);
+        console.log(`[pools] Cleaned up runner ${runner.name}`);
+      } catch (err) {
+        console.error(`[pools] Failed to cleanup runner ${runner.name}:`, err);
+        // Still try to delete the database record
+        try {
+          deleteRunnerById.run(runner.id);
+        } catch (deleteErr) {
+          console.error(`[pools] Failed to delete runner record ${runner.id} during cleanup:`, deleteErr);
+        }
+      }
+    });
+    
+    // Wait for all cleanup to complete (with a timeout to avoid hanging)
+    await Promise.allSettled(cleanupPromises);
+    
+    // Now delete the pool
     deletePoolById.run(req.params.id);
+    console.log(`[pools] Deleted pool ${req.params.id}`);
     
     res.status(204).send();
   } catch (error) {
diff --git a/backend/tests/pools.test.ts b/backend/tests/pools.test.ts
@@ -195,5 +195,50 @@ describe('Pools API', () => {
       const getResponse = await request(app).get(`/api/pools/${id}`);
       expect(getResponse.status).toBe(404);
     });
+
+    it('should clean up associated runners when deleting a pool', async () => {
+      // Create a pool first
+      const createResponse = await request(app)
+        .post('/api/pools')
+        .send({
+          name: 'test-pool-with-runners',
+          credentialId: credentialId,
+        });
+      
+      const poolId = createResponse.body.pool.id;
+      
+      // Insert test runners directly into the database (simulating runners that exist)
+      // Note: These are simplified test runners without runner_dir or process_id.
+      // The cleanup code handles this gracefully - removeRunner will fail but the
+      // fallback cleanup and database deletion will still succeed.
+      const runnerId1 = 'test-runner-1-' + Date.now();
+      const runnerId2 = 'test-runner-2-' + Date.now();
+      
+      db.prepare(`
+        INSERT INTO runners (id, name, status, pool_id, credential_id, isolation_type, ephemeral, platform, architecture, labels)
+        VALUES (?, ?, 'offline', ?, ?, 'native', 0, 'linux', 'x64', '[]')
+      `).run(runnerId1, 'test-runner-1', poolId, credentialId);
+      
+      db.prepare(`
+        INSERT INTO runners (id, name, status, pool_id, credential_id, isolation_type, ephemeral, platform, architecture, labels)
+        VALUES (?, ?, 'offline', ?, ?, 'native', 0, 'linux', 'x64', '[]')
+      `).run(runnerId2, 'test-runner-2', poolId, credentialId);
+      
+      // Verify runners exist
+      const runnersBefore = db.prepare('SELECT id FROM runners WHERE pool_id = ?').all(poolId);
+      expect(runnersBefore.length).toBe(2);
+      
+      // Delete the pool
+      const response = await request(app).delete(`/api/pools/${poolId}`);
+      expect(response.status).toBe(204);
+      
+      // Verify the pool is deleted
+      const getPoolResponse = await request(app).get(`/api/pools/${poolId}`);
+      expect(getPoolResponse.status).toBe(404);
+      
+      // Verify runners are also deleted (not just orphaned with NULL pool_id)
+      const runnersAfter = db.prepare('SELECT id FROM runners WHERE id IN (?, ?)').all(runnerId1, runnerId2);
+      expect(runnersAfter.length).toBe(0);
+    });
   });
 });
diff --git a/frontend/package.json b/frontend/package.json
@@ -8,7 +8,7 @@
     "build": "tsc -b && vite build",
     "lint": "eslint .",
     "preview": "vite preview",
-    "test": "vitest",
+    "test": "vitest run",
     "typecheck": "tsc -b --noEmit"
   },
   "dependencies": {
diff --git a/frontend/src/components/RunnerManager.tsx b/frontend/src/components/RunnerManager.tsx
@@ -232,19 +232,25 @@ function AddRunnerForm({
 
 function RunnerRow({
   runner,
+  selected,
+  onSelect,
   onStart,
   onStop,
   onSync,
   onDelete,
   onViewLogs,
 }: {
   runner: Runner;
+  selected: boolean;
+  onSelect: (id: string, selected: boolean) => void;
   onStart: (id: string) => void;
   onStop: (id: string) => void;
   onSync: (id: string) => void;
   onDelete: (id: string) => void;
   onViewLogs: (runner: Runner) => void;
 }) {
+  const isOrphaned = runner.ephemeral && !runner.pool_id;
+  
   const platformIcons: Record<string, string> = {
     darwin: '🍎',
     linux: '🐧',
@@ -264,12 +270,27 @@ function RunnerRow({
   const canViewLogs = runner.isolation_type === 'docker' || runner.runner_dir;
   
   return (
-    <tr>
+    <tr className={isOrphaned ? 'bg-yellow-900/20' : ''}>
+      <td>
+        <input
+          type="checkbox"
+          checked={selected}
+          onChange={(e) => onSelect(runner.id, e.target.checked)}
+          className="w-4 h-4 rounded border-forest-600 bg-forest-800 text-forest-400 focus:ring-forest-500"
+        />
+      </td>
       <td>
         <div className="flex items-center gap-3">
           <span className="text-xl">{platformIcons[effectivePlatform] || '💻'}</span>
           <div>
-            <div className="font-medium">{runner.name}</div>
+            <div className="font-medium flex items-center gap-2">
+              {runner.name}
+              {isOrphaned && (
+                <span className="px-1.5 py-0.5 bg-yellow-700 rounded text-xs text-yellow-200" title="Ephemeral runner with no pool - likely orphaned">
+                  orphaned
+                </span>
+              )}
+            </div>
             <div className="text-xs text-muted">{runner.target}</div>
           </div>
         </div>
@@ -363,6 +384,8 @@ function RunnerRow({
 export function RunnerManager() {
   const [showAddForm, setShowAddForm] = useState(false);
   const [selectedRunnerForLogs, setSelectedRunnerForLogs] = useState<Runner | null>(null);
+  const [selectedRunnerIds, setSelectedRunnerIds] = useState<Set<string>>(new Set());
+  const [showOrphanedOnly, setShowOrphanedOnly] = useState(false);
   const queryClient = useQueryClient();
   
   const { data: runnersData, isLoading: runnersLoading } = useQuery({
@@ -398,7 +421,10 @@ export function RunnerManager() {
   
   const deleteMutation = useMutation({
     mutationFn: runnersApi.delete,
-    onSuccess: () => queryClient.invalidateQueries({ queryKey: ['runners'] }),
+    onSuccess: () => {
+      queryClient.invalidateQueries({ queryKey: ['runners'] });
+      setSelectedRunnerIds(new Set());
+    },
   });
   
   const handleDelete = (id: string) => {
@@ -410,6 +436,71 @@ export function RunnerManager() {
   const runners = runnersData?.runners || [];
   const credentials = credentialsData?.credentials || [];
   
+  // Filter runners based on orphaned filter
+  const filteredRunners = showOrphanedOnly 
+    ? runners.filter(r => r.ephemeral && !r.pool_id)
+    : runners;
+  
+  const orphanedCount = runners.filter(r => r.ephemeral && !r.pool_id).length;
+  
+  // Selection helpers
+  const handleSelectRunner = (id: string, selected: boolean) => {
+    setSelectedRunnerIds(prev => {
+      const next = new Set(prev);
+      if (selected) {
+        next.add(id);
+      } else {
+        next.delete(id);
+      }
+      return next;
+    });
+  };
+  
+  const handleSelectAll = (selected: boolean) => {
+    if (selected) {
+      setSelectedRunnerIds(new Set(filteredRunners.map(r => r.id)));
+    } else {
+      setSelectedRunnerIds(new Set());
+    }
+  };
+  
+  const handleBulkDelete = async () => {
+    const count = selectedRunnerIds.size;
+    if (count === 0) return;
+    
+    if (!confirm(`Are you sure you want to delete ${count} runner${count === 1 ? '' : 's'}? This will stop the runners and deregister them from GitHub.`)) {
+      return;
+    }
+    
+    // Delete runners in parallel (could be further optimized with a bulk API endpoint)
+    const ids = Array.from(selectedRunnerIds);
+    const results = await Promise.allSettled(
+      ids.map(id => runnersApi.delete(id))
+    );
+    
+    // Collect and report failures
+    const failedIds: string[] = [];
+    results.forEach((result, index) => {
+      if (result.status === 'rejected') {
+        console.error(`Failed to delete runner ${ids[index]}:`, result.reason);
+        failedIds.push(ids[index]);
+      }
+    });
+    
+    if (failedIds.length > 0) {
+      alert(
+        `Failed to delete ${failedIds.length} runner${failedIds.length === 1 ? '' : 's'}. ` +
+        'Please check the console for more details and try again.'
+      );
+    }
+    
+    setSelectedRunnerIds(new Set());
+    queryClient.invalidateQueries({ queryKey: ['runners'] });
+  };
+  
+  const allFilteredSelected = filteredRunners.length > 0 && filteredRunners.every(r => selectedRunnerIds.has(r.id));
+  const someFilteredSelected = filteredRunners.some(r => selectedRunnerIds.has(r.id));
+  
   return (
     <div className="space-y-6">
       {/* Header */}
@@ -451,23 +542,76 @@ export function RunnerManager() {
           </p>
         </div>
       ) : (
-        <div className="card p-0 overflow-hidden">
-          <table className="table">
-            <thead>
-              <tr>
-                <th>Runner</th>
-                <th>Status</th>
-                <th>OS/Arch</th>
-                <th>Isolation</th>
-                <th>Labels</th>
+        <>
+          {/* Filter and bulk actions bar */}
+          <div className="card flex items-center justify-between">
+            <div className="flex items-center gap-4">
+              <label className="flex items-center gap-2 cursor-pointer">
+                <input
+                  type="checkbox"
+                  checked={showOrphanedOnly}
+                  onChange={(e) => {
+                    setShowOrphanedOnly(e.target.checked);
+                    setSelectedRunnerIds(new Set());
+                  }}
+                  className="w-4 h-4 rounded border-forest-600 bg-forest-800 text-forest-400 focus:ring-forest-500"
+                />
+                <span className="text-sm">
+                  Show orphaned only
+                  {orphanedCount > 0 && (
+                    <span className="ml-1 px-1.5 py-0.5 bg-yellow-700 rounded text-xs text-yellow-200">
+                      {orphanedCount}
+                    </span>
+                  )}
+                </span>
+              </label>
+              {selectedRunnerIds.size > 0 && (
+                <span className="text-sm text-muted">
+                  {selectedRunnerIds.size} selected
+                </span>
+              )}
+            </div>
+            {selectedRunnerIds.size > 0 && (
+              <button
+                onClick={handleBulkDelete}
+                className="btn btn-sm bg-red-700 hover:bg-red-600 text-white"
+              >
+                <Trash2 className="h-4 w-4 mr-1" />
+                Delete Selected ({selectedRunnerIds.size})
+              </button>
+            )}
+          </div>
+          
+          <div className="card p-0 overflow-hidden">
+            <table className="table">
+              <thead>
+                <tr>
+                  <th className="w-10">
+                    <input
+                      type="checkbox"
+                      checked={allFilteredSelected}
+                      ref={(el) => {
+                        if (el) el.indeterminate = someFilteredSelected && !allFilteredSelected;
+                      }}
+                      onChange={(e) => handleSelectAll(e.target.checked)}
+                      className="w-4 h-4 rounded border-forest-600 bg-forest-800 text-forest-400 focus:ring-forest-500"
+                    />
+                  </th>
+                  <th>Runner</th>
+                  <th>Status</th>
+                  <th>OS/Arch</th>
+                  <th>Isolation</th>
+                  <th>Labels</th>
                 <th>Actions</th>
               </tr>
             </thead>
             <tbody>
-              {runners.map((runner) => (
+              {filteredRunners.map((runner) => (
                 <RunnerRow
                   key={runner.id}
                   runner={runner}
+                  selected={selectedRunnerIds.has(runner.id)}
+                  onSelect={handleSelectRunner}
                   onStart={startMutation.mutate}
                   onStop={stopMutation.mutate}
                   onSync={syncMutation.mutate}
@@ -478,6 +622,7 @@ export function RunnerManager() {
             </tbody>
           </table>
         </div>
+        </>
       )}
       
       {/* Add Form Modal */}
