AtlasEngine: retry the settings update when rebuilding the buffers throws

[danielw98]

Summary of the Pull Request

_handleSettingsUpdate() advances _p.s to the new settings before the _recreate*() calls rebuild the buffers those settings describe. The rebuilds allocate and can throw bad_alloc.

When that happens, StartPaint()'s _p.s != _api.s check is already satisfied, so the rebuild is never retried, as such, the engine keeps painting against buffers that never matched its settings, and a later, otherwise-healthy frame reads a row that was never rebuilt.

That is the AtlasEngine::PaintCursor access violation in #20269, and it takes every window in the process down with it.

The fix rolls _p.s back when a rebuild throws, so the next StartPaint() retries the whole update. A transient allocation failure can no longer corrupt the engine for the rest of its lifetime. Ten lines in one production file, plus a test.

References and Relevant Issues

Closes #20269 (my #20406 was closed as its duplicate).

This crash had two independent producers, and #20366 fixed the EnablePainting viewport desync, and the Canary drag-testing in the thread validates that path.

This PR removes the remaining one, which requires an allocation failure at the wrong moment, something drag-testing on a healthy machine can never probe.

@yuu61 identified this exact hole in the thread (the _handleSettingsUpdate exception-safety note) but never sent the rollback, and this PR is that fix, with a deterministic test :-).

Detailed Description of the Pull Request / Additional comments

_p.s must hold the new settings while the rebuilds run, because the _recreate*() helpers read it.

The delayed crash is the point of this bug: the allocation failure only seeds the desync, and the AV lands minutes or days later on a machine that is healthy by then - which matches the "random, can't repro" reports in the thread, including mine.

Validation Steps Performed

The new test (AtlasEngineTests in UnitTests_Control) reproduces the failure deterministically, with no test hooks in production code. It limits the test process's commit charge with a Job Object (current usage + 64 MiB), exhausts that headroom with bounded ballast, and resizes the engine to 426x113 - the rebuild genuinely cannot allocate and StartPaint() returns E_OUTOFMEMORY. The ballast is then released, and the next StartPaint() must recover on its own.

• Without the fix, that frame skips the rebuild behind the already-satisfied settings check and crashes with 0xC0000005, exactly the failure bucket in every report on AtlasEngine::PaintCursor crashes #20269. Reverting just the ten-line AtlasEngine.cpp hunk and rerunning the suite shows it.

• With the fix, the update is retried and the closing PaintCursor on the viewport's last row, which the exact read that crashes in the field - succeeds.

The job's limits are cleared before the test returns (the suite's other tests pass after it in the same host), and the test logs an explicit Skipped where a commit cap cannot be applied.

Local x64 Debug runs: Control 30/30 (the new test runs end to end, no skip), TerminalCore 54/54, clang-format 19.1.5 produces no diff.

PR Checklist

• Closes AtlasEngine::PaintCursor crashes #20269
• Tests added/passed
• Documentation updated (n/a - no user-facing behavior change)
• Schema updated (n/a)

[azure-pipelines]

Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

[danielw98]

@microsoft-github-policy-service agree

[lhecker]

You can achieve the same effect by calling _api.s.write() which advances the settings generation and causes a mismatch as well.

Additionally, it would be more idiomatic to write this in C++:

try {
    // ...
} catch(...) {
    // Retry the update on the next frame
    std::ignore = _api.s.write();
    throw;
}

[DHowett]

@lhecker you may need to read the discussion in the linked issue, if you can stomach it

[lhecker]

I skimmed it and it was impressively disappointing.

I figured that this PR is unlikely to fix the immediate issue. But I do believe it's a genuine issue still. Thinking about it some more, I believe the best approach would be to catch any and all exceptions and fully reset the engine. After all, similar exceptions can be raised in other functions and may cause all sorts of similar issues.

[danielw98]

Thanks @lhecker , you are right that this is a narrow fix for one throw site, and same with the idiomatic shape of the fix.

Digging further, the same shape (advance persistent state before a falliable operation, without rollback if it thows) also shows up in several other places in this same renderer pipeline.

One of them is a real heap-corruption-class OOB write, more than a theoretical concern.

Patching each site individually, as it turns up, is just treating symptoms, and I'm digging deeper to find the root cause.

I'd rather find a fix at the level of the underlying patttern than land another one-off patch.

I'll come back once I have something functional, tested, and reviewed.