Conversation
alexcos20
requested review from
andreip136,
bogdanfazakas,
dnsi0 and
giurgiur99
as code owners
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR introduces shared connection-level compute resources with environment references, adds Service-on-Demand APIs and Docker lifecycle handling, updates delegated authentication, and indexes escrow relock events. Documentation, configuration, CI, templates, persistence, routes, and tests are updated accordingly. ChangesCompute resource model
Service-On-Demand
Authentication and escrow indexing
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)
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
+1778
to
+1796
| // Service-on-Demand expiry: stop services whose paid window has elapsed. | ||
| const expiredServices = await this.db.getExpiredServiceJobs( | ||
| this.getC2DConfig().hash | ||
| ) | ||
| for (const svc of expiredServices) { | ||
| CORE_LOGGER.info(`Service ${svc.serviceId} expired — stopping`) | ||
| await this.stopService(svc.serviceId, svc.owner).catch((e) => { | ||
| CORE_LOGGER.error( | ||
| `Failed to stop expired service ${svc.serviceId}: ${e.message}` | ||
| ) | ||
| }) | ||
| // mark the (now stopped) record as Expired so it is not picked up again | ||
| const [stoppedJob] = await this.db.getServiceJob(svc.serviceId, svc.owner) | ||
| if (stoppedJob) { | ||
| stoppedJob.status = ServiceStatusNumber.Expired | ||
| stoppedJob.statusText = ServiceStatusText[ServiceStatusNumber.Expired] | ||
| await this.db.updateServiceJob(stoppedJob) | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Expired-service cleanup always marks the record Expired, even when teardown fails.
stopService never throws — on a genuine cleanup failure (non-benign Docker error) it internally sets job.status = Error and simply returns the job, so .catch() here never fires. This loop then unconditionally re-fetches the record and overwrites its status to Expired regardless of what stopService actually returned. If the container/network teardown failed, releaseHostPort was never called inside stopService (it's skipped once the try-block throws), so the host ports stay reserved in memory while the record is now marked Expired and will no longer be picked up by getExpiredServiceJobs on the next tick — the Docker container/network can keep running indefinitely with no retry, and after a node restart those same ports won't be re-seeded via seedAllocatedPorts (which only seeds "running" jobs), risking a port collision when a new service later gets allocated the same host port.
🐛 Proposed fix
- await this.stopService(svc.serviceId, svc.owner).catch((e) => {
- CORE_LOGGER.error(
- `Failed to stop expired service ${svc.serviceId}: ${e.message}`
- )
- })
- // mark the (now stopped) record as Expired so it is not picked up again
- const [stoppedJob] = await this.db.getServiceJob(svc.serviceId, svc.owner)
- if (stoppedJob) {
- stoppedJob.status = ServiceStatusNumber.Expired
- stoppedJob.statusText = ServiceStatusText[ServiceStatusNumber.Expired]
- await this.db.updateServiceJob(stoppedJob)
- }
+ const stopped = await this.stopService(svc.serviceId, svc.owner).catch((e) => {
+ CORE_LOGGER.error(
+ `Failed to stop expired service ${svc.serviceId}: ${e.message}`
+ )
+ return null
+ })
+ // Only flip to Expired once teardown actually succeeded; otherwise leave the
+ // Error status so it keeps getting retried instead of leaking the container.
+ if (stopped && stopped.status === ServiceStatusNumber.Stopped) {
+ stopped.status = ServiceStatusNumber.Expired
+ stopped.statusText = ServiceStatusText[ServiceStatusNumber.Expired]
+ await this.db.updateServiceJob(stopped)
+ }📝 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
| // Service-on-Demand expiry: stop services whose paid window has elapsed. | |
| const expiredServices = await this.db.getExpiredServiceJobs( | |
| this.getC2DConfig().hash | |
| ) | |
| for (const svc of expiredServices) { | |
| CORE_LOGGER.info(`Service ${svc.serviceId} expired — stopping`) | |
| await this.stopService(svc.serviceId, svc.owner).catch((e) => { | |
| CORE_LOGGER.error( | |
| `Failed to stop expired service ${svc.serviceId}: ${e.message}` | |
| ) | |
| }) | |
| // mark the (now stopped) record as Expired so it is not picked up again | |
| const [stoppedJob] = await this.db.getServiceJob(svc.serviceId, svc.owner) | |
| if (stoppedJob) { | |
| stoppedJob.status = ServiceStatusNumber.Expired | |
| stoppedJob.statusText = ServiceStatusText[ServiceStatusNumber.Expired] | |
| await this.db.updateServiceJob(stoppedJob) | |
| } | |
| } | |
| // Service-on-Demand expiry: stop services whose paid window has elapsed. | |
| const expiredServices = await this.db.getExpiredServiceJobs( | |
| this.getC2DConfig().hash | |
| ) | |
| for (const svc of expiredServices) { | |
| CORE_LOGGER.info(`Service ${svc.serviceId} expired — stopping`) | |
| const stopped = await this.stopService(svc.serviceId, svc.owner).catch((e) => { | |
| CORE_LOGGER.error( | |
| `Failed to stop expired service ${svc.serviceId}: ${e.message}` | |
| ) | |
| return null | |
| }) | |
| // Only flip to Expired once teardown actually succeeded; otherwise leave the | |
| // Error status so it keeps getting retried instead of leaking the container. | |
| if (stopped && stopped.status === ServiceStatusNumber.Stopped) { | |
| stopped.status = ServiceStatusNumber.Expired | |
| stopped.statusText = ServiceStatusText[ServiceStatusNumber.Expired] | |
| await this.db.updateServiceJob(stopped) | |
| } | |
| } |
🤖 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 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.
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
+3313
to
+3329
| try { | ||
| // 1. LOCKING — lock the consumer's funds in escrow (refundable until claimed). | ||
| job.status = ServiceStatusNumber.Locking | ||
| job.statusText = ServiceStatusText[ServiceStatusNumber.Locking] | ||
| await this.db.updateServiceJob(job) | ||
| const lockTx = await this.escrow.createLock( | ||
| chainId, | ||
| serviceId, | ||
| token, | ||
| job.owner, | ||
| job.payment.cost, | ||
| this.escrow.getMinLockTime(job.duration) | ||
| ) | ||
| if (!lockTx) throw new Error('Escrow lock failed') | ||
| await this.escrow.waitForTransaction(chainId, lockTx) | ||
| job.payment.lockTx = lockTx | ||
| await this.db.updateServiceJob(job) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
lockTx is only persisted after waitForTransaction succeeds, so a timeout loses refund tracking.
If this.escrow.waitForTransaction(chainId, lockTx) throws (e.g. slow confirmation past the default 60s timeout) or otherwise rejects, execution jumps straight to the outer catch, and job.payment.lockTx was never assigned. The catch's refund guard (if (job.payment.lockTx && !job.payment.claimTx && !job.payment.cancelTx)) then evaluates false, so safeCancelLock is never attempted even though createLock actually succeeded on-chain and the consumer's funds are locked. The lock now silently sits until it naturally expires (job.duration + claimDurationTimeout) and is only swept much later by the periodic cleanUpUnknownLocks fallback — the consumer's funds are effectively frozen for a failed service start.
🐛 Proposed fix
const lockTx = await this.escrow.createLock(
chainId,
serviceId,
token,
job.owner,
job.payment.cost,
this.escrow.getMinLockTime(job.duration)
)
if (!lockTx) throw new Error('Escrow lock failed')
- await this.escrow.waitForTransaction(chainId, lockTx)
job.payment.lockTx = lockTx
await this.db.updateServiceJob(job)
+ await this.escrow.waitForTransaction(chainId, lockTx)📝 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
| try { | |
| // 1. LOCKING — lock the consumer's funds in escrow (refundable until claimed). | |
| job.status = ServiceStatusNumber.Locking | |
| job.statusText = ServiceStatusText[ServiceStatusNumber.Locking] | |
| await this.db.updateServiceJob(job) | |
| const lockTx = await this.escrow.createLock( | |
| chainId, | |
| serviceId, | |
| token, | |
| job.owner, | |
| job.payment.cost, | |
| this.escrow.getMinLockTime(job.duration) | |
| ) | |
| if (!lockTx) throw new Error('Escrow lock failed') | |
| await this.escrow.waitForTransaction(chainId, lockTx) | |
| job.payment.lockTx = lockTx | |
| await this.db.updateServiceJob(job) | |
| try { | |
| // 1. LOCKING — lock the consumer's funds in escrow (refundable until claimed). | |
| job.status = ServiceStatusNumber.Locking | |
| job.statusText = ServiceStatusText[ServiceStatusNumber.Locking] | |
| await this.db.updateServiceJob(job) | |
| const lockTx = await this.escrow.createLock( | |
| chainId, | |
| serviceId, | |
| token, | |
| job.owner, | |
| job.payment.cost, | |
| this.escrow.getMinLockTime(job.duration) | |
| ) | |
| if (!lockTx) throw new Error('Escrow lock failed') | |
| job.payment.lockTx = lockTx | |
| await this.db.updateServiceJob(job) | |
| await this.escrow.waitForTransaction(chainId, lockTx) |
🤖 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 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.
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>
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