sillsdev / sillsdev/TheCombine
Merge fails or silently duplicates when a word has left the Frontier
Nobody has claimed this yet.
- Dominant language
- TypeScript
- Stars
- 22
- Forks
- 10
- Avg merge
- 2d 2h
- Merged PRs (30d)
- 9
Description
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.
- 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.
DeleteFrontierWithSessionreturns null for a stale sibling,ArgumentExceptionaborts
the transaction, and the retry 400s with nothing written. - A single child - a column whose only change is a vernacular or flag edit, so
createMergeChildrenyields onesrcWordIdandisEmptyMergeis false. The parent
carries that id,oldIdSetempties, and nothing is left to delete. The one remaining
call isUpdateFrontierWithSession(..., 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-#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.
Related
- #4198 introduced the regression.
- #4365 fixed a different 400 on the same endpoint (merge parent with no id).
- #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.
Contributor guide
No contributing guide indexed for this repository
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with Backend/Repositories/WordRepository.cs at ReplaceFrontierWithSession and the merge flow in MergeService.Merge, then trace deferred review in src/goals/Redux/GoalActions.ts, including mergeAll and the loading logic around the cited lines. Reproduce overlapping or changed deferred sets and retry behavior. Done means stale words do not produce a 400 or silently create a duplicate, with the chosen refresh or failure behavior covered by tests.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- csharp, typescript
- Domain
- api, backend, frontend
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100