feat: initial support for scheduled measurements#797
Open
MartinKolarik wants to merge 53 commits intomasterfrom
Open
feat: initial support for scheduled measurements#797MartinKolarik wants to merge 53 commits intomasterfrom
MartinKolarik wants to merge 53 commits intomasterfrom
Conversation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/measurement/store.ts (1)
195-203:⚠️ Potential issue | 🔴 CriticalFix the
undefinedresultsaccess causing the CI pipeline failure.
measurement.resultscan beundefinedfor records that don't have a fully populated shape (e.g., newly created scheduled measurements), causing a hard crash insidemarkFinishedByTimeout.🐛 Proposed fix
- const inProgressResults = measurement.results.filter(resultObject => resultObject.result.status === 'in-progress'); + const inProgressResults = (measurement.results ?? []).filter(resultObject => resultObject.result.status === 'in-progress');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/measurement/store.ts` around lines 195 - 203, In markFinishedByTimeout, protect against measurement.results being undefined by treating it as an empty array before iterating: when looping over measurements, replace direct access to measurement.results with a safe reference (e.g., const results = measurement.results ?? []) and use that for filtering and updating inProgress results; alternatively skip the results-handling block if measurement.results is falsy, and avoid mutating undefined. Ensure you update references in the same function (measurement, measurements, markFinishedByTimeout) so CI no longer crashes.
♻️ Duplicate comments (2)
src/schedule/executor.ts (2)
69-96: ValidateintervalSecondsbefore creating timers.No guard against
intervalSeconds <= 0:intervalMswould be0, thenext <= nowcheck would not advancenext,delaywould be0, andsetIntervalwould be created with a 0ms period — spinning hot.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/schedule/executor.ts` around lines 69 - 96, The createTimer method can create a 0ms interval when intervalSeconds <= 0 — validate intervalSeconds at the top of createTimer (the function named createTimer and its use of intervalMs and timers map) and reject non-positive values: either throw a clear Error or log and return without creating timers; ensure you check (intervalSeconds > 0) before computing intervalMs, and avoid calling setTimeout/setInterval or storing anything into this.timers when the value is invalid.
112-113:probe_limitis not applied before chunking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/schedule/executor.ts` around lines 112 - 113, The code chunks localProbes immediately using chunkSize (variable chunkSize and probesChunks) without first enforcing the configured probe_limit; before calling _.chunk, read the probe_limit from config (e.g. config.get<number>('measurement.probe_limit') or the appropriate key under measurement.limits), trim localProbes to that limit (localProbes = localProbes.slice(0, probeLimit)) and then perform _.chunk to create probesChunks so the probe_limit is applied correctly.
🧹 Nitpick comments (1)
src/schedule/executor.ts (1)
44-67: Schedule interval changes are silently ignored for running timers.
syncTimers()only creates a timer when!this.timers.has(schedule.id), so if a schedule'sintervalchanges in the DB the existing timer keeps firing at the old cadence. The timer is only recreated on disable → re-enable or server restart.Consider comparing the stored interval against the running one and recreating the timer when it differs, or at minimum add a comment documenting this limitation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/schedule/executor.ts` around lines 44 - 67, syncTimers currently only checks presence of schedule.id in this.timers so interval changes are ignored; update it to detect interval mismatches and recreate timers: store more metadata in this.timers (e.g., { id, interval, timerId }) instead of raw timer objects when calling createTimer, then in syncTimers compare schedule.interval to the stored interval for each running timer and if they differ clearInterval(timerId), delete the entry and call createTimer(schedule.id, schedule.interval) to recreate with the new cadence; ensure createTimer sets the stored interval metadata and clearInterval is called on the correct timerId.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/measurement/store.ts`:
- Around line 195-203: In markFinishedByTimeout, protect against
measurement.results being undefined by treating it as an empty array before
iterating: when looping over measurements, replace direct access to
measurement.results with a safe reference (e.g., const results =
measurement.results ?? []) and use that for filtering and updating inProgress
results; alternatively skip the results-handling block if measurement.results is
falsy, and avoid mutating undefined. Ensure you update references in the same
function (measurement, measurements, markFinishedByTimeout) so CI no longer
crashes.
---
Duplicate comments:
In `@src/schedule/executor.ts`:
- Around line 69-96: The createTimer method can create a 0ms interval when
intervalSeconds <= 0 — validate intervalSeconds at the top of createTimer (the
function named createTimer and its use of intervalMs and timers map) and reject
non-positive values: either throw a clear Error or log and return without
creating timers; ensure you check (intervalSeconds > 0) before computing
intervalMs, and avoid calling setTimeout/setInterval or storing anything into
this.timers when the value is invalid.
- Around line 112-113: The code chunks localProbes immediately using chunkSize
(variable chunkSize and probesChunks) without first enforcing the configured
probe_limit; before calling _.chunk, read the probe_limit from config (e.g.
config.get<number>('measurement.probe_limit') or the appropriate key under
measurement.limits), trim localProbes to that limit (localProbes =
localProbes.slice(0, probeLimit)) and then perform _.chunk to create
probesChunks so the probe_limit is applied correctly.
---
Nitpick comments:
In `@src/schedule/executor.ts`:
- Around line 44-67: syncTimers currently only checks presence of schedule.id in
this.timers so interval changes are ignored; update it to detect interval
mismatches and recreate timers: store more metadata in this.timers (e.g., { id,
interval, timerId }) instead of raw timer objects when calling createTimer, then
in syncTimers compare schedule.interval to the stored interval for each running
timer and if they differ clearInterval(timerId), delete the entry and call
createTimer(schedule.id, schedule.interval) to recreate with the new cadence;
ensure createTimer sets the stored interval metadata and clearInterval is called
on the correct timerId.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/measurement/store.tssrc/schedule/executor.tstest/tests/unit/measurement/store.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
test/tests/integration/schedule/stream-schedule.test.ts (1)
87-92:forEachcallback implicitly returns — Biome lint error.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/tests/integration/schedule/stream-schedule.test.ts` around lines 87 - 92, The Biome lint error is caused by the implicit return from the arrow used inside insertSchedule; change the schedule.configurations.forEach callback to not return a value (e.g., use a block-bodied arrow: config => { configurationIds.add(config.id); } or replace the forEach with a for...of loop) so the callback is explicitly void and only mutates configurationIds while leaving scheduleIds handling unchanged.
🧹 Nitpick comments (1)
test/tests/integration/schedule/stream-schedule.test.ts (1)
31-31:sandbox.restore()is never called — consider adding anafterEachorafterrestore.
sinon.createSandbox()is used, andbeforeEachonly callssandbox.resetHistory(). If future tests stub real methods throughsandbox, those stubs will leak across the suite. Addingsandbox.restore()in theafterEachis the standard guard.♻️ Proposed fix
afterEach(async () => { + sandbox.restore(); if (configurationIds.size) {Also applies to: 39-57
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/tests/integration/schedule/stream-schedule.test.ts` at line 31, The test suite creates a Sinon sandbox (sandbox = sinon.createSandbox()) but only calls sandbox.resetHistory() in beforeEach, so stubs/mocks can leak; add an afterEach hook that calls sandbox.restore() to fully restore and remove any stubs created via sandbox, keeping the existing beforeEach sandbox.resetHistory() intact and ensuring cleanup after each test (reference the sandbox variable and the existing beforeEach).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/tests/integration/schedule/stream-schedule.test.ts`:
- Line 88: Remove the debug console.log calls that pollute CI output: delete the
console.log(`Inserting schedule ${schedule.id}`) call inside insertSchedule and
remove the stray console.log in the first test case (the one around the initial
test at Line ~129). Search for console.log usages in stream-schedule.test.ts and
delete these two debug prints so tests run without extra logging.
---
Duplicate comments:
In `@test/tests/integration/schedule/stream-schedule.test.ts`:
- Around line 87-92: The Biome lint error is caused by the implicit return from
the arrow used inside insertSchedule; change the schedule.configurations.forEach
callback to not return a value (e.g., use a block-bodied arrow: config => {
configurationIds.add(config.id); } or replace the forEach with a for...of loop)
so the callback is explicitly void and only mutates configurationIds while
leaving scheduleIds handling unchanged.
---
Nitpick comments:
In `@test/tests/integration/schedule/stream-schedule.test.ts`:
- Line 31: The test suite creates a Sinon sandbox (sandbox =
sinon.createSandbox()) but only calls sandbox.resetHistory() in beforeEach, so
stubs/mocks can leak; add an afterEach hook that calls sandbox.restore() to
fully restore and remove any stubs created via sandbox, keeping the existing
beforeEach sandbox.resetHistory() intact and ensuring cleanup after each test
(reference the sandbox variable and the existing beforeEach).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/schedule/executor.tstest/tests/integration/schedule/stream-schedule.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/schedule/executor.ts
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/schedule/executor.ts (2)
123-124:⚠️ Potential issue | 🔴 Critical
probe_limitfrom the schedule is never enforced.
localProbesis chunked byauthenticatedTestsPerMeasurementbut never sliced byschedule.probe_limit. All matching probes are dispatched regardless of the configured limit.🛡️ Proposed fix
+ const limitedProbes = schedule.probe_limit + ? localProbes.slice(0, schedule.probe_limit) + : localProbes; const chunkSize = config.get<number>('measurement.limits.authenticatedTestsPerMeasurement'); - const probesChunks = _.chunk(localProbes, chunkSize); + const probesChunks = _.chunk(limitedProbes, chunkSize);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/schedule/executor.ts` around lines 123 - 124, The code currently chunks localProbes using chunkSize but never enforces schedule.probe_limit, so all matching probes are dispatched; update the logic (in the function using localProbes, probesChunks, and chunkSize) to first compute an effectiveLimit = schedule.probe_limit (if present) and then slice localProbes to localProbes.slice(0, effectiveLimit) (or use Math.min(effectiveLimit, localProbes.length)) before calling _.chunk, ensuring only up to probe_limit probes are considered when building probesChunks.
76-108:⚠️ Potential issue | 🔴 CriticalGuard against non-positive
intervalSeconds— infinite loop risk at Line 87.If
intervalSecondsis0(or negative),intervalMsis0and thewhile (next <= now)loop on Line 87 never terminates, freezing the process. Additionally,_.random(0, <negative>)has undefined behavior with lodash.🛡️ Proposed guard
private createTimer (scheduleId: string, intervalSeconds: number) { + if (!intervalSeconds || intervalSeconds <= 0) { + logger.warn(`Skipping schedule ${scheduleId}: invalid interval ${intervalSeconds}s.`); + return; + } + const sec = _.random(0, Math.min(intervalSeconds, 59));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/schedule/executor.ts` around lines 76 - 108, The createTimer method must guard against non-positive intervalSeconds to avoid an infinite loop and lodash random errors: at the start of createTimer (before using _.random and computing intervalMs) validate that intervalSeconds is a positive integer (>0); if not, log an error via logger.error (including scheduleId) and return without scheduling, or throw a clear error depending on existing behavior; ensure you use a safe bound for _.random (e.g., Math.max(1, Math.min(intervalSeconds, 59))) so you never call _.random with a negative max, and keep the rest of the logic using intervalMs = intervalSeconds * 1000 unchanged so fireSchedule and this.timers handling remain correct.test/tests/integration/schedule/stream-schedule.test.ts (1)
89-89:⚠️ Potential issue | 🟡 Minor
forEachcallback implicitly returns the result ofSet.add.Biome flags
lint/suspicious/useIterableCallbackReturn. Use a block body or afor...ofloop.Fix
- schedule.configurations.forEach(config => configurationIds.add(config.id)); + for (const config of schedule.configurations) { + configurationIds.add(config.id); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/tests/integration/schedule/stream-schedule.test.ts` at line 89, The forEach callback on schedule.configurations implicitly returns the value from configurationIds.add which triggers the lint rule; replace the forEach usage with an explicit iterable loop or a block-bodied callback to avoid returning a value—e.g., iterate with for (const config of schedule.configurations) { configurationIds.add(config.id); } or change the arrow to a block body that calls configurationIds.add(config.id); target the schedule.configurations.forEach call and the configurationIds.add invocation when making the change.
🧹 Nitpick comments (3)
test/tests/integration/schedule/stream-schedule.test.ts (2)
44-58: Cleanup relies on loader sync to disarm executor timers — ordering is correct but fragile.The
afterEachdeletes DB rows, then callsgetStreamScheduleLoader().sync(), which triggers the executor'ssyncTimersto clear all timers. Ifsync()were ever made lazy or debounced, leftover timers from one test would leak into the next.A defensive addition would be to also stop the executor directly, or at minimum add a comment documenting why the sync ordering matters.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/tests/integration/schedule/stream-schedule.test.ts` around lines 44 - 58, The afterEach relies on getStreamScheduleLoader().sync() to disarm executor timers which is fragile; update the teardown to explicitly stop the executor before deleting DB rows by calling the executor's shutdown/stop method (e.g., StreamScheduleExecutor.stop() or similar) obtained from getStreamScheduleLoader() or a getStreamScheduleExecutor() helper, then proceed with deleting gp_schedule and gp_schedule_configuration and calling getStreamScheduleLoader().sync(); if no explicit stop exists, add a method to the executor to clear timers and call it here, and add a short comment explaining the ordering requirement to prevent timer leakage between tests.
461-476: Interval-change assertion is loose and timing-dependent.After changing the interval from 1s to 3s and ticking 5000ms, the test asserts
callCount <= 2. With a 3-second interval and up to 59-second random alignment offset (capped to 3 for this interval), the first fire could land anywhere in the first 3 seconds of the tick window, making the count either 1 or 2. This is fine in practice with fake timers, but the assertion documents intent weakly — it doesn't prove the old 1s timer was actually replaced (a stuck 1s timer would produce ~5 calls in 5s, so it would fail, which is the real check).Consider adding a brief comment clarifying that the upper bound rules out the old 1s cadence.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/tests/integration/schedule/stream-schedule.test.ts` around lines 461 - 476, The test's post-update assertion (using requestHandlerStub.callCount <= 2) is loose and timing-dependent; add a brief clarifying comment immediately before that assertion describing the intent: after updating schedule-1 via dashboardClient(...).update(...) and calling getStreamScheduleLoader().sync(), we expect the 1s timer to be replaced by the 3s interval so that, even with random alignment, ticking clock.tickAsyncStepped(5000) should produce at most 2 calls; reference requestHandlerStub, clock.tickAsyncStepped, dashboardClient('gp_schedule').where({ id: 'schedule-1' }).update(...), and getStreamScheduleLoader().sync() in the comment to make the intent explicit.src/schedule/executor.ts (1)
94-107: Timer id stored in map transitions fromsetTimeouttosetInterval—stop()must handle both.
setTimer(first)at Line 107 stores thesetTimeouthandle. When the timeout fires, it's silently replaced by thesetIntervalhandle at Line 104.stop()always callsclearInterval, which works in Node.js for both handle types, but if the code is ever ported or a stricter runtime is used, this would break. More concretely: ifsyncTimersfires during the initial delay and clears thesetTimeout, the old handle is gone, but no danglingsetIntervalis created — so the lifecycle is correct today.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/schedule/executor.ts` around lines 94 - 107, The timers map currently stores only the raw handle via setTimer(id) causing ambiguity between a setTimeout and setInterval handle; update the setTimer function and the timers map entries to include the handle type (e.g., { interval: intervalSeconds, id, kind: 'timeout'|'interval' }) and set kind='timeout' when calling setTimer(first) and kind='interval' when calling setTimer(interval). Then update stop() to read the kind and call clearTimeout for 'timeout' handles and clearInterval for 'interval' handles (or guard both calls based on kind) so the correct clear function is always used for the stored handle.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/schedule/executor.ts`:
- Line 80: The log message is misleading because `sec` is the aligned
second-of-minute when the timer fires, not the interval; update the logger.debug
call (the line that references `scheduleId` and `sec` in
src/schedule/executor.ts) to describe `sec` as the scheduled second or fire time
(e.g., "at second ${sec} of each minute" or "fires at ${sec}s past the minute"),
so the message accurately reflects that `sec` is the aligned second rather than
the interval.
---
Duplicate comments:
In `@src/schedule/executor.ts`:
- Around line 123-124: The code currently chunks localProbes using chunkSize but
never enforces schedule.probe_limit, so all matching probes are dispatched;
update the logic (in the function using localProbes, probesChunks, and
chunkSize) to first compute an effectiveLimit = schedule.probe_limit (if
present) and then slice localProbes to localProbes.slice(0, effectiveLimit) (or
use Math.min(effectiveLimit, localProbes.length)) before calling _.chunk,
ensuring only up to probe_limit probes are considered when building
probesChunks.
- Around line 76-108: The createTimer method must guard against non-positive
intervalSeconds to avoid an infinite loop and lodash random errors: at the start
of createTimer (before using _.random and computing intervalMs) validate that
intervalSeconds is a positive integer (>0); if not, log an error via
logger.error (including scheduleId) and return without scheduling, or throw a
clear error depending on existing behavior; ensure you use a safe bound for
_.random (e.g., Math.max(1, Math.min(intervalSeconds, 59))) so you never call
_.random with a negative max, and keep the rest of the logic using intervalMs =
intervalSeconds * 1000 unchanged so fireSchedule and this.timers handling remain
correct.
In `@test/tests/integration/schedule/stream-schedule.test.ts`:
- Line 89: The forEach callback on schedule.configurations implicitly returns
the value from configurationIds.add which triggers the lint rule; replace the
forEach usage with an explicit iterable loop or a block-bodied callback to avoid
returning a value—e.g., iterate with for (const config of
schedule.configurations) { configurationIds.add(config.id); } or change the
arrow to a block body that calls configurationIds.add(config.id); target the
schedule.configurations.forEach call and the configurationIds.add invocation
when making the change.
---
Nitpick comments:
In `@src/schedule/executor.ts`:
- Around line 94-107: The timers map currently stores only the raw handle via
setTimer(id) causing ambiguity between a setTimeout and setInterval handle;
update the setTimer function and the timers map entries to include the handle
type (e.g., { interval: intervalSeconds, id, kind: 'timeout'|'interval' }) and
set kind='timeout' when calling setTimer(first) and kind='interval' when calling
setTimer(interval). Then update stop() to read the kind and call clearTimeout
for 'timeout' handles and clearInterval for 'interval' handles (or guard both
calls based on kind) so the correct clear function is always used for the stored
handle.
In `@test/tests/integration/schedule/stream-schedule.test.ts`:
- Around line 44-58: The afterEach relies on getStreamScheduleLoader().sync() to
disarm executor timers which is fragile; update the teardown to explicitly stop
the executor before deleting DB rows by calling the executor's shutdown/stop
method (e.g., StreamScheduleExecutor.stop() or similar) obtained from
getStreamScheduleLoader() or a getStreamScheduleExecutor() helper, then proceed
with deleting gp_schedule and gp_schedule_configuration and calling
getStreamScheduleLoader().sync(); if no explicit stop exists, add a method to
the executor to clear timers and call it here, and add a short comment
explaining the ordering requirement to prevent timer leakage between tests.
- Around line 461-476: The test's post-update assertion (using
requestHandlerStub.callCount <= 2) is loose and timing-dependent; add a brief
clarifying comment immediately before that assertion describing the intent:
after updating schedule-1 via dashboardClient(...).update(...) and calling
getStreamScheduleLoader().sync(), we expect the 1s timer to be replaced by the
3s interval so that, even with random alignment, ticking
clock.tickAsyncStepped(5000) should produce at most 2 calls; reference
requestHandlerStub, clock.tickAsyncStepped,
dashboardClient('gp_schedule').where({ id: 'schedule-1' }).update(...), and
getStreamScheduleLoader().sync() in the comment to make the intent explicit.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/tests/unit/measurement/store.test.ts (1)
758-758: Unnecessary stub setup — offloader is never called in this test.
offloaderGetMeasurementStringStub.resolves(...)on line 758 is dead setup: the test immediately assertsoffloaderGetMeasurementStringStub.callCountequals 0, so this stub resolution is never exercised.♻️ Suggested cleanup
- offloaderGetMeasurementStringStub.resolves(buildMinimalMeasurementString('SOME_ID', { from: 'db' })); - const redisValue = buildMinimalMeasurementString('SOME_ID', { from: 'redis' });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/tests/unit/measurement/store.test.ts` at line 758, The test sets up offloaderGetMeasurementStringStub.resolves(buildMinimalMeasurementString('SOME_ID', { from: 'db' })) but never exercises the offloader (the test later asserts offloaderGetMeasurementStringStub.callCount === 0), so remove this unnecessary stub setup from the test (or move it to a test that actually invokes offloaderGetMeasurementStringStub); update the test to avoid resolving an unused stub and keep only stubs that are called (referencing offloaderGetMeasurementStringStub and buildMinimalMeasurementString to locate the code).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/tests/unit/measurement/store.test.ts`:
- Line 758: The test sets up
offloaderGetMeasurementStringStub.resolves(buildMinimalMeasurementString('SOME_ID',
{ from: 'db' })) but never exercises the offloader (the test later asserts
offloaderGetMeasurementStringStub.callCount === 0), so remove this unnecessary
stub setup from the test (or move it to a test that actually invokes
offloaderGetMeasurementStringStub); update the test to avoid resolving an unused
stub and keep only stubs that are called (referencing
offloaderGetMeasurementStringStub and buildMinimalMeasurementString to locate
the code).
There was a problem hiding this comment.
♻️ Duplicate comments (3)
src/schedule/executor.ts (3)
80-80:⚠️ Potential issue | 🟡 MinorFix incorrect interval unit in timer debug log.
Line 80 logs
${intervalMs}s, butintervalMsis milliseconds.Suggested fix
- logger.debug(`Creating schedule timer for ${scheduleId} at ${sec}s with ${intervalMs}s interval.`); + logger.debug(`Creating schedule timer for ${scheduleId}: aligned second=${sec}, interval=${intervalMs}ms.`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/schedule/executor.ts` at line 80, The debug message on creation of the schedule timer mistakenly prints the interval as seconds using `${intervalMs}s`; update the log to reflect the correct unit by either appending "ms" (`${intervalMs}ms`) or converting to seconds (`${(intervalMs/1000)}s`) so the message accurately represents the `intervalMs` value; locate the logger.debug call that references `scheduleId` and `intervalMs` and change the string accordingly.
123-125:⚠️ Potential issue | 🔴 CriticalEnforce
schedule.probe_limitbefore chunking and propagate the limit.Line 123-Line 125 chunks all filtered probes, and Line 139 keeps
limitunset. This can dispatch/store more probes than the schedule allows.Suggested fix
- const chunkSize = config.get<number>('measurement.limits.authenticatedTestsPerMeasurement'); - const probesChunks = _.chunk(localProbes, chunkSize); + const chunkSize = config.get<number>('measurement.limits.authenticatedTestsPerMeasurement'); + const limitedProbes = schedule.probe_limit != null + ? localProbes.slice(0, schedule.probe_limit) + : localProbes; + const probesChunks = _.chunk(limitedProbes, chunkSize); @@ - limit: undefined, + limit: schedule.probe_limit ?? undefined,Also applies to: 139-144
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/schedule/executor.ts` around lines 123 - 125, Before chunking localProbes, apply the schedule.probe_limit cap and propagate that limit into the per-measurement payload: compute an effectiveLimit = Math.min(schedule.probe_limit || Infinity, config.get<number>('measurement.limits.authenticatedTestsPerMeasurement')) then slice localProbes = localProbes.slice(0, effectiveLimit) before calling _.chunk (so probesChunks respects the schedule limit) and ensure any place that constructs measurement/send calls (the variable named limit around lines 139-144) is set to effectiveLimit so the dispatched/stored measurements carry the enforced limit.
76-89:⚠️ Potential issue | 🟠 MajorGuard invalid intervals before timer alignment and scheduling.
Line 77-Line 89 assumes a valid positive interval. A non-finite or non-positive value can break the
whileprogression and create effectively immediate repeat timers.Suggested fix
private createTimer (scheduleId: string, intervalSeconds: number) { + if (!Number.isFinite(intervalSeconds) || intervalSeconds <= 0) { + logger.warn('Skipping schedule timer creation due to invalid interval.', { scheduleId, intervalSeconds }); + return; + } + const sec = _.random(0, Math.min(intervalSeconds, 59)); const intervalMs = intervalSeconds * 1000;#!/bin/bash # Verify whether schedule interval is guaranteed positive before reaching executor. # If no upstream validation exists, this guard should remain in executor. rg -nP -C3 '\binterval\b' src/schedule rg -nP -C3 'validate|schema|zod|joi|positive|>\\s*0|CHECK' src/schedule migrations config🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/schedule/executor.ts` around lines 76 - 89, The createTimer function assumes intervalSeconds is a finite positive number; add a guard at the start of createTimer (method createTimer(scheduleId: string, intervalSeconds: number)) to validate Number.isFinite(intervalSeconds) and intervalSeconds > 0, and handle invalid values by either throwing a clear error or logging and returning/avoiding timer creation (so the while loop and interval math never run with non-finite/non-positive values); ensure any chosen handling is consistent with surrounding executor behavior (e.g., log via logger and skip scheduling) so callers can detect the failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/schedule/executor.ts`:
- Line 80: The debug message on creation of the schedule timer mistakenly prints
the interval as seconds using `${intervalMs}s`; update the log to reflect the
correct unit by either appending "ms" (`${intervalMs}ms`) or converting to
seconds (`${(intervalMs/1000)}s`) so the message accurately represents the
`intervalMs` value; locate the logger.debug call that references `scheduleId`
and `intervalMs` and change the string accordingly.
- Around line 123-125: Before chunking localProbes, apply the
schedule.probe_limit cap and propagate that limit into the per-measurement
payload: compute an effectiveLimit = Math.min(schedule.probe_limit || Infinity,
config.get<number>('measurement.limits.authenticatedTestsPerMeasurement')) then
slice localProbes = localProbes.slice(0, effectiveLimit) before calling _.chunk
(so probesChunks respects the schedule limit) and ensure any place that
constructs measurement/send calls (the variable named limit around lines
139-144) is set to effectiveLimit so the dispatched/stored measurements carry
the enforced limit.
- Around line 76-89: The createTimer function assumes intervalSeconds is a
finite positive number; add a guard at the start of createTimer (method
createTimer(scheduleId: string, intervalSeconds: number)) to validate
Number.isFinite(intervalSeconds) and intervalSeconds > 0, and handle invalid
values by either throwing a clear error or logging and returning/avoiding timer
creation (so the while loop and interval math never run with
non-finite/non-positive values); ensure any chosen handling is consistent with
surrounding executor behavior (e.g., log via logger and skip scheduling) so
callers can detect the failure.
MartinKolarik
force-pushed
the
gh-291
branch
from
1aaaf45 to
3c32777
Compare
Member
Author
|
@alexey-yarmosh please review after jsdelivr/globalping-probe#350 |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/schedule/executor.ts (2)
48-64:⚠️ Potential issue | 🔴 CriticalValidate schedule interval before creating timers.
intervalSecondsis used directly for random second selection, alignment math, andsetInterval. Non-finite or<= 0values can produce invalid scheduling behavior (including a non-advancing alignment loop).Suggested fix
private createTimer (scheduleId: string, intervalSeconds: number) { + if (!Number.isFinite(intervalSeconds) || intervalSeconds <= 0) { + logger.warn(`Skipping schedule ${scheduleId}: invalid interval ${intervalSeconds}s.`); + return; + } + const sec = _.random(0, Math.min(intervalSeconds, 59)); const intervalMs = intervalSeconds * 1000;Also applies to: 76-89
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/schedule/executor.ts` around lines 48 - 64, Validate the schedule interval (e.g., schedule.interval / intervalSeconds) is a finite number > 0 before using it for alignment math, random second selection, or calling createTimer; if invalid, skip creating timers (and clear/delete any existing timer for that schedule id) and optionally log or mark the schedule as misconfigured. Update the logic around the timers map and the createTimer invocation (and the interval comparison with existing timer.interval) to first check Number.isFinite(intervalSeconds) && intervalSeconds > 0 and only proceed when true; apply the same validation to the analogous block at lines 76-89.
117-125:⚠️ Potential issue | 🔴 CriticalApply
schedule.probe_limitbefore chunking and creation.The current flow chunks all filtered probes, so scheduled runs can exceed the configured probe limit.
Suggested fix
const localProbes = this.getFilteredLocalProbes(schedule); + const limitedProbes = schedule.probe_limit != null + ? localProbes.slice(0, schedule.probe_limit) + : localProbes; - if (localProbes.length === 0) { + if (limitedProbes.length === 0) { return; } const chunkSize = config.get<number>('measurement.limits.authenticatedTestsPerMeasurement'); - const probesChunks = _.chunk(localProbes, chunkSize); + const probesChunks = _.chunk(limitedProbes, chunkSize); @@ - limit: undefined, + limit: schedule.probe_limit ?? undefined,Also applies to: 131-140
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/schedule/executor.ts` around lines 117 - 125, The code currently chunks all probes returned by getFilteredLocalProbes(schedule) which can exceed schedule.probe_limit; before computing chunkSize and calling _.chunk, truncate localProbes to at most schedule.probe_limit (if schedule.probe_limit is set) so only that many probes are chunked and processed; update the same logic in the other chunking location in this file (the later block that also constructs probesChunks using config.get('measurement.limits.authenticatedTestsPerMeasurement')) to apply the same truncation to its localProbes variable.
🧹 Nitpick comments (1)
migrations/dashboard/create-tables.js.sql (1)
116-117: Add an index for schedule pickup queries.Scheduler reads will typically filter by
enabledandmode(and possiblytime_series_enabled). Without an index, this will degrade to table scans as schedules grow.Suggested index
CREATE TABLE IF NOT EXISTS gp_schedule ( @@ enabled BOOLEAN NOT NULL DEFAULT 1, time_series_enabled BOOLEAN NOT NULL DEFAULT 0, notes TEXT NULL, + KEY gp_schedule_enabled_mode_idx (enabled, mode, time_series_enabled) ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@migrations/dashboard/create-tables.js.sql` around lines 116 - 117, Add a composite index to support scheduler reads that filter by the enabled, mode, and optionally time_series_enabled columns: create a (concurrent) index on (enabled, mode, time_series_enabled) for the schedules table (e.g., idx_schedules_enabled_mode_time_series) right after the table creation, or use a partial index (WHERE enabled = true) if most rows are enabled=false to save space; reference the enabled, mode, and time_series_enabled column names when adding the index.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@migrations/dashboard/create-tables.js.sql`:
- Line 112: The `interval` column currently declared as `INT UNSIGNED NOT NULL`
allows 0; add a CHECK constraint to disallow zero values (e.g., include a
constraint like ADD CONSTRAINT chk_interval_positive CHECK (`interval` > 0) in
the CREATE TABLE definition or run an ALTER TABLE to add that constraint) so
schedules cannot be created with interval = 0; reference the `interval` column
and name the constraint (e.g., chk_interval_positive) to make it clear and
maintainable.
- Around line 114-115: The CHECK using json_valid(...) only verifies JSON
syntax; update the column CHECK constraints for locations and
measurement_options so they assert both valid JSON and the expected JSON type:
for locations ensure json_valid(locations) AND JSON_TYPE(JSON_EXTRACT(locations,
'$')) = 'ARRAY', and for measurement_options ensure
json_valid(measurement_options) AND JSON_TYPE(JSON_EXTRACT(measurement_options,
'$')) = 'OBJECT'; modify the column definitions that currently reference
locations and measurement_options to include these stricter CHECK expressions so
runtime code receives the expected shapes.
In `@src/schedule/executor.ts`:
- Around line 98-104: The current timer logic uses setInterval inside the first
timeout which calls this.fireSchedule(scheduleId) without waiting for
completion, allowing overlapping runs; replace the setInterval approach with a
self-rescheduling pattern: call this.fireSchedule(scheduleId) and await its
promise (catch errors via onError), then schedule the next run via
setTimeout(..., intervalMs) (unref the timeout) and pass that timer into
setTimer; update the block around the first variable and the logic that
currently creates setInterval so the next invocation is scheduled only after the
prior fireSchedule(scheduleId) promise settles.
---
Duplicate comments:
In `@src/schedule/executor.ts`:
- Around line 48-64: Validate the schedule interval (e.g., schedule.interval /
intervalSeconds) is a finite number > 0 before using it for alignment math,
random second selection, or calling createTimer; if invalid, skip creating
timers (and clear/delete any existing timer for that schedule id) and optionally
log or mark the schedule as misconfigured. Update the logic around the timers
map and the createTimer invocation (and the interval comparison with existing
timer.interval) to first check Number.isFinite(intervalSeconds) &&
intervalSeconds > 0 and only proceed when true; apply the same validation to the
analogous block at lines 76-89.
- Around line 117-125: The code currently chunks all probes returned by
getFilteredLocalProbes(schedule) which can exceed schedule.probe_limit; before
computing chunkSize and calling _.chunk, truncate localProbes to at most
schedule.probe_limit (if schedule.probe_limit is set) so only that many probes
are chunked and processed; update the same logic in the other chunking location
in this file (the later block that also constructs probesChunks using
config.get('measurement.limits.authenticatedTestsPerMeasurement')) to apply the
same truncation to its localProbes variable.
---
Nitpick comments:
In `@migrations/dashboard/create-tables.js.sql`:
- Around line 116-117: Add a composite index to support scheduler reads that
filter by the enabled, mode, and optionally time_series_enabled columns: create
a (concurrent) index on (enabled, mode, time_series_enabled) for the schedules
table (e.g., idx_schedules_enabled_mode_time_series) right after the table
creation, or use a partial index (WHERE enabled = true) if most rows are
enabled=false to save space; reference the enabled, mode, and
time_series_enabled column names when adding the index.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fbb717b5-76b0-4ea2-aedc-ba80ad0b09b9
📒 Files selected for processing (2)
migrations/dashboard/create-tables.js.sqlsrc/schedule/executor.ts
alexey-yarmosh
approved these changes
Mar 5, 2026
Member
|
It would be nice to also add |
Member
Author
|
Yes, I was using the migrations here during dev, but will add them now. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/server.ts (1)
101-106:⚠️ Potential issue | 🟡 MinorMissing graceful shutdown for
scheduleExecutor.The
StreamScheduleExecutorhas astop()method that clears active timers and removes listeners, but it's never called during server termination. This could result in timer callbacks firing after other components (Redis clients, WS server) have been torn down.Proposed fix
reconnectProbes(fetchRawSockets); // Disconnect probes shortly before shutdown to prevent data loss. termListener.on('terminating', ({ delay }) => setTimeout(() => void disconnectProbes(fetchRawSockets, 0), delay - 10000)); + termListener.on('terminating', () => scheduleExecutor.stop());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/server.ts` around lines 101 - 106, The StreamScheduleExecutor instance (scheduleExecutor) isn't stopped on shutdown, so its timers/listeners can fire after other components are torn down; update the termination handling to call scheduleExecutor.stop() inside the termListener 'terminating' handler (before calling disconnectProbes(fetchRawSockets, 0)) so timers are cleared and listeners removed during shutdown, e.g., invoke scheduleExecutor.stop() in the same setTimeout callback (or immediately in the handler) to ensure the executor is cleanly stopped prior to Redis/WS teardown.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docker-compose.dev.yml`:
- Around line 133-144: The new time-series-1-migrator service is not
self-contained because it mounts only ./:/app and assumes local node_modules;
update the time-series-1-migrator service (image node:22-alpine, working_dir
/app, volumes ./:/app, command npm run knex:time-series-1 migrate:latest) so it
installs dependencies inside the container before running the migration (e.g.,
run npm ci or npm install in the container or add a dedicated bind for
node_modules) so the project CLI binary for knex is available when executing the
migration.
---
Outside diff comments:
In `@src/lib/server.ts`:
- Around line 101-106: The StreamScheduleExecutor instance (scheduleExecutor)
isn't stopped on shutdown, so its timers/listeners can fire after other
components are torn down; update the termination handling to call
scheduleExecutor.stop() inside the termListener 'terminating' handler (before
calling disconnectProbes(fetchRawSockets, 0)) so timers are cleared and
listeners removed during shutdown, e.g., invoke scheduleExecutor.stop() in the
same setTimeout callback (or immediately in the handler) to ensure the executor
is cleanly stopped prior to Redis/WS teardown.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0000788f-06b3-4115-a9ed-e9a8a7a486ca
📒 Files selected for processing (5)
.github/workflows/ci.yml.github/workflows/e2e.ymldocker-compose.dev.ymlmigrations/dashboard/create-tables.js.sqlsrc/lib/server.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- migrations/dashboard/create-tables.js.sql
- .github/workflows/e2e.yml
Comment on lines
+133
to
+144
| time-series-1-migrator: | ||
| image: node:22-alpine | ||
| working_dir: /app | ||
| depends_on: | ||
| time-series-1: | ||
| condition: service_healthy | ||
| environment: | ||
| - NODE_ENV=development | ||
| - NODE_CONFIG={"timeSeriesDb":{"connection":{"host":"time-series-1","port":5432}}} | ||
| volumes: | ||
| - ./:/app | ||
| command: sh -c "npm run knex:time-series-1 migrate:latest" |
There was a problem hiding this comment.
Make the new migrator self-contained.
With only .:/app mounted, this container assumes the checkout already has dependencies installed locally. On a clean repo, npm run knex:time-series-1 will not find the project CLI binaries, so the dev bootstrap fails before creating the schema.
Proposed fix
time-series-1-migrator:
image: node:22-alpine
working_dir: /app
depends_on:
time-series-1:
condition: service_healthy
environment:
- NODE_ENV=development
- NODE_CONFIG={"timeSeriesDb":{"connection":{"host":"time-series-1","port":5432}}}
volumes:
- ./:/app
- command: sh -c "npm run knex:time-series-1 migrate:latest"
+ - /app/node_modules
+ command: sh -c "npm ci && npm run knex:time-series-1 migrate:latest"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| time-series-1-migrator: | |
| image: node:22-alpine | |
| working_dir: /app | |
| depends_on: | |
| time-series-1: | |
| condition: service_healthy | |
| environment: | |
| - NODE_ENV=development | |
| - NODE_CONFIG={"timeSeriesDb":{"connection":{"host":"time-series-1","port":5432}}} | |
| volumes: | |
| - ./:/app | |
| command: sh -c "npm run knex:time-series-1 migrate:latest" | |
| time-series-1-migrator: | |
| image: node:22-alpine | |
| working_dir: /app | |
| depends_on: | |
| time-series-1: | |
| condition: service_healthy | |
| environment: | |
| - NODE_ENV=development | |
| - NODE_CONFIG={"timeSeriesDb":{"connection":{"host":"time-series-1","port":5432}}} | |
| volumes: | |
| - ./:/app | |
| - /app/node_modules | |
| command: sh -c "npm ci && npm run knex:time-series-1 migrate:latest" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docker-compose.dev.yml` around lines 133 - 144, The new
time-series-1-migrator service is not self-contained because it mounts only
./:/app and assumes local node_modules; update the time-series-1-migrator
service (image node:22-alpine, working_dir /app, volumes ./:/app, command npm
run knex:time-series-1 migrate:latest) so it installs dependencies inside the
container before running the migration (e.g., run npm ci or npm install in the
container or add a dedicated bind for node_modules) so the project CLI binary
for knex is available when executing the migration.
a03a68e to
0460718
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of #291, but the system is intentionally generic to support custom user-scheduled measurements as well in the future.
Key concepts
Scheduledefines a single logical target, how often it's tested, and which probes it runs on.Configurationdefines a measurement template. E.g., if we wanted to test CDN providers with three different file sizes, that would be three configurations within a single schedule.Scheduling is handled by the API in the end, as it is much more efficient (in terms of storage, API requests for multiple results, etc.) to group multiple probe results into a single measurement. With probe-level scheduling, that would be rather difficult. Two scheduling modes: