Skip to content

fix(app): settings saves from the UI are written by a worker, coalesced, and flushed at exit (#2916) - #2930

Merged
mgradwohl merged 5 commits into
mainfrom
fix/2916-settings-save-worker
Oct 25, 2026
Merged

mgradwohl merged 5 commits into
mainfrom
fix/2916-settings-save-worker

Conversation

@mgradwohl

@mgradwohl mgradwohl commented Oct 25, 2026 •

Copy link
Copy Markdown
Owner

Description

Clicking a setting no longer writes config.toml on the UI thread (AGENTS.md: "the UI thread ... never ... does file I/O"; this was point 4b of the #2731 audit).

  • App/CoalescingWriter.h (new, header-only): fire-and-forget writes of a snapshot. It reuses the ts-battery-read pattern: submit() starts a one-shot std::jthread when none is running, and that thread writes until nothing is pending, then ends, so no thread sits idle between saves. At most one write is in flight. A snapshot submitted while one is being written replaces any earlier snapshot not yet written, so rapid clicks cost one more write, holding the last state. A failed write (one that returns false or throws) is reported once per run of failures, and reported again only after a write has succeeded. flush() and the destructor write anything still pending before they return. If no thread can be started (std::system_error), the snapshot stays pending and the next submit() or flush() writes it.
  • UserConfig::saveInBackground() (new): copies the settings and submits the copy to the ts-config-save writer (new Platform::SETTINGS_SAVE_THREAD_NAME). The worker logs Saved config to <path> on ts-config-save. A run of failures logs one Settings weren't saved ... warning, and each failure's own reason is still logged.
  • UserConfig::save() stays synchronous but flushes the writer first. That way a background write still in flight can't rename an older snapshot over the final settings, so the last state always wins and nothing is lost. load() and resetConfigPathForTesting() flush it too. The file write and its merge base (m_Synced) are now serialised by m_WriteMutex, and m_IsLoaded is atomic because a background write resets it.

UI-thread call sites of save(), and what each one does now:

Call site Now
SystemMetricsPanel CPU Details toggle (the issue's case) saveInBackground()
SettingsLayer::applySettings() (Settings' Save) saveInBackground()
ElevationNoticeLayer "don't show again" saveInBackground()
DumpDetail::rememberDumpDirectory() (a chosen dump folder) saveInBackground()
DemoLayer theme= step now saves, with saveInBackground(). Before, it saved only at exit, despite the comment "As Settings' Apply: saved with the settings"
ShellLayer::onDetach() (exit) synchronous save(), unchanged. Nothing draws after it, and the flush inside it runs first
Settings' Edit config file on a first run kept synchronous, with a comment: the file must exist before the editor opens it, and this happens at most once

Sampling is untouched.

Closes #2916

PR Size

Medium: 654 lines added, 28 removed. Most of that is new and well commented: CoalescingWriter.h is 186 lines and its tests are 270. The changes to existing code come to about 130 lines, most of them in UserConfig.cpp and UserConfig.h.

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update
  • Build/CI improvement

Checklist

  • I have read the CONTRIBUTING guidelines
  • My code follows the project's coding standards
  • I have run clang-tidy and addressed any warnings
  • I have run clang-format on my changes
  • I have run pre-commit run --all-files or installed pre-commit hooks
  • I have added tests that prove my fix/feature works
  • All new and existing tests pass
  • I have updated documentation as needed

Testing

cmake --build --preset win-debug -j 2
ctest --preset win-debug -j 2          # 685/685 passed
.\tools\clang-tidy.ps1 -Files <the 12 touched TUs>
pre-commit run --files <touched>
  • New tests/App/test_CoalescingWriter.cpp (7 tests, fake sink):
    • the write runs on the worker, not the caller;
    • two quick saves while a write is in flight produce one more write, holding the last snapshot;
    • a run of failures is reported once, and again after a success;
    • a sink that throws counts as a failure;
    • flush() and destruction write a pending snapshot.
  • New UserConfigLoadSaveTest cases:
    • a background save is written and loads back;
    • load() waits for a background save;
    • save() after a background save writes the last state;
    • a run of failed background saves logs one warning.
  • test_ProcessDumpView and test_SettingsLayerRender flush before checking the file on disk.
  • clang-tidy (host LLVM 23) reports nothing on new or changed lines. Everything it reports is on existing lines, from checks that are 23-only or that CI's LLVM 22 passes today: readability-trailing-comma, readability-redundant-nested-if, bugprone-signed-bitwise, and the like.
  • One launch with a scratch config:
    • Environment: TASKSMACK_CONFIG_DIR=<scratch>, TASKSMACK_DEMO="2:theme=dracula", TASKSMACK_SCREENSHOT=<png>;after=5.
    • The log shows Theme changed to dracula, then Saved config to ...\config.toml on ts-config-save 2 ms later, then the exit path's synchronous Saved config to ...\config.toml.
    • The scratch config.toml holds [theme] id = 'dracula', and the screenshot renders in Dracula.

Additional Notes

  • About the exit-path ordering: the flush has to come before the final synchronous write, not after it. If the worker were joined afterwards, an older snapshot still in flight could rename itself over the final file. save() does the flush itself, so every synchronous caller gets the same guarantee.
  • .github/copilot-instructions.md's list of thread names now includes ts-config-save.

🤖 Generated with Claude Code

…ed, and flushed at exit (#2916)

UserConfig::saveInBackground() hands a copy of the settings to a CoalescingWriter, a one-shot
ts-config-save std::jthread (the ts-battery-read pattern): at most one write in flight, a newer
snapshot replaces one not written yet, and a run of failed writes is logged once. The CPU Details
toggle, Settings' Save, the privilege notice's "don't show again", the remembered dump folder and
TASKSMACK_DEMO's theme= step now save through it, so the UI thread never writes config.toml.

save() stays synchronous for the exit path (ShellLayer::onDetach) and Settings' first-run Edit
config file, and first flushes the writer, so a background write can never land over the final
settings. load() and resetConfigPathForTesting() flush it too; the file write and its merge base
(m_Synced) are serialised by a mutex.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@mgradwohl mgradwohl added this to the v0.10.0 milestone Oct 25, 2026
@mgradwohl mgradwohl added bug Something isn't working performance ui UI / rendering issues agent:Windows Owned by the Windows Claude session (bug-fix conveyor) labels Oct 25, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

Scanned Files

None

@github-actions github-actions Bot added documentation Improvements or additions to documentation testing Platform labels Oct 25, 2026
@mgradwohl
mgradwohl enabled auto-merge (squash) October 25, 2026 13:14
mgradwohl and others added 2 commits October 25, 2026 08:38
…he save worker's thread name (#2916)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…e-worker

# Conflicts:
#	src/App/ElevationNoticeLayer.cpp
@mgradwohl

Copy link
Copy Markdown
Owner Author

Merged main to clear the conflict in ElevationNoticeLayer.cpp: main moved the suppress step into suppressFutureNotices(), so that helper now calls saveInBackground(). Also fixed the red Fuzz changed parsers job: the user-config fuzzer links UserConfig.cpp, whose save worker now calls Platform::setCurrentThreadName(), so .clusterfuzzlite/build.sh adds src/Platform/ThreadName.cpp to that fuzzer.

mgradwohl and others added 2 commits October 25, 2026 09:28
… throw, as the other noexcept loggers do (#2916)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent:Windows Owned by the Windows Claude session (bug-fix conveyor) bug Something isn't working documentation Improvements or additions to documentation performance Platform testing ui UI / rendering issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Perf] UserConfig::save() on a Settings click runs on the UI thread (#2731 leftover)

1 participant