Skip to content

[LXC] State-aware sandbox lifecycle - #849

Open
Darren Hoehna (dhoehna) wants to merge 109 commits into
microsoft:mainfrom
dhoehna:user/dahoehna/lxc-lifecycle-current
Open

[LXC] State-aware sandbox lifecycle#849
Darren Hoehna (dhoehna) wants to merge 109 commits into
microsoft:mainfrom
dhoehna:user/dahoehna/lxc-lifecycle-current

Conversation

@dhoehna

@dhoehna Darren Hoehna (dhoehna) commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

📖 Description

Implements StatefulSandboxBackend for LXC, so a container can be driven through provision, start, exec, stop, and deprovision rather than only the one-shot spawn path.

  • Adds the five lifecycle phases, with start serialized per container under an advisory flock.
  • Adds network policy enforcement in both directions: the egress FORWARD chain in the host netns before the container starts, and the inbound INPUT chain inside the container's own netns once it is running.
  • Adds ContainerPolicy::requires_firewall(), so enforcement follows the policy — a block default, a non-empty host list, or a proxy — and a start with no network section denies by default.
  • Adds lxc to the Node SDK as a state-aware backend, with per-phase config types and LxcNetworkConfig narrowed to the three fields LXC enforces.
  • Adds two E2E scripts run against a live LXC host: the provision-through-deprovision lifecycle, and a network matrix over 15 cases and 16 checked-in configs.

🔍 Validation

  • cargo test --workspace --no-fail-fast — 2,785 passed, 2 failed. Both failures are pre-existing wslc_common tests that require an unlocked D: drive and reproduce at the base commit.
  • npm test (Node SDK) — 283 tests, 277 passed, 0 failed, 6 skipped.
  • cargo fmt --all -- --check and cargo clippy --workspace --all-targets — clean.
  • run_lxc_state_aware_network_test.sh on a live LXC host — 70 passed, 0 failed, 0 quarantined.
  • run_lxc_state_aware_test.sh on a live LXC host — 8 passed, 0 failed.

✅ Checklist

  • Signed the Contributor License Agreement
  • Linked to an issue
  • Updated documentation (if applicable)
  • Updated Copilot instructions (if build, architecture, or conventions changed)
  • If this PR changes Cargo.lock, the dependency-feed-check check passes

📋 Issue Type

  • Bug fix
  • Feature
  • Task
Microsoft Reviewers: Open in CodeFlow

Darren Hoehna (dhoehna) and others added 30 commits July 13, 2026 09:44
…) (AB#62953349)

Provision/start/exec/stop/deprovision for the LXC backend, modeled on IsolationSessionRunner; reuses lxc CLI wrappers and one-shot lxc-attach PTY streaming. Registers the lxc wire key in Rust dispatch/parser and SDK state-aware routing.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 3b78bec0-e139-4cfd-9c10-092ef986d4f4
… + narrow LXC start network type

Restrict is_valid_container_name to the same character set and length bound
(<=20 chars, alphanumeric/-/_) that NetworkIptablesManager::new uses to derive
the per-container iptables chain name. This makes the container-name ->
chain-name mapping an identity on valid names, so distinct names (e.g. 'a.b'
vs 'ab', or names differing only past the 20th char) can no longer collide onto
the same firewall chain and cross-tear-down each other's rules.

Narrow LxcStartConfig.network to Omit<NetworkConfig, 'proxy'> so the SDK rejects
network.proxy at compile time, matching the Rust runner which rejects it at
start (apply_network_policy). Adds Rust + TypeScript tests for both.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…aware_provision.json)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: e5d2aa5b-7f04-4e4d-83d3-a02efe7020ab
Resolved conflicts:
- src/core/lxc/src/main.rs: kept state-aware imports; dropped now-unused ScriptRunner
- src/Cargo.lock: took main's lock, reconciled via cargo metadata

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: e5d2aa5b-7f04-4e4d-83d3-a02efe7020ab
Re-run GitHub Actions after a transient Hyperlight E2E network flake (hyperlight_networking live-HTTP cases timed out after 30s). No source changes; this empty commit only re-fires the pull_request workflows.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: e5d2aa5b-7f04-4e4d-83d3-a02efe7020ab
- Route Lxc state-aware dispatch through mxc_engine::run_state_aware so the
  lxc binary stays a thin CLI shim instead of hand-rolling the backend match.
- Stop/destroy the container before tearing down its iptables rules in stop(),
  deprovision(), and the start() rollback, discovering the veth first so the
  FORWARD hook rule can still be deleted after the device is gone. Closes an
  unrestricted-egress window during teardown.
- Clear lxc.mount.entry before reapplying filesystem mounts so a restart with a
  tightened policy no longer inherits the previous run's bind mounts (new
  LxcContainer::clear_config_item).
- Kill the timed-out child's whole process group and bound the output drain in
  mxc_pty::run_with_pty so a leaked in-container process holding the pty open can
  no longer hang exec forever (new join_with_timeout helper).

Adds unit/regression tests for each fix.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Resolve conflict in wxc_common/src/state_aware_dispatch.rs: register both the `lxc` (this PR) and `wsb` (upstream microsoft#578) state-aware backend prefixes in backend_from_prefix, and keep both resolve_backend unit tests. Added `correlation_vector: None` to the lxc test to match the ParsedStateAwareRequest field introduced upstream (microsoft#624).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…tate-aware-lifecycle

# Conflicts:
#	sdk/node/src/state-aware-helper.ts
#	sdk/node/src/state-aware-types.ts
#	sdk/node/tests/unit/state-aware-types.test.ts
#	src/backends/lxc/common/src/filesystem_mounts.rs
#	src/core/wxc_common/src/state_aware_backend.rs
Resolve conflict in src/backends/lxc/common/src/filesystem_mounts.rs as a
union of both changes:
- microsoft#633 mount-accumulation fix: clear_config_item("lxc.mount.entry") before
  re-deriving the policy's mounts (so a restart replaces, not unions, mounts).
- upstream microsoft#630 denied-dir masking: rebound_container_paths /
  has_rebound_descendant, iterating &mounts.
Both new unit tests (configure_filesystem_mounts_replaces_not_accumulates and
has_rebound_descendant_detects_nested_rebind_only) are kept.

Validated with `cargo check -p lxc_common --tests` (native linux/liblxc, WSL).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: b6b3323b-7297-4b07-9e6e-ab4b220124e6
…art, SDK surface

Five review findings, all cases where the state-aware LXC path reported
success while enforcing less than the caller asked for.

- Hook FORWARD with -i, not -o. Container-originated packets arrive at the
  host on the host-side veth, so egress matches by input interface. `-o`
  matched traffic flowing toward the container, so container egress -- the
  thing the policy exists to restrict -- was never filtered for the whole
  runtime. The teardown `-D` uses `-i` for the same reason, or the hook
  leaks; `force_cleanup` shares that path so signal and stop/deprovision
  cleanup stay consistent.

- Stop start() from failing open. `apply_firewall_rules` treats a
  non-firewall enforcement mode as a successful no-op, and only warns when
  no veth was discovered. Since `enforcementMode` defaults to
  `capabilities` and LXC has no capability-based network enforcement, a
  policy with allowedHosts/blockedHosts/defaultPolicy=block was silently
  unenforced. Start now rejects that combination, and fails when the veth
  cannot be discovered in firewall mode.

- Narrow LxcNetworkConfig to what LXC actually honors. It was
  `Omit<NetworkConfig,'proxy'>`, which still exposed `removeRulesOnExit`
  (SDK-only, and `wire::Network` is `deny_unknown_fields`, so sending it
  fails the whole request) and `allowLocalNetwork` (deserializes, but the
  LXC backend never turns it into a rule). `enforcementMode` is restricted
  to the firewall modes to match the runtime check above. The existing type
  test asserted the old shape and is updated.

- Export the LXC state-aware types from the package entry point. They were
  missing from sdk/node/src/index.ts, unlike the IsolationSession and
  WindowsSandbox equivalents, so consumers could not import them.

- Document the containerId contract. The API doc said state-aware shapes
  never carry containerId; LXC provision does. Documents the adopt-or-create
  behavior and, importantly, that deprovision destroys an adopted container
  too -- MXC keeps no state between phases, so it cannot tell the two apart.
  Adds the missing LXC row to the policy-honor matrix.

Also applies `cargo fmt`, which fixes the failing format check.

Tests: 478 Rust (cargo test -p lxc_common -p wxc_common -p lxc) and 210 SDK
(npm test) pass; clippy on the Linux crates is clean; fmt is clean.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: ea24fae3-d643-4f19-a701-9e31b9f950fd
…xc executors

The lxc executor's state-aware entry point called mxc_engine::run_state_aware
directly, while wxc's wrapped the same call in telemetry init, backend/phase
process attribution, the MS-CV seed/spin plan, the crash panic hook, and the
terminal emit_state_aware event. So a Linux lifecycle produced no lifecycle
telemetry, carried no correlation vector -- provision returned no cV for the
client to relay into later phases -- and installed no crash hook. Every one of
those is invisible at the call site, which is how it stayed unnoticed.

Moves that orchestration into mxc_engine::run_state_aware_with_telemetry and
calls it from both entry points, so the two cannot drift again. Executors keep
their own terminal behavior (buffer flush, stdout envelope, exit code); only
the observability wrapper is shared. The correlation-vector helpers move with
it, along with their six tests -- ported verbatim rather than rewritten, so
coverage is unchanged.

Also fixes a misleading error from exec_state_aware. LXC has no streaming
SandboxProcess, but the fallback arm reported "backend Lxc does not implement
the state-aware lifecycle" -- untrue, since run_state_aware dispatches every
phase for it, and it points at a provision path that works fine. The message
now separates "no lifecycle at all" from "lifecycle but no streaming exec" and
names the API that does work. Streaming exec for LXC is still unimplemented;
this only makes the gap legible.

Tests: 498 Rust on Linux (mxc_engine, lxc, lxc_common, wxc_common), 40 on
Windows (wxc, mxc_engine); clippy clean on both; fmt clean.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: ea24fae3-d643-4f19-a701-9e31b9f950fd
…suite

tests/configs/lxc_state_aware_provision.json was checked in but no script ever
executed it, and run_lxc_all_tests.sh had no state-aware entry at all -- only
one-shot cases. The unit tests stub the container, so nothing exercised
provision -> start -> exec -> stop -> deprovision against a real host: a phase
that was broken on Linux would still ship green. Both other backends already
have such a script (run_isolation_session_state_aware_tests.ps1,
run_windows_sandbox_state_aware_tests.ps1); LXC is the odd one out.

Adds run_lxc_state_aware_test.sh, which relays the provisioned sandboxId
through every later phase the way a real client does, asserts the lxc:mxc-
prefix, and checks that exec relays a nonzero script exit code rather than
swallowing it. Later phases are generated with the sandboxId injected, matching
how the PowerShell suites build requests inline; only provision reads a static
config, so the distribution/release stay in one place.

sandboxId is extracted with sed rather than jq or python, neither of which is
guaranteed on an LXC test host. An EXIT/INT/TERM trap deprovisions on any early
failure or signal, since a leaked container outlives the run and breaks the next
one; the normal path clears the id first so the container is not deprovisioned
twice.

Verified against a stub lxc-exec (no LXC host needed): the happy path passes
8/8 with the sandboxId relayed into start/stop/deprovision, a failing phase is
counted and exits nonzero without double-deprovisioning, and a SIGTERM mid-run
triggers exactly one cleanup deprovision of the right sandbox. bash -n clean,
LF endings so the suite's CRLF guard passes, mode 100755.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: ea24fae3-d643-4f19-a701-9e31b9f950fd
The enforceability gate added in the previous commit used
default_network_policy == Block as evidence that the caller asked for a
restriction. NetworkPolicy::default() *is* Block, so a start with no `network`
block at all produces exactly that value alongside the default
enforcementMode of `capabilities` -- and was rejected. That breaks every plain
start, including the basic lifecycle in run_lxc_state_aware_test.sh, so the
backend advertised a lifecycle its own E2E test could not complete.

Once the wire `network` block is flattened into ContainerPolicy, an explicitly
requested `defaultPolicy: "block"` is indistinguishable from no block at all,
so it cannot be the trigger. Gates on the host lists instead, which are empty
unless the caller populated them -- the same reasoning has_network_policy
already uses to ignore default_network_policy. allowedHosts/blockedHosts under
a non-firewall mode are still rejected, which was the actual fail-open.

Documents the residual gap rather than hiding it: `defaultPolicy: "block"`
alone is not enforced under `capabilities`, and callers who want a default-deny
container must set enforcementMode explicitly.

Adds a regression test built from ContainerPolicy::default() -- the exact
policy a plain start produces -- so this cannot silently come back.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: ea24fae3-d643-4f19-a701-9e31b9f950fd
Two resolutions needed:

- src/core/wxc/src/main.rs: upstream kept the correlation-vector helpers and
  added log_state_aware_dispatch_error next to them; this branch had moved the
  helpers into mxc_engine so the lxc executor could share them. Kept the move
  and kept upstream's new helper and its call site. Mirrored the same
  diagnostic-error routing into the lxc executor, which is the whole point of
  sharing the orchestration -- upstream improved one entry point and the other
  would otherwise have drifted again immediately.

- src/core/wxc_common/src/state_aware_dispatch.rs: upstream added a source_text
  field to ParsedStateAwareRequest and updated every struct literal it could
  see. resolve_backend_for_lxc_prefix_returns_lxc is added by this branch, so
  it was invisible to that sweep and broke the build after a clean textual
  merge. Added the field.

Tests: 558 Rust on Linux, 43 on Windows, 210 SDK; clippy and fmt clean on both
platforms.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: ea24fae3-d643-4f19-a701-9e31b9f950fd
The merge commit c9001a3 swept 1,691 untracked files into the branch:
sdk/node_modules (1,615), sdk/dist (52), and sdk/dist-tests (24). They were
produced by running npm ci / the SDK build locally to collect test numbers,
and none of them are tracked at the merge base (33f3033) or on main.

Untracked with 'git rm -r --cached'; the files stay on disk locally. No
source change.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: ea24fae3-d643-4f19-a701-9e31b9f950fd
The same bad 'git add' in merge commit c9001a3 rewrote this file with CRLF
endings. The repo stores it as LF, core.autocrlf is false, and .gitattributes
only pins *.sh to LF, so git recorded the flip verbatim and the whole file
showed as rewritten: 1707 insertions / 1949 deletions for what is really a
5 insertion / 247 deletion refactor.

Converted back to LF. The diff for this file is now identical with and
without --ignore-all-space. No source change; cargo check and cargo fmt pass.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: ea24fae3-d643-4f19-a701-9e31b9f950fd
configure_filesystem_mounts cleared every lxc.mount.entry line from the
container config on each start, which deleted non-MXC baseline mounts the
distribution template or the operator had placed there.

Tag each MXC-added mount with a marker comment (set_mxc_mount_entry) and
reclaim only marker-tagged entries on restart (clear_mxc_mount_entries),
leaving foreign lxc.mount.entry lines intact. The generic clear_config_item
is retained for other keys and its tests.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
NetworkIptablesManager::new sanitized and truncated the container name to
MXC-<name>, so two containers whose names shared a prefix, or differed only
in characters the sanitizer strips, collapsed onto one chain -- tearing down
one then flushed and deleted the other's rules. This held even though the
name-validation layer bounded lengths, because new() is also reached from
the signal-time force_cleanup path with the raw name.

Fold a deterministic FNV-1a hash of the full, unsanitized name into the
chain name (MXC-<=15 sanitized>-<8 hex>, <=28 chars, within the netfilter
limit). Distinct names now always produce distinct chains, independent of
caller-side validation. Update the container-name rationale comment
accordingly.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
remove_firewall_rules deleted the FORWARD jump only when the manager still
remembered the veth interface it hooked. A teardown that never learned the
veth (signal-time force_cleanup, or a veth that was never discovered) left
the jump installed; the chain then stayed referenced and the following -X
failed, leaking the whole chain across container lifetimes.

Enumerate the live FORWARD chain (iptables -S FORWARD) and delete every rule
that jumps to this chain by its -j target, so the hook is removed whatever
interface it was scoped to. Parsing is factored into forward_hook_deletions
for testability.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The network policy was applied *after* container.start(), leaving a window
(roughly the container's boot time) in which a container with a deny policy
had unrestricted network. The reviewer flagged this as the most serious
finding.

Move the firewall install ahead of start. iptables accepts an interface name
that does not exist yet, so pin a deterministic host-side veth name
(lxc.net.0.veth.pair = mxcv<hash>, reusing the chain-name hash and fitting the
15-char IFNAMSIZ limit) in the container config, build the chain and its
FORWARD hook against that name, and only then start the container -- the veth
comes up already filtered. A firewall-install failure now aborts the start
instead of proceeding fail-open, and a failed start tears the rules back down.

This removes the post-start veth discovery and wait_for_network from the start
path (discovery is still used by stop/deprovision teardown).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
A non-dry-run exec streams the container's raw PTY output directly to the
executor's stdout during backend.exec(). If the dispatch then returns Err, the
JSON error envelope was printed to that same stdout, so a consumer parsing
stdout as JSON saw the envelope glued onto the tail of the raw output.

Capture whether this run is a streaming exec (Phase::Exec && !dry_run) before
parsed is moved into the telemetry-wrapped dispatch, and in the error branch
send the envelope to stderr for that case while every other phase keeps stdout
as its single client-facing channel. Factor the serialisation into
error_envelope_string so the stdout and stderr paths share one builder and the
last-resort fallback, mirroring the wxc sibling executor.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
trap cleanup EXIT INT TERM ran cleanup twice on a signal: once for the signal
handler and once for the EXIT that the shell then fires. The deprovision phase
was issued twice for the same sandbox. Guard cleanup with a CLEANED_UP flag so
the teardown body executes at most once regardless of how many trapped events
fire.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ec gap

The state-aware section listed only isolation_session and windows_sandbox and
omitted lxc, which the engine dispatches on Linux (non-experimental). Add lxc to
both the prose backend-support note and the API-at-a-glance comment, and
disclose the one real limitation: streaming exec (execInSandbox / IPty) returns
unsupported_phase for lxc, so callers must use the non-streaming
execInSandboxAsync. No network policy field shapes are touched.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Recover the presence signal the wire carries but the parser discarded, then
fix the three coupled default-policy defects a reviewer raised on PR microsoft#633.

The wire type `Network::default_policy` is `Option<NetworkPolicy>`, so the
config distinguishes an explicit `defaultPolicy: "block"` (`Some(Block)`) from
no network block at all (`None`). `config_parser` flattened that `Option` into
the non-`Option` `ContainerPolicy::default_network_policy`, whose struct
default is already `Block`, erasing the distinction. `ContainerPolicy` is the
internal lowered representation, not the wire schema: it is not reachable from
`schema_for!(MxcConfig)`, carries no `JsonSchema` derive, appears in no
generated SDK binding, and is never serialized across a process boundary, so
recovering the bit needs no schema change.

Add an additive internal `default_network_policy_present: bool` to
`ContainerPolicy`, set by the parser when the wire value was present. The
struct's existing `#[serde(default)]` keeps the field backward-compatible.

With the bit restored:
- `has_network_policy` now honors an explicit default policy, so a config whose
  only network setting is `defaultPolicy` is recognized as having a policy.
- `requires_firewall_enforcement` now returns true for an explicit
  `defaultPolicy: "block"`, so under a capabilities (non-firewall) mode the
  start is rejected fail-closed instead of running the default-deny unenforced.
- The already-running ("adopted") container path in `start()`, which keys off
  `has_network_policy`, now returns `already_started` instead of silently
  reporting success and bypassing the default-deny.

The absent case (default-constructed policy, presence bit false) is unchanged:
a plain start with no network block is still not rejected.

Updates the one test whose premise this change invalidates
(`default_policy_alone_does_not_require_firewall_enforcement`, renamed to
`explicit_default_block_requires_firewall_but_absent_or_allow_does_not`) to
assert the new distinction while keeping the absent-block invariant.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Black-box spec tests derived from the chain_name_for / name_hash
contract.  The implementation file was never read; every assertion
traces to a quoted contract clause.

Properties covered:
* Injectivity (collision-freedom) over a 200+ name adversarial corpus
  including the two named families from the contract: shared prefix
  past the truncation point, and names differing only in
  sanitizer-stripped characters.
* Length bound ≤ 28 characters, asserted over the same corpus.
* Shape: MXC- prefix + ≤ 15 sanitized chars + - + 8 hex digits.
* Determinism: repeated calls return identical results; FNV-1a
  regression pins lock the hash values across builds.
* name_hash covers the full unsanitized name (hashes differ for
  inputs that sanitize identically).
* NetworkIptablesManager::new stores chain_name consistent with
  chain_name_for.

Mutation test results (all caught):
1. Hash zeroed via AND 0 at truncation point — 4 failures.
2. Hash computed over sanitized name (strip non-alnum/dash) — 3 failures.
3. Hash truncated to 4 hex digits — 1 failure.
4. Sanitized segment widened past 15 chars (.take(20)) — 4 failures.
5. FNV prime bumped by XOR 1 — 2 failures.

lxc_common test count: 70 → 81 (+11).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Finding 1 (Medium) -- documentation stated the opposite of the code.
The paragraph at mxc-state-aware-sandbox-api.md:1616 said
defaultPolicy: "block" was indistinguishable from an absent policy
and would NOT be enforced under capabilities.  e3e657a inverted that:
the parser now records default_network_policy_present so an explicit
block IS distinguishable, and requires_firewall_enforcement returns
true for it.  Rewrote the paragraph to describe actual behavior
verified from state_aware.rs:158-163 and :227-233.

Finding 2 (Low) -- rejection message named only allowedHosts/blockedHosts.
A caller rejected solely for defaultPolicy: "block" received a message
that mentioned only allowedHosts/blockedHosts.  Extended the message to
name the explicit default policy as an additional trigger alongside the
host lists while keeping the existing voice and error type.

Finding 3 (Low) -- parser assignment for the presence bit was untested.
Existing parser tests asserted only the flattened NetworkPolicy value,
not default_network_policy_present.  Added three end-to-end parser tests:
absent defaultPolicy -> presence false; explicit "block" -> presence
true + value Block; explicit "allow" -> presence true + value Allow.

Mutation proof: deleting the single assignment
  policy.default_network_policy_present = true;
from config_parser.rs with anchor count=1 produced 562 passed / 2 failed
(block_sets_presence_true and allow_sets_presence_true).
Restore confirmed byte-identical.

wxc_common test count: 561 -> 564 (+3).
lxc_common test count: 81 (unchanged).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The iptables chain/veth name derivation truncated the FNV-1a hash to its
low 32 bits, so two attacker-chosen container names could collide onto a
byte-identical chain (proven: "web-frontend-017m3b" and "web-frontend-01kgar"
both -> "MXC-web-frontend-01-3d4a49a5", found in 793,379 candidates).  A
teardown then flushes and deletes the incumbent container's chain and FORWARD
hook, leaving it running with no firewall -- fail-open.

Retain the full 64-bit FNV-1a hash and encode it as a fixed 11-char base36
token (hash mod 36^11, ~2^56.9), shared by both names:
  chain = "MXC-" + <=12 sanitized + "-" + 11 base36 = 28 chars (netfilter)
  veth  = "mxcv" + 11 base36                        = 15 chars (IFNAMSIZ)
The sanitized allowance shrinks 15 -> 12 to fit the wider token.  Determinism
is preserved (no RandomState/DefaultHasher) so force_cleanup reconstructs the
same names cross-process.

Adversarial collision search moves from ~2^32 (sub-second) to ~2^56.9
(infeasible); a 2,000,000-name shared-prefix sweep now yields zero collisions.
This is collision-resistant, not injective -- corrected the five places that
claimed "always distinct" / "collision-free" / "injectivity" to say so.

Tests: converted the collision proof into a regression test, added length-bound,
shape, exact-string cross-process determinism, and 64-bit hash pins; renamed the
former "injectivity" test to state it checks a near-miss corpus only.  All 5
mutants of the derivation are caught.  lxc_common 81 -> 88 passed / 0 failed /
0 ignored; wxc_common 564 passed unchanged.  No schema files touched.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The hash_token comment stated 36^11 = 131_601_804_755_189_760.  The
correct value is 131_621_703_842_267_136.  The code is unaffected --
MODULUS is computed as 36u64.pow(11), so only the comment was wrong --
but a reviewer checking the width argument against the stated number
would have found it did not add up.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 52fb9e8c-4a48-435c-9964-8ec0fee653c6
The doc comment on chain_name_for stated that finding a collision requires
~2^56.9 work and was infeasible to search adversarially.  That conflated
second-preimage with collision resistance.  36^11 is ~56.87 bits, so the
generic birthday work to find some colliding pair is ~2^28.4, and FNV-1a is
non-cryptographic - every step is a bijection, so it inverts rather than
needing a search.

A caller that picks its own containerId can therefore construct a colliding
pair and make teardown of one name remove the other's chain.  Defending
against that needs persisted ownership verification, not a wider hash.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 52fb9e8c-4a48-435c-9964-8ec0fee653c6

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 54 out of 55 changed files in this pull request and generated 1 comment.

Suppressed comments (3)

tests/scripts/run_lxc_state_aware_network_test.sh:743

  • This case never verifies that allowedHosts produces an ACCEPT rule: it only checks that the chain is hooked and ends in DROP, which is identical to default-deny with the allow list ignored. Add a positive connectivity check to a controlled allowed endpoint (plus a denied control), or inspect the destination-specific ACCEPT rule; case 12 has the same gap.
    tests/scripts/run_lxc_state_aware_network_test.sh:305
  • A failed deprovision still clears SANDBOX_ID, so the EXIT trap cannot retry cleanup and the container leaks into later cases/runs. Preserve the ID unless deprovision succeeds, as the lifecycle test already does in tests/scripts/run_lxc_state_aware_test.sh:168-175.
    tests/scripts/run_lxc_state_aware_network_test.sh:793
  • This blocklist assertion is vacuous because omitting defaultPolicy inherits default block, so evil.example.com is denied even if blockedHosts is completely ignored. Exercise the list under defaultPolicy: allow and verify both the blocked destination and an allowed control, or inspect the explicit destination DROP rule.

Comment thread tests/scripts/run_lxc_state_aware_network_test.sh Outdated
LXC enforces network policy from the policy itself and the default is deny-all,
so every LXC run now installs an egress chain even when the request carries no
network section.  A bridged veth only reaches FORWARD while br_netfilter
delivers bridged packets to iptables, and the SDK integration lanes never
enabled it, so the backend correctly refused to report success for a policy it
could not enforce:

  Container veth vethfPaRw9 is attached to a bridge but bridged packets are not
  delivered to iptables (/proc/sys/net/bridge/bridge-nf-call-iptables is absent
  or 0), so chain MXC-lxc-pro-... could never be reached from FORWARD.

Every LXC test failed at the backend probe with exit 255, across all three
schema versions.  The dedicated LXC E2E workflow already does this
(lxc-e2e.yml:44-48) and is green, so this is the same two sysctls on the lane
that was missing them.

Its `iptables -P FORWARD ACCEPT` step is deliberately not copied.  That exists
so the deny cases cannot pass vacuously under Docker's DROP policy, and these
lanes skip the network-dependent LXC tests entirely.

The Azure Pipelines lane has the same gap and gets the same fix, tolerated
rather than required, because a hosted pool may forbid loading modules.  Where
it does, the LXC tests fail exactly as they already would.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 516b2545-8130-4af2-b5e8-e9f6c6d591ff

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 56 out of 57 changed files in this pull request and generated no new comments.

Suppressed comments (2)

sdk/node/src/state-aware-types.ts:140

  • FilesystemConfig also exposes the SDK-only clearPolicyOnExit field. buildStateAwareEnvelope forwards this object unchanged, while Rust's wire::Filesystem uses deny_unknown_fields and has no such member, so a configuration accepted by this new public type fails at runtime as malformed. Narrow the filesystem type just as the network type is narrowed.
  filesystem?: FilesystemConfig;

tests/scripts/run_lxc_state_aware_network_test.sh:307

  • The sandbox ID is cleared even when deprovision fails, which disarms the EXIT trap and can leave a container behind. The same issue occurs for SANDBOX_STARTED: a failed stop is recorded as stopped, so cleanup will not retry it. Retain both states until their corresponding phase succeeds.

The state-aware API doc tells a new backend to add its typed fields to the
experimental block of the wire model and regenerate the schema (section 11.5
of docs/state-aware-lifecycle/mxc-state-aware-sandbox-api.md).  The LXC
state-aware work skipped that step, so experimental.lxc was the only backend
section absent from wire::Experimental: isolationSession, wslc, seatbelt,
windowsSandbox, telemetry, and test all had typed fields and LXC did not.
An experimental.lxc payload was therefore silently ignored during config
parsing rather than type-checked like every peer's.

Both fields are Option<String>, matching the stable top-level lxc section and
both peers' provision phases.  Requiredness stays in the backend, which
already rejects a provision that omits either field, because the experimental
block is intentionally permissive and the schema is an editor and CI
convenience rather than the gate (docs/schema-codegen.md).

Adds two E2E tests: one for the field contract, and one proving a request that
names two containment backends is refused.  The non-string payload is written
to a temp file rather than committed under tests/configs, because a committed
fixture that fails schema validation would need an entry in
config-validation-exemptions.json and no other backend has one.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 7ce509eb-6712-4a1a-8b7f-5e55d51cf868

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 63 out of 65 changed files in this pull request and generated 1 comment.

Comment thread src/core/wxc_common/src/models.rs Outdated
Two resolutions, one textual and one the merge did not surface.

state_aware.rs: main renamed exec_experimental_backend_requires_flag to
_requires_optin, and this branch had inserted a new test immediately above
it.  Adjacency only -- both tests are kept.

one_shot.rs: main added a new one-shot adapter that constructs
wire::Experimental, and this branch had given that struct an lxc field.
Neither side touched the other's text, so git merged clean and the tree
did not compile (E0063).  lxc is None there because experimental.lxc is
state-aware-only, matching isolation_session and seatbelt in the same
function; contract::OneShotExperimental is deny_unknown_fields and has no
lxc member to map.

Tests pin that seam so it cannot be reintroduced silently: the three
one-shot mapping tests now assert lxc is absent, and a new test asserts a
one-shot request carrying experimental.lxc is rejected rather than ignored.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 7ce509eb-6712-4a1a-8b7f-5e55d51cf868

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 63 out of 65 changed files in this pull request and generated no new comments.

Suppressed comments (2)

sdk/node/src/state-aware-helper.ts:40

  • The SDK adds LXC to the state-aware version/prefix registries and envelope shape, but sdk/node/tests/unit/state-aware.test.ts has runtime envelope/routing coverage for the other backends and none for LXC. Add tests that provision emits top-level containerId plus nested experimental.lxc.provision, and that subsequent lxc: phases route correctly; otherwise a registry or lifting regression can pass the current compile-time-only checks.
  lxc: STATE_AWARE_VERSION,

tests/scripts/run_lxc_state_aware_network_test.sh:305

  • A failed deprovision is still followed by clearing SANDBOX_ID, which disarms the EXIT trap's retry and can leak the container/firewall state into subsequent cases. Preserve the ID unless deprovision succeeds, as the lifecycle script already does.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 65 out of 67 changed files in this pull request and generated 1 comment.

Comment thread src/backends/lxc/common/src/lxc_runner.rs Outdated
The removed blocks explained how the mechanism works, which the code already
shows and which rots against it.  What is left states only the constraint a
reader cannot derive from the code itself.

The proxy rejection message keeps the part a caller can act on.  The argv
exposure it used to describe belongs in the review thread, not in output the
user reads at a failure.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 7ce509eb-6712-4a1a-8b7f-5e55d51cf868
A one-shot run started the container and waited for networking before it
applied firewall rules, so container init and early services could transmit
during an interval MXC already reported as deny-all.

iptables accepts a rule naming an interface that does not exist yet, and the
start-host hook runs after liblxc creates the veth pair but before the
container's init execs.  A container with a single veth therefore pins a
deterministic name and installs its chain ahead of start.

Shapes the hook cannot pin (no interfaces, several interfaces, a non-veth
interface, or an already running container) keep the existing post-start
path, so nothing that works today becomes a refusal.

The networking wait now runs after start on both paths.  The workload
attaches afterward, so skipping it could launch the script before the
container has an address.

set_preserve_policy moves after both paths because a pre-start install
followed by a failed start would otherwise leak rules for a container that
never ran.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 7ce509eb-6712-4a1a-8b7f-5e55d51cf868

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 65 out of 67 changed files in this pull request and generated 2 comments.

Suppressed comments (2)

src/backends/lxc/common/src/signal_cleanup.rs:405

  • A signal can arrive after the egress chain is created but before container.start(). In that state lxc-stop -k reports failure because the container is already stopped, so execute_rollback aborts before RemoveFirewall and strands the chain; subsequent starts then fail on the existing chain. Treat a positively stopped container as a successful stop step, while still failing closed when the state probe is unknown.
                RollbackStep::StopContainer => LxcContainer::new(&name, None).kill().is_ok(),

tests/scripts/run_lxc_state_aware_network_test.sh:305

  • The sandbox ID is cleared even when deprovision fails, so the EXIT trap cannot retry cleanup and the failed case leaks its container/firewall state onto the test host. Keep the ID until deprovision succeeds, matching the lifecycle test's cleanup behavior.

Comment thread src/backends/lxc/common/src/state_aware.rs
Comment thread src/backends/lxc/common/src/lxc_runner.rs Outdated
The merge in 669c80e rewrote this file with LF endings while main keeps
CRLF for it.  Every line then differed from main, so the pull request
reported 1487 insertions and 1383 deletions for a change that touches 239
and 135, and merging main back conflicted across the whole file instead of
on the six lines main actually added.

Content is byte-identical; only the line terminators change.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 7ce509eb-6712-4a1a-8b7f-5e55d51cf868
…ifecycle-current

Main's new state-aware contract adapter builds wire::Experimental in seven
places, and this branch gives that struct an lxc field, so the merged tree
did not compile until each site named it.  Every one of those converters
serves a different backend and sets the ones it does not own to None, and
main's state-aware provision contract has no LXC variant, so None is what
the pattern calls for rather than a placeholder.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 7ce509eb-6712-4a1a-8b7f-5e55d51cf868

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 66 out of 68 changed files in this pull request and generated no new comments.

Suppressed comments (3)

src/backends/lxc/common/src/state_aware.rs:948

  • A concurrent exec can pass is_running() immediately after container.start() and run untrusted code before this ingress policy is installed (or while installation is about to fail). The lifecycle lock excludes stop/deprovision but exec does not participate, so successful start is not an atomic state transition from an exec caller's perspective. Use shared locking for exec and exclusive locking for lifecycle transitions, or persist/check a fully-started marker before allowing attach.
            // Inbound enforcement lands only once the container's network
            // namespace exists, so unlike the egress chain it comes after start.
            if let Err(e) = apply_ingress_policy(&container, container_name, request, &mut logger) {

src/backends/lxc/common/src/lxc_runner.rs:379

  • On ingress setup failure, an adopted container that this invocation started with destroyOnExit=false is left running because these branches only destroy newly owned/disposable containers. It therefore survives a failed run without the required INPUT policy (the missing-PID branch below has the same issue). Track whether this invocation started the container and stop/kill it before returning any post-start policy error; preservation should avoid destroying the container, not leave a failed start running.
                match mgr.apply_firewall_rules(&request.policy, logger) {
                    Ok(true) => {}
                    Ok(false) => {
                        if self.destroy_on_exit || container_created {
                            let _ = container.destroy();
                        }
                        return ScriptResponse::error(
                            "Failed to apply inbound network firewall rules.",
                        );
                    }
                    Err(e) => {
                        if self.destroy_on_exit || container_created {
                            let _ = container.destroy();

src/core/wxc_common/src/wire.rs:590

  • Adding this typed field makes one-shot lxc-exec accept experimental.lxc.provision and silently ignore its distribution/release. The CLI now parses both paths through load_mxc_request; its one-shot conversion consumes the other experimental fields but neither maps nor rejects raw_exp.lxc. The adapter-only rejection test does not exercise that path. Explicitly reject this state-aware-only section during one-shot conversion and add a load_mxc_request regression test.

A one-shot run with `destroyOnExit` off on a container it did not create
returned from a failed post-start step while the container kept running,
and the firewall managers then dropped their rules on the way out.  That
left a live container with its egress filtering removed, reported to the
caller as a failure.  All six post-start failure sites now halt it, and
retain its filtering when the halt itself fails.  LXC is the only backend
that can reuse a sandbox it did not create, so no sibling shares this.

Refuse a container with several configured interfaces when a firewall is
requested.  The FORWARD hook matches a single veth, so traffic on the
others never reaches the chain.  A container with no usable veth already
fails when the rules are applied, because nothing calls
`allow_missing_veth_interface` on this path, so only the multi-interface
shape needed a new refusal.

Stop the signal watchdog stranding a chain when `lxc-stop -k` exits
non-zero because the container was already down.  A stranded chain blocks
every later start of that name.

Stop the state-aware network test script deleting pre-existing host
directories it did not create.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 7ce509eb-6712-4a1a-8b7f-5e55d51cf868

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 66 out of 68 changed files in this pull request and generated 1 comment.

Comment thread src/backends/lxc/common/src/lxc_bindings.rs
Extracting stop_left_container_down placed it between the
#[cfg(target_os = "linux")] attribute and run_watchdog, so the gate
attached to the new function and left run_watchdog unconditional.  It
references SigSet, thread, and LxcContainer, all imported only on Linux,
so every Windows job failed to compile while Linux stayed green.

The helper now uses the dead-code pattern the rest of this file already
uses, keeping it compiled on every target so Windows and macOS CI still
type-check and run its tests.

Verified with cargo clippy --workspace --all-targets --target
x86_64-pc-windows-msvc -- -D warnings, which is clean, and the Linux
suite still at 1856 passed / 0 failed.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 7ce509eb-6712-4a1a-8b7f-5e55d51cf868
…hread

The comment justified abandoning the reader by asserting the executor
process exits right after a state-aware exec, so the OS reaps the thread.
That holds for the one-shot CLI, but mxc-sdk and mxc_ffi call this
in-process and keep running, so the thread and its pty fd survive until
the host exits.

Comment only.  The bounded grace itself stays: upstream's plain join can
block forever when a process that escaped into its own session still
holds the pty secondary open, which would defeat the timeout that was
just enforced.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 7ce509eb-6712-4a1a-8b7f-5e55d51cf868

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 66 out of 68 changed files in this pull request and generated 1 comment.

Suppressed comments (4)

src/backends/lxc/common/src/lxc_runner.rs:299

  • pinnable == false still starts the container before applying the firewall. An adopted container with one macvlan/phys interface takes this branch, can transmit during startup, and only afterward fails because live_veth_interface recognizes veth names only. Reject any non-pinnable topology before container.start() whenever the policy requires a firewall, as the state-aware path already does.
        if !running && request.policy.requires_firewall() {
            let pinnable = net_config
                .as_ref()
                .map(Self::pinnable_before_start)
                .unwrap_or(false);
            if pinnable {

src/backends/lxc/common/src/lxc_runner.rs:339

  • Now that egress may already be installed, a nonzero lxc-start result is not safe to handle by immediately returning. As the state-aware path notes, start can fail after the container is already live; if destroy is skipped or the ignored destroy fails, fw_manager then drops and removes the chain, leaving that container running unfiltered. Probe the resulting state and kill/stop it before allowing firewall cleanup; preserve the rules if shutdown cannot be confirmed.
            if let Err(e) = container.start() {
                if self.destroy_on_exit || container_created {
                    let _ = container.destroy();
                }
                return ScriptResponse::error(&format!("Failed to start container: {}", e));

src/core/wxc_common/src/wire.rs:590

  • Because wire::MxcConfig is shared by both request shapes, this new typed field also makes experimental.lxc valid on one-shot requests. The production one-shot path deserializes directly into MxcConfig (config_parser.rs:246-252) and convert_wire_config never consumes or rejects raw_exp.lxc (config_parser.rs:1249-1340), so a caller's requested image is silently ignored. The added adapter test does not cover production parsing; explicitly reject this section for one-shot requests or use request-specific wire types.
    tests/scripts/run_lxc_state_aware_network_test.sh:310
  • This clears SANDBOX_ID even when deprovision fails, which disarms the EXIT trap's retry and leaks the container/firewall into later matrix cases. Capture the return code and only release the ID after successful deprovision, matching run_lxc_state_aware_test.sh:168-175.

Comment thread src/backends/lxc/common/src/lxc_runner.rs
Brings in three commits: CI validation infrastructure fixes including LXC
bridge NAT (b0b66cb), bubblewrap support for hostname proxy endpoints
under proxy-only egress (33f688f), and new bubblewrap prerequisites plus
the isolation session OS version (43a781c).

No conflicts.  None of the sixteen files main changed is an LXC source
file, so the only contact with this branch's subject matter is CI host
preparation: prepare-linux-host.sh replaced report_lxc_network_diagnostics
with ensure_bridge_nat, which installs a MASQUERADE rule for the bridge
subnet rather than only reporting the bridge address, NAT rules, and
lxc-net configuration.  The LXC end-to-end jobs therefore run against a
host network that is set up differently than before this merge.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 7ce509eb-6712-4a1a-8b7f-5e55d51cf868

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 66 out of 68 changed files in this pull request and generated 2 comments.

Comment thread src/backends/lxc/common/src/state_aware.rs
Comment thread tests/scripts/run_lxc_state_aware_network_test.sh Outdated
Brings in isolation session appId support on provision (76a08fc, microsoft#802).

No conflicts.  Unlike the previous merge this one reaches shared surface
rather than a sibling backend: wxc_common models.rs and wire.rs both
changed, and LXC's state-aware path depends on them, so the branch is
rebuilt and retested here rather than relying on the earlier verification.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 7ce509eb-6712-4a1a-8b7f-5e55d51cf868
finish_current_sandbox cleared SANDBOX_ID unconditionally, but `check`
records a failure without aborting, so a deprovision that failed still
released the ID.  That disarms the EXIT trap, which is the only thing that
retries the teardown, and the container and its firewall chain then survive
into every later case in the matrix -- each of which asserts against host
network state.

run_lxc_state_aware_test.sh already releases the ID only on a zero return
for this reason.  The matrix makes the consequence worse here, because one
leaked container contaminates the cases that follow rather than a single
subsequent run.

Raised by Copilot review on PR microsoft#849.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 7ce509eb-6712-4a1a-8b7f-5e55d51cf868

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 66 out of 68 changed files in this pull request and generated 2 comments.

Comment on lines +314 to +316
if [ "$deprovision_rc" -eq 0 ]; then
SANDBOX_ID=""
fi
/// IsolationSession backend config (Windows).
pub isolation_session: Option<IsolationSession>,
/// LXC backend config (Linux).
pub lxc: Option<LxcExperimental>,
The predicate lived on `ContainerPolicy` in wxc_common, the core crate every
backend depends on, but it describes what the iptables firewall must install
rather than a property of the policy.  Nothing outside `network_iptables.rs`
and its two callers in this crate ever read it, and nothing outside `src/`
referenced it at all.

It started in the right place.  287b918 introduced it as a private free
function in `state_aware.rs`; b78cb95 promoted it to a public method on the
shared type while changing what it decided, and the doc comment it carried
there read "True when the policy requires the LXC egress firewall to install
rules" -- a shared-core method documented in terms of one backend.  b5d5cd6
later trimmed that comment away, leaving undocumented public API on the core
policy type with no caller outside LXC.

The neighbouring `needs_host_filtering` stays where it is: wslc and the core
config parser both call it, so it earns its place on the shared type.

Now a `pub(crate)` free function beside `apply_firewall_rules`, its only
consumer.  Bubblewrap is unaffected -- it reaches the predicate through
`NetworkIptablesManager::apply_firewall_rules`, never by name.

No behavior change.  The expression is identical and the same 1,999 tests
pass.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 7ce509eb-6712-4a1a-8b7f-5e55d51cf868
Resolves three conflicts, plus two breakages the textual merge hid.

mxc_engine/src/state_aware.rs: main added a feature-off IsolationSession
arm to exec_state_aware, and this branch had replaced the generic fallback
with exec_unsupported_error, which tells "no state-aware lifecycle at all"
apart from "has the lifecycle but no streaming exec" -- the LXC case.  Both
are kept: main's arm is the more specific match, so it precedes the
fallback.

mxc_engine/src/lib.rs: union of main's exec_state_aware_attached export and
this branch's run_state_aware_with_telemetry.

.github/copilot-instructions.md: keeps this branch's engine bullet, which
names LXC and WSLc among the backends run_state_aware dispatches, and
main's SDK bullet, which documents exec_attached.

The merged tree then failed to build for two reasons git could not see:

- ExecHandle gained a stdin_closer field.  LXC exposes no stdin -- its
  handle is all null pipes -- so it supplies None, for the same reason
  WSLc does.
- The shared exec refusal was reworded upstream.  LXC's test still asserted
  the old substring; main had already moved the WSLc and Windows Sandbox
  tests to the new one.

Verified: fmt clean, clippy clean on Linux (5 crates) and on
x86_64-pc-windows-msvc (5 crates, covering every Windows backend), and
2136 tests passed with 0 failed.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 65 out of 67 changed files in this pull request and generated no new comments.

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

tests/scripts/run_lxc_state_aware_network_test.sh:316

  • Retaining the ID on failure is not enough because this function returns and the next case's start_fresh_sandbox overwrites SANDBOX_ID. The EXIT trap then loses the only reference to this sandbox and cannot retry its cleanup. Abort immediately on failed deprovision so the trap retries the same ID (or track all outstanding IDs).

src/core/wxc_common/src/wire.rs:590

  • Adding this field makes experimental.lxc deserialize successfully on the production one-shot path, but convert_wire_config neither consumes nor rejects raw_exp.lxc, and present_backend_sections does not count it. A one-shot request can therefore specify a provision image here, be accepted, and then silently run with the top-level LXC defaults. The new contract-adapter test does not exercise the production parser; explicitly reject this field during one-shot conversion and add a production-parser regression test.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 65 out of 67 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/backends/lxc/common/src/state_aware.rs:665

  • Returning an unlocked guard when the LXC root is absent breaks the provision critical section. provision calls this same helper before its is_defined/create sequence, so two first provisions on a host with no root can both observe an unused name and concurrently run lxc-create. Keep stop/deprovision idempotent, but ensure provision locks in a durable parent before probing or creating the container.
                    Err(e) if e.kind() == std::io::ErrorKind::NotFound => {
                        return Ok(LifecycleLock { _guard: None })
                    }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Copilot-Instructions PR modifies Copilot instruction files (.github/copilot-instructions.md or .github/instructions/) Needs-Attention Issue needs attention from Microsoft

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants