diff --git a/src/device-config.ts b/src/device-config.ts index 6a0bac82..86d9b301 100644 --- a/src/device-config.ts +++ b/src/device-config.ts @@ -32,7 +32,7 @@ interface ConfigStep { // TODO: This is a bit of a mess, the DeviceConfig class shouldn't // know that the reboot action exists as it is implemented by // DeviceState. Fix this weird circular dependency - action: keyof DeviceActionExecutors | 'reboot'; + action: keyof DeviceActionExecutors | 'reboot' | 'noop'; humanReadableTarget?: Dictionary; target?: string | Dictionary; rebootRequired?: boolean; @@ -131,6 +131,17 @@ export class DeviceConfig { ..._.map(DeviceConfig.configKeys, 'envVarName'), ]; + private rateLimits: Dictionary<{ + duration: number; + lastAttempt: number | null; + }> = { + setVPNEnabled: { + // Only try to switch the VPN once an hour + duration: 60 * 60 * 1000, + lastAttempt: null, + }, + }; + public constructor({ db, config, logger }: DeviceConfigConstructOpts) { this.db = db; this.config = config; @@ -361,7 +372,7 @@ export class DeviceConfig { {}, ); - const steps: ConfigStep[] = []; + let steps: ConfigStep[] = []; const { deviceType, unmanaged } = await this.config.getMany([ 'deviceType', @@ -413,6 +424,27 @@ export class DeviceConfig { }); } + const now = Date.now(); + steps = _.map(steps, step => { + const action = step.action; + if (action in this.rateLimits) { + const lastAttempt = this.rateLimits[action].lastAttempt; + this.rateLimits[action].lastAttempt = now; + + // If this step should be rate limited, we replace it with a noop. + // We do this instead of removing it, as we don't actually want the + // state engine to think that it's successfully applied the target state, + // as it won't reattempt the change until the target state changes + if ( + lastAttempt != null && + Date.now() - lastAttempt < this.rateLimits[action].duration + ) { + return { action: 'noop' } as ConfigStep; + } + } + return step; + }); + // Do we need to change the boot config? if (this.bootConfigChangeRequired(backend, current, target, deviceType)) { steps.push({ @@ -431,7 +463,7 @@ export class DeviceConfig { } public executeStepAction(step: ConfigStep, opts: DeviceActionExecutorOpts) { - if (step.action !== 'reboot') { + if (step.action !== 'reboot' && step.action !== 'noop') { return this.actionExecutors[step.action](step, opts); } } diff --git a/src/device-state.coffee b/src/device-state.coffee index ea5fc80a..6f748824 100644 --- a/src/device-state.coffee +++ b/src/device-state.coffee @@ -578,8 +578,7 @@ module.exports = class DeviceState extends EventEmitter console.log('Scheduling another update attempt due to failure: ', delay, err) @triggerApplyTarget({ force, delay, initial }) - applyTarget: ({ force = false, initial = false, intermediate = false, skipLock = false } = {}) => - nextDelay = 200 + applyTarget: ({ force = false, initial = false, intermediate = false, skipLock = false, nextDelay = 200, retryCount = 0 } = {}) => Promise.try => if !intermediate @applyBlocker @@ -594,11 +593,23 @@ module.exports = class DeviceState extends EventEmitter .then ([ currentState, targetState ]) => @deviceConfig.getRequiredSteps(currentState, targetState) .then (deviceConfigSteps) => - if !_.isEmpty(deviceConfigSteps) - return deviceConfigSteps + noConfigSteps = _.every(deviceConfigSteps, ({ action }) -> action is 'noop') + if not noConfigSteps + return [false, deviceConfigSteps] else @applications.getRequiredSteps(currentState, targetState, extraState, intermediate) - .then (steps) => + .then (appSteps) -> + # We need to forward to no-ops to the next step if the application state is done + # The true and false values below represent whether we should add an exponential + # backoff if the steps are all no-ops. the reason that this has to be different + # is that a noop step from the application manager means that we should keep running + # in a tight loop, waiting for an image to download (for example). The device-config + # steps generally have a much higher time to wait before the no-ops will stop, so we + # should try to reduce the effect that this will have + if _.isEmpty(appSteps) and noConfigSteps + return [true, deviceConfigSteps] + return [false, appSteps] + .then ([backoff, steps]) => if _.isEmpty(steps) @emitAsync('apply-target-state-end', null) if !intermediate @@ -611,12 +622,17 @@ module.exports = class DeviceState extends EventEmitter if !intermediate @reportCurrentState(update_pending: true) if _.every(steps, (step) -> step.action == 'noop') - nextDelay = 1000 + if backoff + retryCount += 1 + # Backoff to a maximum of 10 minutes + nextDelay = Math.min(2 ** retryCount * 1000, 60 * 10 * 1000) + else + nextDelay = 1000 Promise.map steps, (step) => @applyStep(step, { force, initial, intermediate, skipLock }) .delay(nextDelay) .then => - @applyTarget({ force, initial, intermediate, skipLock }) + @applyTarget({ force, initial, intermediate, skipLock, nextDelay, retryCount }) .catch (err) => @applyError(err, { force, initial, intermediate }) diff --git a/src/network.ts b/src/network.ts index 82248e5f..3825305d 100644 --- a/src/network.ts +++ b/src/network.ts @@ -42,7 +42,7 @@ export function enableCheck(enable: boolean) { async function vpnStatusInotifyCallback(): Promise { try { - await fs.lstat(constants.vpnStatusPath); + await fs.lstat(`${constants.vpnStatusPath}/active`); isConnectivityCheckPaused = true; } catch { isConnectivityCheckPaused = false;