Repository navigation
fix: leaks, hangs and visible bugs from the idiom audit - #6026
Merged
norman-abramovitz merged 121 commits intoOct 19, 2026
Merged
norman-abramovitz merged 121 commits into
norman-abramovitz merged 121 commits into
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
toObservable() creates its effect when called and ends it only when the injector is destroyed, so every call with the root injector leaves an effect running for the rest of the session. observeSignal() creates the effect on subscribe and destroys it on unsubscribe, with the same first-emission timing. combineLatestOrEmpty() emits [] for an empty list, where combineLatest([]) completes without a value and leaves take(1) and loading indicators waiting. createSignalWrapper() is a shared copy of the callable signal wrapper that four files define privately. Its asObservable() is backed by a BehaviorSubject: synchronous, one emission per write, no injection context, and it outlives the view that created it. Callers move to these helpers in the following commits.
The feature-flag check built a toObservable() on the root injector for every subscription, and each one kept an effect running for the rest of the session (non-admin users, on every CF endpoint page, orgs list and manage-users). Watch the flags with observeSignal(), which ends with the subscription. The build-after-load factory stays.
isAutoscalerEnabled() and fetchAutoscalerInfo() created a root toObservable() effect at call time, one per app page visit, even when nothing subscribed. Use observeSignal(), which starts on subscribe and stops on unsubscribe.
Every API lookup for a registered private or enterprise git endpoint created a root toObservable() effect that take(1) could not end; the deploy-from-git name validator did it once per debounced keystroke. Use observeSignal() so take(1) ends the watch.
The fallback branch of CurrentUserPermissionsService watched a fresh endpointById() computed through a root toObservable() per call, which outlived its subscribers. Use observeSignal().
The release shell is a reused route, so its helper service lives across tab switches. Each tab visit and graph node click built a new toObservable() on that injector, and the effects piled up until the user left the release. Build the per-call streams with observeSignal() so they end with their subscription.
The import passed each row's signal wrapper to subscribe() as the observer. RxJS took the callable as next(), and the wrapper's callable only reads its signal, so every connect status was dropped: each Connect row stalled and the clusters after it were never imported. Forward each status to the row explicitly, complete the row as failed when the connect errors, and move on to the next action when a row has no user or no usable auth data.
asObservable() returned a new observable on every call. A template that binds it directly would resubscribe on every change detection pass and get the BehaviorSubject's last value replayed each time. Build it once, as the private copies did.
The deployer's signal wrappers used toObservable() on the step's injector. Leaving the deploy page destroyed that injector in the same pass that subscribed the completion notice, so the stream completed empty: no success or failure snackbar and no close(). toObservable() also coalesced writes per change detection pass, so a burst of log frames lost lines in the deploy log. Use the shared BehaviorSubject-backed createSignalWrapper(), and build the deploy state stream once instead of a new toObservable() on every open().
Drop the private createSignalWrapper() copies in the kube config helper, the config import and the namespaces filter for the store one. The import's per-row observable cache goes too: asObservable() now returns the same instance on every call.
With no CF endpoint connected, the per-endpoint scope, role and feature-flag checks combined an empty list with combineLatest(), which completes without a value, so they never answered. can([]) did the same in CurrentUserPermissionsService. Use combineLatestOrEmpty(): an empty set answers false, and an empty permission config is denied. reduceChecks([]) answered true for both combinators, so with no CF connected the admin check said "admin" and every org, space and complex CF permission without an endpoint passed (the app wall showed Create Application). An empty list now gives some([]) = false for || and every([]) = true for &&.
A scope check across all connected CF endpoints mapped each guid to a check of the outer, undefined endpoint guid, so every element answered false and a user with the scope on a connected CF was denied (e.g. the read-only admin and global auditor entries of APPLICATION_VIEW and SPACE_VIEW, FIREHOSE_VIEW). Check each endpoint's own guid.
Step 1 started the scope check, then read the setup state through a toObservable() stream that still replayed the previous value, so the first click succeeded with no scopes whatever the credentials were, and a retry reported the previous attempt. Finish could then save an empty admin scope, which leaves the backend in setup mode. Finish waited with take(10) on replayed streams; when the backend never confirmed it ended in an EmptyError without clearing the "Saving configuration" overlay, which hid the error until a reload. Await each request and read the state it left. Step 1 fails when the UAA returns no scopes; Finish is enabled only for a returned scope. After the save, waitForSetupConfirmed() polls session verification with a 30 s deadline, and any failure clears the overlay so the error shows.
The local account Finish waited on replayed streams with take(10). When the save failed or the backend never left setup mode the wait ended in an EmptyError and the "Saving configuration" overlay stayed up over the error. Await the save, read its state, confirm with waitForSetupConfirmed(), and clear the overlay on any failure.
UaaSetupSignalService only re-exposed UaaSetupDataService.state for the two setup wizards, which now read the data service directly.
A scope check or save that never answered kept the wizard busy, or the "Saving configuration" overlay up, forever. Time the requests out: the check after 130 s (the backend's own UAA call gives up at 120 s, so its clearer error arrives first) and the save after 60 s, with a message saying the server did not answer.
A failed scope check in step 1 was reported as "Failed to save configuration." although nothing was saved. Say "Failed to check the UAA credentials." instead; the save wording is unchanged.
A register failure only skipped the cluster's connect when the error carried a message, so a failure without one left the connect queued against an endpoint that was never registered, and the register row showed nothing. Skip on any failure and fall back to "Failed to register endpoint".
getAuthDataForConnect() read authTypes[type].component for a type the catalog does not know, and the TypeError was thrown inside the previous row's subscriber, which stopped the import. Throw a clear "Unsupported authentication type" error from the helper, and have the import mark that connect failed and move on when the helper throws.
getAuthDataForConnect() creates the auth type's form component to read its values, and destroyed it only when nothing threw on the way, so a failing getValues() or getBody() left it alive. Destroy it in a finally block.
The fixture used authType 'token', which parseAuth() never produces. Use a client certificate user and the kube-cert-auth data parseAuth() builds for it.
A connect that failed without a message, resolved or rejected, left the import row with an empty error. Fall back to "Failed to connect endpoint".
The Add User and Manage Roles picker showed "Loading organizations..." until a reload in three cases: - No orgs: the per-org edit check combined an empty list with combineLatest(), which completes without a value. - A failed fetch: catchError() turned it into that same empty list. - Every later open: publishReplay() on the root service cached the dead stream, so no new request was sent. The component cleared its spinner only on next. Combine with combineLatestOrEmpty(), let a failed fetch error and cache it with shareReplay(), which resets on error, and clear the spinner on next, error and complete. A failure shows "Could not load organizations." with a Retry button. The component spec now awaits whenStable().
The org-name lookup for existing roles caught a failed request before publishReplay(), so "no names" was cached on the root service for the rest of the session. Cache with shareReplay(), which resets on error, and recover outside the cache: the call still resolves without names and the next one fetches again.
The roles service is a root singleton. Its existing and new roles
streams used publishReplay(1), whose subject outlives the refCount,
and toObservable() of the selected users, which holds the last value
it pushed. A new Manage Roles dialog therefore got the previous
dialog's roles first, and the picker could pre-pick the previous
users' orgs. Watch the users with observeSignal() and share with
shareReplay({ refCount: true }) so each dialog starts clean.
With no organizations to manage and nothing searched, the role picker said "No organizations match." Say "You have no organizations where you can manage roles." instead; a search with no hits keeps the old text.
Expanding an org whose space fetch failed left "Loading spaces..." up for good and raised an unhandled error. Clear the spinner on next, error and complete, and show "Could not load spaces." with a Retry for that org instead of "No spaces in this organization."
The users/remove page lost its last in-app link when the old users list config was deleted in 44cacd1. The users tabs now remove users with inline bulk actions (Remove from Space, Remove from Spaces, Remove from Org and Spaces), so the page was only reachable by typing its URL, and it hung on an empty selection and fetched each user four times. Remove its three routes, the component, its URL helper and its e2e page object, and reword the comments that pointed at it.
The page read its users from the users= / user= query parameter: - One lookup per guid in combineLatest() with no error handler, so a single deleted or mistyped guid failed the whole stream and the roles step spun forever. - The cold stream had two subscribers (the title and the seeding), so each user was fetched twice. - users=, gave no guids and combineLatest([]) never emitted; users= fell through to empty wizard state. Parse the guids once, drop empty ones, and load each user once with its own error handling. The page shows the users that loaded with an error naming each guid that did not, and "No users selected" (or "None of the selected users could be loaded") with a link back when none load. The spec now awaits whenStable().
observe() created a root effect and dropped its ref, so stop() only cleared the current timer: a later request change fetched again and re-armed polling. Keep the ref, clear the timer in the effect cleanup and destroy the effect in stop(). The kube node metric stream now uses observeSignal so its watch ends when the stats card unsubscribes.
The org and space services looked their quota up again on every emission of the org or space, so a rename, which patches the org or space but keeps its quota, fetched the same quota again. Skip repeated quota GUIDs.
The no-endpoints customization never rendered. Since the strict-mode change the view query named a template ref that does not exist, so the container was undefined and the create call threw inside a try/catch. The template now renders the custom component with ngComponentOutlet in place of the default message, and the unused templates and subscription are gone. The persistence-flag check now also ends with the page, so a flag that never arrives no longer holds the destroyed component.
Each connect lifted three endpoint signals into observables with toObservable(). Those effects end only with the injector they are given, which is the page or stepper that outlives every connect, so each connect (dialog, stepper entry, kube cluster import) left three effects running after destroy(). Keep the effect and replay subject for each bridge and end them in destroy(). Values and timing are unchanged: one effect per signal, created at construction, latest value replayed to later subscribers.
The connect step of the register stepper made a new connect each time it was entered and destroyed only the last one when the stepper closed. Going back with Previous and on with Next left the earlier connect's subscriptions and signal bridges running. Destroy the previous connect first.
CfInfoDataRegistry, OrgDataRegistry and SpaceDataRegistry counted acquire/release calls, but nothing read the count and nothing evicted at zero; entries live for the session and leave only through evict() when an org or space is deleted. Remove refCount, release() and the destroy hooks and effects that existed only to call it, and say in each registry that entries are sticky on purpose.
The Broker Link row held only a launch icon, so screen readers announced the link as "launch". Show the broker URL as the link text, hide the icon from assistive tech, and say that the link opens in a new tab. Add rel="noopener noreferrer" to match the other new-tab links.
When a kubernetes endpoint was disconnected or unregistered, only the per-endpoint registry entry was dropped. Pods, nodes, services, the generic resource caches (Secrets included), analysis reports and helm releases kept the data fetched with the old credentials for the rest of the session. Add a small root eviction bus. Each cache registers an evictor and the endpoint hook calls it on departure, so every cache drops the departed endpoint's entries and leaves other endpoints alone. Remove the namespace service's direct-fetch cache, which was written but never read.
The apps list config is a root singleton, so the stats poll and the page-change effect it started kept running after the user left the page: one app-stats request per Cloud Foundry every 30 seconds until reload. Callers now pass their DestroyRef and the poll stops when that page is destroyed. A later visit starts a fresh poll, and a page that is already gone does not start one. The application wall also kept its create-permission subscription alive after it was destroyed; it now ends with the wall.
The endpoint name cell made a new signal-to-observable bridge each time its row changed, and each bridge kept watching the endpoints map until the cell was destroyed. Read the endpoint through one computed signal keyed on the row's id instead, and render it directly from the signal. The template-facing field endpoint$ becomes the signal endpoint, and an endpoint removed from the map now renders nothing rather than its last name.
A failed reload of a helm release's detail or history left the data fetched before it in place. After a reconnect with narrower credentials the page went on showing release data the user can no longer read. Blank the detail and history signals on error, the way endpoint eviction does, so the page keeps its signals and shows the empty state.
The kubernetes data caches capped their error lists with slice(0, 50), which keeps the oldest entries: once 50 errors existed, new ones were never recorded. Keep the newest 50. Errors recorded against an endpoint stayed after it was disconnected or unregistered. Each cache's eviction now also drops errors that affect only the departed endpoint, and the namespace cache registers an evictor for its errors. Pod errors were never cleared. A pod error now names its scope, and a later successful load of that scope clears it, as the analysis cache does for its endpoints.
A full apps drain (loadApps/refreshApps) replaced the app list but left the recent apps list as load() fetched it, so the home card kept deleted or renamed apps and old states after a refresh. Rebuild it from the drain with the backend's rule: newest updatedAt first, capped at ten. removeApp only filtered the recent list, so it shrank below ten after a delete. Refill the freed slot from the full list when one is loaded.
The recent apps card could source its own apps from the endpoint data registry when no allApps$ was given. Every caller passes allApps$, so that branch never ran; it acquired registry data it never released and bridged signals on the component injector. Remove it.
load() never started loadDetails(); detail views load the full lists themselves. Say so.
The service broker card and the broker table cell started a space load in an effect and never ended it, so the subscription stayed open after the broker's space changed or the component was destroyed. The effect now keys on the space by value, ends the load in its cleanup, and runs the acquire untracked so the load's own signals do not re-run it.
Eviction cleared the root kube caches but not the requests already on the wire, so a response landing after a disconnect wrote the old credentials' data (or error) back. The eviction bus now keeps a per-endpoint epoch; pods, analysis, node, service and generic resource loads check it before writing and drop a late response. namespacesForEndpoint held the per-endpoint service it got at call time; after the endpoint left and came back, a page kept reading the orphaned instance. The projection now looks the service up again after each eviction.
A releases request in flight across an eviction wrote the departed endpoint's releases back; the load now snapshots every endpoint's eviction epoch when it starts and drops the evicted ones. A pod request finishing after an eviction or refresh deleted the newer request's dedupe entry, so the next read sent a duplicate. It now removes only its own entry. Pod reads no longer touch the endpoint registry: the connect hook creates the entry, and a read for a departed endpoint re-created it. An analysis list request replaced by refresh() still wrote its result and dropped the newer request's handle. A response now writes only while the cache entry still holds its request.
A short invalid session (a failed keepalive near expiry) stops the user activity listener. The listener was re-created only when it had never existed, and a closed Subscription still counts as existing, so it never came back. The user then looked idle and was warned and logged out while working. Re-create it when it is closed, as the session poll already does.
A chart with a projected time range selector subscribed to it in ngAfterContentInit with takeUntilDestroyed() and no DestroyRef. That hook is outside the injection context, so it threw NG0203 and the chart never rendered. Pass a DestroyRef injected as a field.
The keepalive verify runs near session expiry. When it fails, auth resets to the default state, which since 2025-11 starts with verifying set for app init. Nothing verifies again, and the auth guard waits for verifying to clear, so every later navigation hung until a reload. Reset with verifying cleared: the next navigation goes to the login page, which verifies and returns the user if the session is still valid.
Release note for cloudfoundry#6019, cloudfoundry#6020 and cloudfoundry#6021: the pages that waited forever, the views that did not show, the background work that kept running, and the removed users/remove page.
The scaling history and metric history handlers always forwarded
start-time, end-time, page, results-per-page and order, empty or not.
The frontend sends the sort as order-direction, so order went out
empty, and the autoscaler (14.x and later) rejects that with 400
("order": invalid value). Every bound app's Autoscale tab showed the
raw HTTP error, and the metric charts stayed empty.
Forward only the parameters the caller sent, including
order-direction, which both endpoints read (order is deprecated and
ignored on scaling histories).
Since the V3 cutover (fff9dc8) ApplicationService needs AppDetailDataService and the services it depends on, which only the app page's base component provided. The autoscaler pages provided ApplicationService alone, so the policy editor, credential, metric and history pages all failed with NG0201 and rendered nothing. Export the app page's provider list as APP_DETAIL_PROVIDERS and use it for both base components.
nabramovitz
force-pushed
the
norm/fix/6019-6021-audit
branch
from
October 19, 2026 18:27
920092b to
5858eb4
Compare
norman-abramovitz
approved these changes
Oct 19, 2026
norman-abramovitz
left a comment
norman-abramovitz
left a comment
Contributor
There was a problem hiding this comment.
LGTM - Live testing will be able to find more things broken
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.
The fixes for #6019 (leaks), #6020 (hangs) and #6021 (visible bugs), one commit per fix. Each fix has a unit test that fails on the code before it and passes after. Rebased on develop; release note in
changelog.d/0013-audit-leaks-hangs-rendering.md.Shared pieces.
@stratosui/storegains three helpers inhelpers/signal-interop.ts:observeSignal(source, injector): unliketoObservable(), whose effect starts at call time and ends only with the injector, its effect starts on subscribe and ends on unsubscribe.combineLatestOrEmpty(sources): emits[]for an empty list, wherecombineLatest([])completes without a value and leavestake(1)callers and spinners waiting.createSignalWrapper(): one BehaviorSubject-backed copy of the callable signal wrapper that four files defined privately.#6019, leaks.
observe()job stop with their page; the stats poll used to send one request per Cloud Foundry every 30 s until a reload.#6020, hangs.
waitForValue$()errors when the lookup fails; deploy step 2 reports it.verifyingset).#6021, visible bugs.
Removed. The
users/removepage and its three routes; it had no in-app link since 44cacd1, and the users tabs remove users inline.Test helpers. Two cloud-foundry test providers bound the wrong HTTP backend, so every request in 29 specs threw. All 29 pass before and after the fix.
Fixes #6019, fixes #6020, fixes #6021.
AI disclosure (RFC-0047): prepared with AI coding assistance; reviewed by the author.