Repository navigation
Conversation
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
Rohit0301
added
the
safe to test
❌ PR checklist incompleteThis PR cannot be merged until the following are addressed on its linked issue:
The fields live on the linked issue in the Shipping project (open the issue → right sidebar → Projects). After you set them, re-run this check (or push a commit) — issue/project changes do not re-trigger it automatically. Maintainers can bypass this check by adding the |
2bed353 to
4a34fec
Compare
github-actions
Bot
removed
the
safe to test
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
Rohit0301
added
the
safe to test
4a34fec to
8fccd0c
Compare
github-actions
Bot
removed
the
safe to test
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
|
I checked this against the failing merge-queue run on Collate (run 38528815785), and the traces match your explanation exactly. In both attempts of
So the one refetch happened before the background delete finished, and nothing ever asked the server again. The fix targets the right thing. 👍 A few thoughts, none of them blocking:
|
Rohit0301
added
the
safe to test
Jest test CoverageUI tests summary
|
|
Thanks for checking it against the run — good to have the traces confirm it. On (1): I traced the socket path and it holds up. One real caveat before we swap: the socket event only arrives if the socket is connected. If it drops mid-delete, the badge stays stale with no poll to catch it — strictly worse than today's bounded poll on that one path. So the event-driven version probably wants a single delayed refetch as a backstop, which is still a fraction of the current code. Happy to do it as the follow-up the |
A drive file's hard delete answers 202 Accepted and removes the row on a background thread. ContextCenterArchivePage invalidated the archive-count query the moment that request resolved, so the refetch raced the background deletion. When the refetch won, React Query cached the pre-delete total and nothing invalidated the key again, leaving the sidebar badge one too high for the rest of the session. Replace the single fire-and-forget invalidation with reconcileCountAfterAsyncDelete: drop the count optimistically (matching what the page already does to its own list), then refetch until the server agrees or the attempts run out (5 x 1s). Uses refetchQueries rather than invalidateQueries — invalidation only marks an *inactive* query stale without fetching it, so the reconcile would silently no-op whenever the sidebar observing the key is unmounted. On timeout the corrected figure is written back: the last refetch leaves the server's pre-delete total in the cache, and keeping that would restore the staleness this fixes. Deletions still in flight are counted per key so a concurrent delete derives its target from the server's number rather than a cached value a sibling's refetch may have just overwritten. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
8fccd0c to
bcc904f
Compare
github-actions
Bot
removed
the
safe to test
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
Rohit0301
added
the
safe to test
… as landed
The absence probe called getContextFileById without an include param, but
GET /contextCenter/drive/files/{id} defaults to include=non-deleted. Every row
on the Archive page is already soft-deleted (the list is loaded with
include=deleted), so the probe 404'd before the background hard delete even
started. The rejection handler mapped any error to "landed", so the first poll
ended the wait: the race this fixes was still there, only pushed back ~1s.
Let getContextFileById take an include argument and pass Include.All, so the
probe resolves true only once the row is genuinely purged. Narrow the predicate
to a 404 as well, leaving a transient error to the next attempt rather than
ending the poll early.
The helper's unit tests stub hasLanded, so neither gap was visible to them:
cover the include wiring in a new assetAPI test and the transient-error case
in the helper's own suite.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
github-actions
Bot
removed
the
safe to test
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
Rohit0301
added
the
safe to test
github-actions
Bot
removed
the
safe to test
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
Rohit0301
added
the
safe to test
github-actions
Bot
removed
the
safe to test
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
Rohit0301
added
the
safe to test
Sonar flagged the unawaited reconcileCountAfterAsyncDelete call. The float is deliberate — awaiting it would hold `isDeleting` through the ~5s poll and spin the modal button — so `void` says so explicitly. Nothing escapes: the probe handles both branches and refetchQueries resolves rather than throws. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
github-actions
Bot
removed
the
safe to test
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
Code Review ✅ Approved 7 closed / 7 findings🟡 Medium risk · Adding bounded polling before refreshing the Archive badge changes UI cache behavior; slow deletes or failed probes could leave its count temporarily stale. Fixes the Context Center Archive badge going stale after an async hard delete by reconciling the count optimistically and polling until the server agrees. Resolved this session: the poll window and timeout behavior, retry budget recording, back-to-back delete total computation, in-flight deletion accounting, baseline target handling across restore and soft-delete, and the absence probe now including deleted rows. No open issues found. ✅ 7 closed✅ Bug: Poll window is 2s, not 5×1s, and a timeout puts the stale count back
✅ Edge Case: Back-to-back hard deletes compute expectedTotal from a stale refetch
✅ Quality: Unrecorded retry budget for async-delete badge reconciliation
✅ Bug: peakInFlight over-subtracts deletions the server has already applied
✅ Bug: Finished deletes leave
|
| Compact |
|
Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source
Rohit0301
added
the
safe to test
|
🚦 Removed from the merge queue —
|
34594ee
) * fix(ui): converge Archive badge count after an async hard delete A drive file's hard delete answers 202 Accepted and removes the row on a background thread. ContextCenterArchivePage invalidated the archive-count query the moment that request resolved, so the refetch raced the background deletion. When the refetch won, React Query cached the pre-delete total and nothing invalidated the key again, leaving the sidebar badge one too high for the rest of the session. Replace the single fire-and-forget invalidation with reconcileCountAfterAsyncDelete: drop the count optimistically (matching what the page already does to its own list), then refetch until the server agrees or the attempts run out (5 x 1s). Uses refetchQueries rather than invalidateQueries — invalidation only marks an *inactive* query stale without fetching it, so the reconcile would silently no-op whenever the sidebar observing the key is unmounted. On timeout the corrected figure is written back: the last refetch leaves the server's pre-delete total in the cache, and keeping that would restore the staleness this fixes. Deletions still in flight are counted per key so a concurrent delete derives its target from the server's number rather than a cached value a sibling's refetch may have just overwritten. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(ui): poll the deleted row's absence to converge the Archive badge The reconcile targeted `baseline - started`, a count frozen when the first delete started. Anything else moving the archive count inside the ~5s window threw it off, and the page's own restore handler invalidates that very key: a restore mid-window let the poll see its target early and exit, leaving the badge one too high once the background delete landed — the bug this helper exists to fix. A soft-delete in the window broke it the other way, with `clampedTotal` capping the badge one too low. Use the deleted entity's own absence as the completion signal instead: poll `getContextFileById` until it stops resolving, then refetch once and keep whatever the server reports. No precomputed target, so a concurrent restore or soft-delete cannot skew it, and the baseline/in-flight bookkeeping and the clamp all go away with it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(ui): probe the archive delete with Include.All and only treat 404 as landed The absence probe called getContextFileById without an include param, but GET /contextCenter/drive/files/{id} defaults to include=non-deleted. Every row on the Archive page is already soft-deleted (the list is loaded with include=deleted), so the probe 404'd before the background hard delete even started. The rejection handler mapped any error to "landed", so the first poll ended the wait: the race this fixes was still there, only pushed back ~1s. Let getContextFileById take an include argument and pass Include.All, so the probe resolves true only once the row is genuinely purged. Narrow the predicate to a 404 as well, leaving a transient error to the next attempt rather than ending the poll early. The helper's unit tests stub hasLanded, so neither gap was visible to them: cover the include wiring in a new assetAPI test and the transient-error case in the helper's own suite. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * lint fix * fix(ui): mark the archive reconcile float with void Sonar flagged the unawaited reconcileCountAfterAsyncDelete call. The float is deliberate — awaiting it would hold `isDeleting` through the ~5s poll and spin the modal button — so `void` says so explicitly. Nothing escapes: the probe handles both branches and refetchQueries resolves rather than throws. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
3 participants
Describe your changes:
I fixed the Context Center Archive sidebar badge going stale after a document is hard-deleted from the Archive page.
A drive file's hard delete answers 202 Accepted and removes the row on a background thread (
ContextFileResource.delete→deleteByIdAsync).ContextCenterArchivePageinvalidated the archive-count query the moment that request resolved, so the refetch raced the background deletion:staleTime: 0, but no further trigger exists). The badge stays one too high for the rest of the session, until a page reload.The page's own list was never affected, because it optimistically splices the row out and decrements its local total — only the server-backed sidebar badge went stale.
Replaced the single fire-and-forget invalidation with
reconcileCountAfterAsyncDelete: drop the count optimistically (matching what the page already does to its list), then refetch until the server agrees or the attempts run out (5 × 1s).Two details worth a reviewer's eye:
refetchQueries, notinvalidateQueries. Invalidation only marks an inactive query stale without fetching it, so the reconcile would silently no-op whenever the sidebar observing that key is unmounted — exactly the staleness this is meant to prevent.isDeletingthrough the poll would spin the modal's delete button for seconds.Type of change:
High-level design:
N/A — small change.
Tests:
Use cases covered
Unit tests
openmetadata-ui/src/main/resources/ui/src/utils/ContextCenterQueryKeys.test.tsSeven cases on
reconcileCountAfterAsyncDelete: optimistic drop applied synchronously; keeps polling while the server still reports the pre-delete total; stops at the first refetch once the server agrees; terminates after the configured attempts when the deletion never lands; keeps the corrected figure when the budget runs out; does not let a concurrent delete chain off a polluted cache value; no-ops when nothing is cached.(7 new + the 2 existing
ContextCenterArchivePagetests.)Backend integration tests
Ingestion integration tests
Playwright (UI) tests
Run locally with
--retries=0, so a retry-pass could not mask the flakiness being fixed.playwright/e2e/Features/ContextCenterArchive.spec.ts— 3 workers,--retries=068:7exercises the permanent-delete path this PR changes.Downstream check — Collate
AskCollateContextCenter.spec.ts, 20 repeatsThe Collate spec
Count updates › Archive: hard-deleting an archived document updates Archive countis the test that surfaced this bug; it was intermittently failing with the badge stuck at its pre-delete value.npx playwright test --project=askcollate -g "Archive: hard-deleting an archived document updates Archive count" --repeat-each=20 --retries=0 --workers=1--retries=0, so a retry-pass cannot mask the flakiness being fixed. Run at 1 worker because this box carries a VM at ~59% CPU (load avg ~42); at 3 workers Chromium intermittently fails to start (browserType.launch: Timeout 180000ms exceeded), producing failures unrelated to the code under test.Before the fix, that spec failed with:
— the badge never moving, with the assertion polling a value that could no longer change.
Manual testing performed
UI screen recording:
N/A — cache-reconciliation fix with no visual change; the badge simply reaches the correct value without a reload.
🤖 Generated with Claude Code