new arg "release" for stop service - #1434
Conversation
|
@coderabbitai review |
📝 WalkthroughWalkthrough
ChangesService stop release
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
✅ Action performedReview finished.
|
|
/run-security-scan |
alexcos20
left a comment
There was a problem hiding this comment.
AI automated code review (Gemini 3).
Overall risk: low
Summary:
The PR implements a well-designed release argument for the serviceStop endpoint to support early-termination of paid reservations. The code is clean, introduces solid test coverage, and includes a nice refactoring to runExclusive for lifecycle lock management. LGTM!
Comments:
• [WARNING][bug] The refactoring to use this.runExclusive is much cleaner for lock management! However, the previous implementation manually tracked the operation in this.serviceOpPromises (with the comment explicitly mentioning engine stop() drains an in-flight stop too). Please verify if this.runExclusive internally tracks the operation in serviceOpPromises. If it does not, you might lose the graceful shutdown behavior for in-flight stops.
• [INFO][performance] Excellent handling of the early release edge case. Conditionally calling await this.db.updateServiceJob(job) only when the service is already Stopped efficiently avoids duplicate database writes for Running services (where the DB update naturally happens at the end of the teardown process).
• [INFO][style] Using strict equality task.release === true here is excellent for security and predictability, as it guarantees a boolean value and defends against prototype pollution or unexpected type coercions. Just keep in mind that if clients send stringified payloads (e.g., "true"), it will silently evaluate to false (though this is standard for JSON APIs).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/httpRoutes/compute.ts`:
- Line 426: Remove the trailing commas at both affected sites: after release:
req.body.release in src/components/httpRoutes/compute.ts lines 426-426 and after
task.release === true in src/components/core/service/stopService.ts lines 65-65,
preserving the surrounding TypeScript syntax.
🪄 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: 4e889c1a-1476-4c61-957e-d0168700b98d
📒 Files selected for processing (9)
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/stopService.tssrc/components/httpRoutes/compute.tssrc/test/unit/service/serviceNetworkCleanup.test.ts
| nonce: (req.body.nonce as string) || null, | ||
| signature: (req.body.signature as string) || null, | ||
| serviceId: (req.body.serviceId as string) || null, | ||
| release: req.body.release, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the trailing commas from both changed TypeScript sites.
As per coding guidelines: TypeScript formatting requires no trailing commas.
src/components/httpRoutes/compute.ts#L426-L426: remove the comma afterrelease: req.body.release.src/components/core/service/stopService.ts#L65-L65: remove the comma aftertask.release === true.
📍 Affects 2 files
src/components/httpRoutes/compute.ts#L426-L426(this comment)src/components/core/service/stopService.ts#L65-L65
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/httpRoutes/compute.ts` at line 426, Remove the trailing commas
at both affected sites: after release: req.body.release in
src/components/httpRoutes/compute.ts lines 426-426 and after task.release ===
true in src/components/core/service/stopService.ts lines 65-65, preserving the
surrounding TypeScript syntax.
Source: Coding guidelines
Fixes # .
Changes proposed in this PR:
Summary by CodeRabbit
New Features
Documentation
Tests