[Steven Malis (smalis-msft)]

Add a new drop harness that inspects the VM on test failure, and a post test hook to save its output. This will run on any failure: timeouts, panics, or error returns, so it replaces the previous watchdogs.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Note

This error may be related to your runner configuration. You can now configure runners for Copilot code review separately from Copilot cloud agent by creating a copilot-code-review.yml file with your setup steps. Read the docs for details.

[Copilot]

Pull request overview

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

[Copilot]

🟡 Changes recommended

Teardown errors can miss diagnostics, while successful drops can unnecessarily trigger expensive capture; test coverage is also needed.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

petri/src/vm/mod.rs:481

• The new diagnostics path depends on unwinding, Drop, detached-task draining, and post-test hook ordering, but no test exercises it. Add a fake runtime/inspector test that verifies panic and Err exits populate the failure attachment, clean teardown suppresses capture, and an inspect timeout still lets the pool and hooks finish; otherwise regressions in this central failure-only path will only surface during an actual VMM test failure.

impl<T: PetriVmRuntime> Drop for PetriVmRuntimeGuard<T> {
    fn drop(&mut self) {
        let (Some(runtime), Some(snapshots)) = (self.runtime.take(), self.snapshots.clone()) else {

• Files reviewed: 1/1 changed files
• Comments generated: 2
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Diagnostic capture must cover teardown failures and be validated with focused tests.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

petri/src/vm/mod.rs:1904

• Taking the runtime here suppresses the guard before the backend teardown has succeeded. If PetriVmRuntime::teardown returns an error (for example, OpenVMM can fail while acquiring/shutting down the worker and Hyper-V can fail removing the VM), self is then dropped with runtime: None, so this error-return path produces no inspect attachment despite the PR's stated coverage of error returns. Preserve a diagnostic-capable runtime/inspector until teardown succeeds, or capture diagnostics when teardown fails.

        self.runtime.take().teardown().await

• Files reviewed: 1/1 changed files
• Comments generated: 1
• Review effort level: Balanced

[github-actions]

2 Petri tests failed (1 unstable)

[Copilot]

🔵 Needs a closer look

Teardown failures bypass inspection capture, and the new lifecycle behavior lacks focused tests.

Review details

Suppressed comments (2)

petri/src/vm/mod.rs:1924

• Taking the runtime out before awaiting teardown disables failure capture for the teardown operation itself. If backend teardown returns an error, or a still-active watchdog unwinds a direct teardown() call, the guard later sees runtime == None and saves no inspect output, even though the test failed. Keep the runtime guarded until teardown has completed successfully; the consuming backend teardown contract may need to be changed so the runtime remains available on failure.

        self.runtime.take().teardown().await

petri/src/vm/mod.rs:478

• This drop/pool/hook interaction is the core behavior of the PR, but the existing tests in this module cover unrelated helpers only. Please add a fake runtime/inspector test that verifies an unclean drop populates the buffer before post-test processing, a clean take suppresses capture, and panic/error paths do not replace the original test failure; these cases are especially important because field-drop and executor-lifetime changes can silently stop producing attachments.

impl<T: PetriVmRuntime> Drop for PetriVmRuntimeGuard<T> {
    fn drop(&mut self) {
        let (Some(runtime), Some(snapshots)) = (self.runtime.take(), self.snapshots.clone()) else {

• Files reviewed: 1/1 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[github-actions]

6 Petri tests failed (1 unstable)

[Copilot]

Copilot review overview

🟡 Changes recommended

Retaining OpenVmmInspector prevents normal OpenVMM teardown by keeping an extra worker reference alive.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1

Open (1)

• Inspector Arc prevents OpenVMM worker teardown · New

Resolved since last review (3)

• The core diagnostic path is untested: no test exercises a VM/runtime guard being dropped on an…
• Taking the runtime before awaiting teardown suppresses the new failure capture even when…
• The guard captures diagnostics on every non-teardown drop, not only on failures. The standard…

[Copilot]

Copilot review overview

🟡 Changes recommended

Successful VM drops can be misclassified as failures, while teardown failures cannot retain the runtime needed for inspection.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1

Open (1)

• Teardown errors lose VMM inspection diagnostics · New

Resolved since last review (1)

• Inspector Arc prevents OpenVMM worker teardown

Previously missed (1)

In code that hasn't changed since last review

Live VM drops incorrectly trigger failure diagnostics

petri/​src/​vm/​mod.rs:484

This treats every VM dropped without teardown() as a failed test, but successful tests can intentionally drop a live VM. For example, sidecar_aps_unused explicitly returns Ok(()) after “dropping VM” (vmm_tests/.../x86_64.rs:201-203), and both EFI diagnostics tests return success as soon as they see the expected message (multiarch.rs:894-912, 930-958). Those passing tests will now write failure_inspect_* attachments and wait up to 20 seconds. The guard needs to collect into state that a post-test hook saves only when test_passed is false (or all successful-drop call sites must explicitly disarm it); drop alone cannot distinguish an error return from normal completion.

[Copilot]

Copilot review overview

🟡 Changes recommended

Minimal VM builders disable the new diagnostics, and the lifecycle behavior lacks regression coverage.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 · 1

Open (2)

• Teardown errors lose VMM inspection diagnostics
• Add regression tests for failure attachment behavior · New

Previously missed (1)

In code that hasn't changed since last review

Enable failure capture for minimal builders

petri/​src/​vm/​mod.rs:675

Minimal builders opt out of the new failure capture entirely. These builders create real VMs for Burette's boot, network, disk, and virtio-fs tests, and their harness drops the VM state immediately when setup or an iteration returns an error, so those failures are exactly where this guard should run. OpenVMM exposes the same inspector in minimal mode, and clean runs consume the runtime via teardown, so enabling this does not add work to successful benchmarks.

[Copilot]

Copilot review overview

🔵 Needs a closer look

Successful tests that implicitly drop live VMs are incorrectly treated as failures.

Review effort: Balanced
Findings: None

Resolved since last review (2)

• Teardown errors lose VMM inspection diagnostics
• Add regression tests for failure attachment behavior

Previously missed (1)

In code that hasn't changed since last review

Implicit VM drops are misclassified as test failures

petri/​src/​vm/​mod.rs:479

This treats every non-teardown drop as a test failure, but successful tests already intentionally keep a VM alive until returning Ok(()) and then let it drop (for example, vmm_tests/vmm_tests/tests/tests/multiarch/openhcl_servicing.rs:628-637, 752-761, and 1883-1899). Those passing tests will now collect and attach failure_inspect_* output—and may wait up to 10 seconds—despite succeeding. Please gate capture/attachment on the actual test result via the post-test lifecycle (or update all successful implicit-drop callers to perform clean teardown) rather than using implicit drop as the failure signal; the new test should also cover an Ok result with an implicit drop.

[github-actions]

1 Petri tests failed (1 unstable)

[Trevor Jones (tjones60)]

Looks good