feat: add sandbox runtime spec contract and explicit lifecycle API - #1007
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a spec-based reconciliation lifecycle for sandboxes, implementing explicit lifecycle methods (inspect, create, start_existing, stop_existing) in DockerSandboxService along with robust concurrency locking via _named_lock. It also adds comprehensive test suites to verify these behaviors. Feedback on the changes highlights a critical issue where task cancellation during container startup can bypass the compensating container removal and leak orphan containers. Additionally, the review points out that port verification will fail when dynamic port allocation (host port 0) is used, and recommends validating host port conflicts to prevent silent overwrites.
…ated cancellation Also reject conflicting non-zero host ports in _check_no_conflicting_ports, which previously only checked for guest-port collisions.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a spec-based reconciliation lifecycle for sandboxes, adding dataclasses like ResolvedSandboxRuntimeSpec and SandboxInspection, and implementing explicit lifecycle methods (inspect, create, start_existing, stop_existing) in the Docker backend. The feedback focuses on path normalization and cross-platform compatibility. Specifically, the reviewer recommends normalizing volume mount paths and working directories extracted from Docker inspect data to prevent false-positive mismatches, and using os.path.normpath instead of posixpath.normpath for host-side paths to ensure proper behavior on Windows hosts.
…ackend
posixpath.normpath is not a canonical form: POSIX reserves a leading "//"
for implementation-defined interpretation, so normpath keeps exactly two
leading slashes while Docker collapses them in the values it reports back
(Mounts.Destination, Config.WorkingDir). A desired spec built from a
"//"-prefixed absolute path (reachable from an absolute XAGENT_UPLOADS_DIR
or SANDBOX_VOLUMES entry, e.g. "${ROOT}/data" with ROOT=/) therefore never
byte-matched what the backend echoed: create()'s publish-before-verify step
reported a volumes/working_dir mismatch for a container that was exactly
what had been requested, removed it, and raised
SandboxRuntimeConflictError on every attempt. The same gap let "//x" and
"/x" pass the pre-create conflict checks as distinct mount points when the
backend would collapse them onto one.
Route every desired-state path through one canonical_sandbox_path() owner
in sandbox/base.py (resolved spec volumes and working_dir, mount intent
root and extras, and the Docker volume conflict check), which normalizes in
the POSIX domain the sandbox path domain actually uses on both sides of a
mount and collapses the leading slash run. Observed facts stay
backend-native, as ObservedRuntimeFacts documents.
Verified against Docker 29.4.0: with a "//"-prefixed host source, guest
target and working_dir, create() + inspect() now reports zero publish
mismatches, while a non-canonical request the backend actually rewrites
(trailing slash, which Docker Desktop answers with a /host_mnt-prefixed
Source) is still caught and refused.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new spec-based reconciliation lifecycle API for sandboxes, including canonical desired state representation (ResolvedSandboxRuntimeSpec), observed state facts (ObservedRuntimeFacts), and a matcher (spec_matches_inspection). It implements this lifecycle contract for the Docker backend with robust locking, validation, and verification steps, while keeping it disabled for the Boxlite backend. A critical issue was identified in the publish verification step of create() where comparing ports directly will fail when ephemeral ports (host port 0) are requested, as Docker assigns a concrete non-zero port at runtime. A suggestion was provided to map ports by guest port and allow a desired host port of 0 to match any allocated host port.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a spec-based reconciliation lifecycle for sandboxes, adding new data structures, exceptions, and helper functions to define and verify sandbox configurations. It implements these lifecycle methods (inspect, create, start_existing, stop_existing) for the Docker backend while leaving them unsupported for Boxlite. The review feedback highlights critical issues in the publish-verification step of container creation: comparing ephemeral host ports (0) directly will cause false mismatches, and observed volume paths and working directories should be canonicalized to prevent false positives due to minor lexical differences.
_check_no_conflicting_ports already rejects both sides of a port mapping: the guest-side collision that _create_container's guest-keyed dict would silently drop, and the host-side collision Docker only reports when the container starts. The volume checker covered only its host-keyed direction, so two different host sources on the same guest path reached the daemon and came back as a raw APIError 400 "Duplicate mount point" (verified on Docker 29.4.0) instead of the typed pre-create error. Add the symmetric guest-side check, canonicalizing through the same canonical_sandbox_path owner so paths the backend collapses onto one mount point share a key. Nothing that Docker accepts today becomes rejected: a shared guest path with differing sources cannot be created at all, and a shared guest path with a differing mode was already rejected by the host-side direction.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a spec-based reconciliation lifecycle for sandboxes, implementing new explicit lifecycle methods (inspect, create, start_existing, stop_existing) in DockerSandboxService backed by a robust per-name locking mechanism and publish-before-verify validation. It also adds comprehensive unit and integration tests. The review feedback suggests improving the robustness of publish verification by normalizing observed volume paths and working directories with canonical_sandbox_path to avoid false mismatches, and raising SandboxContractError instead of SandboxRuntimeConflictError when verification fails, along with updating the corresponding test assertions.
The docstring only described the pre-create volume-conflict check, but the same typed error also covers pre-create port conflicts and create()'s publish-before-verify mismatch against the observed backend state. Broaden it to the actual desired-state-conflict invariant instead of narrowing to one call site.
rogercloud
left a comment
rogercloud
left a comment
There was a problem hiding this comment.
Summary
This PR introduces a desired-state/observed-state contract for sandbox runtimes: ResolvedSandboxRuntimeSpec (canonicalized, structurally comparable, with a stable fingerprint()), ObservedRuntimeFacts/SandboxInspection (backend-native units, identity-only equality), and a three-state matcher spec_matches_inspection() returning MATCH/MISMATCH/UNVERIFIED. On the Docker side it adds an explicit lifecycle API (inspect/create/start_existing/stop_existing) with publish-before-verify semantics, fingerprint-label attestation, and a reworked waiter-counted per-name lock registry.
The motivation is sound and the root cause is correctly identified: today there is no shared, verifiable representation of "what was asked for" vs. "what is actually running", which is why same-CA external tasks intermittently fail with "Sandbox already exists with different runtime configuration". The change is purely additive — verified repo-wide that nothing outside src/xagent/sandbox/ and tests/sandbox/ calls any of the new surface. This is explicitly step 1 of a multi-PR plan, with the family state machine, generation-aware attach, and SandboxManager integration deferred.
Approach verdict: ACCEPTABLE WITH RESERVATIONS
The desired/observed split, the three-state verdict (rather than a boolean), and label-based attestation are the right abstractions — this is a root-cause fix, not a symptom patch, and the docstrings show unusually careful thinking about the semantics. The reservations are about scope sequencing and about two load-bearing mechanisms whose failure mode is worse than the bug being fixed.
D1 — The API's own stated usage contract has no supported implementation. create()'s and inspect()'s docstrings both tell callers that get-or-create semantics require calling inspect() first "under their own critical section" — but the only per-name mutex, _named_lock, is private (src/xagent/sandbox/docker_sandbox.py:989). There is currently no public way for any caller to span inspect→create atomically. The future reconciler will have to build its own lock registry, which leads directly into D2.
D2 — The new lock registry is a step-for-step duplicate of an existing one. _NamedLockEntry/_named_lock/_drop_named_lock_if_unused (src/xagent/sandbox/docker_sandbox.py:591-596, :989-1042) is structurally identical to _LifecycleLockEntry/_lifecycle_locked/_drop_lifecycle_lock_if_unused in src/xagent/web/sandbox_manager.py:376-414 — same entry dataclass, same waiter counting, same rollback on acquire failure, same identity-checked eviction. The docstring acknowledges the mirroring rather than extracting a shared helper. Given D1, a third copy is the likely outcome. Consider extracting this into one shared utility now, while there are only two copies.
D3 — Publish-before-verify makes an availability/safety tradeoff implicitly. _find_publish_mismatches (src/xagent/sandbox/docker_sandbox.py:452-492) is byte-equality against daemon-echoed values, and any disagreement destroys the just-created container. This module advertises Podman/compatible-runtime support, where mount-source realpath resolution and SELinux mode suffixes are routine. A single unanticipated daemon-side normalization means "no sandbox can ever be created on this deployment" — a strictly worse outage than the intermittent conflict this PR fixes. This should be an explicit policy, not an emergent property of exact-match logic: hard-fail on security-relevant fields (volumes, ports, network_isolated), log-and-degrade on cosmetic ones (working_dir).
D4 — The lifecycle API is reference-count-blind. stop_existing() has no notion of how many actors hold the sandbox, even though spec_matches_inspection's own docstring states that destructive actions "must go through the reference-count check" — and SandboxLeaseSpec, the type that would carry that concept, is defined here and used by nothing. The linked production bug is specifically about multi-actor sandbox sharing, so this is on-target rather than tangential.
D5 — Some surface is shipping well ahead of the consumer that would define its requirements. SandboxMountIntent, SandboxLeaseSpec, SandboxRecoveryRequiredError, and canonical_sandbox_path (as an exported concept) have zero callers outside base.py and tests. These are exactly the pieces most likely to be reshaped once the family state machine PR is designed — see the Simplification section.
Minor design notes: SandboxInspection.state is typed Literal["running","stopped"] while _build_inspection collapses created/exited/dead/restarting into "stopped", yet inspect()'s docstring says a never-started container "must be treated specially" — a distinction the type cannot express without reaching into raw_status. And the single-process/single-owner assumption underpinning _named_lock, _get_live_control, and create()'s check-then-create sequence is never stated, despite otherwise exhaustive docstrings.
Confirmed findings
Major
See inline comments for findings 1, 2, and 4 (docker_sandbox.py:1341, :1444, :1367).
Test coverage gap (major): _find_publish_mismatches has 7 comparison branches (image, volumes, ports, working_dir, network_isolated, cpus, memory); only 1 (volumes) is tested for a mismatch — the other 6 have zero mismatch-triggering coverage, and per D3 a false positive in any of them destroys the just-created container. _build_inspection (docker_sandbox.py:281-345) has no dedicated unit test at all — its image_digest, runtime_networks, labels, network_isolated fields have zero direct assertions, and the state mapping is only exercised for running/exited, never created/dead/restarting. No test exists for either finding 1 (remove-also-fails) or finding 2 (snapshot label inheritance).
Minor
See inline comments for findings 3 (severity-adjusted down from major), 5, 6, 7, 8, 9, 10 (docker_sandbox.py:1044, :1388, base.py:284, base.py:487, docker_sandbox.py:480, :354, :1350).
Additional minor items not mapped to a single line: two divergent derivations of network_isolated (docker_sandbox.py:340 vs :251-254 — the :340 derivation is verified correct against moby source; unify on it). The lock-recycle race fixed here for Docker is left unfixed in Boxlite (boxlite_sandbox.py:475-487). SandboxService.create/SandboxService.inspect are collision-prone names on a public ABC (the test file already has to import inspect as std_inspect). Several docstrings encode caller obligations nothing enforces or tests (spec_matches_inspection's consumer contract, inspect()'s critical-section requirement, SandboxMountIntent's realpath-resolution requirement). canonical_sandbox_path is not exported from src/xagent/sandbox/__init__.py despite being the normalization SandboxMountIntent's contract tells callers to align with. Docstring gaps: create() doesn't mention it's reference-count-blind; start_existing() doesn't state it performs no spec verification before adopting a container; the single-process/single-owner assumption is never stated.
Test coverage
Positive first: the new tests are genuinely non-tautological. The fingerprint field-sensitivity matrix, the full MATCH/MISMATCH/UNVERIFIED verdict matrix (including detecting a newer contract version), the lock mutual-exclusion/release-and-requeue test, and the repeated-cancellation-holds-lock regression test were each independently confirmed to actually pin the properties they claim.
Gaps that matter are covered above (publish-mismatch branch coverage, _build_inspection, findings 1/2). Minor gaps: no real-daemon test creates a network_isolated=True sandbox (pure coverage gap — moby's NetworkDisabled round-trips correctly, not a suspected bug). The fingerprint field-sensitivity matrix omits template_type and snapshot_id. test_docker_lock_infra.py:299 asserts source.count("_SandboxControl(") == 2 over raw file text, which breaks on reformatting. test_docker_lock_infra.py:187's "double cancel" test calls cancel() twice before the task ever runs, so the second call is a no-op — it doesn't test what its docstring claims, and the equivalent scenario is already properly covered at test_docker_lifecycle_api.py:272. No test for the finding-4 concurrency scenario, the finding-5 _controls growth, or _named_lock releasing when the body raises a non-cancellation exception.
Prior review findings (gemini-code-assist) — resolution status
All prior findings were addressed before this pass; each resolution was independently re-verified against the current code.
-
Cancellation during
container.startleaking an orphan container — FIXED._await_shieldednow shields the compensating remove, loops until the task truly completes even under repeated cancellation, and re-raises the originalCancelledErrorafterward so cancellation semantics are preserved. Pinned bytest_docker_lifecycle_api.py:259-341, confirmed passing. -
Ephemeral host port (
0) comparison always fails — WAIVED; the bot's claim was factually wrong and the author's rebuttal is correct. Verified:_build_inspectionreadsfacts.portsexclusively fromHostConfig.PortBindings;NetworkSettings.Portsis never read anywhere in the repo. Docker preserves the requestedHostPort: "0"inPortBindingsfor the container's lifetime — the concrete allocation only appears inNetworkSettings.Ports. A desired0is echoed back as0and the comparison never false-positives. No action needed. -
Missing host-port-collision check — FIXED.
_check_no_conflicting_ports(docker_sandbox.py:406-449) now checks both directions, with a dedicated passing test. Commit-hygiene note only: the fix landed in6735babaa(titled about lock handling) rather than the more plausibly-named9ff4bb4d(titled about volume conflicts). -
Path-normalization false-positive mismatches — FIXED, at the correct layer. The bot's suggestion to normalize the observed side was rightly rejected:
ObservedRuntimeFactsdeliberately stays backend-native by contract, and normalizing it would mask genuine backend rewrites (verified: no normalization call inside_build_inspection). The real bug was on the desired side —posixpath.normpathpreserves a leading//while Docker collapses it, so a//-prefixed path (reachable viaXAGENT_UPLOADS_DIR/SANDBOX_VOLUMESmisconfiguration) never byte-matched. Fixed bycanonical_sandbox_path()(base.py:202-228), verified applied only to desired-side call sites and covered by a parametrized regression test. -
Windows
os.path.normpathvsposixpath.normpath— WAIVED; sound judgment, independently verified. All seven supporting claims check out (Docker-only linux/amd64+arm64 image, no Windows CI job, noos.name/sys.platformbranches in the module, Boxlite uninstallable on Windows perpyproject.tomlplatform markers, theXAGENT_SANDBOX_HOST_STORAGE_ROOToverlay, andget_sandbox_volumes's docstring stating sources are already Docker-host paths). The crux is correct: these strings are the Docker daemon's host paths, not the local process's, so applying local-OS rules would be wrong even under Docker Desktop on Windows. This reasoning is now stated incanonical_sandbox_path's docstring. -
SandboxRuntimeConflictErrorreused for publish-verification mismatch — WAIVED; sound judgment, independently verified. The docstring was actually expanded to cover both the pre-create-conflict and publish-verify-mismatch cases (base.py:46-62). Repo-wide grep confirms zero call sites branch onSandboxRuntimeConflictErroras distinct fromSandboxContractError; all consumption is viapytest.raises/import. A new subclass would be speculative surface today and remains a backward-compatible option later.
Legacy-compatibility claims verified accurate. The PR's claimed three bounded exceptions to legacy behavior (lock registry swap, delete()'s identity-checked control removal, _create_container's optional extra_labels) all hold; _merge_info, list_sandboxes, create_snapshot's body, and the cleanup/warmup paths are confirmed untouched.
Simplification opportunities
src/xagent/sandbox/base.pyL397: yagniSandboxLeaseSpechas zero production callers; only its own test constructs it. Drop it and reintroduce with the PR that actually needs lease/concurrency tracking.src/xagent/sandbox/base.pyL465: yagniSandboxMountIntentand itscovered_extras/covering_extras/disjoint_extrasclassification have zero production callers. Drop the class and its_is_root_or_descendanthelper (L456), reintroducing alongside the mount-reconciliation caller.src/xagent/sandbox/docker_sandbox.pyL1028: shrink_drop_named_lock_if_unusedsplits a two-line waiter-count eviction into its own method for one usage pattern. Inline it into_named_lock's except/finally. (Separate from, and smaller than, D2's cross-file duplication.)src/xagent/sandbox/docker_sandbox.pyL1044: stdlib_await_shielded's hand-rolled shield-and-retry loop duplicates whatasyncio.shieldalready provides. Replace withtask = asyncio.ensure_future(coro); while True: try: return await asyncio.shield(task) except asyncio.CancelledError: if task.done(): raise— but preserve the "always re-raise the original cancellation" and "hold the lock until truly done" properties thattest_docker_lifecycle_api.py:259depends on.
net: roughly -250 lines possible. This is approximate — the two yagni removals also mean deleting tests/sandbox/test_mount_intent.py in full (120 lines) and the lease-related portion of tests/sandbox/test_runtime_spec.py.
Test run
pytest tests/sandbox/ in a clean worktree at this branch: 165 collected, 113 passed, 52 skipped, 0 failures, 0 errors. All 52 skips are Requires reachable Docker daemon, which was unavailable in this review environment — CI must confirm the Docker-gated half.
Recommendation
The design is right and the majority of this PR is high quality, so this is not a rework request. But three things should land before merge:
- Finding 1 (unguarded compensating remove) — a few lines, mirroring the pattern already used 20 lines above it. Two of its three consequences are live today.
- Finding 2 (snapshot label inheritance) — this one silently breaks the PR's own core guarantee, in the direction that matters most (false MATCH on an unverified container). Strip spec labels on commit, or explicitly clear them in
get_or_create()'s legacy path, plus a regression test. - The publish-verification coverage gap — a parametrized test over all 7
_find_publish_mismatchesbranches, plus tests for findings 1 and 2. Publish-before-verify is the mechanism whose false positives destroy containers (D3); shipping it with 1-of-7 branches covered is the single largest risk in the PR.
Findings 3, 4, and the remaining minor items do not need to block: finding 3 is bounded by docker-py's default timeout, and finding 4 is unreachable until start_existing/stop_existing gain a production caller — though it should be tracked explicitly and resolved before that caller lands.
D2 (the duplicated lock registry) and D3 (making the publish-verify failure policy explicit per-field) are strongly recommended for this PR while there are still only two copies and no consumer depending on current behavior. D1 and D4 are legitimately follow-up work, but D1 should at least be acknowledged in the PR description — shipping an API whose docstrings prescribe a critical section that no public primitive can provide is worth flagging for the next author.
Docker copies a container's labels into the image produced by commit(), and merges an image's labels into any container created from it for every key the create request does not set. create(A, spec_A) -> create_snapshot(A) -> a container built from that snapshot therefore inherited spec_A's fingerprint and presented it as its own attestation: MISMATCH where UNVERIFIED was owed, or a false MATCH when the specs agreed, on a container that never went through the verified create() path. Verified on Docker 29.4.0. Fix it at the label mechanism rather than at the snapshot: _create_container now always writes both attestation keys for every container either lifecycle path creates, blank when there is nothing to attest, so no base image can supply them. Docker cannot remove an inherited label, only overwrite it, so _build_inspection reads a blank label as absent -- reading it as a real value would turn UNVERIFIED into MISMATCH and rebuild adoptable containers. Also: - Guard create()'s compensating remove on the publish-mismatch path. A failing remove() no longer replaces SandboxRuntimeConflictError with a raw docker error; the leaked container is logged for operator action. - Reject unrealizable ResolvedSandboxRuntimeSpec values at construction (cpus >= 1, memory >= 128, mirroring SandboxConfig) instead of failing pydantic validation later inside create(). - Reject empty and relative paths in SandboxMountIntent. Its covered/covering/disjoint split is lexical, so "" normalized to "." and then read as disjoint from every absolute path -- the direction that grants a separate mount instead of folding it into an approved root. - Resolve the container before the control object in start_existing/ stop_existing, so probing an absent name no longer installs a _controls entry that nothing evicts. - Extract the per-key lock registry into sandbox/keyed_lock.py so the primitive has one owner and one set of tests. SandboxManager's own lifecycle lock is left as is; it is keyed by lifecycle key rather than container name and can adopt the shared registry separately. - Drop SandboxLeaseSpec, which had no callers. - Export canonical_sandbox_path, the normalization SandboxMountIntent's contract tells callers to align with. - State the contracts the docstrings had left implicit: publish verification is all-or-nothing because the fingerprint it stamps is immutable and covers every field; create()/start_existing()/stop_existing() are reference-count-blind; start_existing() adopts a container without verifying its spec; neither is mutually exclusive with Sandbox.stop(); the whole service assumes a single owning process; SandboxInspection.state is a two-value reduction and facts.raw_status carries the rest. Tests: all seven publish-mismatch branches with negative controls, direct _build_inspection coverage including the state mapping and blank-label normalization, a real-daemon snapshot-attestation regression test, a remove-also-fails publish mismatch, network_isolated=True against a real daemon, control-entry leak bounds, spec bounds, template_type/snapshot_id fingerprint sensitivity plus a guard against uncovered new fields, and the keyed lock registry's identity/eviction/cancellation invariants. Replaces the _SandboxControl construction-site check with an AST walk instead of a raw substring count, and corrects the repeated-cancel test's claim to the invariant it actually reaches.
|
Thanks — this was a thorough pass and several items were real. Fixes are in e9ef5b3; inline threads have individual replies. Below is the disposition for everything in the review body, since those items have no threads to reply in.
Design reservationsD1 — no public primitive spans inspect→create. Answered rather than changed: the consumer holds its own critical section at the layer that owns the identity. D2 — duplicated lock registry. Extracted. The entry dataclass, waiter counting, acquire-failure rollback and identity-checked eviction now live in
D3 — publish-verify failure policy. Agreed it should be an explicit policy; adopted that, but not the per-field tiering, because the tiering is not implementable where you suggested and would create a worse failure than the one it avoids.
Which means the tiering has to live upstream of the label — fields whose exact value cannot be guaranteed get constrained where the desired spec is built. Two such constraints already exist and are what keep exact equality from being brittle: On the Podman framing: the repo's only mentions are D4 — reference-count blindness. Correct layering, so the capability stays where it is, but the contract text was wrong by omission and is fixed. Reference counting needs to know about lifecycle families, attach, and idle eviction — all manager concepts; D5 — surface ahead of its consumer. Split, because the four items are not in the same position:
Minor design notes. Confirmed findingsTest coverage gap (major) — all closed.
Additional minor items.
Test coverage
Simplification
Follow-ups I am not opening from hereExplicit Docker client timeout for lifecycle operations (finding 3, with the |
rogercloud
left a comment
rogercloud
left a comment
There was a problem hiding this comment.
Update summary
One new commit since the last review, e9ef5b3e ("fix: stop snapshot images from laundering spec attestation", 10 files, ~1400 insertions), addresses essentially every finding from the previous pass:
- Fixes the two confirmed major bugs: the unguarded compensating
remove()on publish-verification failure, and the snapshot-attestation-inheritance bug (via a label-mechanism fix:_create_containernow unconditionally writes both attestation label keys — blank when there's nothing to attest — so no base/snapshot image can supply an inherited fingerprint through Docker's label-merge). - Fixes 4 of the 6 confirmed minor bugs (
_controlsleak, unrealizable spec construction, empty/relativeSandboxMountIntentpaths) and closes all 3 test-coverage gaps (all 7_find_publish_mismatchesbranches, direct_build_inspectionunit tests, remove-also-fails + snapshot-inheritance regression tests). - Extracts the per-name lock registry into a new shared module,
src/xagent/sandbox/keyed_lock.py. - Adds docstrings stating several previously-implicit contracts (publish-verify is all-or-nothing, the lifecycle methods are reference-count-blind, no mutual exclusion with
Sandbox.stop(), single-owning-process assumption). - Drops
SandboxLeaseSpec(zero callers) per the prior simplification suggestion; exportscanonical_sandbox_path.
All of the above was independently re-verified against the current code (not taken on the commit message's word) — see the checklist below.
Prior findings — verification status
1. Unguarded compensating remove() on publish-verification failure — FIXED. docker_sandbox.py:1401-1418 now wraps the remove in try/except Exception, logs the sandbox name/mismatched fields/underlying error on removal failure, and unconditionally raises SandboxRuntimeConflictError afterward. except Exception correctly excludes CancelledError. New test: test_conflict_error_survives_a_failing_compensating_remove (test_docker_lifecycle_api.py:409-450) makes remove() itself raise and confirms the typed error still surfaces.
2. Snapshot images inherit spec fingerprint labels — FIXED. _create_container now unconditionally seeds both LABEL_SPEC_FINGERPRINT/LABEL_SPEC_VERSION as blank for every container it builds (both create() and the legacy get_or_create() path), and _attestation_label reads a blank label as absent. Since Docker only merges an image's label into a new container for keys the create request does not set, and the create request now always sets both keys, an inherited (non-blank) label from a base/snapshot image can never survive. Two new real-daemon regression tests in test_bridge_seams.py (TestSnapshotDoesNotLaunderAttestation) directly exercise create(A) -> create_snapshot(A) -> get_or_create(B, template=snapshot) and assert B is UNVERIFIED, covering both the MISMATCH-should-be-UNVERIFIED and false-MATCH-should-be-UNVERIFIED directions. No gaps found.
3. _await_shielded unbounded-hang concern — unchanged, WAIVED (already severity-adjusted to minor last round). The author declined to add an explicit client timeout, reasoning that docker.from_env()'s per-request timeout doesn't bound the connection's whole lifetime differently than already assumed. This doesn't change last round's conclusion: docker-py's 60s default still bounds the realistic wedged-daemon case; not blocking.
4. start_existing/stop_existing vs. DockerSandbox.stop() mutex asymmetry — DOCUMENTED, still open/latent. Verified: stop()'s body is byte-identical to before; only new docstrings on start_existing/stop_existing (docker_sandbox.py:1460-1463, 1487-1488) now explicitly state "Mutual exclusion is per container name and is not shared with Sandbox.stop()... A caller that can issue both concurrently for one name must serialize them itself." This is an honest acknowledgment, not a code fix. Repo-wide grep confirms start_existing/stop_existing still have zero production callers, so this remains non-exploitable today. Please actually close this (e.g. have stop() acquire the same keyed lock) before either method gains a real caller — a documented race is still a race once something starts calling these methods.
5. _controls leak on nonexistent-name probes — FIXED. Both methods now resolve the container and raise SandboxNotFoundError before _get_live_control is ever reached. 4 new regression tests (TestLifecycleDoesNotLeakControlEntries, including a 25-distinct-unknown-names bound check) all pass.
6. from_parts() accepting unrealizable spec values — FIXED. ResolvedSandboxRuntimeSpec.__post_init__ now enforces cpus >= 1 / memory >= 128, matching SandboxConfig's own bounds exactly, raising ValueError at construction. Tests pass.
7. SandboxMountIntent(mount_root="") silently misclassifying mounts — FIXED. __post_init__ now rejects empty/relative mount_root/extra_mounts via _require_absolute_mount_path, raising ValueError rather than silently normalizing to a wrong answer. Tests pass.
Side note on the reply to this finding: the claim that SandboxMountIntent "has real consumers in the stacked reconcile PR" (web/services/workspace_binding.py, a reconcile route in web/sandbox_manager.py) does not hold for the current repository — neither that file nor any such route exists anywhere in this repo (checked locally across all branches and via GitHub-wide code search). That's a reasonable justification for keeping the type around given stated future plans, but it doesn't change that SandboxMountIntent has zero callers in this PR's own diff today. Not a blocker — just noting the claim isn't independently verifiable yet, and the type's behavior is correct regardless of who ends up calling it.
8. Volume mode byte-comparison with no constraint — WAIVED, and the reasoning is actually stronger than stated. get_sandbox_volumes() (src/xagent/config.py:1580-1583) clamps mode to "ro"/"rw" before it can reach SandboxConfig, confirmed as the sole production path constructing sandbox volume config. More importantly, the observed side of the comparison isn't a raw string from Docker at all — _build_inspection derives it purely from the RW boolean flag (docker_sandbox.py:318), so the failure mode this finding described (an SELinux mode suffix like "rw,z" reaching the comparison) structurally cannot occur on either side. Not a blocker. Minor caveat carried forward: SandboxConfig.volumes still has no type-level constraint enforcing this, so it's a convention rather than a guarantee — low priority.
9. Volume conflict check rejecting a legal Docker mount configuration — still open, correctly logged as deferred (not fixed, not disputed). Confirmed unchanged: _create_container still builds a volumes={host: {...}} dict keyed on host path, and the conflict check still rejects reusing one host path at two guest paths for exactly that reason. The author's characterization ("load-bearing for the current implementation, deferring rather than disputing") matches the code. Fine to leave open; worth a tracking note for whenever docker.types.Mount(...) migration happens.
10. Store row persists raw config while the fingerprint attests the canonicalized form — disagree with WAIVED, recommend keeping this open. The author's rebuttal ("already symmetric, canonicalization is owned by from_parts") is true only for the one comparison that exists in this PR today (spec_matches_inspection, currently zero production callers) — it structurally compares a freshly-from_parts()-normalized desired against the label, so that specific path is fine. But nothing in the code enforces that a future reconciler renormalizes the stored raw config via from_parts() before diffing it against anything else; spec_matches_inspection's own docstring explicitly documents the exact fallback this finding warned about ("fall back to a full desired-state comparison against the record" on UNVERIFIED). Since that reconciler lives in a separate, not-yet-reviewable PR, this can't be confirmed safe yet. Recommend either persisting the canonicalized to_backend_config() output instead of the raw caller input, or explicitly noting in the store-row docstring that any future comparison against it MUST re-normalize first. Not a blocker for this PR (no live bug today), but please don't close this thread as resolved — re-open it when the reconciler PR is reviewed if it doesn't renormalize.
17, 18, 20 (test coverage gaps) — all FIXED and independently verified branch-by-branch. All 7 _find_publish_mismatches branches (image, volumes, ports, working_dir, network_isolated, cpus, memory) now have genuine mismatch-triggering tests plus negative controls (TestFindPublishMismatchesEveryBranch). _build_inspection now has direct unit tests via a fake container stub covering image_digest, runtime_networks, labels, network_isolated, and the full raw_status state-mapping (created/exited/dead/restarting/paused/empty). Both the remove-also-fails and snapshot-attestation-inheritance scenarios (findings 1 and 2) now have dedicated regression tests, confirmed to actually exercise the buggy scenario rather than a weaker proxy.
D1-D5 design concerns — updated status
- D2 (duplicated lock registry) — partially addressed. The new
src/xagent/sandbox/keyed_lock.pyextracts a sharedKeyedLockRegistry, anddocker_sandbox.py's_named_lockis now a thin alias over it (confirmed:docker_sandbox.py:54,1050-1057). However,src/xagent/web/sandbox_manager.py's own_LifecycleLockEntry/_lifecycle_locked(sandbox_manager.py:71,333,377) was not migrated — confirmed unchanged, still a separate implementation. The commit message states this is intentional ("SandboxManager's own lifecycle lock is left as is; it can adopt the shared registry separately"). That's a reasonable incremental step — extracting one copy into a reusable, independently-tested primitive (tests/sandbox/test_keyed_lock.py, 247 lines) is real progress even if the second copy isn't migrated yet — but D2 isn't fully closed. Please track thesandbox_manager.pymigration as explicit follow-up work rather than letting it get forgotten now that the new copy looks "done." - D1, D3, D4 — addressed via documentation, not code changes. New docstrings now explicitly state the previously-implicit contracts: publish-verify is all-or-nothing (D3), the lifecycle methods are reference-count-blind (D4), and there's no mutual exclusion between the new methods and
Sandbox.stop()(ties to finding 4/D1). This is a real improvement — an implicit trap is now an explicit, discoverable one — but none of D1/D3/D4 were structurally closed. That's acceptable for this PR's stated scope (a foundational, purely-additive layer with zero production callers), provided these are genuinely tracked for the reconciler PR that will actually call this API, rather than assumed away because they're now documented. - D5 (surface shipped ahead of its consumer) — partially addressed.
SandboxLeaseSpec(zero callers) was dropped entirely, per the simplification suggestion.SandboxMountIntentwas kept (author disputes it's dead code, citing a not-yet-existing stacked PR — see finding 7 note above) but its bug was fixed, so it's at least now correctly-behaved zero-caller code rather than buggy zero-caller code.
Simplification opportunities — updated
_drop_named_lock_if_unused(previously flagged as an unnecessarily separate method) no longer exists as a standalone unit — absorbed into thekeyed_lock.pyextraction. Resolved as part of the D2 refactor._await_shielded's hand-rolled shield-and-retry loop (previously flagged as astdlibsimplification opportunity) is unchanged. Still a valid, low-priority suggestion; not revisited this round.SandboxLeaseSpec(previously flaggedyagni) — dropped entirely, as recommended.SandboxMountIntent(previously flaggedyagni) — kept, per the author's stated future-consumer plans (not yet verifiable in this repo); its bug is now fixed, so no residual simplification finding here beyond the original suggestion, which the author has explicitly declined for stated reasons.
No new simplification findings from this round's changes — keyed_lock.py is a reasonably tight, single-purpose 103-line module with its own dedicated 247-line test file.
Test run
pytest tests/sandbox/ in the refreshed worktree at the new head: 237 collected, 181 passed, 56 skipped, 0 failures, 0 errors. All skips are Docker/Boxlite-daemon-gated (unavailable in this review environment); CI must confirm those, including the two new real-daemon snapshot-attestation regression tests, which are the most important ones to see pass given they pin the fix for the more serious of the two prior major bugs.
Recommendation
Both previously-confirmed major bugs are fixed with real, non-trivial regression tests, and all test-coverage gaps that mattered most (the publish-verification mechanism the PR's whole safety story depends on) are closed. The remaining open items — finding 4 (documented but not code-fixed, still latent), finding 9 (acknowledged, deferred), and finding 10 (disputed, recommend keeping open) — are all either genuinely non-exploitable today (zero production callers of the affected methods) or low-severity conventions rather than active bugs. None of them block this specific PR's merge given its stated scope as a purely-additive, zero-caller foundation.
This PR looks ready to merge, conditional on: (a) CI confirming the Docker-gated tests (especially the two new snapshot-attestation regression tests) actually pass against a real daemon, and (b) findings 4 and 10 being tracked as explicit follow-up items — ideally referenced in the follow-up PR(s) that will give this API its first real caller — rather than being silently forgotten once this PR merges.
…l rows DockerSandbox.stop() took only the sandbox's own exclusive_access barrier, which drains in-flight operation() work and tracks no exclusive holder. Neither container.stop() nor container.start() registers as an operation(), so a stop could interleave with start_existing/stop_existing/delete/ create_snapshot for the same container name. The handle now receives the owning service's KeyedLockRegistry and takes the same per-name entry before its exclusive_access barrier, in the same order every service lifecycle method uses, so the two guards cannot deadlock against each other. The registry is injected rather than the service, so the handle keeps no reverse dependency on DockerSandboxService. The lock is not reentrant, so no lock-holding path may route through the handle: stop_existing() keeps stopping its container through the raw Container.stop API, the same rule that keeps create()'s compensating cleanup on raw container.remove(). create() now persists the canonical desired state produced by to_backend_config() instead of the caller's raw template/config. The row and the fingerprint label were otherwise two different spellings of one spec, leaving every future reader obliged to re-normalize the row through from_parts before comparing it -- an obligation nothing enforces. The conversion drops no information: SandboxTemplate is exactly (type, image, snapshot_id) and SandboxConfig exactly the seven fields the spec carries. The legacy get_or_create() path writes no attestation label and keeps recording the request verbatim.
|
Three things the inline threads don't cover. #1014 lives on a fork, which is why you couldn't find it. You were right that the consumer claim isn't independently verifiable from this repository — the branch is D1, D2 and D4 land in #1014. That includes the The Docker-gated tests already ran in CI on the commit you reviewed. |
rogercloud
left a comment
rogercloud
left a comment
There was a problem hiding this comment.
Update summary
One new, narrowly-scoped commit since the last review, 811a2f2f ("fix: share one lifecycle mutex with sandbox stop and persist canonical rows", 4 files, ~372 insertions). It closes the two remaining open items from the last round:
- Finding 4 (
start_existing/stop_existingnot mutually exclusive withDockerSandbox.stop()) — previously only documented, now actually fixed. - Finding 10 (store row persisting raw config while the fingerprint attests the canonical form) — previously disputed, now fixed at the value level.
Both were independently re-verified against the code (not the commit message), along with a targeted discovery sweep for any regression this change might have introduced.
Verification
Finding 4 — FIXED, cleanly. DockerSandbox now receives the owning service's KeyedLockRegistry (injected as a plain collaborator, not the service itself — confirmed no reverse dependency on DockerSandboxService anywhere in the class). stop() now acquires the same per-name lock entry before its exclusive_access barrier, in the same lock-then-barrier order every other lifecycle method (create, start_existing, stop_existing, delete, create_snapshot) already used — so the two guards can't deadlock against each other. Verified all 4 DockerSandbox(...) construction sites pass the shared registry (no partial wiring), and verified — independently, via two different search angles — that no existing caller holds the per-name lock while calling .stop() on a handle (the lock is correctly non-reentrant, and the commit's own reasoning that lock-holding paths route through the raw Container.stop/Container.remove API instead of the handle holds up). New regression test TestHandleStopSharesTheServiceLifecycleLock::test_stop_and_stop_existing_cannot_overlap_for_one_name directly falsifies the previously-latent race (asserts waiters==2, serialized execution) — this would fail against the pre-fix code.
Finding 10 — FIXED, at the value level. create() now persists to_backend_config()'s canonical output into the store row instead of the caller's raw template/config; the conversion is lossless (SandboxTemplate/SandboxConfig map field-for-field onto the spec). SandboxInfo's schema is unchanged — this is a change in what value gets written, not a new field. Confirmed no existing consumer is affected: web/sandbox_manager.py never calls the new create() path, only the untouched legacy get_or_create() (which deliberately keeps recording the caller's raw request verbatim — pinned by a new test). Worth flagging one thing from the verification, for the record rather than as a new finding: the author's previous rebuttal on this thread ("already symmetric, no risk") had cited a specific function, SandboxManager._spec_from_stored_info, as proof a future reader would renormalize before comparing — that function does not exist anywhere in this repository; it's apparently in a separate, not-yet-merged branch (referenced as PR #1014). The rebuttal at the time was based on code that isn't here, not just an untested assumption. Not an issue with the current fix (which is solid, with a genuine before/after regression test using a deliberately non-canonical path), just worth noting for calibration on future replies that cite specific external symbols — please confirm such symbols actually exist in this repo before relying on them in a review response, or say explicitly "this exists in stacked PR #N" so it can be flagged as unverifiable-here rather than treated as a settled fact.
Discovery sweep — no new issues found. No new coupling introduced by the registry injection; lock/barrier ordering is consistent everywhere; no external code constructs DockerSandbox directly so the new required constructor param didn't break anything; both new test files are genuine regression tests that would fail if their respective fixes were reverted, not superficial checks. One minor, explicitly non-blocking observation: stop() can now block behind a slow/stuck create()/delete()/etc. for the same name (the lock has no timeout), but this is an existing, already-accepted trade-off shared by every other lifecycle method — stop() is just joining it, not introducing a new one.
Test run: pytest tests/sandbox/ at the new head: 186 passed, 56 skipped (Docker/boxlite-daemon-gated, unavailable in this review environment), 0 failures, 0 errors, confirmed by three independent runs across the verification agents.
Recommendation
Every finding from this review is now resolved: the two confirmed major bugs (publish-verify remove guard, snapshot label laundering) and both previously-open minor items (stop() mutex, store-row canonicalization) are fixed with genuine regression tests; all test-coverage gaps are closed; the remaining waived items (findings 3, 8, 9) hold up under independent scrutiny and are appropriately low-priority or explicitly deferred with a stated reason.
This PR is ready to merge, conditional only on CI confirming the Docker-gated tests pass against a real daemon (in particular the two real-daemon snapshot-attestation tests and the new lock-sharing regression test, none of which could run in this Docker-unavailable review environment).
3 participants
Summary
Adds the desired-state contract and explicit lifecycle API that sandbox reconciliation needs, as a purely additive foundation. No production caller uses the new API yet; existing sandbox behavior is unchanged.
Context: same-CA external tasks fail intermittently with
Sandbox already exists with different runtime configuration(xorbitsai/xagent-saas#296). The root cause is that desired sandbox configuration and actually-running configuration have no shared, verifiable representation: provider cache hits skip validation, restarts adopt existing containers without checking, and stored metadata can mask actual container state. This PR introduces that representation; a follow-up PR movesSandboxManageronto it.What's added
Types (
xagent/sandbox/base.py)ResolvedSandboxRuntimeSpec: canonical desired runtime configuration. Structural equality is authoritative;fingerprint()hashes the same structure.from_parts()applies the same defaults and path normalization the backend applies, so desired and actual state are produced by one normalizer.ObservedRuntimeFacts/SandboxInspection: observation-side types with identity equality only and no fingerprint — observed state is never comparable as if it were desired state. Facts carry raw backend units (NanoCpus, memory bytes) so live edits such asdocker update --cpus 0.5stay observable.SandboxMountIntent: lexical mount classification (covered / covering / disjoint) that normalizes and never raises; consumption policy belongs to the caller.spec_matches_inspection()with a three-state verdict (MATCH/MISMATCH/UNVERIFIED). Containers without a spec label areUNVERIFIED, notMISMATCH: a mismatch verdict is advisory, and destruction decisions additionally require the manager's ref-count safety contract.SandboxContractErrorhierarchy (deliberately notRuntimeErrorsubclasses, so generic runtime-error recovery paths cannot swallow contract violations) and asupports_runtime_spec()capability probe.Docker backend (
xagent/sandbox/docker_sandbox.py)inspect(): side-effect-free snapshot built from actualdocker inspectdata; takes no locks.create(): existence check, snapshot resolution, static volume/port conflict validation, container built from the canonical spec, spec fingerprint + contract-version labels stamped at creation, publish-before-verify (a container whose observed state does not match its own label is removed and the call raises), then the store row. Docker name-conflict races normalize toSandboxAlreadyExistsError.start_existing()/stop_existing(): idempotent, guarded by the per-name lock plus the sandbox's exclusive-access drain, converging the store row unconditionally._named_lock), fixing a pre-existing race wheredelete()recycled a lock while other tasks held references to it, without leaking entries in the unbounded per-task name space._SandboxControlconstruction consolidated behind_get_live_controlfor lock-held paths.Boxlite backend:
supports_runtime_spec()returnsFalse; the lifecycle methods keep the base default (SandboxReconcileUnsupportedError). The Boxlite runtime cannot prove mounts/env/network on reattach, so reconciliation is Docker-only.Behavior compatibility
Legacy paths (
get_or_create,list_sandboxes,delete,create_snapshot,cleanup,warmup,_merge_info) keep their semantics. Three deliberate, bounded exceptions:delete()'s control-map removal is now identity-checked,_create_containergained an optionalextra_labelsparameter (legacy call site passes nothing; the label set without it is unchanged).Testing
tests/sandbox/: 137 passed, 12 skipped (Boxlite-runtime skips), sequentially and with-n 4 --dist=loadscope, against a real Docker daemon. Baseline before this change: 18 passed, 12 skipped.UNVERIFIED) and spec-created containers (MATCH) are told apart.tests/web/ -k sandbox: 157 passed.pre-commit(ruff check/format, mypy, isort, codespell): clean on all changed files.