SRVKP-12032: Issued fix for removing cache and re-triggering tr api on every delete operation

[anwesha-palit-redhat]

AI generated PR description

#1146 (comment)

Screen recordings for PLR

PLRdeletionworking.mov

Screen recordings for TR

TRdeletionworking.mov

[openshift-ci-robot]

@anwesha-palit-redhat: This pull request references SRVKP-12032 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "5.0.0" version, but no target version was set.

Details

In 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]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: anwesha-palit-redhat

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [anwesha-palit-redhat]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[anwesha-palit-redhat]

/agentic-review

[qodo-code-review]

PR Reviewer Guide 🔍

Warning

/review is deprecated. Use /agentic_review instead (removal date not yet scheduled).

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 SRVKP-12032 - Partially compliant Compliant requirements: Fix the TaskRuns page so that when a TaskRun is deleted, it disappears from the list without requiring page switching or other workarounds. Ensure the TaskRuns list reflects deletions correctly even when data is sourced/merged from Tekton Results and/or cached data. Non-compliant requirements: Requires further human verification: Validate in the UI that deleting a TaskRun removes it immediately from the TaskRuns list across pagination, sorting, and with Tekton Results enabled/disabled. Validate behavior in hub/multi-cluster scenarios (Kueue-managed) where the data source may differ.

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Refresh logic The refresh trigger uses a concatenated string key and increments based on detecting list length shrink (etcdRuns.length < etcdRunsRef.current.length). This may miss deletions if the watch delivers transient states, if the list is filtered/limited, or if item count stays the same while a deleted item is replaced. Also consider that setTrRefreshKey((k) => k + 1 + Date.now()) mixes string concatenation with numbers, which is easy to misread and may lead to unexpectedly large keys. const etcdRunsRef = React.useRef<Kind[]>([]); const [trRefreshKey, setTrRefreshKey] = React.useState<string>(''); const optionsMemo = useDeepCompareMemoize(options); const isTektonResultEnabled = useFlag(FLAG_PIPELINE_TEKTON_RESULT_INSTALLED); const isList = !optionsMemo?.name; const limit = optionsMemo?.limit; const isPipelineRun = groupVersionKind?.kind === getGroupVersionKindForModel(PipelineRunModel)?.kind; // Hub cluster detection const { isResourceManagedByKueue } = useMultiClusterProxyService({ managedBy: pipelineRunManagedBy, }); const isTaskRunQuery = groupVersionKind?.kind === getGroupVersionKindForModel(TaskRunModel)?.kind; // Extract pipelineRunName from selector for multi-cluster API const pipelineRunName = optionsMemo?.selector?.matchLabels?.[TektonResourceLabel.pipelinerun]; // Use multi-cluster hook for hub clusters fetching TaskRuns const shouldUseMultiCluster = isResourceManagedByKueue && isTaskRunQuery && !!pipelineRunName; const [ mcTaskRuns, mcLoaded, mcError, mcPendingAdmission, mcProxyUnavailable, ] = useMultiClusterTaskRuns<Kind>( shouldUseMultiCluster ? namespace : null, shouldUseMultiCluster ? pipelineRunName : null, isResourceManagedByKueue, pipelineRunFinished, ); // do not include the limit when querying etcd because result order is not sorted const watchOptions = React.useMemo(() => { // reset cached runs as the options have changed return shouldUseMultiCluster ? null : { groupVersionKind, namespace: namespace && namespace !== '-' ? namespace : undefined, isList, selector: optionsMemo?.selector, name: optionsMemo?.name, }; }, [groupVersionKind, namespace, optionsMemo, isList, shouldUseMultiCluster]); const [resources, loaded, error] = useK8sWatchResource(watchOptions); // Use multi-cluster results for hub TaskRuns, otherwise use k8s results const etcdRuns = React.useMemo(() => { if (shouldUseMultiCluster) { if (!mcLoaded || mcError) return []; return mcTaskRuns; } if (!loaded || error) { return []; } const resourcesArray = (isList ? resources : [resources]) as Kind[]; return resourcesArray; }, [ shouldUseMultiCluster, mcLoaded, mcError, mcTaskRuns, isList, resources, loaded, error, ]); const effectiveLoaded = shouldUseMultiCluster ? mcLoaded : loaded; const effectiveError = shouldUseMultiCluster ? mcError : error; const runs = React.useMemo(() => { if (!etcdRuns || etcdRuns.length === 0) { return etcdRuns; } let value = [...etcdRuns]; value.sort((a, b) => b.metadata.creationTimestamp.localeCompare(a.metadata.creationTimestamp), ); if (limit && limit < value?.length) { value = value.slice(0, limit); } return value; }, [etcdRuns, limit]); React.useEffect(() => { if (etcdRuns.length < etcdRunsRef.current.length) { setTrRefreshKey((k) => k + 1 + Date.now()); } etcdRunsRef.current = etcdRuns; }, [etcdRuns]); Loaded state The Tekton Results hook no longer models/propagates an in-flight state (previously the 3rd tuple element was used to suppress loaded updates). Now it sets loaded to true on every successful fetch and updates the list. Re-check that consumers don’t rely on the old "call inflight" behavior and that pagination/loading indicators still behave correctly. const useTRRuns = <Kind extends K8sResourceCommon>( getRuns: ( namespace: string, options?: TektonResultsOptions, nextPageToken?: string, isDevConsoleProxyAvailable?: boolean, ) => Promise<[Kind[], RecordsList]>, namespace: string, options?: TektonResultsOptions, ): [Kind[], boolean, unknown, GetNextPage] => { const isDevConsoleProxyAvailable = useFlag(FLAGS.DEVCONSOLE_PROXY); const [nextPageToken, setNextPageToken] = React.useState<string>(null); const [result, setResult] = React.useState< [Kind[], boolean, unknown, GetNextPage] >([[], false, undefined, undefined]); // reset token if namespace or options change React.useEffect(() => { setNextPageToken(null); }, [namespace, options]); // eslint-disable-next-line consistent-return React.useEffect(() => { let disposed = false; (async () => { try { const tkPipelineRuns = await getRuns( namespace, options, nextPageToken, isDevConsoleProxyAvailable, ); if (!disposed) { const token = tkPipelineRuns[1].nextPageToken; setResult((cur) => [ nextPageToken ? [...cur[0], ...tkPipelineRuns[0]] : tkPipelineRuns[0], true, undefined, token ? (() => { // ensure we can only call this once let executed = false; return () => { if (!disposed && !executed) { executed = true; // trigger the update setNextPageToken(token); return true; } return false; }; })() : undefined, ]); } } catch (e) { if (!disposed) { if (nextPageToken) { setResult((cur) => [cur[0], cur[1], e, undefined]); } else { setResult([[], false, e, undefined]); } } } })(); return () => { disposed = true; }; }, [namespace, options, nextPageToken, getRuns]); return result; }; export const useTRPipelineRuns = ( namespace: string, options?: TektonResultsOptions, ): [PipelineRunKind[], boolean, unknown, GetNextPage] => useTRRuns<PipelineRunKind>(getPipelineRuns, namespace, options); export const useTRTaskRuns = ( namespace: string, options?: TektonResultsOptions, ): [TaskRunKind[], boolean, unknown, GetNextPage] => useTRRuns<TaskRunKind>(getTaskRuns, namespace, options); Access review usage canDeleteTaskRun is converted into a single-element array and later accessed as canDeleteTaskRun[0]. This is an unusual pattern and risks confusion/bugs (e.g., if useAccessReview already returns a boolean, keep it boolean). Also, the deletion check uses obj?.metadata?.annotations?.deletionTimestamp, but deletion timestamp typically resides on metadata.deletionTimestamp, so confirm the intended field to disable deletion correctly. const canDeleteTaskRun = useAccessReview({ group: TaskRunModel.apiGroup, resource: TaskRunModel.plural, verb: 'delete', name, namespace, }) && !obj?.metadata?.annotations?.deletionTimestamp ? [true] : [false]; const onToggle = () => { setIsOpen(!isOpen); }; const onSelect = () => { setIsOpen(false); }; const dropdownItems = [ obj?.metadata?.annotations?.[DELETED_RESOURCE_IN_K8S_ANNOTATION] === 'true' ? ( <DropdownItem key={KEBAB_ACTION_DELETE_ID} component="button" onClick={launchDeleteModal} isDisabled={ !canDeleteTaskRun[0] || obj?.metadata?.annotations?.[DELETED_RESOURCE_IN_K8S_ANNOTATION] === 'true' } data-test-action={KEBAB_ACTION_DELETE_ID} tooltipProps={{ content: t('Resource is being fetched from Tekton Results.'), isVisible: true, }} > {t('Delete {{resourceKind}}', { resourceKind: TaskRunModel.kind })} </DropdownItem> ) : ( <DropdownItem key={KEBAB_ACTION_DELETE_ID} component="button" onClick={launchDeleteModal} data-test-action={KEBAB_ACTION_DELETE_ID} > {t('Delete {{resourceKind}}', { resourceKind: TaskRunModel.kind })} </DropdownItem> ), ]; return ( <Dropdown onSelect={onSelect} onOpenChange={(isOpen: boolean) => setIsOpen(isOpen)} toggle={(toggleRef: React.Ref<MenuToggleElement>) => ( <MenuToggle isDisabled={!canDeleteTaskRun[0]} ref={toggleRef} aria-label="kebab menu" variant="plain" onClick={onToggle}

[anwesha-palit-redhat]

/describe

[qodo-code-review]

PR Summary by Qodo

Fix Tekton Results re-fetch on TaskRun/PipelineRun deletion by removing caches
🐞 Bug fix 🕐 40+ Minutes

Description

• Remove TaskRun/PipelineRun list caching to avoid stale data after deletions.
• Add a refresh key to re-trigger Tekton Results queries when watched runs disappear.
• Simplify list row components to rely on hooks instead of ad-hoc in-memory caches.

Diagram

flowchart TD
  A["Pipeline rows"] --> B["TaskRuns hook"] --> C["Runs hook"] --> D["K8s watch"]
  C --> E["Refresh key"] --> F["TR runs hook"] --> G["Results fetch"] --> H["Results API"]
  D --> E

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Targeted cache invalidation on delete events

• ➕ Retains performance benefits of caching while fixing the stale-delete issue
• ➕ More deterministic than length-based heuristics (e.g., invalidating by UID/name)
• ➖ Requires plumbing delete lifecycle events (or watch events) into cache invalidation
• ➖ More complex than removing caches; higher implementation overhead

2. Use a query library (e.g., react-query) with invalidation

• ➕ Standardized caching, in-flight dedupe, retries, and invalidation semantics
• ➕ Easier to reason about refresh behavior across components
• ➖ Likely too heavy for this change; introduces dependency and refactor scope
• ➖ Migration cost across existing hooks/components

3. Rely solely on watch data (avoid TR merge for lists)

• ➕ Eliminates dual-source consistency issues
• ➕ Simplifies refresh logic and reduces API calls
• ➖ Not always feasible if Tekton Results is the required source (scale/retention)
• ➖ May regress environments where etcd does not contain desired history

Recommendation: The PR’s direction (removing caches and adding a refresh key) is a pragmatic correctness-first fix that addresses the stale list behavior after deletions. If performance becomes a concern, consider reintroducing caching later with explicit invalidation keyed off delete operations or watch events (rather than implicit heuristics).

Files changed (7) +157 / -318

Bug fix (3) +68 / -80

useTaskRuns.tsRemove cacheKey plumbing; add refresh-triggered Tekton Results queries +24/-30

Remove cacheKey plumbing; add refresh-triggered Tekton Results queries

• Removes the cacheKey option and associated call wiring. Drops removed-run reconciliation logic and introduces a refresh key that increments when the watched etcd list shrinks, forcing Tekton Results hooks to refetch after deletions.

src/components/hooks/useTaskRuns.ts

useTektonResults.tsReplace cacheKey with refreshKey and refetch dependency +30/-42

Replace cacheKey with refreshKey and refetch dependency

• Replaces the cacheKey parameter with a refreshKey and adds it to the fetch effect dependency list. Ensures callers can explicitly force a re-query (used when etcd watch indicates deletions).

src/components/hooks/useTektonResults.ts

TaskRunsRow.tsxDisable delete kebab when deletion is in progress +14/-8

Disable delete kebab when deletion is in progress

• Wraps access review with an additional guard to disable the kebab menu when the object indicates a deletion is already underway. Also reformats the multi-cluster proxy hook call for clarity.

src/components/pipelines-tasks/TaskRunsRow.tsx

Refactor (4) +89 / -238

useTektonResult.tsSimplify TR hook signature by removing cacheKey behavior +27/-42

Simplify TR hook signature by removing cacheKey behavior

• Removes local cache key state and cacheKey dependencies from the hook, and simplifies result setting to always update with the latest fetched page. Updates exported hooks to stop accepting/forwarding cacheKey.

src/components/hooks/useTektonResult.ts

PipelineRunsRow.tsxRemove per-row TaskRun cache and always use useTaskRuns hook +3/-47

Remove per-row TaskRun cache and always use useTaskRuns hook

• Deletes module-level caches/in-flight stores for TaskRuns and collapses the two-step fetch/cached component structure into a single component. Stops passing cacheKey into useTaskRuns.

src/components/pipelineRuns-list/PipelineRunsRow.tsx

PipelineRow.tsxRemove per-row TaskRun cache and rely on hook results +3/-46

Remove per-row TaskRun cache and rely on hook results

• Removes ad-hoc TaskRun caching and simplifies the row rendering path to always use useTaskRuns directly. Stops passing cacheKey into the hook and cleans up imports.

src/components/pipelines-list/PipelineRow.tsx

tekton-results.tsRemove Tekton Results response caching and in-flight deduping +56/-103

Remove Tekton Results response caching and in-flight deduping

• Deletes the global cache and in-flight store and removes cacheKey parameters from Tekton Results fetch helpers. Refactors getFilteredRecord to always fetch and return current results, while preserving 404-as-empty behavior.

src/components/utils/tekton-results.ts