Skip to content

LT-14056: Crash with Undo then Redo deleted Lexical Relations#383

Merged
aror92 merged 2 commits into
masterfrom
bugfix/LT-14056
Jul 1, 2026
Merged

LT-14056: Crash with Undo then Redo deleted Lexical Relations#383
aror92 merged 2 commits into
masterfrom
bugfix/LT-14056

Conversation

@aror92

@aror92 aror92 commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Add null cache guards. The redo is deleting a lexical relation that was just "undeleted" via undo. Tries to restore refs when the object has already been deleted.


This change is Reviewable

Add null cache guards. The redo is deleting a lexical relation
that was just "undeleted" via undo. Tries to restore refs when
the object has already been deleted.
@github-actions

github-actions Bot commented Jul 1, 2026

Copy link
Copy Markdown

LCM Tests

    16 files  ±0      16 suites  ±0   3m 10s ⏱️ +5s
 2 863 tests ±0   2 843 ✅ ±0   20 💤 ±0  0 ❌ ±0 
11 400 runs  ±0  11 232 ✅ ±0  168 💤 ±0  0 ❌ ±0 

Results for commit 65ac4fa. ± Comparison against base commit c0adfa3.

@aror92 aror92 enabled auto-merge (squash) July 1, 2026 13:42

@mark-sil mark-sil left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@mark-sil reviewed all commit messages and made 1 comment.
Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on aror92).


src/SIL.LCModel/DomainImpl/CmObject.cs line 2634 at r1 (raw file):

			if (Cache == null)
			{
				Debug.Fail("Restoring incoming refs on outgoing refs for an object already deleted");

Why do we want the Debug.Fail() messages (both this one and the other)? To me they imply that there is something wrong that needs to be investigated. If the correct solution is to just return (as you are doing), then we probably shouldn't be alerting the developers.

@aror92 aror92 left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@aror92 made 1 comment.
Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on mark-sil).


src/SIL.LCModel/DomainImpl/CmObject.cs line 2634 at r1 (raw file):

Previously, mark-sil (Mark Kidder) wrote…

Why do we want the Debug.Fail() messages (both this one and the other)? To me they imply that there is something wrong that needs to be investigated. If the correct solution is to just return (as you are doing), then we probably shouldn't be alerting the developers.

The Debug.Fail was an addition recommended by Devin/LcmDeepWiki. I was torn on it, but Devin pointed out that we already have one guard for the same reason in ClearIncomingRefsOnOutgoingRefsInternal, and in that case, it includes the comment "SHOULD never be called on already deleted object" and uses a Debug.Fail message for that reason.

The same logic would apply to both of these guards, so I included the Debug.Fail messages. (i.e. Cache == null means undo/redo is operating on an already deleted object, which we appear to claim should never happen by design.) I'm okay removing the Debug.Fails, though, if we're sure we don't want them.

@aror92 aror92 disabled auto-merge July 1, 2026 15:46
@aror92 aror92 enabled auto-merge (squash) July 1, 2026 15:46

@mark-sil mark-sil left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@mark-sil reviewed 2 files and all commit messages, and resolved 1 discussion.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on aror92).

@aror92 aror92 merged commit 840564b into master Jul 1, 2026
4 checks passed
@aror92 aror92 deleted the bugfix/LT-14056 branch July 1, 2026 15:50
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.

2 participants