From 55b27728dc71efddf79d4582712ca9eba61b6fa2 Mon Sep 17 00:00:00 2001 From: Johannes Millan Date: Thu, 27 Nov 2025 20:35:15 +0100 Subject: [PATCH] feat(automationPlugin): enhance error handling and improve rate limiting for automation rules --- .../automations/src/core/actions.ts | 14 +- .../src/core/automation-manager.ts | 160 ++++++++++-------- .../automations/src/core/data-cache.ts | 6 +- .../src/core/rule-registry.spec.ts | 18 +- .../automations/src/core/rule-registry.ts | 67 +++++--- 5 files changed, 165 insertions(+), 100 deletions(-) diff --git a/packages/plugin-dev/automations/src/core/actions.ts b/packages/plugin-dev/automations/src/core/actions.ts index 785c0fce19..814f59b6a3 100644 --- a/packages/plugin-dev/automations/src/core/actions.ts +++ b/packages/plugin-dev/automations/src/core/actions.ts @@ -52,8 +52,15 @@ export const ActionDisplayDialog: IAutomationAction = { name: 'Display Dialog', execute: async (ctx, event, value) => { if (!value) return; + const escapedValue = value + .replace(/&/g, '&') + .replace(//g, '>') + .replace(/"/g, '"') + .replace(/'/g, '''); + await ctx.plugin.openDialog({ - htmlContent: `

${value}

`, + htmlContent: `

${escapedValue}

`, buttons: [{ label: 'OK', onClick: () => {} }], }); ctx.plugin.log.info(`[Automation] Action: Displayed dialog "${value}"`); @@ -66,11 +73,16 @@ export const ActionWebhook: IAutomationAction = { execute: async (ctx, event, value) => { if (!value) return; try { + const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), 5000); + await fetch(value, { method: 'POST', headers: { 'Content-Type': 'application/json' }, body: JSON.stringify(event), + signal: controller.signal, }); + clearTimeout(timeoutId); ctx.plugin.log.info(`[Automation] Action: Webhook sent to "${value}"`); } catch (e) { ctx.plugin.log.error(`[Automation] Webhook failed: ${e}`); diff --git a/packages/plugin-dev/automations/src/core/automation-manager.ts b/packages/plugin-dev/automations/src/core/automation-manager.ts index 8e5fbe5d53..3552d3a8fb 100644 --- a/packages/plugin-dev/automations/src/core/automation-manager.ts +++ b/packages/plugin-dev/automations/src/core/automation-manager.ts @@ -14,6 +14,11 @@ import * as Actions from './actions'; import { DataCache } from './data-cache'; export class AutomationManager { + private static readonly RATE_LIMIT_MAX = 5; + private static readonly RATE_LIMIT_WINDOW_MS = 1000; + private static readonly CHECK_INTERVAL_MS = 10000; + private static readonly TIME_RULE_COOLDOWN_MS = 60000; + private ruleRegistry: RuleRegistry; private conditionEvaluator: ConditionEvaluator; private actionExecutor: ActionExecutor; @@ -29,7 +34,10 @@ export class AutomationManager { this.ruleRegistry = new RuleRegistry(plugin); this.conditionEvaluator = new ConditionEvaluator(plugin, globalRegistry, this.dataCache); this.actionExecutor = new ActionExecutor(plugin, globalRegistry, this.dataCache); - this.rateLimiter = new RateLimiter(5, 1000); // 5 executions per second + this.rateLimiter = new RateLimiter( + AutomationManager.RATE_LIMIT_MAX, + AutomationManager.RATE_LIMIT_WINDOW_MS, + ); this.initTimeCheck(); } @@ -54,10 +62,9 @@ export class AutomationManager { } private initTimeCheck() { - // Check every 10 seconds this.clearTimeCheck = lazySetInterval(() => { this.checkTimeBasedRules(); - }, 10000); + }, AutomationManager.CHECK_INTERVAL_MS); } destroy() { @@ -68,38 +75,49 @@ export class AutomationManager { } private async checkTimeBasedRules() { - const rules = await this.ruleRegistry.getEnabledRules(); - const now = new Date(); - const currentHours = now.getHours(); - const currentMinutes = now.getMinutes(); - const currentTimeStr = `${currentHours.toString().padStart(2, '0')}:${currentMinutes.toString().padStart(2, '0')}`; + try { + const rules = await this.ruleRegistry.getEnabledRules(); + const now = new Date(); + const currentHours = now.getHours(); + const currentMinutes = now.getMinutes(); + const currentTimeStr = `${currentHours.toString().padStart(2, '0')}:${currentMinutes.toString().padStart(2, '0')}`; - for (const rule of rules) { - if (rule.trigger.type !== 'timeBased' || !rule.trigger.value) continue; + for (const rule of rules) { + try { + if (rule.trigger.type !== 'timeBased' || !rule.trigger.value) continue; - if (rule.trigger.value === currentTimeStr) { - const lastRun = this.lastExecutionTimes.get(rule.id) || 0; - // Prevent multiple executions within the same minute - if (now.getTime() - lastRun < 60000) continue; + if (rule.trigger.value === currentTimeStr) { + const lastRun = this.lastExecutionTimes.get(rule.id) || 0; + // Prevent multiple executions within the same minute + if (now.getTime() - lastRun < AutomationManager.TIME_RULE_COOLDOWN_MS) continue; - // Check conditions (even for time-based rules, though most conditions require a task) - // We pass a dummy event. The evaluator must handle missing tasks gracefully. - const event: TaskEvent = { - type: 'timeBased', - task: undefined, - }; + // Check conditions (even for time-based rules, though most conditions require a task) + // We pass a dummy event. The evaluator must handle missing tasks gracefully. + const event: TaskEvent = { + type: 'timeBased', + task: undefined, + }; - const matches = await this.conditionEvaluator.allConditionsMatch(rule.conditions, event); - if (!matches) continue; + const matches = await this.conditionEvaluator.allConditionsMatch( + rule.conditions, + event, + ); + if (!matches) continue; - this.lastExecutionTimes.set(rule.id, now.getTime()); - this.plugin.log.info(`[Automation] Time-based rule matched: ${rule.name}`); + this.lastExecutionTimes.set(rule.id, now.getTime()); + this.plugin.log.info(`[Automation] Time-based rule matched: ${rule.name}`); - // Execute actions - await this.actionExecutor.executeAll(rule.actions, { - type: 'timeBased', - }); + // Execute actions + await this.actionExecutor.executeAll(rule.actions, { + type: 'timeBased', + }); + } + } catch (e) { + this.plugin.log.error(`[Automation] Error processing time-based rule ${rule.name}: ${e}`); + } } + } catch (e) { + this.plugin.log.error(`[Automation] Error in checkTimeBasedRules: ${e}`); } } @@ -110,57 +128,65 @@ export class AutomationManager { } this.plugin.log.info(`[Automation] Event received: ${event.type}`, event.task.title); - const rules = await this.ruleRegistry.getEnabledRules(); + try { + const rules = await this.ruleRegistry.getEnabledRules(); - for (const rule of rules) { - const triggerImpl = globalRegistry.getTrigger(rule.trigger.type); - // If trigger not found or doesn't match, skip - if (!triggerImpl || !triggerImpl.matches(event, rule.trigger.value)) continue; + for (const rule of rules) { + try { + const triggerImpl = globalRegistry.getTrigger(rule.trigger.type); + // If trigger not found or doesn't match, skip + if (!triggerImpl || !triggerImpl.matches(event, rule.trigger.value)) continue; - const matches = await this.conditionEvaluator.allConditionsMatch(rule.conditions, event); - if (!matches) continue; + const matches = await this.conditionEvaluator.allConditionsMatch(rule.conditions, event); + if (!matches) continue; - // Check rate limit - if (!this.rateLimiter.check(rule.id)) { - this.plugin.log.warn(`[Automation] Rate limit exceeded for rule: ${rule.name}`); + // Check rate limit + if (!this.rateLimiter.check(rule.id)) { + this.plugin.log.warn(`[Automation] Rate limit exceeded for rule: ${rule.name}`); - if (!this.pendingDialogs.has(rule.id)) { - this.pendingDialogs.add(rule.id); + if (!this.pendingDialogs.has(rule.id)) { + this.pendingDialogs.add(rule.id); - const dialogCfg: DialogCfg = { - htmlContent: ` + const dialogCfg: DialogCfg = { + htmlContent: `

High Automation Activity Detected

The rule "${rule.name}" is triggering too frequently (infinite loop protection).

Do you want to disable this rule or continue execution?

`, - buttons: [ - { - label: 'Disable Rule', - color: 'warn', - onClick: async () => { - await this.ruleRegistry.toggleRuleStatus(rule.id, false); - this.plugin.showSnack({ msg: `Rule "${rule.name}" disabled`, type: 'INFO' }); - this.pendingDialogs.delete(rule.id); - }, - }, - { - label: 'Continue', - color: 'primary', - onClick: () => { - this.rateLimiter.reset(rule.id); - this.pendingDialogs.delete(rule.id); - }, - }, - ], - }; + buttons: [ + { + label: 'Disable Rule', + color: 'warn', + onClick: async () => { + await this.ruleRegistry.toggleRuleStatus(rule.id, false); + this.plugin.showSnack({ msg: `Rule "${rule.name}" disabled`, type: 'INFO' }); + this.pendingDialogs.delete(rule.id); + }, + }, + { + label: 'Continue', + color: 'primary', + onClick: () => { + this.rateLimiter.reset(rule.id); + this.pendingDialogs.delete(rule.id); + }, + }, + ], + }; - await this.plugin.openDialog(dialogCfg); + await this.plugin.openDialog(dialogCfg); + } + continue; + } + + this.plugin.log.info(`[Automation] Rule matched: ${rule.name}`); + await this.actionExecutor.executeAll(rule.actions, event); + } catch (e) { + this.plugin.log.error(`[Automation] Error processing rule ${rule.name}: ${e}`); } - continue; } - - this.plugin.log.info(`[Automation] Rule matched: ${rule.name}`); - await this.actionExecutor.executeAll(rule.actions, event); + } catch (e) { + this.plugin.log.error(`[Automation] Error in onTaskEvent: ${e}`); } } getRegistry(): RuleRegistry { diff --git a/packages/plugin-dev/automations/src/core/data-cache.ts b/packages/plugin-dev/automations/src/core/data-cache.ts index 0148673cf1..0125c77269 100644 --- a/packages/plugin-dev/automations/src/core/data-cache.ts +++ b/packages/plugin-dev/automations/src/core/data-cache.ts @@ -8,13 +8,13 @@ interface CacheItem { export class DataCache { private projectsCache: CacheItem | null = null; private tagsCache: CacheItem | null = null; - private readonly TTL = 60000; // 60 seconds + private static readonly CACHE_TTL_MS = 60000; constructor(private plugin: PluginAPI) {} async getProjects(): Promise { const now = Date.now(); - if (this.projectsCache && now - this.projectsCache.timestamp < this.TTL) { + if (this.projectsCache && now - this.projectsCache.timestamp < DataCache.CACHE_TTL_MS) { return this.projectsCache.data; } @@ -25,7 +25,7 @@ export class DataCache { async getTags(): Promise { const now = Date.now(); - if (this.tagsCache && now - this.tagsCache.timestamp < this.TTL) { + if (this.tagsCache && now - this.tagsCache.timestamp < DataCache.CACHE_TTL_MS) { return this.tagsCache.data; } diff --git a/packages/plugin-dev/automations/src/core/rule-registry.spec.ts b/packages/plugin-dev/automations/src/core/rule-registry.spec.ts index 7a5662272c..5a65f5bf23 100644 --- a/packages/plugin-dev/automations/src/core/rule-registry.spec.ts +++ b/packages/plugin-dev/automations/src/core/rule-registry.spec.ts @@ -24,7 +24,7 @@ describe('RuleRegistry', () => { // Since loadRules is called in constructor without await, we need to wait for promises. await new Promise(process.nextTick); - expect(registry.getRules()).toEqual([]); + expect(await registry.getRules()).toEqual([]); }); it('should load existing rules', async () => { @@ -43,7 +43,7 @@ describe('RuleRegistry', () => { registry = new RuleRegistry(mockPlugin); await new Promise(process.nextTick); - expect(registry.getRules()).toEqual(rules); + expect(await registry.getRules()).toEqual(rules); }); it('should add rule and persist', async () => { @@ -61,7 +61,7 @@ describe('RuleRegistry', () => { await registry.addOrUpdateRule(newRule); - expect(registry.getRules()).toContainEqual(newRule); + expect(await registry.getRules()).toContainEqual(newRule); expect(mockPlugin.persistDataSynced).toHaveBeenCalledWith(JSON.stringify([newRule])); }); @@ -81,8 +81,9 @@ describe('RuleRegistry', () => { const updatedRule = { ...rule, name: 'Updated Rule' }; await registry.addOrUpdateRule(updatedRule); - expect(registry.getRules()).toHaveLength(1); - expect(registry.getRules()[0].name).toBe('Updated Rule'); + const rules = await registry.getRules(); + expect(rules).toHaveLength(1); + expect(rules[0].name).toBe('Updated Rule'); }); it('should delete rule', async () => { @@ -99,7 +100,7 @@ describe('RuleRegistry', () => { await new Promise(process.nextTick); await registry.deleteRule('r1'); - expect(registry.getRules()).toHaveLength(0); + expect(await registry.getRules()).toHaveLength(0); expect(mockPlugin.persistDataSynced).toHaveBeenCalledWith(JSON.stringify([])); }); @@ -126,7 +127,8 @@ describe('RuleRegistry', () => { registry = new RuleRegistry(mockPlugin); await new Promise(process.nextTick); - expect(registry.getEnabledRules()).toHaveLength(1); - expect(registry.getEnabledRules()[0].id).toBe('r1'); + const enabledRules = await registry.getEnabledRules(); + expect(enabledRules).toHaveLength(1); + expect(enabledRules[0].id).toBe('r1'); }); }); diff --git a/packages/plugin-dev/automations/src/core/rule-registry.ts b/packages/plugin-dev/automations/src/core/rule-registry.ts index 080bb476cb..18425a89e2 100644 --- a/packages/plugin-dev/automations/src/core/rule-registry.ts +++ b/packages/plugin-dev/automations/src/core/rule-registry.ts @@ -5,6 +5,7 @@ export class RuleRegistry { private rules: AutomationRule[] = []; private plugin: PluginAPI; private initPromise: Promise; + private saveQueue: Promise = Promise.resolve(); constructor(plugin: PluginAPI) { this.plugin = plugin; @@ -18,7 +19,7 @@ export class RuleRegistry { this.rules = JSON.parse(data); } else { this.initDefaultRules(); - this.saveRules(); + await this.saveRules(); } } catch (e) { this.plugin.log.error('Failed to load rules', e); @@ -31,12 +32,15 @@ export class RuleRegistry { this.rules = []; } - async saveRules() { - try { - await this.plugin.persistDataSynced(JSON.stringify(this.rules)); - } catch (e) { - this.plugin.log.error('Failed to save rules', e); - } + private async saveRules() { + this.saveQueue = this.saveQueue.then(async () => { + try { + await this.plugin.persistDataSynced(JSON.stringify(this.rules)); + } catch (e) { + this.plugin.log.error('Failed to save rules', e); + } + }); + await this.saveQueue; } async getRules(): Promise { @@ -51,27 +55,48 @@ export class RuleRegistry { async addOrUpdateRule(rule: AutomationRule) { await this.initPromise; - const index = this.rules.findIndex((r) => r.id === rule.id); - if (index !== -1) { - this.rules[index] = rule; - } else { - this.rules.push(rule); - } - await this.saveRules(); + this.saveQueue = this.saveQueue.then(async () => { + const index = this.rules.findIndex((r) => r.id === rule.id); + if (index !== -1) { + this.rules[index] = rule; + } else { + this.rules.push(rule); + } + try { + await this.plugin.persistDataSynced(JSON.stringify(this.rules)); + } catch (e) { + this.plugin.log.error('Failed to save rules', e); + } + }); + await this.saveQueue; } async deleteRule(ruleId: string) { await this.initPromise; - this.rules = this.rules.filter((r) => r.id !== ruleId); - await this.saveRules(); + this.saveQueue = this.saveQueue.then(async () => { + this.rules = this.rules.filter((r) => r.id !== ruleId); + try { + await this.plugin.persistDataSynced(JSON.stringify(this.rules)); + } catch (e) { + this.plugin.log.error('Failed to save rules', e); + } + }); + await this.saveQueue; } async toggleRuleStatus(ruleId: string, isEnabled: boolean) { await this.initPromise; - const rule = this.rules.find((r) => r.id === ruleId); - if (rule) { - rule.isEnabled = isEnabled; - await this.saveRules(); - } + this.saveQueue = this.saveQueue.then(async () => { + const rule = this.rules.find((r) => r.id === ruleId); + if (rule) { + rule.isEnabled = isEnabled; + try { + await this.plugin.persistDataSynced(JSON.stringify(this.rules)); + } catch (e) { + this.plugin.log.error('Failed to save rules', e); + } + } + }); + await this.saveQueue; } }