fix(csi-driver): repair paths before connecting them, not after

[noctarius]

The bug

The connection monitor ran its two reconcile passes and then the repair pass, so the repair diagnosed controllers the reconcile had created microseconds earlier. A controller that has just connected has not attached its namespace to the head yet, which is indistinguishable from one that never will — so the repair tore down the path it had just made, and deferred the reconnect to "the next reconcile", a reconcile that had already run.

reconcileOptimizedPath(...)      // connect pass
reconcileNonOptimizedPaths(...)  // connect pass
healMonitoredVolume(...)         // teardown pass — nothing follows it
return nil

What it cost

k8s_native_rapid_failover_no_gap 2026-09-12, lvol 9ab68184 on worker-3:

05:10:42.243  Degraded subsystem: active=2 expected=3  device=/dev/nvme24n1
05:10:42.422  running command: [nvme connect -t tcp -a 192.168.10.12 -s 4434 ...]
05:10:42.519  nvme repair: repairing controller-not-contributing ...
05:10:42.683  nvme repair: repaired; the next reconcile will reconnect the path

The path came back at 06:19 — 68 minutes later, because the namespace head went away at 05:11:34 and took the per-volume monitor with it. The volume ran at 2 of 3 paths, and nine seconds after the teardown its primary and secondary both went down. A write blocked 41s against fio's max_latency=40s and failed the run at iteration 30.

Across that 9-hour run the six node plugins tore down and deferred 1,670 times — every one a redundancy hole until the following tick. Worst-case latency for all other volumes sits on a hard ~10s ceiling (18 pods at 9,995–10,013 ms); this was the only sample above 20s.

The fix

Run the repair first. Whatever it tears down, the two reconcile passes below connect again in the same tick, through the connect path that already owns the reachability and node-online checks. The teardown becomes a repair rather than a hole, and no blocking work is added to a loop that walks every device on the node in sequence — nvme connect costs up to 40s per call against a target that is down.

defaultConnectGrace (5s) closes the same race across ticks, for a controller connected by the previous pass that is still attaching. A defect whose teardown set contains one is deferred — reported as a skipped reason like any other policy refusal — and repaired once the window passes.

Why 5s: it has to clear one monitor tick (monitorBaseInterval + monitorJitter) or the skip is a no-op; the observed false verdict came 97 ms after connect, so 5s is ~50× the normal attach; and the cost of waiting is a genuinely broken path repaired 5s later against a 60s ctrl_loss_tmo.

Subtract the teardown from the path snapshot. activePaths is what the monitor captured before recoverPathsWithANA ran, and missingEndpoints counts an endpoint as attached on presence alone regardless of state — so without this a controller just torn down still looks attached, its connect is skipped, and the reorder buys nothing. healMonitoredVolume returns the endpoints it removed and withoutEndpoints takes them out. (Caught by Copilot review; it was a real defect.)

The age comes from the kernel, not from bookkeeping

An NVMe controller publishes no connected at attribute — checked against a live 5.14 node, which carries only address cntlid cntrltype ctrl_loss_tmo dctype ... state subsysnqn transport. But kernfs stamps the controller's sysfs directory when it creates it, and does not disturb that stamp when namespaces attach underneath. Measured on that node during a real NodeStageVolume:

	csi-node log (connect returned)	sysfs dir mtime
nvme0	10:44:15.048770	10:44:15.046347
nvme1	10:44:15.097668	10:44:15.095348

The mtime precedes the command's own return by ~2 ms, and both were unchanged 81 s later with the namespace child present and the volume mounted and in use. nvme.Controller.CreatedAt now carries it.

Reading the kernel's answer beats recording connects in this process: it covers a connect this driver did not make, and it survives a node-plugin restart, which an in-process map does not. It also needs no exception for the "already connected" path, where nothing is created and refreshing an age would defer forever the repair of a controller stuck contributing no path — the spin nvmerepair exists to break.

Testing

TestHealSubsystem_LeavesAJustConnectedControllerAlone, marked Regression: 2026-09-13-nvmerepair-tears-down-its-own-connect. Verified red on the unfixed tree for the right reason (tore down [nvme1]; a controller connected 97ms ago must be left to attach), and re-verified red after the rewrite by stubbing the check out. It also asserts the grace defers rather than abandons: once the window passes, a controller that still contributes nothing is still repaired.

TestWithoutEndpoints_RepairedPathIsSeenAsMissingAgain pins the snapshot subtraction — red on the unfixed tree with missing = [], want only the torn-down 10.0.0.2.

TestScanControllers_CreatedAtIsTheDirectoryMtime pins the atlas-lib side: the scanned CreatedAt is the directory's mtime, and an unreadable one stays zero so the policy layer can tell "young" from "unknown".

go test -race -count=1 -shuffle=on ./pkg/util/, make -C csi-driver unit-test and make -C atlas-lib test all green.

Reviewer notes

• The ordering itself is not unit-pinned. recoverPathsWithANA needs a live control-plane client and real NVMe devices, so the call order is enforced by construction and by the comment. Its two consequences are pinned — the grace and the snapshot subtraction — but the honest verification for the ordering is the next rapid-failover run. The snapshot defect Copilot found lived precisely in that gap, so treat it as a known weak spot in this change.
• No test-plan matrix row. There is no test plan covering the CSI path monitor on this branch (only test-plan-drain-remove.md and test-plan-storagenode-ops.md). Happy to add a minimal one here or file it against main.
• This does not address the related sbcli defect from the same run: the ANA failover handler logs promoting first_sec and never promotes the tertiary when the acting secondary is already offline. Tracked separately.

🤖 Generated with Claude Code

[Copilot]

🟡 Changes recommended

Unresolved critical and moderate findings affect same-tick reconnection, orchestration coverage, and transport-specific grace handling.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

This PR reorders CSI NVMe path repair before reconciliation and adds a grace period for newly connected controllers.

Changes:

• Repairs paths before reconnect passes.
• Tracks recent connects with a 5-second grace window.
• Adds regression coverage for deferred repair.

File summaries

File	Summary
csi-driver/pkg/util/nvmerepair.go	Adds grace tracking; update stale comments and include transport in controller keys.
csi-driver/pkg/util/nvmerepair_test.go	Tests grace-period deferral and eventual repair.
csi-driver/pkg/util/initiator.go	Reorders recovery and records connects; refresh post-repair inputs and add orchestration coverage.

Review details

Suppressed comments (4)

csi-driver/pkg/util/initiator.go:1186

• This comment still says the repair pass runs later in the same tick, but recoverPathsWithANA now calls repair before both reconcile passes. Please describe this as protecting a later repair pass (including a subsequent monitor tick) so the comment does not document the old ordering.

	// A controller that has just appeared has not attached its namespace yet.
	// Record it so the repair pass later in this same tick defers its verdict
	// instead of tearing down what this call created.
	defaultRepairer.noteConnect(conn.Nqn, conn.IP, conn.Port, time.Now())

csi-driver/pkg/util/initiator.go:1384

• The relative order here is the core production fix, but the added regression test calls healSubsystem directly and never exercises recoverPathsWithANA. A future move of healMonitoredVolume below these calls would therefore leave the test suite green while reintroducing the incident; add an orchestration seam/test (or equivalent integration assertion) that records and enforces repair-before-both-reconciles.

	reconcileOptimizedPath(sbcClient, nodeInfo, devicePath, optConn, activeOpt, ctrlLossTmo)
	reconcileNonOptimizedPaths(sbcClient, nodeInfo, devicePath, nonOptConns, activeNonOpt, ctrlLossTmo)

csi-driver/pkg/util/nvmerepair.go:534

• Expired entries are pruned only when noteConnect runs. After the last successful connect, this process-wide map retains endpoints for every old NQN indefinitely; repeated volume attach/detach cycles can therefore grow the node plugin's memory. Prune expired entries while holding the lock in settling (or otherwise give these timestamps a bounded lifecycle).

	now := r.now()
	r.mu.Lock()
	defer r.mu.Unlock()
	var longest time.Duration
	for _, ctrl := range d.Controllers {

csi-driver/pkg/util/nvmerepair.go:504

• This new comment ends with an incomplete sentence, so it does not clearly state what callers no longer need to remember.

// monitor's reconcile from having to remember to.

• Files reviewed: 3/3 changed files
• Comments generated: 3
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.