fix(#194): enqueue cluster inventory sync on registration and make a resync reliably triggerable

[jgruberf5]

Summary

A registered ROKS/OpenShift cluster showed no pod inventory on the Kubernetes page and last_synced_at was never set, even after 70+ minutes and repeated no-op PUTs meant to force a rescan. Capability detection worked; only inventory was missing.

Root cause

Defect 1 — the sync ran but was never recorded. Every registration path already enqueues scan_cluster_async (the POST create route, and the roks/ibm, container, opentofu and ssh auto-registration tasks all call enqueue_cluster_scan), and the async task runs ClusterScanner.scan() and commits. But scan() only persisted capabilities, discovered namespaces and the running release — it never wrote KubernetesCluster.last_synced_at. So "never scanned" and "scanned and genuinely empty" were indistinguishable from the API, and every no-op PUT (which does enqueue a scan) still left last_synced_at null. Nothing anywhere in the codebase ever assigned KubernetesCluster.last_synced_at.

Fix: scan() now stamps last_synced_at at the very end, after all analysis has completed, so a scan that raises early does not falsely record a sync. Because all scan paths (registration/PUT async task, the /scan endpoint, upgrade pre-checks) funnel through this one method, the fix covers them all.

Defect 2 — no reliable, documented resync trigger. Forcing a refresh depended on an undocumented no-op PUT. Added POST /api/k8s/clusters/{id}/resync (owner/admin) which validates the cluster exists (clean 404) and enqueues the same background scan, returning immediately. The PUT path already enqueues a scan unconditionally; a test now locks that a no-op PUT still triggers a rescan.

What the tests lock (mocked K8s client, mutation-checked)

• A completed scan stamps last_synced_at, and it persists across the async task's commit; a scan that fails before completion does not stamp it (reverting the stamp fails 3 tests, the negative test stays green).
• A populated fetch surfaces pod inventory — 6 running Multus pods read as 6 and DETECTED, not 0 (the reported symptom).
• Registration (POST create) enqueues the initial sync.
• A no-op PUT enqueues a rescan; the resync endpoint enqueues a scan, 404s an unknown cluster, and is denied to viewers.

Files changed

• backend/services/scanner/__init__.py — stamp last_synced_at on scan completion.
• backend/routes/k8s/clusters.py — add POST /k8s/clusters/{id}/resync.
• backend/tests/component/test_cluster_inventory_sync.py — new scanner behaviour tests.
• backend/tests/integration/test_routes_k8s_clusters.py — registration/PUT/resync route tests.

Out of scope (noted for follow-up)

Honouring k8s_sync_enabled / k8s_sync_interval_seconds for periodic resync, and an automatic re-scan after a project's modules reach applied (issue suggestions 3 and 4). This PR closes the "never synced / can't force a resync" defects.

Closes #194

https://claude.ai/code/session_01UpRYiFserdBE5ESHn759N4

[jgruberf5]

Self-review (cold, adversarial) — the last_synced_at fix holds; one misleading test corrected

An independent cold auditor reviewed this PR, executing the code and specifically probing the central risk: does this fix make the reporter's 18 pods appear, or only record an empty scan?

Central finding (MAJOR — fixed, cb45656d): the last_synced_at stamp is sound, but TestPodInventoryPopulated claimed to resolve the reporter's "0 Multus pods while 18 running" ground truth, and that claim is false. The real fetch (scanner/fetch.py:794) reads pods only from kube-system; on ROKS/OpenShift Multus lives in openshift-multus, which is never queried — so running_pods stays 0 and the stamp records a fresh time over it. The test hand-built kube_system_pods while labelling the DaemonSet openshift-multus, proving only that analyze_multus counts a handed list.
→ Reframed the test + docstrings to lock what the fix actually does (count + stamp over a fetched namespace) and to not claim the OpenShift symptom is fixed. The pre-existing namespace-scoping gap is filed as #202 (the reporter retracted the Multus framing — BNK 2.3, 2.4-gated panel — so it's low-priority, but tracked).

MINOR (fixed): removed the redundant get_cluster_details() existence check in the resync route — require_cluster_owner already 404s a missing cluster before the body runs (test_resync_unknown_cluster_404 still green via the dependency).

Held under attack (verified clean):

• Stamp placement — last_synced_at is the last mutation, after all analyzers/recommendations/write-backs; a scan that raises early leaves it NULL (mutation-tested: commenting the stamp reds exactly the 3 stamp tests, the failure-path test stays green).
• All scan entry points funnel through scan() — register/PUT/resync async task, sync /scan routes, adaptive selector, upgrade pre-checks; no fetch/analyze path bypasses the stamp.
• Resync authz/404/enqueue — viewer→403, other-owner blocked, missing→404, admin allowed; enqueues the same task as register/PUT.

Standing issue #194 (last_synced_at never set → "never scanned" vs "genuinely empty" indistinguishable) is correctly and non-vacuously fixed. Verified: 22 passed, ruff clean.

[bonnyr-f5]

Review discipline pass — verdict: BLOCK (two narrow conditions)

Three independent cold audits (clean context, no prior review threads) plus an invariant sweep. Everything below verified by execution. Reviewed at 9728fc5.

To be clear up front: the one-line scanner change is the right fix at the right altitude. Putting the stamp inside ClusterScanner.scan() means all six call sites inherit it, and the PR body is admirably accurate — it states plainly that the registration/PUT enqueues already existed rather than claiming them. The block is about two narrow conditions, not the approach.

Verified green

• 22/22 tests pass at HEAD. Removing the stamp reds 3 tests, so the guard is non-vacuous.
• ruff clean. openapi.json and api-generated.ts are consistent with the source, including for the docstring-only commit — no drift.
• INV-1 (tenant scoping) refuted as a concern. require_cluster_owner (backend/routes/auth.py:188-202) loads the cluster, raises NotFoundError if absent, loads its project, then _check_ownership. It is ownership enforcement, not authentication-only. /resync cannot be aimed at another tenant's cluster.
• No new Celery task name, route collision, or migration — INV-7 not engaged.

---

Must fix 1 — the stamp fires on a scan that fetched nothing, which defeats the reporter's stated requirement

The comment at backend/services/scanner/__init__.py:230-233 states the goal as distinguishing "never scanned" from "scanned and genuinely empty." As written it cannot, because essentially no realistic failure prevents the stamp:

Every fetcher swallows its exception and returns an empty default — _discover_api_groups (fetch.py:56-58 → frozenset()), _fetch_nodes (:87-88 → []), _fetch_namespaces (:229-231), _fetch_daemonsets, _fetch_storage_classes, _fetch_crds. fetch_scan_data has no aggregate failure signal; it returns a fully-shaped dict of empties. load_kubeconfig never contacts the API server, so it does not raise on an unreachable or unauthorized cluster.

This PR's own test demonstrates it. test_scan_stamps_last_synced_at calls _run_scan(db, cluster) with no fetch data, which defaults to dict(_EMPTY_FETCH_DATA) — every key empty — and asserts last_synced_at is set. That is precisely the state an expired-token cluster produces.

Failure scenario: cluster 16's bearer token expires. Every call 401s, all swallowed, analysis reports "not detected / 0 pods", the stamp is written, cluster_scan_task.py:34 commits it. K8sClusterList.tsx:538 then renders "Last synced 10 seconds ago" over an empty panel.

This matters more than a normal severity call, because of what the reporter asked for:

"The consequence I care about is the one in the original report: 'never scanned' and 'scanned, genuinely empty' are indistinguishable from the API. … Had that field been populated I would have diagnosed it correctly and probably not filed at all."
— #194 follow-up

And the same comment notes registration deliberately precedes bnk up, so the registration scan captures a pre-install cluster by construction — which is the normal path here, not an edge case. A permanent NULL at least said "we have no data." A timestamp over an empty panel asserts the opposite.

test_failed_scan_does_not_stamp_last_synced_at guards only the coarse case where fetch_scan_data itself raises, which the real fetch path almost never does.

Fix shape: derive a success signal from fetch_scan_data (a hard-failure count, or require the version/namespace preflight to have succeeded) and stamp only then — or add sync_status / sync_error beside it. The codebase already has that exact convention: backend/models/release_source.py:33-35 carries last_synced_at + sync_status (idle|syncing|success|error) + sync_error, with an index at :47. models/kubernetes.py:36 has only the timestamp.

Must fix 2 — INV-4: silent merge collision with open PR #203

PR #203 changes the analyze_multus call in backend/services/scanner/__init__.py:

-            data["crds"], data["crd_names"], data["kube_system_pods"], data["daemonsets"]
+            data["crds"], data["crd_names"], data["multus_pods"], data["daemonsets"]

This PR's new _EMPTY_FETCH_DATA (backend/tests/component/test_cluster_inventory_sync.py:26-38) defines kube_system_pods and no multus_pods. The two edits are ~130 lines apart in the same file, so git auto-merges with no conflict marker and neither author gets a signal.

I applied #203's change and ran this PR's tests. Measured result: 3 of 4 fail with KeyError: 'multus_pods' — test_scan_stamps_last_synced_at, test_last_synced_at_persists_across_commit, test_multus_pods_are_counted_not_zero. (test_failed_scan_does_not_stamp_last_synced_at survives because its fetch raises before the subscript.) Patching analyze_multus to a no-op does not save them: data["multus_pods"] is evaluated as an argument before the mock is called.

Not a one-key fix. test_multus_pods_are_counted_not_zero seeds kube_system_pods with six pods and asserts running_pods == 6, but #203 routes that count through multus_pods filtered to the primary DaemonSet. Adding "multus_pods": [] converts the KeyError into 0 != 6 — the test's premise is invalidated, not just its fixture.

#203 updated all five pre-existing fixtures; it simply could not see a file this PR had not created yet. This needs coordination on merge order, not a unilateral fix.

---

Minor

3. /resync returns success: true when nothing was enqueued. enqueue_cluster_scan swallows every broker exception to a WARNING (backend/tasks/cluster_scan_task.py:44-49), and the handler returns {"success": True, "message": "Inventory sync enqueued"} unconditionally (clusters.py:129-133). Reproduced: with .delay() raising, POST /resync → 200 {"success":true,...} and last_synced_at stays None. A live broker with a down worker gives the same result.

The docstring asserts "there is no silently-swallowed background no-op" (clusters.py:126-127) — true for the 404 case it names, false for the enqueue case one frame down, and now published in openapi.json and the TS types. This is the same failure class as #194: the operator triggers a sync, gets a success, and has nothing. The test patches enqueue_cluster_scan out entirely, so the honest-response property is unasserted. Cheapest fix: return the task id, or enqueued: false on the swallow.

4. The upgrade health gate now holds an uncommitted row write across its whole window. _execute_health_gate (backend/services/bnk_upgrade_execution_service.py:390) loops while time.time() < deadline: calling scanner.scan(cluster_id) with sleep(10)/sleep(15) between iterations and no commit() in the method. Previously scan()'s writes were conditional (discovered_namespaces only when changed), so a steady-state cluster emitted no UPDATE. last_synced_at = datetime.now(UTC) is always dirty, so every iteration now emits UPDATE kubernetes_clusters … and holds that row lock uncommitted across the sleeps. A concurrent scan_cluster_async commit for the same cluster blocks. Related prior guidance: #144 on avoiding long-held locks during multi-minute operations.

5. The comment overstates its own reach. scanner/__init__.py:234-236 says "Every scan path … flows through here … Flushed here; the caller commits." backend/database.py:59-60 is explicit that routes not calling commit get no auto-commit. Callers that never commit: get_adaptive_module_plan (clusters.py:271), get_adaptive_module_plan_from_scan (:347), and the health gate above. Harmless in outcome — a missed stamp, never wrong data — but the comment asserts a property about other code that does not hold, which is how the next reader gets it wrong.

6. /resync does not invalidate _scan_cache. _SCAN_CACHE_TTL_SEC = 600.0 (clusters.py:223), and the UI's scan call defaults to force = false (frontend-v2/src/lib/api/kubernetes.ts:182-185). So after a resync the panel can serve up to ten-minute-old data while the card footer says "just now". The in-file precedent is one line: deploy_hugepages does _scan_cache.pop(cluster_id, None). The deeper version isn't fixable by a pop — the cache lives in the API process and the scan runs in the worker — which is itself an argument for the synchronous path.

7. /resync has zero callers and duplicates a better existing endpoint. A repo-wide grep finds no caller in frontend-v2/src, mcp-server, tests/e2e, or docs (the resync hits are resyncCWCCerts, unrelated). Meanwhile POST /scan?force=true already exists, is UI-wired, is exposed as an MCP tool, and — thanks to this diff — stamps last_synced_at and commits it (clusters.py:250-251). It is a strictly better resync: synchronous, returns the actual results, refreshes the cache. Relatedly, the docstring's claim that "operators previously relied on a no-op PUT" misstates the prior art: /scan?force=true was the documented, UI-wired rescan, as clusters.py:220-222 and cluster_scan_task.py:9 both say.

8. Scope against the issue — worth resolving before "Closes #194". In the follow-up above the reporter also retracted the framing this PR tests:

"I filed this leading with '0 Multus pods', and that was the wrong emphasis. This deployment is BNK 2.3, not 2.4 … 0 is the expected reading here and is not evidence of a defect."

test_multus_pods_are_counted_not_zero asserts against that retracted symptom, and the PR body cites it as "the reported symptom". The same comment says suggestions 1, 3 and 4 are the ones they stand behind, and that suggestion 2 "may simply be wrong" — this PR ships 1 and 2 and skips 3 and 4. Suggestion 3 is still fully open: k8s_sync_enabled / k8s_sync_interval_seconds have no reader anywhere outside the model, schemas and four serializer dicts, and celery_app.py:120-156 adds no beat entry, so an operator can set them, get a 200, and nothing happens. The PR is explicit that this is deliberate follow-up, which is good — but Closes #194 would auto-close an issue whose two stood-behind suggestions remain open.

9. No dedup, cooldown, or queue guard on the new async trigger. scan_cluster_async matches no task_routes entry so it lands on default, alongside health-monitor and worker-heartbeat-keepalive on 60s beats, against --concurrency=4 on two workers. The code's own comment puts a scan at "~25 K8s API calls … 30-60s on real fleets". The reporter's harness polls every 60s; pointed at /resync it would enqueue a scan per poll indefinitely. Both throttle idioms already exist in-repo (_SCAN_CACHE_TTL_SEC; the last_synced_at TTL returning skipped_recent in services/module_catalog_service.py:184).

Nits

• No cross-project-operator 403 test for /resync. test_viewer_cannot_resync covers the role gate and test_resync_enqueues_scan uses admin (which short-circuits ownership), so the ownership branch of require_cluster_owner is correct but unwitnessed.
• scan_metadata.scanned_at is start_time while last_synced_at is end_time — up to a minute apart for the same scan, giving two answers to "when was this scanned".
• ClusterOperationResponse's docstring still reads "Generic response for cluster mutations (delete)."
• mcp-server tool catalog and docs/API_REFERENCE.md gained no /resync entry (the latter was already incomplete).

Reported separately

One pre-existing issue outside this diff's scope was surfaced by the sweep — it concerns an unscoped, low-privilege-reachable path that this change amplifies from a read into a write. It affects released versions, so the details are being routed privately rather than posted here.

---

Verdict: BLOCK, on must-fix 1 and 2 only. Must-fix 2 is coordination with #203. Must-fix 1 is a genuine design correction, but a small one, and the stamp's placement inside scan() is already right — it needs a success signal, not a rewrite. Everything else is Minor and none of it is load-bearing.