fix(cubeapi): return 404 when deleting missing sandbox

[ihanzh]

Summary

Fixes #231.

This PR changes sandbox deletion behavior so deleting a nonexistent sandbox returns 404 Not Found instead of 204 No Content.

Previously, CubeMaster returned "no such sandbox" without setting a non-success ret code, so CubeAPI treated the delete operation as successful. CubeMaster now returns ErrorCode_NotFound, and CubeAPI maps that CubeMaster not-found response to AppError::NotFound.

Changes

• Return ErrorCode_NotFound from CubeMaster when deleting a missing sandbox.
• Map CubeMaster delete not-found errors to CubeAPI 404 Not Found.
• Add focused tests for CubeMaster and CubeAPI delete-missing behavior.

Validation

gofmt -l CubeMaster/pkg/service/sandbox/sandbox_remove.go CubeMaster/pkg/service/sandbox/sandbox_remove_test.go

cargo fmt --manifest-path CubeAPI/Cargo.toml -- --check

git diff --check

CUBE_MASTER_CONFIG_PATH=/home/han/project/CubeSandbox/CubeMaster/conf.yaml GOCACHE=/tmp/cubesandbox-go-cache GOMODCACHE=/tmp/cubesandbox-go-mod-cache go test ./pkg/service/sandbox

CARGO_HOME=/tmp/cubesandbox-cargo-home CARGO_TARGET_DIR=/tmp/cubesandbox-cubeapi-target cargo test --manifest-path CubeAPI/Cargo.toml services::sandboxes::tests::kill_sandbox_maps_cubemaster_not_found_to_app_not_found

Screenshots

Not applicable. This is an API behavior fix.

[cubesandboxbot]

Review: fix(cubeapi): return 404 when deleting missing sandbox

This is a well-scoped, semantically correct change. The overall assessment across all dimensions is positive — the PR is ready to merge.

Summary

• Correctness: ✅ Error code mapping is correct end-to-end
• Security: ✅ No vulnerability introduced
• Performance: ✅ No negative impact. Minor positive: reduced client retry pressure
• Documentation: ✅ No updates needed (OpenAPI spec already declares 404)
• Test coverage: ⚠ Core path covered; gaps noted below

What is done well

1. Semantically correct error mapping — deleting a nonexistent resource is a client error (404), not a server error (500).
2. Consistent helper reuse — kill_sandbox uses sandbox_not_found_or_internal for both transport-level and ret-envelope errors.
3. Test covers the contract — Go test verifies both error code and absence of after-destroy success hook.

Findings worth addressing

1. Potential bug: Sync path returns MasterInternalError instead of NotFound on race (sandbox_remove.go:101-106). If req.Sync is true and the sandbox is deleted between cache lookup and cubelet call, DestroySandbox wraps it as MasterInternalError regardless of what the cubelet returned. Pre-existing, but worth following up.

2. Test gap: sync path completely untested. Not even the success case has coverage.

3. Minor: sandbox_id dropped in non-not-found error messages (sandboxes.rs:632-633). Including it in the anyhow! message would improve debuggability.

Conclusion

Well-structured PR, ready to merge. Items 1 and 2 are follow-up candidates.

[kinwin-ustc]

LGTM

[chenhengqi]

We have testify in go.mod already, please use it for test cases.

[cubesandboxbot]

Suggestion: Include the sandbox ID in the error message for better debuggability, consistent with the Rust side which always includes it:

rsp.Ret.RetMsg = "no such sandbox: " + req.SandboxID

[cubesandboxbot]

Suggestion: The Sync: true field is unused here — the early return at line 99 fires before the sync path (line 101) is reached. Consider removing it or adding a comment explaining it is immaterial to avoid confusing future readers.