refactor: split cloud-hypervisor cleanup registry into focused modules#8519
Conversation
There was a problem hiding this comment.
🔵 Needs a closer look
Security-critical crash-recovery cleanup behavior warrants final human and CI validation despite no concrete static-review defects.
Pull request overview
Splits the security-critical Cloud Hypervisor cleanup registry into focused modules while preserving its facade and behavior.
Changes:
- Extracts dependency resolution, persistence, network, and VMM cleanup logic.
- Moves process termination alongside existing process helpers.
- Retains existing public registry interfaces and callers.
File summaries
| File | Description |
|---|---|
cleanup-registry.ts |
Reduces registry to orchestration. |
cleanup-dependencies.ts |
Centralizes dependencies and command helpers. |
cleanup-network.ts |
Encapsulates network validation and teardown. |
cleanup-vmm-identity.ts |
Encapsulates account, group, and ACL cleanup. |
cleanup-record-store.ts |
Encapsulates persistence, claims, mounts, and directory cleanup. |
cleanup-process.ts |
Adds process termination logic. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
🚀 Security Guard has started processing this pull request |
|
🛡️ Smoke Copilot Network Isolation confirmed the egress allowlist is enforced. ✅ Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "example.com"See Network Configuration for more information.
|
|
❌ Smoke Copilot BYOK AOAI (Entra) reports failed. AOAI BYOK (Entra) mode investigation needed...
|
|
✅ Smoke Copilot BYOK completed. Copilot BYOK mode operational. 🔓
|
|
✅ Smoke Claude passed Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "api.anthropic.com"See Network Configuration for more information.
|
|
✨ The prophecy is fulfilled... Smoke Codex has completed its mystical journey. The stars align. 🌟 Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "msfeed25.pkgs.visualstudio.com"See Network Configuration for more information.
|
|
✅ Build Test Suite completed successfully!
|
|
📡 Smoke OTel Tracing completed. All tracing scenarios validated. ✅
|
|
🔌 Smoke Services — All services reachable! ✅
|
|
❌ Smoke Copilot BYOK AOAI (api-key) reports failed. AOAI BYOK (api-key) mode investigation needed...
|
|
📰 VERDICT: Smoke Copilot has concluded. All systems operational. This is a developing story. 🎤
|
|
Chroot tests passed! Smoke Chroot - All security and functionality tests succeeded.
|
|
📰 DEVELOPING STORY: Smoke Docker Sbx reports failed. Our correspondents are investigating the incident...
|
|
❌ Smoke Gemini reports failed. Facets need polishing...
|
Smoke Test: Claude Engine Validation
Overall result: PASS Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "api.anthropic.com"See Network Configuration for more information.
|
|
Smoke Test: Copilot Engine —
Overall: PASS
|
github-actions
Bot
added
the
smoke-copilot-network-isolation
|
✅ Allowed domain (github.com) reachable: Overall status: PASS Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "example.com"See Network Configuration for more information.
|
Smoke Test: Services Connectivity
Overall: PASS
|
|
Smoke Test: Copilot BYOK (Direct Mode) ✅ PASS
Running as
|
✅ Coverage Check PassedOverall Coverage
📁 Per-file Coverage Changes (3 files)
✨ New Files (4 files)
Coverage comparison generated by |
📡 OTel Tracing Smoke Test Results
All scenarios pass or show expected/benign results. No regressions detected in the OTEL tracing integration.
|
🏗️ Build Test Suite Results
Overall: 8/8 ecosystems passed — PASS Notes:
|
Chroot Version Comparison Results
Overall: FAILED — Node.js version mismatch between host and chroot environments. The
|
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "msfeed25.pkgs.visualstudio.com"See Network Configuration for more information.
|
src/cloud-hypervisor/cleanup-registry.tshad grown to 870 lines, with a singleDurableCloudHypervisorCleanupRegistryclass owning network teardown, VMM identity/ACL cleanup, process lifecycle, and record persistence — including a 97-linedeleteVmmIdentitymethod. This splits it along the boundaries the test suite already uses, with no behaviour change.Module layout
cleanup-registry.ts(241 lines) — orchestration facade only:create,createPending,reapPending,reapRecord. Still exportsDurableCloudHypervisorCleanupRegistry,CloudHypervisorCleanupHandle,CloudHypervisorCleanupRegistry,CloudHypervisorNetworkResourceand re-exportsCleanupRegistryDependencies, so none of the 16 callers change.cleanup-dependencies.ts— dependency interfaces,resolveCleanupDependencies(), and the sharedrunCommand/runChecked/pathExists/formatErrorhelpers.cleanup-network.ts—deleteNetwork,validateRecordResources, file/interface identity validation.cleanup-vmm-identity.ts—deleteVmmIdentity, decomposed intoreleaseVmmAcls,deleteVmmAccount,assertRuntimeIdentityUnchanged,deleteVmmGroup.cleanup-record-store.ts— record read/write, registry directory checks, claim locking, virtiofsd unmounting, mount assertions,removeExactDirectory.cleanup-process.ts— gainsstopProcess, next to the existing process identity helpers.Shape of the change
Private methods that only forwarded to
this.dependenciesbecame free functions taking the resolved dependency bag, matching the existing style incleanup-process.ts/cleanup-identity.ts:Error messages, command invocations, and revalidation ordering are preserved verbatim;
deleteVmmAccountreturns the expected gid that the group check previously read from a mutable local. Duplicated private-file writing inwriteRecord/claimRecordis consolidated into onewritePrivateFilehelper ('wx'andO_WRONLY|O_CREAT|O_EXCLare equivalent).