Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion apps/api/src/box/dto/box.dto.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
import { ApiProperty, ApiPropertyOptional, ApiSchema } from '@nestjs/swagger'
import { BoxState } from '../enums/box-state.enum'
import { IsEnum, IsOptional } from 'class-validator'
import { Box } from '../entities/box.entity'
import { Box, BoxLaunchConfig } from '../entities/box.entity'
import { BoxDesiredState } from '../enums/box-desired-state.enum'
import { BoxClass } from '../enums/box-class.enum'

Expand Down Expand Up @@ -203,6 +203,16 @@ export class BoxDto {
@IsOptional()
volumes?: BoxVolume[]

@ApiPropertyOptional({
description: 'Launch configuration for the box main process.',
type: 'object',
additionalProperties: true,
required: false,
})
@IsOptional()
// Return the persisted launch configuration for later foreground attach and recovery flows.
launchConfig?: BoxLaunchConfig

@ApiPropertyOptional({
description: 'The creation timestamp of the box',
example: '2024-10-01T12:00:00Z',
Expand Down Expand Up @@ -270,6 +280,7 @@ export class BoxDto {
networkAllowList: box.networkAllowList,
labels: box.labels,
volumes: box.volumes,
launchConfig: box.launchConfig,
state: this.getBoxState(box),
desiredState: box.desiredState,
errorReason: box.errorReason,
Expand Down
11 changes: 11 additions & 0 deletions apps/api/src/box/dto/create-box.dto.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import { IsEnum, IsObject, IsOptional, IsString, IsNumber, IsBoolean, IsArray, I
import { ApiPropertyOptional, ApiSchema } from '@nestjs/swagger'
import { BoxClass } from '../enums/box-class.enum'
import { BoxVolume } from './box.dto'
import { BoxLaunchConfig } from '../entities/box.entity'

@ApiSchema({ name: 'CreateBox' })
export class CreateBoxDto {
Expand Down Expand Up @@ -170,4 +171,14 @@ export class CreateBoxDto {
@IsOptional()
@IsArray()
volumes?: BoxVolume[]

@ApiPropertyOptional({
description: 'Launch configuration for the box main process. Used by remote foreground run.',
type: 'object',
additionalProperties: true,
})
@IsOptional()
@IsObject()
// The REST boundary validates and maps these fields; the service layer only persists them.
launchConfig?: BoxLaunchConfig
}
30 changes: 30 additions & 0 deletions apps/api/src/box/entities/box.entity.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,29 @@ import {
DEFAULT_AUTO_RESUME,
} from '../constants/box-lifecycle.constants'

/**
* Launch configuration for the Box main process.
*
* A remote foreground run cannot rely on transient create-request parameters. After an API
* or Runner restart, the control plane still needs these fields to resume the create-attach-start flow.
*/
export interface BoxLaunchConfig {
/** Overrides the image's default entrypoint. */
entrypoint?: string[]
/** Command and arguments passed to the entrypoint. */
cmd?: string[]
/** Working directory used when starting the main process. */
workingDir?: string
/** Whether to allocate a terminal for the main process. */
tty?: boolean
/** Whether the client requests detachment from the main process. */
detach?: boolean
/** Whether to attach the output stream before starting the main process. */
foreground?: boolean
/** Whether to delete the Box after the foreground process exits, implementing remote `run --rm`. */
autoDeleteAfterExit?: boolean
}

@Entity('box')
@Unique(['organizationId', 'name'])
@Index('box_state_idx', ['state'])
Expand Down Expand Up @@ -138,6 +161,13 @@ export class Box {
})
volumes: BoxVolume[] = []

@Column({
type: 'jsonb',
nullable: true,
})
// Persist with the Box so foreground launch semantics survive control-plane restarts.
launchConfig?: BoxLaunchConfig

@CreateDateColumn({
type: 'timestamp with time zone',
})
Expand Down
55 changes: 54 additions & 1 deletion apps/api/src/box/managers/box-actions/box-start.action.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -168,11 +168,64 @@ describe('BoxStartAction.handleRunnerBoxUnknownStateOnDesiredStateStart', () =>

const result = await (action as BoxAction).run(box, lockCode)

expect(createBox).toHaveBeenCalledWith(box, expect.any(Object))
expect(createBox).toHaveBeenCalledWith(box, expect.any(Object), false)
expect(result).toBe(SYNC_AGAIN)
expect(updatedFields.some((u) => u.state === BoxState.CREATING)).toBe(true)
})

it('creates a foreground box without starting it on a v0 runner', async () => {
const runnerId = 'runner-boot-foreground'

const box = new Box('region-1', 'foreground-box')
box.runnerId = runnerId
box.image = 'boxlite/base'
box.state = BoxState.UNKNOWN
box.desiredState = BoxDesiredState.STARTED
box.pending = true
box.launchConfig = {
cmd: ['-lc', 'echo foreground-ok; exit 7'],
detach: false,
foreground: true,
autoDeleteAfterExit: true,
}

const runner = { id: runnerId, state: RunnerState.READY, apiVersion: '0' } as Runner
const runnerService = { findOneOrFail: jest.fn(async () => runner) }

const createBox = jest.fn(async () => undefined)
const runnerAdapterFactory = { create: jest.fn(async () => ({ createBox }) as any) }

const lockCode = new LockCode('lock-boot-foreground')
const updatedFields: Partial<Box>[] = []
const boxRepository = {
update: jest.fn(async (_id: string, opts: { updateData: Partial<Box> }) => {
updatedFields.push(opts.updateData)
return box
}),
}
const redisLockProvider = { getCode: jest.fn(async () => lockCode) }
const organizationService = { findOne: jest.fn(async () => ({ boxMetadata: {} })) }

const action = new BoxStartAction(
runnerService as any,
runnerAdapterFactory as any,
boxRepository as any,
organizationService as any,
{} as any,
redisLockProvider as any,
{} as any,
)

const result = await (action as BoxAction).run(box, lockCode)

expect(createBox).toHaveBeenCalledWith(box, expect.any(Object), true)
expect(result).not.toBe(SYNC_AGAIN)
expect(updatedFields).toContainEqual({
state: BoxState.STOPPED,
desiredState: BoxDesiredState.STOPPED,
})
})

it('moves an unknown box with no image to ERROR without calling createBox', async () => {
const runnerId = 'runner-boot-2'

Expand Down
22 changes: 21 additions & 1 deletion apps/api/src/box/managers/box-actions/box-start.action.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import { Injectable, Logger } from '@nestjs/common'
import { BoxRepository } from '../../repositories/box.repository'
import { Box } from '../../entities/box.entity'
import { BoxState } from '../../enums/box-state.enum'
import { BoxDesiredState } from '../../enums/box-desired-state.enum'
import { DONT_SYNC_AGAIN, BoxAction, SYNC_AGAIN, SyncState } from './box.action'
import { RunnerState } from '../../enums/runner-state.enum'
import { RunnerService } from '../../services/runner.service'
Expand Down Expand Up @@ -77,7 +78,26 @@ export class BoxStartAction extends BoxAction {
}

const runnerAdapter = await this.runnerAdapterFactory.create(runner)
await runnerAdapter.createBox(box, metadata)
// Prepare the VM and main-process configuration first, delaying process start so fast commands
// cannot exit before the client attaches.
const skipStart = box.launchConfig?.foreground === true
await runnerAdapter.createBox(box, metadata, skipStart)

if (skipStart && runner.apiVersion === '0') {
// V0 creation is synchronous; when skipStart returns successfully, the Box exists but is not running.
// Update desiredState as well so the state reconciler does not immediately start it.
await this.updateBoxState(
box,
BoxState.STOPPED,
lockCode,
undefined,
undefined,
undefined,
undefined,
{ desiredState: BoxDesiredState.STOPPED },

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛑 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.

Suggested change
{ desiredState: BoxDesiredState.STOPPED },
import { BoxDesiredState } from '../../enums/box-desired-state.enum'

)
return DONT_SYNC_AGAIN
}

await this.updateBoxState(box, BoxState.CREATING, lockCode)
return SYNC_AGAIN
Expand Down
3 changes: 3 additions & 0 deletions apps/api/src/box/managers/box-actions/box.action.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ export abstract class BoxAction {
errorReason?: string,
daemonVersion?: string,
recoverable?: boolean,
extraUpdateData?: Partial<Box>,
) {
// check if the lock code is still valid
const lockKey = getStateChangeLockKey(box.id)
Expand All @@ -64,6 +65,8 @@ export abstract class BoxAction {
}

const updateData: Partial<Box> = {
// Some state transitions must atomically update related fields such as desiredState.
...extraUpdateData,
Comment on lines 67 to +69

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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-L96
  • apps/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

state,
}

Expand Down
11 changes: 9 additions & 2 deletions apps/api/src/box/runner-adapter/runnerAdapter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,12 +46,19 @@ export interface RunnerAdapter {
runnerInfo(signal?: AbortSignal): Promise<RunnerInfo>

boxInfo(boxId: string): Promise<RunnerBoxInfo>
createBox(box: Box, metadata?: { [key: string]: string }): Promise<StartBoxResponse | undefined>
/**
* Creates a Box. When skipStart=true, it prepares the runtime without starting the main process,
* allowing the remote foreground flow to attach the output stream before explicitly starting it.
*/
createBox(
box: Box,
metadata?: { [key: string]: string },
skipStart?: boolean,
): Promise<StartBoxResponse | undefined>
startBox(
boxId: string,
authToken: string,
metadata?: { [key: string]: string },
skipStart?: boolean,
): Promise<StartBoxResponse | undefined>
stopBox(boxId: string, force?: boolean): Promise<void>
destroyBox(boxId: string): Promise<void>
Expand Down
14 changes: 13 additions & 1 deletion apps/api/src/box/runner-adapter/runnerAdapter.v0.ts
Original file line number Diff line number Diff line change
Expand Up @@ -249,7 +249,11 @@ export class RunnerAdapterV0 implements RunnerAdapter {
}
}

async createBox(box: Box, metadata?: { [key: string]: string }): Promise<StartBoxResponse | undefined> {
async createBox(
box: Box,
metadata?: { [key: string]: string },
skipStart?: boolean,
): Promise<StartBoxResponse | undefined> {
const response = await this.boxApiClient.create({
id: box.id,
image: box.image ?? '',
Expand All @@ -259,12 +263,20 @@ export class RunnerAdapterV0 implements RunnerAdapter {
memoryQuota: box.mem,
storageQuota: box.disk,
env: box.env,
// The synchronous V0 API also needs the full configuration to preserve launch semantics
// during mixed-version deployments.
entrypoint: box.launchConfig?.entrypoint,
cmd: box.launchConfig?.cmd,
workingDir: box.launchConfig?.workingDir,
tty: box.launchConfig?.tty,
detach: box.launchConfig?.detach,
networkBlockAll: box.networkBlockAll,
networkAllowList: box.networkAllowList,
metadata,
authToken: box.authToken,
organizationId: box.organizationId,
regionId: box.region,
skipStart,
})

if (!response?.data?.daemonVersion) {
Expand Down
13 changes: 12 additions & 1 deletion apps/api/src/box/runner-adapter/runnerAdapter.v2.ts
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,11 @@ export class RunnerAdapterV2 implements RunnerAdapter {
}
}

async createBox(box: Box, metadata?: { [key: string]: string }): Promise<StartBoxResponse | undefined> {
async createBox(
box: Box,
metadata?: { [key: string]: string },
skipStart?: boolean,
): Promise<StartBoxResponse | undefined> {
if (!box.image) {
throw new Error(`Box ${box.id} has no image; cannot create on runner`)
}
Expand All @@ -128,6 +132,12 @@ export class RunnerAdapterV2 implements RunnerAdapter {
memoryQuota: box.mem,
storageQuota: box.disk,
env: box.env,
// Store launch configuration in the async job payload so task replay survives Runner restarts.
entrypoint: box.launchConfig?.entrypoint,
cmd: box.launchConfig?.cmd,
workingDir: box.launchConfig?.workingDir,
tty: box.launchConfig?.tty,
detach: box.launchConfig?.detach,
volumes: box.volumes?.map((volume) => ({
volumeId: volume.volumeId,
mountPath: volume.mountPath,
Expand All @@ -137,6 +147,7 @@ export class RunnerAdapterV2 implements RunnerAdapter {
networkAllowList: box.networkAllowList,
metadata,
authToken: box.authToken,
skipStart,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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.

organizationId: box.organizationId,
regionId: box.region,
}
Expand Down
2 changes: 2 additions & 0 deletions apps/api/src/box/services/box.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,8 @@ export class BoxService {
if (createBoxDto.volumes !== undefined) {
box.volumes = this.resolveVolumes(createBoxDto.volumes)
}
// Persist launch parameters with the Box instead of keeping them only in the initial Runner request.
box.launchConfig = createBoxDto.launchConfig

box.runnerId = runner.id
box.pending = true
Expand Down
38 changes: 38 additions & 0 deletions apps/api/src/box/services/job-state-handler.service.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -52,3 +52,41 @@ describe('JobStateHandlerService RESIZE_BOX completion', () => {
})
})
})

describe('JobStateHandlerService CREATE_BOX completion', () => {
it('keeps a foreground skip-start box stopped after a completed V2 create job', async () => {
const box = {
id: 'box-foreground',
state: BoxState.CREATING,
desiredState: BoxDesiredState.STARTED,
}
const boxRepository = {
findOne: jest.fn().mockResolvedValue(box),
update: jest.fn().mockResolvedValue(undefined),
} as any
const redisLockProvider = {
unlock: jest.fn().mockResolvedValue(undefined),
} as any
const service = new JobStateHandlerService(boxRepository, redisLockProvider)
const job = new Job({
id: 'job-create-foreground',
type: JobType.CREATE_BOX,
status: JobStatus.COMPLETED,
runnerId: 'runner-1',
resourceType: ResourceType.BOX,
resourceId: box.id,
payload: JSON.stringify({ skipStart: true }),
})

await service.handleJobCompletion(job)

expect(boxRepository.update).toHaveBeenCalledWith(box.id, {
updateData: {
state: BoxState.STOPPED,
desiredState: BoxDesiredState.STOPPED,
errorReason: null,
},
entity: box,
})
})
})
15 changes: 13 additions & 2 deletions apps/api/src/box/services/job-state-handler.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -102,8 +102,19 @@ export class JobStateHandlerService {
const updateData: Partial<Box> = {}

if (job.status === JobStatus.COMPLETED) {
this.logger.debug(`CREATE_BOX job ${job.id} completed successfully, marking box ${boxId} as STARTED`)
updateData.state = BoxState.STARTED
const payload = job.getPayload<{ skipStart?: boolean }>() ?? {}
// CREATE_BOX completion only means the runtime exists; a skipStart job has not started its main process.
const skippedStart = payload.skipStart === true
this.logger.debug(
`CREATE_BOX job ${job.id} completed successfully, marking box ${boxId} as ${
skippedStart ? 'STOPPED' : 'STARTED'
}`,
)
updateData.state = skippedStart ? BoxState.STOPPED : BoxState.STARTED
if (skippedStart) {
// Prevent the state reconciler from starting the main process before the client attaches its output stream.
updateData.desiredState = BoxDesiredState.STOPPED
}
updateData.errorReason = null
const metadata = job.getResultMetadata()
if (metadata?.daemonVersion && typeof metadata.daemonVersion === 'string') {
Expand Down
Loading