Don't delete the constraints that the solver couldn't satisfy. - #1744
ruevs merged 2 commits into
Conversation
Sorry, something went wrong.
PruneOrphans() and PruneRequestsAndConstraints() tag the requests and constraints that they mean to delete and then call RemoveTagged(), which removes every element whose tag is nonzero -- but neither of them clears the tags first. System::Solve() tags the constraints whose equations it couldn't satisfy, so that they can be listed for the user, and so the regeneration that immediately follows a failed solve deleted exactly those constraints: fail to solve a sketch once, and the constraints that failed are gone, along with any chance of correcting the value that caused it. Clear the tags before setting them. The request lists get the same treatment, not because anything tags a request today, but because they are pruned by the same tag-then-RemoveTagged() protocol, and would lose requests the same way if anything ever did. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@phkahler I think we should not merge this yet because it causes a memory leak But it is a serious bug - it boils down to fixing the problem that: when constraints fail to solve they are deleted. To reproduce the bug:
For cross reference BoykoNeov#2 BoykoNeov#4 |
Sorry, something went wrong.
Agreed. Also, this feels like a new bug to me. Did 3.0 or 3.1 delete the constraints too? Also want to merge #1688 soon. Any idea why it causes a leak? Clearing tags should not do that. |
Sorry, something went wrong.
Group::Clear() frees every other dynamically-allocated member of a group, but not solved.remove -- the list that System::Solve() fills with the constraints whose equations it couldn't satisfy, and that FindWhichToRemoveToFixJacobian() fills with the ones it suggests removing to fix a redundant system. SolveGroup() empties that list before every solve, so it never grows during a session, but whatever is in it when the sketch is closed, the group is deleted, or the process exits is leaked. Nothing aliases the list: the undo stack zeroes solved when it shallow copies a group, and SolveGroup() clears the list directly rather than through Group::Clear(), so the report shown in the text window is unaffected. The leak is as old as the list, but it needs a solve to fail before there is anything to leak, and until the preceding commit's test nothing in the suite made one fail -- so the sanitizer build had nothing to find. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Written by Claude Opus 5 — both this text and the code it describes; posted by @BoykoNeov. @ruevs @phkahler Thanks — the leak is real, and it is not caused by clearing the tags. @phkahler's instinct is right there. It is a pre-existing ownership bug that this branch's test is simply the first thing in the suite to reach. Pushed a second commit that fixes it. Where the 256 bytes come fromSystem::Solve() reports the constraints it could not satisfy by appending them to the list the caller passes as bad — which is g->solved.remove (generate.cpp:560), the list the text window shows under "the constraints to remove" (textscreens.cpp:670). Note that the failed-solve path is not the only one that fills it: a redundant system fills the same list from FindWhichToRemoveToFixJacobian() and then returns REDUNDANT_OKAY, so a sketch that solves fine but is over-constrained leaks it too. SolveGroup() empties that list before every solve (generate.cpp:558), so during a session it never grows. But Group::Clear() — the function whose comment says "The group structure includes pointers to other dynamically-allocated memory. This clears and frees them all." — frees every other dynamic member of a group and not this one. So whatever is in solved.remove when the sketch is closed, or the group is deleted, or the process exits, is leaked. 256 bytes is exactly the first List::ReserveMore() on an empty list: 64 slots × 4-byte hConstraint, one allocation, matching the ASan report. That it is pre-existing, measured rather than assumedI instrumented Group::Clear() to print when it is called with a non-empty solved.remove, and ran the new test both ways:
So master leaks the same allocation on any failed solve; the reason CI is green today is that no test in the suite made a solve fail before this one. (Without the fix the constraint is deleted, but that happens after bad->Add(), and the list still holds the stale handle.) The fixOne line in Group::Clear(): solved.remove.Clear();, next to the other members it already frees. Nothing aliases the list — the undo stack zeroes solved when it shallow-copies a group (undoredo.cpp:57), and SolveGroup() clears the list directly rather than through Group::Clear(), so the text-window report is unaffected. I could not add a regression test for it: MSVC has no LeakSanitizer, so there is nothing to assert against locally. The ASan build in CI is the guard — it caught this one, and it will catch a recurrence. > Did 3.0 or 3.1 delete the constraints too?No. Both are clean, and I checked the tags rather than guessing:
So, for constraints — the only half that matters here, since nothing tags a request — it is a regression after v3.1, first shipped in v3.2. (The request half of the fix is prophylactic: it is pruned by the same protocol and would lose requests the same way if anything ever did tag one.) |
Sorry, something went wrong.
|
Merged. Thank you @BoykoNeov |
Sorry, something went wrong.
Fixes a data-loss bug that is independent of any particular sketch: when a solve fails, SolveSpace deletes the constraints that failed.
I found this while working on #1247 — the constraint count dropped from 7 to 5 after a failed solve, and a later FindById on one of the missing handles asserted. It is not specific to that model or to that issue; any group that fails to solve loses the constraints the solver couldn't satisfy, and with them the value the user needs to edit to recover.
Root cause
PruneOrphans() and PruneRequestsAndConstraints() (src/generate.cpp) both follow the same protocol: walk the list, set tag = 1 on the elements to delete, then call RemoveTagged(), which removes every element whose tag is nonzero. Neither of them clears the tags first, so they delete their own selection plus whatever happened to be tagged already.
System::Solve() tags exactly the constraints whose equations it couldn't satisfy — that is its didnt_converge: path, where the tag is used to avoid listing the same constraint twice while building the report. A failed solve is immediately followed by a regeneration, the regeneration prunes, and the prune sees those leftover tags as delete-me marks.
So the constraints that fail are precisely the constraints that get destroyed.
Why the fix is safe
SK.request.ClearTags() / SK.constraint.ClearTags() before each tagging loop. Two things I checked rather than assumed:
The request lists get the same treatment. Nothing tags a request today, so that half is a no-op — but they are pruned by the identical tag-then-RemoveTagged() protocol and would lose requests the same way if anything ever did.
Regression test
test/core/prune/failed_solve_keeps_constraints — load a right triangle with an angle dimension, set that angle to 95°, which no triangle of this shape has (the direction cosine is 100/sqrt(100² + h²), positive for every h, and cos 95° is not), so the solver gives up and tags. The test then asserts the request and constraint counts are unchanged and that the angle constraint is still there with its value intact — a bare count would also pass if the wrong constraint vanished and another appeared.
Without the fix it fails on the count; with it, it passes.
Verification
Notes for review
🤖 Generated with Claude Code