Add background scheduler for monitor checks - #1
Merged
Conversation
Implements the core monitoring engine: a cron job that runs every minute, checks all due monitors via HTTP GET with 5s timeout, logs results to check_logs, and updates monitor state (last_status, consecutive_failures, is_alerted) atomically within a transaction. - node-cron scheduler with overlap prevention - Transaction support added to db.js (withTransaction) - Checks service: runCheck, processCheck, checkAllDueMonitors - Concurrent checks via Promise.allSettled (one failure doesn't block others) - Alert flag set when consecutive_failures >= threshold - Recovery resets consecutive_failures and is_alerted Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (11)
📝 WalkthroughWalkthroughAdds a background monitor scheduler using ChangesMonitor Scheduler Pipeline
Sequence Diagram(s)sequenceDiagram
participant Cron as node-cron
participant Scheduler as scheduler/index.js
participant Service as checks.service.js
participant Safety as resolveAndValidate
participant HTTP as fetch
participant TX as withTransaction
participant DB as PostgreSQL
Cron->>Scheduler: tick every minute
Scheduler->>Scheduler: check isRunning guard
Scheduler->>Service: checkAllDueMonitors()
Service->>DB: findDueMonitors() SELECT
DB-->>Service: monitor rows
loop each monitor (p-limit throttled, Promise.allSettled)
Service->>Service: processCheck(monitor)
Service->>Safety: resolveAndValidate(url)
alt URL safe
Safety-->>Service: {safe: true}
Service->>HTTP: GET with AbortSignal.timeout(5000)
HTTP-->>Service: response
Service->>Service: cancel response.body
Service->>TX: withTransaction(callback)
TX->>DB: BEGIN
TX->>DB: insertCheckLog(client, monitorId, checkResult)
TX->>DB: updateMonitorAfterCheck(client, monitorId, updates)
TX->>DB: COMMIT
else URL unsafe
Safety-->>Service: {safe: false, reason}
Service-->>Service: return {status: down, message: Blocked...}
end
end
Scheduler->>Scheduler: reset isRunning in finally
Note over Cron,DB: Separate daily job at 03:00
Cron->>Scheduler: tick at 03:00
Scheduler->>DB: deleteExpiredCheckLogs()
DB-->>Scheduler: deleted count
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~28 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
context1/feature-specs/08-scheduler.md (2)
35-37: 💤 Low valueAdd language identifier to shell code block.
Markdown linting requires fenced code blocks to specify a language. Add
shorbashidentifier.Proposed fix
-``` +```sh cd backend && npm install node-cron -``` +```🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@context1/feature-specs/08-scheduler.md` around lines 35 - 37, The fenced code block containing the npm install command is missing a language identifier, which violates markdown linting rules. Add `sh` as the language identifier immediately after the opening triple backticks (before the newline) so the code block properly specifies that it contains shell commands. This applies to the code block in the Scheduler section that shows the cd backend && npm install node-cron command.Source: Linters/SAST tools
249-249: 💤 Low valueUse hyphen for compound adjective modifying "logic".
"failed DB writes" is a compound adjective that should be hyphenated when it directly precedes the noun. Consider: "Retry logic for failed-DB writes" or restructure to "Retry logic for DB write failures."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@context1/feature-specs/08-scheduler.md` at line 249, The line contains a compound adjective that should be hyphenated when modifying a noun. Review the phrase at line 249 and apply hyphenation to compound modifiers that precede nouns, such as changing "Rate limiting checks" to "Rate-limiting checks" to follow standard grammar conventions for compound adjectives. Apply this same rule throughout the document wherever compound adjectives (like "failed DB writes" becoming "failed-DB writes") directly precede the noun they modify.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/src/config/db.js`:
- Around line 19-21: The catch block for the transaction in the database
connection code can lose the original error if the ROLLBACK query itself fails.
Wrap the client.query('ROLLBACK') call in its own try-catch block so that if
ROLLBACK throws an error, it does not overwrite the original transaction error.
In the ROLLBACK catch block, log the rollback failure but then re-throw the
original error that was caught in the outer catch block, ensuring the root cause
of the transaction failure is not masked by any rollback failure.
In `@backend/src/db/checks.queries.js`:
- Around line 3-11: The findDueMonitors function performs a plain SELECT query
without row-level locking, allowing multiple concurrent app instances to read
and claim the same monitors for processing, which causes duplicate processing
and data integrity issues. Modify the function to use an atomic claim pattern by
replacing the SELECT with an UPDATE statement that uses FOR UPDATE SKIP LOCKED
to lock due monitors and atomically claim them (e.g., by updating a claim
timestamp or instance identifier), then RETURNING the claimed rows to ensure
each due monitor is exclusively processed by only one instance across all
running app servers.
In `@backend/src/services/checks.service.js`:
- Around line 87-89: The Promise.allSettled call in the checks service processes
all monitors from dueMonitors.map concurrently without any concurrency limit,
which can cause resource exhaustion. Implement concurrency limiting by using a
library like p-limit (or similar) to cap the number of simultaneous processCheck
calls, allowing only a reasonable number of monitors to be processed in parallel
at any given time. Apply the concurrency limiter to the dueMonitors.map call so
that the queue of processCheck promises is throttled rather than all being
created at once.
- Around line 10-14: Before the fetch operation in the checks.service.js block
containing the GET request with signal and redirect options, implement URL
validation to protect against SSRF attacks. Parse and validate the url parameter
to ensure it only uses http or https protocols and does not resolve to private
IP ranges (10.0.0.0/8, 172.16.0.0/12, 192.168.0.0/16), localhost addresses
(127.0.0.1, ::1), link-local addresses (169.254.x.x), or metadata services
(169.254.169.254). Perform hostname resolution safety checks to verify the
resolved IP is not in a reserved range, and either change the redirect option
from 'follow' to 'manual' or validate that any redirect targets pass the same IP
range validation before following them. Throw an appropriate error if the URL
fails any validation check to prevent the fetch from executing.
In `@context1/feature-specs/08-scheduler.md`:
- Line 44: The backend specification assumes Node 24 for native fetch support,
but there is no explicit version constraint in the package.json or .nvmrc file
to enforce this requirement. Add an `engines` field to the backend's
package.json file specifying the minimum Node.js version as 24.0.0, or
alternatively create a .nvmrc file in the backend directory containing the Node
version. This ensures developers use the correct Node.js version and prevents
compatibility issues from using older versions.
---
Nitpick comments:
In `@context1/feature-specs/08-scheduler.md`:
- Around line 35-37: The fenced code block containing the npm install command is
missing a language identifier, which violates markdown linting rules. Add `sh`
as the language identifier immediately after the opening triple backticks
(before the newline) so the code block properly specifies that it contains shell
commands. This applies to the code block in the Scheduler section that shows the
cd backend && npm install node-cron command.
- Line 249: The line contains a compound adjective that should be hyphenated
when modifying a noun. Review the phrase at line 249 and apply hyphenation to
compound modifiers that precede nouns, such as changing "Rate limiting checks"
to "Rate-limiting checks" to follow standard grammar conventions for compound
adjectives. Apply this same rule throughout the document wherever compound
adjectives (like "failed DB writes" becoming "failed-DB writes") directly
precede the noun they modify.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d75601b9-4475-4c6c-9734-6d4cd089f435
⛔ Files ignored due to path filters (1)
backend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
backend/package.jsonbackend/src/config/db.jsbackend/src/db/checks.queries.jsbackend/src/scheduler/index.jsbackend/src/server.jsbackend/src/services/checks.service.jscontext1/feature-specs/08-scheduler.md
Comment on lines
+3
to
+11
| export const findDueMonitors = async () => { | ||
| const result = await query( | ||
| `SELECT id, url, failure_threshold, consecutive_failures, is_alerted, interval_minutes | ||
| FROM monitors | ||
| WHERE next_check_at <= NOW() | ||
| AND is_active = true | ||
| AND is_deleted = false` | ||
| ); | ||
| return result.rows; |
There was a problem hiding this comment.
Due monitor selection is not globally exclusive.
findDueMonitors() uses a plain read, so concurrent app instances can pick the same monitor in the same minute. Combined with snapshot-based counter math in processCheck (backend/src/services/checks.service.js Line 58-Line 69), this can double-insert logs and lose failure-count updates.
Please switch to an atomic “claim due monitors” pattern (e.g., FOR UPDATE SKIP LOCKED + UPDATE ... RETURNING) so each due monitor is processed once across instances.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/src/db/checks.queries.js` around lines 3 - 11, The findDueMonitors
function performs a plain SELECT query without row-level locking, allowing
multiple concurrent app instances to read and claim the same monitors for
processing, which causes duplicate processing and data integrity issues. Modify
the function to use an atomic claim pattern by replacing the SELECT with an
UPDATE statement that uses FOR UPDATE SKIP LOCKED to lock due monitors and
atomically claim them (e.g., by updating a claim timestamp or instance
identifier), then RETURNING the claimed rows to ensure each due monitor is
exclusively processed by only one instance across all running app servers.
| - Standard for scheduled tasks in Node.js | ||
| - Easy to read: `'* * * * *'` = every minute | ||
|
|
||
| The backend is Node 24, which has native `fetch` — no HTTP client needed. |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify Node.js version target in project
echo "=== Checking Node version target ==="
[ -f backend/.nvmrc ] && echo ".nvmrc:" && cat backend/.nvmrc
[ -f backend/package.json ] && echo "package.json engines:" && jq '.engines' backend/package.json 2>/dev/null || echo "No engines field"
find backend -name .tool-versions -o -name .github/workflows/*.yml | head -5 | xargs grep -l "node\|Node" 2>/dev/null | head -3Repository: omarmenew24-collab/Uptime-Monitoring
Length of output: 144
🏁 Script executed:
# Search more thoroughly for Node.js version specifications
find . -type f -name "*.yml" -o -name "*.yaml" | head -20Repository: omarmenew24-collab/Uptime-Monitoring
Length of output: 62
🏁 Script executed:
# Check for Node version in root .nvmrc or other config files
ls -la | grep -E "nvmrc|tool-versions|volta"
cat .nvmrc 2>/dev/null || echo "No root .nvmrc"Repository: omarmenew24-collab/Uptime-Monitoring
Length of output: 95
🌐 Web query:
Node.js 24 native fetch support availability
💡 Result:
Node.js 24 has native, global fetch support (it is not behind an experimental flag in modern Node 24 builds). In Node v24 documentation, fetch is described as a browser-compatible implementation, available as a global (along with Request/Response/Headers) [1][2]. The underlying implementation is powered by Undici [3]. Practical availability notes for Node.js 24 (as of today, 2026-06-21): 1) What you can use: In Node 24, fetch and related classes (Request, Response, Headers) are available from the global scope [1][4]. 2) How it’s implemented: Node’s fetch API is powered by Undici [3]. 3) No extra “experimental flag” needed: Node’s “Global objects” documentation for v24 states it is “No longer behind --experimental-fetch CLI flag” [1]. Confidence: high that Node.js 24 provides native global fetch support, based on official Node.js docs for v24 globals and the Node Learn/Undici materials [1][3].
Citations:
- 1: https://nodejs-api-docs-tooling.vercel.app/globals.html
- 2: https://nodejs.org/docs/v24.11.1/api/
- 3: https://nodejs.org/en/learn/getting-started/fetch
- 4: https://www.thenodebook.com/runtime-platform/web-platform-apis
🏁 Script executed:
# More thorough search for CI/CD and configuration files
find . -path ./node_modules -prune -o -type f \( -name "*.yml" -o -name "*.yaml" -o -name ".github" -type d \) -print | head -30Repository: omarmenew24-collab/Uptime-Monitoring
Length of output: 62
🏁 Script executed:
# Check if .github/workflows directory exists and what's inside
ls -la .github/workflows/ 2>/dev/null || echo "No .github/workflows directory"Repository: omarmenew24-collab/Uptime-Monitoring
Length of output: 111
🏁 Script executed:
# Search for any mention of Node version in common config files
cat package.json | head -50Repository: omarmenew24-collab/Uptime-Monitoring
Length of output: 125
🏁 Script executed:
# Check if backend/package.json exists and examine it more carefully
cat backend/package.jsonRepository: omarmenew24-collab/Uptime-Monitoring
Length of output: 739
🏁 Script executed:
# Look at the spec file mentioned in the review
cat "context1/feature-specs/08-scheduler.md"Repository: omarmenew24-collab/Uptime-Monitoring
Length of output: 9994
🏁 Script executed:
# Check directory structure to understand project layout
ls -la backend/ 2>/dev/null | head -20Repository: omarmenew24-collab/Uptime-Monitoring
Length of output: 567
Add Node.js version constraint to match spec assumption.
The spec correctly states that Node 24 has native fetch. However, the backend's package.json lacks an engines field, and there is no .nvmrc or CI configuration to enforce Node 24. Developers could inadvertently use Node 18, 20, or 22. Add an explicit version constraint:
"engines": {
"node": ">=24.0.0"
}Or create backend/.nvmrc with 24 to document the target version.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@context1/feature-specs/08-scheduler.md` at line 44, The backend specification
assumes Node 24 for native fetch support, but there is no explicit version
constraint in the package.json or .nvmrc file to enforce this requirement. Add
an `engines` field to the backend's package.json file specifying the minimum
Node.js version as 24.0.0, or alternatively create a .nvmrc file in the backend
directory containing the Node version. This ensures developers use the correct
Node.js version and prevents compatibility issues from using older versions.
Wraps the ROLLBACK call in its own try/catch so a failed rollback (e.g. dropped connection) doesn't swallow the original transaction error. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Two layers of defense against Server-Side Request Forgery: 1. Input validation (Zod schema): blocks private IPs, localhost, and reserved hostnames at monitor creation time 2. DNS resolution check (before fetch): resolves hostname and verifies the IP isn't private/reserved before the HTTP request is sent, catching cases where a public hostname resolves to a private IP Blocked ranges: 10.0.0.0/8, 172.16.0.0/12, 192.168.0.0/16, 127.0.0.0/8, 169.254.0.0/16, ::1, fc00::/7, fe80::/10 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/src/services/checks.service.js (1)
22-26:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winHTTP redirects bypass SSRF protection.
The
redirect: 'follow'option allows the request to follow redirects without re-validating the target URL. An attacker could host a URL that passes validation, then redirects tohttp://169.254.169.254/or other internal endpoints.Change to
redirect: 'manual'and either reject redirects or validate each redirect target.🛡️ Proposed fix to prevent redirect-based SSRF
const response = await fetch(url, { method: 'GET', signal: AbortSignal.timeout(CHECK_TIMEOUT_MS), - redirect: 'follow', + redirect: 'manual', }); const responseTimeMs = Date.now() - startTime; - if (response.ok) { + // For monitors, a redirect (3xx) should still be considered "up" + // but we don't follow it to prevent SSRF + if (response.ok || (response.status >= 300 && response.status < 400)) { return { status: 'up', responseCode: response.status,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/services/checks.service.js` around lines 22 - 26, The fetch request in the checks service uses redirect: 'follow' which bypasses SSRF protection by automatically following redirects without re-validating the target URL. Change the redirect option from 'follow' to 'manual' to prevent automatic redirect following. After making this change, add logic to handle redirect responses (HTTP 3xx status codes) by either rejecting them entirely if not needed for your use case, or by validating each redirect target URL against your SSRF protection rules before allowing the redirect to be followed.
🧹 Nitpick comments (1)
backend/src/schemas/monitors.schema.js (1)
8-13: 💤 Low valueConsider surfacing the specific rejection reason in the error message.
The current error message is generic. Zod 4's
.refine()supports dynamic messages via function, which would help users understand why their URL was rejected (e.g., "Private IP" vs "Invalid hostname").♻️ Optional improvement for dynamic error messages
url: z.string().trim() .regex(urlPattern, 'Must be a valid HTTP or HTTPS URL with a domain') - .refine((val) => { - const result = validateUrlHostname(val); - return result.safe; - }, 'Private, reserved, or internal URLs are not allowed'), + .refine((val) => { + const result = validateUrlHostname(val); + return result.safe; + }, (val) => { + const result = validateUrlHostname(val); + return { message: result.reason || 'Private, reserved, or internal URLs are not allowed' }; + }),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/schemas/monitors.schema.js` around lines 8 - 13, The error message in the refine method for the URL validation is static and does not communicate the specific reason why a URL was rejected. Modify the second argument of the refine call to use a function instead of a string literal that takes the URL value as a parameter and returns a dynamic error message. Inside this function, call validateUrlHostname to get the result object, then construct and return a message that includes the specific rejection reason from the result object rather than the generic 'Private, reserved, or internal URLs are not allowed' message.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/src/utils/url-safety.js`:
- Around line 8-26: The isPrivateIPv4 function does not validate that each
parsed octet is within the valid IPv4 range of 0-255, allowing malformed inputs
like 192.168.1.256 or 10.abc.0.1 to potentially bypass security checks when they
produce NaN or out-of-range values. After splitting and mapping the IP string to
numbers, add an additional validation check to ensure each octet in the parts
array is a valid number and falls within the range 0-255. Add this validation
immediately after the length check to reject malformed inputs before processing
the private IP ranges.
- Around line 28-34: The isPrivateIPv6 function has incomplete IPv6 address
detection that creates an SSRF vulnerability by missing IPv4-mapped addresses
(like ::ffff:10.0.0.1), full-form representations with leading zeros, and
incomplete ULA range matching. Replace the manual string-based checks in
isPrivateIPv6 with a dedicated IPv6 parsing library such as ipaddr.js or
ip-address that properly handles RFC-compliant private address detection
including IPv4-mapped addresses, compressed and full-form representations, and
complete ULA range (fc00::/7) coverage.
- Around line 66-87: The resolveAndValidate function returns a resolved IP
address that is never used in the subsequent fetch request, creating a
time-of-check-time-of-use vulnerability where DNS can change between validation
and actual use. Modify the code to either pass the resolved IP from
resolveAndValidate to the fetch call in checks.service.js (by constructing the
URL with the IP address directly or using a custom DNS resolver) or implement IP
pinning at the socket level. Additionally, modify the lookup() call to use the {
all: true } option to validate all resolved addresses instead of only the first
one, ensuring that any hostname with multiple A records has all of them
validated for private IPs.
---
Outside diff comments:
In `@backend/src/services/checks.service.js`:
- Around line 22-26: The fetch request in the checks service uses redirect:
'follow' which bypasses SSRF protection by automatically following redirects
without re-validating the target URL. Change the redirect option from 'follow'
to 'manual' to prevent automatic redirect following. After making this change,
add logic to handle redirect responses (HTTP 3xx status codes) by either
rejecting them entirely if not needed for your use case, or by validating each
redirect target URL against your SSRF protection rules before allowing the
redirect to be followed.
---
Nitpick comments:
In `@backend/src/schemas/monitors.schema.js`:
- Around line 8-13: The error message in the refine method for the URL
validation is static and does not communicate the specific reason why a URL was
rejected. Modify the second argument of the refine call to use a function
instead of a string literal that takes the URL value as a parameter and returns
a dynamic error message. Inside this function, call validateUrlHostname to get
the result object, then construct and return a message that includes the
specific rejection reason from the result object rather than the generic
'Private, reserved, or internal URLs are not allowed' message.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 54c904ab-941f-421d-9ee2-6a514ffef8d9
📒 Files selected for processing (3)
backend/src/schemas/monitors.schema.jsbackend/src/services/checks.service.jsbackend/src/utils/url-safety.js
1. Concurrency limit: checks now run 50 at a time via p-limit instead of all-at-once, preventing socket/memory exhaustion 2. Retention job: daily cron at 3 AM deletes check_logs older than 30 days, preventing unbounded table growth 3. Fetch body drain: response.body.cancel() after reading status prevents TCP socket leaks under sustained check volume 4. IPv6 SSRF: replaced hand-rolled IPv6 checks with ipaddr.js which handles IPv4-mapped IPv6 (::ffff:169.254.169.254), full-form addresses, and all RFC edge cases Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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)
backend/src/utils/url-safety.js (1)
71-76:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate all resolved IP addresses, not just the first.
lookup()without{ all: true }returns only one address. A hostname with multiple A/AAAA records could have both public and private IPs, but only the first resolved address is validated. An attacker could configure DNS to return a public IP first (validated) followed by a private IP that might be used by the connection.- const { address } = await lookup(hostname); - if (isPrivateIP(address)) { - return { safe: false, reason: `Hostname resolves to a private IP (${address})` }; + const addresses = await lookup(hostname, { all: true }); + for (const { address } of addresses) { + if (isPrivateIP(address)) { + return { safe: false, reason: `Hostname resolves to a private IP (${address})` }; + } } - return { safe: true, ip: address }; + return { safe: true, ip: addresses[0].address };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/utils/url-safety.js` around lines 71 - 76, The lookup() call in the hostname validation logic only returns and validates a single IP address. Modify the lookup() call to include the { all: true } option so it returns all resolved addresses for the hostname, then iterate through all returned addresses to validate that none of them are private IPs using isPrivateIP() before returning { safe: true }. If any address in the array is private, return the unsafe result with the reason indicating which private IP was found.
🧹 Nitpick comments (2)
backend/src/utils/url-safety.js (1)
9-15: ⚡ Quick winConsider adding
carrierGradeNatto blocked ranges.The
ipaddr.jslibrary also classifies100.64.0.0/10(RFC 6598 shared address space) ascarrierGradeNat. In some cloud environments, this range could route to internal infrastructure. Consider adding it:const PRIVATE_RANGES = [ 'loopback', 'private', 'linkLocal', 'uniqueLocal', 'unspecified', + 'carrierGradeNat', ];🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/utils/url-safety.js` around lines 9 - 15, The PRIVATE_RANGES array in url-safety.js is missing the 'carrierGradeNat' classification which represents RFC 6598 shared address space (100.64.0.0/10). Add 'carrierGradeNat' as a string element to the PRIVATE_RANGES array definition alongside the existing entries like 'loopback', 'private', 'linkLocal', 'uniqueLocal', and 'unspecified' to ensure this address range is properly blocked in cloud environments where it could route to internal infrastructure.backend/src/db/retention.queries.js (1)
6-10: Consider batched deletion for high-volume deployments.With frequent monitor checks, this query could delete a large number of rows in a single transaction, potentially causing lock contention. For production deployments with high check volume:
- Ensure an index exists on
check_logs.checked_at- Consider batching deletes in chunks (e.g.,
DELETE ... LIMIT 10000in a loop) to reduce lock durationThis is acceptable for initial deployment but may need revisiting at scale.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/db/retention.queries.js` around lines 6 - 10, The DELETE query against check_logs that removes rows based on checked_at age performs a single large deletion that can cause lock contention in high-volume deployments. To fix this, ensure an index exists on the check_logs.checked_at column to optimize the deletion query performance, then refactor the query execution to batch the deletes in chunks (such as 10000 rows per iteration in a loop) rather than deleting all matching rows in a single transaction. This approach reduces lock duration and improves database stability at scale while maintaining the same retention cleanup functionality.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/src/scheduler/index.js`:
- Around line 22-24: The retention cleanup success message in the scheduler is
incorrectly using console.error instead of console.log, which pollutes error
streams and triggers unnecessary monitoring alerts. Change the console.error
call in the block that logs the deletion count to console.log since successful
deletion of old check logs is an informational message, not an error condition.
---
Outside diff comments:
In `@backend/src/utils/url-safety.js`:
- Around line 71-76: The lookup() call in the hostname validation logic only
returns and validates a single IP address. Modify the lookup() call to include
the { all: true } option so it returns all resolved addresses for the hostname,
then iterate through all returned addresses to validate that none of them are
private IPs using isPrivateIP() before returning { safe: true }. If any address
in the array is private, return the unsafe result with the reason indicating
which private IP was found.
---
Nitpick comments:
In `@backend/src/db/retention.queries.js`:
- Around line 6-10: The DELETE query against check_logs that removes rows based
on checked_at age performs a single large deletion that can cause lock
contention in high-volume deployments. To fix this, ensure an index exists on
the check_logs.checked_at column to optimize the deletion query performance,
then refactor the query execution to batch the deletes in chunks (such as 10000
rows per iteration in a loop) rather than deleting all matching rows in a single
transaction. This approach reduces lock duration and improves database stability
at scale while maintaining the same retention cleanup functionality.
In `@backend/src/utils/url-safety.js`:
- Around line 9-15: The PRIVATE_RANGES array in url-safety.js is missing the
'carrierGradeNat' classification which represents RFC 6598 shared address space
(100.64.0.0/10). Add 'carrierGradeNat' as a string element to the PRIVATE_RANGES
array definition alongside the existing entries like 'loopback', 'private',
'linkLocal', 'uniqueLocal', and 'unspecified' to ensure this address range is
properly blocked in cloud environments where it could route to internal
infrastructure.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ecf60e6d-dbe9-45cc-8e24-00cf26898fdf
⛔ Files ignored due to path filters (1)
backend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
backend/package.jsonbackend/src/db/retention.queries.jsbackend/src/scheduler/index.jsbackend/src/services/checks.service.jsbackend/src/utils/url-safety.jscontext1/learning.md
✅ Files skipped from review due to trivial changes (1)
- context1/learning.md
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/package.json
- backend/src/services/checks.service.js
Comment on lines
+22
to
+24
| if (deleted > 0) { | ||
| console.error(`Retention: deleted ${deleted} check logs older than 30 days`); | ||
| } |
There was a problem hiding this comment.
Use console.log for informational messages.
Successful retention cleanup is not an error. Using console.error for this message will pollute error streams and monitoring alerts.
if (deleted > 0) {
- console.error(`Retention: deleted ${deleted} check logs older than 30 days`);
+ console.log(`Retention: deleted ${deleted} check logs older than 30 days`);
}📝 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
| if (deleted > 0) { | |
| console.error(`Retention: deleted ${deleted} check logs older than 30 days`); | |
| } | |
| if (deleted > 0) { | |
| console.log(`Retention: deleted ${deleted} check logs older than 30 days`); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/src/scheduler/index.js` around lines 22 - 24, The retention cleanup
success message in the scheduler is incorrectly using console.error instead of
console.log, which pollutes error streams and triggers unnecessary monitoring
alerts. Change the console.error call in the block that logs the deletion count
to console.log since successful deletion of old check logs is an informational
message, not an error condition.
- dns.lookup() now uses { all: true } to resolve every A/AAAA record.
If ANY resolved IP is private, the check is blocked. Closes the
multi-record bypass where one public + one private IP passes validation.
- Reverted IP pinning approach (broke HTTPS due to TLS cert/SNI mismatch).
- The narrow TOCTOU window between our DNS check and fetch's internal
resolution is documented as an accepted limitation — true socket-level
IP pinning requires custom undici internals, not justified for MVP.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Native fetch is available from Node 22+. Without this constraint, a developer on Node 18/20 gets a cryptic "fetch is not defined" at runtime. The engines field makes npm warn on install. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Owner
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
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
1 participant
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.
Implements the core monitoring engine: a cron job that runs every minute, checks all due monitors via HTTP GET with 5s timeout, logs results to check_logs, and updates monitor state (last_status, consecutive_failures, is_alerted) atomically within a transaction.
Summary by CodeRabbit