Deep Review: 20260415-184215-pr-316

// pass, so the lastAction write races against it.
func (w *dockerWatcher) patchContainerLastAction(ctx context.Context, id string, lastAction *containersv1alpha1.ContainerLastAction) error {
	key := client.ObjectKey{Name: id, Namespace: w.apiNamespace}
	return retry.RetryOnConflict(retry.DefaultRetry, func() error {
		var latest containersv1alpha1.Container
		if err := w.k8s.Get(ctx, key, &latest); err != nil {
			return err
		}
		patch := client.MergeFromWithOptions(latest.DeepCopy(), client.MergeFromWithOptimisticLock{})
		latest.Status.LastAction = lastAction
		return w.k8s.Status().Patch(ctx, &latest, patch)
	})
}

// removeActionAnnotation clears the AnnotationAction annotation only if its
// value still matches the action we just processed. A concurrent writer may
// have replaced the annotation with a fresh action between dispatch and
// cleanup; in that case the new value must survive so the next reconcile
// picks it up.
func (w *dockerWatcher) removeActionAnnotation(ctx context.Context, id, observed string) error {
	key := client.ObjectKey{Name: id, Namespace: w.apiNamespace}
	return retry.RetryOnConflict(retry.DefaultRetry, func() error {
		var latest containersv1alpha1.Container
		if err := w.k8s.Get(ctx, key, &latest); err != nil {
			return err
		}

removeActionAnnotation reads the latest object, checks that the annotation still matches the processed value (lines 460–462), then sends a merge patch to remove it. client.MergeFrom does not include resourceVersion in the patch, so the API server applies it unconditionally. If a concurrent writer replaces the annotation with a new action between the Get and the Patch, the merge patch removes the new value and the action is silently lost.

The compare-and-swap check on lines 460–462 is correct in intent but does not close the TOCTOU window — it guards against changes visible in the cache at Get time, not changes committed to etcd after the Get.

The same pattern appears in patchContainerLastAction (line 442), though the consequence there is a lost status update rather than a lost action.

// failure instead of a silent success.
func (w *dockerWatcher) containerRunState(ctx context.Context, id string) (running, paused bool, err error) {
	inspect, err := w.cli.ContainerInspect(ctx, id, dockerclient.ContainerInspectOptions{})
	if err != nil {
		return false, false, err
	}
	return inspect.Container.State.Running, inspect.Container.State.Paused, nil
}

// patchContainerLastAction writes status.lastAction with retry-on-conflict.
// The main engine sync writes to the status subresource on every reconcile

Fix: use client.MergeFromWithOptions(latest.DeepCopy(), client.MergeFromWithOptimisticLock{}) to include resourceVersion in the patch. The existing RetryOnConflict loop already handles the resulting 409 correctly.

	if !ok {
		return nil
	}

	log := logf.FromContext(ctx).WithName("docker-watcher")
	action := containersv1alpha1.ContainerAction(raw)
	requestedAt := metav1.Now()

	// The webhook rejects invalid action values, but one written while the
	// webhook is offline can still reach storage. Drop such values here;
	// otherwise the CRD enum rejects the status.lastAction write, the
	// annotation stays in place, and every reconcile retries forever.
	if !action.IsValid() {
		log.Info("Dropping invalid container action annotation", "id", c.Name, "action", raw)
		return w.removeActionAnnotation(ctx, c.Name, raw)
	}

	dockerErr := w.dispatchContainerAction(ctx, log, c.Name, action)

	lastAction := &containersv1alpha1.ContainerLastAction{
		Action:      action,
		RequestedAt: requestedAt,
		CompletedAt: metav1.Now(),
	}
	if dockerErr == nil {
		lastAction.State = containersv1alpha1.ContainerActionSucceeded
	} else {
		lastAction.State = containersv1alpha1.ContainerActionFailed
		lastAction.Error = dockerErr.Error()
		log.Info("Container action failed", "id", c.Name, "action", action, "error", dockerErr)
	}

	if err := w.patchContainerLastAction(ctx, c.Name, lastAction); err != nil {
		return fmt.Errorf("failed to patch lastAction for %s: %w", c.Name, err)
	}

The update webhook validates only spec immutability; annotations are free-form. A misspelled annotation value (e.g. action=staart) passes the webhook, reaches dispatchContainerAction (which returns "unknown container action"), and then patchContainerLastAction tries to write the invalid value into status.lastAction.action. The CRD enum (start|stop|pause|unpause|restart) rejects the status write, so patchContainerLastAction fails and removeActionAnnotation never runs. The bad annotation persists and every subsequent reconcile retries the same impossible action.

Fix: validate the annotation value in the update webhook, and add a defensive guard in processContainerAction that removes the annotation and logs a warning for unrecognized values instead of attempting a status write that cannot succeed.