Fix(plane)/fix issue: control plane - #1089
Conversation
📦 BoxLite review — couldn't completepowered by BoxLite |
|
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (15)
🚧 Files skipped from review as they are similar to previous changes (15)
📝 WalkthroughWalkthroughLaunch configuration is added to box creation contracts, persisted with boxes, forwarded through V0 and V2 runner paths, and applied during runtime creation. Foreground launches defer process startup and settle in ChangesForeground launch configuration
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Client
participant BoxliteBoxController
participant BoxService
participant RunnerAdapterV2
participant JobStateHandlerService
Client->>BoxliteBoxController: Create box with launch configuration
BoxliteBoxController->>BoxService: Create box with launchConfig
BoxService->>RunnerAdapterV2: createBox(box, metadata, skipStart)
RunnerAdapterV2->>JobStateHandlerService: CREATE_BOX job with launch fields
JobStateHandlerService->>BoxService: Set state and desiredState to STOPPED
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@apps/api/src/box/managers/box-actions/box.action.ts`:
- Around line 67-69: The desired-state update currently allows notifications
before the database transaction commits. In
apps/api/src/box/managers/box-actions/box.action.ts lines 67-69, prevent
extraUpdateData from bypassing the service-level post-commit transition flow; in
apps/api/src/box/managers/box-actions/box-start.action.ts lines 87-96 and
apps/api/src/box/services/job-state-handler.service.ts lines 113-116, route
foreground creation and V2 create-job completion through that same flow. Ensure
BoxEvents.DESIRED_STATE_UPDATED is emitted only after the surrounding
transaction successfully commits.
In `@apps/api/src/box/runner-adapter/runnerAdapter.v2.ts`:
- Line 150: Update RunnerAdapterV2.inferStateFromJob so completed CREATE_BOX
jobs map to STOPPED when job.getPayload().skipStart is true, while preserving
the existing STARTED result otherwise. Add a regression test covering a
foreground CREATE_BOX job and the subsequent inferred state.
In `@apps/api/src/box/services/box.service.ts`:
- Around line 258-259: Update BoxService.create so image-matching warm-pool
boxes are reused only when createBoxDto.launchConfig is absent. Ensure requests
with launchConfig continue through normal creation, preserving launchConfig
persistence and requested entrypoint, command, working directory, detach mode,
and foreground status; leave assignWarmPoolBox unchanged unless it is extended
to apply the complete configuration.
🪄 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: 703a814a-daa7-49d9-ab69-3c95320b011c
⛔ Files ignored due to path filters (1)
apps/go.work.sumis excluded by!**/*.sum
📒 Files selected for processing (20)
apps/api/src/box/dto/box.dto.tsapps/api/src/box/dto/create-box.dto.tsapps/api/src/box/entities/box.entity.tsapps/api/src/box/managers/box-actions/box-start.action.spec.tsapps/api/src/box/managers/box-actions/box-start.action.tsapps/api/src/box/managers/box-actions/box.action.tsapps/api/src/box/runner-adapter/runnerAdapter.tsapps/api/src/box/runner-adapter/runnerAdapter.v0.tsapps/api/src/box/runner-adapter/runnerAdapter.v2.tsapps/api/src/box/services/box.service.tsapps/api/src/box/services/job-state-handler.service.spec.tsapps/api/src/box/services/job-state-handler.service.tsapps/api/src/boxlite-rest/boxlite-box.controller.tsapps/api/src/boxlite-rest/dto/create-box.dto.spec.tsapps/api/src/boxlite-rest/dto/create-box.dto.tsapps/api/src/boxlite-rest/mappers/box-to-box.mapper.spec.tsapps/api/src/boxlite-rest/mappers/box-to-box.mapper.tsapps/libs/runner-api-client/src/models/create-box-dto.tsapps/runner/pkg/api/dto/box.goapps/runner/pkg/boxlite/client.go
| const updateData: Partial<Box> = { | ||
| // 状态转换有时需要原子地修正 desiredState 等关联字段。 | ||
| ...extraUpdateData, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Publish foreground desired-state transitions only after commit.
These paths persist desiredState: STOPPED through BoxRepository.update. Route the atomic state/desired-state transition through a service-level post-commit flow (or transactional outbox), then notify proxies only after the write commits; otherwise a proxy can act on an uncommitted or rolled-back desired state.
apps/api/src/box/managers/box-actions/box.action.ts#L67-L69: prevent generic extra fields from bypassing the post-commit desired-state notification flow.apps/api/src/box/managers/box-actions/box-start.action.ts#L87-L96: use the post-commit flow for V0 foreground creation.apps/api/src/box/services/job-state-handler.service.ts#L113-L116: use the same post-commit flow for V2 create-job completion.
Based on learnings, BoxEvents.DESIRED_STATE_UPDATED side effects must be emitted only after the surrounding transaction commits.
📍 Affects 3 files
apps/api/src/box/managers/box-actions/box.action.ts#L67-L69(this comment)apps/api/src/box/managers/box-actions/box-start.action.ts#L87-L96apps/api/src/box/services/job-state-handler.service.ts#L113-L116
🤖 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 `@apps/api/src/box/managers/box-actions/box.action.ts` around lines 67 - 69,
The desired-state update currently allows notifications before the database
transaction commits. In apps/api/src/box/managers/box-actions/box.action.ts
lines 67-69, prevent extraUpdateData from bypassing the service-level
post-commit transition flow; in
apps/api/src/box/managers/box-actions/box-start.action.ts lines 87-96 and
apps/api/src/box/services/job-state-handler.service.ts lines 113-116, route
foreground creation and V2 create-job completion through that same flow. Ensure
BoxEvents.DESIRED_STATE_UPDATED is emitted only after the surrounding
transaction successfully commits.
Source: Learnings
| networkAllowList: box.networkAllowList, | ||
| metadata, | ||
| authToken: box.authToken, | ||
| skipStart, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve skipStart when inferring V2 box state.
Line 150 forwards the flag, but RunnerAdapterV2.inferStateFromJob still maps every completed CREATE_BOX job to STARTED. After a foreground job completes, job-state-handler.service.ts stores STOPPED; a later boxInfo() call can report STARTED again. Read job.getPayload().skipStart in the CREATE_BOX state mapping and add a regression test.
Suggested fix
- case JobType.CREATE_BOX:
- return job.status === JobStatus.COMPLETED ? BoxState.STARTED : BoxState.CREATING
+ case JobType.CREATE_BOX: {
+ if (job.status !== JobStatus.COMPLETED) return BoxState.CREATING
+ const skippedStart = job.getPayload<{ skipStart?: boolean }>()?.skipStart === true
+ return skippedStart ? BoxState.STOPPED : BoxState.STARTED
+ }🤖 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 `@apps/api/src/box/runner-adapter/runnerAdapter.v2.ts` at line 150, Update
RunnerAdapterV2.inferStateFromJob so completed CREATE_BOX jobs map to STOPPED
when job.getPayload().skipStart is true, while preserving the existing STARTED
result otherwise. Add a regression test covering a foreground CREATE_BOX job and
the subsequent inferred state.
| // 启动参数必须随 Box 落库，不能只存在于首次发往 Runner 的请求中。 | ||
| box.launchConfig = createBoxDto.launchConfig |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not reuse warm-pool boxes when launch configuration is present.
When an image-matching warm-pool box exists, create() returns through assignWarmPoolBox() before reaching these lines. That path neither persists launchConfig nor applies the requested entrypoint, command, working directory, or detach mode; it also emits the reused box as STARTED, breaking foreground creation.
Gate warm-pool reuse on the absence of launch configuration, or implement a complete reconfiguration path.
Proposed fix
- } else if (image) {
+ } else if (image && !createBoxDto.launchConfig) {Based on the supplied BoxService.create and assignWarmPoolBox flow:
📝 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.
| // 启动参数必须随 Box 落库，不能只存在于首次发往 Runner 的请求中。 | |
| box.launchConfig = createBoxDto.launchConfig | |
| } else if (image && !createBoxDto.launchConfig) { |
🤖 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 `@apps/api/src/box/services/box.service.ts` around lines 258 - 259, Update
BoxService.create so image-matching warm-pool boxes are reused only when
createBoxDto.launchConfig is absent. Ensure requests with launchConfig continue
through normal creation, preserving launchConfig persistence and requested
entrypoint, command, working directory, detach mode, and foreground status;
leave assignWarmPoolBox unchanged unless it is extended to apply the complete
configuration.
![boxlite-agent[bot]](https://avatars.githubusercontent.com/in/4244458?s=60&v=4){kind=small}
| undefined, | ||
| undefined, | ||
| undefined, | ||
| { desiredState: BoxDesiredState.STOPPED }, |
There was a problem hiding this comment.
🛑 BoxDesiredState used but never imported
Line 95 references BoxDesiredState.STOPPED but the file's imports (lines 7-19) never import BoxDesiredState, so apps/api fails to compile/typecheck; verified via grep '^import' showing no such import.
| { desiredState: BoxDesiredState.STOPPED }, | |
| import { BoxDesiredState } from '../../enums/box-desired-state.enum' |
grep '^import' box-start.action.ts — BoxDesiredState used at line 95 but never imported
| createDto.launchConfig = { | ||
| entrypoint: dto.entrypoint, | ||
| cmd: dto.cmd, | ||
| workingDir: dto.working_dir, | ||
| tty: dto.tty, | ||
| detach: dto.detach, | ||
| // detach=false 表示 CLI 需要像本地运行一样持续接收主进程输出和退出码。 | ||
| foreground: dto.detach === false, |
There was a problem hiding this comment.
createDto.launchConfig.autoDeleteAfterExit is computed from auto_delete for foreground runs but no consumer of BoxLaunchConfig.autoDeleteAfterExit exists anywhere in the diff or repo (grep found none), so run --rm-style foreground deletion is silently a no-op despite being modeled as supported.
1 participant
Initial Demo
Summary
boxlite runreliable #1043为可靠的远程前台运行补充控制面基础能力：持久化主进程启动配置，并延迟启动主进程，避免客户端完成连接前，快速命令已经执行结束。
Changes
持久化 entrypoint、命令参数、工作目录、TTY、detach 模式和退出后清理意图。
在 REST 接口入口校验并转换前台启动参数。
通过 Runner API V0 和 V2 传递启动配置及 skipStart。
前台 Box 创建完成后保持 STOPPED，同时将 desiredState 设置为 STOPPED，防止状态协调器提前启动主进程。
Go Runner 已支持命令参数、工作目录和 detach 配置；创建时的 TTY 暂不支持，仅记录警告日志。
增加启动参数映射、校验，以及同步和异步状态处理测试。
Risks / rollout
主进程 attach 代理尚未端到端实现，因此这次修改只是远程前台运行的基础能力，还不是完整解决方案。
autoDeleteAfterExit 目前只记录 --rm 意图，主进程退出后自动删除 Box 的逻辑尚未实现。
当前 Go SDK 不支持创建时设置 TTY，tty: true 只会产生警告日志。
新增启动字段暂时没有 Runner capability 或最低版本检查。部署时应先执行数据库迁移，再升级 Runner，最后升级 API。
Summary by CodeRabbit
New Features
Bug Fixes
Tests