NO-ISSUE: Fix NS_BINDING_ABORTED noise and lifecycle cache signal coupling#16773
Open
perdasilva wants to merge 5 commits into
Open
NO-ISSUE: Fix NS_BINDING_ABORTED noise and lifecycle cache signal coupling#16773perdasilva wants to merge 5 commits into
perdasilva wants to merge 5 commits into
Conversation
Contributor
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
Contributor
|
@perdasilva: This pull request explicitly references no jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
Bot
added
component/olm
Contributor
WalkthroughChangesAbort handling and lifecycle caching
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 13 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (13 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
When useOperatorLifecycle re-runs (deps changed) while a fetch is in-flight, fetchLifecycleData returns the existing cached promise (which was created with the previous AbortController's signal). When the old controller is then aborted, that promise rejects with AbortError. The .catch handler only checked !controller.signal.aborted (the NEW controller, which is not aborted), so the AbortError was incorrectly surfaced as a real error, producing NS_BINDING_ERROR noise in the browser console. Fix: also skip setError when err.name === 'AbortError', while still calling setLoading(false) so the loading state settles correctly. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Fix A — console-fetch.ts: Don't console.warn for AbortErrors. Aborted fetches are expected when AbortController.abort() is called (component unmount, dep change), not failures. The warn produced NS_BINDING_ABORTED noise in Firefox devtools. Fix B — useOperatorLifecycle.ts: When fetchLifecycleData returns a cached in-flight promise tied to a different caller's signal, chain a .catch() that detects if the original was aborted by someone else while our signal is still live and retries with a fresh request. Previously, the spurious AbortError would propagate to the hook's .catch and leave the row in an empty non-loading state. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
perdasilva
force-pushed
the
NO-ISSUE-lifecycle-abort-error-fix
branch
from
July 17, 2026 11:52
f74b63a to
4068438
Compare
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: perdasilva The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test-prow-playwright-e2e.sh (1)
10-14: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUsage comment doesn't list the new
feature-gatescenario.The script now accepts
feature-gateas a scenario (line 62), but the usage comment (line 10) still says[e2e|release|smoke]and the scenario list (lines 13-14) omits it.📝 Proposed fix
# ./test-prow-playwright-e2e.sh [e2e|release|smoke|feature-gate] [arguments passed to: playwright test ...] # # Scenarios (first argument; default: e2e): # e2e, release — full Playwright suite (default project / config) # smoke — Playwright smoke project (--project=smoke) +# feature-gate — Playwright tests tagged `@feature-gate` (--grep `@feature-gate`)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test-prow-playwright-e2e.sh` around lines 10 - 14, Update the usage documentation for test-prow-playwright-e2e.sh to include feature-gate in the accepted scenario list and describe its behavior alongside e2e, release, and smoke, matching the scenario handling implemented in the script.
🧹 Nitpick comments (2)
test-prow-e2e-techpreview.sh (1)
18-20: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider adding
ARTIFACT_DIRvalidation for consistency with the Playwright techpreview script.The Playwright techpreview script (
test-prow-playwright-e2e-techpreview.sh, lines 22-30) validates thatARTIFACT_DIRis set, absolute, and not/. This script lacks that validation. Withset -u, an unsetARTIFACT_DIRfails cryptically at line 20; an empty or relative value would silently produce a brokenINSTALLER_DIR.🛡️ Proposed fix
set -exuo pipefail +ARTIFACT_DIR=${ARTIFACT_DIR:-/tmp/artifacts} INSTALLER_DIR=${INSTALLER_DIR:=${ARTIFACT_DIR}/installer} + +if [ -z "$ARTIFACT_DIR" ]; then + echo "Error: ARTIFACT_DIR is not set" >&2 + exit 1 +fi +case "$ARTIFACT_DIR" in + /) echo "Error: ARTIFACT_DIR must not be '/'" >&2; exit 1 ;; + /*) ;; + *) echo "Error: ARTIFACT_DIR must be an absolute path, got: $ARTIFACT_DIR" >&2; exit 1 ;; +esac +export ARTIFACT_DIR +mkdir -p "${ARTIFACT_DIR}"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test-prow-e2e-techpreview.sh` around lines 18 - 20, Add validation immediately after set -exuo pipefail in the techpreview script to ensure ARTIFACT_DIR is set, non-empty, absolute, and not /. Keep the existing INSTALLER_DIR defaulting behavior, but make it run only after validation so unset, empty, or relative values fail with a clear error.frontend/integration-tests/tsconfig.json (1)
5-5: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueDrop the Cypress path from
include
frontend/integration-tests/tsconfig.jsoncan keeptypes: ["cypress"]; the../node_modules/cypressglob only widens the set of files TypeScript scans.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/integration-tests/tsconfig.json` at line 5, Update the include configuration in frontend/integration-tests/tsconfig.json to remove the ../node_modules/cypress entry, leaving only the integration test TypeScript glob. Preserve the existing types configuration for Cypress.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/superpowers/specs/2026-07-16-techpreview-e2e-test-filtering-design.md`:
- Around line 75-79: Update the spec’s techPreviewOnly fixture description to
match the implementation in the techPreviewOnly fixture: query the FeatureGate
custom resource through k8sClient.getClusterCustomResource and skip unless
spec.featureSet equals TechPreviewNoUpgrade, removing the page.evaluate and
window.SERVER_FLAGS.techPreview behavior.
In `@frontend/integration-tests/support/tech-preview-guard.ts`:
- Around line 26-27: Update requireFeatureGate in tech-preview-guard.ts so it
evaluates enabled gates only from the FeatureGate status entry matching
spec.featureSet, rather than aggregating all feature sets. Fetch the full
FeatureGate resource and filter its parsed JSON in JavaScript, matching the
active-feature-set handling in the Playwright requireFeatureGate fixture.
- Around line 14-22: Remove the top-level before() side effect in
tech-preview-guard.ts and export the guard as a named function such as
requireTechPreview(). Keep its existing featureSet check and skip behavior, and
require callers to explicitly invoke it when they need the TechPreview-only
guard while allowing requireFeatureGate imports to run on graduated or otherwise
enabled clusters.
In
`@frontend/packages/operator-lifecycle-manager/src/hooks/useOperatorLifecycle.ts`:
- Around line 54-57: Update the AbortError recovery in fetchLifecycleData to
delete cacheKey only if the cache still references the rejected promise being
handled, preserving a retry installed by another caller; otherwise reuse the
existing retry. Add a regression test covering three callers sharing the aborted
promise and verifying only the first retry is retained.
---
Outside diff comments:
In `@test-prow-playwright-e2e.sh`:
- Around line 10-14: Update the usage documentation for
test-prow-playwright-e2e.sh to include feature-gate in the accepted scenario
list and describe its behavior alongside e2e, release, and smoke, matching the
scenario handling implemented in the script.
---
Nitpick comments:
In `@frontend/integration-tests/tsconfig.json`:
- Line 5: Update the include configuration in
frontend/integration-tests/tsconfig.json to remove the ../node_modules/cypress
entry, leaving only the integration test TypeScript glob. Preserve the existing
types configuration for Cypress.
In `@test-prow-e2e-techpreview.sh`:
- Around line 18-20: Add validation immediately after set -exuo pipefail in the
techpreview script to ensure ARTIFACT_DIR is set, non-empty, absolute, and not
/. Keep the existing INSTALLER_DIR defaulting behavior, but make it run only
after validation so unset, empty, or relative values fail with a clear error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: affb4291-ecb3-465d-a419-2f0563bb14dc
📒 Files selected for processing (16)
.gitignoredocs/superpowers/plans/2026-07-16-techpreview-e2e-test-filtering.mddocs/superpowers/specs/2026-07-16-techpreview-e2e-test-filtering-design.mdfrontend/.eslintignorefrontend/e2e/fixtures/index.tsfrontend/e2e/tests/olm/olmv1-catalog.spec.tsfrontend/integration-tests/.eslintrc.cjsfrontend/integration-tests/support/tech-preview-guard.tsfrontend/integration-tests/tsconfig.jsonfrontend/packages/console-shared/src/utils/__tests__/console-fetch.spec.tsfrontend/packages/console-shared/src/utils/console-fetch.tsfrontend/packages/operator-lifecycle-manager/src/hooks/__tests__/useOperatorLifecycle.spec.tsfrontend/packages/operator-lifecycle-manager/src/hooks/useOperatorLifecycle.tstest-prow-e2e-techpreview.shtest-prow-playwright-e2e-techpreview.shtest-prow-playwright-e2e.sh
Author
|
/retest |
When N callers share the same aborted cached promise, the previous fix unconditionally deleted the cache before retrying, causing each caller to start its own redundant fetch and overwrite the previous retry. Fix: capture the promise reference before .catch() and only delete when the cache still holds that exact promise. The first caller to run the handler starts the single retry; all later callers see a newer promise in the cache and reuse it via the recursive fetchLifecycleData call. Adds a three-caller regression test. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
The NS_BINDING_ABORTED errors are inherent to the abort-on-unmount pattern: every row unmount (sort, filter, navigation) cancels its in-flight lifecycle request, Firefox logs the cancellation, and the cache-sharing fixes we added are complex responses to a self-inflicted problem. Root fix: let requests complete naturally. - Remove AbortController, abortRef, cleanup abort, and all signal-passing code from useOperatorLifecycle. - Simplify fetchLifecycleData: no more AbortError special-casing, Fix-B retry logic, or multi-caller stomp guard — just deduplicate concurrent requests via the module-level cache and let every request reach the backend exactly once. - When a component unmounts mid-flight the request finishes, populates the cache, and React silently discards the resulting state update. The next render of the same operator (after sort/filter/remount) gets an instant cache hit. - Drop the abort-specific test cases; replace with deduplication and cache-hit tests that describe the new invariants. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@frontend/packages/operator-lifecycle-manager/src/hooks/useOperatorLifecycle.ts`:
- Around line 100-112: Guard the fetchLifecycleData callbacks in the
useOperatorLifecycle effect with an active/cancelled flag, and mark the effect
inactive during cleanup so superseded requests cannot update data, error, or
loading state. Preserve request caching and return the cleanup function from the
effect. Add a regression test that rerenders from A to B, resolves B before A,
and verifies B remains displayed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7995f5f3-1484-4bc8-933b-56ba13a73fd8
📒 Files selected for processing (2)
frontend/packages/operator-lifecycle-manager/src/hooks/__tests__/useOperatorLifecycle.spec.tsfrontend/packages/operator-lifecycle-manager/src/hooks/useOperatorLifecycle.ts
Comment on lines
+100
to
+112
| fetchLifecycleData(cacheKey, url) | ||
| .then((result) => { | ||
| if (!controller.signal.aborted) { | ||
| setData(result); | ||
| setError(null); | ||
| setLoading(false); | ||
| } | ||
| setData(result); | ||
| setError(null); | ||
| setLoading(false); | ||
| }) | ||
| .catch((err: Error) => { | ||
| if (!controller.signal.aborted) { | ||
| setData(null); | ||
| setError(err); | ||
| setLoading(false); | ||
| } | ||
| setData(null); | ||
| setError(err); | ||
| setLoading(false); | ||
| }); | ||
|
|
||
| return () => { | ||
| controller.abort(); | ||
| }; | ||
| return undefined; // no cleanup: requests complete naturally |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Prevent superseded requests from updating this hook.
When the inputs change while request A is pending, request B starts; if B resolves first and A resolves later, A unconditionally overwrites B’s state. Keep the request cacheable, but ignore callbacks from a cleaned-up effect. Add a regression test that rerenders from A to B, resolves B then A, and verifies B remains displayed.
Proposed fix
useEffect(() => {
+ let active = true;
+
// ...
fetchLifecycleData(cacheKey, url)
.then((result) => {
+ if (!active) return;
setData(result);
setError(null);
setLoading(false);
})
.catch((err: Error) => {
+ if (!active) return;
setData(null);
setError(err);
setLoading(false);
});
- return undefined; // no cleanup: requests complete naturally
+ return () => {
+ active = false;
+ };
}, [packageName, catalogName, catalogNamespace]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| fetchLifecycleData(cacheKey, url) | |
| .then((result) => { | |
| if (!controller.signal.aborted) { | |
| setData(result); | |
| setError(null); | |
| setLoading(false); | |
| } | |
| setData(result); | |
| setError(null); | |
| setLoading(false); | |
| }) | |
| .catch((err: Error) => { | |
| if (!controller.signal.aborted) { | |
| setData(null); | |
| setError(err); | |
| setLoading(false); | |
| } | |
| setData(null); | |
| setError(err); | |
| setLoading(false); | |
| }); | |
| return () => { | |
| controller.abort(); | |
| }; | |
| return undefined; // no cleanup: requests complete naturally | |
| useEffect(() => { | |
| let active = true; | |
| // ... | |
| fetchLifecycleData(cacheKey, url) | |
| .then((result) => { | |
| if (!active) return; | |
| setData(result); | |
| setError(null); | |
| setLoading(false); | |
| }) | |
| .catch((err: Error) => { | |
| if (!active) return; | |
| setData(null); | |
| setError(err); | |
| setLoading(false); | |
| }); | |
| return () => { | |
| active = false; | |
| }; | |
| }, [packageName, catalogName, catalogNamespace]); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@frontend/packages/operator-lifecycle-manager/src/hooks/useOperatorLifecycle.ts`
around lines 100 - 112, Guard the fetchLifecycleData callbacks in the
useOperatorLifecycle effect with an active/cancelled flag, and mark the effect
inactive during cleanup so superseded requests cannot update data, error, or
loading state. Preserve request caching and return the cleanup function from the
effect. Add a regression test that rerenders from A to B, resolves B before A,
and verifies B remains displayed.
Contributor
|
@perdasilva: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
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
2 participants
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.
Analysis / Root cause:
When
useOperatorLifecyclere-runs (e.g. subscription loads after CSV, ormultiple components share the same cache key),
fetchLifecycleDatareturns thecached in-flight promise tied to the previous caller's AbortController signal.
When that old signal is aborted, the AbortError propagates to the new caller
whose signal is still live — three problems result:
coFetchcallsconsole.warnfor every abort, flooding Firefox devtoolswith
NS_BINDING_ABORTEDnoise on the Installed Operators page..catchonly checked!controller.signal.aborted(the newcontroller), not whether the error was an AbortError, so it incorrectly
surfaced the error in UI state.
stalled in an empty non-loading state instead of retrying.
Solution description:
console-fetch.ts: Don'tconsole.warnforDOMExceptionAbortErrors —they are expected cancellations from
AbortController.abort(), not failures.useOperatorLifecycle.ts(.catchguard): SkipsetErrorwhenerr.name === 'AbortError'regardless of which controller triggered it;still call
setLoading(false)so the hook settles cleanly.useOperatorLifecycle.ts(fetchLifecycleData): When returning a cachedin-flight promise, chain
.catch()that detects if the original was abortedby another caller while our signal is still live, deletes the stale cache
entry, and issues a fresh retry rather than leaving the component stalled.
Screenshots / screen recording:
N/A — no visual change; eliminates console noise and fixes error-state regression on the Installed Operators page when lifecycle metadata is enabled.
Test setup:
Tested locally against a TechPreview cluster (
TechPreviewNoUpgradefeature set) with--olm-lifecycle-metadata=true.Test cases:
coFetchdoes notconsole.warnwhen fetch is aborted via AbortController (new)coFetchdoesconsole.warnfor real network errors (new)useOperatorLifecycledoes not surfaceAbortErroras error state (new)useOperatorLifecycleretries when cached in-flight promise is aborted by another caller (new)useOperatorLifecyclesurfaces real network errors correctly (new)useOperatorLifecycleskips fetch when parameters are missing (new)Browser conformance: N/A
🤖 Generated with Claude Code
Summary by CodeRabbit