Add support for services#163
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
/run-security-scan |
alexcos20
left a comment
alexcos20
left a comment
There was a problem hiding this comment.
AI automated code review (Gemini 3).
Overall risk: low
Summary:
This is an excellent PR introducing the Service-on-Demand lifecycle commands. The architectural split between CLI parsing, core actions, and helpers is clean. Input validation, cost estimations, interactive prompts, and robust polling (especially the old container ID check during restarts) are very well implemented. A few minor informational suggestions are provided for edge cases.
Comments:
• [INFO][style] options.env is not defined as an option for the startService command (only as a positional <computeEnvId>). This fallback is redundant but harmless. You could simplify this to const envId = computeEnvId;.
• [INFO][logic] Because template values are copied before explicit flags are applied, if a template has a tag and the user explicitly passes --checksum, both tag and checksum will be truthy. This causes specCount > 1 and triggers the validation error below. Since cli.ts already prevents mixing --template and --image, this is perfectly fine for now, but worth noting if you ever want to allow users to override a template's image specifications in the future.
• [INFO][style] The userData file and inline parsing logic here works perfectly, though it duplicates some of the validation logic found in parseUserData inside src/serviceHelpers.ts. Since template validation isn't strictly needed for a restart, this is acceptable, but consider reusing parseUserData if you wish to completely centralize the parsing logic.
• [INFO][logic] The use of notContainerId to avoid race conditions when checking for Running status immediately after a restart is a fantastic piece of defensive programming. Excellent handling of this edge case!
![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
🤖 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/commands.ts`:
- Around line 1873-1879: Update the expiry logging in the service extension
success path near the extendPayments output to guard both oldExpiry and
newJob.expiresAt before calling toISOString(). Reuse the same safe
expiry-formatting behavior established by printServiceJob, ensuring undefined or
zero values do not throw after a successful serviceExtend.
In `@test/serviceFlow.test.ts`:
- Around line 40-49: Call homedir() when constructing the default address-file
path instead of interpolating the function reference; update both
test/serviceFlow.test.ts sites at lines 40-49 and 62-72, including getAddresses
and the before hook default.
🪄 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: 7723449d-06bd-4c66-9e53-be4ad933bf80
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
README.mdpackage.jsonsrc/cli.tssrc/commands.tssrc/serviceHelpers.tstest/serviceFlow.test.ts
| console.log(chalk.green(`Service ${serviceId} extended.`)); | ||
| console.log( | ||
| ` expiry: ${new Date(oldExpiry).toISOString()} → ${new Date( | ||
| newJob.expiresAt | ||
| ).toISOString()}` | ||
| ); | ||
| console.log(` extendPayments: ${newJob.extendPayments?.length ?? 0}`); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Guard expiresAt before toISOString().
If oldExpiry or newJob.expiresAt is undefined/0, new Date(...).toISOString() throws RangeError. Since this runs after serviceExtend succeeds, the throw is swallowed by the outer catch and the user sees "Error extending service" despite a successful (paid) extend. printServiceJob already guards this same field.
🛡️ Suggested guard
- console.log(chalk.green(`Service ${serviceId} extended.`));
- console.log(
- ` expiry: ${new Date(oldExpiry).toISOString()} → ${new Date(
- newJob.expiresAt
- ).toISOString()}`
- );
+ console.log(chalk.green(`Service ${serviceId} extended.`));
+ const fmt = (t?: number) =>
+ typeof t === "number" && t > 0 ? new Date(t).toISOString() : "n/a";
+ console.log(` expiry: ${fmt(oldExpiry)} → ${fmt(newJob.expiresAt)}`);📝 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.
| console.log(chalk.green(`Service ${serviceId} extended.`)); | |
| console.log( | |
| ` expiry: ${new Date(oldExpiry).toISOString()} → ${new Date( | |
| newJob.expiresAt | |
| ).toISOString()}` | |
| ); | |
| console.log(` extendPayments: ${newJob.extendPayments?.length ?? 0}`); | |
| console.log(chalk.green(`Service ${serviceId} extended.`)); | |
| const fmt = (t?: number) => | |
| typeof t === 'number' && t > 0 ? new Date(t).toISOString() : 'n/a'; | |
| console.log(` expiry: ${fmt(oldExpiry)} → ${fmt(newJob.expiresAt)}`); | |
| console.log(` extendPayments: ${newJob.extendPayments?.length ?? 0}`); |
🤖 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/commands.ts` around lines 1873 - 1879, Update the expiry logging in the
service extension success path near the extendPayments output to guard both
oldExpiry and newJob.expiresAt before calling toISOString(). Reuse the same safe
expiry-formatting behavior established by printServiceJob, ensuring undefined or
zero values do not throw after a successful serviceExtend.
| const getAddresses = () => { | ||
| const data = JSON.parse( | ||
| fs.readFileSync( | ||
| process.env.ADDRESS_FILE || | ||
| `${homedir}/.ocean/ocean-contracts/artifacts/address.json`, | ||
| "utf8" | ||
| ) | ||
| ); | ||
| return data.development; | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
homedir from os is a function; ${homedir} never calls it. Both default-path constructions interpolate the function reference instead of the resolved home directory, producing an invalid ADDRESS file path. Call homedir().
test/serviceFlow.test.ts#L40-L49: change${homedir}to${homedir()}ingetAddresses.test/serviceFlow.test.ts#L62-L72: change${homedir}to${homedir()}in thebeforehook default.
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 41-45: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFileSync(
process.env.ADDRESS_FILE ||
${homedir}/.ocean/ocean-contracts/artifacts/address.json,
"utf8"
)
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
📍 Affects 1 file
test/serviceFlow.test.ts#L40-L49(this comment)test/serviceFlow.test.ts#L62-L72
🤖 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 `@test/serviceFlow.test.ts` around lines 40 - 49, Call homedir() when
constructing the default address-file path instead of interpolating the function
reference; update both test/serviceFlow.test.ts sites at lines 40-49 and 62-72,
including getAddresses and the before hook default.
1 participant
Fixes #160
Add Service-on-Demand support to ocean-cli
Adds full CLI support for Service-on-Demand — launching long-running Docker
containers (JupyterLab, inference servers, nginx, …) on an Ocean Node compute
environment, paid up-front via on-chain escrow for a requested duration, and
reachable through forwarded ports. Unlike a compute job (runs an algorithm to
completion and exits), a service stays up until it expires, is stopped, or
is extended.
New commands (8)
getServiceTemplatesserviceTemplatesstartService--templateor--image)getServiceStatusmyServicesgetServiceslistServices--status/--include-all/--fromfiltersserviceLogscomputeServiceLogs--since)extendServicerestartService--cmd/--entrypointoverridestopServiceAll commands support both positional args and named options, work over HTTP and
P2P (transport auto-selected by ocean.js), and are picked up automatically by the
interactive REPL (no manual command-list maintenance).
Changes
src/serviceHelpers.ts(new) — pure, testable helpers: status labels &coloring, environment↔template resource matching (
availableFor,envSatisfiesTemplate,findServiceEnvironments,templateMismatchReason),default-resource resolution, client-side cost estimation,
userDataparsing/validation (keys-only echo — values are never logged), escrow
pre-verification with actionable remediation output, status polling, and a
human-first job pretty-printer.
src/commands.ts— eight newCommandsmethods plus a shared payment-prompthelper.
startServiceorchestrates the full flow: resolve env → resolvecontainer spec (template and/or explicit flags, with the
template.command → dockerCmd/template.entrypoint → dockerEntrypointrename) → resolveresources → estimate cost → pre-verify escrow → confirm → start → poll to
Runningand print the endpoint.src/cli.ts— command registration + option parsing (ports, JSONcmd/entrypoint, status filter). Read-only
getServiceTemplates/getServicesaccept an optional
--nodetarget, mirroringgetComputeEnvironments.test/serviceFlow.test.ts(new) — end-to-end system test covering the fulllifecycle, with a
skipLifecycleguard so it skips cleanly on a node withoutservices support.
README.md— feature bullet, a "Service-on-Demand" examples section, and aper-command option reference for all eight commands.
Notable correctness details
serviceRestartarg order (#2114):dockerCmd/dockerEntrypointsit betweenuserDataandsignal— the call passes all three before theAbortSignal.getServicesis node-wide, not owner-scoped (#2115): returnsServiceJobListed(docker image-spec fields stripped); results may include other owners' services.
serviceGetStreamableLogs(#2113): returns an async-iterable ornull—the null case is handled, and consumption reuses the exact buffer-then-print
pattern from
computeStreamableLogs(no forced timeout — logs are long-lived).surface only as an async
Error/*Failedstatus), and the CLI prints the exactdepositEscrow/authorizeEscrowremediation commands on failure.userDatavalues are never logged — only keys are echoed.Commandsmethods neverprocess.exitonrecoverable errors; the payment prompt opens its own readline (the REPL pauses to
yield stdin), mirroring
startCompute.Testing
Verified end-to-end against a running barge (ocean-node v4) — all eight commands,
both the happy path and failure/skip paths.
test/serviceFlow.test.tspasses10/10 in ~40 s using a lightweight custom image
(
nginxinc/nginx-unprivileged:alpineon port 8080):The client-side cost estimate matched the node-computed cost exactly, and
getServicesconfirmed thatdockerCmd/dockerEntrypoint/dockerfilearestripped from
ServiceJobListed.Summary by CodeRabbit
New Features
Documentation