You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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:
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:
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.
In Merge Duplicates, defer a set containing word A.
Later, defer another set that also contains word A (a superset, or a different pairing).
Start Review Deferred Duplicates. Both sets load as steps.
Merge the first set. Word A leaves the Frontier and its replacement gets a new id.
Reach the second step and merge it.
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:
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.
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.
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.
Update merge failure handling #4366 adds the missing exception logging, which is how a recurrence of this would be
confirmed from server logs, and stops a post-merge bookkeeping failure from presenting a
committed merge as retryable.
Describe the bug
Since #4198, a merge fails with
400 .../mergewhen any of its words has left the Frontiersince 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.
ReplaceFrontierWithSessionthrows 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.Mergeended withawait Task.WhenAll(childrenIds.Select(id => _wordService.DeleteFrontierWord(...)))andignored 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
UpdateMergeGraylistprunes ids that have left the Frontier, but only at load time, so itdoes 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,
UpdateFrontierWithSessionis called withcreateIfNotFound: true, finds nothing toreplace, 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:
AddToMergeGraylistonly removes entries fullycontained in the new set, and
isUnavailableSetonly rejects a candidate that is asubset of an existing entry, so
{w1,w2}and{w1,w2,w3}can both sit in the graylist.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
mergeAllsends the merge, then records it on the goal and blacklists the result. Arejection 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:
DeleteFrontierWithSessionreturns null for a stale sibling,ArgumentExceptionabortsthe transaction, and the retry 400s with nothing written.
createMergeChildrenyields onesrcWordIdandisEmptyMergeis false. The parentcarries that id,
oldIdSetempties, and nothing is left to delete. The one remainingcall is
UpdateFrontierWithSession(..., createIfNotFound: true), which finds nothing toreplace 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'sGetWordlookup succeeds on the retry and nothingupstream catches the staleness.
#4366 narrows this by not rejecting for failures that happen after the merge request
returns, so a failed
asyncUpdateGoalorblacklistAddno longer presents a committedmerge 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:
than loading every step up front. Staleness within a session disappears. Does not help
with concurrent edits by another user.
already left the Frontier. Simple, but merges a set against data the user did not see.
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: trueshould 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
confirmed from server logs, and stops a post-merge bookkeeping failure from presenting a
committed merge as retryable.