Skip to content

Commit 129ee4e

Browse files
Giovani Guizzomerencia
andauthored
fix: scheduled jobs not stopping with Sidequest (#100)
* fix: job builder as protected * chore: upgrade all deps * chore: update yarn lock * feat: implement ScheduledJobRegistry for managing scheduled cron jobs and add related tests * chore: upgrade all deps * fix: mock node-cron schedule function to return a mocked schedule object --------- Co-authored-by: merencia <lucas@merencia.com>
1 parent cf71b27 commit 129ee4e

16 files changed

Lines changed: 13735 additions & 13318 deletions

File tree

examples/package.json

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@
1313
"sidequest": "workspace:*"
1414
},
1515
"devDependencies": {
16-
"@types/express": "^5",
17-
"typescript": "^5.8.3"
16+
"@types/express": "^5.0.3",
17+
"typescript": "^5.9.2"
1818
}
1919
}

package.json

Lines changed: 23 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -45,46 +45,46 @@
4545
"devDependencies": {
4646
"@commitlint/cli": "^19.8.1",
4747
"@commitlint/config-conventional": "^19.8.1",
48-
"@eslint/js": "^9.29.0",
49-
"@rollup/plugin-commonjs": "^28.0.5",
48+
"@eslint/js": "^9.35.0",
49+
"@rollup/plugin-commonjs": "^28.0.6",
5050
"@rollup/plugin-json": "^6.1.0",
5151
"@rollup/plugin-node-resolve": "^16.0.1",
5252
"@rollup/plugin-replace": "^6.0.2",
53-
"@rollup/plugin-typescript": "^12.1.2",
53+
"@rollup/plugin-typescript": "^12.1.4",
5454
"@semantic-release/changelog": "^6.0.3",
5555
"@semantic-release/commit-analyzer": "^13.0.1",
5656
"@semantic-release/exec": "^7.1.0",
5757
"@semantic-release/git": "^10.0.1",
58-
"@semantic-release/github": "^11.0.3",
59-
"@semantic-release/release-notes-generator": "^14.0.3",
60-
"@tailwindcss/cli": "^4.1.10",
61-
"@tailwindcss/postcss": "^4.1.10",
62-
"@tsconfig/recommended": "^1.0.8",
58+
"@semantic-release/github": "^11.0.5",
59+
"@semantic-release/release-notes-generator": "^14.1.0",
60+
"@tailwindcss/cli": "^4.1.13",
61+
"@tailwindcss/postcss": "^4.1.13",
62+
"@tsconfig/recommended": "^1.0.10",
6363
"@types/express": "^5.0.3",
64-
"@types/node": "^24.0.3",
64+
"@types/node": "^24.3.1",
6565
"@vitest/coverage-v8": "3.2.4",
66-
"cross-env": "^7.0.3",
67-
"eslint": "^9.29.0",
68-
"globals": "^16.2.0",
66+
"cross-env": "^10.0.0",
67+
"eslint": "^9.35.0",
68+
"globals": "^16.3.0",
6969
"husky": "^9.1.7",
70-
"postcss": "^8.5.5",
71-
"prettier": "^3.5.3",
72-
"prettier-plugin-organize-imports": "^4.1.0",
70+
"postcss": "^8.5.6",
71+
"prettier": "^3.6.2",
72+
"prettier-plugin-organize-imports": "^4.2.0",
7373
"rimraf": "^6.0.1",
74-
"rollup": "^4.43.0",
74+
"rollup": "^4.50.0",
7575
"rollup-plugin-copy": "^3.5.0",
7676
"rollup-plugin-copy-watch": "^0.0.1",
7777
"rollup-plugin-delete": "^3.0.1",
78-
"rollup-plugin-dts": "^6.2.1",
78+
"rollup-plugin-dts": "^6.2.3",
7979
"rollup-plugin-postcss": "^4.0.2",
8080
"semantic-release": "^24.2.7",
81-
"tailwindcss": "^4.1.10",
82-
"tsx": "^4.19.4",
81+
"tailwindcss": "^4.1.13",
82+
"tsx": "^4.20.5",
8383
"turbo": "^2.5.6",
84-
"typescript": "^5.8.3",
85-
"typescript-eslint": "^8.34.0",
86-
"vitepress": "^1.6.3",
87-
"vitest": "^3.2.3"
84+
"typescript": "^5.9.2",
85+
"typescript-eslint": "^8.42.0",
86+
"vitepress": "^1.6.4",
87+
"vitest": "^3.2.4"
8888
},
8989
"packageManager": "yarn@4.9.4"
9090
}

packages/backends/mongo/package.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@
4545
"dependencies": {
4646
"@sidequest/backend": "workspace:*",
4747
"@sidequest/core": "workspace:*",
48-
"mongodb": "^6.17.0"
48+
"mongodb": "^6.19.0"
4949
},
5050
"devDependencies": {
5151
"@sidequest/backend-test": "workspace:*"

packages/backends/mysql/package.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,7 @@
4646
"@sidequest/backend": "workspace:*",
4747
"@sidequest/core": "workspace:*",
4848
"knex": "^3.1.0",
49-
"mysql2": "^3.14.2"
49+
"mysql2": "^3.14.4"
5050
},
5151
"devDependencies": {
5252
"@sidequest/backend-test": "workspace:*"

packages/backends/postgres/package.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,7 @@
4646
"@sidequest/backend": "workspace:*",
4747
"@sidequest/core": "workspace:*",
4848
"knex": "^3.1.0",
49-
"pg": "^8.15.6"
49+
"pg": "^8.16.3"
5050
},
5151
"devDependencies": {
5252
"@sidequest/backend-test": "workspace:*"

packages/cli/package.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,6 @@
3939
"dependencies": {
4040
"@sidequest/backend": "workspace:*",
4141
"commander": "^14.0.0",
42-
"inquirer": "^12.7.0"
42+
"inquirer": "^12.9.4"
4343
}
4444
}

packages/dashboard/package.json

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -48,15 +48,15 @@
4848
"express": "^5.1.0",
4949
"express-basic-auth": "^1.2.1",
5050
"express-ejs-layouts": "^2.5.1",
51-
"morgan": "^1.10.0"
51+
"morgan": "^1.10.1"
5252
},
5353
"devDependencies": {
5454
"@highlightjs/cdn-assets": "^11.11.1",
55-
"@types/feather-icons": "^4",
56-
"@types/morgan": "^1",
55+
"@types/feather-icons": "^4.29.4",
56+
"@types/morgan": "^1.9.10",
5757
"chart.js": "^4.5.0",
58-
"daisyui": "^5.0.43",
58+
"daisyui": "^5.1.7",
5959
"feather-icons": "^4.29.2",
60-
"htmx.org": "^2.0.4"
60+
"htmx.org": "^2.0.6"
6161
}
6262
}

packages/engine/package.json

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -44,8 +44,8 @@
4444
"dependencies": {
4545
"@sidequest/backend": "workspace:*",
4646
"@sidequest/core": "workspace:*",
47-
"node-cron": "^4.1.1",
48-
"piscina": "^5.1.2"
47+
"node-cron": "^4.2.1",
48+
"piscina": "^5.1.3"
4949
},
5050
"peerDependencies": {
5151
"sidequest": "workspace:*"

packages/engine/src/engine.ts

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import { cpus } from "os";
55
import { inspect } from "util";
66
import { DEFAULT_WORKER_PATH } from "./constants";
77
import { JOB_BUILDER_FALLBACK } from "./job/constants";
8+
import { ScheduledJobRegistry } from "./job/cron-registry";
89
import { JobBuilder, JobBuilderDefaults } from "./job/job-builder";
910
import { grantQueueConfig, QueueDefaults } from "./queue/grant-queue-config";
1011
import { clearGracefulShutdown, gracefulShutdown } from "./utils/shutdown";
@@ -253,6 +254,10 @@ export class Engine {
253254
if (!this.shuttingDown) {
254255
this.shuttingDown = true;
255256
logger("Engine").debug("Closing Sidequest engine...");
257+
258+
// Stop all scheduled cron jobs first
259+
await ScheduledJobRegistry.stopAll();
260+
256261
if (this.mainWorker) {
257262
const promise = new Promise((resolve) => {
258263
this.mainWorker!.on("exit", resolve);
Lines changed: 208 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,208 @@
1+
import { ScheduledTask } from "node-cron";
2+
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
3+
import { ScheduledJobRegistry } from "./cron-registry";
4+
5+
describe("ScheduledJobRegistry", () => {
6+
let mockTask: ScheduledTask;
7+
let stopSpy: ReturnType<typeof vi.fn>;
8+
9+
beforeEach(() => {
10+
// Create a mock ScheduledTask
11+
stopSpy = vi.fn().mockResolvedValue(undefined);
12+
mockTask = {
13+
id: "test-task-1",
14+
stop: stopSpy,
15+
getStatus: vi.fn().mockReturnValue("scheduled"),
16+
} as unknown as ScheduledTask;
17+
});
18+
19+
afterEach(async () => {
20+
// Clean up after each test
21+
await ScheduledJobRegistry.stopAll();
22+
vi.clearAllMocks();
23+
});
24+
25+
describe("register", () => {
26+
it("should register a scheduled task and return the task ID", () => {
27+
const taskId = ScheduledJobRegistry.register(mockTask);
28+
29+
expect(taskId).toBe("test-task-1");
30+
expect(ScheduledJobRegistry.hasTask("test-task-1")).toBe(true);
31+
expect(ScheduledJobRegistry.hasTask("test-task-2")).toBe(false);
32+
expect(ScheduledJobRegistry.getTaskCount()).toBe(1);
33+
});
34+
35+
it("should handle multiple task registrations", () => {
36+
const mockTask2 = {
37+
id: "test-task-2",
38+
stop: vi.fn().mockResolvedValue(undefined),
39+
getStatus: vi.fn().mockReturnValue("scheduled"),
40+
} as unknown as ScheduledTask;
41+
42+
const taskId1 = ScheduledJobRegistry.register(mockTask);
43+
const taskId2 = ScheduledJobRegistry.register(mockTask2);
44+
45+
expect(taskId1).toBe("test-task-1");
46+
expect(taskId2).toBe("test-task-2");
47+
expect(ScheduledJobRegistry.getTaskCount()).toBe(2);
48+
expect(ScheduledJobRegistry.hasTask("test-task-1")).toBe(true);
49+
expect(ScheduledJobRegistry.hasTask("test-task-2")).toBe(true);
50+
});
51+
52+
it("should overwrite task with same ID", () => {
53+
const mockTask2 = {
54+
id: "test-task-1", // Same ID as first task
55+
stop: vi.fn().mockResolvedValue(undefined),
56+
getStatus: vi.fn().mockReturnValue("scheduled"),
57+
} as unknown as ScheduledTask;
58+
59+
ScheduledJobRegistry.register(mockTask);
60+
expect(ScheduledJobRegistry.getTaskCount()).toBe(1);
61+
62+
ScheduledJobRegistry.register(mockTask2);
63+
expect(ScheduledJobRegistry.getTaskCount()).toBe(1); // Still 1, task was overwritten
64+
});
65+
});
66+
67+
describe("stop", () => {
68+
it("should stop and unregister a specific task", async () => {
69+
ScheduledJobRegistry.register(mockTask);
70+
expect(ScheduledJobRegistry.hasTask("test-task-1")).toBe(true);
71+
72+
const result = await ScheduledJobRegistry.stop("test-task-1");
73+
74+
expect(result).toBe(true);
75+
expect(stopSpy).toHaveBeenCalledOnce();
76+
expect(ScheduledJobRegistry.hasTask("test-task-1")).toBe(false);
77+
expect(ScheduledJobRegistry.getTaskCount()).toBe(0);
78+
});
79+
80+
it("should return false when trying to stop a non-existent task", async () => {
81+
const result = await ScheduledJobRegistry.stop("non-existent-task");
82+
expect(result).toBe(false);
83+
});
84+
85+
it("should handle task stop errors gracefully", async () => {
86+
const errorStopSpy = vi.fn().mockRejectedValue(new Error("Stop failed"));
87+
const errorTask = {
88+
id: "error-task",
89+
stop: errorStopSpy,
90+
getStatus: vi.fn().mockReturnValue("scheduled"),
91+
} as unknown as ScheduledTask;
92+
93+
ScheduledJobRegistry.register(errorTask);
94+
expect(ScheduledJobRegistry.hasTask("error-task")).toBe(true);
95+
96+
// The stop method should still throw the error
97+
await ScheduledJobRegistry.stop("error-task");
98+
99+
// But the task should still be removed from registry even if stop fails
100+
expect(ScheduledJobRegistry.hasTask("error-task")).toBe(false);
101+
expect(ScheduledJobRegistry.getTaskCount()).toBe(0);
102+
});
103+
});
104+
105+
describe("stopAll", () => {
106+
it("should stop all registered tasks and clear the registry", async () => {
107+
const stopSpy1 = vi.fn().mockResolvedValue(undefined);
108+
const stopSpy2 = vi.fn().mockResolvedValue(undefined);
109+
110+
const mockTask1 = {
111+
id: "task-1",
112+
stop: stopSpy1,
113+
getStatus: vi.fn().mockReturnValue("scheduled"),
114+
} as unknown as ScheduledTask;
115+
116+
const mockTask2 = {
117+
id: "task-2",
118+
stop: stopSpy2,
119+
getStatus: vi.fn().mockReturnValue("scheduled"),
120+
} as unknown as ScheduledTask;
121+
122+
ScheduledJobRegistry.register(mockTask1);
123+
ScheduledJobRegistry.register(mockTask2);
124+
expect(ScheduledJobRegistry.getTaskCount()).toBe(2);
125+
126+
await ScheduledJobRegistry.stopAll();
127+
128+
expect(stopSpy1).toHaveBeenCalledOnce();
129+
expect(stopSpy2).toHaveBeenCalledOnce();
130+
expect(ScheduledJobRegistry.getTaskCount()).toBe(0);
131+
});
132+
133+
it("should handle empty registry gracefully", async () => {
134+
expect(ScheduledJobRegistry.getTaskCount()).toBe(0);
135+
await expect(ScheduledJobRegistry.stopAll()).resolves.not.toThrow();
136+
expect(ScheduledJobRegistry.getTaskCount()).toBe(0);
137+
});
138+
139+
it("should continue stopping other tasks even if one fails", async () => {
140+
const stopSpy1 = vi.fn().mockResolvedValue(undefined);
141+
const stopSpy2 = vi.fn().mockResolvedValue(undefined);
142+
const errorStopSpy = vi.fn().mockRejectedValue(new Error("Stop failed"));
143+
144+
const mockTask1 = {
145+
id: "task-1",
146+
stop: stopSpy1,
147+
getStatus: vi.fn().mockReturnValue("scheduled"),
148+
} as unknown as ScheduledTask;
149+
150+
const errorTask = {
151+
id: "error-task",
152+
stop: errorStopSpy,
153+
getStatus: vi.fn().mockReturnValue("scheduled"),
154+
} as unknown as ScheduledTask;
155+
156+
const mockTask2 = {
157+
id: "task-2",
158+
stop: stopSpy2,
159+
getStatus: vi.fn().mockReturnValue("scheduled"),
160+
} as unknown as ScheduledTask;
161+
162+
ScheduledJobRegistry.register(mockTask1);
163+
ScheduledJobRegistry.register(errorTask);
164+
ScheduledJobRegistry.register(mockTask2);
165+
expect(ScheduledJobRegistry.getTaskCount()).toBe(3);
166+
167+
await ScheduledJobRegistry.stopAll();
168+
169+
expect(stopSpy1).toHaveBeenCalledOnce();
170+
expect(errorStopSpy).toHaveBeenCalledOnce();
171+
expect(stopSpy2).toHaveBeenCalledOnce();
172+
173+
// Registry should be cleared even with errors
174+
expect(ScheduledJobRegistry.getTaskCount()).toBe(0);
175+
});
176+
});
177+
178+
describe("registry state management", () => {
179+
it("should properly manage registry state across multiple operations", async () => {
180+
// Start with empty registry
181+
expect(ScheduledJobRegistry.getTaskCount()).toBe(0);
182+
183+
// Register multiple tasks
184+
ScheduledJobRegistry.register(mockTask);
185+
const mockTask2 = {
186+
id: "task-2",
187+
stop: vi.fn().mockResolvedValue(undefined),
188+
getStatus: vi.fn().mockReturnValue("scheduled"),
189+
} as unknown as ScheduledTask;
190+
ScheduledJobRegistry.register(mockTask2);
191+
192+
expect(ScheduledJobRegistry.getTaskCount()).toBe(2);
193+
expect(ScheduledJobRegistry.hasTask("test-task-1")).toBe(true);
194+
expect(ScheduledJobRegistry.hasTask("task-2")).toBe(true);
195+
196+
// Stop one task
197+
await ScheduledJobRegistry.stop("test-task-1");
198+
expect(ScheduledJobRegistry.getTaskCount()).toBe(1);
199+
expect(ScheduledJobRegistry.hasTask("test-task-1")).toBe(false);
200+
expect(ScheduledJobRegistry.hasTask("task-2")).toBe(true);
201+
202+
// Stop all remaining tasks
203+
await ScheduledJobRegistry.stopAll();
204+
expect(ScheduledJobRegistry.getTaskCount()).toBe(0);
205+
expect(ScheduledJobRegistry.hasTask("task-2")).toBe(false);
206+
});
207+
});
208+
});

0 commit comments

Comments
 (0)