feat(go): add box archive import and export - #1162
Conversation
📝 WalkthroughWalkthroughThe PR adds asynchronous archive export and import to the Go SDK through new C and Rust FFI APIs. It adds callback ownership handling, cancellation and shutdown behavior, documentation, unit tests, and a VM-backed round-trip integration test. ChangesGo archive SDK
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
There was a problem hiding this comment.
Pull request overview
This PR exposes BoxLite’s existing box archive export/import functionality through the Go SDK, wiring Go-facing Box.Export / Runtime.Import to new Go-private C/FFI bridge entry points, and adding unit + opt-in real-VM integration coverage. It also coordinates runtime teardown with in-flight archive operations so Go callers don’t lose callbacks when Runtime.Close / Runtime.Shutdown runs.
Changes:
- Add Go SDK APIs:
(*Box).Exportand(*Runtime).Import, including argument validation and “check ctx before submission; always await committed callback” semantics. - Add Go-private bridge surface (
sdks/go/bridge.h, callback accessors, Go callback dispatch) and Rust C SDK support for string-returning export events + import handle events. - Add targeted Go unit tests and an opt-in VM-backed integration round-trip test, plus make targets for running that integration test.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| sdks/go/runtime.go | Adds an archive operation gate and integrates it into Close/Shutdown sequencing. |
| sdks/go/README.md | Documents archive export/import usage and archive lifetime semantics. |
| sdks/go/integration/archive/archive_test.go | Adds a VM-backed round-trip archive integration test (opt-in build tags). |
| sdks/go/bridge.h | Declares Go-private C bridge callbacks + export/import entry points. |
| sdks/go/bridge.c | Switches to bridge.h and adds callback accessor functions for Go archive callbacks. |
| sdks/go/bridge_callback.go | Adds Go dispatch callbacks for export (string payload) and import (box handle payload). |
| sdks/go/archive.go | Introduces Box.Export and Runtime.Import implementations using the new bridge functions. |
| sdks/go/archive_test.go | Adds unit tests for validation, cancellation behavior, and teardown waiting semantics. |
| sdks/c/src/runtime.rs | Adds string-event dispatch helper and tests ownership transfer for export/import events. |
| sdks/c/src/lib.rs | Registers the new archive module in the C SDK crate. |
| sdks/c/src/event_queue.rs | Adds new runtime event variants for Go export/import completions. |
| sdks/c/src/archive.rs | Implements the Go-private C symbols to async-export and import archives and post results via the event queue. |
| sdks/c/cbindgen.toml | Excludes Go-private bridge types/symbols from the public generated C header. |
| make/test.mk | Adds test:integration:go target for the archive round-trip test. |
| make/help.mk | Documents the new Go integration test make target. |
Suppressed comments (1)
sdks/go/archive.go:130
- Import returns ErrRuntimeClosed when the archive gate is closed. Because Runtime.Shutdown closes the same gate (sdks/go/runtime.go:154), this can yield ErrRuntimeClosed after Shutdown, which is inconsistent with other operations that report ErrStopped for a shut down runtime. Consider returning an ErrStopped-coded *Error (or letting the native layer return ErrStopped) when the gate is closed due to shutdown, reserving ErrRuntimeClosed for Runtime.Close races.
if !r.archiveOperations.tryAcquire() {
return nil, ErrRuntimeClosed
}
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sdks/go/runtime.go (1)
135-150: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Closecan now double-free the native runtime under concurrent calls.
Closechecksr.handle == nil, then clearsr.handleat the end. No lock protects that read-modify-write. Two concurrentClosecalls can both pass the nil check and both callC.boxlite_runtime_free(r.handle).The new
closeAndWaitcall makes this reachable in practice.Closenow blocks for the full duration of an in-flight archive operation. A secondCloseon another goroutine enters the same window and blocks on the same gate. When the archive operation finishes,closeAndWaitreturns on both goroutines and both proceed to free the handle. Before this change,Closedid not block, so the window was very small.Guard the teardown with a
sync.Once, or take a mutex around the handle check and the free.🔒️ Proposed fix
type Runtime struct { handle *C.CBoxliteRuntime + closeOnce sync.Once drainOnce sync.Oncefunc (r *Runtime) Close() error { - if r.handle == nil { - return nil - } - - r.archiveOperations.closeAndWait() - r.closingOnce.Do(func() { - if r.closing != nil { - close(r.closing) - } - }) - r.stopDrain() - C.boxlite_runtime_free(r.handle) - r.handle = nil + r.closeOnce.Do(func() { + if r.handle == nil { + return + } + r.archiveOperations.closeAndWait() + r.closingOnce.Do(func() { + if r.closing != nil { + close(r.closing) + } + }) + r.stopDrain() + C.boxlite_runtime_free(r.handle) + r.handle = nil + }) return nil }🤖 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 `@sdks/go/runtime.go` around lines 135 - 150, Make Runtime.Close concurrency-safe by guarding the handle nil check and native teardown with a sync.Once or mutex. Ensure only one caller executes archive shutdown, stopDrain, C.boxlite_runtime_free, and sets r.handle to nil, while concurrent callers return without repeating the free.
🧹 Nitpick comments (10)
sdks/go/bridge_callback.go (1)
113-151: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winThe new callbacks free payloads more thoroughly than the existing ones.
goBoxliteOnExportBoxandgoBoxliteOnImportBoxrelease the payload on theh == 0path and on the channel type-mismatch path.goBoxliteOnCreateBoxat Line 97 andgoBoxliteOnGetBoxat Line 183 return on those same two paths without freeing, so they leak aCBoxHandle.The new code is correct. The older callbacks should adopt the same pattern in a follow-up change.
🤖 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 `@sdks/go/bridge_callback.go` around lines 113 - 151, Update goBoxliteOnCreateBox and goBoxliteOnGetBox to free their CBoxHandle payloads on both the h == 0 path and the channel type-mismatch path, matching the cleanup behavior in goBoxliteOnImportBox. Use the existing freeBoxHandlePayload helper before each early return while preserving the current callback flow.sdks/c/src/archive.rs (2)
256-256: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument why the dangling pointers are safe in these tests.
Each test passes a dangling
BoxHandleorRuntimeHandlepointer. The tests pass only because every exercised path returns beforeexport_boxandimport_boxreach&*handleand&*runtime. That coupling is invisible at the call site. If a later change moves the dereference above the argument checks, these tests trigger undefined behavior instead of failing an assertion.Add a short comment that records the invariant.
♻️ Suggested comment
fn export_rejects_invalid_arguments_synchronously() { let dest = CString::new("/tmp/export.boxlite").unwrap(); + // Safe only because every case below returns during argument + // validation, before `export_box` dereferences the handle. let dangling_handle = ptr::NonNull::<BoxHandle>::dangling().as_ptr();Also applies to: 296-296, 339-340
🤖 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 `@sdks/c/src/archive.rs` at line 256, Add a short safety comment beside the dangling BoxHandle and RuntimeHandle test pointers, including the instances around dangling_handle, documenting that exercised export_box/import_box paths validate arguments and return before dereferencing them; preserve the existing test setup.
24-24: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd test coverage for the C SDK bridge signatures.
sdks/cusesedition.workspace = true("2024") andrust-version = "1.88", so#[unsafe(no_mangle)]is supported;BoxliteErrorCodeis#[repr(C)]with explicit discriminants, matching the manualenum BoxliteErrorCodeheader return type. The remaining risk is thatsdks/go/bridge.hdeclares only thecboxbridge.cFFI symbols, whilecbindgen.tomlexcludes these functions from generated headers. Add a signature-invariant check so future parameter/return-type changes cannot drift across the C/Go boundary.🤖 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 `@sdks/c/src/archive.rs` at line 24, Add a signature-invariant test for the C SDK bridge, covering the FFI symbols declared in sdks/go/bridge.h and implemented by cboxbridge.c, including parameter and return types. Keep the check aligned with the manually maintained header and excluded cbindgen functions so future Rust bridge signature changes cannot drift across the C/Go boundary.sdks/c/src/runtime.rs (1)
946-970: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider covering the
RuntimeEvent::GoImportBoxarm end to end.
go_import_dispatch_transfers_handle_ownership_to_callbackcallsdispatch_handle_eventdirectly with a stand-in type. It proves generic ownership transfer. It does not prove that the new match arm at Line 560 routesGoImportBoxto the handle dispatcher. A small test that constructs the event and callsdispatch_eventwould close that gap.🤖 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 `@sdks/c/src/runtime.rs` around lines 946 - 970, Add an end-to-end test for the RuntimeEvent::GoImportBox branch that constructs the event with an owned import handle and invokes dispatch_event, using the existing consume_go_import_handle callback and drop counter to verify routing and ownership transfer. Keep the generic dispatch_handle_event test unchanged.sdks/go/archive.go (1)
92-92: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider documenting the unbounded wait on the native callback.
completed := <-resulthas no escape path. If the native bridge never queues the completion event,ExportorImportblocks forever. Because the archive gate holdsCloseandShutdown, a lost callback also blocks runtime teardown forever. The current design is intentional and keeps the drain loop alive, so no change is required now. Add a short note in the code that a lost native event blocks teardown, so future maintainers understand the coupling.Also applies to: 162-162
🤖 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 `@sdks/go/archive.go` at line 92, Add a concise comment beside the blocking receives in Export and Import explaining that a lost native completion event causes the operation and runtime teardown to wait indefinitely because the archive gate remains held. Preserve the existing unbounded wait and drain-loop behavior.make/test.mk (1)
351-356: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign the dependency style, and reconsider the
test:all:goname.Two points on this target:
The prerequisite style differs from every neighboring target.
test:unit:goandtest:integration:cinvoke@$(MAKE) dev:goand@$(MAKE) dev:cinside the recipe.test:integration:rustandtest:integration:cliuse the$(if $(SETUP_DONE),,...)guard so a parent target does not rebuild dependencies. This target uses a plaindev\:goprerequisite, which skips theSETUP_DONEguard. Pick one of the two existing styles.
test:all:goat line 359 still runs onlytest:unit:go. The name now understates the suite, because a second Go suite exists. Keeping the integration test out of the default matrices is intentional, per the comment at lines 351-352. Consider renaming the help text fortest:all:go, or state in its comment that the archive integration test is excluded on purpose.♻️ Proposed change for point 1
# Go SDK archive round-trip integration test. Intentionally excluded from # default test matrices and CI. -test\:integration\:go: dev\:go +test\:integration\:go: `@echo` "🧪 Running Go SDK archive integration test (requires VM)..." + @$(MAKE) dev:go `@cd` sdks/go && go test -count=1 -tags=boxlite_dev,boxlite_integration -v $(GOTEST_FILTER) ./integration/archive🤖 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 `@make/test.mk` around lines 351 - 356, Align test:integration:go with neighboring targets by using either the in-recipe @$(MAKE) dev:go dependency style or the existing SETUP_DONE guard, rather than a plain prerequisite. Also clarify test:all:go’s scope by updating its help text or comment to explicitly state that it runs only the unit suite and intentionally excludes test:integration:go.sdks/go/archive_test.go (1)
154-254: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSplit the duplicated preparation-cancellation subtests into one shared helper.
This function spans 101 lines, which exceeds the 100-line limit for files under
sdks/. TheImportandExportsubtests also duplicate the same sequence: build a server, create the REST runtime, lock the archive gate, sample the context, cancel, unlock, and assert. Extract a shared helper that takes the server constructor and the operation closure, in the same style astestArchiveTeardownWaits. This removes the duplication and brings the function under the limit.As per path instructions: "Limit code blocks to a maximum of 100 lines" and "Keep functions focused and cohesive - each function should do one thing well".
🤖 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 `@sdks/go/archive_test.go` around lines 154 - 254, Split TestArchiveCancellationDuringPreparationPreventsNativeSubmission into a shared helper, following the pattern of testArchiveTeardownWaits. Have the helper accept the server-construction callback and archive-operation callback, centralize runtime setup, archive gate locking, context sampling/cancellation, unlocking, result validation, and assertArchiveRequestNotSubmitted, then reduce the Import and Export subtests to configure and invoke it while preserving their existing operation-specific setup and cleanup.Source: Path instructions
sdks/go/runtime.go (1)
55-67: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider creating
idleeagerly instead of lazily.The lazy creation in
closeAndWaitis correct, becausereleaseandcloseAndWaitboth holdmu, andcloseAndWaitre-readsg.activeafter it creates the cond. The correctness therefore depends on a non-obvious ordering argument, and theg.idle != nilguard inreleaselooks like a missed wakeup at first reading. Add acond()accessor that creates the cond undermu, and use it in both methods. This keeps the zero-valuearchiveOperationGateusable while removing the nil guard.♻️ Proposed refactor
+// cond returns the idle condition variable. The caller must hold g.mu. +// The gate is usable as a zero value, so the cond is created on first use. +func (g *archiveOperationGate) cond() *sync.Cond { + if g.idle == nil { + g.idle = sync.NewCond(&g.mu) + } + return g.idle +} + func (g *archiveOperationGate) release() { g.mu.Lock() defer g.mu.Unlock() if g.active == 0 { panic("boxlite: archive operation gate released without an active operation") } g.active-- - if g.active == 0 && g.idle != nil { - g.idle.Broadcast() + if g.active == 0 { + g.cond().Broadcast() } } func (g *archiveOperationGate) closeAndWait() { g.mu.Lock() defer g.mu.Unlock() g.closed = true - if g.idle == nil { - g.idle = sync.NewCond(&g.mu) - } for g.active != 0 { - g.idle.Wait() + g.cond().Wait() } }🤖 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 `@sdks/go/runtime.go` around lines 55 - 67, Refactor archiveOperationGate by adding a cond() accessor that initializes and returns idle while holding mu, then use it in both release and closeAndWait. Remove the idle nil guard in release while preserving zero-value usability and the existing close-and-wait synchronization behavior.sdks/go/integration/archive/archive_test.go (2)
18-199: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSplit this test into phase helpers.
TestIntegrationArchiveRoundTripspans 182 lines, which exceeds the 100-line limit for files undersdks/. The body also performs several distinct jobs: create and seed the source box, export and validate the archive, import and validate box identity and state, and verify the restored box after archive deletion. Extract each phase into a named helper, for exampleexportAndValidateArchive,assertRestoredIdentity, andassertRestoredBoxUsable. The test then reads as a sequence of phases and stays under the limit.As per path instructions: "Limit code blocks to a maximum of 100 lines" and "Keep functions focused and cohesive - each function should do one thing well".
🤖 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 `@sdks/go/integration/archive/archive_test.go` around lines 18 - 199, The TestIntegrationArchiveRoundTrip function is too long and combines multiple responsibilities. Extract cohesive phase helpers for creating/seeding the source box, exporting and validating the archive, validating restored identity/state, and verifying restored usability after archive deletion; keep TestIntegrationArchiveRoundTrip as a short sequence invoking those helpers while preserving shared cleanup and assertions.Source: Path instructions
201-209: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove
ctxto the first parameter ofassertExec.Go convention and common linters expect
ctx context.Contextfirst.assertExeccurrently putsctxthird, aftertandbox, and there are four call sites that also need to passctxfirst.🤖 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 `@sdks/go/integration/archive/archive_test.go` around lines 201 - 209, Update the assertExec function signature to place ctx context.Context immediately after t, before box and the remaining arguments. Adjust all four assertExec call sites to pass ctx as the first argument after t, preserving the existing operation, output, command, and args values.
🤖 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/go/runtime.go`:
- Around line 153-154: Decide and enforce the intended Shutdown contract in
Runtime.Shutdown: if shutdown is terminal, document that explicitly; otherwise
replace archiveOperations.closeAndWait with a non-terminal idle-drain mechanism
such as waitIdle, or restore admission on every native-submission and
context-cancellation error path before returning. Ensure failed or canceled
Shutdown calls leave Export and Import usable when the runtime handle remains
valid.
---
Outside diff comments:
In `@sdks/go/runtime.go`:
- Around line 135-150: Make Runtime.Close concurrency-safe by guarding the
handle nil check and native teardown with a sync.Once or mutex. Ensure only one
caller executes archive shutdown, stopDrain, C.boxlite_runtime_free, and sets
r.handle to nil, while concurrent callers return without repeating the free.
---
Nitpick comments:
In `@make/test.mk`:
- Around line 351-356: Align test:integration:go with neighboring targets by
using either the in-recipe @$(MAKE) dev:go dependency style or the existing
SETUP_DONE guard, rather than a plain prerequisite. Also clarify test:all:go’s
scope by updating its help text or comment to explicitly state that it runs only
the unit suite and intentionally excludes test:integration:go.
In `@sdks/c/src/archive.rs`:
- Line 256: Add a short safety comment beside the dangling BoxHandle and
RuntimeHandle test pointers, including the instances around dangling_handle,
documenting that exercised export_box/import_box paths validate arguments and
return before dereferencing them; preserve the existing test setup.
- Line 24: Add a signature-invariant test for the C SDK bridge, covering the FFI
symbols declared in sdks/go/bridge.h and implemented by cboxbridge.c, including
parameter and return types. Keep the check aligned with the manually maintained
header and excluded cbindgen functions so future Rust bridge signature changes
cannot drift across the C/Go boundary.
In `@sdks/c/src/runtime.rs`:
- Around line 946-970: Add an end-to-end test for the RuntimeEvent::GoImportBox
branch that constructs the event with an owned import handle and invokes
dispatch_event, using the existing consume_go_import_handle callback and drop
counter to verify routing and ownership transfer. Keep the generic
dispatch_handle_event test unchanged.
In `@sdks/go/archive_test.go`:
- Around line 154-254: Split
TestArchiveCancellationDuringPreparationPreventsNativeSubmission into a shared
helper, following the pattern of testArchiveTeardownWaits. Have the helper
accept the server-construction callback and archive-operation callback,
centralize runtime setup, archive gate locking, context sampling/cancellation,
unlocking, result validation, and assertArchiveRequestNotSubmitted, then reduce
the Import and Export subtests to configure and invoke it while preserving their
existing operation-specific setup and cleanup.
In `@sdks/go/archive.go`:
- Line 92: Add a concise comment beside the blocking receives in Export and
Import explaining that a lost native completion event causes the operation and
runtime teardown to wait indefinitely because the archive gate remains held.
Preserve the existing unbounded wait and drain-loop behavior.
In `@sdks/go/bridge_callback.go`:
- Around line 113-151: Update goBoxliteOnCreateBox and goBoxliteOnGetBox to free
their CBoxHandle payloads on both the h == 0 path and the channel type-mismatch
path, matching the cleanup behavior in goBoxliteOnImportBox. Use the existing
freeBoxHandlePayload helper before each early return while preserving the
current callback flow.
In `@sdks/go/integration/archive/archive_test.go`:
- Around line 18-199: The TestIntegrationArchiveRoundTrip function is too long
and combines multiple responsibilities. Extract cohesive phase helpers for
creating/seeding the source box, exporting and validating the archive,
validating restored identity/state, and verifying restored usability after
archive deletion; keep TestIntegrationArchiveRoundTrip as a short sequence
invoking those helpers while preserving shared cleanup and assertions.
- Around line 201-209: Update the assertExec function signature to place ctx
context.Context immediately after t, before box and the remaining arguments.
Adjust all four assertExec call sites to pass ctx as the first argument after t,
preserving the existing operation, output, command, and args values.
In `@sdks/go/runtime.go`:
- Around line 55-67: Refactor archiveOperationGate by adding a cond() accessor
that initializes and returns idle while holding mu, then use it in both release
and closeAndWait. Remove the idle nil guard in release while preserving
zero-value usability and the existing close-and-wait synchronization behavior.
🪄 Autofix
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: cc52b850-c1bf-4cc9-b2f3-eac8d5fb7b1d
📒 Files selected for processing (15)
make/help.mkmake/test.mksdks/c/cbindgen.tomlsdks/c/src/archive.rssdks/c/src/event_queue.rssdks/c/src/lib.rssdks/c/src/runtime.rssdks/go/README.mdsdks/go/archive.gosdks/go/archive_test.gosdks/go/bridge.csdks/go/bridge.hsdks/go/bridge_callback.gosdks/go/integration/archive/archive_test.gosdks/go/runtime.go
📦 BoxLite review — couldn't completepowered by BoxLite |
a6be08b to
e4dc2e4
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated no new comments.
Suppressed comments (2)
sdks/c/src/archive.rs:23
- The TokioRuntimeDropGuard docs are currently hard to read (lowercase start, sentence fragments, trailing whitespace). This makes it unclear why the guard exists and when it’s needed.
// use it to prevent the runtime from being dropped while the import task is running.
// The runtime is cloned into the task,
// and this guard will drop that clone when the task completes.
sdks/c/src/archive.rs:42
- TokioRuntimeDropGuard::drop spawns a new OS thread just to
drop(runtime)when Arc::try_unwrap fails. That path isn’t the final Arc ref, so thread creation is unnecessary overhead and makes teardown harder to reason about.
match Arc::try_unwrap(runtime) {
Ok(runtime) => runtime.shutdown_background(),
Err(runtime) => {
drop(std::thread::spawn(move || drop(runtime)));
}
}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
src/boxlite/src/runtime/rt_impl.rs (1)
378-391: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRecord the post-shutdown contract in the doc comment.
import_boxnow rejects requests after shutdown, butexportkeeps working after shutdown. The Go integration test atsdks/go/integration/archive/archive_test.golines 201-225 depends on exactly this asymmetry. State the rejection in the doc comment so callers do not have to read the body to learn it.📝 Proposed doc addition
/// Import a box from a `.boxlite` archive. /// /// Creates a new box with a new ID from archived disk images and /// configuration. The imported box starts in `Stopped` state. + /// + /// Returns [`BoxliteError::Stopped`] if the runtime has been shut down. + /// The check runs before archive validation. pub async fn import_box(🤖 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/rt_impl.rs` around lines 378 - 391, Update the doc comment for import_box to explicitly state that it rejects calls after the runtime has been shut down, while preserving the documented import behavior and distinguishing it from export’s post-shutdown behavior.sdks/c/src/archive.rs (2)
285-285: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument why a dangling handle is safe in these tests.
The tests pass
ptr::NonNull::dangling().as_ptr()as thehandleandruntimearguments. This is safe only becauseexport_boxandimport_boxvalidatedest,archive_path,name_or_null, andcbbefore they dereference the pointer. If a future change moveslet handle_ref = &*handle;above the argument validation, these tests become undefined behavior instead of failing assertions. Add a comment that records this ordering dependency.Also applies to: 368-369
🤖 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 `@sdks/c/src/archive.rs` at line 285, Add a comment beside the dangling handle setup in the affected tests explaining that it is safe only because export_box and import_box validate dest, archive_path, name_or_null, and cb before dereferencing handle or runtime. Explicitly document that moving handle_ref = &*handle or equivalent dereferences before validation would make these tests undefined behavior.
32-44: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueThe
Errbranch of the guard cannot drop the runtime.
Arc::try_unwrapreturnsErronly when anotherArcclone still exists. Dropping the returned clone therefore only decrements the reference count. It never runs theRuntimedestructor. The spawned OS thread performs no useful work and costs one thread per completed import.♻️ Proposed simplification
impl Drop for TokioRuntimeDropGuard { fn drop(&mut self) { let Some(runtime) = self.0.take() else { return; }; - match Arc::try_unwrap(runtime) { - Ok(runtime) => runtime.shutdown_background(), - Err(runtime) => { - drop(std::thread::spawn(move || drop(runtime))); - } - } + // Only the last clone can run the runtime destructor, and dropping a + // runtime from inside its own worker thread would block. Hand it to + // the background shutdown path instead. + if let Ok(runtime) = Arc::try_unwrap(runtime) { + runtime.shutdown_background(); + } } }🤖 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 `@sdks/c/src/archive.rs` around lines 32 - 44, Remove the spawned-thread handling from TokioRuntimeDropGuard::drop. In the Arc::try_unwrap error branch, directly drop the returned Arc so this guard only releases its reference; allow the Runtime destructor to run when the final Arc owner is dropped.sdks/go/integration/archive/archive_test.go (1)
228-236: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove
ctxahead ofoperationin the signature.Go places
context.Contextfirst, or directly after*testing.Tin a test helper. Here it sits third, afterbox. Several linters that check context placement flag this. Reordering toassertExec(t, ctx, box, operation, wantStdout, command, args...)also makes the four adjacentstringparameters easier to read at each call site.🤖 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 `@sdks/go/integration/archive/archive_test.go` around lines 228 - 236, Reorder the parameters of assertExec so ctx follows t and precedes box, operation, wantStdout, command, and args. Update every assertExec call site to use the new order while preserving all argument values and behavior.sdks/go/archive_test.go (1)
569-627: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate the two archive test servers.
newArchiveImportServerandnewArchiveExportServerduplicate the/v1/config,/v1/boxes/source-box, and export route handlers. They differ only in which route signalsstartedand blocks onrelease, and in the capability flags returned by/v1/config. One helper that takes the blocking route removes about 30 duplicated lines and keeps the two fixtures from drifting.♻️ Sketch of a single parameterized helper
// newArchiveServer serves the REST routes the archive tests need. The handler // for blockPath signals started and then waits for release. func newArchiveServer(t *testing.T, blockPath string, started chan<- struct{}, release <-chan struct{}) *httptest.Server { t.Helper() var startOnce sync.Once gate := func(path string) { if path == blockPath { startOnce.Do(func() { close(started) }) <-release } } return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { switch { case r.Method == http.MethodGet && r.URL.Path == "/v1/config": w.Header().Set("Content-Type", "application/json") _, _ = io.WriteString(w, `{"capabilities":{"import_enabled":true,"export_enabled":true}}`) case r.Method == http.MethodGet && r.URL.Path == "/v1/boxes/source-box": w.Header().Set("Content-Type", "application/json") _, _ = io.WriteString(w, sourceBoxJSON) case r.Method == http.MethodPost && r.URL.Path == "/v1/boxes/import": gate(r.URL.Path) w.Header().Set("Content-Type", "application/json") _, _ = io.WriteString(w, importedBoxJSON) case r.Method == http.MethodPost && r.URL.Path == "/v1/boxes/source-box/export": gate(r.URL.Path) w.Header().Set("Content-Type", "application/octet-stream") _, _ = io.WriteString(w, "archive-bytes") default: http.NotFound(w, r) } })) }🤖 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 `@sdks/go/archive_test.go` around lines 569 - 627, Replace newArchiveImportServer and newArchiveExportServer with one parameterized newArchiveServer helper accepting the blocking route path, started channel, and release channel. Consolidate the shared /v1/config, source-box, and export handlers, using a route-matching gate to signal started once and wait on release only for the selected path; preserve the import and export capability responses required by their callers.
🤖 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/src/archive.rs`:
- Around line 18-24: Run cargo fmt for the Rust file, focusing on the
TokioRuntimeDropGuard area and the corresponding code near the later reported
location; remove all trailing whitespace from the affected comments, including
both comment blocks, while preserving their content and behavior.
In `@sdks/go/integration/archive/archive_test.go`:
- Around line 18-226: Split TestIntegrationArchiveRoundTrip into focused phase
helpers or t.Run subtests, keeping the existing execution order and assertions
intact. Introduce a small fixture struct to carry source, restored, IDs, and
runtimeIsShutdown between phases, and retain shared setup and cleanup in the
parent test. Separate source creation/seeding, archive export/validation, import
identity/state validation, deleted-archive independence, and post-shutdown
validation so no code block exceeds 100 lines.
In `@sdks/go/runtime.go`:
- Around line 85-87: Update the comment near Close’s shutdown sequence to remove
the claim that parked Export or Import callers release per-call resources before
the drain goroutine and native runtime stop. Describe that closing r.closing
wakes them with ErrRuntimeClosed, while EventQueue::mark_closed safely drains
and drops undelivered owned payloads during native teardown.
---
Nitpick comments:
In `@sdks/c/src/archive.rs`:
- Line 285: Add a comment beside the dangling handle setup in the affected tests
explaining that it is safe only because export_box and import_box validate dest,
archive_path, name_or_null, and cb before dereferencing handle or runtime.
Explicitly document that moving handle_ref = &*handle or equivalent dereferences
before validation would make these tests undefined behavior.
- Around line 32-44: Remove the spawned-thread handling from
TokioRuntimeDropGuard::drop. In the Arc::try_unwrap error branch, directly drop
the returned Arc so this guard only releases its reference; allow the Runtime
destructor to run when the final Arc owner is dropped.
In `@sdks/go/archive_test.go`:
- Around line 569-627: Replace newArchiveImportServer and newArchiveExportServer
with one parameterized newArchiveServer helper accepting the blocking route
path, started channel, and release channel. Consolidate the shared /v1/config,
source-box, and export handlers, using a route-matching gate to signal started
once and wait on release only for the selected path; preserve the import and
export capability responses required by their callers.
In `@sdks/go/integration/archive/archive_test.go`:
- Around line 228-236: Reorder the parameters of assertExec so ctx follows t and
precedes box, operation, wantStdout, command, and args. Update every assertExec
call site to use the new order while preserving all argument values and
behavior.
In `@src/boxlite/src/runtime/rt_impl.rs`:
- Around line 378-391: Update the doc comment for import_box to explicitly state
that it rejects calls after the runtime has been shut down, while preserving the
documented import behavior and distinguishing it from export’s post-shutdown
behavior.
🪄 Autofix
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: 74884d73-2728-4813-867b-91a42f47565f
📒 Files selected for processing (8)
sdks/c/src/archive.rssdks/c/src/event_queue.rssdks/go/README.mdsdks/go/archive.gosdks/go/archive_test.gosdks/go/integration/archive/archive_test.gosdks/go/runtime.gosrc/boxlite/src/runtime/rt_impl.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- sdks/go/README.md
- sdks/go/archive.go
e4dc2e4 to
ccabfae
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated no new comments.
Suppressed comments (1)
sdks/c/src/archive.rs:45
- TokioRuntimeDropGuard::drop spawns a new OS thread in the common case where the Tokio runtime still has other Arc owners (Arc::try_unwrap fails). That means every Import will typically create an extra thread just to drop an Arc, which is expensive and can exhaust resources under load. The guard can simply hold an Arc clone for the task lifetime and then let it drop normally; runtime shutdown should be handled by the owner that is actually freeing the runtime, not by each import task.
match Arc::try_unwrap(runtime) {
Ok(runtime) => runtime.shutdown_background(),
Err(runtime) => {
drop(std::thread::spawn(move || drop(runtime)));
}
ccabfae to
00a0484
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated no new comments.
Suppressed comments (3)
sdks/c/include/boxlite.h:492
- PR description says the archive bridge symbols should be excluded from the public C header, but boxlite_box_export/boxlite_runtime_import are now declared in sdks/c/include/boxlite.h. Either hide these declarations from the public header (e.g., move them to a Go-only header like sdks/go/bridge.h or guard with an internal macro) or update the PR description to reflect that the C SDK API surface is expanding to include archive import/export.
// Submit a box export.
//
// On success, the callback owns the returned path and must release it with
// `boxlite_free_string`. The archive itself is never deleted by this bridge.
enum BoxliteErrorCode boxlite_box_export(CBoxHandle *handle,
const char *dest,
CBoxExportCb cb,
void *user_data,
CBoxliteError *out_error);
// Submit a trusted archive import.
//
// A null or empty `name_or_null` leaves the new box unnamed. The callback
// owns the returned stopped box handle. The caller retains ownership of the
// archive file; this bridge never removes it.
enum BoxliteErrorCode boxlite_runtime_import(CBoxliteRuntime *runtime,
const char *archive_path,
const char *name_or_null,
CRuntimeImportCb cb,
void *user_data,
CBoxliteError *out_error);
sdks/c/src/archive.rs:42
- TokioRuntimeDropGuard::drop spawns a new OS thread on the common path where Arc::try_unwrap fails. That will happen whenever other Arc clones exist (which is typical), so each import can spawn a thread just to drop an Arc — unnecessary overhead and potential resource exhaustion under load. Drop the Arc directly in the Err branch (only the Ok branch needs shutdown_background to avoid blocking when this is the last ref).
match Arc::try_unwrap(runtime) {
Ok(runtime) => runtime.shutdown_background(),
Err(runtime) => {
drop(std::thread::spawn(move || drop(runtime)));
}
}
sdks/c/include/boxlite.h:158
- PR description says the archive bridge symbols should be excluded from the public C header, but this header now publicly defines the archive callback typedefs (CBoxExportCb/CRuntimeImportCb). If these APIs are intended to be Go-internal only, consider moving these typedefs to sdks/go/bridge.h (or guarding them behind an internal-only macro) and keeping the public C SDK surface unchanged. If they are intended to become part of the C SDK, the PR description should be updated accordingly to avoid mismatched expectations.
This issue also appears on line 472 of the same file.
// Box export completion.
typedef void (*CBoxExportCb)(char*, CBoxliteError*, void*);
typedef struct RuntimeHandle CBoxliteRuntime;
// Runtime import completion.
typedef void (*CRuntimeImportCb)(CBoxHandle*, CBoxliteError*, void*);
typedef struct OptionsHandle CBoxliteOptions;
Summary
Expose the existing box archive export and import capabilities through the public C API and Go SDK. Preserve caller ownership of archive files and provide an opt-in real-VM round-trip test.
Call graph
Before
LiteBox::export (LiteBox · src/boxlite/src/litebox/mod.rs:193) — Rust archive export with no Go SDK entry point
Runtime::import_box (Runtime · src/boxlite/src/runtime/rt_impl.rs:382) — Rust archive import with no Go SDK entry point
After
(*Box).Export (Box · sdks/go/archive.go:61) — validates input and submits export
└─ boxlite_box_export (public C FFI · sdks/c/src/archive.rs:51) — invokes LiteBox::export asynchronously
└─ LiteBox::export (LiteBox · src/boxlite/src/litebox/mod.rs:193) — writes the caller-owned archive
└─ dispatch_event (Runtime event queue · sdks/c/src/runtime.rs:527) — delivers the archive path to Go
(*Runtime).Import (Runtime · sdks/go/archive.go:127) — maps an empty name to None and submits import
└─ boxlite_runtime_import (public C FFI · sdks/c/src/archive.rs:67) — invokes Runtime::import_box asynchronously
└─ Runtime::import_box (Runtime · src/boxlite/src/runtime/rt_impl.rs:382) — creates a new stopped box
└─ dispatch_event (Runtime event queue · sdks/c/src/runtime.rs:527) — delivers the owned box handle to Go
Changes
Box.ExportandRuntime.Importwith input validation and callback-backed result delivery.How to verify
make test:unit:go FILTER='Archive|AbandonOwnedResult'GOFLAGS=-race make test:unit:go FILTER='Archive|AbandonOwnedResult'make test:unit:ffimake test:unit:rust FILTER='import_after_shutdown'make test:unit:gomake test:unit:coremake fmt:checkmake lintmake test:integration:go FILTER=TestIntegrationArchiveRoundTripRisks / rollout
Summary by CodeRabbit
New Features
Bug Fixes
Tests