feat: atomic slot reservation

server/src/browser-management/classes/BrowserPool.ts

@@ -36,6 +36,14 @@ interface BrowserPoolInfo {
      * Can be "reserved", "initializing", "ready" or "failed".
      */
     status?: "reserved" | "initializing" | "ready" | "failed",
+    /**
+     * Timestamp when the browser slot was created/reserved
+     */
+    createdAt?: number,
+    /**
+     * Timestamp when the browser was last accessed
+     */
+    lastAccessed?: number,
 }
 
 /**
@@ -66,6 +74,12 @@ export class BrowserPool {
      */
     private userToBrowserMap: Map<string, string[]> = new Map();
 
+    /**
+     * Locks for atomic operations to prevent race conditions
+     * Key format: "userId-state", Value: timestamp when lock was acquired
+     */
+    private reservationLocks: Map<string, number> = new Map();
+
     /**
      * Adds a remote browser instance to the pool for a specific user.
      * If the user already has two browsers, the oldest browser will be closed and replaced.
@@ -570,7 +584,7 @@ export class BrowserPool {
     };
 
     /**
-     * Reserves a browser slot immediately without creating the actual browser.
+     * Reserves a browser slot atomically to prevent race conditions.
      * This ensures slot counting is accurate for rapid successive requests.
      * 
      * @param id browser ID to reserve
@@ -578,31 +592,65 @@ export class BrowserPool {
      * @param state browser state ("recording" or "run")
      * @returns true if slot was reserved, false if user has reached limit
      */
-    public reserveBrowserSlot = (id: string, userId: string, state: BrowserState = "run"): boolean => {
+    public reserveBrowserSlotAtomic = (id: string, userId: string, state: BrowserState = "run"): boolean => {
-        // Check if user has available slots first
+        const lockKey = `${userId}-${state}`;
-        if (!this.hasAvailableBrowserSlots(userId, state)) {
+        
-            logger.log('debug', `Cannot reserve slot for user ${userId}: no available slots`);
+        if (this.reservationLocks.has(lockKey)) {
+            logger.log('debug', `Reservation already in progress for user ${userId} state ${state}`);
             return false;
         }
+        
+        try {
+            this.reservationLocks.set(lockKey, Date.now());
+            
+            if (!this.hasAvailableBrowserSlots(userId, state)) {
+                logger.log('debug', `Cannot reserve slot for user ${userId}: no available slots`);
+                return false;
+            }
 
-        // Reserve the slot with null browser
+            if (this.pool[id]) {
-        this.pool[id] = {
+                logger.log('debug', `Browser slot ${id} already exists`);
-            browser: null,
+                return false;
-            active: false,
+            }
-            userId,
-            state,
-            status: "reserved"
-        };
 
-        // Update the user-to-browser mapping
+            const now = Date.now();
-        let userBrowserIds = this.userToBrowserMap.get(userId) || [];
+            
-        if (!userBrowserIds.includes(id)) {
+            this.pool[id] = {
-            userBrowserIds.push(id);
+                browser: null,
-            this.userToBrowserMap.set(userId, userBrowserIds);
+                active: false,
+                userId,
+                state,
+                status: "reserved",
+                createdAt: now,
+                lastAccessed: now
+            };
+
+            const userBrowserIds = this.userToBrowserMap.get(userId) || [];
+            if (!userBrowserIds.includes(id)) {
+                userBrowserIds.push(id);
+                this.userToBrowserMap.set(userId, userBrowserIds);
+            }
+
+            logger.log('info', `Atomically reserved browser slot ${id} for user ${userId} in state ${state}`);
+            return true;
+            
+        } catch (error: any) {
+            logger.log('error', `Error during atomic slot reservation: ${error.message}`);
+            if (this.pool[id] && this.pool[id].status === "reserved") {
+                this.deleteRemoteBrowser(id);
+            }
+            return false;
+        } finally {
+            this.reservationLocks.delete(lockKey);
         }
+    };
 
-        logger.log('info', `Reserved browser slot ${id} for user ${userId} in state ${state}`);
+    /**
-        return true;
+     * Legacy method - kept for backwards compatibility but now uses atomic version
+     * @deprecated Use reserveBrowserSlotAtomic instead
+     */
+    public reserveBrowserSlot = (id: string, userId: string, state: BrowserState = "run"): boolean => {
+        return this.reserveBrowserSlotAtomic(id, userId, state);
     };
 
     /**
@@ -630,17 +678,89 @@ export class BrowserPool {
     };
 
     /**
-     * Marks a reserved slot as failed and removes it.
+     * Marks a reserved slot as failed and removes it with proper cleanup.
      * 
      * @param id browser ID to mark as failed
      */
     public failBrowserSlot = (id: string): void => {
         if (this.pool[id]) {
             logger.log('info', `Marking browser slot ${id} as failed`);
+            
+            // Attempt to cleanup browser resources before deletion
+            const browserInfo = this.pool[id];
+            if (browserInfo.browser) {
+                try {
+                    // Try to close browser if it exists
+                    browserInfo.browser.switchOff?.().catch((error: any) => {
+                        logger.log('warn', `Error closing failed browser ${id}: ${error.message}`);
+                    });
+                } catch (error: any) {
+                    logger.log('warn', `Error during browser cleanup for ${id}: ${error.message}`);
+                }
+            }
+            
             this.deleteRemoteBrowser(id);
         }
     };
 
+    /**
+     * Cleanup stale browser slots that have been in reserved/initializing state too long
+     * This prevents resource leaks from failed initializations
+     */
+    public cleanupStaleBrowserSlots = (): void => {
+        const now = Date.now();
+        const staleThreshold = 5 * 60 * 1000; // 5 minutes
+        
+        const staleSlots: string[] = [];
+        
+        for (const [id, info] of Object.entries(this.pool)) {
+            const isStale = info.status === "reserved" || info.status === "initializing";
+            const createdAt = info.createdAt || 0;
+            const age = now - createdAt;
+            
+            if (isStale && info.browser === null && age > staleThreshold) {
+                staleSlots.push(id);
+            }
+        }
+        
+        staleSlots.forEach(id => {
+            const info = this.pool[id];
+            logger.log('warn', `Cleaning up stale browser slot ${id} with status ${info.status}, age: ${Math.round((now - (info.createdAt || 0)) / 1000)}s`);
+            this.failBrowserSlot(id);
+        });
+        
+        if (staleSlots.length > 0) {
+            logger.log('info', `Cleaned up ${staleSlots.length} stale browser slots`);
+        }
+        
+        this.cleanupStaleReservationLocks();
+    };
+
+    /**
+     * Cleans up reservation locks that are older than 1 minute
+     * This prevents locks from being held indefinitely due to crashes
+     */
+    private cleanupStaleReservationLocks = (): void => {
+        const now = Date.now();
+        const lockTimeout = 60 * 1000; // 1 minute
+        
+        const staleLocks: string[] = [];
+        
+        for (const [lockKey, timestamp] of this.reservationLocks.entries()) {
+            if (now - timestamp > lockTimeout) {
+                staleLocks.push(lockKey);
+            }
+        }
+        
+        staleLocks.forEach(lockKey => {
+            this.reservationLocks.delete(lockKey);
+        });
+        
+        if (staleLocks.length > 0) {
+            logger.log('warn', `Cleaned up ${staleLocks.length} stale reservation locks`);
+        }
+    };
+
     /**
      * Gets the current status of a browser slot.
      * 
@@ -653,4 +773,22 @@ export class BrowserPool {
         }
         return this.pool[id].status || null;
     };
+
+    /**
+     * Returns all browser instances in the pool.
+     * Used for cleanup operations like graceful shutdown.
+     * 
+     * @returns Map of browser IDs to browser instances
+     */
+    public getAllBrowsers = (): Map<string, RemoteBrowser> => {
+        const browsers = new Map<string, RemoteBrowser>();
+        
+        for (const [id, info] of Object.entries(this.pool)) {
+            if (info.browser) {
+                browsers.set(id, info.browser);
+            }
+        }
+        
+        return browsers;
+    };
 }