Skip to content

Merge fails or silently duplicates when a word has left the Frontier #4367

Description

@imnasnainaec

Describe the bug

Since #4198, a merge fails with 400 .../merge when any of its words has left the Frontier
since the merge set was loaded. A word leaves the Frontier whenever it is edited or merged
anywhere, because each edit writes a new document with a new id.

ReplaceFrontierWithSession throws when a merge child is no longer in the Frontier:

https://github.com/sillsdev/TheCombine/blob/master/Backend/Repositories/WordRepository.cs#L553-L559

Before #4198 this was tolerated: MergeService.Merge ended with
await Task.WhenAll(childrenIds.Select(id => _wordService.DeleteFrontierWord(...))) and
ignored the result, so a child that had already left the Frontier was skipped and the merge
succeeded.

Review Deferred Duplicates is the most exposed flow, because every step is loaded up front
and never refreshed:

https://github.com/sillsdev/TheCombine/blob/master/src/goals/Redux/GoalActions.ts#L277-L280

UpdateMergeGraylist prunes ids that have left the Frontier, but only at load time, so it
does not help once the session is underway.

There is a worse variant that does not error. Which word in the set carries the stale id
decides the outcome. If a child does, the merge 400s. If the parent does,
UpdateFrontierWithSession is called with createIfNotFound: true, finds nothing to
replace, and creates a brand-new word — the merge reports success and the project quietly
gains a duplicate entry. That path is silent and, unlike the 400, leaves bad data behind.

To Reproduce

Deferred sets may legitimately overlap: AddToMergeGraylist only removes entries fully
contained in the new set, and isUnavailableSet only rejects a candidate that is a
subset of an existing entry, so {w1,w2} and {w1,w2,w3} can both sit in the graylist.

  1. In Merge Duplicates, defer a set containing word A.
  2. Later, defer another set that also contains word A (a superset, or a different pairing).
  3. Start Review Deferred Duplicates. Both sets load as steps.
  4. Merge the first set. Word A leaves the Frontier and its replacement gets a new id.
  5. Reach the second step and merge it.
  6. Either 400 https://<server>/v1/projects/<projectId>/merge, or a silent duplicate,
    depending on which word the parent column inherited its guid from.

The same thing happens without overlapping sets if another user in the project — or the
same user in another tab, or in Review Entries — edits one of the words while the deferred
review is open.

Retrying a save is a second way in, and the duplicate is silent

mergeAll sends the merge, then records it on the goal and blacklists the result. A
rejection does not prove the merge failed: the server can commit before the response is
lost. Any retry then resubmits child ids the committed merge already removed, which lands
in exactly the two cases above.

Which one depends on the size of the set, and the small one is the dangerous one:

  • More than one child. The parent takes one child's id; the others are stale.
    DeleteFrontierWithSession returns null for a stale sibling, ArgumentException aborts
    the transaction, and the retry 400s with nothing written.
  • A single child - a column whose only change is a vernacular or flag edit, so
    createMergeChildren yields one srcWordId and isEmptyMerge is false. The parent
    carries that id, oldIdSet empties, and nothing is left to delete. The one remaining
    call is UpdateFrontierWithSession(..., createIfNotFound: true), which finds nothing to
    replace and creates a new word. The retry returns 200 and the project gains a
    duplicate.

Note the original word is still in the words collection after a merge - only the Frontier
copy is deleted - so MergePrepParent's GetWord lookup succeeds on the retry and nothing
upstream catches the staleness.

#4366 narrows this by not rejecting for failures that happen after the merge request
returns, so a failed asyncUpdateGoal or blacklistAdd no longer presents a committed
merge as retryable. It cannot close the case where the merge request itself throws after
the server committed; that needs the work below.

Expected behavior

Merging a set whose words have changed underneath should not 400 and should never create a
duplicate entry. Either the set is refreshed and the user re-reviews it against current
data, or the merge proceeds on the words that are still current.

Environment

Reported on thecombine.app (v3.0.0); present on master. Backend regression introduced in
#4198.

Possible fixes

Three places this could be addressed, not mutually exclusive:

  1. Frontend, root cause. Fetch each deferred step's words when the step is entered rather
    than loading every step up front. Staleness within a session disappears. Does not help
    with concurrent edits by another user.
  2. Backend, tolerate. Restore the pre-Use Mongodb replica set and transactions #4198 behavior of skipping children that have
    already left the Frontier. Simple, but merges a set against data the user did not see.
  3. Backend, fail cleanly. Return a distinguishable status for a stale set so the frontend
    can reload that step and ask the user to review it again. Best UX, most work.

Whichever is chosen, the parent-side hole should be closed too: createIfNotFound: true
should not silently create a word when the id it was given has vanished from the Frontier.
That single change turns the silent-duplicate case into an ordinary error, and is worth
doing regardless of which of the three is picked.

For an ambiguous merge response specifically, the frontend should reload the current set
rather than leaving the pre-merge tree on screen for the user to resubmit.

Related

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions