Repository navigation
Keep /api/issues fast on clusters with many Helm releases - #2058
Merged
Merged
Conversation
PR Summary by QodoCancel Helm storage reads when callers disconnect
AI Description
Diagram
High-Level Assessment
Files changed (12)
|
Code Review by Qodo
1.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
| helmNamespaces = resolveHelmListNamespaces(ctx, "") | ||
| } | ||
| releases, err := helmClient.ListReleasesAcrossNamespaces(helmNamespaces, username, groups) | ||
| releases, err := helmClient.ListIssueReleasesAcrossNamespaces(ctx, helmNamespaces, username, groups) |
There was a problem hiding this comment.
5. Mcp issue stream logs every client cancel as a helm failure ð Bug â Observability
nativeHelmIssuesForContext now passes the MCP context into ListIssueReleasesAcrossNamespaces, but its error branch still logs every error that is not a forbidden error as 'Failed to list Helm releases for issue stream'. The REST sibling nativeHelmIssuesForRequest was changed in this PR to stay quiet when ctx.Err() != nil, so MCP callers that disconnect or time out now produce false Helm failure logs that look like storage or API problems.
Agent Prompt
## Issue description
The MCP Helm issue stream logs context cancellation as a Helm list failure, unlike the REST handler.
## Fix Focus Areas
- internal/mcp/tools.go[3105-3112]
## Recommended Fix
Do the same as `nativeHelmIssuesForRequest`: if `ctx.Err() != nil`, return nil without logging, and only then fall through to the forbidden and other-error branches.
â Copy this prompt and use it to remediate the issue with your preferred AI generation tools
/api/issues finds failed and stuck Helm releases by listing and decoding every Helm release Secret. That read ran on context.Background(), so when the caller gave up (a closed tab, a proxy timeout) the read kept pulling data from the API server for nobody. The Helm batch upgrade check and the packages inventory read the same way, and the cluster-wide upgrade check read every Secret twice. - Helm release storage reads take a context and stop with it: the Secret list, the decode loop, and row building. ListReleasesAcrossNamespaces, ListReleasesAsUser, ListReleases and the batch upgrade check take a context; the issues handler, Helm page, upgrade check, dashboard summary, packages inventory and MCP tools pass theirs. - The batch upgrade check reads releases and their storage namespaces from one storage snapshot instead of Helm's list action plus, for the cluster-wide scope, a second full Secret read. - Issue detection skips per-release resource health, which it never reads. The issues response still waits for the whole Helm read while the caller does; this change only stops the read when the caller leaves. ListReleasesAcrossNamespaces no longer runs Helm's per-namespace reachability probe, so an unreachable cluster reports the Secret list error instead of "kubernetes cluster unreachable". Nothing here proves that slow Helm reads caused any particular production report; the change was measured on a local test cluster only.
The packages inventory is cached for 60 seconds per namespace scope and shared by every caller. Now that the Helm read stops with its request, a caller that leaves mid-read produced an inventory with no Helm releases and a "context canceled" Helm error, and that result was cached for everyone else asking about the same namespaces. ListPackages now returns the context error instead of caching when the caller has gone, and the REST handler stays quiet in that case.
Helm reads now stop when their caller leaves, which surfaces as a "context canceled" or "context deadline exceeded" error. The REST issues handler already treated that as nobody left to tell. Four other callers did not: - the MCP issue stream and the MCP dashboard logged it as a Helm list failure - the REST dashboard did the same - the Helm release list and the batch upgrade check answered 500, which records a Helm error in the diagnostics log and, for a timeout, asks the connection check whether the cluster is unreachable Each now returns quietly when its own context is done.
One recounted how the handler behaved before this change, and one restated its test's name.
The batch upgrade check now reads release Secrets directly instead of through Helm's list action, and took its Kubernetes client from Radar's process-wide client. A Helm client bound to an explicit rest.Config would then check upgrades against whatever cluster Radar's own client points at. Today only the process-wide Helm client runs upgrade checks, so the two always agree, but the standalone client promises every read targets its own config. Release storage reads now use the receiver's rest.Config when it has one, with impersonation when a user is given, and Radar's client otherwise. A missing Radar client is now an error instead of a nil pointer panic.
Helm, packages and dashboard requests run under the server's 60 second request timeout. When that runs out the context reports a deadline, not a cancellation, and the caller is still waiting. The earlier checks treated both the same, so a Helm read slower than the server's budget answered an empty 504 with nothing logged, and the packages inventory returned nothing instead of its other sources plus a Helm error. Only a canceled context now counts as the caller leaving. A deadline is reported as a failure again, and its partial packages inventory is cached as before. The upgrade impact scan gets the same quiet path for a caller leaving.
Its deadline case answers a Helm 5xx that reads as a connectivity error, which can mark the cluster disconnected for the rest of the package run.
Stuck uninstall timing records a start only for reads that began on the context still connected. The multi-namespace Secret read now carries that context into the history analysis, and no test covered it there.
/api/issues found failed and stuck Helm releases by listing every Helm release Secret with its encoded payload and decoding all of them. A Helm issue only comes from a release whose newest revision is failed or pending, and Helm labels every release Secret with the release name, revision and status. Issue detection now lists the Secret labels alone, then lists and decodes only the releases whose newest revision is failed or pending, plus any release with the same name in that read, so revisions group exactly as a full read groups them. If a release Secret lacks Helm's labels, the read falls back to decoding everything. The same identity makes both lists, so permissions behave as before. Skipped releases still clear their uninstall start, as a full read does. A parity test compares the issue rows with a full read across scopes, statuses, storage namespaces and name collisions.
Issue detection lists Helm release labels and then the selected release Secrets with two clients. Each read the cluster config on its own, so a context switch between the two could list labels on one cluster and Secrets on the other, and that response would miss Helm issues. Both clients now come from the same config.
hisco
force-pushed
the
friction/issues-helm-deadline
branch
from
October 12, 2026 19:04
9f09b5c to
2eedbbf
Compare
hisco
changed the title
hisco
added a commit
to StefanSa/radar
that referenced
this pull request
Oct 13, 2026
- Helm list: the batch upgrade check guessed across all repos, so the list could offer an upgrade from the wrong repo while the drawer refused it. It now uses the recorded source first, like the single-release check. - Pruned versions: a recorded source whose index no longer lists the installed version disabled upgrades. Recorded provenance now only needs the version list from that source; manual association still needs the exact installed version. - Credentialed repository URLs: installing from an alias or a direct repository URL that carries credentials or a query failed with "failed to record chart source". It now installs without provenance, as before, and logs why. Link-local hosts are still rejected. - Concurrent Helm operations: associating a source rewrote the release record and could clobber a running upgrade. It now refuses pending or uninstalling releases, re-reads the release before writing, and returns 409 if it changed. - Private repos: adding a repo that is already configured downloaded its index without the configured credentials and got a 401. It now uses the configured entry. - Permission errors: a Kubernetes denial while associating a source (for example no permission to update the release Secret) returned 500. It now returns 403, like the other release writes. - Source state in the dialog: while the source was loading, or when reading it failed, the dialog said "Not recorded". It now says it is checking, or shows the error with a Retry button. - Repository config writes: adding a repository rewrote repositories.yaml in place, so a concurrent reader could see an empty file, or an entry whose index was not cached yet. The index is now cached first and the config is replaced by rename. - Link-local check: an IPv6 address with a zone (fe80::1%eth0) skipped the check. The zone is now ignored when classifying the address. - Version picker: changing a release's source did not refresh the cached version list. All three source mutations now invalidate it. - Operator notice: it named helm.repositories, which the chart does not have. It now names helm.ociSources only. - Dialog copy: "Verified exact matches" claimed more than the check does. It now says the listed sources publish the chart at the installed version. - Removed TestCandidateOrderingStable, which sorted its own slice and did not exercise production code. - Moved machine: when the recorded repository is not configured here, the dialog showed its URL but made the user retype it into an empty form. It now offers "Add <repository> and use it", which adds the recorded URL and associates it in one click. It keeps the recorded name when that name is free here, and otherwise picks the first free "<name>-N", so a name that points at a different repository is never reused. - Re-associating the recorded repository under a new local name no longer needs the installed version in its index. The release already records that URL, and repositories prune old versions. - Upgrade version picker: when the repository no longer publishes the installed version, the picker left it out, so nothing showed where the release is now. It is now listed in version order as "(current, no longer in the repository)" and cannot be selected. - Helm list on clusters with release Secrets: the faster release read added in skyhook-io#2058 decodes the Secret payload only, and Helm keeps release labels on the Secret itself, so the batch upgrade check never saw the recorded source. The decoded releases now get the Secret's labels, as Helm's own List does.
hisco
added a commit
that referenced
this pull request
Oct 13, 2026
- Helm list: the batch upgrade check guessed across all repos, so the list could offer an upgrade from the wrong repo while the drawer refused it. It now uses the recorded source first, like the single-release check. - Pruned versions: a recorded source whose index no longer lists the installed version disabled upgrades. Recorded provenance now only needs the version list from that source; manual association still needs the exact installed version. - Credentialed repository URLs: installing from an alias or a direct repository URL that carries credentials or a query failed with "failed to record chart source". It now installs without provenance, as before, and logs why. Link-local hosts are still rejected. - Concurrent Helm operations: associating a source rewrote the release record and could clobber a running upgrade. It now refuses pending or uninstalling releases, re-reads the release before writing, and returns 409 if it changed. - Private repos: adding a repo that is already configured downloaded its index without the configured credentials and got a 401. It now uses the configured entry. - Permission errors: a Kubernetes denial while associating a source (for example no permission to update the release Secret) returned 500. It now returns 403, like the other release writes. - Source state in the dialog: while the source was loading, or when reading it failed, the dialog said "Not recorded". It now says it is checking, or shows the error with a Retry button. - Repository config writes: adding a repository rewrote repositories.yaml in place, so a concurrent reader could see an empty file, or an entry whose index was not cached yet. The index is now cached first and the config is replaced by rename. - Link-local check: an IPv6 address with a zone (fe80::1%eth0) skipped the check. The zone is now ignored when classifying the address. - Version picker: changing a release's source did not refresh the cached version list. All three source mutations now invalidate it. - Operator notice: it named helm.repositories, which the chart does not have. It now names helm.ociSources only. - Dialog copy: "Verified exact matches" claimed more than the check does. It now says the listed sources publish the chart at the installed version. - Removed TestCandidateOrderingStable, which sorted its own slice and did not exercise production code. - Moved machine: when the recorded repository is not configured here, the dialog showed its URL but made the user retype it into an empty form. It now offers "Add <repository> and use it", which adds the recorded URL and associates it in one click. It keeps the recorded name when that name is free here, and otherwise picks the first free "<name>-N", so a name that points at a different repository is never reused. - Re-associating the recorded repository under a new local name no longer needs the installed version in its index. The release already records that URL, and repositories prune old versions. - Upgrade version picker: when the repository no longer publishes the installed version, the picker left it out, so nothing showed where the release is now. It is now listed in version order as "(current, no longer in the repository)" and cannot be selected. - Helm list on clusters with release Secrets: the faster release read added in #2058 decodes the Secret payload only, and Helm keeps release labels on the Secret itself, so the batch upgrade check never saw the recorded source. The decoded releases now get the Secret's labels, as Helm's own List does.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What is wrong today
/api/issuesfinds failed and stuck Helm releases by listing every Helm release Secret with its full payload and decoding all of them, on every request. The cost grows with the number of releases and their history. On a kind cluster with real chart payloads (10 revisions per release, about 120 KB per Secret), main takes 8 s at 150 releases and 13 s at 200, and Radar's memory peaks above 1 GB during the read.Radar Cloud gives each cluster 7 s. Past that, a scoped Issues page shows "Issues query failed for this scope" and Home shows the cluster as errored, even though the cluster is connected and healthy.
The read also runs on
context.Background(), so when the caller gives up it keeps pulling data from the API server for nobody. The batch upgrade check and the packages inventory read the same way.What the change does
Issue detection reads labels first. Helm labels every release Secret with the release name, revision and status. A Helm issue can only come from a release whose newest revision is failed or pending (
helmhistory.Analyzeraises nothing else), so:failed,pending-install,pending-upgrade,pending-rollbackoruninstalling, plus any release with the same name in that read.Both lists run as the caller, with the same verb (
list secrets), so permissions and the 403 handling don't change. If any release Secret lacks Helm's labels, that scope falls back to the full read. The Helm page, upgrade check and dashboard still do the full read; they need every release.Helm reads also take a context and stop with it: the Secret list, the decode loop and row building. A caller leaving is not logged or reported as a Helm failure, and the packages inventory, cached per namespace scope for every caller, is not cached from a read cut short. A read that outlasts the server's 60 s request timeout is still reported as a failure.
Smaller pieces:
action.NewListplus, cluster-wide, a second full read.rest.Configwhen it has one.Where to look hardest
issueStatusesininternal/helm/issue_releases.go. Skipping a release is only safe if no other newest status can raise an issue.TestOnlyIssueStatusesRaiseIssuesruns every Helm status throughhelmhistory.Analyzeto hold that.storageNamespace) forms one history whose newest revision wins. Reading every release that shares a name with a failed or pending one keeps that grouping intact.forgetUninstallStarts), as the full read does for any release that isn't uninstalling.ListReleasesAcrossNamespacesno longer runs Helm's per-namespace reachability probe. An unreachable cluster now reports the Secret list error instead of "kubernetes cluster unreachable".Evidence
kind cluster, real chart payloads (kube-prometheus-stack, cert-manager, ingress-nginx, redis, podinfo), 10 revisions per release, one in 25 releases failed. Radar's own
/api/issues, warm, 5 runs (3 for main at 200). The laptop was heavily loaded during these runs, so absolute numbers are noisy; the ratio held in every run.Peak RSS is the whole Radar process. Most of what remains on this PR is Radar's Secret informer cache, which holds the same release Secrets (425 MB idle at 300 releases).
The Helm issue rows were identical between main and this PR on the same cluster, including releases added in pending-install, pending-upgrade, pending-rollback and uninstalling states, an install failure, a rolled-back upgrade, one name failed in one namespace and healthy in another, and a release stored in two namespaces.
Through a local Radar Cloud stack at 150 releases (one cluster):
With this PR's Radar behind the hub and web from main, the same pages answer at 0.7 s, so the Radar change alone is enough.
Cancellation, local KWOK cluster, 400 release Secrets (about 210 MB), link to the API server throttled to 20 MB/s, caller gives up after 2 s:
Deliberately not in scope
Tests
TestIssueReleasesMatchAFullReadcompares the label read with the full read on one fixture for cluster-wide, every-namespace, one-namespace and empty scopes: failed install, failed upgrade, stuck and fresh pending states, stuck and fresh uninstalls, rolled-back and since-fixed failures, uninstalled and unknown releases, one name in two namespaces, a release stored away from its namespace, and one stored in two namespaces. Every returned row must equal the full read's row field for field.TestIssueReleasesReadOnlyFailedOrPendingReleasesInFullserves 62 release Secrets from a fake API server and checks only the failed release's 2 are sent in full. It fails on the full read.TestIssueReleasesFallBackToAFullReadWithoutHelmLabelsandTestOnlyIssueStatusesRaiseIssues.TestIssuesStopReadingHelmStorageWhenCallerGivesUppasses here and fails when the Helm reads are put back oncontext.Background(). Further tests cover the packages read and its cache, the batch upgrade check, list and decode cancellation, and a caller leaving not being logged or recorded as a Helm error.go test -racepasses forinternal/helm,internal/server,internal/mcpandinternal/issues.go vetpasses forinternal/helm,internal/mcpandinternal/issues.internal/serverhas the same 4 "copies lock" findings in test files as main.