Feature/use native sqlite#1423
Conversation
|
/run-security-scan |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis change introduces Docker Service-on-Demand APIs and lifecycle management, shared compute-resource pools, delegated JWT validation, native ChangesService-on-Demand and compute resources
Native SQLite and authentication
Sequence Diagram(s)sequenceDiagram
participant Client
participant ServiceStartHandler
participant C2DEngineDocker
participant Escrow
participant Docker
Client->>ServiceStartHandler: SERVICE_START
ServiceStartHandler->>C2DEngineDocker: createServiceJob
C2DEngineDocker-->>Client: Starting service job
C2DEngineDocker->>Escrow: create and claim lock
C2DEngineDocker->>Docker: pull/build image and start container
Docker-->>C2DEngineDocker: container endpoint
C2DEngineDocker-->>Client: Running status and endpoint
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 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 |
alexcos20
left a comment
alexcos20
left a comment
There was a problem hiding this comment.
AI automated code review (Gemini 3).
Overall risk: medium
Summary:
Excellent architectural improvement migrating from the native sqlite3 dependency to Node's built-in node:sqlite. The abstraction in SqliteClient is extremely well-designed, seamlessly handling type sanitization and path initialization, and the synchronous refactoring correctly maintains backward compatibility. However, there is a critical typo in the pinned Node.js version (22.22.2) which does not exist and will break CI pipelines and local setups.
Comments:
• [ERROR][bug] Node.js version v22.22.2 does not exist (the v22 release line is currently at ~v22.14.x). Pinning to a nonexistent version will cause actions/setup-node to fail immediately. Please update to a valid version that satisfies your >=22.13.0 requirement (e.g., 22.13.1 or 22.14.0).
- node-version: 'v22.22.2'
+ node-version: '22.13.1'(Note: Ensure you apply this fix to the matrix strategy on line 45 as well)
• [ERROR][bug] Matches the nonexistent version from CI. Please update this to a valid Node.js version so that nvm use does not fail for developers.
-22.22.2
+22.13.1• [INFO][style] Excellent design choice here. Eagerly ensuring the directory exists (fs.mkdirSync) and configuring PRAGMA busy_timeout in the central client wrapper removes repetitive boilerplate from the individual DB classes and guarantees consistency.
• [INFO][style] Centralizing the sanitize logic for node:sqlite's stricter bindings (gracefully mapping undefined and boolean) is a fantastic way to ensure a smooth migration without scattering type coercions throughout the consuming code.
• [INFO][other] Good catch handling the Uint8Array to string conversion here via Buffer.from(blob). This correctly accounts for the change in how node:sqlite returns BLOBs compared to the old sqlite3 addon.
|
gemini is wrong: https://nodejs.org/en/download/archive/v22.22.2 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/ci.yml (1)
116-121: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDisable credential persistence for the external Barge checkout.
The checked-out repository's scripts run immediately afterward and can access the persisted GitHub token. Add
persist-credentials: falseto limit supply-chain exposure.Proposed fix
with: repository: 'oceanprotocol/barge' path: 'barge' ref: '43cfdfd21154a2bae00770b779e7c39390ff5043' + persist-credentials: false🤖 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 @.github/workflows/ci.yml around lines 116 - 121, Update the “Checkout Barge” actions/checkout step to set persist-credentials to false in its with configuration, while preserving the existing repository, path, and ref values.Source: Linters/SAST tools
🟠 Major comments (25)
docs/env.md-230-234 (1)
230-234: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve environment-specific constraints during migration.
Line 261 says to move all
constraintsto connection-level resources, but Line 234 documents them as valid environment overrides. Moving them unconditionally makes environment-specific requirements pool-wide and changes scheduling behavior. Only move constraints intended for every environment.Also applies to: 243-261
🤖 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 `@docs/env.md` around lines 230 - 234, Update the migration guidance around the environment-level resources and constraints so environment-specific constraints remain attached to their environment. Move constraints to connection-level resources only when they are intended to apply to every environment; preserve per-environment overrides, including empty constraints used to remove inherited restrictions.src/utils/config/schemas.ts-193-201 (1)
193-201: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReject every legacy hardware field, not only two of them.
.passthrough()acceptsplatform,memoryTotal,type,kind,shareable, and other unknown fields, while the migration check only detectsinitanddriverVersion. This contradicts the documented startup-error contract. Make resource references strict or explicitly reject the complete hardware-field set.Proposed strict resource-reference schema
export const EnvironmentResourceRefSchema = z .object({ id: z.string(), total: z.number().optional(), min: z.number().optional(), max: z.number().optional(), constraints: z.array(ResourceConstraintSchema).optional() }) - .passthrough() + .strict()Also applies to: 437-450
🤖 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 `@src/utils/config/schemas.ts` around lines 193 - 201, Update EnvironmentResourceRefSchema and the corresponding schema at the additional resource-reference definition to reject unknown legacy hardware fields instead of accepting them via passthrough. Replace the permissive object behavior with strict validation, or explicitly forbid the complete legacy hardware-field set including platform, memoryTotal, type, kind, shareable, init, and driverVersion, while preserving the existing valid fields.src/utils/config/schemas.ts-454-471 (1)
454-471: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winValidate free-tier resource references against the pool.
Only
env.resourcesis checked againstpoolIds;env.free.resourcescan reference an unknown resource and pass startup validation. Apply the same validation to both resource lists.🤖 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 `@src/utils/config/schemas.ts` around lines 454 - 471, Extend the resource-reference validation in the Docker configuration schema to also iterate over each environment’s free-tier resources, such as env.free.resources, using the existing poolIds set. Report unknown IDs through ctx.addIssue with the corresponding free-resource path, while preserving the current env.resources validation.src/components/c2d/compute_engine_docker.ts-620-642 (1)
620-642: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winStop every cron and delayed startup task.
stop()drains the internal loop and service starts, butstartCrons()also creates untracked startup timeouts andscanDBUpdateTimer. Those callbacks can continue Docker/database work after the engine is stopped.Track and clear all three startup timeouts, and clear
scanDBUpdateTimeralongside the image-cleanup and payment timers.🤖 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 `@src/components/c2d/compute_engine_docker.ts` around lines 620 - 642, Update startCrons() to track all three delayed startup timeout handles so stop() can clear them and prevent callbacks from running after shutdown. In stop(), clear and reset those startup handles, and also clear scanDBUpdateTimer alongside the existing image-cleanup and payment timers. Preserve the existing internal-loop and service-start draining behavior.src/components/c2d/compute_engine_docker.ts-3104-3115 (1)
3104-3115: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winFail closed when the image scan cannot complete.
checkImageVulnerability()returnsvulnerable: falsewhen scanning fails, so both new service paths accept an unscanned image even whenscanImagesis enabled. Treat a missing or invalid report as a scan failure and reject the image.Also applies to: 3164-3176
🤖 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 `@src/components/c2d/compute_engine_docker.ts` around lines 3104 - 3115, Update the image-scan handling around checkImageVulnerability in both affected service paths to fail closed when the scan result is missing, invalid, or indicates scanning could not complete, rather than relying only on vulnerable. When scanImages is enabled, reject and remove the image for any such failure, while preserving the existing vulnerability rejection behavior and error context.src/components/c2d/compute_engine_docker.ts-3475-3478 (1)
3475-3478: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRelease reservations only after confirmed Docker teardown.
cleanupServiceDocker()swallows stop/removal errors while releasing CPU cores, and its caller then releases host ports.stopService()likewise releases CPUs before cleanup. A leaked running container can therefore overlap another service on the same cores or port.Return cleanup success and release CPU/port reservations only after removal succeeds or Docker confirms the resource is already absent.
Also applies to: 3513-3525, 3586-3617
🤖 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 `@src/components/c2d/compute_engine_docker.ts` around lines 3475 - 3478, Update cleanupServiceDocker and its callers, including the stopService path and the catch block around container creation, to return and check whether Docker teardown succeeded or confirmed the container is absent. Only release CPU cores and host ports after that confirmed outcome; do not swallow teardown failures and release reservations unconditionally. Preserve the existing refund behavior while ensuring all affected paths use the cleanup result before releasing reservations.src/components/c2d/compute_engine_base.ts-494-508 (1)
494-508: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDeduplicate discrete capacity as well as discrete usage.
Each environment carries the same globally aggregated discrete
inUse, so the code takes its maximum. Summing each environment’s discretetotalstill multiplies the same physical pool and can permit over-allocation whenphysicalLimitshas no entry.Proposed fix
let globalUsed = 0 let globalTotal = 0 let discreteInUse: number | undefined + let discreteTotal: number | undefined for (const e of allEnvironments) { const res = this.getResource(e.resources, resourceId) if (res) { - globalTotal += res.total || 0 if (res.kind === 'discrete') { + discreteTotal = Math.max(discreteTotal ?? 0, res.total || 0) discreteInUse = Math.max(discreteInUse ?? 0, res.inUse || 0) } else { + globalTotal += res.total || 0 globalUsed += res.inUse || 0 } } } + if (discreteTotal !== undefined) globalTotal += discreteTotal if (discreteInUse !== undefined) globalUsed += discreteInUse🤖 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 `@src/components/c2d/compute_engine_base.ts` around lines 494 - 508, Update the resource aggregation loop in the compute-engine method containing discreteInUse so discrete total capacity is deduplicated like discrete usage: track the maximum res.total for discrete resources, exclude it from globalTotal per environment, and add the deduplicated value once after iteration. Preserve summed totals for non-discrete resources and the existing discrete inUse behavior.src/components/core/utils/escrow.ts-50-59 (1)
50-59: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReject reverted or timed-out escrow transactions before continuing.
provider.waitForTransactioncan return a receipt even when the tx reverted, and it can still returnnullon timeout. This helper should fail unless the receipt exists andstatus === 1; otherwise the dependent claim/extend flow can continue without a confirmed lock.🤖 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 `@src/components/core/utils/escrow.ts` around lines 50 - 59, The waitForTransaction helper must validate the provider result before returning. Capture the receipt from provider.waitForTransaction and throw when it is null or its status is not 1, while preserving the existing chain, confirmation, and timeout parameters.src/components/c2d/compute_engine_base.ts-87-143 (1)
87-143: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winRemove the lint suppressions on these service stubs. Return
Promise.resolve(...)from the defaults and prefix unused params with_instead of keepingasyncno-op methods here.🤖 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 `@src/components/c2d/compute_engine_base.ts` around lines 87 - 143, Remove the eslint-disable comments from createServiceJob, processServiceStart, stopService, restartService, getServiceStatus, and getServiceStreamableLogs. Rename unused parameters in these stubs with an _ prefix, remove async from the no-op methods, and return Promise.resolve with the existing default values (null, empty array, or undefined as appropriate).Source: Coding guidelines
src/test/unit/compute.test.ts-1385-1415 (1)
1385-1415: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftKeep the escrow refundable until
container.start()succeeds.processServiceStartclaims the lock before container creation/start, so a failure after that point recordsclaimTxand bypasses the refund path. The consumer is charged even though the service never reachesRunning, which conflicts with the documented claim-on-success/refund-on-failure flow.🤖 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 `@src/test/unit/compute.test.ts` around lines 1385 - 1415, Update processServiceStart so the escrow lock remains refundable until container.start() completes successfully; move claimTx creation/claimLock handling after the successful start, while preserving cleanup and Error status on failures. Update the related documentation in docs/API.md (1927-1930) if needed to match this claim-on-success/refund-on-failure behavior; the test scenarios in src/test/unit/compute.test.ts (1385-1415) require no direct change beyond validating the corrected flow.src/components/core/service/utils.ts-84-100 (1)
84-100: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winScan the full range before reporting port exhaustion.
Trying only 50 random candidates can reject service startup even when many ports remain free. Pick a random starting offset, then inspect each candidate once.
Proposed fix
const size = rangeEnd - rangeStart + 1 - for (let i = 0; i < Math.min(size, 50); i++) { - const candidate = rangeStart + Math.floor(Math.random() * size) + const offset = Math.floor(Math.random() * size) + for (let i = 0; i < size; i++) { + const candidate = rangeStart + ((offset + i) % size) if (allocatedPorts.has(candidate)) continue🤖 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 `@src/components/core/service/utils.ts` around lines 84 - 100, Update allocateHostPort to scan the entire range exactly once instead of limiting attempts to 50 random candidates. Choose a random starting offset, iterate through all range offsets with wraparound, and retain the existing allocatedPorts reservation and release behavior while checking each candidate.src/components/httpRoutes/compute.ts-480-485 (1)
480-485: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDo not log the signature and nonce.
req.queryincludes authentication material from Lines 492-493. Serializing the complete query exposes it to application logs. Log only non-sensitive fields such asserviceId,node, andsince.Proposed fix
HTTP_LOGGER.logMessage( - `ServiceGetStreamableLogsCommand request received with query: ${JSON.stringify( - req.query - )}`, + `ServiceGetStreamableLogsCommand request received: ${JSON.stringify({ + serviceId: req.query.serviceId, + node: req.query.node, + since: req.query.since + })}`, true )🤖 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 `@src/components/httpRoutes/compute.ts` around lines 480 - 485, Update the request logging around ServiceGetStreamableLogsCommand to avoid serializing the full req.query, which contains the signature and nonce. Log only the non-sensitive fields serviceId, node, and since while preserving the existing HTTP_LOGGER.logMessage call behavior.src/components/database/sqlite.ts-22-39 (1)
22-39: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winPreserve the Promise API without suppressing
require-await.The synchronous migration leaves every method
asyncwithout anawaitand adds repeated lint suppressions. Use a small helper that converts synchronous results or exceptions into resolved/rejected Promises, then removeasyncand the disables.Proposed pattern
+ private execute<T>(operation: () => T): Promise<T> { + try { + return Promise.resolve(operation()) + } catch (error) { + return Promise.reject(error) + } + } - // eslint-disable-next-line require-await - async createNonce(address: string, nonce: number) { - this.db.run(insertSQL, [address, nonce]) - return { id: address, nonce } + createNonce(address: string, nonce: number) { + return this.execute(() => { + this.db.run(insertSQL, [address, nonce]) + return { id: address, nonce } + }) }As per coding guidelines, “Treat
require-awaitviolations as errors; do not leave unnecessary async functions.”Also applies to: 42-80, 83-102
🤖 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 `@src/components/database/sqlite.ts` around lines 22 - 39, Update createTableForNonce, createTableForConfig, and the other migration methods in the same file to remove async and require-await suppressions while preserving their Promise-based API. Add and reuse a small helper that executes synchronous database operations and returns a resolved Promise with the result or a rejected Promise when an exception occurs.Source: Coding guidelines
src/components/core/service/getStreamableLogs.ts-84-87 (1)
84-87: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDo not expose raw engine errors to clients.
Docker/runtime errors can contain internal paths, container details, or command data. Keep the detailed message in server logs and return a stable generic 500 response.
CORE_LOGGER.error(message) - return { stream: null, status: { httpStatus: 500, error: message } } + return { + stream: null, + status: { httpStatus: 500, error: 'Failed to stream service logs' } + }🤖 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 `@src/components/core/service/getStreamableLogs.ts` around lines 84 - 87, Update the catch block in getStreamableLogs to keep the detailed error message in CORE_LOGGER.error, but replace the returned status error with a stable generic 500-safe message. Preserve the null stream and HTTP 500 response while ensuring raw engine details are never exposed to clients.src/components/core/service/getStreamableLogs.ts-1-14 (1)
1-14: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winUse
Readablefor the shared stream response type. The cast here is masking a broader type mismatch:P2PCommandResponse.streamand the C2D log producer signatures still useStream/NodeJS.ReadableStream. Switch the shared response type and engine methods toReadable, then return the stream directly.🤖 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 `@src/components/core/service/getStreamableLogs.ts` around lines 1 - 14, Update the shared stream typing used by getStreamableLogs: replace Stream/NodeJS.ReadableStream with Node’s Readable in P2PCommandResponse and the related C2D engine log-producer methods, then remove the cast and return the Readable stream directly from the handler. Preserve existing stream behavior and imports while aligning all affected signatures.Source: Coding guidelines
src/components/database/sqliteClient.ts-35-38 (1)
35-38: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftAvoid a 5-second synchronous lock wait.
DatabaseSyncruns on the main thread, soPRAGMA busy_timeout = 5000can block request handling for up to 5 seconds whenever SQLite is locked. Use a shorter timeout with retry/backoff, or move SQLite work off the main thread.🤖 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 `@src/components/database/sqliteClient.ts` around lines 35 - 38, Update the SQLite initialization flow around DatabaseSync and the busy_timeout pragma to avoid blocking the main thread for up to five seconds: use a substantially shorter timeout and implement bounded retry/backoff for transient lock failures, preserving normal database operations and propagating persistent errors.src/components/database/sqliteClient.ts-27-59 (1)
27-59: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftAdd
close()toSqliteClientand wire wrapper shutdown
SqliteClientowns a persistentDatabaseSynchandle, butSQLiteProvider,SQLiteAuthToken,SQLiteCompute, andPersistentStorageFactorynever release it. Expose a close method and call it from the owning teardown path so shutdown and tests don’t leak file handles or leave the DB locked.🤖 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 `@src/components/database/sqliteClient.ts` around lines 27 - 59, Add a public close() method to SqliteClient that closes its DatabaseSync handle, then update the teardown or shutdown paths of SQLiteProvider, SQLiteAuthToken, SQLiteCompute, and PersistentStorageFactory to invoke the client’s close method when they release their resources. Ensure each owner closes the database exactly once, including test cleanup paths.src/test/unit/auth/token.test.ts-90-101 (1)
90-101: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winRemove the
require-awaitsuppression.Return a resolved promise from the fake instead of declaring an unnecessary
asyncfunction.Proposed fix
- // eslint-disable-next-line require-await - sendTo: async (_peer: string, _msg: string) => { + sendTo: (_peer: string, _msg: string) => { tracker.calls++ const httpStatus = opts.httpStatus ?? 200 if (httpStatus !== 200) { - return { status: { httpStatus } } + return Promise.resolve({ status: { httpStatus } }) } - return { + return Promise.resolve({ status: { httpStatus: 200 }, stream: Readable.from(JSON.stringify(opts.verdict)) - } + }) }As per coding guidelines, “Treat
require-awaitviolations as errors; do not leave unused variables or unnecessary async functions.”🤖 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 `@src/test/unit/auth/token.test.ts` around lines 90 - 101, Update the fake sendTo implementation in the token test to remove the unnecessary async declaration and its eslint-disable suppression, while preserving the existing tracker, status, and stream response behavior by returning a resolved promise explicitly.Source: Coding guidelines
src/components/core/service/extendService.ts-20-31 (1)
20-31: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRequire a finite positive integer duration.
parseIntallows values such asNaN,Infinity, or partially numeric strings to bypass this check and later corrupt expiry, duration, or pricing calculations.Proposed fix
- if (commandValidation.valid) { - if (parseInt(String(command.additionalDuration)) <= 0) + if (commandValidation.valid) { + if ( + !Number.isSafeInteger(command.additionalDuration) || + command.additionalDuration <= 0 + ) return buildInvalidRequestMessage('Invalid additionalDuration') }🤖 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 `@src/components/core/service/extendService.ts` around lines 20 - 31, Update validate in the ServiceExtendCommand validation flow to require additionalDuration to be a finite, strictly positive integer, rejecting NaN, Infinity, fractional values, and partially numeric strings. Preserve the existing required-parameter validation and InvalidRequest response for invalid durations.src/components/core/service/extendService.ts-54-72 (1)
54-72: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winUse the authenticated address for service operations.
The auth result is ignored after validation, so a valid token can be presented for one address while lookup, ownership checks, escrow, and mutation still run against
task.consumerAddress.
src/components/core/service/extendService.ts#L54-L72: useauth.consumerAddressfor job lookup, ACL checks, escrow calls, and persistence.src/components/core/service/stopService.ts#L41-L60: useauth.consumerAddressfor lookup, ownership checks, andstopService.🤖 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 `@src/components/core/service/extendService.ts` around lines 54 - 72, Use the authenticated address from the auth result, auth.consumerAddress, throughout service operations instead of task.consumerAddress. In src/components/core/service/extendService.ts lines 54-72, update job lookup, ownership/ACL checks, escrow calls, and persistence; in src/components/core/service/stopService.ts lines 41-60, update lookup, ownership checks, and stopService calls. Preserve token validation while ensuring every operation targets the authenticated consumer.src/components/database/sqliteAuthToken.ts-37-46 (1)
37-46: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDo not suppress every schema-migration failure.
The catch also hides permission, corruption, I/O, and malformed-schema errors. Startup then succeeds with no
chainIdcolumn, and later inserts fail. Check the schema before altering it or rethrow anything other than “column already exists.”🤖 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 `@src/components/database/sqliteAuthToken.ts` around lines 37 - 46, Update the migration try/catch around the synchronous ALTER TABLE exec to suppress only the expected duplicate-column case. Check whether authTokens already contains chainId before altering it, or inspect the caught error and rethrow any failure that is not specifically “column already exists,” so permission, corruption, I/O, and malformed-schema errors propagate during startup.src/components/database/sqliteAuthToken.ts-24-25 (1)
24-25: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winRemove the
require-awaitsuppressions.These provider operations are synchronous now. Make the internal methods return
void; the public database wrapper can retain its asynchronous API.As per coding guidelines,
require-awaitviolations are errors and unnecessary async functions must not be left in place.Also applies to: 49-50, 104-105
🤖 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 `@src/components/database/sqliteAuthToken.ts` around lines 24 - 25, Remove the require-await suppression comments and async modifiers from the synchronous internal provider methods, including createTable and the methods at the other referenced locations, changing their return types to void. Keep the public database wrapper’s asynchronous API unchanged and update any internal calls as needed to work without awaiting these methods.Source: Coding guidelines
src/components/database/sqliteAuthToken.ts-95-97 (1)
95-97: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDo not write authentication tokens to logs.
The token is a bearer credential; anyone with log access could replay it before invalidation completes or retrieve it from retained logs.
Proposed fix
- DATABASE_LOGGER.info(`Auth token ${token} is invalid`) + DATABASE_LOGGER.info('Expired auth token is being invalidated')🤖 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 `@src/components/database/sqliteAuthToken.ts` around lines 95 - 97, Remove the bearer token value from the log message in the invalid-token branch of the token validation method, while preserving the invalidation call and informational logging with a non-sensitive message.Source: Linters/SAST tools
src/components/core/service/startService.ts-111-123 (1)
111-123: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReject decrypted
userDataunless it is a plain object.Valid JSON can still be
null, an array, or a primitive. Those values pass this pre-escrow check but cannot safely become the container environment map, causing a delayed background failure.Proposed fix
if (task.userData) { try { - await decryptUserData(task.userData, node.getKeyManager()) + const userData = await decryptUserData(task.userData, node.getKeyManager()) + if ( + userData === null || + typeof userData !== 'object' || + Array.isArray(userData) + ) { + throw new Error('userData must decrypt to an object') + } } catch {🤖 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 `@src/components/core/service/startService.ts` around lines 111 - 123, Update the userData validation in the startService flow around decryptUserData so it captures the decrypted value and accepts it only when it is a non-null plain object, rejecting arrays and primitive values through buildInvalidParametersResponse/buildInvalidRequestMessage. Preserve the existing ECIES decryption error response for values that cannot be decrypted.src/components/core/service/startService.ts-36-37 (1)
36-37: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRequire a finite positive integer duration.
parseInt()accepts values such as"3600junk"and letsNaNpass becauseNaN <= 0is false. Invalid durations can consequently reach pricing and job persistence.Proposed fix
- if (parseInt(String(command.duration)) <= 0) + if ( + typeof command.duration !== 'number' || + !Number.isSafeInteger(command.duration) || + command.duration <= 0 + ) return buildInvalidRequestMessage('Invalid duration')🤖 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 `@src/components/core/service/startService.ts` around lines 36 - 37, Update the duration validation in startService to require a finite positive integer: reject non-numeric values, NaN, Infinity, decimals, and strings with trailing characters such as “3600junk,” while preserving valid positive integer durations before pricing or persistence.
🟡 Minor comments (6)
docs/env.md-198-198 (1)
198-198: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse level-three headings below
## Compute.These level-four headings skip a hierarchy level and fail the reported Markdown lint rule.
Also applies to: 216-216, 243-243
🤖 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 `@docs/env.md` at line 198, Update the “Connection-level fields” heading and the other affected headings to level-three headings under “## Compute,” preserving their existing titles and content while maintaining the required Markdown heading hierarchy.Source: Linters/SAST tools
src/utils/config/schemas.ts-285-293 (1)
285-293: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winDetect split shell invocations.
Because
commandis an argument array, the normal form['sh', '-c', payload]never matches/sh\s+-c/within one argument. Check adjacent arguments as well, otherwise the intended injection warning is bypassed.🤖 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 `@src/utils/config/schemas.ts` around lines 285 - 293, Update the shell-injection detection in the tmpl.command iteration to also inspect adjacent command arguments, so split invocations such as “sh” followed by “-c” trigger the existing warning. Preserve detection of inline “sh -c” patterns and backticks, and keep the current warning context and index behavior.src/@types/OceanNode.ts-109-109 (1)
109-109: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCorrect the documented default path.
config.jsonusesdocs/serviceTemplates/, while this comment claims the default isdatabases/serviceTemplates/. Keep the configuration contract unambiguous.- serviceTemplatesPath?: string // folder of *.json service templates; defaults to 'databases/serviceTemplates/' + serviceTemplatesPath?: string // folder of *.json service templates; defaults to 'docs/serviceTemplates/'🤖 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 `@src/`@types/OceanNode.ts at line 109, Update the documentation comment for serviceTemplatesPath in OceanNode to state the configured default path docs/serviceTemplates/ instead of databases/serviceTemplates/, keeping the configuration contract unambiguous.src/test/integration/services.test.ts-223-224 (1)
223-224: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the unnecessary
asyncmodifier and lint suppression
readLogsWithTimeoutreturns aPromisedirectly, soasyncis redundant here. Dropping it also removes therequire-awaitdisable.Proposed fix
- // eslint-disable-next-line require-await - async function readLogsWithTimeout( + function readLogsWithTimeout(🤖 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 `@src/test/integration/services.test.ts` around lines 223 - 224, Update readLogsWithTimeout to remove the unnecessary async modifier and return the existing Promise directly; also delete the accompanying require-await eslint suppression.Source: Coding guidelines
docs/Ocean Node.postman_collection.json-5-20 (1)
5-20: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDocument the issuer peer ID required when creating auth tokens.
CREATE_AUTH_TOKENsignsconsumerAddress + nonce + command + issuerPeerId, but the collection describes only the first three fields and provides no peer-ID variable. Following this example therefore produces an invalid signature. Add the node peer ID and the command-specific signing instructions.Also applies to: 785-799
🤖 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 `@docs/Ocean` Node.postman_collection.json around lines 5 - 20, Update the collection description and variables around CREATE_AUTH_TOKEN to document that its signature includes issuerPeerId in addition to consumerAddress, nonce, and command. Add a dedicated issuer peer ID collection variable with an appropriate placeholder, and include command-specific signing guidance so generated signatures use the required four fields.src/test/unit/service/serviceUtils.test.ts-99-121 (1)
99-121: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRelease successful allocations when any concurrent allocation fails.
Promise.all()can reject after some ports were reserved, beforeportsis assigned or thefinallyblock is entered. Those reservations then leak into subsequent tests.Proposed fix
- const ports = await Promise.all( + const results = await Promise.allSettled( Array.from({ length: count }, () => allocateHostPort(rangeStart, rangeEnd)) ) + const ports = results.flatMap((result) => + result.status === 'fulfilled' ? [result.value] : [] + ) try { + const failure = results.find((result) => result.status === 'rejected') + if (failure?.status === 'rejected') throw failure.reason + expect(new Set(ports).size).to.equal(count) // all unique🤖 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 `@src/test/unit/service/serviceUtils.test.ts` around lines 99 - 121, Update the allocateHostPort concurrency test to track successfully resolved ports independently of the Promise.all result, and release those ports in cleanup even when another allocation rejects before ports is assigned. Preserve the uniqueness and range assertions for successful allocations while ensuring every reserved port is passed to releaseHostPort.
🧹 Nitpick comments (6)
src/components/database/sqliteCompute.ts (1)
149-167: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winIndex the service lifecycle queries.
The two-second engine loop filters
service_jobsbyclusterHash,status, andexpiresAt. Without a matching index, every pending/running/expiry check scans the growing history table.Proposed fix
CREATE TABLE IF NOT EXISTS service_jobs ( serviceId TEXT PRIMARY KEY, owner TEXT, clusterHash TEXT, status INTEGER, expiresAt INTEGER, dateCreated TEXT, body BLOB ); + + CREATE INDEX IF NOT EXISTS service_jobs_lifecycle_idx + ON service_jobs (clusterHash, status, expiresAt);🤖 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 `@src/components/database/sqliteCompute.ts` around lines 149 - 167, Update createServiceTable to add an index on service_jobs covering the lifecycle query filters clusterHash, status, and expiresAt, preferably alongside the table creation using CREATE INDEX IF NOT EXISTS. Keep the existing table schema and error handling intact.docs/compute-pricing.md (1)
44-44: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd language specifier to the fenced code block.
As highlighted by static analysis, this fenced code block lacks a language specifier. Using
textwill resolve the linting warning and improve rendering.📝 Proposed fix
-``` +```text DOCKER_COMPUTE_ENVIRONMENTS🤖 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 `@docs/compute-pricing.md` at line 44, Update the fenced code block containing DOCKER_COMPUTE_ENVIRONMENTS to specify the text language, using a ```text fence while preserving the block’s contents.Source: Linters/SAST tools
docs/serviceTemplates/vllm-dual-lite-gpu.json (1)
6-6: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPin the vLLM images used by these templates to tested tags or digests.
latestis used across all vLLM service templates here, so the container content can drift independently of the memory and CLI assumptions in these commands.🤖 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 `@docs/serviceTemplates/vllm-dual-lite-gpu.json` at line 6, Update the vLLM image tag in the service templates, including the configuration containing the "tag" field, to use a tested, immutable version tag or digest instead of "latest"; apply the same pinned image version consistently across all vLLM templates while preserving the existing memory and CLI settings.src/test/unit/service/serviceJobsDatabase.test.ts (2)
289-360: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake each resource-accounting test self-contained.
Lines 304-310 depend on the CPU job created by the preceding test, and Lines 336-360 depend on the GPU job created earlier. Running either test alone or changing execution order produces different results. Seed and clean up each test’s required jobs in lifecycle hooks or within the test itself.
🤖 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 `@src/test/unit/service/serviceJobsDatabase.test.ts` around lines 289 - 360, Make the resource-accounting tests around getUsedResources and checkIfResourcesAreAvailable self-contained by creating their required CPU or GPU service jobs within each test or a scoped setup hook. Remove reliance on jobs seeded by preceding tests, and clean up created jobs afterward so execution order and isolated runs produce the same results.
27-67: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDrop the
require-awaitsuppression hereThese overrides don’t await anything. Return
Promise.resolve(...)from each method instead of keepingasyncand disablingrequire-awaitfor the whole helper.🤖 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 `@src/test/unit/service/serviceJobsDatabase.test.ts` around lines 27 - 67, Remove the require-await suppression around SharedAccountingEngine and make each override non-async by returning Promise.resolve(...) with the existing values. Update getComputeEnvironments, checkDockerImage, startComputeJob, stopComputeJob, getComputeJobStatus, getComputeJobResult, and cleanupExpiredStorage while preserving their current return types and results.Source: Coding guidelines
src/test/integration/escrow.test.ts (1)
227-258: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the ReLock test create its own lock.
This test relies on state created by the preceding test, so running it with
--grep, retrying it, or continuing after an earlier failure cannot reliably exercise ReLock. Create the prerequisite lock in this test or a lifecycle hook.🤖 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 `@src/test/integration/escrow.test.ts` around lines 227 - 258, Update the “indexes a ReLock event” test to create its prerequisite lock before calling reLock, rather than relying on state from the preceding test. Reuse the existing lock-creation setup and ensure the locally created lock uses the same jobId, payer, token, and initial amount values needed by the assertions; keep the ReLock indexing and event validation unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b7e68224-98a7-4c94-bf93-b1734519f79b
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (83)
.dockerignore.env.example.github/workflows/ci.yml.github/workflows/docker.yml.nvmrcCLAUDE.mdDockerfileREADME.mdconfig.jsondocs/API.mddocs/GPU.mddocs/Ocean Node.postman_collection.jsondocs/compute-pricing.mddocs/env.mddocs/serviceTemplates/README.mddocs/serviceTemplates/llamacpp-phi4-cpu.jsondocs/serviceTemplates/vllm-dual-lite-cpu.jsondocs/serviceTemplates/vllm-dual-lite-gpu.jsondocs/serviceTemplates/vllm-hf-model.jsondocs/serviceTemplates/vllm-nomic-embed.jsondocs/serviceTemplates/vllm-qwen-0_5b.jsondocs/services.mdpackage.jsonscripts/ocean-node-quickstart.shsrc/@types/C2D/C2D.tssrc/@types/C2D/ServiceOnDemand.tssrc/@types/Escrow.tssrc/@types/OceanNode.tssrc/@types/commands.tssrc/components/Auth/index.tssrc/components/Indexer/processor.tssrc/components/Indexer/processors/EscrowEventProcessor.tssrc/components/c2d/compute_engine_base.tssrc/components/c2d/compute_engine_docker.tssrc/components/c2d/compute_engines.tssrc/components/c2d/serviceResourceMatching.tssrc/components/core/compute/startCompute.tssrc/components/core/handler/authHandler.tssrc/components/core/handler/coreHandlersRegistry.tssrc/components/core/handler/handler.tssrc/components/core/handler/persistentStorage.tssrc/components/core/service/extendService.tssrc/components/core/service/getStatus.tssrc/components/core/service/getStreamableLogs.tssrc/components/core/service/getTemplates.tssrc/components/core/service/index.tssrc/components/core/service/restartService.tssrc/components/core/service/startService.tssrc/components/core/service/stopService.tssrc/components/core/service/templateLoader.tssrc/components/core/service/utils.tssrc/components/core/utils/escrow.tssrc/components/core/utils/nonceHandler.tssrc/components/database/AuthTokenDatabase.tssrc/components/database/C2DDatabase.tssrc/components/database/ElasticSchemas.tssrc/components/database/SQLLiteConfigDatabase.tssrc/components/database/SQLLiteNonceDatabase.tssrc/components/database/TypesenseSchemas.tssrc/components/database/sqlite.tssrc/components/database/sqliteAuthToken.tssrc/components/database/sqliteClient.tssrc/components/database/sqliteCompute.tssrc/components/httpRoutes/compute.tssrc/components/persistentStorage/PersistentStorageFactory.tssrc/test/integration/algorithmsAccess.test.tssrc/test/integration/auth.test.tssrc/test/integration/escrow.test.tssrc/test/integration/imageCleanup.test.tssrc/test/integration/services.test.tssrc/test/unit/auth/token.test.tssrc/test/unit/c2d/serviceResourceMatching.test.tssrc/test/unit/compute.test.tssrc/test/unit/service/serviceHandlers.test.tssrc/test/unit/service/serviceJobsDatabase.test.tssrc/test/unit/service/serviceSchemas.test.tssrc/test/unit/service/serviceUtils.test.tssrc/test/unit/service/templateLoader.test.tssrc/test/unit/sqliteClient.test.tssrc/test/utils/signature.tssrc/utils/config/constants.tssrc/utils/config/schemas.tssrc/utils/constants.ts
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 4
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/ci.yml (1)
116-121: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDisable credential persistence for the external Barge checkout.
The checked-out repository's scripts run immediately afterward and can access the persisted GitHub token. Add
persist-credentials: falseto limit supply-chain exposure.Proposed fix
with: repository: 'oceanprotocol/barge' path: 'barge' ref: '43cfdfd21154a2bae00770b779e7c39390ff5043' + persist-credentials: false🤖 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 @.github/workflows/ci.yml around lines 116 - 121, Update the “Checkout Barge” actions/checkout step to set persist-credentials to false in its with configuration, while preserving the existing repository, path, and ref values.Source: Linters/SAST tools
🟠 Major comments (25)
docs/env.md-230-234 (1)
230-234: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve environment-specific constraints during migration.
Line 261 says to move all
constraintsto connection-level resources, but Line 234 documents them as valid environment overrides. Moving them unconditionally makes environment-specific requirements pool-wide and changes scheduling behavior. Only move constraints intended for every environment.Also applies to: 243-261
🤖 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 `@docs/env.md` around lines 230 - 234, Update the migration guidance around the environment-level resources and constraints so environment-specific constraints remain attached to their environment. Move constraints to connection-level resources only when they are intended to apply to every environment; preserve per-environment overrides, including empty constraints used to remove inherited restrictions.src/utils/config/schemas.ts-193-201 (1)
193-201: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReject every legacy hardware field, not only two of them.
.passthrough()acceptsplatform,memoryTotal,type,kind,shareable, and other unknown fields, while the migration check only detectsinitanddriverVersion. This contradicts the documented startup-error contract. Make resource references strict or explicitly reject the complete hardware-field set.Proposed strict resource-reference schema
export const EnvironmentResourceRefSchema = z .object({ id: z.string(), total: z.number().optional(), min: z.number().optional(), max: z.number().optional(), constraints: z.array(ResourceConstraintSchema).optional() }) - .passthrough() + .strict()Also applies to: 437-450
🤖 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 `@src/utils/config/schemas.ts` around lines 193 - 201, Update EnvironmentResourceRefSchema and the corresponding schema at the additional resource-reference definition to reject unknown legacy hardware fields instead of accepting them via passthrough. Replace the permissive object behavior with strict validation, or explicitly forbid the complete legacy hardware-field set including platform, memoryTotal, type, kind, shareable, init, and driverVersion, while preserving the existing valid fields.src/utils/config/schemas.ts-454-471 (1)
454-471: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winValidate free-tier resource references against the pool.
Only
env.resourcesis checked againstpoolIds;env.free.resourcescan reference an unknown resource and pass startup validation. Apply the same validation to both resource lists.🤖 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 `@src/utils/config/schemas.ts` around lines 454 - 471, Extend the resource-reference validation in the Docker configuration schema to also iterate over each environment’s free-tier resources, such as env.free.resources, using the existing poolIds set. Report unknown IDs through ctx.addIssue with the corresponding free-resource path, while preserving the current env.resources validation.src/components/c2d/compute_engine_docker.ts-620-642 (1)
620-642: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winStop every cron and delayed startup task.
stop()drains the internal loop and service starts, butstartCrons()also creates untracked startup timeouts andscanDBUpdateTimer. Those callbacks can continue Docker/database work after the engine is stopped.Track and clear all three startup timeouts, and clear
scanDBUpdateTimeralongside the image-cleanup and payment timers.🤖 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 `@src/components/c2d/compute_engine_docker.ts` around lines 620 - 642, Update startCrons() to track all three delayed startup timeout handles so stop() can clear them and prevent callbacks from running after shutdown. In stop(), clear and reset those startup handles, and also clear scanDBUpdateTimer alongside the existing image-cleanup and payment timers. Preserve the existing internal-loop and service-start draining behavior.src/components/c2d/compute_engine_docker.ts-3104-3115 (1)
3104-3115: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winFail closed when the image scan cannot complete.
checkImageVulnerability()returnsvulnerable: falsewhen scanning fails, so both new service paths accept an unscanned image even whenscanImagesis enabled. Treat a missing or invalid report as a scan failure and reject the image.Also applies to: 3164-3176
🤖 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 `@src/components/c2d/compute_engine_docker.ts` around lines 3104 - 3115, Update the image-scan handling around checkImageVulnerability in both affected service paths to fail closed when the scan result is missing, invalid, or indicates scanning could not complete, rather than relying only on vulnerable. When scanImages is enabled, reject and remove the image for any such failure, while preserving the existing vulnerability rejection behavior and error context.src/components/c2d/compute_engine_docker.ts-3475-3478 (1)
3475-3478: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRelease reservations only after confirmed Docker teardown.
cleanupServiceDocker()swallows stop/removal errors while releasing CPU cores, and its caller then releases host ports.stopService()likewise releases CPUs before cleanup. A leaked running container can therefore overlap another service on the same cores or port.Return cleanup success and release CPU/port reservations only after removal succeeds or Docker confirms the resource is already absent.
Also applies to: 3513-3525, 3586-3617
🤖 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 `@src/components/c2d/compute_engine_docker.ts` around lines 3475 - 3478, Update cleanupServiceDocker and its callers, including the stopService path and the catch block around container creation, to return and check whether Docker teardown succeeded or confirmed the container is absent. Only release CPU cores and host ports after that confirmed outcome; do not swallow teardown failures and release reservations unconditionally. Preserve the existing refund behavior while ensuring all affected paths use the cleanup result before releasing reservations.src/components/c2d/compute_engine_base.ts-494-508 (1)
494-508: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDeduplicate discrete capacity as well as discrete usage.
Each environment carries the same globally aggregated discrete
inUse, so the code takes its maximum. Summing each environment’s discretetotalstill multiplies the same physical pool and can permit over-allocation whenphysicalLimitshas no entry.Proposed fix
let globalUsed = 0 let globalTotal = 0 let discreteInUse: number | undefined + let discreteTotal: number | undefined for (const e of allEnvironments) { const res = this.getResource(e.resources, resourceId) if (res) { - globalTotal += res.total || 0 if (res.kind === 'discrete') { + discreteTotal = Math.max(discreteTotal ?? 0, res.total || 0) discreteInUse = Math.max(discreteInUse ?? 0, res.inUse || 0) } else { + globalTotal += res.total || 0 globalUsed += res.inUse || 0 } } } + if (discreteTotal !== undefined) globalTotal += discreteTotal if (discreteInUse !== undefined) globalUsed += discreteInUse🤖 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 `@src/components/c2d/compute_engine_base.ts` around lines 494 - 508, Update the resource aggregation loop in the compute-engine method containing discreteInUse so discrete total capacity is deduplicated like discrete usage: track the maximum res.total for discrete resources, exclude it from globalTotal per environment, and add the deduplicated value once after iteration. Preserve summed totals for non-discrete resources and the existing discrete inUse behavior.src/components/core/utils/escrow.ts-50-59 (1)
50-59: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReject reverted or timed-out escrow transactions before continuing.
provider.waitForTransactioncan return a receipt even when the tx reverted, and it can still returnnullon timeout. This helper should fail unless the receipt exists andstatus === 1; otherwise the dependent claim/extend flow can continue without a confirmed lock.🤖 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 `@src/components/core/utils/escrow.ts` around lines 50 - 59, The waitForTransaction helper must validate the provider result before returning. Capture the receipt from provider.waitForTransaction and throw when it is null or its status is not 1, while preserving the existing chain, confirmation, and timeout parameters.src/components/c2d/compute_engine_base.ts-87-143 (1)
87-143: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winRemove the lint suppressions on these service stubs. Return
Promise.resolve(...)from the defaults and prefix unused params with_instead of keepingasyncno-op methods here.🤖 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 `@src/components/c2d/compute_engine_base.ts` around lines 87 - 143, Remove the eslint-disable comments from createServiceJob, processServiceStart, stopService, restartService, getServiceStatus, and getServiceStreamableLogs. Rename unused parameters in these stubs with an _ prefix, remove async from the no-op methods, and return Promise.resolve with the existing default values (null, empty array, or undefined as appropriate).Source: Coding guidelines
src/test/unit/compute.test.ts-1385-1415 (1)
1385-1415: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftKeep the escrow refundable until
container.start()succeeds.processServiceStartclaims the lock before container creation/start, so a failure after that point recordsclaimTxand bypasses the refund path. The consumer is charged even though the service never reachesRunning, which conflicts with the documented claim-on-success/refund-on-failure flow.🤖 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 `@src/test/unit/compute.test.ts` around lines 1385 - 1415, Update processServiceStart so the escrow lock remains refundable until container.start() completes successfully; move claimTx creation/claimLock handling after the successful start, while preserving cleanup and Error status on failures. Update the related documentation in docs/API.md (1927-1930) if needed to match this claim-on-success/refund-on-failure behavior; the test scenarios in src/test/unit/compute.test.ts (1385-1415) require no direct change beyond validating the corrected flow.src/components/core/service/utils.ts-84-100 (1)
84-100: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winScan the full range before reporting port exhaustion.
Trying only 50 random candidates can reject service startup even when many ports remain free. Pick a random starting offset, then inspect each candidate once.
Proposed fix
const size = rangeEnd - rangeStart + 1 - for (let i = 0; i < Math.min(size, 50); i++) { - const candidate = rangeStart + Math.floor(Math.random() * size) + const offset = Math.floor(Math.random() * size) + for (let i = 0; i < size; i++) { + const candidate = rangeStart + ((offset + i) % size) if (allocatedPorts.has(candidate)) continue🤖 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 `@src/components/core/service/utils.ts` around lines 84 - 100, Update allocateHostPort to scan the entire range exactly once instead of limiting attempts to 50 random candidates. Choose a random starting offset, iterate through all range offsets with wraparound, and retain the existing allocatedPorts reservation and release behavior while checking each candidate.src/components/httpRoutes/compute.ts-480-485 (1)
480-485: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDo not log the signature and nonce.
req.queryincludes authentication material from Lines 492-493. Serializing the complete query exposes it to application logs. Log only non-sensitive fields such asserviceId,node, andsince.Proposed fix
HTTP_LOGGER.logMessage( - `ServiceGetStreamableLogsCommand request received with query: ${JSON.stringify( - req.query - )}`, + `ServiceGetStreamableLogsCommand request received: ${JSON.stringify({ + serviceId: req.query.serviceId, + node: req.query.node, + since: req.query.since + })}`, true )🤖 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 `@src/components/httpRoutes/compute.ts` around lines 480 - 485, Update the request logging around ServiceGetStreamableLogsCommand to avoid serializing the full req.query, which contains the signature and nonce. Log only the non-sensitive fields serviceId, node, and since while preserving the existing HTTP_LOGGER.logMessage call behavior.src/components/database/sqlite.ts-22-39 (1)
22-39: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winPreserve the Promise API without suppressing
require-await.The synchronous migration leaves every method
asyncwithout anawaitand adds repeated lint suppressions. Use a small helper that converts synchronous results or exceptions into resolved/rejected Promises, then removeasyncand the disables.Proposed pattern
+ private execute<T>(operation: () => T): Promise<T> { + try { + return Promise.resolve(operation()) + } catch (error) { + return Promise.reject(error) + } + } - // eslint-disable-next-line require-await - async createNonce(address: string, nonce: number) { - this.db.run(insertSQL, [address, nonce]) - return { id: address, nonce } + createNonce(address: string, nonce: number) { + return this.execute(() => { + this.db.run(insertSQL, [address, nonce]) + return { id: address, nonce } + }) }As per coding guidelines, “Treat
require-awaitviolations as errors; do not leave unnecessary async functions.”Also applies to: 42-80, 83-102
🤖 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 `@src/components/database/sqlite.ts` around lines 22 - 39, Update createTableForNonce, createTableForConfig, and the other migration methods in the same file to remove async and require-await suppressions while preserving their Promise-based API. Add and reuse a small helper that executes synchronous database operations and returns a resolved Promise with the result or a rejected Promise when an exception occurs.Source: Coding guidelines
src/components/core/service/getStreamableLogs.ts-84-87 (1)
84-87: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDo not expose raw engine errors to clients.
Docker/runtime errors can contain internal paths, container details, or command data. Keep the detailed message in server logs and return a stable generic 500 response.
CORE_LOGGER.error(message) - return { stream: null, status: { httpStatus: 500, error: message } } + return { + stream: null, + status: { httpStatus: 500, error: 'Failed to stream service logs' } + }🤖 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 `@src/components/core/service/getStreamableLogs.ts` around lines 84 - 87, Update the catch block in getStreamableLogs to keep the detailed error message in CORE_LOGGER.error, but replace the returned status error with a stable generic 500-safe message. Preserve the null stream and HTTP 500 response while ensuring raw engine details are never exposed to clients.src/components/core/service/getStreamableLogs.ts-1-14 (1)
1-14: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winUse
Readablefor the shared stream response type. The cast here is masking a broader type mismatch:P2PCommandResponse.streamand the C2D log producer signatures still useStream/NodeJS.ReadableStream. Switch the shared response type and engine methods toReadable, then return the stream directly.🤖 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 `@src/components/core/service/getStreamableLogs.ts` around lines 1 - 14, Update the shared stream typing used by getStreamableLogs: replace Stream/NodeJS.ReadableStream with Node’s Readable in P2PCommandResponse and the related C2D engine log-producer methods, then remove the cast and return the Readable stream directly from the handler. Preserve existing stream behavior and imports while aligning all affected signatures.Source: Coding guidelines
src/components/database/sqliteClient.ts-35-38 (1)
35-38: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftAvoid a 5-second synchronous lock wait.
DatabaseSyncruns on the main thread, soPRAGMA busy_timeout = 5000can block request handling for up to 5 seconds whenever SQLite is locked. Use a shorter timeout with retry/backoff, or move SQLite work off the main thread.🤖 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 `@src/components/database/sqliteClient.ts` around lines 35 - 38, Update the SQLite initialization flow around DatabaseSync and the busy_timeout pragma to avoid blocking the main thread for up to five seconds: use a substantially shorter timeout and implement bounded retry/backoff for transient lock failures, preserving normal database operations and propagating persistent errors.src/components/database/sqliteClient.ts-27-59 (1)
27-59: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftAdd
close()toSqliteClientand wire wrapper shutdown
SqliteClientowns a persistentDatabaseSynchandle, butSQLiteProvider,SQLiteAuthToken,SQLiteCompute, andPersistentStorageFactorynever release it. Expose a close method and call it from the owning teardown path so shutdown and tests don’t leak file handles or leave the DB locked.🤖 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 `@src/components/database/sqliteClient.ts` around lines 27 - 59, Add a public close() method to SqliteClient that closes its DatabaseSync handle, then update the teardown or shutdown paths of SQLiteProvider, SQLiteAuthToken, SQLiteCompute, and PersistentStorageFactory to invoke the client’s close method when they release their resources. Ensure each owner closes the database exactly once, including test cleanup paths.src/test/unit/auth/token.test.ts-90-101 (1)
90-101: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winRemove the
require-awaitsuppression.Return a resolved promise from the fake instead of declaring an unnecessary
asyncfunction.Proposed fix
- // eslint-disable-next-line require-await - sendTo: async (_peer: string, _msg: string) => { + sendTo: (_peer: string, _msg: string) => { tracker.calls++ const httpStatus = opts.httpStatus ?? 200 if (httpStatus !== 200) { - return { status: { httpStatus } } + return Promise.resolve({ status: { httpStatus } }) } - return { + return Promise.resolve({ status: { httpStatus: 200 }, stream: Readable.from(JSON.stringify(opts.verdict)) - } + }) }As per coding guidelines, “Treat
require-awaitviolations as errors; do not leave unused variables or unnecessary async functions.”🤖 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 `@src/test/unit/auth/token.test.ts` around lines 90 - 101, Update the fake sendTo implementation in the token test to remove the unnecessary async declaration and its eslint-disable suppression, while preserving the existing tracker, status, and stream response behavior by returning a resolved promise explicitly.Source: Coding guidelines
src/components/core/service/extendService.ts-20-31 (1)
20-31: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRequire a finite positive integer duration.
parseIntallows values such asNaN,Infinity, or partially numeric strings to bypass this check and later corrupt expiry, duration, or pricing calculations.Proposed fix
- if (commandValidation.valid) { - if (parseInt(String(command.additionalDuration)) <= 0) + if (commandValidation.valid) { + if ( + !Number.isSafeInteger(command.additionalDuration) || + command.additionalDuration <= 0 + ) return buildInvalidRequestMessage('Invalid additionalDuration') }🤖 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 `@src/components/core/service/extendService.ts` around lines 20 - 31, Update validate in the ServiceExtendCommand validation flow to require additionalDuration to be a finite, strictly positive integer, rejecting NaN, Infinity, fractional values, and partially numeric strings. Preserve the existing required-parameter validation and InvalidRequest response for invalid durations.src/components/core/service/extendService.ts-54-72 (1)
54-72: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winUse the authenticated address for service operations.
The auth result is ignored after validation, so a valid token can be presented for one address while lookup, ownership checks, escrow, and mutation still run against
task.consumerAddress.
src/components/core/service/extendService.ts#L54-L72: useauth.consumerAddressfor job lookup, ACL checks, escrow calls, and persistence.src/components/core/service/stopService.ts#L41-L60: useauth.consumerAddressfor lookup, ownership checks, andstopService.🤖 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 `@src/components/core/service/extendService.ts` around lines 54 - 72, Use the authenticated address from the auth result, auth.consumerAddress, throughout service operations instead of task.consumerAddress. In src/components/core/service/extendService.ts lines 54-72, update job lookup, ownership/ACL checks, escrow calls, and persistence; in src/components/core/service/stopService.ts lines 41-60, update lookup, ownership checks, and stopService calls. Preserve token validation while ensuring every operation targets the authenticated consumer.src/components/database/sqliteAuthToken.ts-37-46 (1)
37-46: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDo not suppress every schema-migration failure.
The catch also hides permission, corruption, I/O, and malformed-schema errors. Startup then succeeds with no
chainIdcolumn, and later inserts fail. Check the schema before altering it or rethrow anything other than “column already exists.”🤖 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 `@src/components/database/sqliteAuthToken.ts` around lines 37 - 46, Update the migration try/catch around the synchronous ALTER TABLE exec to suppress only the expected duplicate-column case. Check whether authTokens already contains chainId before altering it, or inspect the caught error and rethrow any failure that is not specifically “column already exists,” so permission, corruption, I/O, and malformed-schema errors propagate during startup.src/components/database/sqliteAuthToken.ts-24-25 (1)
24-25: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winRemove the
require-awaitsuppressions.These provider operations are synchronous now. Make the internal methods return
void; the public database wrapper can retain its asynchronous API.As per coding guidelines,
require-awaitviolations are errors and unnecessary async functions must not be left in place.Also applies to: 49-50, 104-105
🤖 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 `@src/components/database/sqliteAuthToken.ts` around lines 24 - 25, Remove the require-await suppression comments and async modifiers from the synchronous internal provider methods, including createTable and the methods at the other referenced locations, changing their return types to void. Keep the public database wrapper’s asynchronous API unchanged and update any internal calls as needed to work without awaiting these methods.Source: Coding guidelines
src/components/database/sqliteAuthToken.ts-95-97 (1)
95-97: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDo not write authentication tokens to logs.
The token is a bearer credential; anyone with log access could replay it before invalidation completes or retrieve it from retained logs.
Proposed fix
- DATABASE_LOGGER.info(`Auth token ${token} is invalid`) + DATABASE_LOGGER.info('Expired auth token is being invalidated')🤖 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 `@src/components/database/sqliteAuthToken.ts` around lines 95 - 97, Remove the bearer token value from the log message in the invalid-token branch of the token validation method, while preserving the invalidation call and informational logging with a non-sensitive message.Source: Linters/SAST tools
src/components/core/service/startService.ts-111-123 (1)
111-123: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReject decrypted
userDataunless it is a plain object.Valid JSON can still be
null, an array, or a primitive. Those values pass this pre-escrow check but cannot safely become the container environment map, causing a delayed background failure.Proposed fix
if (task.userData) { try { - await decryptUserData(task.userData, node.getKeyManager()) + const userData = await decryptUserData(task.userData, node.getKeyManager()) + if ( + userData === null || + typeof userData !== 'object' || + Array.isArray(userData) + ) { + throw new Error('userData must decrypt to an object') + } } catch {🤖 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 `@src/components/core/service/startService.ts` around lines 111 - 123, Update the userData validation in the startService flow around decryptUserData so it captures the decrypted value and accepts it only when it is a non-null plain object, rejecting arrays and primitive values through buildInvalidParametersResponse/buildInvalidRequestMessage. Preserve the existing ECIES decryption error response for values that cannot be decrypted.src/components/core/service/startService.ts-36-37 (1)
36-37: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRequire a finite positive integer duration.
parseInt()accepts values such as"3600junk"and letsNaNpass becauseNaN <= 0is false. Invalid durations can consequently reach pricing and job persistence.Proposed fix
- if (parseInt(String(command.duration)) <= 0) + if ( + typeof command.duration !== 'number' || + !Number.isSafeInteger(command.duration) || + command.duration <= 0 + ) return buildInvalidRequestMessage('Invalid duration')🤖 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 `@src/components/core/service/startService.ts` around lines 36 - 37, Update the duration validation in startService to require a finite positive integer: reject non-numeric values, NaN, Infinity, decimals, and strings with trailing characters such as “3600junk,” while preserving valid positive integer durations before pricing or persistence.
🟡 Minor comments (6)
docs/env.md-198-198 (1)
198-198: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse level-three headings below
## Compute.These level-four headings skip a hierarchy level and fail the reported Markdown lint rule.
Also applies to: 216-216, 243-243
🤖 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 `@docs/env.md` at line 198, Update the “Connection-level fields” heading and the other affected headings to level-three headings under “## Compute,” preserving their existing titles and content while maintaining the required Markdown heading hierarchy.Source: Linters/SAST tools
src/utils/config/schemas.ts-285-293 (1)
285-293: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winDetect split shell invocations.
Because
commandis an argument array, the normal form['sh', '-c', payload]never matches/sh\s+-c/within one argument. Check adjacent arguments as well, otherwise the intended injection warning is bypassed.🤖 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 `@src/utils/config/schemas.ts` around lines 285 - 293, Update the shell-injection detection in the tmpl.command iteration to also inspect adjacent command arguments, so split invocations such as “sh” followed by “-c” trigger the existing warning. Preserve detection of inline “sh -c” patterns and backticks, and keep the current warning context and index behavior.src/@types/OceanNode.ts-109-109 (1)
109-109: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCorrect the documented default path.
config.jsonusesdocs/serviceTemplates/, while this comment claims the default isdatabases/serviceTemplates/. Keep the configuration contract unambiguous.- serviceTemplatesPath?: string // folder of *.json service templates; defaults to 'databases/serviceTemplates/' + serviceTemplatesPath?: string // folder of *.json service templates; defaults to 'docs/serviceTemplates/'🤖 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 `@src/`@types/OceanNode.ts at line 109, Update the documentation comment for serviceTemplatesPath in OceanNode to state the configured default path docs/serviceTemplates/ instead of databases/serviceTemplates/, keeping the configuration contract unambiguous.src/test/integration/services.test.ts-223-224 (1)
223-224: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the unnecessary
asyncmodifier and lint suppression
readLogsWithTimeoutreturns aPromisedirectly, soasyncis redundant here. Dropping it also removes therequire-awaitdisable.Proposed fix
- // eslint-disable-next-line require-await - async function readLogsWithTimeout( + function readLogsWithTimeout(🤖 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 `@src/test/integration/services.test.ts` around lines 223 - 224, Update readLogsWithTimeout to remove the unnecessary async modifier and return the existing Promise directly; also delete the accompanying require-await eslint suppression.Source: Coding guidelines
docs/Ocean Node.postman_collection.json-5-20 (1)
5-20: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDocument the issuer peer ID required when creating auth tokens.
CREATE_AUTH_TOKENsignsconsumerAddress + nonce + command + issuerPeerId, but the collection describes only the first three fields and provides no peer-ID variable. Following this example therefore produces an invalid signature. Add the node peer ID and the command-specific signing instructions.Also applies to: 785-799
🤖 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 `@docs/Ocean` Node.postman_collection.json around lines 5 - 20, Update the collection description and variables around CREATE_AUTH_TOKEN to document that its signature includes issuerPeerId in addition to consumerAddress, nonce, and command. Add a dedicated issuer peer ID collection variable with an appropriate placeholder, and include command-specific signing guidance so generated signatures use the required four fields.src/test/unit/service/serviceUtils.test.ts-99-121 (1)
99-121: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRelease successful allocations when any concurrent allocation fails.
Promise.all()can reject after some ports were reserved, beforeportsis assigned or thefinallyblock is entered. Those reservations then leak into subsequent tests.Proposed fix
- const ports = await Promise.all( + const results = await Promise.allSettled( Array.from({ length: count }, () => allocateHostPort(rangeStart, rangeEnd)) ) + const ports = results.flatMap((result) => + result.status === 'fulfilled' ? [result.value] : [] + ) try { + const failure = results.find((result) => result.status === 'rejected') + if (failure?.status === 'rejected') throw failure.reason + expect(new Set(ports).size).to.equal(count) // all unique🤖 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 `@src/test/unit/service/serviceUtils.test.ts` around lines 99 - 121, Update the allocateHostPort concurrency test to track successfully resolved ports independently of the Promise.all result, and release those ports in cleanup even when another allocation rejects before ports is assigned. Preserve the uniqueness and range assertions for successful allocations while ensuring every reserved port is passed to releaseHostPort.
🧹 Nitpick comments (6)
src/components/database/sqliteCompute.ts (1)
149-167: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winIndex the service lifecycle queries.
The two-second engine loop filters
service_jobsbyclusterHash,status, andexpiresAt. Without a matching index, every pending/running/expiry check scans the growing history table.Proposed fix
CREATE TABLE IF NOT EXISTS service_jobs ( serviceId TEXT PRIMARY KEY, owner TEXT, clusterHash TEXT, status INTEGER, expiresAt INTEGER, dateCreated TEXT, body BLOB ); + + CREATE INDEX IF NOT EXISTS service_jobs_lifecycle_idx + ON service_jobs (clusterHash, status, expiresAt);🤖 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 `@src/components/database/sqliteCompute.ts` around lines 149 - 167, Update createServiceTable to add an index on service_jobs covering the lifecycle query filters clusterHash, status, and expiresAt, preferably alongside the table creation using CREATE INDEX IF NOT EXISTS. Keep the existing table schema and error handling intact.docs/compute-pricing.md (1)
44-44: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd language specifier to the fenced code block.
As highlighted by static analysis, this fenced code block lacks a language specifier. Using
textwill resolve the linting warning and improve rendering.📝 Proposed fix
-``` +```text DOCKER_COMPUTE_ENVIRONMENTS🤖 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 `@docs/compute-pricing.md` at line 44, Update the fenced code block containing DOCKER_COMPUTE_ENVIRONMENTS to specify the text language, using a ```text fence while preserving the block’s contents.Source: Linters/SAST tools
docs/serviceTemplates/vllm-dual-lite-gpu.json (1)
6-6: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPin the vLLM images used by these templates to tested tags or digests.
latestis used across all vLLM service templates here, so the container content can drift independently of the memory and CLI assumptions in these commands.🤖 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 `@docs/serviceTemplates/vllm-dual-lite-gpu.json` at line 6, Update the vLLM image tag in the service templates, including the configuration containing the "tag" field, to use a tested, immutable version tag or digest instead of "latest"; apply the same pinned image version consistently across all vLLM templates while preserving the existing memory and CLI settings.src/test/unit/service/serviceJobsDatabase.test.ts (2)
289-360: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake each resource-accounting test self-contained.
Lines 304-310 depend on the CPU job created by the preceding test, and Lines 336-360 depend on the GPU job created earlier. Running either test alone or changing execution order produces different results. Seed and clean up each test’s required jobs in lifecycle hooks or within the test itself.
🤖 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 `@src/test/unit/service/serviceJobsDatabase.test.ts` around lines 289 - 360, Make the resource-accounting tests around getUsedResources and checkIfResourcesAreAvailable self-contained by creating their required CPU or GPU service jobs within each test or a scoped setup hook. Remove reliance on jobs seeded by preceding tests, and clean up created jobs afterward so execution order and isolated runs produce the same results.
27-67: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDrop the
require-awaitsuppression hereThese overrides don’t await anything. Return
Promise.resolve(...)from each method instead of keepingasyncand disablingrequire-awaitfor the whole helper.🤖 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 `@src/test/unit/service/serviceJobsDatabase.test.ts` around lines 27 - 67, Remove the require-await suppression around SharedAccountingEngine and make each override non-async by returning Promise.resolve(...) with the existing values. Update getComputeEnvironments, checkDockerImage, startComputeJob, stopComputeJob, getComputeJobStatus, getComputeJobResult, and cleanupExpiredStorage while preserving their current return types and results.Source: Coding guidelines
src/test/integration/escrow.test.ts (1)
227-258: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the ReLock test create its own lock.
This test relies on state created by the preceding test, so running it with
--grep, retrying it, or continuing after an earlier failure cannot reliably exercise ReLock. Create the prerequisite lock in this test or a lifecycle hook.🤖 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 `@src/test/integration/escrow.test.ts` around lines 227 - 258, Update the “indexes a ReLock event” test to create its prerequisite lock before calling reLock, rather than relying on state from the preceding test. Reuse the existing lock-creation setup and ensure the locally created lock uses the same jobId, payer, token, and initial amount values needed by the assertions; keep the ReLock indexing and event validation unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b7e68224-98a7-4c94-bf93-b1734519f79b
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (83)
.dockerignore.env.example.github/workflows/ci.yml.github/workflows/docker.yml.nvmrcCLAUDE.mdDockerfileREADME.mdconfig.jsondocs/API.mddocs/GPU.mddocs/Ocean Node.postman_collection.jsondocs/compute-pricing.mddocs/env.mddocs/serviceTemplates/README.mddocs/serviceTemplates/llamacpp-phi4-cpu.jsondocs/serviceTemplates/vllm-dual-lite-cpu.jsondocs/serviceTemplates/vllm-dual-lite-gpu.jsondocs/serviceTemplates/vllm-hf-model.jsondocs/serviceTemplates/vllm-nomic-embed.jsondocs/serviceTemplates/vllm-qwen-0_5b.jsondocs/services.mdpackage.jsonscripts/ocean-node-quickstart.shsrc/@types/C2D/C2D.tssrc/@types/C2D/ServiceOnDemand.tssrc/@types/Escrow.tssrc/@types/OceanNode.tssrc/@types/commands.tssrc/components/Auth/index.tssrc/components/Indexer/processor.tssrc/components/Indexer/processors/EscrowEventProcessor.tssrc/components/c2d/compute_engine_base.tssrc/components/c2d/compute_engine_docker.tssrc/components/c2d/compute_engines.tssrc/components/c2d/serviceResourceMatching.tssrc/components/core/compute/startCompute.tssrc/components/core/handler/authHandler.tssrc/components/core/handler/coreHandlersRegistry.tssrc/components/core/handler/handler.tssrc/components/core/handler/persistentStorage.tssrc/components/core/service/extendService.tssrc/components/core/service/getStatus.tssrc/components/core/service/getStreamableLogs.tssrc/components/core/service/getTemplates.tssrc/components/core/service/index.tssrc/components/core/service/restartService.tssrc/components/core/service/startService.tssrc/components/core/service/stopService.tssrc/components/core/service/templateLoader.tssrc/components/core/service/utils.tssrc/components/core/utils/escrow.tssrc/components/core/utils/nonceHandler.tssrc/components/database/AuthTokenDatabase.tssrc/components/database/C2DDatabase.tssrc/components/database/ElasticSchemas.tssrc/components/database/SQLLiteConfigDatabase.tssrc/components/database/SQLLiteNonceDatabase.tssrc/components/database/TypesenseSchemas.tssrc/components/database/sqlite.tssrc/components/database/sqliteAuthToken.tssrc/components/database/sqliteClient.tssrc/components/database/sqliteCompute.tssrc/components/httpRoutes/compute.tssrc/components/persistentStorage/PersistentStorageFactory.tssrc/test/integration/algorithmsAccess.test.tssrc/test/integration/auth.test.tssrc/test/integration/escrow.test.tssrc/test/integration/imageCleanup.test.tssrc/test/integration/services.test.tssrc/test/unit/auth/token.test.tssrc/test/unit/c2d/serviceResourceMatching.test.tssrc/test/unit/compute.test.tssrc/test/unit/service/serviceHandlers.test.tssrc/test/unit/service/serviceJobsDatabase.test.tssrc/test/unit/service/serviceSchemas.test.tssrc/test/unit/service/serviceUtils.test.tssrc/test/unit/service/templateLoader.test.tssrc/test/unit/sqliteClient.test.tssrc/test/utils/signature.tssrc/utils/config/constants.tssrc/utils/config/schemas.tssrc/utils/constants.ts
🛑 Comments failed to post (4)
src/components/c2d/compute_engine_docker.ts (2)
3272-3279: 🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy lift
Serialize start, stop, and restart per service.
processServiceStart()can continue whilestopService()marks the same record stopped, then claim payment, create a container, and overwrite it back toRunning.restartService()has the same overlap risk.Use a per-service mutex or cancellation controller, and re-check persisted status before claiming or creating the container.
Also applies to: 3574-3585, 3633-3646
🤖 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 `@src/components/c2d/compute_engine_docker.ts` around lines 3272 - 3279, Serialize processServiceStart, stopService, and restartService per service using a shared per-service mutex or cancellation controller. In processServiceStart, re-read and validate the persisted service status after acquiring serialization and immediately before claiming payment or creating the container, aborting if it is no longer Starting; apply equivalent coordination to restartService so overlapping lifecycle operations cannot restore a stopped service to Running.
3314-3329: 🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy lift
Make Escrow transitions durable and claim only after startup succeeds.
A crash after
createLock()orclaimLock()but before the following database update loses the on-chain outcome. Additionally, the lock is claimed before port allocation and container startup, so a later Docker failure leaves the consumer charged for a service that never ran.Persist/reconcile each transaction state idempotently and move final claiming after successful container startup, with compensating teardown if claiming fails.
Also applies to: 3362-3404
🤖 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 `@src/components/c2d/compute_engine_docker.ts` around lines 3314 - 3329, Update the service-job escrow flow around createLock, claimLock, and the startup sequence to persist each transaction state before and after on-chain operations, and reconcile existing transaction IDs idempotently after crashes. Move claimLock until port allocation and container startup have completed successfully; if claiming fails, perform compensating container teardown and preserve the recoverable job state.src/components/c2d/serviceResourceMatching.ts (1)
3-13: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Fix potential undefined dereference when resolving dockerfile.
When
dockerfileis provided butserviceIdis not,serviceId!.toLowerCase()will throw a runtime error. EnsureserviceIdis validated or provide a safe fallback before calling.toLowerCase().🐛 Proposed fix to guard against undefined `serviceId`
export function resolveServiceImage( image: string, tag?: string, checksum?: string, dockerfile?: string, serviceId?: string ): string { - if (dockerfile) return `${serviceId!.toLowerCase()}-svc-image:latest` + if (dockerfile && serviceId) return `${serviceId.toLowerCase()}-svc-image:latest` if (checksum) return `${image}@${checksum}` return `${image}:${tag ?? 'latest'}` }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.export function resolveServiceImage( image: string, tag?: string, checksum?: string, dockerfile?: string, serviceId?: string ): string { if (dockerfile && serviceId) return `${serviceId.toLowerCase()}-svc-image:latest` if (checksum) return `${image}@${checksum}` return `${image}:${tag ?? 'latest'}` }🤖 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 `@src/components/c2d/serviceResourceMatching.ts` around lines 3 - 13, Update resolveServiceImage so the dockerfile branch does not dereference an absent serviceId: validate serviceId before calling toLowerCase and provide the established safe fallback or handling for missing values. Preserve the checksum and tag-based resolution branches unchanged.src/components/core/service/extendService.ts (1)
175-218: 🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy lift
Persist a recoverable extension state before irreversibly claiming payment.
If
updateServiceJobfails afterclaimLocksucceeds, the consumer is charged but the service expiry remains unchanged. Persist a pending extension before claiming, then finalize it afterward with idempotent recovery/reconciliation.🤖 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 `@src/components/core/service/extendService.ts` around lines 175 - 218, Update the extension flow around claimLock and updateServiceJob to persist a recoverable pending-extension state before claiming payment, including the payment and expiry details needed for reconciliation. After a successful claim, finalize that pending state and record the extension; ensure failures after claimLock remain recoverable through idempotent recovery/reconciliation rather than leaving payment claimed with unchanged expiry.
3 participants
Closes #1407
Replace the
sqlite3native addon with Node's built-innode:sqliteDrops the external
sqlite3npm dependency and moves every embedded SQLite database ontoNode's built-in
node:sqlitemodule (DatabaseSync/StatementSync), behind one smallshared client. This is a drop-in engine swap: no on-disk format change, no schema
change, and no change to any public method signature of the database wrapper classes.
Motivation
sqlite3is a native C++ addon (hasInstallScript: true→ node-gyp / prebuilt-binarydownload at install time) and the source of recurring pain documented in
CLAUDE.md:broken bindings after Node version switches (
npm_config_build_from_source=true npm rebuild sqlite3), GLIBC mismatches, and a native compile step in the Docker builder.node:sqliteships inside the Node binary: zero install scripts, zero node-gyp, noprebuilt-binary supply chain — identical behaviour in the slim Docker runner and on dev
machines. The synchronous API also removes ~50% of the code (callback→Promise plumbing) and
fixes one latent bug (see below). SQLite usage here is a perfect fit: small, local,
single-process key-value / job-tracking tables (nonce, config, C2D jobs, auth tokens,
persistent-storage registry) — no heavy queries, no multi-process contention.
Node version requirement (breaking for very old 22.x)
node:sqliteis available unflagged since Node 22.13.0. To fail fast instead ofcrashing at runtime with
ERR_UNKNOWN_BUILTIN_MODULE: node:sqlite:.nvmrc:22→22.22.2(matches the Dockerfile and CI exactly; any>=22.13works)..github/workflows/ci.yml:v22.15.0→v22.22.2(all jobs).package.json: added"engines": { "node": ">=22.13.0" }.Dev (
.nvmrc), CI, and the Dockerfile (node:22.22.2) now all run the same Node. No newruntime flag is needed — the existing
--experimental-specifier-resolution=nodeonstartisunchanged. A harmless
ExperimentalWarning: SQLite is an experimental featureis printed onstderr at startup (the
startscript already runs with--trace-warnings).What changed
New shared client —
src/components/database/sqliteClient.ts. A thin synchronouswrapper around
DatabaseSyncthat owns one handle per file and centralizes thesqlite3-compatibility concerns:
run/get/all/execmapping the old callback API to synchronous calls.sanitize()on every binding:node:sqlitethrows onundefinedandbooleanbindings where
sqlite3silently coerced them — soundefined→NULL,true/false→
1/0, applied centrally.mkdirSync(dirname)in the constructor:DatabaseSyncopens eagerly and throws ifthe parent dir is missing (the old driver deferred opening), so directory creation moves
into the client and the previously per-caller
mkdirblocks are removed.PRAGMA busy_timeout = 5000for the long-running server process. WAL is intentionallynot enabled in this PR (zero behaviour change / no new
-wal/-shmsidecar files); itcan be added later as a follow-up knob.
Engine swap (mechanical) — same public signatures, internals become
try { …sync… } catch (e) { log; throw }:database/sqlite.ts(SQLiteProvider: nonce + config DBs).database/sqliteAuthToken.ts(SQLiteAuthToken).database/sqliteCompute.ts(SQLiteCompute: C2D jobs, docker images, service jobs) —the largest file.
persistentStorage/PersistentStorageFactory.ts(bucket registry; schema setup is nowsynchronous in the constructor, so the DB is ready before it returns).
Redundant
mkdirremoved fromSQLLiteNonceDatabase.ts,SQLLiteConfigDatabase.ts,and
AuthTokenDatabase.ts(now handled by the client).Dependency + docs:
sqlite3removed frompackage.json/package-lock.json(−~756 lockfile lines;
sqlite3+ its uniquenode-addon-api/bindings/prebuilds aregone —
node-addon-apiremains forkeccak/secp256k1).CLAUDE.mdupdated (drop therebuild sqlite3recovery note; state the ≥22.13 requirement and thenode:sqlitebacking).Behavioural fixes / gotchas handled
Uint8Array, notBuffer. Every read-side.toString()on ablob is now
Buffer.from(blob).toString()—generateJSONFromBlobandmapServiceRowsin
sqliteCompute.ts. This was the Tech Spike - Database evaluation #1 silent-breakage risk; write-side is unchanged(
Bufferis aUint8Arrayand binds as BLOB).this.changes→ returned value.deleteJob/updateJob/updateServiceJob(compute) and
dbDeleteBucket/dbUpdateBucketLabel(persistent storage) readchangesoff the object returned byrun(). Kept as a JSnumber(noreadBigInts), sothe existing
Promise<number>signatures and=== 1comparisons are unaffected.sqliteAuthToken.createTablepreviously didawait this.db.exec(…),but
sqlite3'sexectakes a callback and returns the Database object, so theawaitnever waited — the
CREATE TABLEraced thechainIdALTER TABLE(masked by sqlite3'sinternal statement queue). With synchronous
execthis is now genuinely ordered; themigration is wrapped in a
try/catchthat swallows only the expected "duplicate column".src/matched sqlite3 error codes (SQLITE_CONSTRAINTetc.), so the changederror shape (
ERR_SQLITE_ERROR) is a non-issue; errors still land inDATABASE_LOGGER.Tests
src/test/unit/sqliteClient.test.ts, no daemon): eager parent-dircreation,
runchangeson hit/miss,getundefinedon no-row, bind sanitization(
undefined→NULL, boolean→1/0), BLOB round-trip (Bufferin →Uint8Arrayout →Buffer.from(...).toString()), andINSERT … ON CONFLICTupsert.src/test/integration/imageCleanup.test.ts: its ~5 raw-handle probes(
(sqliteProvider as any).db.get/run(…, cb)) rewritten to the synchronousSqliteClientAPI.
against real
databases/*.sqlitefiles written by the oldsqlite3driver — decoded5 pre-existing C2D jobs + 141 service jobs from old blobs, read nonces/config version, and
validated a stored auth token. Confirms the BLOB-decode path and format are compatible in
both directions (rollback = revert the PR; files stay compatible).
Summary by CodeRabbit