Fixes #30526: mirror incident re-assignment into the TCRS timeline - #30525
Fixes #30526: mirror incident re-assignment into the TCRS timeline#30525manerow wants to merge 3 commits into
Conversation
✅ PR checks passedThe linked issue has a description and all required Shipping project fields set. Thanks! |
Code Review ✅ Approved 1 resolved / 1 findingsRefactors TCRS timeline tests to use SequencedCollection getLast(), addressing the collection access finding. Code is clean and well-tested. ✅ 1 resolved✅ Quality: Use SequencedCollection getLast() instead of get(size()-1)
OptionsDisplay: compact → Showing less information. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar |
✅ Playwright Results — workflow succeededValidated commit ✅ 537 passed · ❌ 0 failed · 🟡 0 flaky · ⏭️ 5 skipped · 🧰 0 lifecycle flaky PerformanceBlocking targets: ✅ met · Optimization targets: 🟡 in progress Shard-job maxima below are not the full workflow wall time; the linked run includes build, fixture, planning, and reporting. 🕒 Full workflow signal wall (to summary) 46m 49s ⏱️ Max setup 2m 57s · max shard execution 18m 20s · max shard-job elapsed before upload 22m 8s · reporting 3s 🌐 202.77 requests/attempt · 2.86 app boots/UI scenario · 5.27% common-shard skew Optimization targets still in progress:
How to debug locally# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip # view trace |
Describe your changes:
Fixes #30526
Re-assigning an open incident returned 200 and updated the Task, but every UI surface kept showing the previous assignee.
Task is the source of truth for incidents;
IncidentTcrsSyncHandlermirrors it into the TCRS time series for the legacy consumers (DQ page badge, search aggregations, dashboards, exporters). Both the test case page header and the Incident Manager list write to the Task and then read the assignee back out of TCRS, so a mirror that misses the change is a mirror that shows a stale assignee.reassignis a self-loop:TestCaseResolutionTaskWorkflow.jsontargets stageassignedand the edge isAssignedStage -> AssignedStage, soworkflowStageIdis identical before and after. The handler had two guards, and neither looked at the assignee:Assigned -> Assignedwould have been skipped anyway.The result was that
testCaseResolutionStatusDetails.assigneestayed frozen at the first assignee for the life of the incident, on/testCaseIncidentStatus/stateId/{id},?latest=true,/search/listand the inlinetestCase.incidentStatus.What changed
The mirrored projection of a task is its status plus, in the
assignedstage, its assignee. That is now the only thing the handler keys off:handleTaskCreateandhandleTaskUpdatecollapse into onesync(Task), called from bothpostCreateandpostUpdate. The stage pre-image guard is gone: it was a performance filter that had quietly become the correctness filter, which is how a same-stage transition slipped through.Appending rather than patching matches
cleanUpAssignees, which already treats an assignee change as a new record, and matches the pre-task-first behaviour the mirror exists to preserve. Severity survives becausesyncFromTaskinherits it from the previous record for thestateId, andsetResolutionMetricsis unaffected since it only writes onNew -> *and onResolved.The assignee id is compared rather than the whole details object because a stored
EntityReferencecomes back hydrated with extra fields; a deep compare would report a difference on every unrelated task update and append duplicates.No UI change is needed: the write happens inside the resolve request, so the existing read-back picks it up.
Cost:
getLatestRecordForStateIdnow runs on every incident-task update instead of only on stage changes. Incident tasks change at human rate, and a repeated failure on an open incident does not touch the task, so this is a few small indexed reads per incident.Type of change:
High-level design:
N/A — small change.
Tests:
Use cases covered
assignedstage updates the assignee shown on the test case page header and in the Incident Manager list.Resolvedrecord.Backend integration tests
openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/IncidentTaskIntegrationIT.javatestReassignment_AssignedToAssignedstrengthened: it asserted the Task assignee and the TCRS status type, but the type was alreadyAssignedfrom the first assign and the helper usedanyMatch, so it passed without a record ever being written. It now asserts the latest TCRS record's assignee and the full assignment timeline.testUpdatesThatDoNotChangeTheMirror_AppendNothingadded, covering the no-op update and duplicate-Resolvedcases.assertTcrsStatusEventuallydeleted. It assertedanyMatch(status)over the whole timeline, which is what made the reassign test vacuous, and it is blind to the same class of bug elsewhere: intestResolveAfterReopen_CompletesSameTaskthe timeline isNew -> Resolved -> Ack -> Resolved, soanyMatch(Resolved)passes on the first record even if the second is never mirrored. All 9 remaining call sites now assert the latest record and, where the incident is assigned, its assignee.assertLatestTcrsEventually/tcrsTimelinehelpers added.Manual testing performed
Local stack on this branch:
POST /v1/tasks/{id}/resolve {"transitionId":"assign"}to user A, confirmedAssigned/ A.POST /v1/tasks/{id}/resolve {"transitionId":"reassign"}to user B.Before the change, step 4 reported A on every TCRS path.
UI screen recording / screenshots:
Not applicable.
Checklist:
Fixesabove.Greptile Summary
The PR updates incident-to-TCRS synchronization and its integration coverage.
Confidence Score: 3/5
The PR is not yet safe to merge because concurrent incident updates can leave legacy consumers observing a stale final TCRS projection.
The previously reported race remains: post-commit synchronization independently reads the latest TCRS record and later inserts a new one without per-state serialization or an atomic compare-and-append operation, so concurrent task updates can persist projections in the wrong final order.
Files Needing Attention: openmetadata-service/src/main/java/org/openmetadata/service/events/lifecycle/handlers/IncidentTcrsSyncHandler.java; openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TestCaseResolutionStatusRepository.java
Important Files Changed
Sequence Diagram
sequenceDiagram participant Client participant Task as TaskRepository participant Sync as IncidentTcrsSyncHandler participant TCRS as TCRS Repository Client->>Task: Update incident task Task->>Task: Commit canonical task state Task->>Sync: sync(updated) Sync->>TCRS: Read latest projection Sync->>Sync: Compare status and assignee ID alt Projection changed Sync->>TCRS: Append projected record else Projection unchanged Sync-->>Task: Skip append endReviews (3): Last reviewed commit: "test(dq): use SequencedCollection getLas..." | Re-trigger Greptile