feat: repair a box startup whose job completion was lost - #1091
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughBoxLite now records container start evidence and exposes it through SDKs. The runner uses this evidence during transitional-state synchronization. The API reconciles stalled startup jobs, while ChangesLifecycle reconciliation
Job atomicity
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant BoxLite
participant BoxSyncService
participant BoxService
participant JobService
BoxLite->>BoxSyncService: provide local state and start evidence
BoxSyncService->>BoxService: report transitional or started state
BoxService->>JobService: complete stalled startup job
JobService-->>BoxService: commit job update
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/boxlite/src/runtime/layout.rs (1)
31-37: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueConsider an atomic write (temp file + rename) for the started record.
write()truncates then writes in a separate step, so a reader (the runner'sbox_syncpoll) racing a lifecycle's record replacement could momentarily see an empty/partial file. GivenreadStartedRecordon the Go side treats any parse failure as "no evidence" and just retries next cycle, this self-heals — but a temp-write + rename would remove the window entirely for negligible cost.♻️ Proposed fix
pub fn write(&self, path: &Path) -> std::io::Result<()> { let json = serde_json::to_string(self).expect("two scalar fields are infallible"); - std::fs::write(path, json) + let tmp = path.with_extension("tmp"); + std::fs::write(&tmp, json)?; + std::fs::rename(&tmp, path) }🤖 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/boxlite/src/runtime/layout.rs` around lines 31 - 37, Update StartedRecord::write to write the serialized record to a temporary file in the same directory, then atomically rename it over the target path. Preserve the existing serialization and std::io::Result behavior while ensuring readers never observe a truncated or partially written record.apps/api/src/box/services/box.service.ts (1)
576-596: 🚀 Performance & Scalability | 🔵 TrivialConfirm indexing supports this lookup pattern.
findStalledStartupJobqueries onrunnerId + resourceType + resourceId + type + statusordered bycreatedAt, and can run once per stalled-box sync tick across many runners. If the jobs table doesn't already have a composite index covering these columns, this could become a sequential-scan hot spot as job history grows. Worth a quick check if it isn't already indexed for existing job queries.🤖 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 576 - 596, Verify that the jobs table has a composite index supporting the predicates used by findStalledStartupJob—runnerId, resourceType, resourceId, type, and status—with createdAt included for the descending ordering. If no suitable existing index covers this lookup, add the minimal database/entity index needed and update the migration accordingly.
🤖 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.
Nitpick comments:
In `@apps/api/src/box/services/box.service.ts`:
- Around line 576-596: Verify that the jobs table has a composite index
supporting the predicates used by findStalledStartupJob—runnerId, resourceType,
resourceId, type, and status—with createdAt included for the descending
ordering. If no suitable existing index covers this lookup, add the minimal
database/entity index needed and update the migration accordingly.
In `@src/boxlite/src/runtime/layout.rs`:
- Around line 31-37: Update StartedRecord::write to write the serialized record
to a temporary file in the same directory, then atomically rename it over the
target path. Preserve the existing serialization and std::io::Result behavior
while ensuring readers never observe a truncated or partially written record.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 47e29eea-8029-425c-ab87-15caa3e2e25f
📒 Files selected for processing (14)
apps/api/src/box/services/box.service.spec.tsapps/api/src/box/services/box.service.start-reconciliation.spec.tsapps/api/src/box/services/box.service.tsapps/api/src/box/services/job.service.claim.spec.tsapps/api/src/box/services/job.service.transaction.spec.tsapps/api/src/box/services/job.service.tsapps/api/src/config/configuration.tsapps/runner/pkg/boxlite/client.goapps/runner/pkg/boxlite/create_invariant_test.goapps/runner/pkg/services/box_sync.goapps/runner/pkg/services/box_sync_test.gosrc/boxlite/src/litebox/box_impl.rssrc/boxlite/src/runtime/layout.rssrc/boxlite/tests/container_start_record.rs
📦 BoxLite review — couldn't completepowered by BoxLite |
|
Follow-up stacked on this branch: #1104 — It fixes usage-period drift (the daily roll-over copying stale resources forward, and boxes that never got a period at all). The dependency is branch-level only: it touches six files under |
e2e, simplifiedBefore After Two notes on the framing, from tracing the pre-PR paths:
Neither changes the verdict on the mechanism; they'd just make the problem statement match what main does. |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/boxlite/src/litebox/box_impl.rs (1)
1004-1044: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
record_container_startedcan lose a concurrent state update.
record_container_startedclonesBoxStateunder a write lock, releases the lock, then callssave_boxwith the stale clone. If a concurrent operation (for examplestop(), triggered by a fast health-check failure or explicit cancellation) acquires the lock, mutates state, and persists it first, this function's latersave_boxcall overwrites the database with the older clone. That reverts the concurrent update (status, exit_code, health_status) and pairscontainer_started_atwith a state row a concurrent writer had already superseded — breaking the documented invariant that the timestamp and PID "are written and read as one row" (seestate.rslines 244-248).
stop()andinit_live_state()avoid this by holding the write-lock guard through theirsave_boxcall.record_container_startedshould do the same.🔒 Proposed fix: hold the lock through the save
fn record_container_started(&self) { - let snapshot = { - let mut state = self.state.write(); - state.mark_container_started(); - state.clone() - }; - - if let Err(error) = self - .runtime - .box_manager - .save_box(&self.config.id, &snapshot) - { - tracing::error!( - box_id = %self.config.id, - pid = ?snapshot.pid, - error = %error, - "Container.Start succeeded but the start could not be recorded" - ); - } + let mut state = self.state.write(); + state.mark_container_started(); + + if let Err(error) = self.runtime.box_manager.save_box(&self.config.id, &state) { + tracing::error!( + box_id = %self.config.id, + pid = ?state.pid, + error = %error, + "Container.Start succeeded but the start could not be recorded" + ); + } }🤖 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/boxlite/src/litebox/box_impl.rs` around lines 1004 - 1044, Update record_container_started to retain the state write-lock guard through the save_box call, rather than cloning state and releasing the lock before persistence. Pass the guarded current state directly to save_box while preserving the existing error logging and mark_container_started behavior.
🤖 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.
Outside diff comments:
In `@src/boxlite/src/litebox/box_impl.rs`:
- Around line 1004-1044: Update record_container_started to retain the state
write-lock guard through the save_box call, rather than cloning state and
releasing the lock before persistence. Pass the guarded current state directly
to save_box while preserving the existing error logging and
mark_container_started behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bd5a87d7-e2aa-4d42-a254-dad1ac207244
📒 Files selected for processing (14)
apps/runner/pkg/boxlite/client.goapps/runner/pkg/boxlite/create_invariant_test.goapps/runner/pkg/services/box_sync.goapps/runner/pkg/services/box_sync_test.gosdks/c/include/boxlite.hsdks/c/src/event_queue.rssdks/c/src/info.rssdks/go/info.gosrc/boxlite/src/litebox/box_impl.rssrc/boxlite/src/litebox/state.rssrc/boxlite/src/rest/types.rssrc/boxlite/src/runtime/rt_impl.rssrc/boxlite/src/runtime/types.rssrc/boxlite/tests/container_started_at.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/runner/pkg/boxlite/create_invariant_test.go
ad1314a to
85bafad
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/runner/pkg/boxlite/create_invariant_test.go (1)
37-67: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftTraverse the enclosing block after the exact
bx.Startcall.
containsCallrecursively findsbx.Start, so Line 39 matches the outerif !skipStartstatement. The slice at Line 48 starts after that outerif. It therefore skips any fallible operation or non-nil return added later in the same block.Client.Createcurrently uses this exact nesting inapps/runner/pkg/boxlite/client.goLines 219-317, so the guard does not enforce its stated invariant. Find the enclosing block and exact call position, then inspect post-start control-flow paths while exempting only the return that handlesbx.Startitself. Add a regression case for a post-start failure inside the same block.🤖 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/runner/pkg/boxlite/create_invariant_test.go` around lines 37 - 67, Update the invariant test around the Client.Create AST traversal: locate the exact bx.Start call and its enclosing block rather than matching the outer statement containing it, then inspect all subsequent control-flow paths in that block for non-nil error returns while exempting only bx.Start’s own failure return. Add a regression case covering a fallible operation and failure return after bx.Start within the same block.
🤖 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 `@sdks/c/include/boxlite.h`:
- Around line 319-323: Move the started_at_unix_ms member in the public CBoxInfo
declaration to after the existing network member so all pre-existing member
offsets remain unchanged. Keep the Rust CBoxInfo order consistent with the
updated header, and only introduce an explicit ABI version break if preserving
the layout is impossible.
---
Outside diff comments:
In `@apps/runner/pkg/boxlite/create_invariant_test.go`:
- Around line 37-67: Update the invariant test around the Client.Create AST
traversal: locate the exact bx.Start call and its enclosing block rather than
matching the outer statement containing it, then inspect all subsequent
control-flow paths in that block for non-nil error returns while exempting only
bx.Start’s own failure return. Add a regression case covering a fallible
operation and failure return after bx.Start within the same block.
🪄 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: 103e95fe-a09c-4ee2-a232-874306c76c2c
⛔ Files ignored due to path filters (1)
apps/go.work.sumis excluded by!**/*.sum
📒 Files selected for processing (14)
apps/runner/pkg/boxlite/client.goapps/runner/pkg/boxlite/create_invariant_test.goapps/runner/pkg/services/box_sync.goapps/runner/pkg/services/box_sync_test.gosdks/c/include/boxlite.hsdks/c/src/event_queue.rssdks/c/src/info.rssdks/go/info.gosrc/boxlite/src/litebox/box_impl.rssrc/boxlite/src/litebox/state.rssrc/boxlite/src/rest/types.rssrc/boxlite/src/runtime/rt_impl.rssrc/boxlite/src/runtime/types.rssrc/boxlite/tests/started_at.rs
🚧 Files skipped from review as they are similar to previous changes (7)
- src/boxlite/src/runtime/rt_impl.rs
- src/boxlite/src/rest/types.rs
- apps/runner/pkg/boxlite/client.go
- src/boxlite/src/litebox/box_impl.rs
- sdks/c/src/event_queue.rs
- apps/runner/pkg/services/box_sync.go
- sdks/c/src/info.rs
|
|
Correction to my previous comment — §3 (the
|
Blocker:
|
| file:line | |
|---|---|
src/cli/src/commands/inspect.rs:237 |
confirmed by the compile above |
sdks/python/src/info.rs:374 |
fn core_info(...) -> BoxInfo |
sdks/node/src/info.rs:259 |
fn core_info(...) -> BoxInfo |
All three are #[cfg(test)], which is why a plain cargo build stays green — but make/quality.mk:207,210 runs
cargo clippy --workspace --all-targets --all-features -- -D warnings
and --all-targets compiles test code. The Rust CI jobs had not reported on this head when I checked (only CodeQL and the review bots had), so this is very likely to go red once they do.
Fix is mechanical — started_at: None in each. But greening CI that way leaves the more interesting gap, which is why I'd raise it in the same breath:
PyBoxInfo (sdks/python/src/info.rs:277-303) and JsBoxInfo (sdks/node/src/info.rs:172-213) are separate binding structs, so they compile fine and simply drop the new field — the Python and Node SDKs silently do not expose started_at. Same for src/cli/src/commands/inspect.rs:46-57 InspectStatePresenter, which is deliberately docker-shaped (Status/Running/Pid/ExitCode) — and docker's own State is {Status, Running, Pid, ExitCode, StartedAt, FinishedAt}, so StartedAt is precisely the missing member of the set it is otherwise mirroring. BoxStateInfo (runtime/types.rs:470-482), which both bindings expose as .state, is unextended too.
So the compile error is really flagging three places that each need a decision (expose it, or deliberately don't), not three places that need None pasted in.
|
Two fixes to the comment above. The compile blocker itself is unaffected — I re-ran all three checks and they reproduce.
|
85bafad to
e0469a6
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds a durable “container actually started” timestamp (started_at) to BoxLite’s BoxInfo and wires it through the runner sync + API so a box stuck in CREATING/STARTING (due to a lost job-completion callback) can be safely reconciled to STARTED. It also hardens job claiming/status updates to prevent concurrent writers from racing on the same Job row.
Changes:
- Add and persist
started_atas evidence thatContainer.Startsucceeded for the current lifecycle (and expose it across CLI + SDKs). - Extend runner
BoxSyncto fetch transitional boxes and only reportSTARTEDwhen local state isSTARTEDandstarted_atis present; add tests and an invariant guard aroundClient.Create. - Make API Job updates transactional with row locks and claim pending jobs via conditional UPDATE; allow API-side reconciliation for stalled startup jobs.
Reviewed changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/cli/src/commands/inspect.rs | Updates CLI inspect test fixture to include started_at. |
| src/boxlite/tests/started_at.rs | New integration tests asserting started_at lifecycle semantics. |
| src/boxlite/src/runtime/types.rs | Adds started_at to BoxInfo and wires it from state. |
| src/boxlite/src/runtime/rt_impl.rs | Uses adopt_recovered_shim to keep PID/started_at consistent on recovery. |
| src/boxlite/src/rest/types.rs | Ensures REST conversion sets started_at to None (not known remotely). |
| src/boxlite/src/litebox/state.rs | Adds started_at to persisted BoxState plus invariants and unit tests. |
| src/boxlite/src/litebox/box_impl.rs | Records started_at after successful Container.Start and guards invariants. |
| sdks/python/src/info.rs | Updates Python SDK test fixture to include started_at. |
| sdks/node/src/info.rs | Updates Node SDK test fixture to include started_at. |
| sdks/go/info.go | Adds StartedAt to Go SDK and converts from C milliseconds. |
| sdks/c/tests/test_info.c | Extends C test expectations + ABI layout assert for started_at. |
| sdks/c/src/info.rs | Adds started_at to C FFI struct and populates it. |
| sdks/c/src/event_queue.rs | Updates FFI leak tests to include new fields. |
| sdks/c/include/boxlite.h | Exposes started_at in the public C header. |
| apps/runner/pkg/services/box_sync.go | Reads started_at from ListInfo, fetches transitional boxes, gates reporting. |
| apps/runner/pkg/services/box_sync_test.go | New tests for started-at conversion and transitional reconciliation behavior. |
| apps/runner/pkg/boxlite/create_invariant_test.go | New AST-based guard ensuring no fallible steps after bx.Start in Create. |
| apps/runner/pkg/boxlite/client.go | Documents invariant and exports ToBoxState for single-snapshot reads. |
| apps/runner/pkg/boxlite/box_state_test.go | New unit tests for ToBoxState mapping behavior. |
| apps/go.work.sum | Updates Go workspace sums for new/updated dependencies. |
| apps/api/src/config/configuration.ts | Adds BOX_SYNC_START_CONFIRMATION_STALL_SECONDS configuration. |
| apps/api/src/box/services/job.service.ts | Transactional updateJobStatus + CAS-style claimPendingJobs. |
| apps/api/src/box/services/job.service.transaction.spec.ts | New tests asserting transaction boundary + lock request. |
| apps/api/src/box/services/job.service.claim.spec.ts | New tests asserting conditional claim semantics + error propagation. |
| apps/api/src/box/services/box.service.ts | Allows STARTED reconciliation for stalled startup jobs; transitional state filtering fix. |
| apps/api/src/box/services/box.service.start-reconciliation.spec.ts | New tests for reconciliation and state-filter validation behavior. |
| apps/api/src/box/services/box.service.spec.ts | Updates service construction for new dependencies. |
Suppressed comments (1)
apps/runner/pkg/services/box_sync.go:30
- BoxSyncServiceConfig still hard-codes
Boxliteas*blclient.Client, but the service now depends only on theboxStateReaderinterface. Keeping the concrete type here prevents injecting a stub via the normal constructor path, undermining the stated goal of exercising the sync loop without a live runtime.
type BoxSyncServiceConfig struct {
Logger *slog.Logger
Boxlite *blclient.Client
Interval time.Duration
}
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/runner/pkg/boxlite/create_invariant_test.go`:
- Around line 147-198: Update fallibleReturnsAfterStart to validate the start
guard’s condition as well as its Init: require a != comparison against nil using
the same error identifier assigned by bx.Start, and treat any mismatch as the
existing malformed-shape error. Add a TestFallibleReturnsAfterStart case with an
err == nil guard and a fallible step in its body, verifying the mismatch is
reported rather than exempting that body.
In `@sdks/node/src/info.rs`:
- Line 276: Expose the BoxInfo.started_at value through JsBoxInfo::from in
sdks/node/src/info.rs:276-276 and PyBoxInfo::from in
sdks/python/src/info.rs:391-391, adding tests that verify nonzero timestamp
conversion; update InspectPresenter::from in
src/cli/src/commands/inspect.rs:254-254 to include started_at in inspect output,
or explicitly document its intentional omission.
In `@src/boxlite/src/litebox/state.rs`:
- Around line 235-257: Add the missing started_at field to the BoxInfo literal
in the CLI inspect command, initializing it to None, or use BoxInfo::new(...) if
that constructor produces the same intended value. Ensure the public BoxInfo
construction compiles while preserving the existing test behavior.
🪄 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: 99cbe6c8-afe4-4a20-b52d-42ee3c0348ac
⛔ Files ignored due to path filters (1)
apps/go.work.sumis excluded by!**/*.sum
📒 Files selected for processing (19)
apps/runner/pkg/boxlite/box_state_test.goapps/runner/pkg/boxlite/client.goapps/runner/pkg/boxlite/create_invariant_test.goapps/runner/pkg/services/box_sync.goapps/runner/pkg/services/box_sync_test.gosdks/c/include/boxlite.hsdks/c/src/event_queue.rssdks/c/src/info.rssdks/c/tests/test_info.csdks/go/info.gosdks/node/src/info.rssdks/python/src/info.rssrc/boxlite/src/litebox/box_impl.rssrc/boxlite/src/litebox/state.rssrc/boxlite/src/rest/types.rssrc/boxlite/src/runtime/rt_impl.rssrc/boxlite/src/runtime/types.rssrc/boxlite/tests/started_at.rssrc/cli/src/commands/inspect.rs
🚧 Files skipped from review as they are similar to previous changes (8)
- src/boxlite/src/rest/types.rs
- sdks/c/src/event_queue.rs
- src/boxlite/src/litebox/box_impl.rs
- src/boxlite/src/runtime/rt_impl.rs
- src/boxlite/src/runtime/types.rs
- src/boxlite/tests/started_at.rs
- sdks/go/info.go
- apps/runner/pkg/services/box_sync.go
e0469a6 to
d6825b4
Compare
035a13b to
f983773
Compare
f983773 to
2227901
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 31 out of 31 changed files in this pull request and generated no new comments.
Suppressed comments (1)
apps/runner/pkg/services/box_sync.go:233
canReportcurrently blocks all state updates when the API box is CREATING/STARTING unless the local state is STARTED andstartedAtis present. That means if the local box is actually STOPPED/ERROR (or any non-STARTED state), the runner will skip reconciliation and the API can remain stuck in a transitional state indefinitely.
The gating should only apply when the runner is about to report STARTED for a transitional remote state; other local states should still be reported normally.
func (s *BoxSyncService) canReport(local localContainerState, remoteState apiclient.BoxState) bool {
switch remoteState {
case apiclient.BOXSTATE_CREATING, apiclient.BOXSTATE_STARTING:
return local.state == enums.BoxStateStarted && local.startedAt != nil
default:
return true
}
DorianZheng
left a comment
DorianZheng
left a comment
There was a problem hiding this comment.
Five findings from an adversarial review pass, verified against this branch. Ordered by severity: the first is a hard blocker on the normal polling path.
A sixth finding (appending started_at to CBoxInfo changes sizeof for old C list clients) was checked and dropped — no SONAME/dylib versioning, the Go SDK links statically against a version-pinned header+lib pair, and #1009/#1031 set the same precedent already. Not raising it.
2227901 to
a98687e
Compare
Two paths mutated a Job with an unguarded read-modify-write, both
relying on protection that TypeORM does not provide: `save()` never
checks the @VersionColumn, because optimistic locking is only enforced
when a row is read with `lock: {mode: 'optimistic'}`, and its UPDATE
predicate is the id alone.
updateJobStatus read the job, validated the transition, and saved it back
over three separate statements. Two writers could both read the same
status, both judge their transition valid, and the second save would
overwrite the first with no conflict raised. It now runs in one
transaction holding a pessimistic_write lock on the row, so a concurrent
update blocks and then re-reads the committed status. The completion
handler still runs after the transaction commits, keeping its side
effects outside the lock.
claimPendingJobs had the same shape and a comment asserting the version
column protected it. Two overlapping polls could therefore claim one
PENDING job and hand the same work out twice, which also moves the
`startedAt` clock that stale-job handling reads. The selected candidates
are now claimed in one conditional UPDATE with both `id IN (...)` and
`status = PENDING` predicates. Each row can be won by only one poller,
while a database error rolls back the entire statement instead of leaving
an earlier subset committed. `RETURNING id` identifies the rows won by
this poll, and real database errors propagate rather than being treated as
lost races.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A box whose CREATE_BOX or START_BOX job finished on the runner but whose
completion callback never reached the API stays in CREATING or STARTING
forever, even though it is running. Nothing reconciled it: BoxSync only
fetched boxes the control plane already believed were STARTED, and
updateState refused to touch a box in a transitional state.
Extend the existing BoxSync loop to cover it, using local evidence that
the box's init was actually launched.
BoxLite now writes {box_dir}/started after the guest's Container.Start
returns success, and clears it when the next lifecycle boots. This is the
positive counterpart of the shim's exit file and follows the same
per-lifecycle discipline: one write point, one removal point. The record
names the shim it belongs to, so a reader pairs it with the box's live
PID and cannot mistake a leftover for current evidence. BoxStatus::Running
alone could not serve here — booting publishes Running before the separate
Container.Start RPC runs.
The runner reads that file directly, so nothing new crosses the C ABI.
BoxSync asks for transitional boxes through the existing for-runner
endpoint and reports its local view through the existing state endpoint;
no new API surface, and the generated client is unchanged. The
transitional query degrades to "no candidates this cycle" when the API
rejects it, so an older API cannot cost us the STARTED reconciliation
that has worked on its own for far longer.
The API decides when to believe the runner, because the API owns the job:
it completes the startup job only once that job has been claimed and has
then stopped making progress for BOX_SYNC_START_CONFIRMATION_STALL_SECONDS
(60s by default). Completing the job is what moves the box, so the
existing completion handler keeps ownership of the STARTED transition, the
pending flag, the activity stamp, the state event, and the Redis unlock.
Before the job stalls, a runner reporting STARTED is answered with a
silent no-op rather than a 400 — it is telling the truth, just early.
Container.Start's success is only usable as evidence of a successful job
body while bx.Start stays the last fallible step of Client.Create.
Nothing enforced that, so TestCreateHasNoFallibleStepAfterStart asserts
it against the source.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Keep recoverable lifecycle evidence consistent across state persistence, SDKs, and CLI output. Expose started_at through public metadata APIs and cover recovery, transitions, and conversions with regression tests.
a98687e to
5fa3c43
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 32 out of 32 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
apps/runner/pkg/services/box_sync.go:69
- GetLocalContainerStates uses
box.Nameas the primary map key, but remoteStates is keyed by APIbox.Idand SyncBoxState passes this key as the{boxId}path parameter. If a box has a user-assigned name that differs from its id, reconciliation will silently skip it (no remote match) and/or attempt to update by name instead of id.
boxId := box.Name
if boxId == "" {
boxId = box.ID
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 32 out of 32 changed files in this pull request and generated no new comments.
Suppressed comments (2)
apps/api/src/box/services/job.service.ts:492
Repository.update()is being called with a third argument ({ returning: [...] }), but TypeORM’sRepository.updatesignature only accepts(criteria, partialEntity). This will fail type-checking/compilation and also won’t reliably give yourawrows. Use a query builder with.returning()(Postgres) to atomically claim and collect the claimed IDs.
},
{ returning: ['id'] },
)
sdks/c/include/boxlite.h:325
- The C header comment includes Rustdoc-style markup (
[`Self::pid`]), which is confusing/noisy in generated C docs. Use plain field naming (e.g.,pid) in C-facing comments.
// Unix milliseconds of the most recently recorded successful guest
// `Container.Start`; `0` when none was recorded. Preserved after stop or
// reboot; when [`Self::pid`] is nonzero, the timestamp describes that live
// PID. Milliseconds — not `created_at`'s seconds — preserve sub-second
// ordering against a job's timeline.
> **Stacked on #1091.** Base branch is `fix/usage_failure_lt`, not `main`, so the > diff here is only the six usage files. **Merge #1091 first**; GitHub will then > retarget this PR to `main` on its own. Reviewing it against `main` before that > shows #1091's fourteen files as well. > > The dependency is branch-level, not code-level: nothing here calls anything > #1091 adds, and the two touch disjoint files. It is stacked because it was > written on top of that branch, so rebasing it onto `main` today would be a > clean no-op — it just has not been done, to keep the diff reviewable. ## Problem A box's open usage period is maintained by in-process events, which are fire-and-forget. A handler that throws, or whose process dies mid-transition, leaves the box and its period disagreeing and nothing notices. Two gaps let that disagreement persist indefinitely: **The daily roll-over preserved drift instead of correcting it.** It copied the closing period's resources into the replacement, so a period charging no cpu for a running box was re-copied every day, forever. A disk resize that landed while the box was stopped never reached the ledger at all. **Nothing ever scanned from the box side.** The roll-over walks the *period* table, so a box that never got a period is invisible to it — there is no row to find. Its one-day cutoff also lets a wrong period bill for a further day. ## Approach `expectedOpenPeriod(box)` becomes the single source of truth for the state → period rule — full compute while running, disk alone once stopped, nothing otherwise — and the event handler, the roll-over and the new reconcile pass all answer the question the same way. **Roll-over** re-derives resources from the box rather than copying them forward. A box that is gone or terminal yields no shape and so is not reopened, which is what stops a deleted box from accruing. **Reconcile pass** (every 5 min) scans from the box side, which is why both are needed — their blind spots are opposite. The roll-over is the only thing that can see a period whose box row was deleted outright; this is the only thing that can see a box that never got a period. - Sharded by runner so each scan rides `box_runnerid_idx` rather than reading the box table end to end. Measured on 20k boxes across 50 runners: bitmap index scan, ~400 rows rechecked, ~1.7 ms. The trailing `runnerId IS NULL` shard is not optional — a box that reached DESTROYED or ARCHIVED has had its runnerId cleared, and those are exactly the periods that must be closed. - Two-minute grace window before a box is eligible. The per-box lock's TTL is 60 s, so a handler can legitimately take a full minute to reach the ledger; reconciling inside that window would race it and collide on the one-open-period index. Anything under 60 s is provably too short. - Each candidate is re-checked under the per-box lock against `expectedOpenPeriod`, so one the event handler fixed in the meantime is left alone. The SQL is a deliberately wide filter, not the authority. - Repairs are counted on `usage_period_drift_repaired{kind=missing|orphan|stale_shape}`. ## Decisions worth challenging - **Billing deliberately diverges from quota.** `BOX_STATES_CONSUMING_COMPUTE` counts CREATING and STARTING because the runner has already pinned the resources; billing does not charge for a box the tenant cannot use yet. Divergence here is a pricing decision, not a bug — `expected-usage-period.spec.ts` asserts it so nobody "fixes" it by accident. - **Corrections start now and are never backdated.** The window a box spent mis-billed cannot be reconstructed (its `updatedAt` has moved on for unrelated reasons), and guessing it would replace a known gap with an invented charge. - **The resource comparison is qualified by state.** A bare `p.cpu <> b.cpu` is permanently true for every stopped box — a stopped period *should* charge no cpu — and those false positives would fill each page and starve real drift out of the batch forever. - **A float tolerance is required, not cosmetic.** cpu/gpu/mem/disk are double precision on both sides; without `RESOURCE_EPSILON` the pass would rewrite an already-correct period on every run, fragmenting the ledger into unbillable slivers. - ARCHIVING bills nothing because no code in this repository assigns it. Pricing a state the product does not have yet would be inventing a rule. No database migration, no API surface change, no client regeneration. ## Verification Ran against a real Postgres 16 + Redis (`DB_*`/`REDIS_*` pointed at a disposable database; the integration spec builds its schema by running the migrations, so it exercises the DDL that ships, and skips when no database is reachable). | | Result | |---|---| | usage suites (unit + integration) | 3 suites / **82 tests** pass | | full api suite | 58 suites / **321 tests** pass | | `tsc -p api/tsconfig.spec.json --noEmit` | exit 0 | | `eslint api/src/usage --max-warnings=0` | clean | | `prettier --check` | clean | **Two-side verified.** With the roll-over hunk reverted and everything else intact, three roll-over assertions fail on the bug itself — a running box's rolled-over period comes back `cpu: 0, gpu: 0, mem: 0` instead of `2/1/4`: ``` ● the daily roll-over on its own › carries the resources the box has now, not the ones the closing period held - ObjectContaining { "cpu": 2, "gpu": 1, "mem": 4, } + BoxUsagePeriod { "cpu": 0, "gpu": 0, "mem": 0, ... } ``` Restoring the hunk turns all three green. A full revert of every production file is red too, but only as a compile error, since the reconcile pass is new surface its specs cannot load without — so the roll-over hunk is the isolation that carries real signal. Stated plainly: the reconcile tests demonstrate new behaviour, they do not reproduce a prior bug. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved usage billing across box states, including compute, disk-only, inactive, and terminal states. * Added automatic reconciliation to repair missing, orphaned, or outdated usage periods. * Updated rollover processing to reflect current resource usage and avoid terminal boxes. * Added tolerance for minor resource-value differences when validating usage periods. * **Tests** * Expanded coverage for billing rules, reconciliation, rollover behavior, resource drift, and idempotency. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Summary
Repair boxes left in
CREATINGorSTARTINGwhen the runner starts the container successfully but its job-completion callback never reaches the API.Persist lifecycle-scoped
Container.Startevidence in BoxLite, use it to reconcile only the matching stalled startup job through the normal completion path, and prevent concurrent workers from claiming or finalizing the same job row from stale state.Call graph
Startup recovery
Before (
origin/mainat48b49977)After (
035a13b2)Job-row concurrency
Before
After
Fixes #
Changes
started_atafter a successful guestContainer.Startin the same BoxLite state row as lifecycle state and PID.started_atthrough RustBoxInfo, the C ABI, Go, Node.js, Python, and CLI inspection output.ListInfosnapshot.CREATINGandSTARTINGboxes as reconciliation candidates without disrupting the existingSTARTEDreconciliation path.STARTEDonly when BoxLite reports both localSTARTEDstate and durable start evidence.CREATE_BOXorSTART_BOXjob through the existing completion handler.UPDATE … WHERE status = PENDING RETURNING *.How to verify
Risks / rollout
No database migration is required.
started_atis optional and serde-defaulted in the existing BoxLite state row; legacy rows therefore load without evidence until a later successful start.CBoxInfogains a tail field. Existing field offsets remain unchanged, but native libraries and generated SDK bindings should be rebuilt and released together.No strict API/runner deployment order is required:
STARTEDreconciliation remains; lost-callback confirmation is skippedThe default 60-second stall window delays recovery intentionally so it does not race a completion callback that is still progressing. It is configurable through
BOX_SYNC_START_CONFIRMATION_STALL_SECONDS.Any failure of the additional transitional-box query is treated as “confirmation unavailable this cycle.” This preserves ordinary reconciliation but delays startup recovery until a later successful cycle.
Recording
started_atholds the BoxLite lifecycle-state lock while synchronously saving to SQLite, so database contention can delay other state readers.The PostgreSQL conditional-claim and pessimistic-lock paths are covered by mock/stub unit tests in this PR, not by a real PostgreSQL integration test.