Repository navigation
fix: compose concurrent VMM device ACL cleanup - #9969
Conversation
Track temporary peer VMM ACEs in durable cleanup state. Compose each locked restoration from the workflow baseline plus live peers. Retain that target across account deletion for cleanup-order safety. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: cfb6df6b-e245-428d-9ba7-bfa413e4afa7
There was a problem hiding this comment.
🟡 Changes recommended
The temporary-account classifier can remove a durable ACE belonging to a similarly prefixed non-AWF account.
2 open findings
What changed in this PR
Fixes concurrent Cloud Hypervisor VMM ACL cleanup while preserving durable runner access.
Changes:
- Composes cleanup targets from durable baselines and live peer VMM ACEs.
- Persists temporary peer UIDs for crash recovery.
- Adds deterministic concurrent-cleanup regressions.
| File | Description |
|---|---|
src/cloud-hypervisor/vmm-identity.ts |
Classifies peers and composes ACL restoration. |
src/cloud-hypervisor/vmm-identity.test.ts |
Updates restoration expectations. |
src/cloud-hypervisor/manager-start.ts |
Passes temporary UIDs to cleanup records. |
src/cloud-hypervisor/device-acl.ts |
Adds ACL composition utilities. |
src/cloud-hypervisor/device-acl.test.ts |
Tests both cleanup orders. |
src/cloud-hypervisor/cleanup-vmm-identity.ts |
Applies composed targets during recovery. |
src/cloud-hypervisor/cleanup-vmm-identity.test.ts |
Adds two-identity cleanup regressions. |
src/cloud-hypervisor/cleanup-registry.ts |
Extends cleanup-handle API. |
src/cloud-hypervisor/cleanup-identity.ts |
Adds persisted peer UID validation. |
src/cloud-hypervisor/cleanup-handle.ts |
Stores and releases peer UID metadata. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
@copilot Fix the code for all comments in this review thread. When a review comment includes a suggested change, apply the suggestion exactly. Do not make changes beyond what is described in the linked review thread. |
|
| Metric | Base | PR | Delta |
|---|---|---|---|
| Lines | 93.24% | 93.23% | ➡️ -0.01% |
| Statements | 91.82% | 91.81% | ➡️ -0.01% |
| Functions | 90.59% | 90.55% | 📉 -0.04% |
| Branches | 85.58% | 85.51% | 📉 -0.07% |
📁 Per-file Coverage Changes (7 files)
| File | Lines (Before → After) | Statements (Before → After) |
|---|---|---|
src/cloud-hypervisor/device-acl.ts |
100.0% → 94.4% (-5.56%) | 100.0% → 93.9% (-6.10%) |
src/cloud-hypervisor/cleanup-identity.ts |
91.9% → 90.0% (-1.86%) | 92.1% → 90.3% (-1.81%) |
src/cloud-hypervisor/vmm-identity.ts |
97.5% → 96.1% (-1.41%) | 94.3% → 92.9% (-1.41%) |
src/enclave/host-executor-server.ts |
90.3% → 90.3% (+0.00%) | 88.8% → 89.1% (+0.31%) |
src/cloud-hypervisor/cleanup-handle.ts |
94.1% → 94.3% (+0.22%) | 90.7% → 91.1% (+0.33%) |
src/cloud-hypervisor/cleanup-vmm-identity.ts |
80.8% → 82.3% (+1.53%) | 80.8% → 82.2% (+1.46%) |
src/log-directory-setup.ts |
96.8% → 100.0% (+3.18%) | 96.9% → 100.0% (+3.13%) |
Coverage comparison generated by scripts/ci/compare-coverage.ts
Addressed both review findings and pushed as |
|
✅ Copilot review passed with no inline comments. @lpcox Add the |
|
📰 VERDICT: Smoke Copilot has concluded. All systems operational. This is a developing story. 🎤
|
|
💎 Smoke Gemini is crystallizing results on this pull request... |
|
✅ Build Test Suite completed successfully!
|
|
Smoke Cloud Hypervisor completed. Cloud Hypervisor + Copilot passed.
|
|
🪪 Smoke Copilot BYOK AOAI (Entra) is testing Azure OpenAI BYOK (Entra / GitHub OIDC) mode on this pull request... |
|
🚀 Security Guard has started processing this pull request |
|
✨ The prophecy is fulfilled... Smoke Codex has completed its mystical journey. The stars align. 🌟
|
|
📡 Smoke OTel Tracing completed. All tracing scenarios validated. ✅
|
|
✅ Smoke Copilot BYOK completed. Copilot BYOK mode operational. 🔓
|
|
✅ Smoke Claude passed
|
|
🛡️ Smoke Copilot Network Isolation confirmed the egress allowlist is enforced. ✅
|
|
🔑 Smoke Copilot BYOK AOAI (api-key) is testing Azure OpenAI BYOK (api-key) mode on this pull request... |
|
🔌 Smoke Services — All services reachable! ✅
|
|
Chroot tests passed! Smoke Chroot - All security and functionality tests succeeded.
|
Exercise exact temporary identity classification and fail-closed composition. Cover durable peer UID persistence, validation, release, and both mask shapes. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: cfb6df6b-e245-428d-9ba7-bfa413e4afa7
|
| Metric | Base | PR | Delta |
|---|---|---|---|
| Lines | 93.24% | 93.23% | ➡️ -0.01% |
| Statements | 91.82% | 91.81% | ➡️ -0.01% |
| Functions | 90.59% | 90.55% | 📉 -0.04% |
| Branches | 85.58% | 85.53% | 📉 -0.05% |
📁 Per-file Coverage Changes (6 files)
| File | Lines (Before → After) | Statements (Before → After) |
|---|---|---|
src/cloud-hypervisor/device-acl.ts |
100.0% → 94.4% (-5.56%) | 100.0% → 93.9% (-6.10%) |
src/cloud-hypervisor/cleanup-identity.ts |
91.9% → 90.0% (-1.86%) | 92.1% → 90.3% (-1.81%) |
src/cloud-hypervisor/vmm-identity.ts |
97.5% → 96.1% (-1.41%) | 94.3% → 93.2% (-1.09%) |
src/cloud-hypervisor/cleanup-handle.ts |
94.1% → 94.3% (+0.22%) | 90.7% → 91.1% (+0.33%) |
src/cloud-hypervisor/cleanup-vmm-identity.ts |
80.8% → 82.3% (+1.53%) | 80.8% → 82.2% (+1.46%) |
src/log-directory-setup.ts |
96.8% → 100.0% (+3.18%) | 96.9% → 100.0% (+3.13%) |
Coverage comparison generated by scripts/ci/compare-coverage.ts
Cloud Hypervisor + Copilot Smoke Test
All checks passed.
|
|
Smoke Copilot: PASS ✅
|
Smoke Test: Claude Engine Validation
Overall result: PASS
|
Smoke Test: Copilot BYOK (Direct) Results✅ GitHub MCP: Connected, PR data verified Status: PASS Running as: @lpcox (author/assignee)
|
|
EGRESS_RESULT allow=pass deny=pass
Overall: PASS — @lpcox
|
|
Smoke services: Redis PING ✅, pg_isready ✅, SELECT 1 ✅. Overall: PASS
|
|
OTEL smoke (run 38518783035)
|
✅ Coverage Check PassedOverall Coverage
📁 Per-file Coverage Changes (6 files)
Coverage comparison generated by |
|
9968: test: cover staged mount cleanup failures
|
🏗️ Build Test Suite Results
Overall: 6/8 ecosystems passed — FAIL Failures
Notes
The
|
Chroot Version Comparison
Result: Not all tests passed (Node.js mismatch), so the
|


Summary
Diagnosis
Run
38515513439proved mount cleanup and workload isolation were healthy, while final KVM ACL restoration lost the workflow-owned runner ACE. The two concurrent VMMs used distinct UIDs 999 and 997. Under the shared device ACL lock, a later runtime could capture an earlier runtime's temporary ACE in its durable baseline. Restoring the entire captured ACL made cleanup order observable: one cleanup could erase a still-live peer, while the other could later resurrect an already-cleaned peer or overwrite the runner baseline.This change classifies and records temporary AWF VMM ACEs at capture time. Each locked restoration removes only the current runtime's ACE, strips captured temporary peers that are no longer live, preserves validated live peer ACEs, and restores the exact durable runner baseline. The pre-deletion composed target is retained and re-applied after
userdel/groupdel, preserving fail-closed identity validation and durable recovery.Validation
npm run buildnpm run lint -- --quietnpm test -- --runInBand— 462 suites passed, 8,246 tests passednpm run test:coverage -- --runInBand— passedCampaign follow-up
Do not resume or retry run
38515513439. After this PR merges, dispatch the unchanged Phase 4 campaign as a fresh run with a new campaign ID.