The gap I flagged in my last review — no unit test for the "skip CreateDiskImage when disk.img already exists" branch — has been filled. TestDiskVolumeControllerCreateSkipsExistingImage (lines 91–144 of disk_volume_controller_test.go) does exactly what I asked: it pre-marks both the volume directory and disk.img as existing via ops.existingPaths, runs reconcile, and asserts ops.createdImages is empty while also confirming the entity reaches DV_READY. That's the right shape for this test.
The rest of the change is unmodified from my previous review, and my analysis of it still holds: the DV_PENDING / PROVISIONING ordering fix is correct, the VolumePathExists guard targets the file correctly, and the blackbox test improvement is a genuine win. No new concerns from this pass.
Ready to graduate to human review.
Posted to the PR's comment threads when you submit.
TestDiskVolumeControllerCreateSkipsExistingImage covers this exactly — pre-populates ops.existingPaths for the disk.img path, runs reconcile, and asserts ops.createdImages is empty plus the entity reaches DV_READY. Concern is addressed, closing this.Verdict: ready
{
"owner": "mirendev",
"repo": "runtime",
"number": 970,
"verdict": "ready",
"event": "comment",
"summary": "The gap I flagged in my last review — no unit test for the \"skip `CreateDiskImage` when `disk.img` already exists\" branch — has been filled. `TestDiskVolumeControllerCreateSkipsExistingImage` (lines 91–144 of `disk_volume_controller_test.go`) does exactly what I asked: it pre-marks both the volume directory and `disk.img` as existing via `ops.existingPaths`, runs reconcile, and asserts `ops.createdImages` is empty while also confirming the entity reaches `DV_READY`. That's the right shape for this test.\n\nThe rest of the change is unmodified from my previous review, and my analysis of it still holds: the `DV_PENDING` / `PROVISIONING` ordering fix is correct, the `VolumePathExists` guard targets the file correctly, and the blackbox test improvement is a genuine win. No new concerns from this pass.\n\nReady to graduate to human review.",
"replies": [
{
"thread_id": "PRRT_kwDONZ__ss6Tn-63",
"body": "The new `TestDiskVolumeControllerCreateSkipsExistingImage` covers this exactly — pre-populates `ops.existingPaths` for the `disk.img` path, runs reconcile, and asserts `ops.createdImages` is empty plus the entity reaches `DV_READY`. Concern is addressed, closing this.",
"resolve": true
}
],
"posted_to_pr": true,
"draft": true
}This is a draft, so I'm judging whether it's ready for human review — and it is, with one minor gap worth noting.
The fix closes a race in disk undelete: previously, the command wrote DV_READY and DiskStatusProvisioned immediately on creation, which meant a lease could bind against a volume that wasn't yet registered in the runner's in-memory state. That produced a terminal FAILED lease. The fix introduces a proper ordering:
disk_volume is created in DV_PENDING (not DV_READY), so the DiskVolumeController drives it through the normal mount-then-ready sequence.PROVISIONING (not PROVISIONED), so the disk isn't advertised as leasable until the volume controller promotes it after a real mount.DiskVolumeController.createVolume now skips CreateDiskImage when disk.img is already present, so the restored data isn't truncated.The causal chain is clear and the comments in each changed file explain the invariant well.
disk_volume_controller.go — the VolumePathExists(imagePath) guard is the right call. The imagePath is constructed inside createVolume as filepath.Join(volumePath, "disk.img"), so it correctly targets the image file, not just the directory. VolumePathExists is already used in several other places against directory paths (where a directory also satisfies the check), but here it's targeting a file — I verified the real implementation at disk_ops_linux.go line 49 uses os.Stat, which works on both files and directories. ✓
The PROVISIONING → PROVISIONED promotion chain — I traced through the codebase to confirm this path exists. The normal debug disk create flow also starts with PROVISIONING, and the DiskVolumeController promotes the disk entity once DV_READY is reached via updateVolumeState. The undelete path now uses the same sequence. ✓
Test: t.Fatalf in the poll callback — harness.Poll calls the condition in the same goroutine (no go keyword, no t.Run), so t.Fatalf is safe there. The change also replaces a silent 60-second timeout with a fast, diagnostic failure message — a concrete improvement to the test's debuggability. ✓
There's no unit test for the "disk image already present, skip CreateDiskImage" branch in createVolume. The existing test TestDiskVolumeControllerReconcileVolumePresent exercises the create-from-scratch path and verifies ops.createdImages has one entry. A restore scenario would need ops.existingPaths to include the imagePath so that VolumePathExists returns true, and then the assertion would be assert.Empty(t, ops.createdImages). This is the only newly added runtime behaviour without a unit test. It's not a blocker — the blackbox test covers the end-to-end scenario — but a unit test here would make regressions very cheap to catch.
VolumePathExists(imagePath) skip-path has no unit test. The existing TestDiskVolumeControllerReconcileVolumePresent covers create-from-scratch (expects ops.createdImages to have one entry). Consider adding a parallel test — call it TestDiskVolumeControllerCreateSkipsExistingImage — where ops.existingPaths[imagePath] = true before reconcile, and assert ops.createdImages is empty afterwards. This is the only new runtime behaviour without unit coverage.Verdict: ready