Skip to content

chore: bugfix for two way no merge and better error handling when merging (CM-1117)#4068

Open
themarolt wants to merge 14 commits intomainfrom
better-error-message-handling-CM-1117
Open

chore: bugfix for two way no merge and better error handling when merging (CM-1117)#4068
themarolt wants to merge 14 commits intomainfrom
better-error-message-handling-CM-1117

Conversation

@themarolt
Copy link
Copy Markdown
Contributor

@themarolt themarolt commented May 4, 2026

Note

Medium Risk
Touches member identity conflict handling and automatic merge/redirect behavior during activity ingestion, which can affect member linkage and downstream data correctness. Changes also alter how merge failures/no-merge cases are surfaced in integration.results and Slack reporting.

Overview
Member identity conflicts now resolve via redirects and structured lookups. MemberService adds conflict-aware identity insertion plus syncIdentitiesAfterRedirect to follow merge survivors, backfill/verify identities on the surviving member, and cap chained merges; update() can now return a redirect memberId when a merge occurs.

Activity ingestion reuses redirects and avoids redundant updates. ActivityService tracks per-batch member redirects (including actor==objectMember), skips updates for already-absorbed members, and enhances handleMemberIdentityError to avoid Postgres error.detail parsing, treat stale-prefetch self-conflicts as success, and attach specific metadata.errorMessage values for no-merge/merge-failure cases.

Reporting groups errors more usefully. The cron Slack report groups integration.results errors by error.metadata.errorMessage first (falling back to the outer errorMessage) so merge-related failures appear as distinct buckets. Additionally, findMembersByIdentities now uses parameterized identity tuples instead of string interpolation.

Reviewed by Cursor Bugbot for commit 3918348. Bugbot is set up for automated code reviews on this repo. Configure here.

…ging

Signed-off-by: Uroš Marolt <uros@marolt.me>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR addresses edge cases in the data sink member-merge flow (especially “two-way” no-merge relationships) and improves observability by attaching more specific error metadata for identity-merge conflicts, which is then used for better error grouping in the cron reporting job.

Changes:

  • Fix no-merge detection to handle bidirectional (A↔B) no-merge rows without throwing.
  • Enhance verified-identity conflict handling by treating “identity already belongs to this member” as a no-op and by setting more explicit metadata.errorMessage values when merges are blocked or fail.
  • Update integration error reporting to group errors by error.metadata.errorMessage when present.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
services/apps/data_sink_worker/src/service/member.service.ts Adjusts no-merge detection to avoid exceptions when multiple matching rows exist (two-way no-merge).
services/apps/data_sink_worker/src/service/activity.service.ts Adds special-case handling for stale prefetch identity conflicts and enriches metadata error messaging around merge attempts.
services/apps/cron_service/src/jobs/integrationResultsReporting.job.ts Groups error breakdown by error.metadata.errorMessage for more actionable aggregation in Slack reports.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread services/apps/data_sink_worker/src/service/activity.service.ts
Comment thread services/apps/cron_service/src/jobs/integrationResultsReporting.job.ts Outdated
Copilot AI review requested due to automatic review settings May 5, 2026 06:20
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

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 3 out of 3 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread services/apps/data_sink_worker/src/service/member.service.ts Outdated
Comment thread services/apps/data_sink_worker/src/service/member.service.ts Outdated
Comment thread services/apps/data_sink_worker/src/service/member.service.ts
Signed-off-by: Uroš Marolt <uros@marolt.me>
Comment thread services/apps/data_sink_worker/src/service/member.service.ts
Signed-off-by: Uroš Marolt <uros@marolt.me>
Copilot AI review requested due to automatic review settings May 5, 2026 08:29
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

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 3 out of 3 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread services/apps/data_sink_worker/src/service/member.service.ts Outdated
Comment thread services/apps/data_sink_worker/src/service/member.service.ts
Signed-off-by: Uroš Marolt <uros@marolt.me>
Comment thread services/apps/data_sink_worker/src/service/activity.service.ts
Copilot AI review requested due to automatic review settings May 5, 2026 09:47
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

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 3 out of 3 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread services/apps/data_sink_worker/src/service/member.service.ts
Comment thread services/apps/data_sink_worker/src/service/member.service.ts Outdated
…tic identity

Signed-off-by: Uroš Marolt <uros@marolt.me>
Comment thread services/apps/data_sink_worker/src/service/activity.service.ts
Comment thread services/apps/data_sink_worker/src/service/member.service.ts Outdated
Signed-off-by: Uroš Marolt <uros@marolt.me>
Copilot AI review requested due to automatic review settings May 5, 2026 19:51
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

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 3 out of 3 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread services/apps/data_sink_worker/src/service/member.service.ts
Comment thread services/apps/data_sink_worker/src/service/activity.service.ts
Comment thread services/apps/data_sink_worker/src/service/activity.service.ts
Signed-off-by: Uroš Marolt <uros@marolt.me>
Comment thread services/apps/data_sink_worker/src/service/activity.service.ts
Signed-off-by: Uroš Marolt <uros@marolt.me>
Copilot AI review requested due to automatic review settings May 5, 2026 21:52
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

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 4 out of 4 changed files in this pull request and generated 4 comments.

Comments suppressed due to low confidence (1)

services/libs/data-access-layer/src/members/identities.ts:596

  • findMembersByIdentities can generate invalid SQL when called with default args (no memberIdToIgnore and onlyVerified=false): conditions stays empty, so the query becomes WHERE with nothing after it. Consider either adding a guard that pushes a TRUE condition when conditions.length === 0, or conditionally omitting the WHERE clause.
        on mi.platform = i.platform
        and lower(mi.value) = lower(i.value)
        and mi.type = i.type
        and mi."deletedAt" is null
    where ${conditions.join(' and ')}
  `,

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread services/apps/data_sink_worker/src/service/member.service.ts
Comment thread services/apps/data_sink_worker/src/service/member.service.ts
Comment thread services/apps/data_sink_worker/src/service/activity.service.ts
Comment thread services/apps/data_sink_worker/src/service/activity.service.ts
Signed-off-by: Uroš Marolt <uros@marolt.me>
Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 3918348. Configure here.

if (
key.slice(0, sep1) === id.platform &&
key.slice(sep1 + 1, sep2) === id.type &&
key.slice(sep2 + 1).toLowerCase() === id.value.toLowerCase() &&
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Missing .trim() causes owner lookup to fail with whitespace

Medium Severity

The value comparison in insertIdentitiesWithConflictResolution uses key.slice(sep2 + 1).toLowerCase() === id.value.toLowerCase() without .trim(). Since findMembersByIdentities trims values before querying (line 577 in identities.ts), the map keys contain trimmed values. When incoming identity values have leading/trailing whitespace, this comparison will never match, causing owner to remain undefined and the function to re-throw instead of resolving the conflict via merge. The equivalent code in handleMemberIdentityError and syncIdentitiesAfterRedirect correctly uses .trim() on both sides.

Additional Locations (2)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 3918348. Configure here.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants