Skip to content

fix(governance): resolve inherited fields freshly instead of from a never-cleared cache - #31133

Open
harshach wants to merge 15 commits into
mainfrom
harshach/inherited-reviewers-workflow-it
Open

fix(governance): resolve inherited fields freshly instead of from a never-cleared cache#31133
harshach wants to merge 15 commits into
mainfrom
harshach/inherited-reviewers-workflow-it

Conversation

@harshach

@harshach harshach commented Aug 6, 2026

Copy link
Copy Markdown
Collaborator

Describe your changes:

Fixes #

A glossary term whose reviewers are inherited from its glossary was judged to have none: the
approval workflow's reviewers gate evaluated false, no approval task was created, and the term
settled in Draft. A term with reviewers attached directly worked. Restarting the server
fixed it
— the signature of a stale in-memory cache, and the observation that identified the real
cause.

Root cause

EntityRepository.inheritanceParentCache was a static ThreadLocal<Map<...>> with no TTL, no size
bound and no eviction
, cleared in exactly one place: ImpersonationCleanupFilter, a JAX-RS
response filter — so only on HTTP request threads.

The approval workflow does not run on one. A change event reaches WorkflowEventConsumer on a
Quartz thread and signals the process via runtimeService.signalEventReceived, which is Flowable's
synchronous API; no node on the glossary path is marked async. So the trigger filter, the attribute
gates and the status transition all execute inline on that thread. It never cleared the cache, so a
glossary snapshot read before its reviewer was added was served for the life of the pod.

Two further properties made it worse: it is static (shared by every repository on that thread) and
it did superset field matching, so one stale entry answered several different field queries. And
because it is a ThreadLocal, the thread performing the write cannot reach it — write-based
invalidation was not implementable, which is why none existed.

This affected every inheritable fieldowners and domains as well as reviewers.

Fix

Delete the cache rather than add invalidation to it. It was a redundant third tier duplicating
layers that are already correct: parent lookups now fall through to the Guava L1 → Redis L2 → DB, and
both caches are invalidated inline on write and across nodes via CacheInvalidationPubSub. Redis is
opt-in (cache.provider defaults to none), so when it is off the read simply goes to the database.

  • The batch path still issues one bulk load per parent type, so list endpoints do not regress to
    an N+1; the added cost lands on repeated single-entity reads inside one request.
  • Superset matching disappears. Only TableRepository/DatabaseSchemaRepository built variable field
    strings, so only they used it; narrower is a strict subset, so correctness is unchanged.

Clean the ThreadLocals on the threads that never did. PerRequestContextCleaner centralises the
per-request set (previously inline in the JAX-RS filter) and is invoked from the Quartz consumer tick
and, via WorkflowThreadCleanupListener, at Flowable job completion.

Workflow reads open a FreshReadScope (fromCache=false). Their reads are gating decisions where
a stale answer silently routes an entity to the wrong terminal status, and cross-node L1 invalidation
requires Redis; workflow volume is low, so the fresh read is the cheaper side of that trade. A
thread-scoped marker was chosen over threading a fromCache boolean because getParentEntity is
overridden in ~21 repositories.

An earlier revision of this PR added a reviewers-only getEffectiveReviewers helper. It is
reverted here
: it covered only reviewers (not owners/domains) and replaced the reviewer list
rather than merging own + inherited entries the way inheritance does. The root fix makes it
redundant.

Type of change:

  • Bug fix

High-level design:

Net effect is a deletion: EntityRepository loses ~170 lines. New pieces are three small classes —
PerRequestContextCleaner (shared cleanup), WorkflowThreadCleanupListener (Flowable job boundary),
FreshReadScope (thread-scoped fresh-read marker, modelled on the existing EntityCacheBypass).

No schema change, no migration, and no workflow-definition change — customized workflow definitions
benefit unchanged, because the fix corrects the data the rules see rather than the rules.

Tests:

Use cases covered

  • A term inheriting its reviewer from the glossary reaches In Review with an open approval task.
  • Inheritance reflects a parent mutated after an earlier read on the same thread (the reported
    sequence: glossary gains a reviewer after a term already ran the workflow).
  • Reviewer attached directly still works (control), and a term with no reviewers anywhere still
    auto-approves.
  • Per-request ThreadLocals are cleared at Flowable job completion and not on unrelated engine events.

Unit tests

  • EntityRepositoryInheritanceParentTestfails without the fix. Verified by temporarily
    reintroducing the memoization: expected: <1> but was: <0> inherited domains, i.e. the production
    symptom. The fake repository returns a fresh snapshot per parent read, as a real read does —
    returning the same mutable instance would let a memoized reference appear up to date and hide the
    bug.
  • WorkflowThreadCleanupListenerTest — clears on JOB_EXECUTION_SUCCESS/FAILURE, leaves
    unrelated events alone, never fails the job.
  • CheckEntityAttributesImplTest — pins how the shipped reviewers rule reads the entity.

Backend integration tests

  • GlossaryTermInheritedReviewerApprovalIT (7 tests): direct, glossary-inherited,
    parent-term-inherited, grandparent-inherited, inherited-with-owner-set, remove-direct-reviewer
    fallback, and reviewer-added-after-first-term.

Verified: 852 unit tests and ~1,100 integration tests green — TableResourceIT (304),
GlossaryResourceIT (99), WorkflowDefinitionResourceIT (54), GlossaryTermResourceIT (29),
DatabaseSchemaResourceIT, EntityCacheInvalidationIT, GlossaryTermMoveApprovalIT,
ImpersonationCleanupFilterTest.

Still outstanding for a reviewer to weigh: inheritance is cross-cutting, so a full
openmetadata-integration-tests run is warranted before merge; I ran a targeted subset.

Ingestion integration tests

  • Not applicable (no ingestion changes).

Playwright (UI) tests

  • GlossaryInheritedReviewerApproval.spec.ts — drives the reported ordering (glossary with no
    reviewers → term → add reviewer → new terms) against a deployed stack, which is where the
    long-lived threads that caused this actually exist. Not yet executed against a live stack.

Manual testing performed

Root cause was confirmed on the reporting instance: the workflow recovered after a server restart and
regressed again once a glossary was edited, which is what an in-memory, never-invalidated cache
predicts and what led to deleting it.

UI screen recording / screenshots:

Not applicable — no UI changes.

Checklist:

  • I have read the CONTRIBUTING document.
  • My PR title is Fixes <issue-number>: <short explanation>
  • My PR is linked to a GitHub issue via Fixes #<issue-number> above.
  • I have commented on my code, particularly in hard-to-understand areas.
  • For JSON Schema changes: not applicable — no schema changes.
  • For UI changes: not applicable.
  • I have added tests and listed them above.
  • I have added a test that covers the exact scenario we are fixing.

🤖 Generated with Claude Code

The gate that decides whether an approval task is created
(CheckGlossaryTermHasReviewers -> CheckEntityAttributesImpl) evaluated
`some reviewers` over whatever the entity read returned, so it was correct only
when that read had both requested and applied field inheritance. A glossary term
that inherits its reviewers from its parent glossary could therefore be judged to
have none, skip the approval task, and settle in Draft.

Two sibling paths already resolve the chain explicitly -- setDefaultStatus and
SetApprovalAssigneesImpl -- so "what status" and "who gets the task" were robust
while "should there be a task at all" was not. TagRepository.setInheritedFields
compounds the exposure by swallowing exceptions and silently skipping
inheritance, leaving the same gate with an empty reviewers list.

Add an EntityRepository.getEffectiveReviewers hook, overridden by
GlossaryTermRepository (term -> parent term -> glossary) and TagRepository
(tag -> classification), and have the gate resolve effective reviewers before
evaluating the rule. The resolved reviewers are injected into the rule input map
rather than onto the entity, which may be request-cached and must not be mutated.

This covers both gates using the reviewers rule: GlossaryApprovalWorkflow and
AIAssetApprovalWorkflow, and needs no workflow migration -- it fixes the data the
rule sees, so already-customized workflow definitions benefit unchanged.

Adds GlossaryTermInheritedReviewerApprovalIT covering direct, glossary-inherited,
parent-term-inherited, inherited-with-owner-set, and remove-direct-reviewer
fallback scenarios.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings August 6, 2026 18:21
@harshach
harshach requested a review from a team as a code owner August 6, 2026 18:21
harshach and others added 2 commits August 6, 2026 11:34
…r entity type

Replace the GlossaryTermRepository and TagRepository overrides with a single
generic implementation on EntityRepository that walks getParentEntity -- the same
hook the inheritance path already uses -- so any entity declaring an inheritance
parent participates without type-specific code. GlossaryTermRepository already
implements that hook as "parent term, else glossary", which is exactly the walk
the removed override duplicated.

Reading the parent applies the parent's own inheritance, so a glossary's reviewers
still surface for a term nested under an intermediate term that has none. Covered
by a new grandparent test.

Resolution now goes through resolveInheritanceParentLeniently, which swallows
EntityNotFoundException. This fixes a regression the previous commit introduced:
the per-type lookups used Entity.getEntity(..., NON_DELETED), which throws when
the parent is missing or soft-deleted, turning a previously silent skip into a
BpmnError that aborted the approval gate and left the term with no task at all.

TagRepository is reverted to its original state. Covering tags generically would
require overriding getParentEntity there, which also feeds create-permission
evaluation in CreateResourceContext -- a behaviour change out of scope here. Tags
keep their existing read-path inheritance; their setInheritedFields still swallows
exceptions and silently skips inheritance, which is worth addressing separately.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…to buildRuleContext

Add CheckEntityAttributesImplTest, which drives the delegate against the rule
GlossaryApprovalWorkflow.json actually ships and asserts the node result. Unlike
the integration tests, the unit level can force the failing condition directly --
an entity read that returns no reviewers on the term while the parent supplies
them -- so reviewersRule_inheritedReviewersOnly_evaluatesTrue is a genuine
fail-without-fix test: reverting the gate to JsonUtils.getMap(entity) fails it
with "expected: <true> but was: <false>", the exact false that routes a term to
Draft. Also covers direct reviewers, no reviewers anywhere, an inherited
reference without an FQN, and an entity type that does not support reviewers.

Rename buildRuleData to buildRuleContext: RuleEngine.apply names its parameter
`context`, so the helper now matches the vocabulary of the API it feeds. The Map
return type is required -- RuleEngine.apply takes a Map<String, Object>, and every
other call site passes JsonUtils.getMap(entity). Applying the reviewers override
to that map rather than to the entity is deliberate and now documented: getMap
already returns a detached copy, while the entity may be request-cached.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@github-actions github-actions Bot added backend safe to test Add this label to run secure Github workflows on PRs labels Aug 6, 2026
@github-actions

github-actions Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

❌ PR checklist incomplete

This PR cannot be merged until the following are addressed on its linked issue:

  • No GitHub issue is linked. Link an issue in the Development section of the PR (or add Fixes #12345 to the description). For a same-org cross-repo issue, add Fixes open-metadata/<repo>#123 to the description.

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 skip-pr-checks label.

…arent cache

Adds a Playwright spec and an integration test that drive the ordering the bug
needs: the glossary is created with NO reviewers, a first term runs the approval
workflow (populating EntityRepository.inheritanceParentCache on a workflow
executor thread), and only then is the reviewer added. Every term created
afterwards must still be gated as "has reviewers".

That cache is a static ThreadLocal cleared only by ImpersonationCleanupFilter, a
JAX-RS response filter, so workflow threads never clear it and can serve a
reviewer-less glossary snapshot indefinitely. A server restart clears it, which
matches the reported recovery.

Both tests currently PASS in-process: the embedded integration environment does
not keep a poisoned executor thread alive across the ordering, so the sequence
alone does not reproduce there. They stand as regression guards, and the
Playwright spec is the one that exercises a real deployed stack where the
executor threads are long lived.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings August 6, 2026 19:45
@harshach
harshach requested a review from a team as a code owner August 6, 2026 19:45

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Suppressed comments (4)

openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/EntityRepository.java:7884

  • getEffectiveReviewers relies on getParentEntity(...) to find an inheritance parent. Repositories like TagRepository do not override getParentEntity/getParentReference (default returns null), so tags/classifications will never resolve inherited reviewers via this path and the reviewers gate will still see an empty list.
    List<EntityReference> reviewers = listOrEmpty(entity.getReviewers());
    if (reviewers.isEmpty() && supportsReviewers) {
      EntityInterface parent = resolveInheritanceParentLeniently(entity, FIELD_REVIEWERS);
      if (parent != null) {
        reviewers = listOrEmpty(parent.getReviewers());

openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/Glossary/GlossaryInheritedReviewerApproval.spec.ts:195

  • This assertion can race task creation: the term may reach In Review before the approval task is persisted/indexed, which can make the test flaky under load. Consider polling until at least one open approval task is visible (similar to the status poll).
          expect(
            await getOpenApprovalTaskCount(apiContext, term.fullyQualifiedName),
            `Term ${term.name} must have an open approval task for the inherited reviewer`
          ).toBeGreaterThan(0);

openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/EntityRepository.java:7879

  • getEffectiveReviewers is described/used as an extensibility hook (e.g., GlossaryTerm/Tag reviewer chains), but it is declared final, which prevents repository-specific overrides and makes the API inconsistent with that intent.

This issue also appears on line 7880 of the same file.

  public final List<EntityReference> getEffectiveReviewers(T entity) {

openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/Glossary/GlossaryInheritedReviewerApproval.spec.ts:113

  • PR description/checklist says Playwright tests are "Not applicable", but this PR adds a new Playwright E2E spec. Please update the PR description’s Tests section/checklist so reviewers/CI expectations match the actual changes.

This issue also appears on line 192 of the same file.

test.describe(
  'Glossary Approval - inherited reviewers',
  { tag: ['@Features', '@Governance'] },
  () => {

…ever-cleared cache

Glossary terms whose reviewers were inherited from their glossary were judged to
have none: the approval workflow's reviewers gate evaluated false, no approval
task was created, and the term settled in Draft. Restarting the server fixed it.

EntityRepository.inheritanceParentCache was a static ThreadLocal with no TTL, no
size bound and no eviction, cleared only by ImpersonationCleanupFilter -- a
JAX-RS response filter, so only on HTTP request threads. The approval workflow
does not run on one: a change event reaches WorkflowEventConsumer on a Quartz
thread and signals the process synchronously, so the filter, the attribute gates
and the status transition all execute there. That thread never cleared the cache,
so a glossary read before its reviewer was added was served for the life of the
pod. Being a ThreadLocal it also cannot be evicted from the thread that performs
the write, which is why no invalidation existed. It affected every inheritable
field -- owners and domains as well as reviewers.

Delete it. Parent lookups now fall through to the Guava L1 and the Redis L2,
which are invalidated inline on write and across nodes via CacheInvalidationPubSub;
with Redis disabled they fall through to the database. The batch path still issues
one bulk load per parent type, so list endpoints do not regress to an N+1 -- the
cost lands on repeated single-entity reads within one request. Its superset-field
matching also disappears, which only TableRepository and DatabaseSchemaRepository
relied on; narrower is a strict subset, so correctness is unchanged.

Two threads that run entity reads still never cleared the remaining per-request
ThreadLocals, so PerRequestContextCleaner now centralises that set and is called
from the Quartz consumer tick and, via WorkflowThreadCleanupListener, at Flowable
job completion.

Workflow reads additionally open a FreshReadScope, which forces fromCache=false.
Their reads are gating decisions where a stale answer silently routes an entity to
the wrong terminal status, and cross-node L1 invalidation requires Redis; workflow
volume is low, so the fresh read is the cheaper side of that trade.

This makes the reviewers-only getEffectiveReviewers helper added earlier on this
branch redundant, so it is reverted: it covered only reviewers and replaced the
list rather than merging own and inherited entries the way inheritance does.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings August 6, 2026 20:54

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.

Suppressed comments (3)

openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/Glossary/GlossaryInheritedReviewerApproval.spec.ts:31

  • This block comment refers to EntityRepository.inheritanceParentCache and ImpersonationCleanupFilter as the mechanism behind the ordering requirement, but this PR removes the inheritance parent ThreadLocal cache and adds explicit per-job cleanup/fresh-read scoping. The comment should be updated so future readers don't chase a deleted implementation detail.
 * The ordering matters. The approval workflow resolves inherited fields through
 * `EntityRepository.inheritanceParentCache`, a static ThreadLocal cleared only by
 * `ImpersonationCleanupFilter` — a JAX-RS response filter. Workflow execution threads never pass
 * through that filter, so a glossary snapshot captured while it had NO reviewers can be served
 * indefinitely. These tests therefore create the glossary WITHOUT reviewers first, let a term run the

openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/GlossaryTermInheritedReviewerApprovalIT.java:183

  • This test Javadoc still states (in present tense) that the bug is caused by EntityRepository.inheritanceParentCache being a static ThreadLocal only cleared by ImpersonationCleanupFilter. In this PR that cache is removed and cleanup is handled via PerRequestContextCleaner/Flowable job listeners, so the Javadoc is now misleading and should be rewritten to describe the scenario being regression-tested rather than the deleted mechanism.
   * Stale inheritance-parent cache. {@code EntityRepository.inheritanceParentCache} is a static
   * ThreadLocal cleared only by {@code ImpersonationCleanupFilter}, a JAX-RS response filter — so on
   * Flowable's async job-executor threads it is never cleared for the life of the process.
   *

openmetadata-service/src/main/java/org/openmetadata/service/governance/workflows/WorkflowHandler.java:289

  • The PR description explains the fix as adding an EntityRepository.getEffectiveReviewers hook and wiring the reviewers gate to use it, but the code changes here focus on clearing ThreadLocals at Flowable/Quartz boundaries and bypassing caches via FreshReadScope. If the implementation approach has changed, the PR description should be updated so reviewers and future maintainers understand the actual mechanism being relied on (or the missing reviewer-resolution hook should be added).
    // Add Global Failure Listener + per-job ThreadLocal cleanup for the async executor pool
    processEngineConfiguration.setEventListeners(
        List.of(new WorkflowFailureListener(), new WorkflowThreadCleanupListener()));

waitForSettledStatus accepted Draft as settled, but a term under a reviewed parent
is written as Draft at creation, before the workflow runs. The poll therefore
returned on its first sample and every assertion read the pre-workflow value, so
the spec reported the inherited-reviewer bug on a healthy build. Wait only for a
status the workflow itself commits; a term that never leaves Draft now times out
with a message naming the bug, which is the real signal.

WorkflowThreadCleanupListener matched job-boundary events by name string. A
Flowable rename would have fallen through to the no-op branch and silently stopped
clearing the per-request ThreadLocals on the async-executor pool, restoring the
stale-read leak with no compile error. Compare against the enum constants instead.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings August 6, 2026 21:24
@harshach harshach changed the title fix(governance): resolve inherited reviewers in the glossary approval gate fix(governance): resolve inherited fields freshly instead of from a never-cleared cache Aug 6, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.

Suppressed comments (4)

openmetadata-service/src/main/java/org/openmetadata/service/apps/bundles/changeEvent/AbstractEventConsumer.java:503

  • Cleanup at the end of the tick isn’t guaranteed to run if init(...) or downstream processing throws (or returns early in paths not shown here), which can reintroduce the ThreadLocal retention/staleness this change aims to prevent. Wrap the tick body in a try/finally so PerRequestContextCleaner.clear() is always executed on exit (success, error, or early return).
  public void execute(JobExecutionContext jobExecutionContext) {
    // Quartz worker threads are long lived and never pass through the JAX-RS response filter, so
    // per-request ThreadLocal caches would otherwise persist across ticks and serve indefinitely
    // stale reads. Destinations on this thread read entities (governance workflows resolve
    // inherited reviewers here), so start each tick from a clean slate.
    PerRequestContextCleaner.clear();
    this.init(jobExecutionContext);
    if (this.eventSubscription == null) {
      LOG.error("Skipping job execution - EventSubscription could not be loaded");
      return;
    }

openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/Glossary/GlossaryInheritedReviewerApproval.spec.ts:42

  • | string makes TermStatus effectively “any string”, which defeats the purpose of the union (and reduces type-safety for assertions/logging). Consider replacing | string with the specific additional states you actually use (e.g. '' for the initial value) so TypeScript flags unexpected statuses.
type TermStatus = 'Draft' | 'In Review' | 'Approved' | 'Rejected' | string;

openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/Glossary/GlossaryInheritedReviewerApproval.spec.ts:86

  • The poll loop parses JSON without asserting a successful response. If the API intermittently returns non-2xx (or HTML/error payloads), the failure can become a confusing JSON parse error rather than a clear HTTP error. Consider checking response.ok()/response.status() inside the poll and failing with a targeted message (status + body) to make test failures actionable.
        const response = await apiContext.get(
          `/api/v1/glossaryTerms/${termId}?fields=reviewers`
        );
        const term = await response.json();
        status = term.entityStatus;

openmetadata-service/src/main/java/org/openmetadata/service/governance/workflows/WorkflowThreadCleanupListener.java:45

  • Even though isFailOnException() returns false, letting an exception escape onEvent can still create noisy logs and may skip cleanup on the very boundary where you most need it. Consider wrapping PerRequestContextCleaner.clear() in a try/catch (logging at debug/warn) to guarantee the listener is best-effort and never throws.
  public void onEvent(FlowableEvent event) {
    // Registered engine-wide, so every event lands here: keep the check cheap and first. Compare
    // against the enum constants rather than their names so that if Flowable renames an event type,
    // this fails to compile instead of silently never clearing again.
    if (event.getType() == FlowableEngineEventType.JOB_EXECUTION_SUCCESS
        || event.getType() == FlowableEngineEventType.JOB_EXECUTION_FAILURE) {
      PerRequestContextCleaner.clear();
    }
  }

The exit-side clear sat in the inner finally, which init() precedes and which the
"subscription could not be loaded" early return skips entirely. Quartz worker
threads are shared with every other scheduled job, so state left behind on those
paths is inherited by whatever runs next on the thread — the entry-side clear only
protects the next AbstractEventConsumer tick, not an unrelated job. Bracket the
tick in try/finally so cleanup runs on success, error and early return alike.

The accompanying test pins the end-to-end invariant and fails if cleanup is
removed altogether. It does not isolate the exit clear from the entry clear: the
paths that previously skipped cleanup run inside the private init(), which offers
no seam to populate the ThreadLocals mid-tick. The unconditional exit clear is
enforced structurally by the try/finally rather than by that test, and the javadoc
says so.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings August 6, 2026 21:55

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated no new comments.

Suppressed comments (2)

openmetadata-service/src/test/java/org/openmetadata/service/apps/bundles/changeEvent/AbstractEventConsumerTest.java:172

  • This test asserts the thread is clean after execute(), but it can still pass if the exit-side cleanup is accidentally removed (because the new entry-side clear wipes the pre-set ThreadLocal before any work runs). Verifying that PerRequestContextCleaner.clear() is invoked twice (entry + finally) makes the invariant testable and guards against regressions.
  void execute_leavesThreadCleanForTheNextJob() {
    ImpersonationContext.setImpersonatedBy("someone");

    try {
      testEventConsumer.execute(jobExecutionContext);

openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/Glossary/GlossaryInheritedReviewerApproval.spec.ts:203

  • This step is described as creating terms "in a batch" to increase the chance of hitting a poisoned async-executor thread, but the loop currently serializes (it waits for each term to settle before creating the next). That makes the repro less reliable because it can keep landing on the same clean thread by chance. Create all probe terms first, then poll/assert in parallel.
      await test.step('Every term created afterwards inherits the reviewer and gets an approval task', async () => {
        for (let index = 0; index < REVIEWER_ADDED_PROBE_COUNT; index++) {
          const term = await createTermWithoutReviewers(
            apiContext,
            glossaryFqn,

Copilot AI review requested due to automatic review settings August 7, 2026 11:02

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated no new comments.

Suppressed comments (2)

openmetadata-service/src/main/java/org/openmetadata/service/util/FreshReadScope.java:36

  • FreshReadScope is implemented by forcing fromCache=false, which routes reads through dao.findEntityById(...) and bypasses the entire caching path (Guava L1 + optional Redis loader). The Javadoc currently implies it only opts out of in-process caches (and not Redis), which is misleading for future maintainers.
 * <p>{@link #enter()} restores the previous value rather than clearing, so nesting is safe. This is
 * the same shape as {@link org.openmetadata.service.cache.EntityCacheBypass}, which opts out of the
 * Redis layer; this one opts out of the in-process caches.

openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/Glossary/GlossaryInheritedReviewerApproval.spec.ts:152

  • This suite relies on cross-test ordering and shared mutable state (the first test adds a reviewer to the shared glossary; the second test assumes that reviewer exists). The repo Playwright config enables fullyParallel: true, so tests within a file/describe can run concurrently and this can become flaky. Mark the describe block as serial (or merge into a single test) to enforce the required ordering.
test.describe(
  'Glossary Approval - inherited reviewers',
  { tag: ['@Features', '@Governance'] },
  () => {

Copilot AI review requested due to automatic review settings August 7, 2026 11:25

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@gitar-bot

gitar-bot Bot commented Aug 7, 2026

Copy link
Copy Markdown
Code Review ✅ Approved 6 resolved / 6 findings

Removes the static inheritance parent cache and adds thread-scoped context cleaners and fresh read scopes to fix stale inherited field resolutions in workflows. No issues found.

✅ 6 resolved
Edge Case: Unresolvable parent reviewer aborts the approval gate

📄 openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TagRepository.java:260-268 📄 openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/GlossaryTermRepository.java:1792-1806 📄 openmetadata-service/src/main/java/org/openmetadata/service/governance/workflows/elements/nodes/automatedTask/impl/CheckEntityAttributesImpl.java:80-90
buildRuleData now calls getEffectiveReviewersUntyped, which for tags (TagRepository.getEffectiveReviewers) does Entity.getEntity(tag.getClassification(), FIELD_REVIEWERS, NON_DELETED), and for glossary terms (resolveEffectiveReviewers) fetches the parent term / glossary with Include.NON_DELETED. These Entity.getEntity calls throw EntityNotFoundException when the parent (classification / parent term / glossary) is missing or soft-deleted. The exception propagates out of checkAttributes, is wrapped as RuntimeException, and becomes a BpmnError(WORKFLOW_RUNTIME_EXCEPTION) that the boundary error event routes to a terminal error end — leaving the term stuck in its current status with no approval task ever created. The PR comment explicitly notes the previous setInheritedFields path swallowed exactly this failure and skipped inheritance silently; the new path converts that graceful skip into a hard workflow failure. Guard the inherited lookups so a missing parent falls back to the entity's own reviewers (empty) instead of aborting the gate.

Bug: waitForSettledStatus returns on transient Draft, making tests flaky

📄 openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/Glossary/GlossaryInheritedReviewerApproval.spec.ts:69-83 📄 openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/Glossary/GlossaryInheritedReviewerApproval.spec.ts:152-154 📄 openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/Glossary/GlossaryInheritedReviewerApproval.spec.ts:183-190
The poll's .toMatch(/In Review|Approved|Rejected|Draft/) includes Draft, but a term is created in Draft/Unprocessed and only transitions to In Review/Approved after the async workflow runs. Because Draft matches, expect.poll resolves on the very first sample while the term is still in transient Draft, defeating the stated purpose of waiting for a committed status. The subsequent expect(status).toBe('In Review') (and the seed's toBe('Approved')) then fail intermittently even when the workflow would have moved the term correctly — producing false detections of the inherited-reviewer bug. Drop Draft from the settle-match so the poll waits for a committed status; a term genuinely stuck in Draft will then time out with the descriptive message, which is the actual failure signal for this bug.

Quality: Job-boundary detection relies on stringly-typed event names

📄 openmetadata-service/src/main/java/org/openmetadata/service/governance/workflows/WorkflowThreadCleanupListener.java:36-44
WorkflowThreadCleanupListener.onEvent switches on event.getType().name() against the string literals "JOB_EXECUTION_SUCCESS"/"JOB_EXECUTION_FAILURE" rather than the FlowableEngineEventType enum constants. If Flowable ever renames these event types across a version bump, the switch silently falls through to the no-op branch and the per-request ThreadLocals stop being cleared on the async-executor pool — reintroducing exactly the indefinitely-stale-read leak this listener exists to prevent, with no compile error or test failure to signal it. Prefer comparing against FlowableEngineEventType.JOB_EXECUTION_SUCCESS/FAILURE directly (as the unit test already references) so a rename becomes a compile break.

Edge Case: expect.poll for approval task lacks explicit timeout

📄 openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/Glossary/GlossaryInheritedReviewerApproval.spec.ts:200-210
The new expect.poll(...).toBeGreaterThan(0) at lines 200-210 omits a timeout, so it falls back to the global expect timeout (Playwright default 5s) instead of the STATUS_TIMEOUT used elsewhere for workflow-driven state. Although the term already reached In Review, approval-task creation can lag slightly behind the status commit, so under load this poll may flake. Add timeout: STATUS_TIMEOUT (or a suitable smaller bound) to the poll options for consistency with waitForSettledStatus.

Edge Case: Workflow cancel inside delete txn is not atomic with it

📄 openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/EntityRepository.java:4859-4869 📄 openmetadata-service/src/main/java/org/openmetadata/service/governance/workflows/WorkflowHandler.java:1560-1566
cancelInstancesForEntity runs inside the JDBI delete transaction (before dao.delete), but Flowable's deleteProcessInstance commits against its own datasource/transaction immediately. If the entity-delete transaction rolls back after this point, the process instances are already permanently deleted while the entity row survives — leaving a live entity whose governance/approval workflow was destroyed, the same 'stuck' class of failure this PR fixes. Consider moving the cancellation to a post-commit hook (as the code already does for invalidate()) so cancellation only happens once the delete actually commits.

...and 1 more resolved from earlier reviews

Options

Display: compact → Showing less information.

Comment with these commands to change the behavior for this request:

Compact
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backend safe to test Add this label to run secure Github workflows on PRs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants