races.md

Concurrency Analysis: TOCTOU Race in ServiceDirectory

Executive Summary

This document analyses the com.sixt.service.envoy-control-plane codebase for concurrency issues in the ServiceDirectory, focusing on the pod scraping path. The TOCTOU gap in ExtractFromPod causes Collect call amplification that directly explains the slow-startup-without-Linkerd observation.

Key correction from earlier version: The original analysis claimed that pod addresses could be lost due to the TOCTOU race. This was disproven by tracing the code and confirmed by tests — updateRecordFromPod (line 388-406) always adds the missing address and triggers notification. The actual impact is resource waste and startup latency, not data loss.

---

1. The TOCTOU Gap in ExtractFromPod

File: internal/directory/service_directory.go:146-165

Code Pattern

sd.mu.RLock()
record := sd.serviceInfoCache[getPodOwnerID(pod)]   // line 147: read
sd.mu.RUnlock()                                      // line 148: unlock

// ... gap: no lock held ...

if record == nil {
    created, err = sd.createRecordFromPod(...)       // line 154: acquires write lock internally
}

sd.mu.Lock()                                         // line 163: re-acquire
updated := created || sd.updateRecordFromPod(...)    // line 164
sd.mu.Unlock()

What Actually Happens (traced step by step)

For two pods (A, B) with the same owner arriving concurrently:

Pod A goroutine:                          Pod B goroutine:
─────────────────                         ─────────────────
147: record = cache[owner] → nil          147: record = cache[owner] → nil
148: RUnlock                              148: RUnlock

154: createRecordFromPod(podA)            154: createRecordFromPod(podB)
  338: Collect(podA_IP:port) [slow]         338: Collect(podB_IP:port) [slow]
  ... finishes first ...                    ... finishes second ...
  358: Lock()
  372: cache[owner] == nil → create it
  376: cache[owner] = {Addr: podA_IP}
  383: return created=true                  358: Lock() [was blocked]
                                            372: cache[owner] != nil → EXISTS
                                            373: return created=false

163: Lock()                               163: Lock()
164: created=true || ...                  164: created=false || updateRecordFromPod(podB)
     → short-circuit, updated=true              395: record = cache[owner] → EXISTS
165: Unlock()                                  401: podB_IP not in Addresses → ADD IT
                                               403: return true → updated=true
167: notify ✓                             165: Unlock()
                                          167: notify ✓

Addresses are NOT lost. The updateRecordFromPod at line 164 always runs for the losing goroutine (since created=false, the || does not short-circuit). It looks up the record (which now exists), finds Pod B's IP is missing, adds it, and returns true. Notification fires.

What IS the Problem: Collect Amplification

The real issue is that ALL N goroutines independently call Collector.Collect (line 338) — a gRPC reflection network call — even though only 1 result is needed per owner. All pods in a ReplicaSet return identical metadata.

This is the root cause of slow startup without Linkerd:

1. Informer fires OnAdd(isInInitialList=true) for every pod during initial list
2. Each OnAdd spawns a goroutine → ExtractFromPod(pod, isInitial=true) → IsInInit.Add(1)
3. Due to the TOCTOU gap, ALL N pods for the same owner call createRecordFromPod → Collect
4. Each Collect has a retry policy: 5 attempts, 5s→60s exponential backoff, waitForReady: true
5. Without Linkerd: raw TCP, no connection reuse → slow connects, UNAVAILABLE retries
6. With Linkerd: connection pooling + L7 retries → Collect returns fast
7. Each goroutine holds IsInInit elevated → /ready stays unhealthy until ALL complete

With 100 services × 3 replicas = 300 pods, that's 300 Collect calls instead of 100.

Domain Constraint

All pods in a ReplicaSet run the same container image. Collector.Collect returns identical results from any pod in the same RS. The redundant calls are pure waste.

---

2. TOCTOU in createOrUpdateRecordWithREST (HTTPRoute path)

File: internal/directory/service_directory.go:471-499

Note: This is a real bug but is independent of the slow-startup problem (which was observed after disabling REST on STAGE — meaning this code path was not exercised).

Code Pattern

sd.mu.RLock()
record := sd.serviceInfoCache[ownerID]    // line 472
sd.mu.RUnlock()                           // line 473

if record == nil {
    updated, err = sd.createRecordFromPod(...)  // line 477: NETWORK CALL in gap
}

sd.mu.Lock()                              // line 486
// ... updateRESTServiceInfo ...          // line 494
sd.mu.Unlock()

What Actually Happens

When two HTTPRoute events for the same owner arrive concurrently:

• Both see record == nil, both call createRecordFromPod (redundant Collect)
• Both then call updateRESTServiceInfo sequentially under the write lock
• UpdateRESTMethods (with routeNum=0) replaces the method list from its own matches only
• The second writer overwrites the first writer's REST paths

REST endpoints ARE lost in this path (unlike the pod path above). This is confirmed by TestExtractFromHTTPRoute_ConcurrentSameOwner_TOCTOU which fails reliably.

---

Test Results Summary

Test	Result	What it proves
RedundantCollect	PASS + warning: 10 calls	N×Collect amplification per owner
InitBlocking	PASS: peak=N	IsInInit blocked N× → slow startup
CollectFailureAmplification	PASS: N failures	Cascading errors when pod not ready
SubscriberNotification	PASS	Addresses NOT lost (disproves original claim)
RaceDetector	PASS	Shallow-clone pointer safety
HTTPRoute_TOCTOU	FAIL	REST endpoint loss (real, separate bug)
AddRemove	PASS	Add/remove ordering is safe
WithPodWatcher	PASS + warning	Cross-watcher amplification

---

Recommended Fix

Use x/sync/singleflight keyed by ownerID wrapping the Collector.Collect call:

import "golang.org/x/sync/singleflight"

// In ServiceDirectory:
var collectGroup singleflight.Group

// In createRecordFromPod, replace direct Collect call:
result, err, _ := sd.collectGroup.Do(ownerID, func() (interface{}, error) {
    return sd.Collector.Collect(ctx, addr)
})

This ensures only 1 Collect call per owner regardless of how many pods arrive concurrently. Benefits:

• IsInInit duration reduced from N×Collect to 1×Collect
• Error amplification eliminated (1 failure, not N)
• Network load reduced by N× during startup
• Startup time matches the with-Linkerd behavior

ServiceDirectory lock-upgrade race

Analysis of the read-then-write locking pattern used in
internal/directory/service_directory.go and the concurrent paths in the
watcher (internal/watcher/pod_watcher.go,
internal/watcher/http_route_watcher.go) that make the race reachable in
production.

What "lock-upgrade" means here

sync.RWMutex has no atomic upgrade from RLock to Lock. The pattern in
ExtractFromPod (service_directory.go:146–165) is:

sd.mu.RLock()
record := sd.serviceInfoCache[getPodOwnerID(pod)]
sd.mu.RUnlock()
// (A) window — no lock held
var created bool
if record == nil {
    created, err = sd.createRecordFromPod(ctx, serviceName, pod) // takes its own Lock internally
    // ...
}
// (B) window — no lock held
sd.mu.Lock()
updated := created || sd.updateRecordFromPod(ctx, pod)
sd.mu.Unlock()

There are two unprotected windows (A and B) between the read and the write. The
decision "should I create?" is made on data that may already be stale by the
time we act. The code tries to compensate by re-checking inside
createRecordFromPod under the write lock — but the compensation is incomplete.

The same shape appears in createOrUpdateRecordWithREST
(service_directory.go:471–499).

Example 1 — Lost address + no notification

Setup: a Deployment with two pods, A and B, sharing the same ownerID
(controller.UID of the ReplicaSet — see getPodOwnerID at
service_directory.go:660). Pod B's address is already in the cache. Pod A is
being added for the first time. Independently, pod B is being removed.

Two goroutines run concurrently (informer handlers + spawned worker goroutines —
see "Verifying the concurrency" below):

Time	G1: ExtractFromPod(A)	G2: RemovePodInfo(B)	cache state
t0	—	—	{ownerID: {addrs: {B}}}
t1	RLock → record = {addrs:{B}} → RUnlock	—	unchanged
t2	record != nil → skip createRecordFromPod → created = false	—	unchanged
t3	(scheduler pause)	Lock → delete B from addrs → len==0 → delete(cache, ownerID) → Unlock	{}
t4	Lock → updateRecordFromPod(A) → reads cache[ownerID] → nil → returns false → Unlock	—	{}
t5	updated = false || false = false → no notification

Pod A's event is silently dropped. The control plane never learns about it
until the next pod event arrives — which can be a long time on a stable
deployment. updateRecordFromPod (service_directory.go:395) returns false when
the record is nil, exactly because it can't distinguish "expected nil" from
"raced out from under us":

record := sd.serviceInfoCache[getPodOwnerID(pod)]
if record == nil {
    return false   // silently swallows the lost-race case
}

Example 2 — Duplicate gRPC reflection, discarded result

Setup: two pods A and B with the same ownerID, both being added for the first
time (fresh deployment rollout — informer fires OnAdd for both in parallel).

createRecordFromPod performs sd.Collector.Collect(...) over gRPC
(service_directory.go:338) before acquiring the write lock — that's a
remote call.

Time	G1: ExtractFromPod(A)	G2: ExtractFromPod(B)	cache
t0	RLock → record = nil → RUnlock	—	{}
t1	—	RLock → record = nil → RUnlock	{}
t2	enters createRecordFromPod → Collector.Collect(A:grpc) (slow)	enters createRecordFromPod → Collector.Collect(B:grpc) (slow)	{}
t3	network returns Info_A	network returns Info_B	{}
t4	Lock → cache[ownerID] == nil ✓ → store {addrs:{A}, Info:Info_A} → Unlock → returns created=true	(blocked on Lock)	{ownerID: {addrs:{A}, Info_A}}
t5	continues to second Lock block → updateRecordFromPod(A) → addr A already present → returns false → updated = true || false = true → notify	Lock acquired → re-check cache[ownerID] != nil → returns created=false, nil → Info_B discarded	unchanged
t6		created = false → Lock → updateRecordFromPod(B) → adds addr B → returns true → notify	{ownerID: {addrs:{A,B}, Info_A}}

End state: addresses are correct (both A and B), but Collector.Collect ran
twice and Info_B was discarded.

Severity caveat: pods sharing one ownerID belong to the same ReplicaSet
(a rolling deploy creates a new ReplicaSet with a different UID, so the new
pods get a different ownerID). Pods of one ReplicaSet are expected to be
homogeneous, so Info_B is usually identical to Info_A and the
"first-writer-wins" semantics are not observable — the visible impact is just
the wasted gRPC reflection work, which is multiplied by N at fleet startup
when N pods of one ReplicaSet are added in parallel. The discarded-metadata
angle only matters in the rare case where pod-to-pod Service metadata can
legitimately differ inside a ReplicaSet.

The re-check at service_directory.go:372 is the right defensive move given
the lock-upgrade pattern, but its semantics are "skip silently" — there is no
merge, no retry, no log line.

Example 3 — createOrUpdateRecordWithREST same shape

Same windows at service_directory.go:471–499:

sd.mu.RLock()
record := sd.serviceInfoCache[ownerID]
sd.mu.RUnlock()
// window A
if record == nil {
    updated, err = sd.createRecordFromPod(ctx, serviceName, pods[0])
    // ...
}
// window B
sd.mu.Lock()
defer sd.mu.Unlock()
for _, pod := range pods {
    u := sd.updateRecordFromPod(ctx, pod)
    updated = updated || u
}
u, err := sd.updateRESTServiceInfo(...)

Failure mode: between RUnlock and Lock, RemovePodInfo clears the record.
updateRESTServiceInfo then hits service_directory.go:423–425:

record := sd.serviceInfoCache[ownerID]
if record == nil {
    return false, fmt.Errorf("service record not found for owner ID: %s", ownerID)
}

…and returns an error to the watcher, which logs and moves on. The HTTPRoute
reconcile is lost until the next event.

Why the "re-check inside createRecordFromPod" mitigation is incomplete

It correctly prevents double-insert of the cache entry. It does not
prevent:

• The record being deleted after it was created (Example 1) → outer caller's
  updateRecordFromPod sees nil and silently no-ops. This is the load-bearing
  bug.
• Losing the created=true signal (Example 2) → outer caller sees
  created=false. In this specific timeline the loser still adds its address
  via updateRecordFromPod, which sets updated=true and triggers a notify,
  so no notification is lost — what is lost is Info_B, the just-collected
  metadata.
• Wasted upstream work (Collect over gRPC) when racers collide for the same
  ownerID.

The pattern only papers over the symptom that is easiest to spot (duplicate map
insert).

Verifying the concurrency

The race only matters if the watchers can call ExtractFromPod /
RemovePodInfo / ExtractFromHTTPRoute concurrently. They can.

Where concurrency actually comes from

pod_watcher.go:132 — OnAdd calls ExtractFromPod inside a fresh goroutine:

go func() {
    defer func() {
        close(done)
        h.workInProgress.Delete(pod.UID)
    }()
    if err := h.w.ServiceDirectory.ExtractFromPod(pod, isInInitialList); err != nil {
        ...
    }
}()

pod_watcher.go:128 — the dedupe:

if _, loaded := h.workInProgress.LoadOrStore(pod.UID, done); loaded {
    logger.Info("pod already in processing")
    return
}

Keyed on pod.UID. Two different pods of the same Deployment have different
UIDs but share an ownerID (controller.UID of the ReplicaSet — see
getPodOwnerID at service_directory.go:660). So this dedupe does not prevent
two ExtractFromPod goroutines from touching serviceInfoCache[ownerID] at
the same time.

OnDelete (pod_watcher.go:212) runs RemovePodInfo synchronously on the
informer's handler goroutine — but at pod_watcher.go:206 it only waits for the
same pod's in-flight Add:

if done, _ := h.workInProgress.Load(pod.UID); done != nil {
    <-done.(chan struct{})  // waits for THIS pod's Add, not other pods
}

So RemovePodInfo(podB) does not wait for ExtractFromPod(podA) even when
they share an ownerID.

The HTTPRoute informer is an entirely separate informer with its own
goroutines (http_route_watcher.go:126), so ExtractFromHTTPRoute runs
concurrently with both ExtractFromPod and RemovePodInfo.

Which scenarios actually fire

Scenario	Real?	Why
Two ExtractFromPod for different pods of the same Deployment (Example 2)	Yes	Different pod.UID, dedupe doesn't apply; both spawn goroutines from OnAdd
ExtractFromPod(A) racing RemovePodInfo(B) same ownerID (Example 1)	Yes	OnDelete(B) only waits on B's own Add channel; A's goroutine runs in parallel
ExtractFromHTTPRoute racing ExtractFromPod for same ownerID (Example 3)	Yes	Two separate informers, each spawns its own goroutines
Two ExtractFromPod for the same pod UID	No	workInProgress.LoadOrStore returns early — dedupe holds

The sd.mu mutex is the only thing keeping the cache consistent — which is
exactly why the lock-upgrade gap matters. If the watcher serialized everything
on a single worker goroutine, the RLock/Lock pattern would still be ugly but
harmless. It does not.

Concrete trigger

Easiest reproducer in production: a fresh Deployment rollout. The informer's
initial sync emits an OnAdd for every Ready pod, each of which go's into
ExtractFromPod. For a Deployment with N replicas (sharing one ReplicaSet UID
= one ownerID), you get N goroutines hammering serviceInfoCache[ownerID] in
parallel — and each one calls Collector.Collect over gRPC outside the lock
before deciding whether to create. That is the exact shape of Example 2.

Note: go test -race will not catch this. Every access to
serviceInfoCache goes through sd.mu; both racers take the lock. There is no
Go-level data race — the bug is a logical lost-update across two separate
critical sections. To prove it, write a deterministic concurrency test that
injects barriers into the mocked Collector (or into the directory between the
RUnlock and the second Lock) to force the interleaving in Example 1.

So: the race window is real, the trigger is normal startup, and the load-bearing
symptom (Example 1: occasional missed pod additions/notifications when an Add
races a Delete on the same ownerID) is the kind of intermittent inconsistency
that is very hard to debug after the fact.

Fix — owner-scoped upsert with reservation

The bug comes from making the create-or-update decision under one critical
section and committing it under a different one. The fix is to not split that
decision across two locks. The slow operation (Collector.Collect over gRPC)
must stay outside the lock — holding sd.mu across Collect would block
removals, snapshots, REST updates, and notifications for up to the 60s context
timeout. So the address bookkeeping and the metadata collection need to be
decoupled.

Design

Separate "this pod exists at this address" (cheap, must be atomic) from "here
is the gRPC reflection metadata for this owner" (slow, best-effort, can be
filled in later).

1. Lock, look up record by ownerID.
2. If record exists → add pod address to Addresses, set metadata-pending
   if not yet populated, Unlock, notify.
3. If absent → insert a reserved record now: address + activeServiceMapping
   entry + a generation counter or metadata-pending = true. Unlock,
   notify.
4. Run Collector.Collect outside the lock, using the pod's gRPC port.
5. Lock again. If cache[ownerID] still exists and its generation
   matches the one observed at step 3 (no one deleted-and-recreated under us),
   populate Info, clear metadata-pending. Otherwise discard the collected
   metadata. Unlock, notify if Info changed.

Why this works for Example 1: pod A's address is inserted under the same lock
that excludes pod B's delete. Whichever order they interleave, the final cache
state is a faithful sequence of the two operations — there is no "decision made
on stale data, committed later" gap. Pod A cannot be silently dropped.

Why this works for Example 3: createOrUpdateRecordWithREST does its REST-info
update under the same atomic upsert. If the record was deleted in the meantime,
the upsert is the authoritative creation; there is no "service record not
found" error path to fall into.

Optimizing the duplicate-Collect case (Example 2)

Layer a per-ownerID singleflight.Group on top of step 4: parallel goroutines
processing different pods of the same ownerID share one in-flight Collect.
This is an optimization (eliminates the wasted gRPC reflection at fleet
startup), not a correctness fix.

What I previously proposed and why it was wrong

• "Single write lock, drop the RLock, Collect speculatively before the lock"
  — broken. To know whether to Collect, the code needs to know whether the
  record already exists; an unlocked map read for that is an actual Go data
  race, and always-Collect throws away the existing optimization. The RLock in
  the current code is doing real work; the fix is to make the decision and
  the mutation share one critical section, not to remove the early read.
• "Holding sd.mu.Lock() across Collect" — would fix Example 1 but is
  unacceptable: Collect can take up to 60s and would serialize every
  directory operation behind it.
• "Load-or-store with re-check" — that is exactly the existing pattern.
  Re-checking inside createRecordFromPod prevents the duplicate insert but
  not the lost update in Example 1.

Proving the fix

The right way to demonstrate both the bug and a fix is a deterministic
concurrency test:

• Mock Collector that blocks on a per-call channel released by the test.
• Drive ExtractFromPod(A) and RemovePodInfo(B) concurrently with the same
  ownerID, ordered with the channels so the interleaving in Example 1 fires
  every run.
• Assert: after both return, serviceInfoCache[ownerID].Addresses contains
  pod A's IP and a subscriber notification was delivered.

Today this test fails. With the reservation fix it passes — and it documents
the invariant going forward.

A node rollout is a plausible trigger for the exact “lost part of pod IPs” symptom.

During a node rollout, Kubernetes can evict/delete pods on one node while replacement pods on another node become Ready. For the same Deployment/ReplicaSet owner ID, that creates overlapping events like:

• RemovePodInfo(oldPodB) for a terminating pod
• ExtractFromPod(newPodA) for a newly Ready pod
• possibly several ExtractFromPod calls in parallel for multiple replacements

The dangerous case is:

1. Cache has only old pod B for that owner: {B}.
2. New pod A is added and sees the record exists.
3. Old pod B is removed and deletes the whole owner record because address count becomes zero.
4. New pod A continues, tries to update the now-missing record, gets false, and sends no notification.

End state: pod A exists and is Ready in Kubernetes, but its IP is absent from serviceInfoCache until another event for that same owner happens.

One nuance: this race most directly explains losing the last remaining old IP while a replacement IP is concurrently added. If a Deployment had many cached addresses, removing one old pod would not delete the owner record, so the add would usually succeed. The symptom would therefore be more likely during low replica counts, uneven node draining, readiness gaps, or when removals temporarily reduce an owner’s cached address set to zero.

That's the key issue I think:

Keyed on pod.UID. Two different pods of the same Deployment have different
UIDs but share an ownerID (controller.UID of the ReplicaSet — see
getPodOwnerID at service_directory.go:660). So this dedupe does not prevent
two ExtractFromPod goroutines from touching serviceInfoCache[ownerID] at
the same time.

That's the key issue I think:

Keyed on pod.UID. Two different pods of the same Deployment have different
UIDs but share an ownerID (controller.UID of the ReplicaSet — see
getPodOwnerID at service_directory.go:660). So this dedupe does not prevent
two ExtractFromPod goroutines from touching serviceInfoCache[ownerID] at
the same time.

Sure, that's expected.

The locks in Service Directory actually do restrict storage of 2 records for the same owner ID.

The main part of record creating code is here: https://github.com/Sixt/com.sixt.service.envoy-control-plane/blob/main/internal/directory/service_directory.go#L372

It uses double-checked locking pattern.

Also, it doesn't matter for which one Pod the record is created first. Since the stored data expected to be the same.