feat(logs): add opt-in host-side container log capture#963
Open
xhebox wants to merge 3 commits into
Open
Conversation
📦 BoxLite review — 1 issue ·
|
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds opt-in container init stdout/stderr capture, persists the setting through API and SDK layers, configures a guest-visible log path, and asynchronously writes output to rotating host-visible log files. ChangesContainer Log Capture Feature
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant BoxService
participant BoxLiteRuntime
participant GuestContainer
participant HostLogFile
Client->>BoxService: create box with captureLogs
BoxService->>BoxLiteRuntime: create options with capture_logs
BoxLiteRuntime->>GuestContainer: init with log_capture_path
GuestContainer->>HostLogFile: write and rotate stdout/stderr
GuestContainer->>HostLogFile: finish capture during shutdown
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![boxlite-agent[bot]](https://avatars.githubusercontent.com/in/4244458?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces an opt-in captureLogs / capture_logs option to persist container init/entrypoint stdout/stderr on the host under the box’s managed logs directory, enabling post-hoc log retrieval for detached boxes and long-lived workloads. It also includes supporting runtime/build-system changes around libkrun/libkrunfw artifact handling (kernel sidecar support, env plumbing) and musl/static build configuration.
Changes:
- Add end-to-end
capture_logsoption plumbing across runtime, guest RPC, SDKs (Node/Python/Go/C), REST/OpenAPI, runner DTOs, and API persistence/migrations. - Implement guest-side init stdout/stderr file capture via pipe-draining threads writing to a host-backed virtiofs-mounted log file.
- Adjust libkrun/libkrunfw integration: link vendored libkrun as a Rust dependency, optionally expose/use
libkrunfw.binvia env-controlledkrun_set_kernel, and update static/musl build scripts/config.
Reviewed changes
Copilot reviewed 60 out of 61 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/shim/Cargo.toml | Adds shim feature passthrough for libkrunfw kernel build features. |
| src/shim/build.rs | Removes linker workaround now that libkrun is no longer linked as a Rust staticlib. |
| src/shim/.cargo/config.toml | Adds shim-local Linux static-link rustflags. |
| src/shared/src/constants.rs | Adds mount tag + guest mountpoint constants for container log capture share. |
| src/shared/proto/boxlite/v1/service.proto | Extends Container.Init request with optional log_capture_path. |
| src/guest/src/service/container.rs | Validates/consumes log_capture_path and passes it into container start; adds unit tests. |
| src/guest/src/container/stdio.rs | Implements init stdout/stderr background capture to an append-only log file; adds tests. |
| src/guest/src/container/mod.rs | Updates container start example signature to include new parameters. |
| src/guest/src/container/lifecycle.rs | Starts capture before init runs and threads stdio capture into Container::start. |
| src/guest/.cargo/config.toml | Consolidates musl rustflags across targets (guest). |
| src/deps/libkrun-sys/src/lib.rs | Converts to a facade that forwards krun_* surface to vendored Rust libkrun. |
| src/deps/libkrun-sys/Cargo.toml | Adds optional vendored libkrun dependency + new kernel-build features. |
| src/deps/libkrun-sys/build.rs | Refocuses build.rs on libkrunfw sidecars; adds pyelftools requirement check for kernel builds. |
| src/deps/libgvproxy-sys/build.rs | Restricts glibc libresolv static-link path injection to target_env=gnu. |
| src/cli/Cargo.toml | Adds feature passthrough for libkrunfw kernel build features. |
| src/boxlite/src/vmm/krun/engine.rs | Adds env-driven external kernel discovery/format parsing and calls krun_set_kernel. |
| src/boxlite/src/vmm/controller/spawn.rs | Propagates krunfw kernel env vars into shim process. |
| src/boxlite/src/util/mod.rs | Honors explicit BOXLITE_RUNTIME_DIR and exports it to spawned commands. |
| src/boxlite/src/util/binary_finder.rs | Uses split_paths and makes explicit runtime dir override take precedence over embedded runtime. |
| src/boxlite/src/runtime/options.rs | Adds capture_logs option to BoxOptions with default false. |
| src/boxlite/src/runtime/layout.rs | Documents {cid}.log and adds container_log_path() helper. |
| src/boxlite/src/runtime/constants.rs | Re-exports shared logs constants. |
| src/boxlite/src/rest/types.rs | Adds capture_logs to REST request type and serialization path. |
| src/boxlite/src/portal/interfaces/container.rs | Threads log_capture_path through Container.Init gRPC call. |
| src/boxlite/src/litebox/init/types.rs | Stores log_capture_path in init pipeline context. |
| src/boxlite/src/litebox/init/tasks/vmm_spawn.rs | Creates host log file, mounts logs virtiofs share, and computes guest capture path. |
| src/boxlite/src/litebox/init/tasks/guest_init.rs | Passes log_capture_path into container init pipeline. |
| src/boxlite/src/jailer/shim_copy.rs | Updates libkrunfw sidecar copy semantics to include optional kernel artifact. |
| src/boxlite/src/jailer/sandbox/bwrap.rs | Propagates krunfw kernel env vars into sandbox environment. |
| src/boxlite/src/jailer/common/rlimit.rs | Fixes musl vs glibc resource-argument typing for getrlimit/setrlimit. |
| src/boxlite/src/jailer/bwrap.rs | Propagates krunfw kernel env vars into bubblewrap wrapper. |
| src/boxlite/Cargo.toml | Adds features for krunfw kernel artifact builds and updates krun feature semantics. |
| src/boxlite/build.rs | Updates shim discovery for current cargo target; removes allow-multiple-definition linker arg for tests. |
| sdks/python/src/options.rs | Exposes capture_logs option in Python SDK and maps into BoxOptions. |
| sdks/node/src/options.rs | Exposes captureLogs in Node SDK and maps into BoxOptions. |
| sdks/node/lib/simplebox.ts | Adds captureLogs to SimpleBoxOptions and forwards it to native layer. |
| sdks/node/lib/native-contracts.ts | Updates JS/native contract typing with captureLogs. |
| sdks/go/options.go | Adds WithCaptureLogs option and maps to C bridge setter. |
| sdks/go/bridge_cgo_prebuilt.go | Adds -lunwind to Linux LDFLAGS for Go prebuilt cgo bridge. |
| sdks/go/bridge_cgo_dev.go | Adds -lunwind to Linux LDFLAGS for Go dev cgo bridge. |
| sdks/c/src/options.rs | Adds C ABI setter for capture_logs and updates internal options handle mutation. |
| sdks/c/include/boxlite.h | Exposes boxlite_options_set_capture_logs in public C header. |
| scripts/common.sh | Adds helper to map legacy env toggles to Cargo feature args. |
| scripts/build/build-shim.sh | Updates Linux shim build to choose target based on rustc host and use shim-local cargo config. |
| scripts/build/build-runtime.sh | Forwards feature args when building runtime artifacts. |
| scripts/build/build-libseccomp.sh | Supports native-musl hosts (uses CC when host==target). |
| scripts/build/build-guest.sh | Supports native musl toolchains and builds from guest dir to pick up guest-local cargo config. |
| openapi/box.openapi.yaml | Adds capture_logs field to OpenAPI schema with default false. |
| Cargo.lock | Updates lockfile for new vendored/libkrun dependency graph and related crates. |
| apps/runner/pkg/boxlite/stubs.go | Threads CaptureLogs through runner recover → create flow. |
| apps/runner/pkg/boxlite/client.go | Applies WithCaptureLogs(true) when runner DTO requests capture. |
| apps/runner/pkg/api/dto/box.go | Adds captureLogs to create/recover DTOs. |
| apps/api/src/migrations/pre-deploy/1762539090000-add-box-log-capture-migration.ts | Adds DB column captureLogs with default false. |
| apps/api/src/boxlite-rest/mappers/box-to-box.mapper.ts | Maps REST capture_logs into internal captureLogs. |
| apps/api/src/boxlite-rest/dto/create-box.dto.ts | Adds REST DTO field capture_logs. |
| apps/api/src/box/services/box.service.ts | Bypasses warm-pool reuse when log capture requested; persists captureLogs on box. |
| apps/api/src/box/runner-adapter/runnerAdapter.v2.ts | Includes captureLogs in runner v2 create/recover payloads. |
| apps/api/src/box/runner-adapter/runnerAdapter.v0.ts | Includes captureLogs in runner v0 create/recover payloads (typed as optional extension). |
| apps/api/src/box/entities/box.entity.ts | Adds captureLogs column to Box entity. |
| apps/api/src/box/dto/create-box.dto.ts | Adds captureLogs to API CreateBox DTO (camelCase). |
| .cargo/config.toml | Consolidates musl rustflags (incl. musl_v1_2_3 cfg) at workspace level. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (4)
src/guest/src/container/stdio.rs (1)
134-164: 🩺 Stability & Availability | 🔵 Trivial | ⚖️ Poor tradeoffCapture ownership transfer degrades immediate-crash diagnostics.
start_capturetakes ownership ofstdout_rx/stderr_rx. Per the docstring, once captured,drain_output()(used byContainer::diagnose_exitin lifecycle.rs) returns empty strings. WhencaptureLogsis enabled and the init process exits immediately, theContainerInitResponseerror reason will lack the stdout/stderr text that was previously embedded for diagnosis — the caller must separately inspect the log file, and background capture threads racing against the synchronousis_running()/diagnose_exit()check may not have finished writing yet anyway.This looks like an accepted tradeoff (documented in
drain_output's doc comment), but worth confirming it's intentional given the diagnostic-quality regression on the immediate-crash 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/guest/src/container/stdio.rs` around lines 134 - 164, Confirm the ownership transfer in start_capture on stdout_rx/stderr_rx is intentional, since it causes drain_output (and thus Container::diagnose_exit in lifecycle.rs) to lose immediate-crash stdout/stderr details once captureLogs is enabled. If the regression is unintended, adjust start_capture/spawn_capture_reader so diagnostics can still read buffered output before capture takes over, or preserve a fallback path in diagnose_exit to include the init text even after capture begins.sdks/c/src/options.rs (1)
150-154: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMissing docstring on new public FFI function.
boxlite_options_set_capture_logshas no doc comment describing thevalsemantics (0 disables, non-zero enables). The coding guidelines for this path require comprehensive docstrings on public functions.As per coding guidelines,
sdks/**/*.{js,ts,jsx,tsx,py,java,go,rs,rb,php,cs,cpp,c,h,swift,kt}should "Write comprehensive docstrings for all public functions and classes."📝 Suggested docstring
+/// Enables or disables persisting the container's init stdout/stderr to a +/// host-visible log file. `val` is non-zero to enable, zero to disable. #[unsafe(no_mangle)] pub unsafe extern "C" fn boxlite_options_set_capture_logs(opts: *mut CBoxliteOptions, val: c_int) { options_set_capture_logs(opts, val) }🤖 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/options.rs` around lines 150 - 154, The new public FFI function boxlite_options_set_capture_logs in options.rs is missing the required doc comment. Add a comprehensive docstring directly above this exported function describing what val does, including that 0 disables log capture and any non-zero value enables it, and make sure the docs match the existing public FFI style used for options_set_capture_logs and related symbols.Source: Coding guidelines
sdks/node/src/options.rs (1)
717-717: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNo test exercises
capture_logs: Some(true)mapping.Existing tests only set
capture_logs: None(for compile-fix purposes); none assert thatSome(true)maps toopts.capture_logs == true.Also applies to: 755-755
🤖 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/node/src/options.rs` at line 717, The options mapping for capture_logs is only covered for the None case, so add a test that exercises the Some(true) path and asserts it maps to opts.capture_logs == true. Update the existing coverage around the options conversion in the relevant options mapping tests, using the capture_logs field and the options struct/mapping function to verify the boolean is preserved.apps/api/src/box/runner-adapter/runnerAdapter.v0.ts (1)
21-21: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRegenerate the runner-api-client DTOs to include
captureLogs.CreateBoxDTOandRecoverBoxDTOinapps/libs/runner-api-clientstill omit this field even thoughapps/runner/pkg/api/dto/box.goalready defines it, so these ad hoc& { captureLogs?: boolean }intersections keep the generated client out of sync with the wire contract.🤖 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.v0.ts` at line 21, The generated runner-api-client DTOs are out of sync because `CreateBoxDTO` and `RecoverBoxDTO` still lack the `captureLogs` field and the adapter is compensating with ad hoc intersections. Regenerate the DTOs in `apps/libs/runner-api-client` from the source contract in `apps/runner/pkg/api/dto/box.go` so `captureLogs` is included directly in the generated types, then remove the local `& { captureLogs?: boolean }` workaround from `runnerAdapter.v0.ts` and use the updated `CreateBoxDTO`/`RecoverBoxDTO` symbols as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/api/src/boxlite-rest/mappers/box-to-box.mapper.ts`:
- Line 37: The REST box response is missing the capture_logs field, so update
the box response DTO and the boxToBoxResponse mapping to include it. Add
capture_logs to BoxResponseDto and ensure boxToBoxResponse reads from
box.captureLogs (consistent with the create path in createDto.captureLogs) so
callers can retrieve the capture setting after creation.
In `@sdks/go/bridge_cgo_dev.go`:
- Line 16: The CGO bridge in bridge_cgo_dev.go now links against libunwind, but
the Linux setup scripts do not ensure the matching development package is
installed. Update the Ubuntu/musllinux/manylinux install steps to add the
libunwind dev dependency, or adjust the bridge build flags so -lunwind is only
enabled when libunwind is available. Keep the fix aligned with the
bridge_cgo_dev.go linkage and the Linux environment setup scripts.
In `@src/boxlite/build.rs`:
- Around line 967-971: The shim lookup in the build helper currently prefers
env::var("TARGET") first, which can select a musl-specific boxlite-shim before
the GNU Linux fallback. Update the shim resolution logic in the function that
builds target_path so musl targets are skipped here or the GNU Linux shim is
checked first, using the existing TARGET/profile-based lookup and the Linux shim
selection path to keep GNU preferred.
In `@src/boxlite/src/jailer/bwrap.rs`:
- Around line 636-644: The sandbox setup currently forwards
BOXLITE_KRUNFW_KERNEL_PATH in bwrap::build_shim_command, but the host kernel
file itself is not mounted, so Krun::find_krunfw_kernel_path can still fail
inside the jail. Update the bwrap configuration to detect when the configured
kernel path is absolute and exists on the host, then add it as a read-only bind
mount into the sandbox alongside the existing home/system/shim mounts. Keep the
env forwarding logic intact, but ensure the path is actually reachable inside
the jailed environment.
In `@src/boxlite/src/jailer/sandbox/bwrap.rs`:
- Around line 150-158: The sandbox setup in bwrap.rs forwards
BOXLITE_KRUNFW_KERNEL_PATH via bwrap_cmd.setenv, but the referenced file may
still be unreachable after --clearenv and sandboxing. Update the bwrap command
construction around the env forwarding loop to also bind-mount the forwarded
kernel path as read-only before launch, using the existing path handling in the
sandbox setup so the shim can access it even when it is not already covered by
ctx.readonly_paths() or ctx.writable_paths().
In `@src/guest/src/container/lifecycle.rs`:
- Around line 174-181: The container setup in Container::new leaves an orphaned
libcontainer state if stdio.start_capture(log_capture) fails after
start::create_container_with_stdio has already created the container. Move the
log capture setup on ContainerStdio before calling create_container_with_stdio
so failures happen before any container state exists; use the existing
ContainerStdio::new and stdio.start_capture flow to ensure Self is only
constructed after capture is ready.
In `@src/guest/src/container/stdio.rs`:
- Around line 182-210: The spawn_capture_reader helper currently panics on
thread creation failure via expect, and it also drops log write errors silently
inside the reader loop. Change spawn_capture_reader to return the thread spawn
Result (or another fallible outcome) instead of unwrapping, then update
start_capture to handle spawn failure by logging and skipping or propagating it
rather than crashing the guest agent. Also add a tracing::warn! when
log.write_all(chunk) fails so write/disk errors are observable.
In `@src/guest/src/service/container.rs`:
- Around line 247-257: The `Container::init` flow validates `log_capture_path`
too late, after `rootfs` bind mounting has already mutated filesystem state.
Move the `log_capture_config(init_req.log_capture_path)` check up with the other
early input validation near the `config.entrypoint.is_empty()` guard, before any
bind mounts or CA cert installation happen, and remove the later duplicate
validation block while keeping the resulting `log_capture` value available for
the later `init()` logic.
In `@src/shared/proto/boxlite/v1/service.proto`:
- Around line 212-214: The proto comment for log_capture_path overstates
guest-side validation by claiming the path is checked to stay under
/run/boxlite/logs, but the guest-side log_capture_config in
src/guest/src/service/container.rs only enforces non-empty and absolute paths.
Fix this by either adding a prefix check in log_capture_config to reject paths
outside the expected logs directory, or by updating the comment on
log_capture_path to describe the actual validation performed; use
configure_log_capture and log_capture_config as the key symbols to locate the
relevant host/guest path handling.
---
Nitpick comments:
In `@apps/api/src/box/runner-adapter/runnerAdapter.v0.ts`:
- Line 21: The generated runner-api-client DTOs are out of sync because
`CreateBoxDTO` and `RecoverBoxDTO` still lack the `captureLogs` field and the
adapter is compensating with ad hoc intersections. Regenerate the DTOs in
`apps/libs/runner-api-client` from the source contract in
`apps/runner/pkg/api/dto/box.go` so `captureLogs` is included directly in the
generated types, then remove the local `& { captureLogs?: boolean }` workaround
from `runnerAdapter.v0.ts` and use the updated `CreateBoxDTO`/`RecoverBoxDTO`
symbols as-is.
In `@sdks/c/src/options.rs`:
- Around line 150-154: The new public FFI function
boxlite_options_set_capture_logs in options.rs is missing the required doc
comment. Add a comprehensive docstring directly above this exported function
describing what val does, including that 0 disables log capture and any non-zero
value enables it, and make sure the docs match the existing public FFI style
used for options_set_capture_logs and related symbols.
In `@sdks/node/src/options.rs`:
- Line 717: The options mapping for capture_logs is only covered for the None
case, so add a test that exercises the Some(true) path and asserts it maps to
opts.capture_logs == true. Update the existing coverage around the options
conversion in the relevant options mapping tests, using the capture_logs field
and the options struct/mapping function to verify the boolean is preserved.
In `@src/guest/src/container/stdio.rs`:
- Around line 134-164: Confirm the ownership transfer in start_capture on
stdout_rx/stderr_rx is intentional, since it causes drain_output (and thus
Container::diagnose_exit in lifecycle.rs) to lose immediate-crash stdout/stderr
details once captureLogs is enabled. If the regression is unintended, adjust
start_capture/spawn_capture_reader so diagnostics can still read buffered output
before capture takes over, or preserve a fallback path in diagnose_exit to
include the init text even after capture begins.
🪄 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: e9fe977b-3baf-4d4c-9053-1052ce1a9e6e
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (60)
.cargo/config.tomlapps/api/src/box/dto/create-box.dto.tsapps/api/src/box/entities/box.entity.tsapps/api/src/box/runner-adapter/runnerAdapter.v0.tsapps/api/src/box/runner-adapter/runnerAdapter.v2.tsapps/api/src/box/services/box.service.tsapps/api/src/boxlite-rest/dto/create-box.dto.tsapps/api/src/boxlite-rest/mappers/box-to-box.mapper.tsapps/api/src/migrations/pre-deploy/1762539090000-add-box-log-capture-migration.tsapps/runner/pkg/api/dto/box.goapps/runner/pkg/boxlite/client.goapps/runner/pkg/boxlite/stubs.goopenapi/box.openapi.yamlscripts/build/build-guest.shscripts/build/build-libseccomp.shscripts/build/build-runtime.shscripts/build/build-shim.shscripts/common.shsdks/c/include/boxlite.hsdks/c/src/options.rssdks/go/bridge_cgo_dev.gosdks/go/bridge_cgo_prebuilt.gosdks/go/options.gosdks/node/lib/native-contracts.tssdks/node/lib/simplebox.tssdks/node/src/options.rssdks/python/src/options.rssrc/boxlite/Cargo.tomlsrc/boxlite/build.rssrc/boxlite/src/jailer/bwrap.rssrc/boxlite/src/jailer/common/rlimit.rssrc/boxlite/src/jailer/sandbox/bwrap.rssrc/boxlite/src/jailer/shim_copy.rssrc/boxlite/src/litebox/init/tasks/guest_init.rssrc/boxlite/src/litebox/init/tasks/vmm_spawn.rssrc/boxlite/src/litebox/init/types.rssrc/boxlite/src/portal/interfaces/container.rssrc/boxlite/src/rest/types.rssrc/boxlite/src/runtime/constants.rssrc/boxlite/src/runtime/layout.rssrc/boxlite/src/runtime/options.rssrc/boxlite/src/util/binary_finder.rssrc/boxlite/src/util/mod.rssrc/boxlite/src/vmm/controller/spawn.rssrc/boxlite/src/vmm/krun/engine.rssrc/cli/Cargo.tomlsrc/deps/libgvproxy-sys/build.rssrc/deps/libkrun-sys/Cargo.tomlsrc/deps/libkrun-sys/build.rssrc/deps/libkrun-sys/src/lib.rssrc/guest/.cargo/config.tomlsrc/guest/src/container/lifecycle.rssrc/guest/src/container/mod.rssrc/guest/src/container/stdio.rssrc/guest/src/service/container.rssrc/shared/proto/boxlite/v1/service.protosrc/shared/src/constants.rssrc/shim/.cargo/config.tomlsrc/shim/Cargo.tomlsrc/shim/build.rs
💤 Files with no reviewable changes (1)
- src/shim/build.rs
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
sdks/c/include/boxlite.h (1)
584-585: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the public C API and name the parameter descriptively.
Add a Doxygen comment explaining the option and use a name such as
enabledinstead ofval.Proposed declaration
-void boxlite_options_set_capture_logs(CBoxliteOptions *opts, int val); +/** + * Enable or disable container init stdout/stderr capture. + * + * `@param` opts Options handle to modify. + * `@param` enabled Non-zero enables capture; zero disables it. + */ +void boxlite_options_set_capture_logs(CBoxliteOptions *opts, int enabled);🤖 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/include/boxlite.h` around lines 584 - 585, Update the public declaration boxlite_options_set_capture_logs to rename the parameter from val to a descriptive name such as enabled, and add a Doxygen comment documenting that the option controls whether logs are captured.Source: Coding guidelines
src/boxlite/src/rest/types.rs (1)
170-170: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a regression assertion for
capture_logspropagation.
from_optionsnow setscapture_logs, buttest_create_box_request_from_optionsdoes not verify it. Assert the defaultSome(false)value and add an enabled case so this cross-layer contract cannot silently regress.🤖 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/rest/types.rs` at line 170, Add regression coverage in test_create_box_request_from_options for the capture_logs field populated by from_options: assert the default options produce Some(false), then add a case with capture_logs enabled and assert it propagates as Some(true).
🤖 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/node/lib/simplebox.ts`:
- Line 367: Update getOrCreate() to validate captureLogs before reusing an
existing named box. When the requested captureLogs setting differs from the
existing box, do not return the reused instance; instead reject the reuse or
route through create() so the requested log-capture behavior is applied.
In `@src/guest/src/container/lifecycle.rs`:
- Around line 396-435: Update shutdown and stop_init so termination failures are
propagated instead of discarded, and wait until the container is confirmed
stopped after both SIGTERM and SIGKILL. Move the is_shutdown store in shutdown
to occur only after stop_init completes successfully, ensuring Drop retains its
fallback behavior if shutdown fails or processes remain alive.
In `@src/guest/src/container/stdio.rs`:
- Around line 147-158: Update start_capture to validate that path resolves
within the managed /run/boxlite/logs directory before any file creation, append,
or rotation occurs. Resolve the parent directory and reject absolute or
otherwise resolved paths outside that directory, including malformed requests
that merely satisfy the existing absolute-path validator; apply the same
containment validation to the related rotation logic near the additional
location.
- Around line 276-321: Update rotate to preserve a valid active log path if
reopening the file fails after renaming it to the archive: stage or otherwise
restore the active path before returning the error, and ensure self.file remains
usable for fallback writes. Apply the same transaction-safe behavior to the
corresponding rotation logic near the second rotate implementation, and add
coverage for failure after the active rename.
---
Nitpick comments:
In `@sdks/c/include/boxlite.h`:
- Around line 584-585: Update the public declaration
boxlite_options_set_capture_logs to rename the parameter from val to a
descriptive name such as enabled, and add a Doxygen comment documenting that the
option controls whether logs are captured.
In `@src/boxlite/src/rest/types.rs`:
- Line 170: Add regression coverage in test_create_box_request_from_options for
the capture_logs field populated by from_options: assert the default options
produce Some(false), then add a case with capture_logs enabled and assert it
propagates as Some(true).
🪄 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: aa4af101-8303-42f9-a707-cbfe35722734
📒 Files selected for processing (33)
apps/api/src/box/dto/create-box.dto.tsapps/api/src/box/entities/box.entity.tsapps/api/src/box/runner-adapter/runnerAdapter.v0.tsapps/api/src/box/runner-adapter/runnerAdapter.v2.tsapps/api/src/box/services/box.service.tsapps/api/src/boxlite-rest/dto/create-box.dto.tsapps/api/src/boxlite-rest/mappers/box-to-box.mapper.tsapps/api/src/migrations/pre-deploy/1762539090000-add-box-log-capture-migration.tsapps/runner/pkg/api/dto/box.goapps/runner/pkg/boxlite/client.goapps/runner/pkg/boxlite/stubs.goopenapi/box.openapi.yamlsdks/c/include/boxlite.hsdks/c/src/options.rssdks/go/options.gosdks/node/lib/native-contracts.tssdks/node/lib/simplebox.tssdks/node/src/options.rssdks/python/src/options.rssrc/boxlite/src/litebox/init/tasks/guest_init.rssrc/boxlite/src/litebox/init/tasks/vmm_spawn.rssrc/boxlite/src/litebox/init/types.rssrc/boxlite/src/portal/interfaces/container.rssrc/boxlite/src/rest/types.rssrc/boxlite/src/runtime/constants.rssrc/boxlite/src/runtime/layout.rssrc/boxlite/src/runtime/options.rssrc/guest/src/container/lifecycle.rssrc/guest/src/container/mod.rssrc/guest/src/container/stdio.rssrc/guest/src/service/container.rssrc/guest/src/service/guest.rssrc/shared/proto/boxlite/v1/service.proto
🚧 Files skipped from review as they are similar to previous changes (24)
- src/boxlite/src/runtime/constants.rs
- apps/api/src/migrations/pre-deploy/1762539090000-add-box-log-capture-migration.ts
- src/boxlite/src/litebox/init/types.rs
- apps/api/src/box/runner-adapter/runnerAdapter.v2.ts
- apps/api/src/box/entities/box.entity.ts
- apps/runner/pkg/boxlite/stubs.go
- src/boxlite/src/portal/interfaces/container.rs
- sdks/node/lib/native-contracts.ts
- apps/api/src/box/services/box.service.ts
- openapi/box.openapi.yaml
- src/boxlite/src/runtime/layout.rs
- sdks/python/src/options.rs
- apps/runner/pkg/boxlite/client.go
- sdks/node/src/options.rs
- sdks/c/src/options.rs
- apps/api/src/box/dto/create-box.dto.ts
- src/boxlite/src/litebox/init/tasks/guest_init.rs
- apps/api/src/boxlite-rest/dto/create-box.dto.ts
- apps/api/src/box/runner-adapter/runnerAdapter.v0.ts
- src/shared/proto/boxlite/v1/service.proto
- apps/api/src/boxlite-rest/mappers/box-to-box.mapper.ts
- sdks/go/options.go
- src/guest/src/service/container.rs
- src/boxlite/src/litebox/init/tasks/vmm_spawn.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Add opt-in host-side persistence for container init and entrypoint stdout/stderr through
captureLogs/capture_logs. When enabled, output is written to{box}/logs/{container_id}.log, including for detached boxes, without changing existingexec()streaming.Captured logs are bounded with size-based rotation: the active file rotates at 10 MiB and four numbered archives are retained (
.log.1through.log.4).Closes #909.
Changes
jqduring runtime assembly, avoiding GNU/BSDseddifferences on Darwin.Log layout
Rotation is performed by the guest-side writer that owns the file descriptor. This avoids
copytruncateraces and lets inode-aware host tailers follow the active path after rename/reopen.How to verify
cargo check -p boxlite-guestcargo test -p boxlite-guest container::stdiocargo test -p boxlite-guestcargo clippy -p boxlite-guest --all-targets -- -D warningsgit diff --checkpackage_idThe stdio tests cover capture and drain behavior, rotation and oldest-file pruning, resuming size accounting from an existing log, and continuing to drain when rotation fails.
Summary by CodeRabbit
captureLogs(capture_logs) setting to capture container init stdout/stderr to host-visible log files when creating or recovering boxes.