add admin stopCompute#1392
Conversation
|
Important Review skippedDraft detected. 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:
📝 WalkthroughWalkthroughAdds a new admin-only ChangesAdmin StopJob Command
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
/run-security-scan |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/core/admin/stopJob.ts`:
- Around line 65-71: In the catch block of the stopJob function (lines 65-71),
the code directly accesses error.message without verifying that the caught value
is an Error instance. This can crash the handler if a non-Error value (string,
null, or plain object) is thrown. Use a type guard or utility function to safely
extract the error message: check if error is an instance of Error and access its
message property, otherwise convert the error to a string or use a fallback
message. Apply this safe extraction in both places where error.message is used:
in the CORE_LOGGER.error call and in the status object's error field.
🪄 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: 65b798a3-40c6-4dfe-92f0-df30a45db663
📒 Files selected for processing (5)
src/@types/commands.tssrc/components/core/admin/stopJob.tssrc/components/core/handler/coreHandlersRegistry.tssrc/test/integration/stopJob.test.tssrc/utils/constants.ts
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:
The PR correctly introduces the stopJob admin command, integrating it properly with the existing admin handler framework and command registry. Validation and authorization checks are properly implemented, leveraging the AdminCommandHandler base class. A few minor improvements related to error handling and HTTP status semantics are suggested.
Comments:
• [WARNING][bug] Is it guaranteed that engines.getC2DByHash(hash) throws an error if the engine is not found? If it can return undefined or null, the subsequent call to engine.stopComputeJob(...) will throw a TypeError and crash the execution abruptly. Adding a truthiness check is safer.
try {
engine = await engines.getC2DByHash(hash)
+ if (!engine) {
+ throw new Error('C2D engine not found')
+ }
} catch (e) {• [INFO][style] When a client provides an invalid hash that cannot be mapped to a C2D environment, returning an HTTP 400 Bad Request or 404 Not Found is semantically more appropriate than a 500 Internal Server Error, as the failure is due to bad client input, not an internal server misconfiguration.
- status: { httpStatus: 500, error: 'Invalid C2D Environment' }
+ status: { httpStatus: 400, error: 'Invalid C2D Environment' }• [WARNING][bug] In TypeScript, caught errors are of type unknown or any. Directly accessing error.message could cause a runtime crash if a non-Error value is thrown, and will cause type errors if strict mode is enabled. It's safer to handle the error type explicitly.
- } catch (error) {
- CORE_LOGGER.error(error.message)
+ } catch (error: any) {
+ const errorMessage = error instanceof Error ? error.message : String(error)
+ CORE_LOGGER.error(errorMessage)
return {
stream: null,
- status: { httpStatus: 500, error: error.message }
+ status: { httpStatus: 500, error: errorMessage }
}
}• [INFO][style] The integration tests cover the negative edge cases thoroughly (missing jobId, non-admin, bad format, no engines). To ensure full coverage, consider using a mocking tool (like sinon or jest.mock) to mock the C2D engine and test the successful execution (happy path) where stopComputeJob is successfully called.
1 participant
Changes proposed in this PR:
Admins need a way to forcefully terminate a running compute job without being the job's consumer. The existing
ComputeStopHandlerrequires a consumer signature proving job ownership. A new admin-onlystopJobcommand bypasses that ownership requirement, allowing the node owner to stop any job by its ID.Summary by CodeRabbit
Summary by CodeRabbit