Skip to content

applecontainer: Run / Start / Stop / Remove (PR-C)#61

Closed
bilby91 wants to merge 1 commit into
mainfrom
m6/pr-c-run-start
Closed

applecontainer: Run / Start / Stop / Remove (PR-C)#61
bilby91 wants to merge 1 commit into
mainfrom
m6/pr-c-run-start

Conversation

@bilby91
Copy link
Copy Markdown
Member

@bilby91 bilby91 commented May 15, 2026

Summary

Fourth PR of M6. Replaces the RunContainer / StartContainer /
StopContainer / RemoveContainer stubs with real implementations.
Branched off PR-B (#60) since it depends on the inspect/find helpers
and the JSON envelope contract; rebase onto main after PR-B
merges
.

What lands

Bridge (Swift, lifecycle.swift)

  • ac_run — wraps ContainerClient.create with a full
    ContainerConfiguration built from a RunSpec JSON payload.
    Kernel fetched via ClientKernel.getDefaultKernel. Image must
    already be in the local content store (pull is PR-F's job).
  • ac_startclient.bootstrap + process.start in detached
    mode (no stdio). Idempotent: a running container is a no-op
    success (matches CLI behavior + Docker semantics).
  • ac_stop — wraps ContainerClient.stop with a Go-supplied
    grace period (defaults to Apple's 5s SIGTERM + SIGKILL).
  • ac_delete — wraps ContainerClient.delete with the force flag.
  • All four documented per the PR-B style guide. Sync timeout is 60s
    to cover cold first-runs that need to fetch the kernel + init image.

Mount mapping

  • runtime.MountBindFilesystem.virtiofs (matches the design
    §11.1 finding — virtiofs is identity-permissive; this is what we
    want for host-directory binds).
  • runtime.MountTmpfsFilesystem.tmpfs.
  • runtime.MountVolume → treated as virtiofs binds for now;
    real named-volume lifecycle is a later PR.

cgo shim

  • shim.h / shim.c — four new function-pointer slots + wrappers,
    resolved via dlsym in ac_load. Partial-failure path resets all
    pointers and dlcloses the handle.

Runtime methods (lifecycle_darwin_arm64.go)

  • runSpecToWire marshals runtime.RunSpec into the bridge JSON.
    Intentionally drops RunArgs, Privileged, SecurityOpt — these
    Docker-specific knobs don't map onto Apple's container model
    (documented in design/runtime-applecontainer.md §8). The wire
    type doesn't have fields for them, so a future caller depending on
    them fails the build instead of silently losing the field.
  • RunContainer returns a Container in StateCreated.
  • StartContainer idempotent via the bridge.
  • StopContainer clamps Timeout to int32 seconds with overflow
    guard.
  • RemoveContainer.RemoveVolumes silently dropped (volume support
    deferred).
  • mapRunErr / mapLifecycleErr translate Apple's notFound
    errors into runtime.ImageNotFoundError /
    runtime.ContainerNotFoundError.

Inspect path tightened (PR-B file, inspect_darwin_arm64.go)

  • containerMount.Type now decodes Apple's enum-with-associated-
    values shape ({"virtiofs":{}}, {"tmpfs":{}}, …) via a custom
    mountTypeWire decoder. Was a plain string in PR-B because PR-B's
    smoke tests didn't create any mounts. Surfaces as
    runtime.MountBind / MountTmpfs / MountVolume.
  • ReadOnly derived from options containing "ro" (Apple's
    actual wire shape, replacing PR-B's synthetic readonly field).

Tests

  • TestLifecycle_EndToEnd — Run → Start → Inspect(running) → Start
    (idempotent) → Stop → Inspect(stopped) → Remove →
    Inspect(notfound). 5s state-poll loop absorbs the apiserver's
    async status transitions.
  • TestRunContainer_MissingImage — asserts
    ImageNotFoundError contract when the caller hasn't pulled the
    image first.
  • TestRunContainer_BindMount — virtiofs round-trip with trailing-
    slash absorption.

Test plan

  • make bridge && go test ./runtime/applecontainer/... — full
    package green
  • GOOS=linux GOARCH=amd64 CGO_ENABLED=0 go build ./runtime/applecontainer/...
    — stub still compiles
  • go vet ./... clean

Summary by CodeRabbit

Release Notes

  • New Features

    • Container lifecycle management: create, start, stop, and delete containers on macOS/ARM64
    • Support for bind mounts, temporary filesystems, and volume mounts with proper configuration
    • Container state inspection with mount details
    • Robust error handling for missing images and containers
  • Tests

    • Added comprehensive end-to-end container lifecycle tests
    • Added tests for error cases and mount validation

Review Change Stack

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 15, 2026

📝 Walkthrough

Walkthrough

This PR implements container lifecycle operations (run, start, stop, delete) across a three-layer architecture: a Swift bridge that performs the work, a C shim that dynamically loads it, and Go runtime methods that call through the shim. Mount type handling is updated to flexibly decode wire formats, and comprehensive tests verify the end-to-end flow and error contracts.

Changes

Apple Container Lifecycle Bridge

Layer / File(s) Summary
Bridge C contract and function declarations
applecontainer-bridge/include/ac_bridge.h
Four new C-exported entrypoints (ac_run, ac_start, ac_stop, ac_delete) are declared with documented ownership, threading model (Swift Task + DispatchSemaphore), JSON envelope response shapes, and timeout behavior (up to 60s for lifecycle operations).
Swift bridge implementation for lifecycle operations
applecontainer-bridge/Sources/ACBridge/lifecycle.swift
JSON wire structs and four C-exported functions implement container creation (with image resolution, process/mount configuration, user parsing), idempotent start, configurable stop with grace period, and force deletion. Process config merges spec overrides with image defaults; mount conversion validates type and required fields.
C shim dynamic loader and wrapper functions
runtime/applecontainer/shim.c, runtime/applecontainer/shim.h
Dynamic symbol loading extends ac_load to resolve the four new entrypoints; error cleanup resets function pointers; wrapper functions (ac_run_p, etc.) call resolved symbols when available or return NULL.
Go runtime lifecycle method implementations
runtime/applecontainer/lifecycle_darwin_arm64.go
Four Runtime methods (RunContainer, StartContainer, StopContainer, RemoveContainer) marshal specs to JSON, call the C shim, decode responses, validate context/load state, convert timeout to seconds, and map bridge errors to typed ImageNotFoundError/ContainerNotFoundError.
Mount type decoding and projection update
runtime/applecontainer/inspect_darwin_arm64.go
Updated containerMount wire struct with custom mountTypeWire JSON decoder that accepts either a string or enum-object form; mount projection maps Apple variant names to runtime types via mountKindToRuntime and derives read-only status from options.
Runtime stub refactoring
runtime/applecontainer/runtime_darwin_arm64.go
Lifecycle stub implementations are removed (moved to lifecycle_darwin_arm64.go); comment updated to reflect the new organization.
End-to-end lifecycle and error contract tests
runtime/applecontainer/lifecycle_darwin_arm64_test.go
Three test functions verify full lifecycle transitions (run → start → stop → remove), missing image error typing via *runtime.ImageNotFoundError, bind-mount round-trip inspection with path normalization, and idempotent start behavior. Includes a state-polling helper for waiting on desired container state.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

  • crunchloop/devcontainer#59: Extended shim dynamic symbol loading and wrapper infrastructure established in PR-A2.
  • crunchloop/devcontainer#58: Builds directly on the Swift/Go runtime skeleton from PR-B by replacing lifecycle stubs with real implementations wired through new bridge entrypoints.

Poem

🐰 A bridge of bytes now spans the gap,
Swift speaks to Go with nary a mishap,
Containers run and stop with grace,
Mount types dance in their proper place,
Tests confirm the harmony so right—
Chef's kiss to lifecycle's flight! 🎯

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 35.48% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and specifically summarizes the main change: implementing the four container lifecycle operations (Run, Start, Stop, Remove) for the Apple container runtime, with '(PR-C)' indicating the phase.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch m6/pr-c-run-start

Comment @coderabbitai help to get the list of available commands and usage tips.

Fourth PR of M6. Replaces the RunContainer / StartContainer /
StopContainer / RemoveContainer stubs with real implementations.

Bridge (Swift):
- lifecycle.swift: ac_run wraps ContainerClient.create with a full
  ContainerConfiguration built from a RunSpec JSON payload, kernel
  fetched via ClientKernel.getDefaultKernel. ac_start does
  bootstrap + process.start in detached mode with idempotent
  short-circuit when the snapshot already reports running. ac_stop
  wraps ContainerClient.stop with a Go-supplied grace period.
  ac_delete wraps ContainerClient.delete with the force flag.
- ac_bridge.h: documented per the PR-B style guide. Notes Apple's
  60s sync timeout for these calls (cold first-run needs to fetch
  the kernel and init image).
- Mount mapping: bind → Filesystem.virtiofs (matches the design §11.1
  finding — virtiofs is identity-permissive). Tmpfs → Filesystem.tmpfs.
  Named volumes treated as virtiofs binds for PR-C; real volume
  lifecycle is a later PR.

cgo shim (Go side):
- shim.h / shim.c: four new function-pointer slots and wrappers.

Runtime methods (lifecycle_darwin_arm64.go):
- runSpecToWire marshals runtime.RunSpec into the bridge JSON shape.
  Drops RunArgs, Privileged, SecurityOpt (intentionally absent on
  the wire type so a future caller depending on them fails the
  build instead of silently losing the field). Documented in
  design §8.
- RunContainer: marshal → ac_run → decode → returns
  runtime.Container in Created state.
- StartContainer: idempotent via bridge.
- StopContainer: clamps Timeout to int32 seconds with overflow guard.
- RemoveContainer: RemoveVolumes silently dropped (volume support
  deferred).
- mapRunErr / mapLifecycleErr translate Apple's `notFound`
  errors into the runtime.{Container,Image}NotFoundError contract.

Inspect path tightened (inspect_darwin_arm64.go):
- containerMount.Type now decodes Apple's enum-with-associated-values
  shape (`{"virtiofs":{}}` etc) via a custom mountTypeWire decoder.
  Surfaces as runtime.MountBind / MountTmpfs / MountVolume so callers
  reason about mounts in Docker-style terms.
- ReadOnly is now derived from the `options` array containing "ro"
  (Apple's wire shape, replacing the synthetic boolean field that
  doesn't exist in ContainerSnapshot).

Tests (lifecycle_darwin_arm64_test.go):
- TestLifecycle_EndToEnd: Run → Start → Inspect(running) → Start
  (idempotent) → Stop → Inspect(stopped) → Remove →
  Inspect(notfound). 5s state-poll loop absorbs the apiserver's
  async status transitions.
- TestRunContainer_MissingImage: asserts ImageNotFoundError contract
  when the caller hasn't pulled the image first.
- TestRunContainer_BindMount: creates a container with a virtiofs
  bind and verifies the inspect path round-trips the mount; absorbs
  Apple's trailing-slash path normalization via TrimRight.

Test plan:
- `make bridge && go test ./runtime/applecontainer/...` — full
  package green
- `GOOS=linux GOARCH=amd64 CGO_ENABLED=0 go build ./runtime/applecontainer/...`
  — stub still compiles
- `go vet ./...` clean

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@bilby91 bilby91 changed the base branch from m6/pr-b-inspect-find to main May 15, 2026 03:18
@bilby91 bilby91 force-pushed the m6/pr-c-run-start branch from f82d51c to 36055fe Compare May 15, 2026 03:18
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 4

🧹 Nitpick comments (1)
runtime/applecontainer/lifecycle_darwin_arm64_test.go (1)

126-176: 💤 Low value

Consider verifying mount type mapping in the round-trip assertion.

The test verifies Source and Target but doesn't check that the mount type is correctly mapped (virtiofs → bind). Adding a check for m.Type == string(runtime.MountBind) would strengthen the round-trip verification.

🔧 Suggested enhancement
 	for _, m := range details.Mounts {
 		if m.Target == "/mnt/work" &&
-			strings.TrimRight(m.Source, "/") == strings.TrimRight(hostDir, "/") {
+			strings.TrimRight(m.Source, "/") == strings.TrimRight(hostDir, "/") &&
+			m.Type == string(runtime.MountBind) {
 			found = true
 		}
 	}
🤖 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 `@runtime/applecontainer/lifecycle_darwin_arm64_test.go` around lines 126 -
176, In TestRunContainer_BindMount, extend the mount round-trip check to assert
the mount type is mapped to a bind mount by including a comparison against
string(runtime.MountBind) (e.g. require m.Type == string(runtime.MountBind))
when scanning details.Mounts for the entry with Target "/mnt/work" and Source
matching hostDir; update the found condition to include this type check and
adjust the failure message to surface the observed m.Type for easier diagnosis.
🤖 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 `@applecontainer-bridge/Sources/ACBridge/lifecycle.swift`:
- Around line 240-250: The ac_stop wrapper currently calls
runSync(timeoutSeconds: lifecycleTimeoutSeconds) which can return before the
ContainerClient.stop grace period completes; update ac_stop so the runSync
timeout is derived from the requested timeoutSeconds when >0 (e.g. compute a
syncTimeout = max(lifecycleTimeoutSeconds, Int(timeoutSeconds) + smallBuffer) or
clamp/reject excessively large timeoutSeconds), pass that syncTimeout into
runSync, and ensure you handle the Int32→Int conversion and fallback for
non-positive timeoutSeconds (use .default behavior and the normal
lifecycleTimeoutSeconds); use the ac_stop, runSync, lifecycleTimeoutSeconds, and
ContainerStopOptions symbols to locate the changes.
- Around line 213-229: The start path is a check-then-act race: multiple callers
can observe client.get(id:) status != .running and both call
client.bootstrap(...) / process.start(), causing one to fail; make ac_start
idempotent by collapsing "already started" failures into success or serializing
starts per container. Specifically, after calling client.bootstrap(id:...,
stdio:...) and process.start(), catch the error that indicates the container is
already running (or re-check with client.get(id:) on failure) and return
"{\"ok\":true}" for that case; alternatively implement a per-container mutex
around ac_start to serialize calls so only one caller can execute
client.bootstrap/process.start at a time. Ensure you reference and handle
client.get(id:), client.bootstrap(id:...), and process.start() in the fix.

In `@runtime/applecontainer/lifecycle_darwin_arm64.go`:
- Line 79: The cgo bridge calls (C.ac_run_p, C.ac_start_p, C.ac_stop_p,
C.ac_delete_p) are synchronous and currently invoked directly in RunContainer,
StartContainer, StopContainer, and RemoveContainer so ctx cancellation after the
call starts is ignored; fix by performing each C call in a dedicated goroutine
that sends its result (the raw C string) or an error on a result channel, then
in the caller use select to wait for either the result or ctx.Done(); if ctx is
done first return ctx.Err() to the caller, but ensure the background goroutine
still consumes the C result and calls goStringAndFree to release C memory (so no
leaks) even when the API returned early. Include this pattern for functions
RunContainer, StartContainer, StopContainer, RemoveContainer and their
respective C calls C.ac_run_p, C.ac_start_p, C.ac_stop_p, C.ac_delete_p.
- Around line 127-135: The timeout rounding currently adds 999_999_999 to
opts.Timeout.Nanoseconds(), which can overflow; update the rounding in the block
that sets graceSec (the code that reads opts.Timeout.Nanoseconds(), computes
secs, and assigns graceSec) to use safe ceiling division without addition: read
nanos := opts.Timeout.Nanoseconds(), compute secs := (nanos-1)/1_000_000_000 + 1
(ensuring nanos>0), then clamp secs to int64(1<<31-1) and assign graceSec =
int32(secs); adjust the existing secs and clamp logic around that calculation to
prevent negative wraparound.

---

Nitpick comments:
In `@runtime/applecontainer/lifecycle_darwin_arm64_test.go`:
- Around line 126-176: In TestRunContainer_BindMount, extend the mount
round-trip check to assert the mount type is mapped to a bind mount by including
a comparison against string(runtime.MountBind) (e.g. require m.Type ==
string(runtime.MountBind)) when scanning details.Mounts for the entry with
Target "/mnt/work" and Source matching hostDir; update the found condition to
include this type check and adjust the failure message to surface the observed
m.Type for easier diagnosis.
🪄 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: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 491f0255-13ae-4cfd-9bcf-45231f9fa340

📥 Commits

Reviewing files that changed from the base of the PR and between c212bc8 and 36055fe.

📒 Files selected for processing (8)
  • applecontainer-bridge/Sources/ACBridge/lifecycle.swift
  • applecontainer-bridge/include/ac_bridge.h
  • runtime/applecontainer/inspect_darwin_arm64.go
  • runtime/applecontainer/lifecycle_darwin_arm64.go
  • runtime/applecontainer/lifecycle_darwin_arm64_test.go
  • runtime/applecontainer/runtime_darwin_arm64.go
  • runtime/applecontainer/shim.c
  • runtime/applecontainer/shim.h

Comment on lines +213 to +229
let snap = try await client.get(id: id)
// Idempotency: if the container is already running, return
// success. Matches the CLI's behavior in
// ContainerStart.swift L60-72 and Docker's "start" on a
// running container (no-op).
if snap.status == .running {
return "{\"ok\":true}"
}
// Detached start: no stdio attachment. ProcessIO with
// detach=true returns [nil,nil,nil] for stdio; we replicate
// that directly without instantiating ProcessIO.
let process = try await client.bootstrap(
id: id,
stdio: [nil, nil, nil],
dynamicEnv: [:]
)
try await process.start()
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Make ac_start idempotent under concurrent callers.

This is a check-then-act sequence against external state: two callers can both observe status != .running and then race in bootstrap() / start(), so one of them can still fail even though the API is documented as a no-op success for already-started containers. Please collapse the backend's "already running/started" path to success, or serialize starts per container.

🤖 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 `@applecontainer-bridge/Sources/ACBridge/lifecycle.swift` around lines 213 -
229, The start path is a check-then-act race: multiple callers can observe
client.get(id:) status != .running and both call client.bootstrap(...) /
process.start(), causing one to fail; make ac_start idempotent by collapsing
"already started" failures into success or serializing starts per container.
Specifically, after calling client.bootstrap(id:..., stdio:...) and
process.start(), catch the error that indicates the container is already running
(or re-check with client.get(id:) on failure) and return "{\"ok\":true}" for
that case; alternatively implement a per-container mutex around ac_start to
serialize calls so only one caller can execute client.bootstrap/process.start at
a time. Ensure you reference and handle client.get(id:),
client.bootstrap(id:...), and process.start() in the fix.

Comment on lines +240 to +250
public func ac_stop(_ idPtr: UnsafePointer<CChar>?, _ timeoutSeconds: Int32) -> UnsafePointer<CChar>? {
guard let id = readCString(idPtr) else { return dupNullArgErr("id") }
return runSync(timeoutSeconds: lifecycleTimeoutSeconds) {
do {
// Apple's ContainerStopOptions has its own grace-period
// knob. timeoutSeconds <= 0 uses the API default (5s
// SIGTERM, then SIGKILL).
let opts: ContainerStopOptions = timeoutSeconds > 0
? .init(timeoutInSeconds: timeoutSeconds, signal: SIGTERM)
: .default
try await ContainerClient().stop(id: id, opts: opts)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Don't let the bridge timeout undercut the requested stop grace period.

ContainerStopOptions honors the caller-provided timeoutSeconds, but the outer runSync wait is still hard-capped at 60s. Any grace period above that can make the bridge return a timeout while the daemon is still legitimately waiting to stop the container. Derive the sync timeout from timeoutSeconds (with a small buffer), or reject/clamp larger values before calling into Swift.

🤖 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 `@applecontainer-bridge/Sources/ACBridge/lifecycle.swift` around lines 240 -
250, The ac_stop wrapper currently calls runSync(timeoutSeconds:
lifecycleTimeoutSeconds) which can return before the ContainerClient.stop grace
period completes; update ac_stop so the runSync timeout is derived from the
requested timeoutSeconds when >0 (e.g. compute a syncTimeout =
max(lifecycleTimeoutSeconds, Int(timeoutSeconds) + smallBuffer) or clamp/reject
excessively large timeoutSeconds), pass that syncTimeout into runSync, and
ensure you handle the Int32→Int conversion and fallback for non-positive
timeoutSeconds (use .default behavior and the normal lifecycleTimeoutSeconds);
use the ac_stop, runSync, lifecycleTimeoutSeconds, and ContainerStopOptions
symbols to locate the changes.

cSpec := C.CString(string(specBytes))
defer C.free(unsafe.Pointer(cSpec))

raw := goStringAndFree(C.ac_run_p(cSpec))
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

🧩 Analysis chain

🏁 Script executed:

# Locate and read the file
fd -t f "lifecycle_darwin_arm64.go" -x wc -l {}

Repository: crunchloop/devcontainer

Length of output: 122


🏁 Script executed:

# Read the entire file to understand context handling
fd -t f "lifecycle_darwin_arm64.go" -x cat -n {}

Repository: crunchloop/devcontainer

Length of output: 9640


🏁 Script executed:

# Search for the cgo function definitions and their usage
rg "C\.(ac_run_p|ac_start_p|ac_stop_p|ac_delete_p)" --context 5 -A 5

Repository: crunchloop/devcontainer

Length of output: 2205


🏁 Script executed:

# Check if these C functions are declared elsewhere with their signatures
rg "ac_run_p|ac_start_p|ac_stop_p|ac_delete_p" --type c --type go

Repository: crunchloop/devcontainer

Length of output: 1117


Bridge lifecycle calls do not honor context cancellation after execution begins

Each method (RunContainer at line 79, StartContainer at line 107, StopContainer at line 139, and RemoveContainer at line 166) checks ctx.Err() before invoking its respective cgo bridge call (C.ac_run_p, C.ac_start_p, C.ac_stop_p, C.ac_delete_p). However, the cgo function signatures do not accept context parameters, and once the synchronous C call begins, cancellation or deadline expiry will not interrupt it. The operation will run to completion or until the bridge's internal timeout, regardless of caller context state. This violates caller expectations about context propagation and can cause operations to block longer than intended.

🤖 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 `@runtime/applecontainer/lifecycle_darwin_arm64.go` at line 79, The cgo bridge
calls (C.ac_run_p, C.ac_start_p, C.ac_stop_p, C.ac_delete_p) are synchronous and
currently invoked directly in RunContainer, StartContainer, StopContainer, and
RemoveContainer so ctx cancellation after the call starts is ignored; fix by
performing each C call in a dedicated goroutine that sends its result (the raw C
string) or an error on a result channel, then in the caller use select to wait
for either the result or ctx.Done(); if ctx is done first return ctx.Err() to
the caller, but ensure the background goroutine still consumes the C result and
calls goStringAndFree to release C memory (so no leaks) even when the API
returned early. Include this pattern for functions RunContainer, StartContainer,
StopContainer, RemoveContainer and their respective C calls C.ac_run_p,
C.ac_start_p, C.ac_stop_p, C.ac_delete_p.

Comment on lines +127 to +135
var graceSec int32
if opts.Timeout > 0 {
// Round up to whole seconds, clamp to MaxInt32 in the unlikely
// case of a wildly-large timeout.
secs := (opts.Timeout.Nanoseconds() + 999_999_999) / 1_000_000_000
if secs > 1<<31-1 {
secs = 1<<31 - 1
}
graceSec = int32(secs)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

find . -name "lifecycle_darwin_arm64.go" -type f

Repository: crunchloop/devcontainer

Length of output: 118


🏁 Script executed:

cat -n ./runtime/applecontainer/lifecycle_darwin_arm64.go

Repository: crunchloop/devcontainer

Length of output: 9640


🏁 Script executed:

find . -type f -name "*.go" | xargs grep -l "type StopOptions struct" | head -5

Repository: crunchloop/devcontainer

Length of output: 88


🏁 Script executed:

cat -n ./runtime/runtime.go | head -100

Repository: crunchloop/devcontainer

Length of output: 4602


🏁 Script executed:

cat -n ./runtime/runtime.go | grep -A 10 "type StopOptions"

Repository: crunchloop/devcontainer

Length of output: 438


🏁 Script executed:

python3 << 'EOF'
# Verify the rounding logic
# Original: (nanos + 999_999_999) / 1_000_000_000
# Proposed: (nanos - 1) / 1_000_000_000 + 1

def test_rounding():
    test_cases = [
        (1, 1),                    # 1 nanosecond -> 1 second (round up)
        (999_999_999, 1),          # 999_999_999 nanoseconds -> 1 second (round up)
        (1_000_000_000, 1),        # Exactly 1 second
        (1_000_000_001, 2),        # 1s + 1ns -> 2 seconds (round up)
        (2_000_000_000, 2),        # Exactly 2 seconds
        (2_999_999_999, 3),        # ~3 seconds -> 3 seconds (round up)
    ]
    
    for nanos, expected in test_cases:
        # Original formula (with potential overflow issues)
        result_original = (nanos + 999_999_999) // 1_000_000_000
        
        # Proposed formula
        result_proposed = (nanos - 1) // 1_000_000_000 + 1
        
        match = "✓" if result_original == expected == result_proposed else "✗"
        print(f"{match} nanos={nanos:>12}: expected={expected}, original={result_original}, proposed={result_proposed}")

# Test overflow scenario
print("\n--- Overflow Analysis ---")
import sys
max_int64 = 2**63 - 1
print(f"int64.MaxValue = {max_int64}")
print(f"int64.MaxValue - 999_999_999 = {max_int64 - 999_999_999}")
print(f"Adding 999_999_999 to (MaxValue - 999_999_999) causes overflow:")
print(f"  Original: {(max_int64 - 999_999_999) + 999_999_999} = MaxValue (OK)")
print(f"  But in int64: wraps to {(max_int64 - 999_999_999 + 999_999_999) - 2**64} if exceeded")

# In Python, int doesn't overflow, but let's show the conceptual issue
large_nanos = max_int64 - 500_000_000
would_overflow = (large_nanos + 999_999_999)
print(f"\nLarge timeout: {large_nanos} nanoseconds")
print(f"After adding 999_999_999: {would_overflow}")
print(f"In int64, this would overflow and become negative")

test_rounding()
EOF

Repository: crunchloop/devcontainer

Length of output: 838


Timeout rounding can overflow before clamping

Line 131: (opts.Timeout.Nanoseconds() + 999_999_999) can overflow for very large positive durations (>~292 years). The addition wraps to a negative int64, which bypasses the clamp check since the condition if secs > 1<<31-1 only guards against positive overflow, not negative wraparound. This produces an invalid negative grace period.

Use ceiling division without the addition: (nanos - 1) / 1_000_000_000 + 1

Proposed fix
 	var graceSec int32
 	if opts.Timeout > 0 {
-		// Round up to whole seconds, clamp to MaxInt32 in the unlikely
-		// case of a wildly-large timeout.
-		secs := (opts.Timeout.Nanoseconds() + 999_999_999) / 1_000_000_000
+		// Round up to whole seconds without overflow, then clamp to MaxInt32.
+		nanos := opts.Timeout.Nanoseconds()
+		secs := (nanos - 1) / 1_000_000_000 + 1
 		if secs > 1<<31-1 {
 			secs = 1<<31 - 1
 		}
 		graceSec = int32(secs)
 	}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
var graceSec int32
if opts.Timeout > 0 {
// Round up to whole seconds, clamp to MaxInt32 in the unlikely
// case of a wildly-large timeout.
secs := (opts.Timeout.Nanoseconds() + 999_999_999) / 1_000_000_000
if secs > 1<<31-1 {
secs = 1<<31 - 1
}
graceSec = int32(secs)
var graceSec int32
if opts.Timeout > 0 {
// Round up to whole seconds without overflow, then clamp to MaxInt32.
nanos := opts.Timeout.Nanoseconds()
secs := (nanos - 1) / 1_000_000_000 + 1
if secs > 1<<31-1 {
secs = 1<<31 - 1
}
graceSec = int32(secs)
}
🤖 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 `@runtime/applecontainer/lifecycle_darwin_arm64.go` around lines 127 - 135, The
timeout rounding currently adds 999_999_999 to opts.Timeout.Nanoseconds(), which
can overflow; update the rounding in the block that sets graceSec (the code that
reads opts.Timeout.Nanoseconds(), computes secs, and assigns graceSec) to use
safe ceiling division without addition: read nanos :=
opts.Timeout.Nanoseconds(), compute secs := (nanos-1)/1_000_000_000 + 1
(ensuring nanos>0), then clamp secs to int64(1<<31-1) and assign graceSec =
int32(secs); adjust the existing secs and clamp logic around that calculation to
prevent negative wraparound.

@bilby91
Copy link
Copy Markdown
Member Author

bilby91 commented May 15, 2026

Superseded by #62 (single PR covering the full applecontainer runtime backend). Closing.

@bilby91 bilby91 closed this May 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant