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 the "when was this synced" signal was missing.

Root cause

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), and only when the scan genuinely reached the cluster's API server. Because all scan paths (registration/PUT async task, the /scan endpoint, the upgrade health gate) funnel through this one method, the fix covers them all.

Reached-signal (only stamp on a scan that actually contacted the cluster)

Every fetcher in scanner/fetch.py swallows its exception and returns an empty default (frozenset() / [] / None), and load_kubeconfig never contacts the API server — so an expired-token or unreachable cluster produced a fully-shaped empty dict that was indistinguishable, key-by-key, from a reachable-but-empty cluster. Stamping there would write a fresh sync time over a panel with no data — strictly worse than the NULL that honestly says "we have never gotten data from this cluster."

fetch_scan_data now derives a reached boolean from the three preflight-class signals that all traverse the same reach-and-authenticate path — the /apis group discovery, the /version call, and the namespace list. A 401 / connection failure fails all three; a genuinely reachable cluster (even an empty one) always returns at least a version, registered API groups and the built-in namespaces. scan() stamps last_synced_at only when reached is true. A genuinely-empty-but-reachable cluster still stamps; an unreachable / 401 one does not and stays NULL.

The stamp records start_time — the same instant surfaced as scan_metadata.scanned_at — so "when was this scanned" has a single answer across the DB stamp and the result payload.

Rescan trigger

There is no new endpoint: POST /api/k8s/clusters/{id}/scan?force=true already exists, is UI-wired and exposed as an MCP tool, runs synchronously, returns the actual scan results, refreshes the scan cache, and — thanks to this PR's scan() change — now stamps last_synced_at and commits it. An earlier revision of this PR added a POST .../resync endpoint; it was removed as a zero-caller duplicate of the strictly-better /scan?force=true (per review).

Concurrency: no long-held row lock in the upgrade health gate

scan() now always writes last_synced_at, so its flush leaves an UPDATE kubernetes_clusters pending. The upgrade health gate (_execute_health_gate) loops scan() with 10s/15s sleeps between iterations, so that row lock would otherwise be held uncommitted across every sleep, blocking a concurrent scan_cluster_async commit for the same cluster. The gate now commits each iteration's scan writes (and rolls back a failed iteration) before sleeping, releasing the lock — consistent with the service's ENG-006 "commit after every step / health snapshot" convention.

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

• A scan that reached the cluster stamps last_synced_at, and it persists across the async task's commit; a scan that fails before completion does not stamp it.
• An unreachable / expired-token scan (all-empty, reached=false) does not stamp — the timestamp-over-empty-panel failure this issue reported.
• analyze_multus counts the Multus pods the fetch surfaced (6 → 6, DETECTED) over a namespace the scan actually reads; the fixture mirrors fix(#202): count Multus pods in the DaemonSet's namespace so OpenShift (openshift-multus) reports correctly #203's fetch-dict shape so the eventual merge stays clean.
• Registration (POST create) and a no-op PUT each enqueue a scan.

Files changed

• backend/services/scanner/fetch.py — derive and return the reached signal.
• backend/services/scanner/__init__.py — stamp last_synced_at only when reached (start_time); corrected the commit-ownership comment.
• backend/services/bnk_upgrade_execution_service.py — commit/rollback each health-gate scan iteration so no row lock is held across the sleeps.
• backend/schemas/k8s.py — tightened ClusterOperationResponse docstring (delete-only).
• backend/tests/component/test_cluster_inventory_sync.py — reached/not-reached + pod-inventory tests.
• backend/tests/integration/test_routes_k8s_clusters.py — registration/PUT route tests.
• backend/openapi.json, frontend-v2/src/types/api-generated.ts — regenerated (resync endpoint removed).

Out of scope (follow-up issues filed)

Honouring k8s_sync_enabled / k8s_sync_interval_seconds for periodic resync (#212) and an automatic re-scan after a project's modules reach applied (#213) — the reporter's stood-behind suggestions 3 and 4. This PR closes the "never synced" defect only, so it does not auto-close the issue.

Relates to #194

https://claude.ai/code/session_01UCsZXDxBsWV2s4kT47DwDW

[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.

[jgruberf5]

Round-2 minors also addressed @ 20d3d075

Following the two blocker fixes (the reached success-signal so an expired-token cluster no longer stamps; the #203 merge forward-compat), I've now cleared every Minor + Nit so a next round has nothing:

• Findings 3/6/7/9 → removed /resync entirely. You're right it's a zero-caller duplicate of the strictly-better POST /scan?force=true (synchronous, UI-wired, an MCP tool, and — thanks to this PR's stamp change — it now stamps last_synced_at AND commits). Deleted the endpoint + its 3 tests; regenerated openapi.json + api-generated.ts (0 refs remain; --check OK). That moots the swallow-success, cache-staleness, no-throttle, and 403-test items at once.
• Finding 4 (row-lock across the health-gate loop): _execute_health_gate now commit()s each successful scan() iteration (rollback on failure) before its sleep, so the always-dirty last_synced_at UPDATE no longer holds a kubernetes_clusters row lock across the multi-minute loop (aligned with the service's ENG-006 commit-after-step convention).
• Finding 5: rewrote the commit-ownership comment — the stamp is flushed; most callers commit, but get_adaptive_module_plan/_from_scan are read-only and never commit (missed stamp, never wrong data).
• Finding 8: Closes #194 → Relates to #194; filed Honor k8s_sync_enabled / k8s_sync_interval_seconds (no scheduler reader exists) #212 (honor k8s_sync_enabled/k8s_sync_interval_seconds — no scheduler reader exists) and Auto-rescan a cluster's inventory after its modules reach 'applied' #213 (auto-rescan after modules reach applied) for the reporter's stood-behind suggestions 3+4.
• Nits: ClusterOperationResponse docstring tightened; last_synced_at now stamps start_time so it equals scan_metadata.scanned_at.

Verified: 211 passed, ruff + --check clean; re-ran the #203 merge check (applied its analyze_multus one-liner → 5/5 green, no KeyError). Re-requesting.

[jgruberf5]

CI status note

The code review above stands. The only red CI on this PR is the two repo-wide P4 security gates:

• P4 · Docker Build + Scan — Trivy CRITICAL CVE-2026-56854 (golang.org/x/crypto/ssh auth bypass) in the bundled helm 3.20.0 binary.
• P4 · Security Audit — gitpython 3.1.58 (PYSEC-2026-3785/86/87/88).

Both are environmental and repo-wide, not caused by this PR's code: the advisory/vuln DBs updated after staging last audited clean on 2026-08-24, so every open PR (and staging itself, if re-run) is red on them. Both are fixed in #215 (a documented .trivyignore suppression for the un-fixable-yet helm CVE + a gitpython patch bump). This PR goes green on P4 once #215 merges to staging and this branch rebases.

All P1/P2/P3 gates are green. Awaiting further review.

https://claude.ai/code/session_01UCsZXDxBsWV2s4kT47DwDW