greenfield-processor for cdn uploads#111
Open
ice-cronus wants to merge 3 commits into
Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a new greenfield-processor service (code, build, Docker, docs) and many supporting modules: event routing, batching, Redis-backed atomic batch ops and locks, resumable Greenfield downloads with integrity checks, dual HTTP/FTP Bunny uploads, validation, metrics, health routes, and extensive unit/integration tests. Also extends checksum parsing to accept JSON arrays. Changes
Sequence DiagramsequenceDiagram
participant Router as Router Worker
participant Redis as Redis
participant BatchAsm as Batch Assembler
participant Queue as BullMQ (cdn-batches)
participant Upload as Upload Worker
participant SP as Greenfield SP
participant Bunny as Bunny CDN
Router->>Redis: validate & classify event
alt disposition == upload
Router->>Redis: RPUSH pending-uploads (atomic)
Router->>BatchAsm: trigger flush or schedule delayed flush
else skip
Router->>Redis: log/ignore
end
BatchAsm->>Redis: LUA pop up to N items (atomic)
BatchAsm->>Queue: enqueue upload-batch job with items
Queue->>Upload: deliver batch job
Upload->>Redis: acquire per-item lock (SET NX EX)
Upload->>SP: resolve storage provider endpoint (cache or fetch)
Upload->>SP: fetch/stream object (Range resume supported)
Upload->>Upload: compute/verify integrity hash
alt file < httpUploadSizeLimit
Upload->>Bunny: HTTP PUT streaming upload
else
Upload->>Bunny: FTP append/upload (resumable)
end
Upload->>Redis: store upload metadata, release lock, update progress
alt failures exceed retries
Upload->>Redis: push to cdn:dead-letter-uploads
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ice-cronus
force-pushed
the
feature/greenfield-processor
branch
from
93cc505 to
6e925d4
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (7)
backend/services/greenfield-processor/src/create-pinned-dispatcher.test.ts-12-18 (1)
12-18:⚠️ Potential issue | 🟡 MinorMissing
awaitonclose()calls.
dispatcher.close()returns aPromise<void>. Not awaiting these calls could lead to test flakiness or unhandled promise rejections.🔧 Proposed fix
- it('creates distinct dispatchers per call', () => { + it('creates distinct dispatchers per call', async () => { const d1 = createPinnedDispatcher('203.0.113.10', 4); const d2 = createPinnedDispatcher('203.0.113.11', 4); expect(d1).not.toBe(d2); - d1.close(); - d2.close(); + await d1.close(); + await d2.close(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/services/greenfield-processor/src/create-pinned-dispatcher.test.ts` around lines 12 - 18, The test creates dispatchers with createPinnedDispatcher('203.0.113.10', 4) and createPinnedDispatcher('203.0.113.11', 4) but calls d1.close() and d2.close() without awaiting them; change the test to be async (make the it(...) callback async) and await both d1.close() and d2.close() to ensure promises are settled and avoid flakiness/unhandled rejections when closing the dispatchers.backend/services/greenfield-processor/src/cleanup-stale-files.test.ts-11-13 (1)
11-13:⚠️ Potential issue | 🟡 MinorAdd
afterEachcleanup for temp directories.The test creates a temp directory in
beforeEachbut never cleans it up, causing temp directories to accumulate across test runs. Other test files in this service (e.g.,compute-integrity-hash.test.ts) include proper cleanup.🧹 Proposed fix to add cleanup
+import { describe, it, expect, beforeEach, afterEach } from 'vitest'; -import { describe, it, expect, beforeEach } from 'vitest'; -import { mkdtemp, writeFile, mkdir, stat, utimes } from 'node:fs/promises'; +import { mkdtemp, rm, writeFile, mkdir, stat, utimes } from 'node:fs/promises'; import { join } from 'node:path'; import { tmpdir } from 'node:os'; import pino from 'pino'; import cleanupStaleFiles from './cleanup-stale-files.js'; const logger = pino({ level: 'silent' }); let tempDir: string; beforeEach(async () => { tempDir = await mkdtemp(join(tmpdir(), 'cleanup-test-')); }); +afterEach(async () => { + await rm(tempDir, { recursive: true, force: true }); +});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/services/greenfield-processor/src/cleanup-stale-files.test.ts` around lines 11 - 13, Add an afterEach cleanup that removes the temporary directory created in beforeEach so tempDir created via mkdtemp(join(tmpdir(), 'cleanup-test-')) is deleted after each test; locate the beforeEach block and add an afterEach that checks the tempDir variable and calls the appropriate removal function (e.g., fs.rm or fs.rmdir with recursive/force options or rimraf equivalent) to delete the directory and avoid leaking temp directories across runs.backend/services/greenfield-processor/src/create-batch.ts-37-37 (1)
37-37:⚠️ Potential issue | 🟡 MinorUnguarded
JSON.parsemay throw on malformed Redis data.If corrupted or non-JSON data exists in the Redis list, this will throw and crash the batch assembly. Consider wrapping in try/catch with logging and filtering out invalid items, or moving them to a dead-letter queue.
🛡️ Proposed defensive parsing
- return results.map((raw) => JSON.parse(raw) as PendingUploadItem); + return results.reduce<PendingUploadItem[]>((acc, raw) => { + try { + acc.push(JSON.parse(raw) as PendingUploadItem); + } catch { + // Log or dead-letter malformed item; skip for now + } + return acc; + }, []);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/services/greenfield-processor/src/create-batch.ts` at line 37, The direct JSON.parse in the return of create-batch.ts is unsafe — wrap parsing of each entry from results (the array mapped to PendingUploadItem) in a try/catch that logs the offending raw value and error (use your logger or processLogger) and filters out malformed entries (or enqueue them to a dead-letter queue), so createBatch (or the function containing results.map) returns only successfully parsed PendingUploadItem objects without throwing on corrupted Redis data.backend/services/greenfield-processor/src/collect-metrics.ts-72-79 (1)
72-79:⚠️ Potential issue | 🟡 MinorMissing error handling in async
collect()may break metrics scraping.If Redis is unreachable during a Prometheus scrape, the unhandled rejection from
redis.llen()could cause the/metricsendpoint to fail. Consider wrapping in try/catch with a fallback or logged warning.🛡️ Proposed fix with error handling
async collect() { + try { const [pending, deadLetter] = await Promise.all([ redis.llen('cdn:pending-uploads'), redis.llen('cdn:dead-letter-uploads'), ]); this.set({ queue: 'pending' }, pending); this.set({ queue: 'dead-letter' }, deadLetter); + } catch { + // Redis unavailable; metrics will show stale values + } },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/services/greenfield-processor/src/collect-metrics.ts` around lines 72 - 79, The async collect() method may throw if redis.llen() rejects, so wrap the Promise.all call in a try/catch inside collect(), log the error (e.g., console.error or the service logger if available), and set safe fallback metric values (e.g., 0) via this.set({ queue: 'pending' }, fallback) and this.set({ queue: 'dead-letter' }, fallback) so the /metrics scrape never rejects; keep the existing this.set calls but move them into the try block for successful results and into the catch block to apply fallbacks and a logged warning.backend/services/greenfield-processor/src/config.ts-72-73 (1)
72-73:⚠️ Potential issue | 🟡 MinorUpdate ReDoS protection to use actively maintained library.
The ReDoS risk is valid: a misconfigured pattern (e.g., with catastrophic backtracking) could cause service disruption at startup. However, the recommended library
safe-regexis unmaintained (last updated October 2019), and its suggested alternativevuln-regex-detectoris also inactive (last commit January 2022). Useredos-detectorinstead, which is actively maintained (latest release February 2026).🛡️ Proposed fix using redos-detector
+import { isVulnerable } from 'redos-detector'; + function validateSpHostnamePattern(pattern: string): void { if (!pattern) return; if (pattern.length > MAX_SP_PATTERN_LENGTH) { throw new Error( `ALLOWED_SP_HOSTNAME_PATTERN too long: ${pattern.length} exceeds max ${MAX_SP_PATTERN_LENGTH}`, ); } try { new RegExp(pattern); } catch (err) { const message = err instanceof Error ? err.message : String(err); throw new Error( `ALLOWED_SP_HOSTNAME_PATTERN is not a valid regex: ${message}`, ); } + if (isVulnerable(pattern)) { + throw new Error( + 'ALLOWED_SP_HOSTNAME_PATTERN contains potentially catastrophic backtracking', + ); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/services/greenfield-processor/src/config.ts` around lines 72 - 73, Replace the naive RegExp construction check (the try { new RegExp(pattern); } usage) with the actively maintained redos-detector: add an import for redos-detector (e.g., isVulnerable), call isVulnerable(pattern) before instantiating RegExp, and treat a true result as a validation failure (log/throw) to block patterns with catastrophic backtracking; then only create new RegExp(pattern) when isVulnerable returns false. Ensure you reference the existing pattern variable and the code path that currently calls new RegExp so the detection runs at the same validation point.backend/services/greenfield-processor/src/validate.ts-92-109 (1)
92-109:⚠️ Potential issue | 🟡 MinorReject non-finite numeric payload values.
typeof value === 'number'still letsNaNandInfinitythrough, so those fields can bypass validation and poison downstream size/version logic. Guard both helpers withNumber.isFinite(value).🔧 Proposed fix
function assertPositiveNumber( record: Record<string, unknown>, field: string, ): void { const value = record[field]; - if (typeof value !== 'number' || value <= 0) { + if (typeof value !== 'number' || !Number.isFinite(value) || value <= 0) { throw new Error(`Event payload field "${field}" must be a positive number`); } } @@ function assertNonNegativeNumber( record: Record<string, unknown>, field: string, ): void { const value = record[field]; - if (typeof value !== 'number' || value < 0) { + if (typeof value !== 'number' || !Number.isFinite(value) || value < 0) { throw new Error(`Event payload field "${field}" must be a non-negative number`); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/services/greenfield-processor/src/validate.ts` around lines 92 - 109, The helpers assertPositiveNumber and assertNonNegativeNumber currently check typeof value === 'number' which permits NaN and Infinity; update both to first ensure Number.isFinite(value) and then perform the numeric comparison (for assertPositiveNumber require > 0, for assertNonNegativeNumber require >= 0), and keep the same error messages referencing the field when validation fails so non-finite values are rejected before downstream size/version logic uses them.backend/services/greenfield-processor/src/upload-worker.ts-69-80 (1)
69-80:⚠️ Potential issue | 🟡 MinorSkipped items are not marked as completed.
When an item fails and exceeds
MAX_RETRIES, it's added todeadLetteredbut not added tocompleted. This means if the batch retries (due to other failures), the dead-lettered item will be processed again and re-added to the dead-letter queue. Consider marking dead-lettered items as completed to prevent duplicate processing.🐛 Proposed fix
if (retryCount > MAX_RETRIES) { logger.warn({ item, retryCount }, 'item moved to dead-letter queue'); deadLettered.push({ ...item, retryCount }); + completed.add(key); } else { failCount++; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/services/greenfield-processor/src/upload-worker.ts` around lines 69 - 80, The dead-lettered items are not being marked as completed, so they can be retried; in the catch block where you handle retryCount > MAX_RETRIES (using failCounts, MAX_RETRIES, deadLettered), increment the completed counter (the same completed variable used for updateProgress) and ensure you push the item to deadLettered with retryCount before calling updateProgress(job, completed, failCounts); this marks dead-lettered items as finished and prevents them from being reprocessed.
🧹 Nitpick comments (28)
backend/services/greenfield-processor/src/compute-integrity-hash.ts (2)
51-59:normalizeToHexaccepts arbitrary invalid input.When
hashis neither a 64-char hex string nor valid 32-byte base64, the function returns the lowercased input as-is (line 58). This could cause confusing comparison failures with hashes like"not-a-hash"being compared directly against computed hex.Consider throwing an error for clearly invalid formats, or documenting this permissive behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/services/greenfield-processor/src/compute-integrity-hash.ts` around lines 51 - 59, normalizeToHex currently returns the lowercased input for any non-hex, non-32-byte-base64 value which lets invalid strings (e.g., "not-a-hash") silently pass; change normalizeToHex to validate inputs and throw a descriptive Error when the input is neither a 64-char hex nor a 32-byte base64 (i.e., after failing /^[0-9a-f]{64}$/ and Buffer.from(...,'base64') length check), so callers of normalizeToHex receive a clear failure instead of unexpected lowercase values.
43-48: Consider logging unlink failures instead of silently swallowing.The empty
.catch(() => {})silently swallows unlink errors. While this is intentional best-effort cleanup, adding a debug log would aid troubleshooting (e.g., permission issues or locked files).💡 Optional: Add logging
if (actual !== expected) { - await unlink(filePath).catch(() => {}); + await unlink(filePath).catch((err) => { + // Best-effort cleanup; log for debugging + console.debug(`Failed to unlink ${filePath}: ${err.message}`); + }); throw new Error( `Integrity hash mismatch: expected ${expected}, got ${actual}`, ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/services/greenfield-processor/src/compute-integrity-hash.ts` around lines 43 - 48, In compute-integrity-hash.ts where unlink(filePath).catch(() => {}) is used after an integrity mismatch, replace the empty catch with a small debug log that records the unlink failure (include filePath and the error); use the repository's existing logger (e.g., logger.debug or processLogger.debug) if available, otherwise fall back to console.debug, so the cleanup remains best-effort but failures are visible for troubleshooting.backend/services/greenfield-processor/src/fetch-with-pinned-dns.ts (1)
32-35: Cleanup leaks the dispatcher if caller forgets to callcleanup().If the caller consumes the response but never calls
cleanup(), the dispatcher (and its connection pool) leaks. Consider documenting this requirement clearly, or using a pattern likeSymbol.dispose/usingfor automatic cleanup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/services/greenfield-processor/src/fetch-with-pinned-dns.ts` around lines 32 - 35, The returned object leaks the dispatcher if the caller never calls cleanup(); modify fetch-with-pinned-dns to ensure dispatcher is closed automatically by either: 1) wiring dispatcher.close() to the response stream lifecycle (e.g., call dispatcher.close() in response.body 'end'/'close'/'error' handlers so the connection pool is released when the response is consumed/terminates) or 2) exposing a disposable protocol (implement and return a Symbol.dispose method on the returned object or provide a using-friendly API) and also document the requirement; ensure references to dispatcher, cleanup, and response in the function are updated so the dispatcher is reliably closed even when callers forget to call cleanup().backend/packages/greenfield-client/parser_test.go (1)
86-108: Consider adding edge case tests for robustness.The tests cover the main scenarios well. Consider adding tests for:
- Malformed JSON starting with
[(e.g.,[invalid) to verify fallback behavior- Whitespace handling in comma-separated values (e.g.,
"abc, def")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/packages/greenfield-client/parser_test.go` around lines 86 - 108, Add two unit tests for parseChecksums to cover edge cases: (1) a test named TestParseChecksums_MalformedJSONArray that passes a malformed JSON string starting with "[" (e.g., "[invalid") and asserts the function falls back to the comma/trim behavior or returns nil as appropriate for your implementation; (2) a test named TestParseChecksums_WhitespaceCommaSeparated that passes a comma-separated string with spaces (e.g., "abc, def ,ghi ") and asserts the parsed slice trims whitespace around values (e.g., []string{"abc","def","ghi"}). Place both tests in parser_test.go alongside the existing TestParseChecksums_* tests and use the same testing style (t.Parallel() and require assertions) to keep consistency with parseChecksums.backend/packages/greenfield-client/parser.go (1)
204-209: Silent JSON parse fallback may mask malformed input.When
rawstarts with[but isn't valid JSON (e.g.,[invalidor["abc), the function silently falls back to comma-splitting, which would return unexpected results like["[invalid"]. Consider logging or returning an error for inputs that look like JSON arrays but fail to parse.💡 Optional: Add logging for parse failures
if strings.HasPrefix(raw, "[") { var parsed []string if err := json.Unmarshal([]byte(raw), &parsed); err == nil { return parsed } + // Log or handle malformed JSON-like input if needed } return strings.Split(raw, ",")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/packages/greenfield-client/parser.go` around lines 204 - 209, The code currently checks strings.HasPrefix(raw, "[") and attempts json.Unmarshal into parsed but silently falls back to comma-splitting on error; change this so a failed json.Unmarshal on an input that looks like a JSON array does not get silently ignored: either (preferred) propagate the parse failure to the caller by returning an error from the surrounding function (update the signature to return ([]string, error) and return the json.Unmarshal error when it fails) or (alternate) log the failure with context (include raw and err) before taking any fallback path. Make the change at the json.Unmarshal call site (the variables raw and parsed) so inputs beginning with "[" no longer silently produce misleading results.backend/services/greenfield-processor/src/cleanup-stale-files.ts (1)
25-29: Log directory read failures instead of silently returning.Silently ignoring
readdirerrors could hide permission issues or filesystem problems that operators should be aware of.♻️ Proposed fix
try { entries = await readdir(dir, { withFileTypes: true }); - } catch { + } catch (err) { + logger.debug({ err, dir }, 'failed to read directory for cleanup'); return; }Note: This requires passing
loggertocleanupDirectory, which the function already receives.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/services/greenfield-processor/src/cleanup-stale-files.ts` around lines 25 - 29, The try/catch around the readdir call in cleanupDirectory silently returns on error; instead log the failure via the provided logger so operators see permission/FS issues. In the catch for the await readdir(dir, { withFileTypes: true }) call inside cleanupDirectory, call logger.error (include the dir and the caught error) before returning, so failures are recorded; ensure the caught error is captured (e.g., catch (err)) and include contextual text like "failed to read directory" with dir and err in the log message.backend/services/greenfield-processor/src/config.test.ts (1)
23-41: Consider adding a test for whenALLOWED_SP_HOSTNAME_PATTERNis not set.The tests cover empty string and various invalid values, but don't verify behavior when the env var is entirely absent. This is a common scenario that should be explicitly tested.
♻️ Suggested addition
it('accepts missing hostname pattern (env var not set)', () => { delete process.env.ALLOWED_SP_HOSTNAME_PATTERN; expect(() => loadConfig()).not.toThrow(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/services/greenfield-processor/src/config.test.ts` around lines 23 - 41, Add a test that verifies behavior when ALLOWED_SP_HOSTNAME_PATTERN is not present by deleting process.env.ALLOWED_SP_HOSTNAME_PATTERN before calling loadConfig and asserting it does not throw; specifically, in the same test suite that contains the other hostname pattern tests, add a case named like "accepts missing hostname pattern (env var not set)" which runs delete process.env.ALLOWED_SP_HOSTNAME_PATTERN and then expect(() => loadConfig()).not.toThrow(), ensuring the absence of the env var is handled the same as an empty or default pattern.backend/services/greenfield-processor/src/select-upload-strategy.ts (1)
25-26: Remove redundant variable assignment.
remotePathis just a rename ofcdnPathwith no transformation. PassingcdnPathdirectly is clearer.♻️ Minor cleanup
- const remotePath = cdnPath; - await uploadToBunnyFtp({ localFilePath, remotePath, ftpClient }); + await uploadToBunnyFtp({ localFilePath, remotePath: cdnPath, ftpClient });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/services/greenfield-processor/src/select-upload-strategy.ts` around lines 25 - 26, The assignment remotePath = cdnPath is redundant; update the call site in select-upload-strategy.ts to pass cdnPath directly to uploadToBunnyFtp instead of creating remotePath. Remove the remotePath variable declaration and use uploadToBunnyFtp({ localFilePath, remotePath: cdnPath, ftpClient }) or simply uploadToBunnyFtp({ localFilePath, cdnPath, ftpClient }) adapting to the function's parameter shape so only cdnPath is passed.backend/services/greenfield-processor/src/routes.ts (1)
23-29: Consider adding a timeout toredis.ping()to prevent health-check hangs.If Redis is unresponsive,
ping()may hang indefinitely, causing the health endpoint to never respond. A timeout ensures the health check fails fast. Additionally, logging the caught error would aid diagnostics.♻️ Proposed improvement
try { - await deps.redis.ping(); - } catch { + await Promise.race([ + deps.redis.ping(), + new Promise((_, reject) => + setTimeout(() => reject(new Error('redis ping timeout')), 5000), + ), + ]); + } catch (err) { + deps.logger?.warn?.({ err }, 'health check redis ping failed'); return reply .status(503) .send({ ok: false, reason: 'redis unreachable' });Note: If adding a logger dependency isn't feasible, just adding the timeout would still improve reliability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/services/greenfield-processor/src/routes.ts` around lines 23 - 29, The health-check currently awaits deps.redis.ping() which can hang; wrap deps.redis.ping() in a timeout (e.g. Promise.race or an AbortController-based timeout) so the ping rejects after a short threshold and is caught; in the catch handler log the error (use existing logger if available or console.error) and then return reply.status(503).send({ ok: false, reason: 'redis unreachable' }) so the endpoint fails fast; update the code surrounding deps.redis.ping(), the catch block that calls reply.status, and add the small timeout helper inline or as a tiny function to keep behavior deterministic.backend/services/greenfield-processor/src/fetch-with-pinned-dns.test.ts (1)
27-27:server.close()should be awaited to ensure clean teardown.Without waiting for the close callback, the test process may exit before sockets are fully released, potentially causing flaky tests or port conflicts in subsequent runs.
♻️ Proposed fix
-afterAll(() => { server.close(); }); +afterAll(async () => { + await new Promise<void>((resolve, reject) => { + server.close((err) => (err ? reject(err) : resolve())); + }); +});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/services/greenfield-processor/src/fetch-with-pinned-dns.test.ts` at line 27, The teardown currently calls server.close() without awaiting it, which can cause flaky tests; update the afterAll handler to return or await the server.close completion (e.g., make the afterAll callback async and await a promisified server.close or return a Promise that resolves in the close callback) so the test runner waits for sockets to be released; target the existing afterAll and server.close() invocation in fetch-with-pinned-dns.test.ts when making this change.backend/services/greenfield-processor/src/classify-event.ts (1)
1-6:application/x-brotlishould use exact match instead of prefix.Unlike
image/,video/,audio/(which are MIME type families),application/x-brotliis a specific MIME type. UsingstartsWithwould incorrectly match hypothetical types likeapplication/x-brotli-compressed.♻️ Suggested refinement
-const UPLOADABLE_PREFIXES = [ +const UPLOADABLE_PREFIXES = [ 'image/', 'video/', 'audio/', - 'application/x-brotli', ]; + +const UPLOADABLE_EXACT = new Set(['application/x-brotli']);Then update the check:
+ if (UPLOADABLE_EXACT.has(mimeBase)) return 'upload'; + const isUploadable = UPLOADABLE_PREFIXES.some((prefix) => mimeBase.startsWith(prefix), );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/services/greenfield-processor/src/classify-event.ts` around lines 1 - 6, The UPLOADABLE_PREFIXES entry "application/x-brotli" is a full MIME type but is being treated as a prefix; change the logic so only entries that are true families (e.g., strings ending with "/") are used with startsWith, and exact types are compared with equality. Update the UPLOADABLE_PREFIXES (or split into UPLOADABLE_PREFIXES and UPLOADABLE_EXACT) and modify the classifier check (the code that iterates these and currently calls startsWith) to first test exact equality for non-family entries like "application/x-brotli" and use startsWith only for entries that end with "/" (e.g., "image/", "video/", "audio/").backend/services/greenfield-processor/src/upload-to-bunny-ftp.test.ts (1)
7-41: Use isolated temp directories and cleanup to reduce test flakinessLine 7 uses a shared fixed temp directory and generated files are not removed. Prefer
mkdtempper test (or suite) plus cleanup inafterEach/afterAllto avoid residue across runs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/services/greenfield-processor/src/upload-to-bunny-ftp.test.ts` around lines 7 - 41, Replace the shared TEST_DIR with a per-test temporary directory created via mkdtemp and ensure cleanup after each test: make TEST_DIR a let that is assigned in beforeEach by calling fs.mkdtemp (or equivalent) and update createTestFile to use that variable; add an afterEach that removes the temp directory (rm -r or fs.rm with recursive) and reset testCounter as needed so generated files are isolated and removed between tests (update references in beforeEach, createTestFile, and add afterEach cleanup).backend/services/greenfield-processor/src/acquire-item-lock.test.ts (1)
11-63: Consider adding failure-path tests for Redis command rejectionsA small addition for
redis.set/redis.evalrejection cases would verify that lock acquisition/release errors propagate as expected.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/services/greenfield-processor/src/acquire-item-lock.test.ts` around lines 11 - 63, Add tests that simulate Redis command rejections to ensure errors propagate: in acquire-item-lock.test.ts add one test where createMockRedis makes redis.set reject (e.g., returns a rejected promise) and assert acquireItemLock('...') rejects with that error, and another where acquireItemLock successfully acquires but redis.eval is mocked to reject and assert result.release() rejects and that redis.eval was called with the expected args (referencing acquireItemLock, redis.set, and redis.eval to locate code). Ensure mocks return rejected Promises and assertions use async/await with expect(...).rejects to verify propagation.backend/services/greenfield-processor/src/upload-to-bunny-http.ts (1)
34-34: Extract timeout into a named constantLine 34 uses
120_000inline. A named constant improves readability and avoids magic numbers.As per coding guidelines: "Avoid magic numbers..."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/services/greenfield-processor/src/upload-to-bunny-http.ts` at line 34, Extract the inline timeout number used in the AbortSignal call into a named constant (e.g., UPLOAD_TIMEOUT_MS or BUNNY_UPLOAD_TIMEOUT_MS) so the code reads signal: AbortSignal.timeout(UPLOAD_TIMEOUT_MS); define the constant near the top of upload-to-bunny-http.ts (module scope) with a descriptive name and use that constant instead of 120_000 to remove the magic number and improve readability.backend/services/greenfield-processor/src/resolve-storage-provider.ts (2)
24-24: Minor: redundant|| undefined.Empty string is already falsy, so
allowedSpHostnamePattern || undefinedis equivalent to justallowedSpHostnamePatternwhen passed to a function expectingstring | undefined.- cached, allowedSpHostnamePattern || undefined, + cached, allowedSpHostnamePattern,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/services/greenfield-processor/src/resolve-storage-provider.ts` at line 24, Remove the redundant "|| undefined" when passing allowedSpHostnamePattern into the call — replace the argument expression "allowedSpHostnamePattern || undefined" with just "allowedSpHostnamePattern" (the call where variables like cached and allowedSpHostnamePattern are passed, e.g., in resolveStorageProvider or its invocation). This keeps the parameter type string | undefined intact while avoiding the unnecessary fallback expression.
14-19:resolveStorageProviderexceeds the 3-parameter limit.Per coding guidelines, functions should use an options object beyond 3 parameters. This function has 4.
♻️ Proposed refactor using options object
+interface ResolveStorageProviderOptions { + bucketName: string; + greenfieldClient: GreenfieldQueryClient; + redis: Redis; + allowedSpHostnamePattern?: string; +} + export default async function resolveStorageProvider( - bucketName: string, - greenfieldClient: GreenfieldQueryClient, - redis: Redis, - allowedSpHostnamePattern?: string, + options: ResolveStorageProviderOptions, ): Promise<ResolvedStorageProvider> { + const { bucketName, greenfieldClient, redis, allowedSpHostnamePattern } = options;As per coding guidelines: "Enforce maximum function parameters of 3, using options object beyond 3 parameters".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/services/greenfield-processor/src/resolve-storage-provider.ts` around lines 14 - 19, The function resolveStorageProvider currently takes four parameters which violates the 3-parameter rule; refactor it to accept a single options object instead. Change the signature of resolveStorageProvider to accept an options parameter (e.g., { bucketName, greenfieldClient, redis, allowedSpHostnamePattern? }) and update all internal references and callers to destructure those properties; update any related types (ResolvedStorageProvider usages, call sites) to pass the new options object. Ensure exported default remains resolveStorageProvider and preserve runtime behavior and types when converting parameters to properties on the options object.backend/services/greenfield-processor/src/upload-to-bunny-ftp.ts (1)
30-36:executeUploadexceeds the 3-parameter limit.Per coding guidelines, functions should have a maximum of 3 parameters, using an options object beyond that. This function has 5 parameters.
♻️ Proposed refactor using options object
+interface ExecuteUploadOptions { + client: Client; + localFilePath: string; + remotePath: string; + localSize: number; + remoteSize: number; +} + async function executeUpload( - client: Client, - localFilePath: string, - remotePath: string, - localSize: number, - remoteSize: number, + options: ExecuteUploadOptions, ): Promise<void> { + const { client, localFilePath, remotePath, localSize, remoteSize } = options; if (remoteSize === localSize) return;Then update the call site:
- await executeUpload(ftpClient, localFilePath, remotePath, localSize, remoteSize); + await executeUpload({ client: ftpClient, localFilePath, remotePath, localSize, remoteSize });As per coding guidelines: "Enforce maximum function parameters of 3, using options object beyond 3 parameters".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/services/greenfield-processor/src/upload-to-bunny-ftp.ts` around lines 30 - 36, The function executeUpload currently has five parameters (client, localFilePath, remotePath, localSize, remoteSize) which violates the 3-parameter rule; refactor executeUpload to accept (client: Client, opts: { localFilePath: string; remotePath: string; localSize: number; remoteSize: number }) or fully to a single options object (e.g., opts: { client: Client; localFilePath: string; remotePath: string; localSize: number; remoteSize: number }) and update all call sites to pass the new options object or destructured properties accordingly; ensure the implementation references opts.localFilePath, opts.remotePath, etc., and keep the Client type and function name executeUpload unchanged for easy lookup.backend/services/greenfield-processor/src/validate-endpoint.ts (1)
99-105: Duplicate regex compilation (minor).The hostname pattern is compiled here and also in
config.tsduring validation. Since config validation happens at startup, this second compilation is safe but redundant. Consider caching the compiled regex if performance becomes a concern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/services/greenfield-processor/src/validate-endpoint.ts` around lines 99 - 105, The assertHostnameAllowed function recompiles the same anchored pattern into a RegExp each call; cache compiled regexes keyed by the anchored pattern (e.g., a module-level Map<string, RegExp>) and use that cache instead of new RegExp every time so subsequent calls use the cached RegExp; reference assertHostnameAllowed and anchorPattern when adding the cache and ensure the cache lookup/insert happens before calling regex.test(hostname).backend/services/greenfield-processor/src/server.ts (4)
90-94: Consider adding timeout to worker close operations.Worker close operations could hang if a job is stuck. Consider wrapping with a timeout to ensure shutdown completes within a reasonable timeframe.
♻️ Suggested pattern
const SHUTDOWN_TIMEOUT_MS = 30_000; async function closeWithTimeout( closeFn: () => Promise<void>, label: string, logger: pino.Logger, ): Promise<void> { const timeout = new Promise<never>((_, reject) => setTimeout(() => reject(new Error(`${label} close timed out`)), SHUTDOWN_TIMEOUT_MS) ); await Promise.race([closeFn(), timeout]).catch((err) => { logger.error({ err }, `${label} close error`); }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/services/greenfield-processor/src/server.ts` around lines 90 - 94, Add a timeout wrapper when closing each worker to avoid hangs: implement a SHUTDOWN_TIMEOUT_MS constant and a helper like closeWithTimeout(closeFn, label, logger) that races worker.close() against a timeout Promise and logs via logger.error on rejection; replace the current await worker.close().catch(...) loop to call closeWithTimeout(() => worker.close(), `worker-${idOrIndex}`, logger) for each worker so slow/stuck closes are bounded and still logged safely.
53-57: Shutdown order: FTP pool should drain before batch queue closes.The
batchQueueis in the resources array afterftpPool, but in-flight uploads may still need FTP connections. Consider ensuringbatchQueuedrains before closing the FTP pool, or movingftpPoolafterbatchQueuein the resources array.♻️ Suggested reorder
const shutdown = createShutdown( [routerWorker, uploadWorker, flushWorker], - [redis, app, ftpPool, batchQueue], + [batchQueue, ftpPool, app, redis], logger, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/services/greenfield-processor/src/server.ts` around lines 53 - 57, The shutdown sequence currently passes resources to createShutdown with ftpPool before batchQueue, which risks closing FTP connections while batchQueue is still processing; update the resources array in the createShutdown call so batchQueue appears before ftpPool (i.e., ensure batchQueue is closed/drained prior to draining/closing ftpPool) by reordering the arguments passed to createShutdown in server.ts where shutdown is constructed using createShutdown([routerWorker, uploadWorker, flushWorker], [redis, app, ftpPool, batchQueue], logger).
63-70: Type cast throughunknownis fragile.The double cast
as unknown as GreenfieldQueryClientbypasses type safety. While the@ts-expect-errorcomment explains the lack of type declarations, consider adding a runtime validation or at minimum documenting which methods are expected on the interface.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/services/greenfield-processor/src/server.ts` around lines 63 - 70, The createGreenfieldClient function is using a fragile double cast via Client.create(...) as unknown as GreenfieldQueryClient; replace this by validating the returned object at runtime (or constructing a wrapper) to ensure it implements the GreenfieldQueryClient surface (e.g., check presence/types of expected methods like queryMethodX, queryMethodY) instead of blind casting, or add a small adapter that calls Client.create and maps/throws if required methods are missing; update the function to perform these runtime checks and/or document the exact expected methods on GreenfieldQueryClient and why the adapter is used, referencing createGreenfieldClient and Client.create.
16-61: Main function exceeds 30-line limit.At 46 lines,
main()exceeds the 30-line guideline. This is a common pattern for service entrypoints, but consider extracting dependency initialization into a separatecreateDependencies()function.As per coding guidelines: "Enforce maximum function length of 30 lines"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/services/greenfield-processor/src/server.ts` around lines 16 - 61, The main() function is too long; extract dependency initialization into a new createDependencies() helper that returns the initialized objects (config, logger, redis, greenfieldClient, ftpPool, metrics, batchQueue, flushWorker, routerWorker, uploadWorker, app or a subset needed by main) so main() becomes the high-level orchestration (call cleanupStaleFiles, await createDependencies(), registerRoutes, app.listen, logger.info, createShutdown, and signal handlers). Move the calls that build resources and workers (loadConfig, pino, cleanupStaleFiles invocation, createRedisClient, createGreenfieldClient, createFtpPool, collectMetrics, createBatchAssembler, createRouterWorker, createUploadWorker, and Fastify instantiation) into createDependencies(), return the created symbols, and update main() to use those returned symbols and keep calls to registerRoutes() and createShutdown() (referencing registerRoutes and createShutdown) so main() stays under 30 lines.backend/services/greenfield-processor/src/download-from-greenfield.test.ts (2)
78-80: Consider adding mock cleanup in beforeEach.While the file system cleanup is handled correctly, consider adding
vi.clearAllMocks()inbeforeEachto ensure mock call counts are reset between tests, which helps prevent test interdependence.📝 Proposed fix
beforeEach(async () => { + vi.clearAllMocks(); await rm(TEST_DIR, { recursive: true, force: true }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/services/greenfield-processor/src/download-from-greenfield.test.ts` around lines 78 - 80, Add mock cleanup to the test setup: in the beforeEach that currently calls rm(TEST_DIR, { recursive: true, force: true }), also call vi.clearAllMocks() so any mocked functions are reset between tests; update the beforeEach block (where rm and TEST_DIR are used) to include vi.clearAllMocks() before or after the filesystem cleanup to ensure mock call counts and states are cleared for each test.
1-314: Test file exceeds 250-line guideline.At 314 lines, this file exceeds the 250-line limit from coding guidelines. However, the tests are well-organized and cover security-critical functionality (path traversal, size limits, checksum verification, redirect rejection). Consider whether splitting is worthwhile given the cohesion of testing a single module.
As per coding guidelines: "Enforce maximum file length of 250 lines using ESLint
max-linesrule"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/services/greenfield-processor/src/download-from-greenfield.test.ts` around lines 1 - 314, The test file exceeds the 250-line rule; split or extract shared setup to reduce length. Move reusable pieces (TEST_DIR, TEST_BODY, MAX_DOWNLOAD_SIZE, SEGMENT_SIZE, computeExpectedHash, server setup/teardown and beforeAll/afterAll hooks) into a new test helper module (e.g., test-utils) and import them into this file, or split the suite into two focused spec files (e.g., download-from-greenfield.unit.test.ts for validation/security tests and download-from-greenfield.flow.test.ts for download/resume flows) while keeping the core function under test (downloadFromGreenfield) intact and preserving individual test names and expectations.backend/services/greenfield-processor/src/upload-worker.ts (2)
46-89: Function exceeds 30-line limit and has 6 parameters.
processBatchis 44 lines and has 6 parameters, exceeding both the 30-line and 3-parameter guidelines. Consider extracting the retry/dead-letter logic into a separate function and using an options object for dependencies.As per coding guidelines: "Enforce maximum function length of 30 lines" and "Enforce maximum function parameters of 3, using options object beyond 3 parameters"
♻️ Suggested refactor pattern
interface ProcessBatchDeps { config: CdnUploaderConfig; redis: Redis; greenfieldClient: GreenfieldQueryClient; ftpPool: FtpPool; logger: Logger; } async function processBatch( job: Job<UploadBatchJob>, deps: ProcessBatchDeps, ): Promise<void> { // ... implementation }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/services/greenfield-processor/src/upload-worker.ts` around lines 46 - 89, The processBatch function exceeds length and parameter limits: refactor by replacing the 5 dependency parameters (config, redis, greenfieldClient, ftpPool, logger) with a single deps object (e.g., ProcessBatchDeps) and extract the per-item retry/dead-letter handling into a helper (e.g., handleItemFailure or processWithRetries) so processBatch only iterates items, calls processItem, calls updateProgress, and delegates retry counting, dead-letter accumulation, and failCount increments to the new helper which will reference MAX_RETRIES, itemKey, updateProgress, and pushToDeadLetter; ensure the new helper updates failCounts and returns whether the batch should count a failure or add to deadLettered so processBatch remains <30 lines and uses at most 2 parameters (job, deps).
144-187: Function exceeds 30-line limit and has 8 parameters.
processItemCoreis 44 lines with 8 parameters. The download-upload-store sequence could be extracted, and dependencies should use an options object.As per coding guidelines: "Enforce maximum function length of 30 lines" and "Enforce maximum function parameters of 3, using options object beyond 3 parameters"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/services/greenfield-processor/src/upload-worker.ts` around lines 144 - 187, processItemCore is too long and has too many positional params; refactor it to accept a single options object (e.g., ProcessItemOptions) instead of 8 positional parameters and split the download->upload->store flow into smaller helpers: extract the download logic into a function (e.g., downloadFromGreenfieldOrTemp) that calls downloadFromGreenfield and returns localPath, extract the upload/cleanup logic that uses ftpPool.acquire/release and selectAndUpload into a function (e.g., uploadLocalFileToCdn) which performs upload and ensures unlink in its finally, and extract the metadata persistence into a small helper (e.g., persistUploadMetadata) that wraps storeUploadMetadata; then make processItemCore a thin coordinator (<=30 lines) that resolves provider via resolveAndCacheProvider, calls those helpers, and logs the final result.backend/services/greenfield-processor/ARCHITECTURE.md (2)
9-27: Add language specifier to fenced code block.The diagram code block should have a language specifier to satisfy markdown lint rules and improve rendering.
📝 Proposed fix
-``` +```text greenfield-ingester (Go) --> BullMQ: greenfield-events🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/services/greenfield-processor/ARCHITECTURE.md` around lines 9 - 27, The fenced code block in ARCHITECTURE.md lacks a language specifier; update the triple-backtick fence that wraps the diagram (the block starting with the line containing "greenfield-ingester (Go) --> BullMQ: greenfield-events") to include a language tag such as text or mermaid (e.g., change ``` to ```text) so the markdown linter and renderer recognize the block type while leaving the diagram contents unchanged.
79-79: Document additional rejected IP ranges.The SSRF protection section lists common private ranges but omits CGNAT (100.64.x.x), benchmark (198.18.x.x), and reserved (240.x.x.x) ranges that the implementation also rejects (as verified by tests in
validate-endpoint.test.tslines 195-208).📝 Suggested update
-- DNS resolution performed; resolved IP checked against private IPv4 (10.x, 172.x, 192.168.x, 127.x, 169.254.x, 0.x) and IPv6 (::1, fe80::/10, fc00::/7, ::ffff: mapped) ranges +- DNS resolution performed; resolved IP checked against private IPv4 (10.x, 172.16-31.x, 192.168.x, 127.x, 169.254.x, 0.x, 100.64.x CGNAT, 198.18.x benchmark, 240.x reserved) and IPv6 (::1, ::, fe80::/10, fc00::/7, ::ffff: mapped, 2002:: 6to4, 64:ff9b:: NAT64) ranges🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/services/greenfield-processor/ARCHITECTURE.md` at line 79, Update the SSRF protection line in ARCHITECTURE.md to list all IP ranges the implementation actually rejects: add CGNAT (100.64.0.0/10), benchmark/test networks (198.18.0.0/15), and reserved/experimental space (240.0.0.0/4) alongside the existing private and IPv6 ranges; reference the validate-endpoint.test.ts checks (around lines 195–208) to ensure wording matches the test-covered ranges and keep the list concise and in CIDR notation for clarity.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0667809d-3498-4e4b-8e9b-6c0ea8059b4e
⛔ Files ignored due to path filters (1)
backend/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (53)
backend/.claude/architecture.mdbackend/packages/greenfield-client/parser.gobackend/packages/greenfield-client/parser_test.gobackend/services/docker-compose.ymlbackend/services/greenfield-processor/ARCHITECTURE.mdbackend/services/greenfield-processor/Dockerfilebackend/services/greenfield-processor/Makefilebackend/services/greenfield-processor/package.jsonbackend/services/greenfield-processor/src/acquire-item-lock.test.tsbackend/services/greenfield-processor/src/acquire-item-lock.tsbackend/services/greenfield-processor/src/batch-assembler.tsbackend/services/greenfield-processor/src/classify-event.test.tsbackend/services/greenfield-processor/src/classify-event.tsbackend/services/greenfield-processor/src/cleanup-stale-files.test.tsbackend/services/greenfield-processor/src/cleanup-stale-files.tsbackend/services/greenfield-processor/src/collect-metrics.tsbackend/services/greenfield-processor/src/compute-integrity-hash.test.tsbackend/services/greenfield-processor/src/compute-integrity-hash.tsbackend/services/greenfield-processor/src/config.test.tsbackend/services/greenfield-processor/src/config.tsbackend/services/greenfield-processor/src/create-batch.tsbackend/services/greenfield-processor/src/create-ftp-pool.test.tsbackend/services/greenfield-processor/src/create-ftp-pool.tsbackend/services/greenfield-processor/src/create-pinned-dispatcher.test.tsbackend/services/greenfield-processor/src/create-pinned-dispatcher.tsbackend/services/greenfield-processor/src/create-redis-client.tsbackend/services/greenfield-processor/src/create-size-limiter.test.tsbackend/services/greenfield-processor/src/create-size-limiter.tsbackend/services/greenfield-processor/src/download-from-greenfield.test.tsbackend/services/greenfield-processor/src/download-from-greenfield.tsbackend/services/greenfield-processor/src/fetch-with-pinned-dns.test.tsbackend/services/greenfield-processor/src/fetch-with-pinned-dns.tsbackend/services/greenfield-processor/src/resolve-content-type.tsbackend/services/greenfield-processor/src/resolve-storage-provider.test.tsbackend/services/greenfield-processor/src/resolve-storage-provider.tsbackend/services/greenfield-processor/src/router-worker.tsbackend/services/greenfield-processor/src/routes.tsbackend/services/greenfield-processor/src/select-upload-strategy.tsbackend/services/greenfield-processor/src/server.tsbackend/services/greenfield-processor/src/store-upload-metadata.test.tsbackend/services/greenfield-processor/src/store-upload-metadata.tsbackend/services/greenfield-processor/src/types.tsbackend/services/greenfield-processor/src/upload-to-bunny-ftp.test.tsbackend/services/greenfield-processor/src/upload-to-bunny-ftp.tsbackend/services/greenfield-processor/src/upload-to-bunny-http.test.tsbackend/services/greenfield-processor/src/upload-to-bunny-http.tsbackend/services/greenfield-processor/src/upload-worker.test.tsbackend/services/greenfield-processor/src/upload-worker.tsbackend/services/greenfield-processor/src/validate-endpoint.test.tsbackend/services/greenfield-processor/src/validate-endpoint.tsbackend/services/greenfield-processor/src/validate.test.tsbackend/services/greenfield-processor/src/validate.tsbackend/services/greenfield-processor/tsconfig.json
| GREENFIELD_PRIVATE_KEY: ${GREENFIELD_PRIVATE_KEY} | ||
| ONLINEIO_ENV: ${ONLINEIO_ENV:-dev} | ||
| REDIS_URL: redis://dragonfly:6379 | ||
| REDIS_URL: redis://:${REDIS_PASSWORD}@dragonfly:6379 |
There was a problem hiding this comment.
Fail fast when REDIS_PASSWORD is unset.
Compose substitutes an empty string for ${REDIS_PASSWORD} when the variable is missing, so this stack can still start without a real Redis password. Use the required-value form (${REDIS_PASSWORD:?REDIS_PASSWORD is required}) anywhere auth depends on it.
Also applies to: 28-36, 47-47, 74-74
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/services/docker-compose.yml` at line 11, Update all uses of the
REDIS_PASSWORD env substitution to the required-value form so the stack fails
fast when the password is missing; specifically replace occurrences used in
REDIS_URL and any other env entries referencing REDIS_PASSWORD with
${REDIS_PASSWORD:?REDIS_PASSWORD is required} (affects the REDIS_URL lines and
the other env lines noted), ensuring the docker-compose service env entries now
use the required-value substitution for REDIS_PASSWORD.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
backend/services/docker-compose.yml (1)
11-11:⚠️ Potential issue | 🟠 MajorUse required-value interpolation for Redis password in all
REDIS_URLentries.Line 11, Line 47, and Line 75 still use
${REDIS_PASSWORD}instead of required-value form, so password enforcement is inconsistent across auth-dependent env values. Use${REDIS_PASSWORD:?REDIS_PASSWORD is required}in all threeREDIS_URLvalues.🔧 Proposed fix
- REDIS_URL: redis://:${REDIS_PASSWORD}@dragonfly:6379 + REDIS_URL: redis://:${REDIS_PASSWORD:?REDIS_PASSWORD is required}@dragonfly:6379 @@ - REDIS_URL: redis://:${REDIS_PASSWORD}@dragonfly:6379 + REDIS_URL: redis://:${REDIS_PASSWORD:?REDIS_PASSWORD is required}@dragonfly:6379 @@ - REDIS_URL: redis://:${REDIS_PASSWORD}@dragonfly:6379 + REDIS_URL: redis://:${REDIS_PASSWORD:?REDIS_PASSWORD is required}@dragonfly:6379#!/bin/bash # Verify all REDIS_URL entries use required-value interpolation for REDIS_PASSWORD rg -n 'REDIS_URL:\s*redis://:\$\{REDIS_PASSWORD' backend/services/docker-compose.ymlExpected result: every reported line should contain
:?REDIS_PASSWORD is required}.Also applies to: 47-47, 75-75
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/services/docker-compose.yml` at line 11, Replace the three REDIS_URL environment values that currently interpolate the password as ${REDIS_PASSWORD} with the required-value form ${REDIS_PASSWORD:?REDIS_PASSWORD is required} so the compose file enforces a password; locate the REDIS_URL entries in backend/services/docker-compose.yml (the three REDIS_URL keys) and update their values to use the required-value interpolation syntax.
🧹 Nitpick comments (7)
backend/services/greenfield-processor/src/router-worker.ts (2)
107-128:resolveEventContentTypeexceeds the 3-parameter limit.The function takes 6 parameters. Use an options object:
♻️ Suggested refactor
+interface ResolveContentTypeDeps { + data: GreenfieldEventPayload; + bucketName: string; + objectName: string; + greenfieldClient: GreenfieldQueryClient; + redis: Redis; + allowedSpHostnamePattern?: string; +} + async function resolveEventContentType( - data: GreenfieldEventPayload, - bucketName: string, - objectName: string, - greenfieldClient: GreenfieldQueryClient, - redis: Redis, - allowedSpHostnamePattern?: string, + deps: ResolveContentTypeDeps, ): Promise<string> { + const { data, bucketName, objectName, greenfieldClient, redis, allowedSpHostnamePattern } = deps;As per coding guidelines: "Enforce maximum function parameters of 3, using options object beyond 3 parameters".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/services/greenfield-processor/src/router-worker.ts` around lines 107 - 128, The function resolveEventContentType currently takes six parameters; refactor it to accept at most three by replacing the latter parameters with a single options object (e.g., signature: resolveEventContentType(data: GreenfieldEventPayload, bucketName: string, opts: { objectName: string; greenfieldClient: GreenfieldQueryClient; redis: Redis; allowedSpHostnamePattern?: string })). Update internal references to use opts.objectName, opts.greenfieldClient, etc., and update all callers to pass the new opts object; also ensure resolveContentType call uses opts fields and resolveStorageProvider is invoked with opts.greenfieldClient, opts.redis and opts.allowedSpHostnamePattern.
48-93:processEventexceeds the 30-line function limit.The function spans ~45 lines. Consider extracting the item construction and queue management into separate helpers.
♻️ Potential extraction
function buildPendingItem( data: GreenfieldEventPayload, contentType: string, ): PendingUploadItem { return { bucketName: data.bucket_name, objectName: data.object_name, payloadSize: data.payload_size, contentType, txHash: data.tx_hash, version: data.version, checksums: data.checksums, }; } async function enqueueAndMaybeFlush( deps: { redis: Redis; batchQueue: Queue<UploadBatchJob>; flushQueue: Queue; config: CdnUploaderConfig; logger: Logger }, item: PendingUploadItem, ): Promise<void> { const length = await pushAndGetLength(deps.redis, JSON.stringify(item)); if (length >= deps.config.batchMaxSize) { await assembleBatch(deps.redis, deps.batchQueue, deps.config, deps.logger); } else { await scheduleDelayedFlush(deps.flushQueue, deps.config.batchFlushIntervalMs); } }As per coding guidelines: "Enforce maximum function length of 30 lines".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/services/greenfield-processor/src/router-worker.ts` around lines 48 - 93, processEvent is too long; extract the PendingUploadItem construction and the queue/flush logic into two helpers: implement buildPendingItem(data, contentType) that returns PendingUploadItem using data.bucket_name/object_name/etc., and implement enqueueAndMaybeFlush({redis, batchQueue, flushQueue, config, logger}, item) which calls pushAndGetLength(redis, JSON.stringify(item)) and then either calls assembleBatch(redis, batchQueue, config, logger) when length >= config.batchMaxSize or scheduleDelayedFlush(flushQueue, config.batchFlushIntervalMs) otherwise; replace the inline item creation and queue code in processEvent with calls to these two helpers.backend/services/greenfield-processor/src/batch-assembler.ts (1)
37-61:assembleBatchexceeds the 3-parameter limit.The function takes 4 parameters (
redis,batchQueue,config,logger). Per coding guidelines, functions should use an options object beyond 3 parameters.♻️ Suggested refactor
+interface AssembleBatchDeps { + redis: Redis; + batchQueue: Queue<UploadBatchJob>; + config: CdnUploaderConfig; + logger: Logger; +} + export async function assembleBatch( - redis: Redis, - batchQueue: Queue<UploadBatchJob>, - config: CdnUploaderConfig, - logger: Logger, + deps: AssembleBatchDeps, ): Promise<void> { + const { redis, batchQueue, config, logger } = deps; const items = await createBatch(redis, config.batchMaxSize);As per coding guidelines: "Enforce maximum function parameters of 3, using options object beyond 3 parameters".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/services/greenfield-processor/src/batch-assembler.ts` around lines 37 - 61, The function assembleBatch currently accepts four positional parameters (redis, batchQueue, config, logger) which violates the 3-parameter limit; refactor it to accept at most three parameters by grouping the latter arguments into an options object (e.g., assembleBatch(redis, batchQueue, { config, logger }) or assembleBatch({ redis, batchQueue, config, logger }) ), update all call sites to pass the new object shape, and adjust internal references inside assembleBatch to destructure the options for config and logger; ensure types/interfaces (CdnUploaderConfig, Logger, Queue<UploadBatchJob>, Redis) are updated accordingly to keep type safety.backend/services/greenfield-processor/src/download-from-greenfield.ts (1)
18-69:downloadFromGreenfieldexceeds the 30-line function limit.The main function spans ~52 lines. Consider extracting logical sections into helper functions to improve readability and comply with the max-lines-per-function guideline.
♻️ Potential extraction
// Extract the write phase into a helper async function executeDownload( response: UndiciResponse, opts: { partPath: string; existingSize: number; maxDownloadSize: number; logger?: Logger }, ): Promise<void> { const writeMode = determineWriteMode(response, opts.existingSize); if (writeMode === 'truncate' && opts.existingSize > 0) { await unlink(opts.partPath).catch((err) => { opts.logger?.debug({ err, partPath: opts.partPath }, 'failed to unlink stale part file'); }); } const startingBytes = writeMode === 'append' ? opts.existingSize : 0; const remainingAllowance = opts.maxDownloadSize - startingBytes; if (remainingAllowance <= 0) { await unlink(opts.partPath).catch(() => {}); throw new AppError('Partial file already exceeds max download size', 413); } await writeResponseToFile(response, { filePath: opts.partPath, mode: writeMode, maxBytes: remainingAllowance }); }As per coding guidelines: "Enforce maximum function length of 30 lines".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/services/greenfield-processor/src/download-from-greenfield.ts` around lines 18 - 69, The function downloadFromGreenfield is too long; extract the download/write logic into a helper (e.g., executeDownload) that takes the response, partPath, existingSize, maxDownloadSize and logger and encapsulates determineWriteMode, the stale unlink logic, remainingAllowance check, and the call to writeResponseToFile; keep post-download steps (verifyDownloadSize, verifyIntegrityHash, rename) in downloadFromGreenfield and ensure cleanup() remains in the original try/finally so behavior is unchanged.backend/services/greenfield-processor/src/upload-worker.ts (3)
130-130: Duplicate lock key construction.Line 130 constructs the lock key manually (
${item.bucketName}:${item.objectName}:${item.version}) but the exporteditemKey(item)function at line 91-93 does the same. Reuse it:♻️ Suggested fix
async function processItem( item: PendingUploadItem, // ... ): Promise<void> { - const lockKey = `${item.bucketName}:${item.objectName}:${item.version}`; + const lockKey = itemKey(item); const lock = await acquireItemLock(redis, lockKey);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/services/greenfield-processor/src/upload-worker.ts` at line 130, Replace the manual lock key construction with the existing helper: instead of building lockKey via `${item.bucketName}:${item.objectName}:${item.version}`, call the exported itemKey(item) helper to produce the same string; update the code that currently assigns to lockKey to use itemKey(item) so the helper is reused and duplication removed (references: lockKey variable and itemKey(item) function).
121-142:processItemhas 8 parameters.This significantly exceeds the 3-parameter limit. Bundle dependencies into an object:
♻️ Suggested refactor
+interface ProcessItemDeps { + config: CdnUploaderConfig; + redis: Redis; + greenfieldClient: GreenfieldQueryClient; + ftpPool: FtpPool; + logger: Logger; + spCache: Map<string, ResolvedStorageProvider>; +} + async function processItem( item: PendingUploadItem, - config: CdnUploaderConfig, - redis: Redis, - greenfieldClient: GreenfieldQueryClient, - ftpPool: FtpPool, - logger: Logger, - spCache: Map<string, ResolvedStorageProvider>, + deps: ProcessItemDeps, ): Promise<void> {As per coding guidelines: "Enforce maximum function parameters of 3, using options object beyond 3 parameters".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/services/greenfield-processor/src/upload-worker.ts` around lines 121 - 142, The function processItem currently accepts 8 parameters which violates the 3-parameter guideline; refactor by grouping dependencies into a single options object (e.g., rename signature to processItem(item: PendingUploadItem, opts: { config: CdnUploaderConfig; redis: Redis; greenfieldClient: GreenfieldQueryClient; ftpPool: FtpPool; logger: Logger; spCache: Map<string, ResolvedStorageProvider> }): Promise<void> ), update all callers to pass the grouped opts, and adapt internal uses (acquireItemLock, processItemCore, and lock.release) to read from the opts object; keep the lock key logic and try/finally semantics unchanged and ensure processItemCore is updated (or overloaded) if it needs the new opts shape.
46-89:processBatchexceeds both parameter and line limits.The function has 6 parameters (limit: 3) and spans ~44 lines (limit: 30). Consider using a deps object and extracting the retry/dead-letter logic.
♻️ Suggested refactor for parameters
+interface ProcessBatchDeps { + config: CdnUploaderConfig; + redis: Redis; + greenfieldClient: GreenfieldQueryClient; + ftpPool: FtpPool; + logger: Logger; +} + async function processBatch( job: Job<UploadBatchJob>, - config: CdnUploaderConfig, - redis: Redis, - greenfieldClient: GreenfieldQueryClient, - ftpPool: FtpPool, - logger: Logger, + deps: ProcessBatchDeps, ): Promise<void> { + const { config, redis, greenfieldClient, ftpPool, logger } = deps;As per coding guidelines: "Enforce maximum function parameters of 3" and "Enforce maximum function length of 30 lines".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/services/greenfield-processor/src/upload-worker.ts` around lines 46 - 89, The processBatch function exceeds allowed parameter (6) and length limits (~44 lines); refactor processBatch to accept a single deps/config object (e.g., { config, redis, greenfieldClient, ftpPool, logger }) plus the Job (or conversely Job and deps) so the parameter count is <=3, and extract the retry/dead-letter handling into one or two small helpers (e.g., handleRetryForItem(item, key, err, failCounts, deadLettered, logger) and finalizeDeadLetters(redis, deadLettered)) as well as the progress update into a helper (e.g., persistProgress(job, completed, failCounts)) so the main loop only orchestrates processing via processItem, updates completed, and delegates retry/dead-letter logic; keep references to existing symbols: processBatch, processItem, updateProgress, pushToDeadLetter, MAX_RETRIES, itemKey, getCompletedKeys, getPersistedFailCounts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/services/greenfield-processor/src/app-error.ts`:
- Around line 1-10: The AppError class must include a machine-readable errorCode
and align with the shared errors utility; either implement a shared error module
or extend this class: add a readonly errorCode: string property to the AppError
constructor (alongside the existing statusCode, defaulting statusCode=500 and a
sensible default errorCode like 'UNKNOWN_ERROR' or accept it as a constructor
param), ensure Error.captureStackTrace is preserved, export/move the class into
shared/utils/errors.ts if creating the shared utility, and update any imports to
use the new shared/utils/errors.ts AppError so all services consume the unified
error type.
---
Duplicate comments:
In `@backend/services/docker-compose.yml`:
- Line 11: Replace the three REDIS_URL environment values that currently
interpolate the password as ${REDIS_PASSWORD} with the required-value form
${REDIS_PASSWORD:?REDIS_PASSWORD is required} so the compose file enforces a
password; locate the REDIS_URL entries in backend/services/docker-compose.yml
(the three REDIS_URL keys) and update their values to use the required-value
interpolation syntax.
---
Nitpick comments:
In `@backend/services/greenfield-processor/src/batch-assembler.ts`:
- Around line 37-61: The function assembleBatch currently accepts four
positional parameters (redis, batchQueue, config, logger) which violates the
3-parameter limit; refactor it to accept at most three parameters by grouping
the latter arguments into an options object (e.g., assembleBatch(redis,
batchQueue, { config, logger }) or assembleBatch({ redis, batchQueue, config,
logger }) ), update all call sites to pass the new object shape, and adjust
internal references inside assembleBatch to destructure the options for config
and logger; ensure types/interfaces (CdnUploaderConfig, Logger,
Queue<UploadBatchJob>, Redis) are updated accordingly to keep type safety.
In `@backend/services/greenfield-processor/src/download-from-greenfield.ts`:
- Around line 18-69: The function downloadFromGreenfield is too long; extract
the download/write logic into a helper (e.g., executeDownload) that takes the
response, partPath, existingSize, maxDownloadSize and logger and encapsulates
determineWriteMode, the stale unlink logic, remainingAllowance check, and the
call to writeResponseToFile; keep post-download steps (verifyDownloadSize,
verifyIntegrityHash, rename) in downloadFromGreenfield and ensure cleanup()
remains in the original try/finally so behavior is unchanged.
In `@backend/services/greenfield-processor/src/router-worker.ts`:
- Around line 107-128: The function resolveEventContentType currently takes six
parameters; refactor it to accept at most three by replacing the latter
parameters with a single options object (e.g., signature:
resolveEventContentType(data: GreenfieldEventPayload, bucketName: string, opts:
{ objectName: string; greenfieldClient: GreenfieldQueryClient; redis: Redis;
allowedSpHostnamePattern?: string })). Update internal references to use
opts.objectName, opts.greenfieldClient, etc., and update all callers to pass the
new opts object; also ensure resolveContentType call uses opts fields and
resolveStorageProvider is invoked with opts.greenfieldClient, opts.redis and
opts.allowedSpHostnamePattern.
- Around line 48-93: processEvent is too long; extract the PendingUploadItem
construction and the queue/flush logic into two helpers: implement
buildPendingItem(data, contentType) that returns PendingUploadItem using
data.bucket_name/object_name/etc., and implement enqueueAndMaybeFlush({redis,
batchQueue, flushQueue, config, logger}, item) which calls
pushAndGetLength(redis, JSON.stringify(item)) and then either calls
assembleBatch(redis, batchQueue, config, logger) when length >=
config.batchMaxSize or scheduleDelayedFlush(flushQueue,
config.batchFlushIntervalMs) otherwise; replace the inline item creation and
queue code in processEvent with calls to these two helpers.
In `@backend/services/greenfield-processor/src/upload-worker.ts`:
- Line 130: Replace the manual lock key construction with the existing helper:
instead of building lockKey via
`${item.bucketName}:${item.objectName}:${item.version}`, call the exported
itemKey(item) helper to produce the same string; update the code that currently
assigns to lockKey to use itemKey(item) so the helper is reused and duplication
removed (references: lockKey variable and itemKey(item) function).
- Around line 121-142: The function processItem currently accepts 8 parameters
which violates the 3-parameter guideline; refactor by grouping dependencies into
a single options object (e.g., rename signature to processItem(item:
PendingUploadItem, opts: { config: CdnUploaderConfig; redis: Redis;
greenfieldClient: GreenfieldQueryClient; ftpPool: FtpPool; logger: Logger;
spCache: Map<string, ResolvedStorageProvider> }): Promise<void> ), update all
callers to pass the grouped opts, and adapt internal uses (acquireItemLock,
processItemCore, and lock.release) to read from the opts object; keep the lock
key logic and try/finally semantics unchanged and ensure processItemCore is
updated (or overloaded) if it needs the new opts shape.
- Around line 46-89: The processBatch function exceeds allowed parameter (6) and
length limits (~44 lines); refactor processBatch to accept a single deps/config
object (e.g., { config, redis, greenfieldClient, ftpPool, logger }) plus the Job
(or conversely Job and deps) so the parameter count is <=3, and extract the
retry/dead-letter handling into one or two small helpers (e.g.,
handleRetryForItem(item, key, err, failCounts, deadLettered, logger) and
finalizeDeadLetters(redis, deadLettered)) as well as the progress update into a
helper (e.g., persistProgress(job, completed, failCounts)) so the main loop only
orchestrates processing via processItem, updates completed, and delegates
retry/dead-letter logic; keep references to existing symbols: processBatch,
processItem, updateProgress, pushToDeadLetter, MAX_RETRIES, itemKey,
getCompletedKeys, getPersistedFailCounts.
🪄 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: 3ce83fc2-94ae-442f-893e-07d90b30709b
📒 Files selected for processing (16)
backend/services/docker-compose.ymlbackend/services/greenfield-processor/package.jsonbackend/services/greenfield-processor/src/acquire-item-lock.tsbackend/services/greenfield-processor/src/app-error.tsbackend/services/greenfield-processor/src/batch-assembler.tsbackend/services/greenfield-processor/src/create-batch.tsbackend/services/greenfield-processor/src/create-pinned-dispatcher.test.tsbackend/services/greenfield-processor/src/download-from-greenfield.tsbackend/services/greenfield-processor/src/resolve-content-type.tsbackend/services/greenfield-processor/src/router-worker.tsbackend/services/greenfield-processor/src/store-upload-metadata.test.tsbackend/services/greenfield-processor/src/store-upload-metadata.tsbackend/services/greenfield-processor/src/upload-to-bunny-http.tsbackend/services/greenfield-processor/src/upload-worker.test.tsbackend/services/greenfield-processor/src/upload-worker.tsbackend/services/greenfield-processor/src/validate.ts
✅ Files skipped from review due to trivial changes (2)
- backend/services/greenfield-processor/src/create-pinned-dispatcher.test.ts
- backend/services/greenfield-processor/package.json
🚧 Files skipped from review as they are similar to previous changes (5)
- backend/services/greenfield-processor/src/acquire-item-lock.ts
- backend/services/greenfield-processor/src/store-upload-metadata.test.ts
- backend/services/greenfield-processor/src/upload-to-bunny-http.ts
- backend/services/greenfield-processor/src/store-upload-metadata.ts
- backend/services/greenfield-processor/src/resolve-content-type.ts
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.
Summary by CodeRabbit
New Features
Infrastructure
Documentation
Tests