allow docker specs on service restart#1429
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 PR elegantly introduces RESPEC mode for serviceRestart, giving users the ability to dynamically update container specifications (e.g., swapping a new image tag to fix a bug) without losing their paid duration or host resources. The distinction between 'all-old' (REUSE) and 'all-new' (RESPEC) prevents fragmented, hard-to-debug container states. The logic is securely implemented with proper edge-case handling, including environment variable decryption validation and Dockerfile build checks.
Comments:
• [INFO][style] Good use of strict validation here. Enforcing an 'all-or-nothing' container specification when any single container parameter is supplied keeps the service state predictable and avoids messy, partial overwrites.
• [INFO][security] Excellent authoritative backstop. Even though the handler fast-fails, having the engine re-validate the daemon's allowImageBuild config prevents race conditions if the environment changes right before the engine processes the operation.
• [INFO][other] The tests clearly demonstrate the expected behavior of falling back to default image CMD and ENTRYPOINT when RESPEC mode omits the command overrides or supplies empty arrays. Very thorough testing.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/core/service/restartService.ts (1)
95-186: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUnclaimed-payment restart returns 500, not the documented 400.
Every other invalid-restart condition (not found, expired, environment missing, services disabled, access denied, undecryptable userData) gets an explicit pre-check here that returns a
4xxviabuildInvalidParametersResponse/inline status. "Payment never claimed" has no such pre-check — it is only detected insideengine.restartService(compute_engine_docker.ts,if (!job.payment?.claimTx) throw new Error(...)), and that thrownErroris caught by the genericcatchat line 182 and mapped tohttpStatus: 500.This is reachable in production:
processServiceStart's failure path can leave a job inErrorstatus withpayment.lockTxset but noclaimTx(escrow lock failed outright) — that job is notExpired, so it sails past every existing guard and only fails deep inside the engine.docs/API.mddocuments this exact case as a400response, so clients built against the docs will mishandle it as a server error.Add an explicit pre-check mirroring the
Expiredcheck above (and thedockerfile/allowImageBuildfast-fail pattern — handler fast-fails with the correct status, engine backstops):🛡️ Proposed fix
// State check — cannot restart an expired service if (job.status === ServiceStatusNumber.Expired) return buildInvalidParametersResponse( buildInvalidRequestMessage('Cannot restart an expired service') ) + + // Payment check — mirrors the engine's authoritative recheck, but must surface as a + // 400 (invalid request) rather than falling through to the generic 500 catch below. + if (!job.payment?.claimTx) + return buildInvalidParametersResponse( + buildInvalidRequestMessage( + 'Cannot restart a service whose payment was never claimed (unpaid or refunded) — start a new service' + ) + )No test in
serviceHandlers.test.ts/services.test.tscurrently exercises this path — worth adding once fixed.🤖 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/restartService.ts` around lines 95 - 186, Before the existing Expired-state check in the restart handler, add a pre-check for jobs with payment.lockTx set but no payment.claimTx, returning buildInvalidParametersResponse with the documented 400 invalid-request response. Keep engine.restartService as the authoritative backstop while ensuring this condition no longer reaches the generic 500 catch; add coverage in the relevant service handler tests if available.
🤖 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.
Outside diff comments:
In `@src/components/core/service/restartService.ts`:
- Around line 95-186: Before the existing Expired-state check in the restart
handler, add a pre-check for jobs with payment.lockTx set but no
payment.claimTx, returning buildInvalidParametersResponse with the documented
400 invalid-request response. Keep engine.restartService as the authoritative
backstop while ensuring this condition no longer reaches the generic 500 catch;
add coverage in the relevant service handler tests if available.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0cf50d9d-a435-46e5-90ad-5cc04317e811
📒 Files selected for processing (11)
docs/API.mddocs/Ocean Node.postman_collection.jsondocs/services.mdsrc/@types/commands.tssrc/components/c2d/compute_engine_base.tssrc/components/c2d/compute_engine_docker.tssrc/components/core/service/restartService.tssrc/components/httpRoutes/compute.tssrc/test/integration/services.test.tssrc/test/unit/compute.test.tssrc/test/unit/service/serviceHandlers.test.ts
1 participant
Allow
serviceRestartto change the Docker image / tagMotivation
Service-on-Demand let a consumer start a long-running container from their own image, but
serviceRestartcould only recreate the container on the exact same image. The realisticrecovery flow was impossible on-node:
Previously the only option was to stop the service and start a brand-new one, losing the paid
window, the reserved resources and the allocated host ports. This PR lets a restart carry a new
image spec while keeping payment, resources, duration and endpoints intact.
Behaviour — restart is now atomic (all-old or all-new)
A restart never mixes new request params over the stored job. There are exactly two modes,
chosen by whether the request carries any container param:
imagebecomes required)Making
imagemandatory as soon as any container param is present is the discriminator thatmakes a partial change impossible: you cannot ride a new
userData/dockerCmdon top of thestored image. In RESPEC mode the image spec is validated exactly like
serviceStart(exactly oneof
tag/checksum/dockerfile;dockerfilerequiresallowImageBuildon the environment).Always preserved: identity (
serviceId, owner, environment), payment/claimTx, resources,duration/
expiresAt, and the allocated host ports/endpoints. Only the container spec can change —there is no extra charge, consistent with the pre-existing restart semantics.
This is not a breaking change: a restart with no container params behaves exactly as before.
Error responses
400— a container param was sent withoutimage(a rejected partial change), or more thanone of
tag/checksum/dockerfilewas supplied.403—dockerfilesupplied but the environment hasallowImageBuild=false(fast-fail in thehandler; the engine re-checks as the authoritative backstop).
Changes
Core
ServiceRestartCommand(src/@types/commands.ts) — addedimage,tag,checksum,dockerfile,additionalDockerFiles; documented the two-mode contract.src/components/httpRoutes/compute.ts) — maps the new body fields.ServiceRestartHandler(src/components/core/service/restartService.ts) — RESPEC validation(image required + mutual-exclusion) and the
allowImageBuildfast-fail gate; forwards the fullspec to the engine.
compute_engine_base.ts,compute_engine_docker.ts) — extendedrestartService/doRestartServicesignatures;doRestartServicenow computes the effective spec as a singleatomic choice (RESPEC ⇒ request values + recomputed
containerImage; REUSE ⇒ stored values),drives the pull-vs-build branch off the effective dockerfile, re-checks
allowImageBuild, andpersists the new image spec (
image/tag/checksum/dockerfile/additionalDockerFiles/containerImage) onto the job so subsequent restarts and orphan-recovery see what is actuallyrunning. Vulnerability scanning of the new image is inherited from
pullImageRef/buildImageFromSpec.Tests
src/test/unit/service/serviceHandlers.test.ts— RESPEC forwarding, REUSE forwards-undefined,400 for partial changes (lone tag/userData/dockerCmd), 400 for multiple image modes, 403 for a
build-disabled dockerfile respec, and 200 for a build-enabled one.
src/test/unit/compute.test.ts— engine-level: RESPEC applies + persists a new tag; RESPECoverrides cmd/entrypoint; REUSE reuses the whole stored spec; RESPEC clears cmd/entrypoint with
empty arrays. (Adjusted the pre-existing tests to the new signature/semantics.)
src/test/integration/services.test.ts—(l4)restart on a freshly-published tag applies andpersists the new image (endpoint still reachable, same hostPort + expiresAt);
(l5)a containerparam without
imageis rejected400.Docs
docs/services.md,docs/API.md(request table + 400/403 responses), and the Postmancollection updated for the two-mode contract and the bug-fix flow.
Summary by CodeRabbit
New Features
Bug Fixes