All three issues from my last review are addressed. Here's what I see now:
Stop() error handling (was my top concern): Lines 248–250 now check the return value from stopTask and log it at Error level. It deliberately continues rather than returning the error — that's the right call for a best-effort shutdown path, and the log ensures it won't go unnoticed.
startTaskAndMonitor error handling: Lines 565–567 do the same: capture stopErr, log it if non-nil, then return the original readiness failure. Clean.
Test robustness: The reflect.ValueOf(...).Pointer() approach is gone. The mock now tracks deleteOptCount (line 72 in task_test.go), and the assertion checks require.Equal(t, 1, task.deleteOptCount) — exactly the "track whether Delete was called with at least one option" approach I suggested. That's solid.
The stopTaskWithGrace implementation itself is well-structured: it uses context.WithoutCancel to detach from caller cancellation so cleanup can't be skipped by an already-cancelled context, registers the wait channel before sending SIGTERM to avoid the fast-exit race, escalates to SIGKILL after the grace period, and guards the final Delete with errdefs.IsNotFound so a task that was already cleaned up doesn't produce a spurious error. The buildkitTask interface makes the unit tests clean and avoids needing a live containerd daemon.
This is ready to graduate from draft to human review.
Verdict: ready
{
"owner": "mirendev",
"repo": "runtime",
"number": 1148,
"verdict": "ready",
"event": "comment",
"summary": "All three issues from my last review are addressed. Here's what I see now:\n\n**`Stop()` error handling (was my top concern):** Lines 248–250 now check the return value from `stopTask` and log it at `Error` level. It deliberately continues rather than returning the error — that's the right call for a best-effort shutdown path, and the log ensures it won't go unnoticed.\n\n**`startTaskAndMonitor` error handling:** Lines 565–567 do the same: capture `stopErr`, log it if non-nil, then return the original readiness failure. Clean.\n\n**Test robustness:** The `reflect.ValueOf(...).Pointer()` approach is gone. The mock now tracks `deleteOptCount` (line 72 in `task_test.go`), and the assertion checks `require.Equal(t, 1, task.deleteOptCount)` — exactly the \"track whether Delete was called with at least one option\" approach I suggested. That's solid.\n\nThe `stopTaskWithGrace` implementation itself is well-structured: it uses `context.WithoutCancel` to detach from caller cancellation so cleanup can't be skipped by an already-cancelled context, registers the wait channel before sending SIGTERM to avoid the fast-exit race, escalates to SIGKILL after the grace period, and guards the final `Delete` with `errdefs.IsNotFound` so a task that was already cleaned up doesn't produce a spurious error. The `buildkitTask` interface makes the unit tests clean and avoids needing a live containerd daemon.\n\nThis is ready to graduate from draft to human review.",
"posted_to_pr": true,
"draft": true
}This draft addresses a real and well-understood bug: a forced server shutdown leaves a lingering BuildKit task behind, which then blocks NewTask on the next miren restart. The fix is directionally correct — stopTaskWithGrace is substantially improved (namespace-clean teardown, pre-registered wait channel to avoid the "fast-exit races past the wait" bug, WithProcessKill on Delete, errdefs.IsNotFound guard), and restartExistingContainer now correctly propagates the stop error so a failed eviction doesn't silently continue. The new buildkitTask interface and the two unit tests in task_test.go are the right shape for this.
Two things deserve attention before this merges.
Stop() ignores the error from stopTask (line 248). stopTask now returns an error, but the Stop() path just calls c.stopTask(ctx, task) and throws it away. That means a task whose Delete call fails during a normal shutdown is silently swallowed — the caller sees nil and assumes everything cleaned up. Given the whole point of this PR is that a failed task cleanup is the bug to catch, this asymmetry is worth closing. I recognize Stop() is a best-effort teardown and may want to log-and-continue rather than return an error, but the current code doesn't even log it; it just calls through as if stopTask still returned nothing.
startTaskAndMonitor also drops the stopTask error (line 563). The waitForReady failure path calls c.stopTask(ctx, task) bare. That's the same issue — a failed cleanup here would go unnoticed. At minimum, log the error so it shows up in diagnostics.
One minor note on the test: the reflect.ValueOf(opt).Pointer() technique for detecting containerd.WithProcessKill (task_test.go line 63) compares function pointers by address. This is not guaranteed by the Go spec — two closures of the same literal may or may not share an address — though for a plain top-level func like WithProcessKill it works in practice. A simpler alternative is to track whether Delete was called with any options at all, or use a more explicit flag.
stopTask now returns an error, but the Stop() path drops it entirely. If task.Delete fails during a normal shutdown, the caller gets nil and there's no trace of it. Either propagate this error or, at minimum, log it. Silently discarding it undermines the whole point of having stopTask return an error.c.stopTask(ctx, task) here in the waitForReady failure cleanup path also drops the error. This is the teardown when the daemon failed to become ready — if the task Delete also fails, it goes completely unnoticed. Log or propagate it.reflect.ValueOf(opt).Pointer() to reflect.ValueOf(containerd.WithProcessKill).Pointer() works for a top-level package-level function today, but Go doesn't guarantee two references to the same function share an address. A more robust approach: instead of tracking whether the option is WithProcessKill, just track whether Delete was called with at least one option (or introduce an explicit deleteOpts []containerd.ProcessDeleteOpts field and assert on it). Worth revisiting before merge.Verdict: caveats