Conversation
alexcos20
requested review from
andreip136,
bogdanfazakas,
dnsi0 and
giurgiur99
as code owners
July 7, 2026 04:10
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds shared connection-level compute resources with environment references, introduces Service-on-Demand APIs and Docker lifecycle handling, migrates SQLite and HTTP access layers, updates delegated authentication, and indexes escrow relock events. Documentation, configuration, CI, templates, persistence, routes, and tests are updated. ChangesCompute resource model
Service-On-Demand
Authentication, escrow, SQLite, and HTTP
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
docs/compute-pricing.md (1)
44-59: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFenced code block missing language identifier.
The "Configuration Layout" ascii diagram block opens with a bare
```, triggering the markdownlint MD040 rule.📝 Suggested fix
-``` +```text DOCKER_COMPUTE_ENVIRONMENTS └── [ Docker connection ] ← socketPath, resources[], 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` around lines 44 - 59, The Configuration Layout diagram in the compute pricing docs uses a fenced code block without a language tag, which trips markdownlint MD040. Update the opening fence for this ascii diagram to use a text language identifier so the block is consistently recognized; keep the rest of the diagram unchanged.Source: Linters/SAST tools
docs/env.md (1)
196-241: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHeading level skips from h2 to h4.
#### Connection-level fields,#### Environment-level fields, and#### Migration from old formatall sit directly under## Compute(h2) with no intervening h3, per the markdownlint hint.📝 Suggested fix
-#### Connection-level fields +### Connection-level fields ... -#### Environment-level fields +### Environment-level fields ... -#### Migration from old format +### Migration from old format🤖 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 196 - 241, The Compute section has heading level skips because the subsections under the top-level `## Compute` are written as `####` instead of the expected `###`. Update the Markdown headings for `Connection-level fields`, `Environment-level fields`, and `Migration from old format` to the correct level so they sit one level below `## Compute` and satisfy the markdownlint structure.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/c2d/compute_engine_docker.ts`:
- Around line 258-267: The custom resource handling in compute_engine_docker.ts
is leaving physicalLimits with an undefined total when res.total is omitted,
which makes checkGlobalResourceAvailability treat the resource as zero-capacity.
Update the custom resource branch in the resource registration logic around
ComputeResource creation so it either rejects configs without a concrete total
or assigns a valid default before calling this.physicalLimits.set for the custom
resource id.
In `@src/utils/config/schemas.ts`:
- Around line 301-357: The validation in the docker config schema only checks
`env.resources`, so `env.free.resources` can still contain legacy or invalid
resource refs. Update the `.superRefine` logic in `schemas.ts` to validate both
`resources` lists on `C2DEnvironmentConfig` and `C2DEnvironmentFreeConfig` when
checking for old-format hardware fields and when verifying pool ids against
`dockerConfig.resources`. Reuse the existing `dockerConfig.environments.forEach`
pass, but include `env.free.resources` alongside `env.resources` so both paths
are rejected consistently.
---
Nitpick comments:
In `@docs/compute-pricing.md`:
- Around line 44-59: The Configuration Layout diagram in the compute pricing
docs uses a fenced code block without a language tag, which trips markdownlint
MD040. Update the opening fence for this ascii diagram to use a text language
identifier so the block is consistently recognized; keep the rest of the diagram
unchanged.
In `@docs/env.md`:
- Around line 196-241: The Compute section has heading level skips because the
subsections under the top-level `## Compute` are written as `####` instead of
the expected `###`. Update the Markdown headings for `Connection-level fields`,
`Environment-level fields`, and `Migration from old format` to the correct level
so they sit one level below `## Compute` and satisfy the markdownlint structure.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f771fb66-2692-4d15-b908-0ec6dcd79a26
📒 Files selected for processing (13)
.env.exampleCLAUDE.mdREADME.mdconfig.jsondocs/GPU.mddocs/compute-pricing.mddocs/env.mdscripts/ocean-node-quickstart.shsrc/@types/C2D/C2D.tssrc/components/c2d/compute_engine_base.tssrc/components/c2d/compute_engine_docker.tssrc/test/unit/compute.test.tssrc/utils/config/schemas.ts
Comment on lines
+301
to
+357
| .superRefine((dockerConfig, ctx) => { | ||
| // Reject old format: env-level resources with init/driverVersion/platform indicate full ComputeResource objects | ||
| // that should have been moved to connection-level resources. | ||
| dockerConfig.environments.forEach((env, envIdx) => { | ||
| ;(env.resources || []).forEach((ref, i) => { | ||
| if ( | ||
| (ref as any).init !== undefined || | ||
| (ref as any).driverVersion !== undefined | ||
| ) { | ||
| ctx.addIssue({ | ||
| code: z.ZodIssueCode.custom, | ||
| message: `environments[${envIdx}].resources[${i}]: hardware fields (init, driverVersion, platform, etc.) must be defined at connection level in "resources", not inside an environment. See migration guide.`, | ||
| path: ['environments', envIdx, 'resources', i] | ||
| }) | ||
| } | ||
| }) | ||
| }) | ||
|
|
||
| // Validate env resource refs point to known pool ids. | ||
| // cpu, ram, disk are always valid (auto-detected from host). | ||
| const autoDetected = new Set(['cpu', 'ram', 'disk']) | ||
| const poolIds = new Set([ | ||
| ...autoDetected, | ||
| ...(dockerConfig.resources ?? []).map((r) => r.id) | ||
| ]) | ||
| dockerConfig.environments.forEach((env, envIdx) => { | ||
| ;(env.resources || []).forEach((ref, i) => { | ||
| if (!poolIds.has(ref.id)) { | ||
| ctx.addIssue({ | ||
| code: z.ZodIssueCode.custom, | ||
| message: `environments[${envIdx}].resources[${i}].id "${ref.id}" not found in connection-level resources`, | ||
| path: ['environments', envIdx, 'resources', i, 'id'] | ||
| }) | ||
| } | ||
| }) | ||
| }) | ||
|
|
||
| // Reject shareable:true on gpu/fpga type resources — these require exclusive access. | ||
| ;(dockerConfig.resources ?? []).forEach((res, i) => { | ||
| if (res.shareable === true && (res.type === 'gpu' || res.type === 'fpga')) { | ||
| ctx.addIssue({ | ||
| code: z.ZodIssueCode.custom, | ||
| message: `Resource "${res.id}": shareable:true is not allowed for type "${res.type}" — GPUs and FPGAs require exclusive access per job`, | ||
| path: ['resources', i] | ||
| }) | ||
| } | ||
| }) | ||
|
|
||
| // Warn (not error) if shareable:true on a fungible resource — it has no effect. | ||
| ;(dockerConfig.resources ?? []).forEach((res) => { | ||
| if (res.shareable === true && res.kind === 'fungible') { | ||
| CONFIG_LOGGER.warn( | ||
| `Resource "${res.id}": shareable:true has no effect on fungible resources` | ||
| ) | ||
| } | ||
| }) | ||
| }) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map relevant files and locate schemas/tests.
git ls-files 'src/utils/config/*.ts' 'test/**/*.ts' 'tests/**/*.ts' | sed -n '1,200p'
printf '\n--- outline schemas.ts ---\n'
ast-grep outline src/utils/config/schemas.ts --view expanded || true
printf '\n--- search for C2DEnvironmentFreeConfigSchema / EnvironmentResourceRefSchema / resolveEnvironmentResources ---\n'
rg -n "C2DEnvironmentFreeConfigSchema|EnvironmentResourceRefSchema|resolveEnvironmentResources|free\.resources|resources" src test tests -g '!**/dist/**' -g '!**/build/**' || trueRepository: oceanprotocol/ocean-node
Length of output: 28307
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the relevant slices of the schema file once we know line ranges.
file=src/utils/config/schemas.ts
wc -l "$file"
printf '\n--- lines 240-380 ---\n'
sed -n '240,380p' "$file" | cat -n
printf '\n--- any tests mentioning free.resources or environment resource validation ---\n'
rg -n "free\.resources|hardware fields|shareable:true|migration guide|environment resource" src test tests -g '!**/dist/**' -g '!**/build/**' || trueRepository: oceanprotocol/ocean-node
Length of output: 9551
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect related runtime validation to see whether free resources are handled elsewhere.
rg -n "resolveEnvironmentResources|env\.free|free\.resources|resources" src/utils src -g '!**/dist/**' -g '!**/build/**' | sed -n '1,240p'Repository: oceanprotocol/ocean-node
Length of output: 26578
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Focus specifically on the schema definitions around environment and free resources.
file=src/utils/config/schemas.ts
printf '\n--- top-level outlines for relevant symbols ---\n'
ast-grep outline "$file" --match 'C2DEnvironmentSchema' --view expanded || true
ast-grep outline "$file" --match 'C2DEnvironmentFreeConfigSchema' --view expanded || true
ast-grep outline "$file" --match 'EnvironmentResourceRefSchema' --view expanded || true
printf '\n--- relevant lines around environment schema definitions ---\n'
sed -n '1,260p' "$file" | cat -nRepository: oceanprotocol/ocean-node
Length of output: 9836
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if tests exist for the specific validation behavior.
rg -n "compute\.test|schemas\.test|free\.resources|resolveEnvironmentResources" test tests src -g '!**/dist/**' -g '!**/build/**' || trueRepository: oceanprotocol/ocean-node
Length of output: 3060
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Comprehensive read-only inspection for the schema and any tests.
file=src/utils/config/schemas.ts
printf 'FILE=%s\n' "$file"
wc -l "$file"
printf '\n--- relevant symbol outline ---\n'
ast-grep outline "$file" --view expanded | sed -n '1,260p' || true
printf '\n--- lines 1-420 ---\n'
sed -n '1,420p' "$file" | cat -n
printf '\n--- repository search for free resources and validation tests ---\n'
rg -n "free\.resources|C2DEnvironmentFreeConfigSchema|EnvironmentResourceRefSchema|resolveEnvironmentResources|shareable:true|migration guide" src test tests -g '!**/dist/**' -g '!**/build/**' || trueRepository: oceanprotocol/ocean-node
Length of output: 22559
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the resolver that handles free.resources at runtime.
file=src/components/c2d/compute_engine_docker.ts
printf '\n--- resolver around resolveEnvironmentResources ---\n'
sed -n '260,460p' "$file" | cat -n
printf '\n--- any direct validation or error handling for unknown ids in free.resources ---\n'
rg -n "not found|resolveEnvironmentResources|free\.resources|envDef\.free\.resources|envResources" "$file" src/test/unit/compute.test.ts src/test/integration/compute.test.ts -nRepository: oceanprotocol/ocean-node
Length of output: 13789
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect type definitions for environment configs to confirm what free.resources can contain.
file=src/@types/C2D/C2D.ts
printf '\n--- relevant C2D types ---\n'
sed -n '110,210p' "$file" | cat -nRepository: oceanprotocol/ocean-node
Length of output: 4103
Include env.free.resources in the same validation pass. C2DEnvironmentFreeConfigSchema.resources is also a list of resource refs, but the migration check and pool-id check only iterate env.resources. That lets invalid or legacy refs in free.resources pass schema validation and get skipped later at runtime.
🤖 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 301 - 357, The validation in the
docker config schema only checks `env.resources`, so `env.free.resources` can
still contain legacy or invalid resource refs. Update the `.superRefine` logic
in `schemas.ts` to validate both `resources` lists on `C2DEnvironmentConfig` and
`C2DEnvironmentFreeConfig` when checking for old-format hardware fields and when
verifying pool ids against `dockerConfig.resources`. Reuse the existing
`dockerConfig.environments.forEach` pass, but include `env.free.resources`
alongside `env.resources` so both paths are rejected consistently.
* services
* bump contract to 2.9.0
There was a problem hiding this comment.
Actionable comments posted: 8
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 winAdd
persist-credentials: falsefor the third-party Barge checkout.Static analysis flags this checkout for credential persistence.
start_ocean.shruns code from the third-partybargerepo right after; leaving the default token persisted in git config gives it access to a token it doesn't need.🔒 Proposed fix
- name: Checkout Barge uses: actions/checkout@v4 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, The third-party checkout in the workflow currently leaves Git credentials persisted, which `start_ocean.sh` can inherit when running code from the `barge` repo. Update the `Checkout Barge` step in the CI workflow to disable credential persistence by adding `persist-credentials: false` to the `actions/checkout@v4` configuration for that checkout.Source: Linters/SAST tools
🧹 Nitpick comments (7)
src/components/c2d/compute_engine_docker.ts (1)
1778-1796: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffExpired-service teardown runs sequentially inside the shared InternalLoop tick.
Each expired service is
await-ed one at a time (c.stop({t:10})allows up to 10s), andisInternalLoopRunning/setNewTimerserialize ticks — so a batch of simultaneously-expiring services can stall processing of unrelated running compute jobs and other services on the same engine for many seconds. Consider running these concurrently (e.g.Promise.all) similar to howprocessJobbatches compute jobs.🤖 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 1778 - 1796, The expired-service cleanup in compute_engine_docker’s internal loop is running one service at a time and can block the whole tick. Update the expired-services handling in the InternalLoop path to process the `expiredServices` batch concurrently instead of awaiting each `stopService` sequentially, using the existing `stopService`, `getServiceJob`, and `updateServiceJob` flow per service. Preserve the per-service error logging and status update, but structure the loop so simultaneous expirations do not stall unrelated compute jobs or services.docs/serviceTemplates/vllm-qwen-0_5b.json (1)
5-6: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPin the image tag instead of using
latest.Using
"tag": "latest"makes the template non-reproducible — a future upstreamvllm/vllm-openaiimage update could break the service or change behavior. Consider pinning to a specific version tag (e.g.,v0.8.3).🤖 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-qwen-0_5b.json` around lines 5 - 6, The service template is using a mutable container tag, so update the vllm/vllm-openai image definition to a fixed version instead of latest. Replace the tag value in the Qwen template JSON with a specific, reproducible release (for example a v0.x.y tag) so deployments of this template stay stable over time.docs/serviceTemplates/vllm-dual-lite-gpu.json (1)
5-6: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPin the image tag instead of using
latest.Same reproducibility concern as the other template —
"tag": "latest"can break the service on upstream image changes.🤖 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` around lines 5 - 6, The service template is using a floating container tag, which can make deployments non-reproducible when the upstream image changes. Update the image configuration in the vllm dual lite GPU template to use a pinned, explicit tag instead of the current latest value, and keep the change localized to the template’s image/tag fields so future runs always pull the same image version.src/test/unit/service/serviceJobsDatabase.test.ts (1)
270-362: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffShared resource accounting tests have implicit interdependencies.
The tests in this
describeblock build on each other: the first creates a service inenv-shared(cpu:3), the third creates one inenv-gpu(gpu0:1), and the fourth relies on that GPU being held globally. If an earlier test fails, later tests may fail for misleading reasons. Consider giving each test its own setup or using unique resource IDs per test to reduce coupling.🤖 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 270 - 362, The shared resource accounting tests in the describe block are coupled because later cases depend on jobs created in earlier cases. Update the tests around SharedAccountingEngine/getUsedResources/checkIfResourcesAreAvailable so each test sets up its own service jobs or uses unique resource IDs/environment names, rather than relying on the cpu/gpu state left by prior tests. This will make the assertions independent and prevent misleading failures when one case breaks..github/workflows/ci.yml (1)
121-121: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winBarge checkout is pinned in
test_integrationbut not intest_system.Line 121 pins barge to a fixed commit for stability, but the
test_systemjob's identical "Checkout Barge" step (214-218) still floats on the default branch, leaving it exposed to the same upstream-drift flakiness this pin is meant to avoid.Also applies to: 214-218
🤖 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 at line 121, `test_system` has the same unpinned Barge checkout as `test_integration`, so update the "Checkout Barge" step in that job to use the same fixed commit ref already used elsewhere in the workflow. Keep the change aligned with the existing checkout step so both jobs reference the same stable Barge revision and avoid upstream drift; use the matching checkout configuration in the workflow rather than leaving the default branch floating.src/components/database/sqliteCompute.ts (1)
244-355: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated
db.all→ Promise →mapServiceRowsboilerplate.
getServiceJob,getRunningServiceJobs,getExpiredServiceJobs, andgetPendingServiceStartsall repeat the same execute/resolve/reject/map pattern, differing only by SQL string and params.♻️ Proposed refactor
+ private runServiceQuery(selectSQL: string, params: Array<string | number>): Promise<ServiceJob[]> { + return new Promise<ServiceJob[]>((resolve, reject) => { + this.db.all(selectSQL, params, (err, rows: any[] | undefined) => { + if (err) { + DATABASE_LOGGER.error(err.message) + reject(err) + } else { + resolve(this.mapServiceRows(rows)) + } + }) + }) + } + getServiceJob(serviceId?: string, owner?: string): Promise<ServiceJob[]> { const params: any[] = [] let selectSQL = `SELECT * FROM service_jobs WHERE 1=1` if (serviceId) { selectSQL += ` AND serviceId = ?` params.push(serviceId) } if (owner) { selectSQL += ` AND owner = ?` params.push(owner) } - return new Promise<ServiceJob[]>((resolve, reject) => { - this.db.all(selectSQL, params, (err, rows: any[] | undefined) => { - if (err) { - DATABASE_LOGGER.error(err.message) - reject(err) - } else { - resolve(this.mapServiceRows(rows)) - } - }) - }) + return this.runServiceQuery(selectSQL, params) }Apply the same substitution to
getRunningServiceJobs,getExpiredServiceJobs, andgetPendingServiceStarts.🤖 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 244 - 355, The methods getServiceJob, getRunningServiceJobs, getExpiredServiceJobs, and getPendingServiceStarts all duplicate the same this.db.all Promise wrapper and mapServiceRows resolution logic. Extract that repeated database execution pattern into a single reusable helper in sqliteCompute.ts, then have each of those methods build its SQL/params and delegate to the helper so only the query-specific parts remain.src/test/integration/services.test.ts (1)
223-248: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDrop the unnecessary
async+ eslint-disable onreadLogsWithTimeout.The function body already returns
new Promise(...)directly — noawaitoccurs, so theasynckeyword (and therequire-awaitsuppression it needs) can be removed rather than worked around. As per coding guidelines,Use async only for functions that actually await something (require-await is an error).♻️ Proposed fix
- // eslint-disable-next-line require-await - async function readLogsWithTimeout( + function readLogsWithTimeout( stream: Readable, timeoutMs = 5000 ): Promise<string> {🤖 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 - 248, The `readLogsWithTimeout` helper in `services.test.ts` is marked `async` even though it only returns a `Promise` and never uses `await`, so remove the unnecessary `async` and the `eslint-disable-next-line require-await` suppression. Keep the existing promise-based implementation and its behavior intact, just simplify the function signature so it matches the `require-await` rule. Reference the `readLogsWithTimeout` function when updating it.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/c2d/compute_engine_base.ts`:
- Around line 445-470: Make running-job accounting fail consistently in
getUsedResources: the new service-job fetch already propagates DB errors, but
getRunningJobs still catches and falls back to an empty list, which can
under-count compute usage and let checkIfResourcesAreAvailable overcommit.
Update getRunningJobs and the surrounding getUsedResources flow in
compute_engine_base so both running-work queries share the same failure
behavior, and ensure any DB fetch error prevents allocation rather than
continuing with partial data.
In `@src/components/c2d/compute_engine_docker.ts`:
- Around line 3313-3329: `compute_engine_docker.ts` in the service start flow
leaves `job.payment.lockTx` unset until after
`this.escrow.waitForTransaction(chainId, lockTx)`, so a timeout/rejection
bypasses the refund path in the outer `catch`. Move the `job.payment.lockTx =
lockTx` assignment (and persist via `this.db.updateServiceJob(job)`) immediately
after `this.escrow.createLock` succeeds, before waiting for confirmation, so
`safeCancelLock` can still run if `waitForTransaction` fails. Use the `lockTx`
handling in the LOCKING section of the job setup flow as the fix point.
- Around line 3183-3208: buildServiceResourceConstraints is resolving GPU
DeviceRequests from the connection-wide resource pool instead of the selected
environment, so multi-environment services can attach the wrong devices. Update
the method to resolve the environment-specific resource list first (matching the
compute-job flow) and pass that resolved list into getDockerDeviceRequest, while
keeping allocateCpus scoped by serviceId/environment as it is now. Use the
existing buildServiceResourceConstraints and getDockerDeviceRequest symbols to
wire the environment-local resources through the device request path.
- Around line 3183-3208: The CPU cpuset path in buildServiceResourceConstraints
is using ComputeResourceRequest.amount directly, so fractional values like 0.5
or 2.5 can make allocateCpus() fall through and return all free cores. Update
buildServiceResourceConstraints and/or allocateCpus so cpuset assignment only
happens for integer CPU counts, or explicitly round/validate the cpu value
before calling allocateCpus(). Keep NanoCpus based on the original CPU amount,
but prevent non-integer requests from being converted into a cpuset string.
- Around line 1778-1796: The expired-service cleanup in compute_engine_docker’s
expired-services loop always rewrites the job to Expired, even if stopService
returned an error-state job, so first inspect the result of stopService for each
serviceId/owner instead of relying on .catch(). If stopService reports failure
(for example by returning a job with Error status or similar), preserve that
status and do not overwrite it to Expired; only mark the record Expired after a
successful teardown, using the existing db.getServiceJob and db.updateServiceJob
flow.
In `@src/components/c2d/serviceResourceMatching.ts`:
- Around line 3-13: The exported resolveServiceImage utility uses a non-null
assertion on the optional serviceId parameter, which makes the signature
misleading and unsafe. Update the function so the dockerfile branch no longer
force-unwraps serviceId; either require serviceId in the signature or
guard/fallback before calling toLowerCase. Keep the fix localized to
resolveServiceImage and ensure any callers remain compatible with the chosen
contract.
In `@src/components/core/service/extendService.ts`:
- Around line 108-116: Normalize task.additionalDuration to a number in
extendService before any arithmetic or downstream use. In the extendService
flow, coerce the raw value once near the maxDuration/remainingSeconds
calculation, then reuse that numeric local for newTotalDuration, pricing, lock
time, and the expiresAt/duration updates so string input cannot concatenate or
leak through. Keep the validation and response logic in place, but base it on
the normalized value rather than task.additionalDuration.
In `@src/test/unit/compute.test.ts`:
- Around line 1291-1292: The test file has a duplicate block-scoped declaration
for network, which will break TypeScript compilation. In the compute.test.ts
setup near the engine and network declarations, remove the extra let network
line so network is declared only once and the existing sinon.SinonStub typing is
preserved.
---
Outside diff comments:
In @.github/workflows/ci.yml:
- Around line 116-121: The third-party checkout in the workflow currently leaves
Git credentials persisted, which `start_ocean.sh` can inherit when running code
from the `barge` repo. Update the `Checkout Barge` step in the CI workflow to
disable credential persistence by adding `persist-credentials: false` to the
`actions/checkout@v4` configuration for that checkout.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Line 121: `test_system` has the same unpinned Barge checkout as
`test_integration`, so update the "Checkout Barge" step in that job to use the
same fixed commit ref already used elsewhere in the workflow. Keep the change
aligned with the existing checkout step so both jobs reference the same stable
Barge revision and avoid upstream drift; use the matching checkout configuration
in the workflow rather than leaving the default branch floating.
In `@docs/serviceTemplates/vllm-dual-lite-gpu.json`:
- Around line 5-6: The service template is using a floating container tag, which
can make deployments non-reproducible when the upstream image changes. Update
the image configuration in the vllm dual lite GPU template to use a pinned,
explicit tag instead of the current latest value, and keep the change localized
to the template’s image/tag fields so future runs always pull the same image
version.
In `@docs/serviceTemplates/vllm-qwen-0_5b.json`:
- Around line 5-6: The service template is using a mutable container tag, so
update the vllm/vllm-openai image definition to a fixed version instead of
latest. Replace the tag value in the Qwen template JSON with a specific,
reproducible release (for example a v0.x.y tag) so deployments of this template
stay stable over time.
In `@src/components/c2d/compute_engine_docker.ts`:
- Around line 1778-1796: The expired-service cleanup in compute_engine_docker’s
internal loop is running one service at a time and can block the whole tick.
Update the expired-services handling in the InternalLoop path to process the
`expiredServices` batch concurrently instead of awaiting each `stopService`
sequentially, using the existing `stopService`, `getServiceJob`, and
`updateServiceJob` flow per service. Preserve the per-service error logging and
status update, but structure the loop so simultaneous expirations do not stall
unrelated compute jobs or services.
In `@src/components/database/sqliteCompute.ts`:
- Around line 244-355: The methods getServiceJob, getRunningServiceJobs,
getExpiredServiceJobs, and getPendingServiceStarts all duplicate the same
this.db.all Promise wrapper and mapServiceRows resolution logic. Extract that
repeated database execution pattern into a single reusable helper in
sqliteCompute.ts, then have each of those methods build its SQL/params and
delegate to the helper so only the query-specific parts remain.
In `@src/test/integration/services.test.ts`:
- Around line 223-248: The `readLogsWithTimeout` helper in `services.test.ts` is
marked `async` even though it only returns a `Promise` and never uses `await`,
so remove the unnecessary `async` and the `eslint-disable-next-line
require-await` suppression. Keep the existing promise-based implementation and
its behavior intact, just simplify the function signature so it matches the
`require-await` rule. Reference the `readLogsWithTimeout` function when updating
it.
In `@src/test/unit/service/serviceJobsDatabase.test.ts`:
- Around line 270-362: The shared resource accounting tests in the describe
block are coupled because later cases depend on jobs created in earlier cases.
Update the tests around
SharedAccountingEngine/getUsedResources/checkIfResourcesAreAvailable so each
test sets up its own service jobs or uses unique resource IDs/environment names,
rather than relying on the cpu/gpu state left by prior tests. This will make the
assertions independent and prevent misleading failures when one case breaks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 821c7e66-de0c-40cb-8366-4e51e8dc5130
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (61)
.dockerignore.github/workflows/ci.yml.github/workflows/docker.ymlCLAUDE.mdDockerfileREADME.mdconfig.jsondocs/API.mddocs/Ocean Node.postman_collection.jsondocs/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.jsonsrc/@types/C2D/C2D.tssrc/@types/C2D/ServiceOnDemand.tssrc/@types/Escrow.tssrc/@types/OceanNode.tssrc/@types/commands.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/coreHandlersRegistry.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/database/C2DDatabase.tssrc/components/database/ElasticSchemas.tssrc/components/database/TypesenseSchemas.tssrc/components/database/sqliteCompute.tssrc/components/httpRoutes/compute.tssrc/test/integration/algorithmsAccess.test.tssrc/test/integration/escrow.test.tssrc/test/integration/services.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/utils/config/constants.tssrc/utils/config/schemas.tssrc/utils/constants.ts
✅ Files skipped from review due to trivial changes (4)
- README.md
- src/components/core/service/index.ts
- docs/services.md
- CLAUDE.md
🚧 Files skipped from review as they are similar to previous changes (3)
- docs/env.md
- config.json
- src/@types/C2D/C2D.ts
Comment on lines
+445
to
+470
|
|
||
| // Fold in on-demand services: they share the same physical resource pool as | ||
| // compute jobs, so a running service must occupy resources too. Services are | ||
| // always paid (no free tier) and always "running" while in the DB's running set, | ||
| // so we only tally their resources — job-slot/queue metrics stay compute-only. | ||
| // Do NOT swallow this failure: getUsedResources feeds the strict resource-availability | ||
| // gate (checkIfResourcesAreAvailable). Under-counting running services would let the | ||
| // engine overcommit shared GPU/CPU/RAM. Let it propagate so the allocation path defers | ||
| // the job (the caller already wraps getComputeEnvironments in try/catch) rather than | ||
| // proceeding with missing service data. | ||
| const serviceJobs: ServiceJob[] = await this.db.getRunningServiceJobs( | ||
| this.getC2DConfig().hash | ||
| ) | ||
| for (const svc of serviceJobs) { | ||
| const isThisEnv = svc.environment === env.id | ||
| for (const resource of svc.resources) { | ||
| const envRes = envResourceMap.get(resource.id) | ||
| if (!envRes) continue | ||
| // discrete resources (GPUs, FPGAs, NICs) tracked globally across all envs; | ||
| // fungible resources (cpu, ram, disk) are per-env exclusive. | ||
| const isGloballyTracked = envRes.kind === 'discrete' | ||
| if (!isGloballyTracked && !isThisEnv) continue | ||
| if (!(resource.id in usedResources)) usedResources[resource.id] = 0 | ||
| usedResources[resource.id] += resource.amount | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Callers of getUsedResources and whether they are guarded
rg -nP -C4 '\bgetUsedResources\s*\(' src --type=ts
# Confirm ComputeResource carries kind/shareable used at 433/465/536/538
rg -nP '(kind|shareable)\s*[?:]' src/@types --type=tsRepository: oceanprotocol/ocean-node
Length of output: 5362
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the full getUsedResources implementation around both fetches
sed -n '381,470p' src/components/c2d/compute_engine_base.ts
# Inspect the caller that uses getUsedResources / getComputeEnvironments
sed -n '1040,1105p' src/components/c2d/compute_engine_docker.ts
# Inspect any other direct callers that may need try/catch
rg -n -C3 '\bgetComputeEnvironments\s*\(|\bgetUsedResources\s*\(' src --type=tsRepository: oceanprotocol/ocean-node
Length of output: 21179
Make running-job accounting fail consistently
src/components/c2d/compute_engine_base.ts:385-470
The new service-job path correctly propagates DB failures, but getRunningJobs() still swallows its error and continues with an empty list. That leaves the same under-count/overcommit gap for compute jobs, so the availability gate can still admit work on a stale view. Either propagate both fetches or have getUsedResources() stop allocation whenever either running-work query fails.
🤖 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 445 - 470, Make
running-job accounting fail consistently in getUsedResources: the new
service-job fetch already propagates DB errors, but getRunningJobs still catches
and falls back to an empty list, which can under-count compute usage and let
checkIfResourcesAreAvailable overcommit. Update getRunningJobs and the
surrounding getUsedResources flow in compute_engine_base so both running-work
queries share the same failure behavior, and ensure any DB fetch error prevents
allocation rather than continuing with partial data.
Comment on lines
+3183
to
+3208
| // Builds Docker HostConfig resource constraints (memory, cpu, GPU device requests) | ||
| // from a service resource request, resolved against the connection-level resource pool. | ||
| private buildServiceResourceConstraints( | ||
| resources: ComputeResourceRequest[], | ||
| serviceId: string, | ||
| environment: string | ||
| ): { | ||
| Memory?: number | ||
| NanoCpus?: number | ||
| DeviceRequests?: any[] | ||
| CpusetCpus?: string | ||
| } { | ||
| const connResources: ComputeResource[] = | ||
| this.getC2DConfig().connection?.resources ?? [] | ||
| const ram = resources.find((r) => r.id === 'ram')?.amount | ||
| const cpu = resources.find((r) => r.id === 'cpu')?.amount | ||
| const deviceRequests = this.getDockerDeviceRequest(resources, connResources) ?? [] | ||
| const cpusetStr = | ||
| cpu && cpu > 0 ? this.allocateCpus(serviceId, cpu, environment) : null | ||
| return { | ||
| Memory: ram ? ram * 1024 ** 3 : undefined, | ||
| NanoCpus: cpu ? cpu * 1e9 : undefined, | ||
| DeviceRequests: deviceRequests.length ? deviceRequests : undefined, | ||
| CpusetCpus: cpusetStr ?? undefined | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Device requests resolved against the wrong resource pool for multi-environment setups.
buildServiceResourceConstraints takes an environment parameter but only uses it for allocateCpus; GPU device requests are built from this.getC2DConfig().connection?.resources (the raw, connection-wide config) instead of the specific environment's resolved resource list. For compute jobs, the equivalent code correctly scopes to the environment (const envResource = env?.resources || [] before calling getDockerDeviceRequest). With multiple environments each owning distinct GPUs (the "shared hardware resource pool" this PR targets), this can select/attach the wrong device(s) to a service container.
🐛 Proposed fix
private buildServiceResourceConstraints(
resources: ComputeResourceRequest[],
serviceId: string,
environment: string
): {
Memory?: number
NanoCpus?: number
DeviceRequests?: any[]
CpusetCpus?: string
} {
- const connResources: ComputeResource[] =
- this.getC2DConfig().connection?.resources ?? []
+ const envResources: ComputeResource[] =
+ this.envs.find((e) => e.id === environment)?.resources ?? []
const ram = resources.find((r) => r.id === 'ram')?.amount
const cpu = resources.find((r) => r.id === 'cpu')?.amount
- const deviceRequests = this.getDockerDeviceRequest(resources, connResources) ?? []
+ const deviceRequests = this.getDockerDeviceRequest(resources, envResources) ?? []📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Builds Docker HostConfig resource constraints (memory, cpu, GPU device requests) | |
| // from a service resource request, resolved against the connection-level resource pool. | |
| private buildServiceResourceConstraints( | |
| resources: ComputeResourceRequest[], | |
| serviceId: string, | |
| environment: string | |
| ): { | |
| Memory?: number | |
| NanoCpus?: number | |
| DeviceRequests?: any[] | |
| CpusetCpus?: string | |
| } { | |
| const connResources: ComputeResource[] = | |
| this.getC2DConfig().connection?.resources ?? [] | |
| const ram = resources.find((r) => r.id === 'ram')?.amount | |
| const cpu = resources.find((r) => r.id === 'cpu')?.amount | |
| const deviceRequests = this.getDockerDeviceRequest(resources, connResources) ?? [] | |
| const cpusetStr = | |
| cpu && cpu > 0 ? this.allocateCpus(serviceId, cpu, environment) : null | |
| return { | |
| Memory: ram ? ram * 1024 ** 3 : undefined, | |
| NanoCpus: cpu ? cpu * 1e9 : undefined, | |
| DeviceRequests: deviceRequests.length ? deviceRequests : undefined, | |
| CpusetCpus: cpusetStr ?? undefined | |
| } | |
| } | |
| // Builds Docker HostConfig resource constraints (memory, cpu, GPU device requests) | |
| // from a service resource request, resolved against the connection-level resource pool. | |
| private buildServiceResourceConstraints( | |
| resources: ComputeResourceRequest[], | |
| serviceId: string, | |
| environment: string | |
| ): { | |
| Memory?: number | |
| NanoCpus?: number | |
| DeviceRequests?: any[] | |
| CpusetCpus?: string | |
| } { | |
| const envResources: ComputeResource[] = | |
| this.envs.find((e) => e.id === environment)?.resources ?? [] | |
| const ram = resources.find((r) => r.id === 'ram')?.amount | |
| const cpu = resources.find((r) => r.id === 'cpu')?.amount | |
| const deviceRequests = this.getDockerDeviceRequest(resources, envResources) ?? [] | |
| const cpusetStr = | |
| cpu && cpu > 0 ? this.allocateCpus(serviceId, cpu, environment) : null | |
| return { | |
| Memory: ram ? ram * 1024 ** 3 : undefined, | |
| NanoCpus: cpu ? cpu * 1e9 : undefined, | |
| DeviceRequests: deviceRequests.length ? deviceRequests : undefined, | |
| CpusetCpus: cpusetStr ?? undefined | |
| } | |
| } |
🤖 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 3183 - 3208,
buildServiceResourceConstraints is resolving GPU DeviceRequests from the
connection-wide resource pool instead of the selected environment, so
multi-environment services can attach the wrong devices. Update the method to
resolve the environment-specific resource list first (matching the compute-job
flow) and pass that resolved list into getDockerDeviceRequest, while keeping
allocateCpus scoped by serviceId/environment as it is now. Use the existing
buildServiceResourceConstraints and getDockerDeviceRequest symbols to wire the
environment-local resources through the device request path.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm whether ComputeResourceRequest/ServiceResourceRequest cpu "amount" can be fractional
# and whether allocateCpus() is exercised with non-integer counts elsewhere.
rg -n "amount" src/@types/C2D/*.ts src/utils/config/schemas.ts
rg -n "freeCores.length === count" -A3 -B3 src/components/c2d/compute_engine_docker.tsRepository: oceanprotocol/ocean-node
Length of output: 650
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the CPU allocation logic and the resource request definitions.
sed -n '2440,2525p' src/components/c2d/compute_engine_docker.ts
printf '\n---\n'
sed -n '1,220p' src/@types/C2D/ServiceOnDemand.ts
printf '\n---\n'
sed -n '1,220p' src/@types/C2D/C2D.ts
printf '\n---\n'
sed -n '1,140p' src/utils/config/schemas.tsRepository: oceanprotocol/ocean-node
Length of output: 20465
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for any normalization or integer-only validation on CPU amounts.
rg -n "cpu.*amount|amount.*cpu|floor\\(|round\\(|parseInt\\(|integer|Number.isInteger|isInteger|NanoCpus|CpusetCpus" src/components/c2d src/utils src/@typesRepository: oceanprotocol/ocean-node
Length of output: 3941
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Probe the exact effect of the equality check for fractional counts with a tiny standalone simulation.
python3 - <<'PY'
def allocate(env_cores, count):
free = []
for core in env_cores:
free.append(core)
if len(free) == count:
break
return free
for count in [0.5, 1, 1.5, 2, 2.5]:
print(count, allocate(list(range(6)), count))
PYRepository: oceanprotocol/ocean-node
Length of output: 246
Clamp or reject fractional CPU counts before cpuset allocation. ComputeResourceRequest.amount is a plain number, and allocateCpus() stops only on freeCores.length === count. For 0.5/2.5, that never matches, so the scan returns every free core and pins the service/job to the whole environment. Skip cpuset assignment for non-integer CPU requests, or round the count before scanning.
🤖 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 3183 - 3208, The
CPU cpuset path in buildServiceResourceConstraints is using
ComputeResourceRequest.amount directly, so fractional values like 0.5 or 2.5 can
make allocateCpus() fall through and return all free cores. Update
buildServiceResourceConstraints and/or allocateCpus so cpuset assignment only
happens for integer CPU counts, or explicitly round/validate the cpu value
before calling allocateCpus(). Keep NanoCpus based on the original CPU amount,
but prevent non-integer requests from being converted into a cpuset string.
Comment on lines
+3
to
+13
| export function resolveServiceImage( | ||
| image: string, | ||
| tag?: string, | ||
| checksum?: string, | ||
| dockerfile?: string, | ||
| serviceId?: string | ||
| ): string { | ||
| if (dockerfile) return `${serviceId!.toLowerCase()}-svc-image:latest` | ||
| if (checksum) return `${image}@${checksum}` | ||
| return `${image}:${tag ?? 'latest'}` | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Non-null assertion on an optional parameter.
serviceId!.toLowerCase() will throw if dockerfile is set but serviceId is omitted — the parameter is typed optional (serviceId?: string) yet force-unwrapped. Since the only caller always supplies it, this doesn't crash today, but the signature is misleading for a small, reusable, exported utility.
🐛 Proposed fix
export function resolveServiceImage(
image: string,
tag?: string,
checksum?: string,
dockerfile?: string,
serviceId?: string
): string {
- if (dockerfile) return `${serviceId!.toLowerCase()}-svc-image:latest`
+ if (dockerfile) {
+ if (!serviceId) throw new Error('serviceId is required when dockerfile is provided')
+ 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.
Suggested change
| export function resolveServiceImage( | |
| image: string, | |
| tag?: string, | |
| checksum?: string, | |
| dockerfile?: string, | |
| serviceId?: string | |
| ): string { | |
| if (dockerfile) return `${serviceId!.toLowerCase()}-svc-image:latest` | |
| if (checksum) return `${image}@${checksum}` | |
| return `${image}:${tag ?? 'latest'}` | |
| } | |
| export function resolveServiceImage( | |
| image: string, | |
| tag?: string, | |
| checksum?: string, | |
| dockerfile?: string, | |
| serviceId?: string | |
| ): string { | |
| if (dockerfile) { | |
| if (!serviceId) throw new Error('serviceId is required when dockerfile is provided') | |
| 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, The
exported resolveServiceImage utility uses a non-null assertion on the optional
serviceId parameter, which makes the signature misleading and unsafe. Update the
function so the dockerfile branch no longer force-unwraps serviceId; either
require serviceId in the signature or guard/fallback before calling toLowerCase.
Keep the fix localized to resolveServiceImage and ensure any callers remain
compatible with the chosen contract.
Comment on lines
+108
to
+116
| const maxDuration = sod?.maxDurationSeconds ?? 86400 | ||
| const remainingSeconds = Math.max(0, Math.floor((job.expiresAt - Date.now()) / 1000)) | ||
| const newTotalDuration = remainingSeconds + task.additionalDuration | ||
| if (newTotalDuration > maxDuration) | ||
| return buildInvalidParametersResponse( | ||
| buildInvalidRequestMessage( | ||
| `Extension would result in ${newTotalDuration}s remaining, exceeding maximum ${maxDuration}s` | ||
| ) | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm whether additionalDuration is ever passed as a string at runtime and
# whether the arithmetic paths rely on numeric coercion.
rg -nP 'additionalDuration' src/components/core/service/extendService.ts src/components/httpRoutes/compute.tsRepository: oceanprotocol/ocean-node
Length of output: 1233
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant service code and the request mapping that feeds it.
sed -n '1,260p' src/components/core/service/extendService.ts
printf '\n--- compute.ts excerpt ---\n'
sed -n '400,470p' src/components/httpRoutes/compute.tsRepository: oceanprotocol/ocean-node
Length of output: 11580
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the command type and validation helper to see what runtime shapes are allowed.
sed -n '1,220p' src/@types/commands.ts
printf '\n--- validateCommands.ts excerpt ---\n'
sed -n '1,220p' src/components/httpRoutes/validateCommands.tsRepository: oceanprotocol/ocean-node
Length of output: 9660
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the ServiceExtendCommand definition and all handler call sites.
rg -n "interface ServiceExtendCommand|class ServiceExtendHandler|new ServiceExtendHandler|runServiceCommand\\(ServiceExtendHandler|SERVICE_EXTEND" src/@types src/components -g '*.ts'Repository: oceanprotocol/ocean-node
Length of output: 851
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '450,490p' src/@types/commands.tsRepository: oceanprotocol/ocean-node
Length of output: 1034
Normalize additionalDuration before using it in arithmetic.
additionalDuration is only cast at the route boundary, so a string value can still reach this handler. remainingSeconds + task.additionalDuration will concatenate for string input, and the same raw value is reused for pricing, lock time, and the expiresAt / duration updates. Coerce once to a number and reuse that local everywhere.
🤖 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 108 - 116,
Normalize task.additionalDuration to a number in extendService before any
arithmetic or downstream use. In the extendService flow, coerce the raw value
once near the maxDuration/remainingSeconds calculation, then reuse that numeric
local for newTotalDuration, pricing, lock time, and the expiresAt/duration
updates so string input cannot concatenate or leak through. Keep the validation
and response logic in place, but base it on the normalized value rather than
task.additionalDuration.
Comment on lines
+1291
to
+1292
| let engine: any | ||
| let network: { id: string; remove: sinon.SinonStub } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Duplicate let network declaration will cause a TypeScript compilation error.
The variable network is declared twice with identical content on consecutive lines. TypeScript will reject this with "Cannot redeclare block-scoped variable 'network'", preventing the entire test file from compiling.
🐛 Proposed fix: remove the duplicate line
describe('service start/restart Docker cleanup on failure', function () {
let engine: any
let network: { id: string; remove: sinon.SinonStub }
- let network: { id: string; remove: sinon.SinonStub }
function makeContainer(startRejects: boolean) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let engine: any | |
| let network: { id: string; remove: sinon.SinonStub } | |
| let engine: any | |
| let network: { id: string; remove: sinon.SinonStub } |
🤖 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 1291 - 1292, The test file has a
duplicate block-scoped declaration for network, which will break TypeScript
compilation. In the compute.test.ts setup near the engine and network
declarations, remove the extra let network line so network is declared only once
and the existing sinon.SinonStub typing is preserved.
* cross-node auth tokens * new validate auth token method * stateless * fix comment reviews * revert to ask remote node * finite number expiry + oom protection * fix test * remove meax ttl * add peerid for signature check * issuerpeerid on create auth token only --------- Co-authored-by: alexcos20 <alex.coseru@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
src/test/utils/signature.ts (1)
21-24: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winRequire callers to choose the peer binding explicitly.
The helper documents node-bound signatures, but
issuerPeerId = ''silently keeps unbound signatures valid by omission. Make the parameter required, or use a separate explicitly named helper for legacy/unbound signatures.Proposed change
- issuerPeerId: string = '' + issuerPeerId: string🤖 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/utils/signature.ts` around lines 21 - 24, The signature helper currently defaults issuerPeerId to an empty string, which allows unbound signatures to be created implicitly. Update the sign helper in signature.ts so caller intent is explicit: either make issuerPeerId a required parameter in the existing function, or split the logic into clearly named helpers for bound and legacy/unbound signatures. Ensure any call sites are updated to pass the peer binding explicitly and that the helper name and signature reflect the binding behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/Auth/index.ts`:
- Around line 160-166: `validateToken()` currently lets a rejected `p2p.sendTo`
throw, instead of treating an unreachable issuer as a failed validation. Update
the `validateToken` flow in `Auth/index.ts` around the `sendTo` call so failures
from `p2p.sendTo` are caught and converted to a `null` return, while preserving
the existing `response?.status?.httpStatus !== 200 || !response.stream` failure
path. Keep the handling localized to `validateToken` and use the existing
`issuerPeerId`, `token`, and `PROTOCOL_COMMANDS.VALIDATE_AUTH_TOKEN` logic.
- Around line 167-183: The delegated-token acceptance path in the Auth flow
currently allows `{ valid: true }` verdicts without a usable expiry and can
construct invalid dates from malformed values. Update the verdict handling in
the token verification logic so `validUntil` is parsed and validated as a finite
timestamp in the future before returning `isValid: true`; otherwise return null.
Use the `verdict` parsing block and the returned object construction in
`Auth/index.ts` to locate the fix, and ensure `validUntil` never becomes `null`
or `Invalid Date` for accepted remote verdicts.
In `@src/components/core/handler/handler.ts`:
- Around line 206-209: The success response in `handler.ts` is missing the
`error` field, making this `P2PCommandResponse` shape inconsistent with the
other status returns in the `Handler` class. Update the success object returned
from the `isAuthRequestValid.address` path to include `error: null` alongside
`stream` and `status`, matching the standard response format used elsewhere in
`Handler`.
In `@src/components/core/utils/nonceHandler.ts`:
- Around line 230-232: The auth-token signature message in
CreateAuthTokenHandler/nonceHandler currently omits chainId, so the same
nonce/signature can be reused across requested chains. Update the message
construction in both the local token creation path and the remote validation
check to include chainId ?? '' alongside consumer, nonce, command, and
issuerPeerId, and make sure the same concatenation is used wherever the
signature is computed or verified.
In `@src/test/integration/auth.test.ts`:
- Around line 198-202: The INVALIDATE_AUTH_TOKEN signature is not bound to the
issuer peer, so it can be replayed across nodes. Update createHashForSignature
usage for the invalidate path in auth.test.ts and the matching validation logic
in validateNonceAndSignature so the hash includes
oceanNode.getKeyManager().getPeerIdString() alongside address, nonce, and
PROTOCOL_COMMANDS.INVALIDATE_AUTH_TOKEN. Make sure the invalidate command’s
signature generation and verification both use the same peer-bound inputs.
---
Nitpick comments:
In `@src/test/utils/signature.ts`:
- Around line 21-24: The signature helper currently defaults issuerPeerId to an
empty string, which allows unbound signatures to be created implicitly. Update
the sign helper in signature.ts so caller intent is explicit: either make
issuerPeerId a required parameter in the existing function, or split the logic
into clearly named helpers for bound and legacy/unbound signatures. Ensure any
call sites are updated to pass the peer binding explicitly and that the helper
name and signature reflect the binding behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 20ccf51f-e7ad-47be-8998-06d06e7f329d
📒 Files selected for processing (10)
src/components/Auth/index.tssrc/components/core/handler/authHandler.tssrc/components/core/handler/coreHandlersRegistry.tssrc/components/core/handler/handler.tssrc/components/core/handler/persistentStorage.tssrc/components/core/utils/nonceHandler.tssrc/test/integration/auth.test.tssrc/test/unit/auth/token.test.tssrc/test/utils/signature.tssrc/utils/constants.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/utils/constants.ts
Comment on lines
206
to
+209
| return { | ||
| stream: null, | ||
| status: { httpStatus: 200 } | ||
| status: { httpStatus: 200 }, | ||
| consumerAddress: isAuthRequestValid.address |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Keep the success status shape consistent.
This success response drops error: null, unlike the other P2PCommandResponse statuses in this class. Add it to avoid downstream checks seeing an undefined error field.
Proposed change
return {
stream: null,
- status: { httpStatus: 200 },
+ status: { httpStatus: 200, error: null },
consumerAddress: isAuthRequestValid.address
}As per coding guidelines, “All handler responses must use P2PCommandResponse format with httpStatus, body, stream, and error properties.” <coding_guidelines>
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| return { | |
| stream: null, | |
| status: { httpStatus: 200 } | |
| status: { httpStatus: 200 }, | |
| consumerAddress: isAuthRequestValid.address | |
| return { | |
| stream: null, | |
| status: { httpStatus: 200, error: null }, | |
| consumerAddress: isAuthRequestValid.address |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/core/handler/handler.ts` around lines 206 - 209, The success
response in `handler.ts` is missing the `error` field, making this
`P2PCommandResponse` shape inconsistent with the other status returns in the
`Handler` class. Update the success object returned from the
`isAuthRequestValid.address` path to include `error: null` alongside `stream`
and `status`, matching the standard response format used elsewhere in `Handler`.
Source: Coding guidelines
Comment on lines
+230
to
+232
| const message = String( | ||
| String(consumer) + String(nonce) + String(command) + String(issuerPeerId) | ||
| ) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Bind chainId into the signed auth-token message.
CreateAuthTokenHandler now stores task.chainId in the JWT, but the signature message omits it. That lets the same unused nonce/signature mint a token for a different requested chain ID; include chainId ?? '' in both local creation and remote validation signature checks if this claim affects authorization.
Proposed fix
const message = String(
- String(consumer) + String(nonce) + String(command) + String(issuerPeerId)
+ String(consumer) +
+ String(nonce) +
+ String(command) +
+ String(issuerPeerId) +
+ String(chainId ?? '')
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const message = String( | |
| String(consumer) + String(nonce) + String(command) + String(issuerPeerId) | |
| ) | |
| const message = String( | |
| String(consumer) + | |
| String(nonce) + | |
| String(command) + | |
| String(issuerPeerId) + | |
| String(chainId ?? '') | |
| ) |
🤖 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/nonceHandler.ts` around lines 230 - 232, The
auth-token signature message in CreateAuthTokenHandler/nonceHandler currently
omits chainId, so the same nonce/signature can be reused across requested
chains. Update the message construction in both the local token creation path
and the remote validation check to include chainId ?? '' alongside consumer,
nonce, command, and issuerPeerId, and make sure the same concatenation is used
wherever the signature is computed or verified.
Comment on lines
+198
to
+202
| const invalidateHash = createHashForSignature( | ||
| consumerAddress, | ||
| newNonce, | ||
| PROTOCOL_COMMANDS.INVALIDATE_AUTH_TOKEN | ||
| ) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== auth.test.ts excerpt ==\n'
sed -n '160,240p' src/test/integration/auth.test.ts
printf '\n== search createHashForSignature ==\n'
rg -n "createHashForSignature|INVALIDATE_AUTH_TOKEN|getPeerIdString|invalidate auth token|invalidateAuthToken|invalidate.*token" src --glob '!**/dist/**' --glob '!**/build/**'
printf '\n== locate signature helper definitions ==\n'
rg -n "function createHashForSignature|const createHashForSignature|export .*createHashForSignature" src --glob '!**/dist/**' --glob '!**/build/**'Repository: oceanprotocol/ocean-node
Length of output: 19063
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== signature helper ==\n'
sed -n '1,120p' src/test/utils/signature.ts
printf '\n== auth handler ==\n'
sed -n '1,220p' src/components/core/handler/authHandler.ts
printf '\n== invalidate auth token handler and routes ==\n'
sed -n '1,220p' src/components/core/handler/invalidateAuthTokenHandler.ts 2>/dev/null || true
sed -n '180,240p' src/components/core/handler/coreHandlersRegistry.ts
printf '\n== unit auth token tests ==\n'
sed -n '1,260p' src/test/unit/auth/token.test.tsRepository: oceanprotocol/ocean-node
Length of output: 18429
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== nonce handler ==\n'
sed -n '1,320p' src/components/core/utils/nonceHandler.ts
printf '\n== auth token implementation ==\n'
sed -n '1,260p' src/components/Auth/index.ts 2>/dev/null || true
sed -n '1,260p' src/components/Auth/*.ts 2>/dev/null || true
printf '\n== auth test helper usages around create/invalidate ==\n'
sed -n '110,220p' src/test/integration/auth.test.tsRepository: oceanprotocol/ocean-node
Length of output: 26044
Bind INVALIDATE_AUTH_TOKEN to the issuer peer. checkNonce still hashes only address + nonce + command for this path, so the same invalidation signature can be replayed on other nodes. Add oceanNode.getKeyManager().getPeerIdString() to the hash here and mirror it in validateNonceAndSignature for the invalidate command.
🤖 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/auth.test.ts` around lines 198 - 202, The
INVALIDATE_AUTH_TOKEN signature is not bound to the issuer peer, so it can be
replayed across nodes. Update createHashForSignature usage for the invalidate
path in auth.test.ts and the matching validation logic in
validateNonceAndSignature so the hash includes
oceanNode.getKeyManager().getPeerIdString() alongside address, nonce, and
PROTOCOL_COMMANDS.INVALIDATE_AUTH_TOKEN. Make sure the invalidate command’s
signature generation and verification both use the same peer-bound inputs.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
* add cpuList
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
* Service lifecycle hardening
* remove axios
* use native sql
* resources constrains
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/components/c2d/compute_engine_base.ts (1)
719-730: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPreserve the shareable-device exemption in the free-tier gate.
Shareable discrete resources bypass Gate 2 here, but the free-resource check below still rejects them when
total - inUseis exhausted.Proposed fix
- if (envResource.total - envResource.inUse < request.amount) + if ( + !isShareableDiscrete && + envResource.total - (envResource.inUse ?? 0) < request.amount + ) throw new Error(`Not enough available ${request.id} for free`)🤖 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 719 - 730, Update the free-resource gate below the shown allocation checks to exempt shareable discrete resources, matching the isShareableDiscrete condition used by Gate 2. Ensure exhausted total - inUse capacity still rejects fungible and exclusive discrete resources while allowing shareable discrete resources to proceed.docs/API.md (1)
1872-1872: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd
45 Restartingto the status definition.The restart documentation uses
Restarting (45), but the canonicalServiceJob.statuslist omits it.Proposed fix
-| status | number | `10` Starting, `20` Locking, `11` PullImage, ... +| status | number | `10` Starting, `20` Locking, `45` Restarting, `11` PullImage, ...🤖 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/API.md` at line 1872, Update the canonical ServiceJob.status definition in the status table to include the missing Restarting state with numeric value 45, while preserving all existing status entries and descriptions.docs/services.md (1)
52-61: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winClarify refund behavior after claim. Escrow is claimed before container creation, so creation/start failures happen after
claimLockand are not refunded. Update the lifecycle text to say only pre-claim failures usecancelLock.🤖 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/services.md` around lines 52 - 61, Update the “Background pipeline” lifecycle text to reflect that container creation or start failures occur after claimLock and are not refunded. Limit the cancelLock/refund behavior to failures before claimLock, such as image pull, build, or scan failures, and adjust the associated failure-status wording accordingly.
🧹 Nitpick comments (4)
src/test/unit/service/serviceJobsDatabase.test.ts (1)
144-191: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy liftExercise the lock through independent database handles. The current cases reuse one
C2DDatabaseinstance, so they only cover single-connection behavior. Add a temp-db test that opens two independent processes/handles and racesacquireServiceLock, asserting exactly one succeeds.🤖 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 144 - 191, Extend the service lifecycle lock tests to use a temporary database with two independently opened C2DDatabase handles or processes, rather than the shared db instance. Race acquireServiceLock for the same serviceId using distinct holder IDs and assert exactly one acquisition succeeds, then cleanly release the winning lock and close both handles.src/components/database/sqliteCompute.ts (1)
68-70: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReturn a synchronous type from
generateJSONFromBlob(). It parses the blob immediately, and the callers already use the result withoutawait, soPromise<any>hides the real contract and weakens type checking.🤖 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 68 - 70, Update generateJSONFromBlob() to declare and return a synchronous parsed JSON value rather than Promise<any>, matching its immediate JSON.parse result and callers’ non-await usage. Remove any asynchronous return typing or wrapping while preserving the existing Buffer decoding and parsing behavior.Source: Coding guidelines
.github/workflows/ci.yml (2)
121-121: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUse the same Barge pin in the system job.
The integration fixture is pinned, but
test_systemstill checks out Barge's moving default branch. Pin that checkout to this SHA as well to prevent the two suites from testing different infrastructure revisions.Proposed fix
- name: Checkout Barge uses: actions/checkout@v4 with: repository: 'oceanprotocol/barge' path: 'barge' + ref: '43cfdfd21154a2bae00770b779e7c39390ff5043'🤖 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 at line 121, Update the Barge checkout used by the test_system job to pin its ref to SHA 43cfdfd21154a2bae00770b779e7c39390ff5043, matching the integration fixture’s existing pin, while leaving other workflow settings unchanged.
45-45: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winKeep the minimum supported Node release in the matrix.
package.jsonadvertises support from Node 22.13.0, but CI now tests only 22.22.2. Add the minimum version so newer APIs cannot silently break the published runtime contract.Proposed matrix
- node: ['v22.22.2'] + node: ['v22.13.0', 'v22.22.2']As per coding guidelines,
**/package.json: “Require Node.js version 22.13 or newer; the repository pins Node 22.22.2 in.nvmrc.”🤖 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 at line 45, Update the Node version matrix in the CI workflow to test both the minimum supported release 22.13.0 and the existing pinned release 22.22.2. Preserve the current 22.22.2 entry while adding 22.13.0 so CI validates the package.json runtime contract.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/API.md`:
- Around line 2024-2047: Restrict the serviceList contract so authentication
alone cannot enumerate other owners’ services; make it owner-scoped or
admin-only, or document and return only non-identifying aggregates while
preserving listing sanitization. Update docs/API.md lines 2024-2047, docs/Ocean
Node.postman_collection.json lines 957-1016, and docs/services.md line 31 to
describe the same authorization, request parameters, response behavior, and
sanitization rules.
In `@docs/compute.md`:
- Line 305: Update the Devices example in getDockerAdvancedConfig documentation
to use an array of host-path strings rather than objects with PathOnHost and
PathInContainer fields; retain the /dev/net/tun host path so the function can
construct the Docker device mappings.
In `@src/components/c2d/compute_engine_base.ts`:
- Around line 344-352: Update the minimum-constraint flow around
enforceDirectConstraints and applyAggregateConstraints to repeat direct and
aggregate floor phases until no amounts change, using a bounded fixed-point
iteration. Only after convergence, run direct and aggregate maximum validation
against the final resource amounts. Add tests covering reversed resource
ordering and aggregate-floor activation of another parent.
In `@src/components/core/service/getServices.ts`:
- Around line 86-114: Update the SERVICE_LIST flow in getServices to accept and
enforce cursor/limit bounds, querying the shared database once instead of once
per engine. Apply clusterHash, status (with status taking precedence over
includeAllStatuses), and fromTimestamp predicates in the database query, then
return only the requested page while preserving the existing listed-job mapping
and pagination behavior.
- Around line 19-27: Add runtime validation for status, includeAllStatuses, and
fromTimestamp in the service retrieval flow: normalize or reject string status
values before comparisons, require includeAllStatuses to be an actual boolean,
and ensure parseFromTimestamp returns null for non-finite numeric results,
including overflow from large digit strings. Preserve existing handling for
valid values and absent timestamps.
In `@src/components/database/sqliteAuthToken.ts`:
- Around line 95-97: The expired-token branch in invalidateTokenEntry must stop
logging the reusable credential. Update the DATABASE_LOGGER.info call to
describe that an expired authentication token was invalidated, without including
token; retain the existing invalidateTokenEntry(token) behavior.
- Around line 37-46: Update the migration try/catch around the synchronous
this.db.exec ALTER TABLE call to suppress errors only when they indicate the
chainId column already exists. Re-throw corruption, permission,
malformed-schema, and all other unexpected errors so startup does not continue
with a broken authTokens table.
In `@src/utils/http.ts`:
- Around line 55-58: Prevent signed URL credentials from appearing in HTTP error
messages: update the shared download error in src/utils/http.ts (lines 55-58),
the upload error in src/components/storage/UrlStorage.ts (lines 62-66), and the
logged asset error in src/utils/asset.ts (lines 63-67) to omit or sanitize
complete URLs while preserving the existing failure behavior.
---
Outside diff comments:
In `@docs/API.md`:
- Line 1872: Update the canonical ServiceJob.status definition in the status
table to include the missing Restarting state with numeric value 45, while
preserving all existing status entries and descriptions.
In `@docs/services.md`:
- Around line 52-61: Update the “Background pipeline” lifecycle text to reflect
that container creation or start failures occur after claimLock and are not
refunded. Limit the cancelLock/refund behavior to failures before claimLock,
such as image pull, build, or scan failures, and adjust the associated
failure-status wording accordingly.
In `@src/components/c2d/compute_engine_base.ts`:
- Around line 719-730: Update the free-resource gate below the shown allocation
checks to exempt shareable discrete resources, matching the isShareableDiscrete
condition used by Gate 2. Ensure exhausted total - inUse capacity still rejects
fungible and exclusive discrete resources while allowing shareable discrete
resources to proceed.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Line 121: Update the Barge checkout used by the test_system job to pin its ref
to SHA 43cfdfd21154a2bae00770b779e7c39390ff5043, matching the integration
fixture’s existing pin, while leaving other workflow settings unchanged.
- Line 45: Update the Node version matrix in the CI workflow to test both the
minimum supported release 22.13.0 and the existing pinned release 22.22.2.
Preserve the current 22.22.2 entry while adding 22.13.0 so CI validates the
package.json runtime contract.
In `@src/components/database/sqliteCompute.ts`:
- Around line 68-70: Update generateJSONFromBlob() to declare and return a
synchronous parsed JSON value rather than Promise<any>, matching its immediate
JSON.parse result and callers’ non-await usage. Remove any asynchronous return
typing or wrapping while preserving the existing Buffer decoding and parsing
behavior.
In `@src/test/unit/service/serviceJobsDatabase.test.ts`:
- Around line 144-191: Extend the service lifecycle lock tests to use a
temporary database with two independently opened C2DDatabase handles or
processes, rather than the shared db instance. Race acquireServiceLock for the
same serviceId using distinct holder IDs and assert exactly one acquisition
succeeds, then cleanly release the winning lock and close both handles.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b5e48749-924e-472d-aea9-7fb65abbdde8
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (56)
.eslintrc.github/workflows/ci.yml.nvmrcCLAUDE.mdREADME.mddocs/API.mddocs/GPU.mddocs/Ocean Node.postman_collection.jsondocs/compute-pricing.mddocs/compute.mddocs/env.mddocs/services.mdpackage.jsonsrc/@types/C2D/C2D.tssrc/@types/C2D/ServiceOnDemand.tssrc/@types/commands.tssrc/components/Indexer/processors/BaseProcessor.tssrc/components/Indexer/purgatory.tssrc/components/c2d/compute_engine_base.tssrc/components/c2d/compute_engine_docker.tssrc/components/core/handler/coreHandlersRegistry.tssrc/components/core/service/extendService.tssrc/components/core/service/getServices.tssrc/components/core/service/getStreamableLogs.tssrc/components/core/service/index.tssrc/components/core/service/restartService.tssrc/components/core/service/startService.tssrc/components/core/service/stopService.tssrc/components/core/service/utils.tssrc/components/database/AuthTokenDatabase.tssrc/components/database/C2DDatabase.tssrc/components/database/SQLLiteConfigDatabase.tssrc/components/database/SQLLiteNonceDatabase.tssrc/components/database/sqlite.tssrc/components/database/sqliteAuthToken.tssrc/components/database/sqliteClient.tssrc/components/database/sqliteCompute.tssrc/components/database/typesenseApi.tssrc/components/httpRoutes/compute.tssrc/components/persistentStorage/PersistentStorageFactory.tssrc/components/storage/ArweaveStorage.tssrc/components/storage/IpfsStorage.tssrc/components/storage/UrlStorage.tssrc/test/integration/imageCleanup.test.tssrc/test/integration/services.test.tssrc/test/unit/compute.test.tssrc/test/unit/http.test.tssrc/test/unit/service/serviceHandlers.test.tssrc/test/unit/service/serviceJobsDatabase.test.tssrc/test/unit/service/serviceNetworkCleanup.test.tssrc/test/unit/service/serviceRestartRace.test.tssrc/test/unit/sqliteClient.test.tssrc/utils/asset.tssrc/utils/config/schemas.tssrc/utils/constants.tssrc/utils/http.ts
💤 Files with no reviewable changes (2)
- docs/GPU.md
- docs/compute-pricing.md
🚧 Files skipped from review as they are similar to previous changes (15)
- src/components/core/service/index.ts
- src/@types/C2D/ServiceOnDemand.ts
- src/components/core/service/getStreamableLogs.ts
- src/components/httpRoutes/compute.ts
- src/utils/constants.ts
- docs/env.md
- src/components/core/service/startService.ts
- src/components/core/service/restartService.ts
- src/components/database/C2DDatabase.ts
- src/@types/commands.ts
- src/utils/config/schemas.ts
- src/components/core/service/extendService.ts
- src/components/c2d/compute_engine_docker.ts
- src/components/core/service/stopService.ts
- src/@types/C2D/C2D.ts
Comment on lines
+2024
to
+2047
| Node-wide service listing. **Authenticated but NOT owner-scoped** — any authenticated | ||
| consumer identity sees every owner's services. By default it returns only the services | ||
| **currently holding a resource reservation** (exactly what the engines count against the | ||
| shared pools): `Running`/`Restarting`/`Stopping`, the mid-start pipeline states, paid | ||
| `Error` (container died, restartable), and explicitly `Stopped` within the paid window. | ||
| `Expired` and never-paid jobs hold nothing and are not listed by default. | ||
|
|
||
| #### Query Parameters | ||
|
|
||
| | name | type | required | description | | ||
| | ----------------- | ------- | -------- | ----------- | | ||
| | consumerAddress | string | v | caller identity (any consumer) | | ||
| | nonce | string | v | request nonce | | ||
| | signature | string | v | signed message (or use an `Authorization` auth-token header) | | ||
| | status | number | | filter to ONE specific `ServiceStatusNumber` (any status, incl. `75` Expired); takes precedence over `includeAllStatuses` | | ||
| | includeAllStatuses | boolean | | `true` returns services in every status instead of only the resource-holding set | | ||
| | fromTimestamp | string | | only services created at/after this moment — ISO date (`2026-01-15T00:00:00Z`) or Unix timestamp (seconds or milliseconds) | | ||
|
|
||
| #### Response (200) | ||
|
|
||
| Array of `ServiceJob`, **listing-sanitized**: `userData`, `dockerCmd`, `dockerEntrypoint`, | ||
| `dockerfile` and `additionalDockerFiles` are stripped (identity, status, resources, | ||
| endpoints and payment metadata are kept). Use the owner-scoped `serviceStatus` to see a | ||
| service's own configuration. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Restrict cross-owner service enumeration.
Any wallet can authenticate, so the current contract exposes owners, live endpoints, and payment metadata without meaningful authorization.
docs/API.md#L2024-L2047: makeserviceListowner-scoped/admin-only or return non-identifying aggregates.docs/Ocean Node.postman_collection.json#L957-L1016: update the request and description to match the restricted contract.docs/services.md#L31-L31: document the corrected authorization and sanitization behavior.
📍 Affects 3 files
docs/API.md#L2024-L2047(this comment)docs/Ocean Node.postman_collection.json#L957-L1016docs/services.md#L31-L31
🤖 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/API.md` around lines 2024 - 2047, Restrict the serviceList contract so
authentication alone cannot enumerate other owners’ services; make it
owner-scoped or admin-only, or document and return only non-identifying
aggregates while preserving listing sanitization. Update docs/API.md lines
2024-2047, docs/Ocean Node.postman_collection.json lines 957-1016, and
docs/services.md line 31 to describe the same authorization, request parameters,
response behavior, and sanitization rules.
| "type": "network", | ||
| "total": 1, | ||
| "description": "SR-IOV NIC", | ||
| "init": { "advanced": { "Devices": [{ "PathOnHost": "/dev/net/tun", "PathInContainer": "/dev/net/tun" }] } } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use host-path strings in the Devices example.
getDockerAdvancedConfig() expects init.advanced.Devices to be a string array and constructs the Docker mappings itself. The documented object would be treated as PathOnHost.
Proposed correction
- "init": { "advanced": { "Devices": [{ "PathOnHost": "/dev/net/tun", "PathInContainer": "/dev/net/tun" }] } }
+ "init": { "advanced": { "Devices": ["/dev/net/tun"] } }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| "init": { "advanced": { "Devices": [{ "PathOnHost": "/dev/net/tun", "PathInContainer": "/dev/net/tun" }] } } | |
| "init": { "advanced": { "Devices": ["/dev/net/tun"] } } |
🤖 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.md` at line 305, Update the Devices example in
getDockerAdvancedConfig documentation to use an array of host-path strings
rather than objects with PathOnHost and PathInContainer fields; retain the
/dev/net/tun host path so the function can construct the Docker device mappings.
Comment on lines
+344
to
+352
| ): void { | ||
| // Two-phase on purpose: a later parent's min bump can raise a target past an earlier | ||
| // parent's already-checked max (e.g. gpu1 {cpu min:8} bumping past gpu0 {cpu max:6}), | ||
| // so all floors settle first (direct, then aggregate) and every ceiling is validated | ||
| // afterwards against the final amounts — the outcome no longer depends on the order | ||
| // resources appear in the env definition. | ||
| this.enforceDirectConstraints(resources, env, isFree, 'min') | ||
| this.applyAggregateConstraints(resources, env, isFree) | ||
| this.enforceDirectConstraints(resources, env, isFree, 'max') |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Resolve chained floors to a fixed point before checking ceilings.
Direct minima are evaluated only once in environment order. If a later floor activates an earlier resource—or an aggregate floor activates another parent—that parent’s constraints are never revisited, allowing under-constrained requests based on resource ordering.
Iterate all minimum phases to a bounded fixed point, then validate aggregate and direct maxima. Add reversed-order and aggregate-activation tests.
Also applies to: 370-485, 500-562
🤖 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 344 - 352, Update the
minimum-constraint flow around enforceDirectConstraints and
applyAggregateConstraints to repeat direct and aggregate floor phases until no
amounts change, using a bounded fixed-point iteration. Only after convergence,
run direct and aggregate maximum validation against the final resource amounts.
Add tests covering reversed resource ordering and aggregate-floor activation of
another parent.
Comment on lines
+19
to
+27
| export function parseFromTimestamp(value?: string): number | undefined | null { | ||
| if (value === undefined || value === null || value === '') return undefined | ||
| if (/^\d+$/.test(String(value))) { | ||
| const n = Number(value) | ||
| // 1e12 ms ≈ Sep 2001; any plausible seconds value is far below it | ||
| return n > 1e12 ? n : n * 1000 | ||
| } | ||
| const t = Date.parse(String(value)) | ||
| return Number.isNaN(t) ? null : t |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
sed -n '1,220p' src/components/core/service/getServices.tsRepository: oceanprotocol/ocean-node
Length of output: 5093
🏁 Script executed:
grep -RIn --line-number --include='*.ts' -E 'enum ServiceStatusNumber|type GetServicesCommand|interface GetServicesCommand|includeAllStatuses|fromTimestamp' src/@types src/components/core/service | sed -n '1,200p'
python3 - <<'PY'
import math
tests = [10**12 + 1, 10**13, 10**15, 10**16, 10**18, 10**100]
for n in tests:
ms = n if n > 1e12 else n * 1000
try:
dt = __import__('datetime').datetime.utcfromtimestamp(ms / 1000)
finite = True
except Exception as e:
finite = False
dt = repr(e)
print(n, ms, finite, dt)
print('Date.parse-like range probe not available in Python, but huge ints remain finite in JS numbers only up to 1.8e308')
PYRepository: oceanprotocol/ocean-node
Length of output: 2489
🏁 Script executed:
sed -n '60,120p' src/@types/C2D/ServiceOnDemand.ts
sed -n '440,485p' src/@types/commands.ts
sed -n '1,220p' src/components/httpRoutes/validateCommands.ts
node - <<'JS'
const enumLike = {0: 'Stopped', 1: 'Running', Running: 1, Stopped: 0}
for (const v of ['Running', 'Stopped', '6', 6, 0, 1, '0']) {
console.log(v, enumLike[v] === undefined, enumLike[v])
}
for (const v of ['9'.repeat(309), '9'.repeat(308), '1e309', '1000000000000000000000']) {
const n = Number(v)
console.log(v.length, Number.isFinite(n), n)
}
JSRepository: oceanprotocol/ocean-node
Length of output: 8446
🏁 Script executed:
rg -n "getServiceJob\\(|getRunningServiceJobs\\(" src -g '*.ts'Repository: oceanprotocol/ocean-node
Length of output: 4089
🏁 Script executed:
sed -n '300,370p' src/components/database/sqliteCompute.ts
node - <<'JS'
const ServiceStatusNumber = {
10: 'Starting',
40: 'Running',
Starting: 10,
Running: 40
}
for (const v of ['Running', '40', '6', 40, 6]) {
console.log(v, ServiceStatusNumber[v], ServiceStatusNumber[v] === undefined)
}
for (const v of ['false', 'true', '', '9'.repeat(309)]) {
const n = Number(v)
console.log(v, typeof v, Boolean(v), n, Number.isFinite(n))
}
JSRepository: oceanprotocol/ocean-node
Length of output: 3318
🏁 Script executed:
rg -n "getAllEngines\\(|class .*C2D|getC2DEngines\\(" src/components src -g '*.ts'Repository: oceanprotocol/ocean-node
Length of output: 7245
Validate status, includeAllStatuses, and fromTimestamp at runtime
status can arrive as a string and still pass the enum lookup, but the later === comparison never matches, and any truthy non-boolean includeAllStatuses flips the code into the full-list path. Add a finite-number guard to fromTimestamp too; very large digit strings become Infinity and return an empty result set.
🤖 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/getServices.ts` around lines 19 - 27, Add runtime
validation for status, includeAllStatuses, and fromTimestamp in the service
retrieval flow: normalize or reject string status values before comparisons,
require includeAllStatuses to be an actual boolean, and ensure
parseFromTimestamp returns null for non-finite numeric results, including
overflow from large digit strings. Preserve existing handling for valid values
and absent timestamps.
Comment on lines
+86
to
+114
| const jobs: ServiceJob[] = [] | ||
| for (const eng of engines.getAllEngines()) { | ||
| const { hash } = eng.getC2DConfig() | ||
| if (task.status !== undefined || task.includeAllStatuses) { | ||
| // status-explicit listing: read ALL of this cluster's jobs (getServiceJob has no | ||
| // cluster column filter, so scope by clusterHash here), then narrow to the one | ||
| // status when given — `status` takes precedence over `includeAllStatuses` | ||
| const all = (await eng.db.getServiceJob()).filter( | ||
| (j: ServiceJob) => j.clusterHash === hash | ||
| ) | ||
| jobs.push( | ||
| ...(task.status !== undefined | ||
| ? all.filter((j: ServiceJob) => j.status === task.status) | ||
| : all) | ||
| ) | ||
| } else { | ||
| // default: the cluster's resource-holding set | ||
| jobs.push(...(await eng.db.getRunningServiceJobs(hash))) | ||
| } | ||
| } | ||
|
|
||
| const fromMs = parseFromTimestamp(task.fromTimestamp) | ||
| const filtered = | ||
| fromMs === undefined || fromMs === null | ||
| ? jobs | ||
| : jobs.filter((j) => Date.parse(j.dateCreated) >= fromMs) | ||
|
|
||
| return { | ||
| stream: Readable.from(JSON.stringify(filtered.map(toListedServiceJob))), |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
Bound and database-filter SERVICE_LIST results.
includeAllStatuses loads the complete persistent job history once per engine despite the shared database, then builds and stringifies the entire result in memory. As expired jobs accumulate, authenticated requests can impose unbounded database, memory, and event-loop work.
Add cursor/limit parameters, apply cluster/status/timestamp filters in the database query, and read the shared database only once.
🤖 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/getServices.ts` around lines 86 - 114, Update the
SERVICE_LIST flow in getServices to accept and enforce cursor/limit bounds,
querying the shared database once instead of once per engine. Apply clusterHash,
status (with status taking precedence over includeAllStatuses), and
fromTimestamp predicates in the database query, then return only the requested
page while preserving the existing listed-job mapping and pagination behavior.
Comment on lines
+37
to
+46
| // Migration for DBs created before the chainId column existed: add it if missing. | ||
| // A fresh table already has the column, so ALTER throws "duplicate column name" — | ||
| // that's the only expected failure, so swallow it. With the synchronous exec above this | ||
| // now runs strictly after the CREATE TABLE, unlike the old callback-based `await exec` | ||
| // which never actually waited and let the CREATE race the ALTER. | ||
| try { | ||
| this.db.exec(`ALTER TABLE authTokens ADD COLUMN chainId TEXT`) | ||
| } catch { | ||
| // column already exists | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Only suppress the expected duplicate-column migration error.
The unconditional catch also hides corruption, permission, and malformed-schema failures, allowing startup with a broken authentication table.
Proposed fix
- } catch {
- // column already exists
+ } catch (error) {
+ if (
+ !(error instanceof Error) ||
+ !/duplicate column name/i.test(error.message)
+ ) {
+ throw error
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Migration for DBs created before the chainId column existed: add it if missing. | |
| // A fresh table already has the column, so ALTER throws "duplicate column name" — | |
| // that's the only expected failure, so swallow it. With the synchronous exec above this | |
| // now runs strictly after the CREATE TABLE, unlike the old callback-based `await exec` | |
| // which never actually waited and let the CREATE race the ALTER. | |
| try { | |
| this.db.exec(`ALTER TABLE authTokens ADD COLUMN chainId TEXT`) | |
| } catch { | |
| // column already exists | |
| } | |
| // Migration for DBs created before the chainId column existed: add it if missing. | |
| // A fresh table already has the column, so ALTER throws "duplicate column name" — | |
| // that's the only expected failure, so swallow it. With the synchronous exec above this | |
| // now runs strictly after the CREATE TABLE, unlike the old callback-based `await exec` | |
| // which never actually waited and let the CREATE race the ALTER. | |
| try { | |
| this.db.exec(`ALTER TABLE authTokens ADD COLUMN chainId TEXT`) | |
| } catch (error) { | |
| if ( | |
| !(error instanceof Error) || | |
| !/duplicate column name/i.test(error.message) | |
| ) { | |
| throw error | |
| } | |
| } |
🤖 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 this.db.exec ALTER TABLE call to
suppress errors only when they indicate the chainId column already exists.
Re-throw corruption, permission, malformed-schema, and all other unexpected
errors so startup does not continue with a broken authTokens table.
Comment on lines
+95
to
+97
| if (validUntilDate < now) { | ||
| DATABASE_LOGGER.info(`Auth token ${token} is invalid`) | ||
| await this.invalidateTokenEntry(token) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not log the authentication token.
Line 96 writes a reusable credential to application logs. Log only that an expired token was invalidated, or use a non-reversible identifier.
Proposed fix
- DATABASE_LOGGER.info(`Auth token ${token} is invalid`)
+ DATABASE_LOGGER.info('Expired auth token was invalidated')📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (validUntilDate < now) { | |
| DATABASE_LOGGER.info(`Auth token ${token} is invalid`) | |
| await this.invalidateTokenEntry(token) | |
| if (validUntilDate < now) { | |
| DATABASE_LOGGER.info('Expired auth token was invalidated') | |
| await this.invalidateTokenEntry(token) |
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 95-95: Avoid logging sensitive data
Context: DATABASE_LOGGER.info(Auth token ${token} is invalid)
Note: [CWE-532] Insertion of Sensitive Information into Log File.
(log-sensitive-data-typescript)
🤖 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, The
expired-token branch in invalidateTokenEntry must stop logging the reusable
credential. Update the DATABASE_LOGGER.info call to describe that an expired
authentication token was invalidated, without including token; retain the
existing invalidateTokenEntry(token) behavior.
Source: Linters/SAST tools
Comment on lines
+55
to
+58
| if (!response.ok) { | ||
| // preserve axios's throw-on-non-2xx contract for all callers | ||
| await response.body?.cancel().catch(() => {}) | ||
| throw new Error(`Request failed with status code ${response.status} (${url})`) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not expose complete URLs in HTTP errors.
Signed URLs may contain credentials in query parameters, which can then leak through logs or API responses.
src/utils/http.ts#L55-L58: omit or sanitize the URL in the shared download error.src/components/storage/UrlStorage.ts#L62-L66: omit or sanitize the upload URL.src/utils/asset.ts#L63-L67: omit the URL because the resulting error is logged.
📍 Affects 3 files
src/utils/http.ts#L55-L58(this comment)src/components/storage/UrlStorage.ts#L62-L66src/utils/asset.ts#L63-L67
🤖 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/http.ts` around lines 55 - 58, Prevent signed URL credentials from
appearing in HTTP error messages: update the shared download error in
src/utils/http.ts (lines 55-58), the upload error in
src/components/storage/UrlStorage.ts (lines 62-66), and the logged asset error
in src/utils/asset.ts (lines 63-67) to omit or sanitize complete URLs while
preserving the existing failure behavior.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes # .
Changes proposed in this PR:
Summary by CodeRabbit