Stabilize flaky Playwright profile tests#30265
Conversation
… reason Supporting OpenMetadata changes for the Collate Data Access Request (DAR) concurrency work — clubbing concurrent policy-agent grants into a single ingestion run. - workflowSettings: add policyAgentConfiguration (pollingIntervalSeconds, batchWindowSeconds, batchMaxSize), read by the Collate batch coordinator. - policyAgentTaskDefinition: add pollingIntervalSeconds node config. - CreateTask: optionally carry a workflow-supplied reason into the created task's payload as `manualGrantReason` (its own structured field). Strict no-op (try/catch + instanceof) for workflows that do not set it, so generic approval workflows (Glossary, etc.) are byte-for-byte unaffected. - Tests: CreateTaskManualGrantReasonTest; the 44 generic CreateTaskTest pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- workflowSettings.policyAgentConfiguration: add `minimum: 1` to pollingIntervalSeconds / batchWindowSeconds / batchMaxSize; add `batchWorkerThreads` (default 4) so the coordinator's worker-pool size is configurable instead of a hard-coded cap. - policyAgentTaskDefinition: add `minimum: 1` to pollingIntervalSeconds and correct the fallback doc path to workflowSettings.policyAgentConfiguration.pollingIntervalSeconds. - Regenerate UI TypeScript for WorkflowSettings and PolicyAgentTaskDefinition so the generated types stay in sync with the schema. - Align the new test's license-header year (2026) with neighbouring tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…er-wide default Removing the per-node default (30) makes the generated field nullable, so an unset node value is now distinguishable from a configured one. The Collate PolicyAgentTask resolves it to the node override → server-wide policyAgentConfiguration.pollingIntervalSeconds → built-in default, which is what the field description already promised. (minimum:1 stays.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
mergeManualGrantReason called JsonUtils.getMap (a Jackson convertValue) even when the payload was already a Map, which can re-type nested values and can throw. Copy the existing map entries directly (mirroring withGrantExpirationDate) and only fall back to a guarded conversion for the unexpected non-Map case, so task creation is never broken by a conversion issue. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Summary.from_step caps the reported failure details to the first 10 (status payload size). That's fine for diagnostics but unsafe when downstream logic needs the COMPLETE per-item failure list — e.g. the Collate Policy Agent batch coordinator, which decides each clubbed request's outcome (grant vs manual) from exactly which policy ids failed; a truncated list silently grants the un-reported failures. Add an opt-in `report_all_failures` flag (default False → every existing connector keeps the 10-cap). The error COUNT was already accurate. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replace per-DAR timer polling with an event-based gateway that races the coordinator's result signal against a safety timer. WorkflowHandler delivers POLICY_AGENT_RESULT_MESSAGE to wake a parked DAR the moment its clubbed batch resolves; the timer is the fallback if the signal is ever missed, and the node re-reads the authoritative DB rows on wake so a missed signal self-corrects. - WorkflowHandler.signalPolicyAgentResult: wake parked DAR(s) by process instance; list() + per-execution delivery so a shared message name with multiple parked nodes or a raced subscription never strands a request. - policyAgentTaskDefinition: drop dead node-level pollingIntervalSeconds (the push design no longer polls); add minimum 1 to timeoutSeconds so a 0 value cannot make the safety timer fire instantly and beat the signal. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…e's connection pool A burst of concurrent task approvals (e.g. many Data Access Requests approved at once) had each resolution call taskService.complete() synchronously on the request thread, all contending for Flowable's own MyBatis connection pool (default 10 active). Excess threads blocked in PooledDataSource.popConnection past the client timeout and left tasks stranded in review. Add a fair admission semaphore (MAX_CONCURRENT_TASK_RESOLUTIONS) around resolveTaskInternal: only a bounded number of resolutions touch Flowable at once; the rest queue efficiently in-JVM (FIFO) and drain in order. The pool is never driven to exhaustion, so other governance workflows keep their connections. This bounds the failure mode from catastrophic pool-stampede to graceful queueing — it does not add capacity (a finite pool always has a ceiling), it makes the existing capacity usable without the popConnection pathology. Verified with a 100-wide unpaced approval burst: 100/100 resolved, 0 timeouts, 0 dead-letters, DB load flat vs idle. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ssignees When a task defaults its assignees to the target entity's owners, a Team owner was assigned as-is. Expand Team references to their member users so the task is assigned to actual people rather than a team placeholder. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Resolve policyAgentTaskDefinition.json: keep both the timeoutSeconds minimum:1 (this branch) and the accessType override field (revoke work from main).
…e report_all_failures, keep admin fallback - CreateTask: include the caught exception in the manualGrantReason merge warn log, and debug-log (not silently swallow) a real lookup error in resolveManualGrantReason. - step.py: declare `report_all_failures = False` on the base `Step` class (so the opt-out is discoverable, not just a getattr string) and reformat the `failures=` conditional to stay under the Ruff line-length limit. - TaskRepository.expandTeamsToUsers: a team with no members intentionally contributes no assignees so the empty list triggers the node's assignAdmins fallback in SetApprovalAssigneesImpl (workflow-managed DARs route to platform admins) rather than pinning the task to a member-less team. Documented to avoid a future "regression". Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Optional, system-set field carrying why a data access request was routed to manual approval (partial auto-grant, enforcement failure, or timeout). Absent on the automatic path. Regenerated the TypeScript type. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… mismatch ruff format the report_all_failures conditional (fixes py-checkstyle) and add an inline pyright ignore for the pre-existing TruncatedStackTraceError vs StackTraceError invariance that the reformat un-baselined (fixes static-checks). Add a unit test for the 10-failure display cap and the report_all_failures opt-out. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…fy polling scope - Extract the report_all_failures display-cap logic into a readable local instead of a nested ternary. No behavior change. - Clarify workflowSettings.pollingIntervalSeconds: it is the batch coordinator's poll of the ingestion run; the Data Access Request workflow node is signalled (pushed), not polled. Drops the stale per-node-override claim. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
❌ 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 |
|
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! |
🔴 Playwright Results — 16 pipeline/setup failure(s)✅ 0 passed · ❌ 0 failed · 🟡 0 flaky · ⏭️ 0 skipped Pipeline and setup failures
How to debug locally# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip # view trace |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| } catch (Exception e) { | ||
| LOG.debug( | ||
| "Failed to expand team {} to users: {}", ref.getFullyQualifiedName(), e.getMessage()); | ||
| } |
There was a problem hiding this comment.
When an owning team cannot be loaded because of a stale reference or transient repository failure, this branch drops that owner and still installs the partial expanded list. The old path retained the team assignee; the new path can create an unassigned task or trigger the workflow's administrator fallback, routing approval away from the configured owner.
Context Used: CLAUDE.md (source)
| } catch (FlowableException ex) { | ||
| // Subscription consumed/removed between query and delivery (timer won the race, or a | ||
| // concurrent signal): safe to ignore — the node resolves from the DB rows regardless. | ||
| LOG.debug( | ||
| "PolicyAgent result signal no-op for execution {}: {}", | ||
| execution.getId(), | ||
| ex.getMessage()); |
There was a problem hiding this comment.
Only a missing or concurrently consumed subscription is a safe no-op, but this catches every FlowableException. A database or optimistic-lock failure is therefore hidden at DEBUG level, so the coordinator returns as if delivery succeeded while the request remains parked until its safety timer.
Context Used: CLAUDE.md (source)
| protected async waitForBlockingAlertsToClear(): Promise<void> { | ||
| await expect(this.page.getByTestId('alert-bar')).toHaveCount(0, { | ||
| timeout: 15_000, | ||
| }); | ||
| } |
There was a problem hiding this comment.
This treats every alert-bar as a blocking overlay and waits for the element to be removed. A persistent or manually dismissible alert can leave the target control usable but make each description, tag, or glossary action time out after 15 seconds; the wait should target the overlay that actually intercepts these controls.
Context Used: CLAUDE.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| private List<EntityReference> expandTeamsToUsers(List<EntityReference> refs) { | ||
| List<EntityReference> result = new ArrayList<>(); | ||
| for (EntityReference ref : refs) { | ||
| if (!Entity.TEAM.equals(ref.getType())) { | ||
| result.add(ref); | ||
| continue; | ||
| } | ||
| try { | ||
| Team team = Entity.getEntity(Entity.TEAM, ref.getId(), "users", Include.NON_DELETED); | ||
| if (!nullOrEmpty(team.getUsers())) { | ||
| result.addAll(team.getUsers()); | ||
| } | ||
| // A team with no members intentionally contributes no assignees: for workflow-managed tasks | ||
| // (e.g. Data Access Requests) an empty assignee list triggers the node's | ||
| // emptyAssigneeStrategy |
There was a problem hiding this comment.
💡 Bug: expandTeamsToUsers can produce duplicate assignees
expandTeamsToUsers concatenates direct user owners and each team's members without de-duplication, so a user who is both a direct owner and a member of an owning team (or a member shared by two owning teams) is added to the assignee list more than once. Downstream updateAssignees tolerates duplicates via removeAll-based set math, but the persisted/echoed assignee list can still surface the same user twice (e.g. duplicate display in the UI) and causes redundant relationship writes. De-duplicate by id before returning.
Key by user id to drop duplicate assignees while preserving order.:
private List<EntityReference> expandTeamsToUsers(List<EntityReference> refs) {
Map<UUID, EntityReference> result = new LinkedHashMap<>();
for (EntityReference ref : refs) {
if (!Entity.TEAM.equals(ref.getType())) {
result.putIfAbsent(ref.getId(), ref);
continue;
}
try {
Team team = Entity.getEntity(Entity.TEAM, ref.getId(), "users", Include.NON_DELETED);
if (!nullOrEmpty(team.getUsers())) {
team.getUsers().forEach(u -> result.putIfAbsent(u.getId(), u));
}
} catch (Exception e) {
LOG.debug(
"Failed to expand team {} to users: {}", ref.getFullyQualifiedName(), e.getMessage());
}
}
return new ArrayList<>(result.values());
}
- Apply fix
Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎
Code Review 👍 Approved with suggestions 0 resolved / 1 findingsStabilizes Playwright profile tests by enforcing PATCH response synchronization and adds robust governance workflow controls. The team-to-user expansion logic requires a de-duplication step to avoid assigning duplicate users to tasks. 💡 Bug: expandTeamsToUsers can produce duplicate assignees📄 openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TaskRepository.java:459-473 expandTeamsToUsers concatenates direct user owners and each team's members without de-duplication, so a user who is both a direct owner and a member of an owning team (or a member shared by two owning teams) is added to the assignee list more than once. Downstream updateAssignees tolerates duplicates via removeAll-based set math, but the persisted/echoed assignee list can still surface the same user twice (e.g. duplicate display in the UI) and causes redundant relationship writes. De-duplicate by id before returning. Key by user id to drop duplicate assignees while preserving order.🤖 Prompt for agentsOptionsDisplay: compact → Showing less information. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar |
2 participants
Summary
Test Plan
Note: yarn lint:playwright and yarn tsc:playwright could not run in this workspace because Yarn fails before invoking the scripts with: open-metadata@workspace:. is missing from the lockfile.
Summary by Gitar
SemaphoreinWorkflowHandlerto prevent connection pool exhaustion.signalPolicyAgentResultmethod to allow manual triggering of parked DAR workflow nodes.manualGrantReasonpassthrough for task payloads inCreateTask.TaskRepositoryto expand team owners into individual member users when defaulting assignees for new tasks.This will update automatically on new commits.
Greptile Summary
This PR expands Policy Agent workflow support and strengthens Playwright save synchronization. The main changes are:
Confidence Score: 4/5
Task assignment and Policy Agent signaling can silently take the wrong fallback path. These paths need fixes before merging.
TaskRepository.java, WorkflowHandler.java, and OverviewPageObject.ts.
Security Review
A failed team-owner expansion can remove the configured approval audience and invoke administrator fallback. This can route an approval to principals who were not listed as the entity owner.
Important Files Changed
Sequence Diagram
%%{init: {'theme': 'neutral'}}%% sequenceDiagram participant DAR as Data Access Request participant PA as Policy Agent Coordinator participant DB as Outcome Store participant WF as Workflow Engine PA->>DB: Write per-policy outcomes PA->>WF: Send policyAgentResult message WF->>DB: Read outcomes WF-->>DAR: Continue grant or manual approval Note over WF,DAR: Safety timer resumes requests when a signal is missed%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% sequenceDiagram participant DAR as Data Access Request participant PA as Policy Agent Coordinator participant DB as Outcome Store participant WF as Workflow Engine PA->>DB: Write per-policy outcomes PA->>WF: Send policyAgentResult message WF->>DB: Read outcomes WF-->>DAR: Continue grant or manual approval Note over WF,DAR: Safety timer resumes requests when a signal is missedReviews (1): Last reviewed commit: "test: stabilize flaky Playwright profile..." | Re-trigger Greptile
Context used (3)