Skip to content

chore: Refactor cronjob controller #3029

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
hmalik88 wants to merge 21 commits into from
Closed

chore: Refactor cronjob controller #3029

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
f2d8bed
refactor cronjob controller
hmalik88 Jan 21, 2025
75020fb
undo comment deletion
hmalik88 Jan 21, 2025
bd7c7c2
Merge branch 'main' into hm/refactor-cronjob-controller
hmalik88 Jan 21, 2025
33dfc92
fix coverage
hmalik88 Jan 21, 2025
546f791
fix coverage again
hmalik88 Jan 21, 2025
5c7ae9a
Merge branch 'main' into hm/refactor-cronjob-controller
hmalik88 Jan 23, 2025
9452deb
apply code review
hmalik88 Jan 23, 2025
2d380a2
update coverage
hmalik88 Jan 23, 2025
131a290
revert change to destroy method
hmalik88 Jan 23, 2025
f7f4cb5
update coverage
hmalik88 Jan 23, 2025
eafee0f
Merge branch 'main' into hm/refactor-cronjob-controller
hmalik88 Jan 29, 2025
99c20e6
Merge branch 'main' into hm/refactor-cronjob-controller
hmalik88 Feb 26, 2025
88b055c
Merge branch 'main' into hm/refactor-cronjob-controller
hmalik88 Feb 28, 2025
2f4ad03
add missing comment
hmalik88 Feb 28, 2025
02d1fee
Merge branch 'main' into hm/refactor-cronjob-controller
hmalik88 Mar 4, 2025
53e27ac
remove validateTime param
hmalik88 Mar 4, 2025
764ddf5
update coverage
hmalik88 Mar 4, 2025
5ac3bbd
skip code coverage on error logs
hmalik88 Mar 11, 2025
d7a3845
Merge branch 'main' into hm/refactor-cronjob-controller
hmalik88 Mar 27, 2025
872791b
revert comment removal
hmalik88 Mar 31, 2025
c059dd1
Merge branch 'main' into hm/refactor-cronjob-controller
hmalik88 Mar 31, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions packages/snaps-controllers/coverage.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"branches": 92.96,
"functions": 96.56,
"lines": 98.05,
"statements": 97.77
"branches": 93.01,
"functions": 97.14,
"lines": 98.13,
"statements": 97.84
}
2 changes: 1 addition & 1 deletion packages/snaps-controllers/src/cronjob/CronjobController.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,7 +308,7 @@ describe('CronjobController', () => {

expect(() =>
cronjobController.scheduleBackgroundEvent(backgroundEvent),
).toThrow('Cannot schedule an event in the past.');
).toThrow('Cannot schedule execution in the past.');

expect(cronjobController.state.events).toStrictEqual({});

Expand Down
247 changes: 143 additions & 104 deletions packages/snaps-controllers/src/cronjob/CronjobController.ts
View file
Open in desktop
Copy link
Member

@FrederikBolding FrederikBolding Jan 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't reviewed this in detail. but generally when I proposed we refactor the CronjobController, the main goal I had in mind was to unify the logic used to manage background events and cronjobs.

We currently treat these as entirely separate entities, but I don't think they need to be. My general thought was that we should investigate treating them as one entity with the only variation being non-recurring versus recurring cronjobs (or background events - no strong opinion on the name).

I had intended for the referenced ticket to be to investigate ways to achieve this.

Copy link
Contributor Author

@hmalik88 hmalik88 Jan 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR unifies some logic, but I inherently found it hard to make a mental model of both being the same thing because the term non-recurring cronjob in of itself is an oxymoron. Maybe there's a better term to encompass both things. One divergence is that we fetch the cron jobs every time from the manifest, maybe if we can fetch once and unify the state then some of the other logic itself can be refactored. We can keep the original ticket open, I think this is an improvement nonetheless.

Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,11 @@ export type CronjobControllerEvents =
| SnapDisabled
| CronjobControllerStateChangeEvent;

type BackgroundEventActions =
| `${typeof controllerName}:scheduleBackgroundEvent`
| `${typeof controllerName}:cancelBackgroundEvent`
| `${typeof controllerName}:getBackgroundEvents`;

export type CronjobControllerMessenger = RestrictedControllerMessenger<
typeof controllerName,
CronjobControllerActions,
Expand Down Expand Up @@ -152,55 +157,128 @@ export class CronjobController extends BaseController<
this._handleEventSnapUpdated = this._handleEventSnapUpdated.bind(this);
this._handleSnapDisabledEvent = this._handleSnapDisabledEvent.bind(this);
this._handleSnapEnabledEvent = this._handleSnapEnabledEvent.bind(this);

// Subscribe to Snap events
/* eslint-disable @typescript-eslint/unbound-method */
this.#initializeEventListeners();

this.messagingSystem.subscribe(
'SnapController:snapInstalled',
this._handleSnapRegisterEvent,
);
// Register action handlers
this.#initializeActionHandlers();

this.messagingSystem.subscribe(
'SnapController:snapUninstalled',
this._handleSnapUnregisterEvent,
);
this.dailyCheckIn().catch((error) => {
logError(error);
});

this.messagingSystem.subscribe(
'SnapController:snapEnabled',
this._handleSnapEnabledEvent,
);
this.#rescheduleBackgroundEvents(Object.values(this.state.events));
}

this.messagingSystem.subscribe(
'SnapController:snapDisabled',
this._handleSnapDisabledEvent,
);
/**
* Initialize event listeners.
*/
#initializeEventListeners() {
/* eslint-disable @typescript-eslint/unbound-method */
const events = {
'SnapController:snapInstalled': this._handleSnapRegisterEvent,
'SnapController:snapUninstalled': this._handleSnapUnregisterEvent,
'SnapController:snapEnabled': this._handleSnapEnabledEvent,
'SnapController:snapDisabled': this._handleSnapDisabledEvent,
'SnapController:snapUpdated': this._handleEventSnapUpdated,
};
/* eslint-disable @typescript-eslint/unbound-method */

this.messagingSystem.subscribe(
'SnapController:snapUpdated',
this._handleEventSnapUpdated,
);
/* eslint-enable @typescript-eslint/unbound-method */
Object.entries(events).forEach(([event, handler]) => {
this.messagingSystem.subscribe(
event as CronjobControllerEvents['type'],
handler,
);
});
}

this.messagingSystem.registerActionHandler(
`${controllerName}:scheduleBackgroundEvent`,
(...args) => this.scheduleBackgroundEvent(...args),
);
/**
* Initialize action handlers.
*/
#initializeActionHandlers(): void {
const handlers: Record<BackgroundEventActions, (...args: any[]) => any> = {
[`${controllerName}:scheduleBackgroundEvent`]: (
event: Omit<BackgroundEvent, 'id' | 'scheduledAt'>,
) => this.scheduleBackgroundEvent(event),
[`${controllerName}:cancelBackgroundEvent`]: (
origin: string,
id: string,
) => this.cancelBackgroundEvent(origin, id),
[`${controllerName}:getBackgroundEvents`]: (snapId: SnapId) =>
this.getBackgroundEvents(snapId),
};

this.messagingSystem.registerActionHandler(
`${controllerName}:cancelBackgroundEvent`,
(...args) => this.cancelBackgroundEvent(...args),
);
Object.entries(handlers).forEach(([action, handler]) => {
this.messagingSystem.registerActionHandler(
action as BackgroundEventActions,
handler,
);
});
}

this.messagingSystem.registerActionHandler(
`${controllerName}:getBackgroundEvents`,
(...args) => this.getBackgroundEvents(...args),
);
/**
* Execute a request.
*
* @param snapId - ID of a Snap.
* @param request - Request to be executed.
*/
async #executeRequest(snapId: SnapId, request: unknown) {
await this.messagingSystem
.call('SnapController:handleRequest', {
snapId,
origin: '',
handler: HandlerType.OnCronjob,
request: request as Record<string, unknown>,
})
.catch((error) => {
logError(error);
});
}

this.dailyCheckIn().catch((error) => {
logError(error);
/**
* Setup a timer.
*
* @param id - ID of a timer.
* @param ms - Time in milliseconds.
* @param snapId - ID of a Snap.
* @param onComplete - Callback function to be executed when the timer completes.
* @param validateTime - Whether the time should be validated.
*/
#setupTimer(
id: string,
ms: number,
snapId: SnapId,
onComplete: () => void,
validateTime = true,
) {
if (validateTime && ms <= 0) {
throw new Error('Cannot schedule execution in the past.');
}

const timer = new Timer(ms);
timer.start(() => {
onComplete();
this.#timers.delete(id);
this.#snapIds.delete(id);
});

this.#rescheduleBackgroundEvents(Object.values(this.state.events));
this.#timers.set(id, timer);
this.#snapIds.set(id, snapId);
}

/**
* Cleanup a timer.
*
* @param id - ID of a timer.
*/
#cleanupTimer(id: string) {
const timer = this.#timers.get(id);
if (timer) {
Copy link
Member

@Mrtenz Mrtenz Feb 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this throw an error if the timer doesn't exist?

Copy link
Contributor Author

@hmalik88 hmalik88 Feb 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm I don't think so, this function is only called when cancelling a background event.

timer.cancel();
this.#timers.delete(id);
this.#snapIds.delete(id);
}
}

/**
Expand Down Expand Up @@ -274,23 +352,21 @@ export class CronjobController extends BaseController<
return;
}

const timer = new Timer(ms);
timer.start(() => {
this.#executeCronjob(job).catch((error) => {
this.#setupTimer(
job.id,
ms,
job.snapId,
() => {
// TODO: Decide how to handle errors.
logError(error);
});

this.#timers.delete(job.id);
this.#schedule(job);
});
this.#executeCronjob(job).catch(logError);
this.#schedule(job);
},
false,
);

if (!this.state.jobs[job.id]?.lastRun) {
this.#updateJobLastRunState(job.id, 0); // 0 for init, never ran actually
}

this.#timers.set(job.id, timer);
this.#snapIds.set(job.id, job.snapId);
}

/**
Expand All @@ -300,12 +376,7 @@ export class CronjobController extends BaseController<
*/
async #executeCronjob(job: Cronjob) {
this.#updateJobLastRunState(job.id, Date.now());
await this.messagingSystem.call('SnapController:handleRequest', {
snapId: job.snapId,
origin: '',
handler: HandlerType.OnCronjob,
request: job.request,
});
await this.#executeRequest(job.snapId, job.request);
}

/**
Expand Down Expand Up @@ -359,10 +430,7 @@ export class CronjobController extends BaseController<
'Only the origin that scheduled this event can cancel it.',
);

const timer = this.#timers.get(id);
timer?.cancel();
this.#timers.delete(id);
this.#snapIds.delete(id);
this.#cleanupTimer(id);
this.update((state) => {
delete state.events[id];
});
Expand All @@ -378,32 +446,12 @@ export class CronjobController extends BaseController<
const now = new Date();
const ms = date.getTime() - now.getTime();

if (ms <= 0) {
throw new Error('Cannot schedule an event in the past.');
}

const timer = new Timer(ms);
timer.start(() => {
this.messagingSystem
.call('SnapController:handleRequest', {
snapId: event.snapId,
origin: '',
handler: HandlerType.OnCronjob,
request: event.request,
})
.catch((error) => {
logError(error);
});

this.#timers.delete(event.id);
this.#snapIds.delete(event.id);
this.#setupTimer(event.id, ms, event.snapId, () => {
this.#executeRequest(event.snapId, event.request).catch(logError);
this.update((state) => {
delete state.events[event.id];
});
});

this.#timers.set(event.id, timer);
this.#snapIds.set(event.id, event.snapId);
}

/**
Expand Down Expand Up @@ -532,32 +580,23 @@ export class CronjobController extends BaseController<
super.destroy();

/* eslint-disable @typescript-eslint/unbound-method */
this.messagingSystem.unsubscribe(
'SnapController:snapInstalled',
this._handleSnapRegisterEvent,
);

this.messagingSystem.unsubscribe(
'SnapController:snapUninstalled',
this._handleSnapUnregisterEvent,
);

this.messagingSystem.unsubscribe(
'SnapController:snapEnabled',
this._handleSnapEnabledEvent,
);

this.messagingSystem.unsubscribe(
'SnapController:snapDisabled',
this._handleSnapDisabledEvent,
);
const events = {
'SnapController:snapInstalled': this._handleSnapRegisterEvent,
'SnapController:snapUninstalled': this._handleSnapUnregisterEvent,
'SnapController:snapEnabled': this._handleSnapEnabledEvent,
'SnapController:snapDisabled': this._handleSnapDisabledEvent,
'SnapController:snapUpdated': this._handleEventSnapUpdated,
};

this.messagingSystem.unsubscribe(
'SnapController:snapUpdated',
this._handleEventSnapUpdated,
);
/* eslint-enable @typescript-eslint/unbound-method */

Object.entries(events).forEach(([event, handler]) => {
this.messagingSystem.unsubscribe(
event as CronjobControllerEvents['type'],
handler,
);
});

this.#snapIds.forEach((snapId) => this.unregister(snapId));
}

Expand Down
Loading