todo: the allocation annotation is a read-modify-write over one blob, with nothing serializing its writers

Found by the review of #119, which narrowed it but does not close it. The write half is pre-existing; the
unpatch half arrived with that PR. Filing it so the residual is tracked rather than living in one code
comment.

What happens

device.gpustack.ai/accelerator.allocated carries one JSON map for the whole Pod, keyed by
container. A strategic merge patch on that key replaces the whole string, so every writer does a
read-modify-write over a shared blob — with no concurrency control. There are two writers, both in
pkg/deviceplugin/controller.go, and both run outside the node allocate mutex on purpose (annotation
I/O is deliberately off the serialized path — see persistAllocation's doc):

• patchAllocatingPod — records a container's allocation;
• unpatchAllocatingPod — takes it back out, or restores what it held before.

Each writer reconstructs the pod's other entries from two sources: the pod as the informer has it, and
this process's in-process reservations for the other containers. That overlay is the existing mitigation,
and it is why the common interleavings are already safe. What is left is that an entry can travel into a
concurrent writer's map by either source, and the two need different fixes.

The interleavings

One Pod, containers main (whose responder refuses) and sidecar (allocated concurrently).

A — via the sibling's cached annotation. Open.

	main	sidecar	annotation
T1	persistAllocation lands		{main}
T2			{main} (informer catches up)
T3		starts; its cached pod carries {main}	{main}
T4	responder refuses → release reservation → unpatch		{sidecar}
T5		persistAllocation: base {main} + itself	{main, sidecar}

main is back — a claim for a container the kubelet never started, which is the leak #105 closed,
re-entering by another route. Note where main came from at T5: the cached annotation, not the
reservation overlay.

B — via the reservation overlay. Closed by #119.

Same shape, but sidecar's cached pod predates T1, so it starts from {} and picks main up from the
reservations instead. #119 releases the reservation before the unpatch, so a sibling reading from then on
does not find it. (Before that ordering, this one resurrected main too.)

C — lost update between two patches. Pre-existing, and close to self-cancelling.

A claim that exists only durably — written by a previous device-manager process, so no reservation backs
it — plus a sibling whose cached pod is older than it: the sibling's patch writes its own map and
erases that claim. Neither source carries it, so the overlay cannot help.

The two conditions fight each other, which is why this one is the least reachable of the three. After a
device-manager restart every claim is durable-only, but the informer completes its initial sync before
the plugin serves, so the sibling's cached pod is at least as new as the claim — and therefore carries it.

Which scenarios this affects

The blob is per Pod, so the concurrency this codebase actually designs for does not touch it: the node
allocate mutex exists for a concurrent Allocate batch across different Pods (its own doc names Kueue
admitting identical Pods together), and those write different annotations. Reaching this needs two writers
inside one Pod.

Three conditions, all required:

1. one Pod with two or more containers claiming accelerators;
2. their annotation writes overlap in time;
3. one of them is a refusal (A), or a durable-only claim meets a stale cache (C).

Condition 1 is where the answer lives:

Workload shape	Affected	Why
An Instance from this operator (workload container + SSH sidecar)	no	the sidecar claims the visibility resource, and a visibility Allocate writes no allocation status at all
Anything sliced or partitioned	no	rule 6 of validatePodAcceleratorRequest rejects a Pod where more than one container claims a slicing family
An init container and an app container both claiming	no	rule 1's group half requires every claim to sit in one container group
Two app containers of one Pod each claiming a whole or shared accelerator	yes	the exclusive and shared families carry no one-container cap
A Pod claiming two manufacturers (container A nvidia.com/gpu, container B huawei.com/npu)	yes, and worse	two device-manager processes write the same blob, which is what rules out the cheap in-process fix

So the exposed shape is a user-authored Pod with two or more containers each holding a whole or shared
accelerator. Nothing this operator generates has that shape, which is why it has never bitten.

Condition 2 is the guardrail today: kubelet admits one Pod at a time and allocates a Pod's containers
sequentially, so two containers of one Pod do not write concurrently in the normal path. This one is a
reading of kubelet's admission path, not a fact verified here — kubelet's device manager is not vendored
into this repo — and it is the weakest link in this analysis. Treat it as "narrow", not "impossible".

What would turn it from latent into live:

• Anything writing this annotation outside Allocate — a future controller, a repair tool, a human
  with kubectl annotate. Today's safety rests on Allocate being the only writer, which is an accident
  of the current feature set rather than an invariant anything enforces.
• A cross-manufacturer Pod. Strictly it does not create the overlap (kubelet still calls the two
  sockets in sequence); it removes the cheap fix, because a per-Pod in-process mutex cannot span two
  processes. That is why the webhook rule is step 1 below.
• Any parallelism added to Allocate handling — batched or concurrent admission.
• The detached compensation context introduced in #119. The compensating patch may now outlive the
  Allocate call that triggered it, by up to its own deadline. That is a new window in which an annotation
  write is not bounded by its request. Kubelet's behaviour still closes it — a container whose Allocate
  fails aborts that Pod's admission, so its siblings are never allocated — but the window is wider than it
  was, and it belongs on this list rather than out of sight.

Why it is not just "add a mutex"

An in-process mutex would be enough only if one process writes a given Pod's annotation. Today two can:
the chart runs one DaemonSet per manufacturer (deploy/gpustack-operator/chart/templates/device-manager/daemonset.yaml),
and nothing forbids a Pod from claiming two manufacturers. Rule 1 of validatePodAcceleratorRequest
(pkg/worker/webhooks/worker/pod.go:489-556) constrains the accelerator family — exclusive,
shared, sliced, partitioned — not the manufacturer. So container A requesting nvidia.com/gpu: 1
and container B requesting huawei.com/npu: 1 satisfies every rule and is admitted, and the two
manufacturers' device-managers then patch the same Pod's annotation from different processes.

Shape of the fix

Two steps, in this order, because the first is what makes the second sufficient.

1. Forbid a Pod from claiming more than one manufacturer. A new rule alongside rule 1, in the same
function and the same style; containerClaims already returns the bases, so the data is in hand. The
error strings of these rules are quoted verbatim in docs/accelerator-requests.md (no test pins that
table), so the docs list and its example need the new one.

This is worth having on its own merits — a pool, an InstanceType and the four-view accounting are all
single-manufacturer by construction, so a cross-manufacturer Pod produces states nothing downstream
models. It is also a new rejection: a Pod that is admitted today would stop being admitted, so it
wants an explicit decision rather than being slipped in.

2. Serialize the read-modify-write per Pod. With step 1 in place a Pod's annotation has exactly one
writing process, so a per-Pod mutex held across read → modify → write in both writers closes A, B and
C together. Two constraints on it: it must be a different lock from allocateMutex (reusing that one
would pull annotation I/O back onto the serialized allocate path, which the current design deliberately
avoids), and per-Pod granularity keeps concurrent Allocates for different Pods fully parallel.

Rejected alternatives, and why they are still worth knowing:

• Conflict-aware read-modify-write — live read through DevicesReconciler.APIReader (already on the
  struct), metadata.resourceVersion in the patch body as a precondition, RetryOnConflict around it.
  Closes everything without step 1, since it is safe across processes too. Costs one live GET per
  annotation write plus rare retries. This is the fallback if step 1 is not wanted.
• One annotation key per container (…allocated.<container>) — structurally the cleanest: a merge
  patch touching only its own key cannot rewrite a sibling, so no lock and no retry. But it is a durable
  format change needing a dual-read migration release, which is a much wider blast radius than this.

How to reproduce without hardware

Drive two Allocate calls for two containers of one Pod through ResourceServer.Allocate against the
fake client, with the refusing responder on one of them, and interleave them so the sibling's patch is
built before the unpatch and applied after it. pkg/deviceplugin/server_test.go already has the pieces:
TestResourceServer_Allocate_Concurrent for two servers over one reconciler, the interceptor-based patch
hooks used by TestResourceServer_Allocate_RefusalFreesTheReservationBeforeTheAnnotation for ordering,
and failingResponder for the refusal. Assert the annotation afterwards: scenario A leaves main in it.

What is unproven until then

Nothing about the compensation's own behaviour — #119 covers the refusal, the replay, the cancelled
context and the ordering with tests. What is unproven is only the sibling-concurrent case: that a Pod
whose containers are allocated at the same time never has one container's claim resurrected or erased by
the other's write.

Two things to distrust rather than the whole change: the claim that kubelet never overlaps two containers
of one Pod (a reading, not something verified in this repo), and the claim that Allocate is the only
writer of this annotation (true today, enforced by nothing).

/kind bug
/area devicemanager

Start with pkg/deviceplugin/controller.go and the persistAllocation documentation, then inspect validatePodAcceleratorRequest in pkg/worker/webhooks/worker/pod.go:489-556 and docs/accelerator-requests.md. Run the focused allocation tests in pkg/deviceplugin/server_test.go, especially TestResourceServer_Allocate_Concurrent and TestResourceServer_Allocate_RefusalFreesTheReservationBeforeTheAnnotation. Done means the chosen concurrency behavior is covered for the described interleaving and the validation rule and documentation agree.