Address code review feedback for sync auto-disable

Antoine de Chevigné · claude · Antoine de Chevigné · commit a9dae10141c8 · 2026-01-09T16:38:42.000+01:00
- Fix race condition: use atomic increment for syncFailedAttempts
- Add null check for explorer.workspace in updateExplorerSyncingProcess
- Simplify backoff calculation using recoveryAttempts counter
- Add max recovery attempts (10) after which manual intervention required
- Add batching (50 explorers) to syncRecoveryCheck job
- Add random jitter (up to 2 min) to stagger recovery checks
- Remove dead code (resetSyncState method)
- Add recoveryAttempts column to migration

Co-Authored-By: Claude Opus 4.5 &lt;noreply@anthropic.com&gt;
diff --git a/run/jobs/syncRecoveryCheck.js b/run/jobs/syncRecoveryCheck.js
@@ -4,6 +4,7 @@
  * Re-enables sync when RPC becomes reachable, using exponential backoff for retries.
  *
  * Backoff schedule: 5m -> 15m -> 1h -> 6h (max)
+ * Max recovery attempts: 10 (after which manual intervention is required)
  *
  * @module jobs/syncRecoveryCheck
  */
@@ -14,8 +15,12 @@ const { withTimeout } = require('../lib/utils');
 const { Op } = require('sequelize');
 const logger = require('../lib/logger');
 
+// Process explorers in batches to avoid long-running jobs
+const BATCH_SIZE = 50;
+
 module.exports = async () => {
-    // Find explorers that are due for a recovery check
+    // Find explorers that are due for a recovery check (with batch limit)
+    // Excludes explorers that have reached max recovery attempts (nextRecoveryCheckAt is null)
     const explorers = await Explorer.findAll({
         where: {
             syncDisabledReason: { [Op.ne]: null },
@@ -25,7 +30,9 @@ module.exports = async () => {
             model: Workspace,
             as: 'workspace',
             attributes: ['id', 'rpcServer']
-        }]
+        }],
+        limit: BATCH_SIZE,
+        order: [['nextRecoveryCheckAt', 'ASC']]  // Process oldest first
     });
 
     if (explorers.length === 0) {
@@ -34,9 +41,20 @@ module.exports = async () => {
 
     let recovered = 0;
     let stillUnreachable = 0;
+    let maxAttemptsReached = 0;
 
     for (const explorer of explorers) {
         try {
+            // Skip if workspace is missing
+            if (!explorer.workspace || !explorer.workspace.rpcServer) {
+                logger.warn({
+                    message: 'Explorer recovery check skipped: no workspace or RPC server',
+                    explorerId: explorer.id,
+                    explorerSlug: explorer.slug
+                });
+                continue;
+            }
+
             const provider = new ProviderConnector(explorer.workspace.rpcServer);
             const block = await withTimeout(provider.fetchLatestBlock());
 
@@ -49,17 +67,38 @@ module.exports = async () => {
                     message: 'Explorer re-enabled after RPC recovery',
                     explorerId: explorer.id,
                     explorerSlug: explorer.slug,
-                    disabledReason: explorer.syncDisabledReason
+                    disabledReason: explorer.syncDisabledReason,
+                    recoveryAttempts: explorer.recoveryAttempts
                 });
             } else {
                 // Block fetch returned null - still unreachable
-                await explorer.scheduleNextRecoveryCheck();
-                stillUnreachable++;
+                const result = await explorer.scheduleNextRecoveryCheck();
+                if (result.maxReached) {
+                    maxAttemptsReached++;
+                    logger.warn({
+                        message: 'Explorer reached max recovery attempts - manual intervention required',
+                        explorerId: explorer.id,
+                        explorerSlug: explorer.slug,
+                        attempts: result.attempts
+                    });
+                } else {
+                    stillUnreachable++;
+                }
             }
         } catch (error) {
             // RPC check failed - schedule next check with backoff
-            await explorer.scheduleNextRecoveryCheck();
-            stillUnreachable++;
+            const result = await explorer.scheduleNextRecoveryCheck();
+            if (result.maxReached) {
+                maxAttemptsReached++;
+                logger.warn({
+                    message: 'Explorer reached max recovery attempts - manual intervention required',
+                    explorerId: explorer.id,
+                    explorerSlug: explorer.slug,
+                    attempts: result.attempts
+                });
+            } else {
+                stillUnreachable++;
+            }
 
             logger.debug({
                 message: 'Explorer recovery check failed',
@@ -70,5 +109,5 @@ module.exports = async () => {
         }
     }
 
-    return `Checked ${explorers.length} explorers: ${recovered} recovered, ${stillUnreachable} still unreachable`;
+    return `Checked ${explorers.length} explorers: ${recovered} recovered, ${stillUnreachable} still unreachable, ${maxAttemptsReached} max attempts reached`;
 };
diff --git a/run/jobs/updateExplorerSyncingProcess.js b/run/jobs/updateExplorerSyncingProcess.js
@@ -9,6 +9,7 @@ const { Explorer, Workspace, RpcHealthCheck, StripeSubscription, StripePlan } =
 const PM2 = require('../lib/pm2');
 const logger = require('../lib/logger');
 
+// Must match SYNC_FAILURE_THRESHOLD in explorer.js
 const SYNC_FAILURE_THRESHOLD = 3;
 
 module.exports = async job => {
@@ -23,17 +24,21 @@ module.exports = async job => {
             {
                 model: Workspace,
                 as: 'workspace',
+                required: false,
                 include: {
                     model: RpcHealthCheck,
-                    as: 'rpcHealthCheck'
+                    as: 'rpcHealthCheck',
+                    required: false
                 }
             },
             {
                 model: StripeSubscription,
                 as: 'stripeSubscription',
+                required: false,
                 include: {
                     model: StripePlan,
-                    as: 'stripePlan'
+                    as: 'stripePlan',
+                    required: false
                 }
             }
         ]
@@ -59,7 +64,11 @@ module.exports = async job => {
             await pm2.delete(explorer.slug);
             return 'Process deleted: no subscription.';
         }
-        else if (explorer.workspace.rpcHealthCheck && !explorer.workspace.rpcHealthCheck.isReachable && existingProcess) {
+        else if (explorer && !explorer.workspace) {
+            await pm2.delete(explorer.slug);
+            return 'Process deleted: no workspace.';
+        }
+        else if (explorer.workspace && explorer.workspace.rpcHealthCheck && !explorer.workspace.rpcHealthCheck.isReachable && existingProcess) {
             await pm2.delete(explorer.slug);
             // Track RPC failure and potentially auto-disable
             const result = await explorer.incrementSyncFailures('rpc_unreachable');
diff --git a/run/migrations/20260109161142-add-sync-failure-tracking.js b/run/migrations/20260109161142-add-sync-failure-tracking.js
@@ -20,6 +20,12 @@ module.exports = {
         allowNull: true
       }, { transaction });
 
+      await queryInterface.addColumn('explorers', 'recoveryAttempts', {
+        type: Sequelize.DataTypes.INTEGER,
+        allowNull: false,
+        defaultValue: 0
+      }, { transaction });
+
       await queryInterface.addColumn('explorers', 'nextRecoveryCheckAt', {
         type: Sequelize.DataTypes.DATE,
         allowNull: true
@@ -47,6 +53,7 @@ module.exports = {
       await queryInterface.removeColumn('explorers', 'syncFailedAttempts', { transaction });
       await queryInterface.removeColumn('explorers', 'syncDisabledAt', { transaction });
       await queryInterface.removeColumn('explorers', 'syncDisabledReason', { transaction });
+      await queryInterface.removeColumn('explorers', 'recoveryAttempts', { transaction });
       await queryInterface.removeColumn('explorers', 'nextRecoveryCheckAt', { transaction });
 
       await transaction.commit();
diff --git a/run/models/explorer.js b/run/models/explorer.js
@@ -33,12 +33,15 @@ const MAX_RPC_ATTEMPTS = 3;
 
 // Sync failure auto-disable configuration
 const SYNC_FAILURE_THRESHOLD = 3;
+const MAX_RECOVERY_ATTEMPTS = 10;
 const RECOVERY_BACKOFF_SCHEDULE = [
     5 * 60 * 1000,       // 5 minutes
     15 * 60 * 1000,      // 15 minutes
     60 * 60 * 1000,      // 1 hour
     6 * 60 * 60 * 1000   // 6 hours (max)
 ];
+// Stagger recovery checks to avoid thundering herd (random jitter up to 2 minutes)
+const RECOVERY_JITTER_MAX = 2 * 60 * 1000;
 
 module.exports = (sequelize, DataTypes) => {
   class Explorer extends Model {
@@ -317,6 +320,7 @@ module.exports = (sequelize, DataTypes) => {
             syncFailedAttempts: 0,
             syncDisabledAt: null,
             syncDisabledReason: null,
+            recoveryAttempts: 0,
             nextRecoveryCheckAt: null
         });
         return this;
@@ -332,82 +336,76 @@ module.exports = (sequelize, DataTypes) => {
 
     /**
      * Increments the sync failure counter and auto-disables if threshold reached.
+     * Uses atomic increment to avoid race conditions.
      * @param {string} [reason='rpc_unreachable'] - Reason for the failure
      * @returns {Promise<{disabled: boolean, attempts: number}>} Result with disable status
      */
     async incrementSyncFailures(reason = 'rpc_unreachable') {
-        const newCount = (this.syncFailedAttempts || 0) + 1;
-        await this.update({ syncFailedAttempts: newCount });
+        // Use atomic increment to avoid race conditions
+        await this.increment('syncFailedAttempts');
+        await this.reload();
 
-        if (newCount >= SYNC_FAILURE_THRESHOLD) {
+        if (this.syncFailedAttempts >= SYNC_FAILURE_THRESHOLD) {
             await this.autoDisableSync(reason);
-            return { disabled: true, attempts: newCount };
+            return { disabled: true, attempts: this.syncFailedAttempts };
         }
-        return { disabled: false, attempts: newCount };
+        return { disabled: false, attempts: this.syncFailedAttempts };
     }
 
     /**
      * Auto-disables sync and schedules first recovery check.
+     * Adds random jitter to avoid thundering herd when many explorers are disabled at once.
      * @param {string} reason - Reason for disabling (e.g., 'rpc_unreachable')
      * @returns {Promise<Explorer>} Updated explorer
      */
     async autoDisableSync(reason) {
-        const nextCheck = new Date(Date.now() + RECOVERY_BACKOFF_SCHEDULE[0]);
+        // Add random jitter to stagger recovery checks
+        const jitter = Math.floor(Math.random() * RECOVERY_JITTER_MAX);
+        const nextCheck = new Date(Date.now() + RECOVERY_BACKOFF_SCHEDULE[0] + jitter);
         await this.update({
             shouldSync: false,
             syncDisabledAt: new Date(),
             syncDisabledReason: reason,
+            recoveryAttempts: 0,
             nextRecoveryCheckAt: nextCheck
         });
         return this;
     }
 
-    /**
-     * Resets all sync failure tracking state.
-     * @returns {Promise<Explorer>} Updated explorer
-     */
-    async resetSyncState() {
-        await this.update({
-            syncFailedAttempts: 0,
-            syncDisabledAt: null,
-            syncDisabledReason: null,
-            nextRecoveryCheckAt: null
-        });
-        return this;
-    }
-
     /**
      * Schedules the next recovery check using exponential backoff.
+     * Increments recovery attempts and returns null if max attempts reached.
      * Backoff schedule: 5m -> 15m -> 1h -> 6h (max)
-     * @returns {Promise<Explorer>} Updated explorer
+     * @returns {Promise<{scheduled: boolean, attempts: number, maxReached: boolean}>} Result
      */
     async scheduleNextRecoveryCheck() {
         if (!this.syncDisabledAt) {
-            return this;
+            return { scheduled: false, attempts: 0, maxReached: false };
         }
 
-        const timeSinceDisabled = Date.now() - new Date(this.syncDisabledAt).getTime();
-        let cumulativeTime = 0;
-        let backoffIndex = 0;
+        const newAttempts = (this.recoveryAttempts || 0) + 1;
 
-        // Find which backoff interval we should use based on time since disabled
-        for (let i = 0; i < RECOVERY_BACKOFF_SCHEDULE.length; i++) {
-            cumulativeTime += RECOVERY_BACKOFF_SCHEDULE[i];
-            if (timeSinceDisabled < cumulativeTime) {
-                backoffIndex = i;
-                break;
-            }
-            backoffIndex = i;
+        // Check if max recovery attempts reached
+        if (newAttempts >= MAX_RECOVERY_ATTEMPTS) {
+            await this.update({
+                recoveryAttempts: newAttempts,
+                nextRecoveryCheckAt: null,
+                syncDisabledReason: 'max_recovery_attempts_reached'
+            });
+            return { scheduled: false, attempts: newAttempts, maxReached: true };
         }
 
-        // Cap at max backoff (last element)
-        if (backoffIndex >= RECOVERY_BACKOFF_SCHEDULE.length) {
-            backoffIndex = RECOVERY_BACKOFF_SCHEDULE.length - 1;
-        }
+        // Use recovery attempts as index, capped at max backoff
+        const backoffIndex = Math.min(newAttempts - 1, RECOVERY_BACKOFF_SCHEDULE.length - 1);
+        // Add random jitter to stagger recovery checks
+        const jitter = Math.floor(Math.random() * RECOVERY_JITTER_MAX);
+        const nextCheck = new Date(Date.now() + RECOVERY_BACKOFF_SCHEDULE[backoffIndex] + jitter);
 
-        const nextCheck = new Date(Date.now() + RECOVERY_BACKOFF_SCHEDULE[backoffIndex]);
-        await this.update({ nextRecoveryCheckAt: nextCheck });
-        return this;
+        await this.update({
+            recoveryAttempts: newAttempts,
+            nextRecoveryCheckAt: nextCheck
+        });
+        return { scheduled: true, attempts: newAttempts, maxReached: false };
     }
 
     /**
@@ -420,6 +418,7 @@ module.exports = (sequelize, DataTypes) => {
             syncFailedAttempts: 0,
             syncDisabledAt: null,
             syncDisabledReason: null,
+            recoveryAttempts: 0,
             nextRecoveryCheckAt: null
         });
         return this;
@@ -806,6 +805,7 @@ module.exports = (sequelize, DataTypes) => {
     syncFailedAttempts: DataTypes.INTEGER,
     syncDisabledAt: DataTypes.DATE,
     syncDisabledReason: DataTypes.STRING,
+    recoveryAttempts: DataTypes.INTEGER,
     nextRecoveryCheckAt: DataTypes.DATE
   }, {
     hooks: {
diff --git a/run/tests/jobs/syncRecoveryCheck.test.js b/run/tests/jobs/syncRecoveryCheck.test.js
diff --git a/run/tests/jobs/updateExplorerSyncingProcess.test.js b/run/tests/jobs/updateExplorerSyncingProcess.test.js
