settings.json is overwritten wholesale on every launch, discarding unmodelled fields, with no write lock

[bobtista]

Problem

Two independent defects in the same write path.

No read-merge. GameLauncher.cs:1422 builds a fresh GeneralsOnlineSettings;
GameSettingsService.cs:258-259 serializes it and calls File.WriteAllTextAsync.
LoadGeneralsOnlineSettingsAsync (:208-234) exists but the launch path never calls it, and
neither model declares [JsonExtensionData]. Any key the user set inside GeneralsOnline that
GenHub does not model is discarded on every Zero Hour launch.

No lock. The semaphore guarding LoadOptionsAsync/SaveOptionsAsync
(GameSettingsService.cs:64-145) does not cover the GeneralsOnline load/save at :208-269.
Concurrent launches race on a non-atomic WriteAllTextAsync.

The same class of loss exists on the INI side: ApplyToOptions builds a new dictionary at
GameSettingsMapper.cs:455 and replaces AdditionalSections["TheSuperHackers"] at :473,
dropping unknown keys — which contradicts the preservation behaviour documented in
docs/dev/game-settings-architecture.md:13-34.

Separately, GameSettingsViewModel.CreateGeneralsOnlineSettings (:1163-1194) populates only
the GeneralsOnline and nested properties, so a manual save writes defaults over the user's
inherited TheSuperHackers choices.

Approach

• Load-merge-write for settings.json, or add [JsonExtensionData] so unknown keys survive.
• Extend the existing semaphore to the GeneralsOnline path, and write via temp-file replace.
• Decide what profile isolation should mean for keys GenHub has no opinion on. The current
  answer is "discard them", which reads as accidental rather than chosen.
• Same treatment for the TheSuperHackers INI section, or correct the doc to match the code.

[coderabbitai]

🔗 Related PRs

#288 - feat: Add GenLauncher file normalization support [merged]
#332 - feat(launching): launch the native BGFX client against retail archives [merged]
#343 - fix(generalsonline): launch and track through the Easy Anti-Cheat bootstrapper [open]
#349 - fix(cas): fall back from unwritable installation pools [merged]
#352 - feat(installations): detect retail installations by archive presence [open]

---

📝 Issue Planner

_{Check the box below or use the @coderabbitai plan command to generate an implementation plan and prompts that you can use with your favorite coding assistant.}

• Create Plan

---

🧪 Issue enrichment is currently in open beta.

To disable automatic issue enrichment, add the following to your .coderabbit.yaml:

issue_enrichment:
  auto_enrich:
    enabled: false

💬 Have feedback or questions? Drop into our discord!

[coderabbitai]

Coding Plan

Summary

• Preserve unknown keys in settings.json with a read-merge-write step plus a [JsonExtensionData] bag, and write atomically with a temp file and File.Move.
• Add a dedicated static SemaphoreSlim for the GeneralsOnline path and guard both load and save.
• Fix the TheSuperHackers INI section by merging instead of replacing, close the load-modify-write race with a single lock around the sequence, and replace the magic string with a constant.
• Correct docs/features/game-settings/index.md to match the actual preservation and persistence behavior. Exclude launch-time and ViewModel wiring, because that code does not exist and would be a separate feature.

Design Choices

Design Choice 1: How should unknown keys in settings.json be preserved?

Options Considered:

1. Add [JsonExtensionData] to the model only. Simple, but only preserves unknown keys when the same loaded object is saved back.
2. Read-merge-write in SaveGeneralsOnlineSettingsAsync. Read the existing file into a JsonNode, overlay the modeled properties from the incoming object, then write. Preserves unknown keys regardless of how the caller built the object.
3. Combine both: read-merge-write in the save path, plus [JsonExtensionData] for load-side robustness.

Chosen Option: 3

Rationale: Read-merge-write guarantees preservation independent of the caller, and [JsonExtensionData] makes load and round-trip robust with minimal cost. The save method receives a GeneralsOnlineSettings object that a caller may build fresh. If preservation relies only on [JsonExtensionData], a fresh object with an empty extension bag would still discard on-disk unknown keys.

Design Choice 2: What should profile isolation mean for keys GenHub does not model?

Options Considered:

1. Discard unmodeled keys (current behavior).
2. Preserve unmodeled keys verbatim across load and save.

Chosen Option: 2

Rationale: Preserve unmodeled keys. This matches the documented preservation guarantee and the existing AdditionalSections/AdditionalProperties idiom used for Options.ini. The ticket asks for an explicit decision instead of accidental discard.

💡 User Tips

Regenerate the plan with different choices with @coderabbitai <feedback>.

Implementation Steps

Phase 1: Preserve unknown keys and add concurrency safety to the GeneralsOnline settings.json path

This phase makes settings.json persistence safe. It adds a dedicated write lock, an atomic write, a read-merge step, and unknown-key preservation. All changes are in GenHub/GenHub/Features/GameSettings/GameSettingsService.cs and the two model classes.

Task 1: Add a dedicated write lock for the GeneralsOnline path

Serialize concurrent reads and writes of the single shared settings.json file.

• Add a private static readonly SemaphoreSlim field in GameSettingsService.cs, separate from _optionsIniWriteSemaphore, dedicated to the settings.json path.
• Acquire the new semaphore in SaveGeneralsOnlineSettingsAsync around the full read-merge-write sequence. Release it in a finally block. Follow the await WaitAsync(); try { ... } finally { Release(); } template used by SaveOptionsAsync.
• Acquire the same semaphore in LoadGeneralsOnlineSettingsAsync around the file read, so a read cannot observe a partial write. Release it in a finally block.
• Keep the existing BeginScope, try/catch, OperationResult<T> return, and log-level conventions unchanged.

Task 2: Read-merge-write and atomic write in SaveGeneralsOnlineSettingsAsync

Preserve on-disk keys that GenHub does not model, and prevent file corruption on crash.

• Before writing, read the existing settings.json if it exists. Parse it into a JsonNode (or JsonObject).
• Overlay the modeled properties from the incoming GeneralsOnlineSettings object onto the parsed node. Keep any keys present on disk that the model does not define.
• If the file does not exist, serialize the incoming object as the base content.
• Write the merged JSON to a temporary file in the target directory. Then move it over the final path with an atomic replace. Use the temp-file-then-File.Move pattern from CasStorage.StoreObjectAsync as the reference.
• Continue to create the target directory if it does not exist, as the current method does.
• Continue to use _jsonSerializerOptions for serialization.

Task 3: Add [JsonExtensionData] and align deserialization options

Make load and round-trip preserve unknown keys, and remove the load/save options mismatch.

• Add a [JsonExtensionData] dictionary property (for example Dictionary<string, JsonElement>) to TheSuperHackersSettings in GenHub.Core/Models/GameSettings/TheSuperHackersSettings.cs. Because GeneralsOnlineSettings inherits TheSuperHackersSettings, one extension-data property covers both models.
• Pass _jsonSerializerOptions to JsonSerializer.Deserialize<GeneralsOnlineSettings> in LoadGeneralsOnlineSettingsAsync, so load and save use the same options.
• Keep the "file missing returns defaults" and "null deserialize returns defaults" behavior unchanged.

🤖 Prompt for AI agents

Implement safe persistence for the GeneralsOnline `settings.json` file in
`GenHub/GenHub/Features/GameSettings/GameSettingsService.cs` and the related
model classes. The goal is to eliminate the blind-overwrite, lock-free,
non-atomic write path.

- Add a `private static readonly SemaphoreSlim` field dedicated to the
`settings.json` path, separate from `_optionsIniWriteSemaphore`.
- In `SaveGeneralsOnlineSettingsAsync`, acquire this new semaphore with `await
WaitAsync()` around the full read-merge-write sequence, and release it in a
`finally` block, following the same template used by `SaveOptionsAsync`.
- In `LoadGeneralsOnlineSettingsAsync`, acquire the same semaphore around the
file read and release it in a `finally` block, so a read cannot observe a
partial write.
- Preserve the existing `BeginScope`, `try/catch`, `OperationResult<T>` return,
and log-level conventions unchanged.
- In `SaveGeneralsOnlineSettingsAsync`, before writing, read the existing
`settings.json` file if it exists and parse it into a `JsonNode` (or
`JsonObject`). Overlay the modeled properties from the incoming
`GeneralsOnlineSettings` object onto the parsed node, keeping any on-disk keys
the model does not define. If the file does not exist, serialize the incoming
object as the base content.
- Write the merged JSON to a temporary file in the target directory, then move
it over the final path with an atomic replace, using the
temp-file-then-`File.Move` pattern from `CasStorage.StoreObjectAsync` as
reference. Keep creating the target directory if missing, and keep using
`_jsonSerializerOptions` for serialization.
- Add a `[JsonExtensionData]` dictionary property (for example
`Dictionary<string, JsonElement>`) to `TheSuperHackersSettings` in
`GenHub.Core/Models/GameSettings/TheSuperHackersSettings.cs`. Since
`GeneralsOnlineSettings` inherits `TheSuperHackersSettings`, this single
property covers both models.
- Pass `_jsonSerializerOptions` to
`JsonSerializer.Deserialize<GeneralsOnlineSettings>` in
`LoadGeneralsOnlineSettingsAsync` so load and save use identical options.
- Keep existing behavior: missing file returns defaults, and a null deserialize
result returns defaults.

Phase 2: Preserve unknown keys in the TheSuperHackers INI section and close the TOCTOU gap

This phase fixes the equivalent data-loss defect on the INI side and makes the read-modify-write sequence atomic. All changes are in GameSettingsService.cs, plus one constants file.

Task 1: Merge instead of replace the TheSuperHackers section

Stop dropping recognized-but-unmodeled keys on save.

• In SaveTheSuperHackersSettingsAsync, do not assign options.AdditionalSections["TheSuperHackers"] to a brand-new dictionary.
• If an existing TheSuperHackers dictionary is present in options.AdditionalSections, merge the modeled keys produced by SerializeTheSuperHackersSettings into that existing dictionary. Preserve all other existing keys.
• If no existing section is present, use the serialized modeled keys as the section content.

Task 2: Close the load-modify-write race in SaveTheSuperHackersSettingsAsync

Make the read, merge, and write sequence atomic against concurrent Options.ini writers.

• Introduce internal helper methods that perform the Options.ini read and write without acquiring _optionsIniWriteSemaphore. The existing public LoadOptionsAsync/SaveOptionsAsync should delegate to these helpers.
• In SaveTheSuperHackersSettingsAsync, acquire _optionsIniWriteSemaphore once, then call the unguarded load helper, merge (Task 2.1), and call the unguarded write helper, all inside the same lock. Release the semaphore in a finally block.
• Do not acquire the semaphore twice in one call path, because SemaphoreSlim is not reentrant.
• Preserve the existing error-propagation convention that returns OperationResult<T>.CreateFailure(result.Errors) for nested failures.

Task 3: Promote the TheSuperHackers section name to a constant

Remove the repeated magic string.

• Add a SectionName = "TheSuperHackers" constant to GenHub.Core/Constants/GameSettingsTheSuperHackersConstants.cs.
• Replace the hard-coded "TheSuperHackers" string literals in GameSettingsService.cs (read, write, parse, and serialize sites) with the new constant.

🤖 Prompt for AI agents

Fix the data-loss and race-condition defects in the `TheSuperHackers` INI
section handling in `GameSettingsService.cs`. The goal is to stop dropping
recognized-but-unmodeled keys on save and to close the load-modify-write race.

- In `SaveTheSuperHackersSettingsAsync`, stop assigning
`options.AdditionalSections["TheSuperHackers"]` to a brand-new dictionary.
Instead, if an existing `TheSuperHackers` dictionary is present in
`options.AdditionalSections`, merge the modeled keys produced by
`SerializeTheSuperHackersSettings` into that existing dictionary, preserving all
other existing keys. If no existing section is present, use the serialized
modeled keys as the section content.
- Introduce internal helper methods that read and write `Options.ini` without
acquiring `_optionsIniWriteSemaphore`. Update the existing public
`LoadOptionsAsync`/`SaveOptionsAsync` to delegate to these helpers.
- In `SaveTheSuperHackersSettingsAsync`, acquire `_optionsIniWriteSemaphore`
exactly once for the whole sequence, then call the unguarded load helper, merge
the section as described above, and call the unguarded write helper, all inside
the same lock. Release the semaphore in a `finally` block. Do not acquire the
semaphore twice in one call path, because `SemaphoreSlim` is not reentrant.
- Preserve the existing error-propagation convention that returns
`OperationResult<T>.CreateFailure(result.Errors)` for nested failures.
- Add a `SectionName = "TheSuperHackers"` constant to
`GenHub.Core/Constants/GameSettingsTheSuperHackersConstants.cs`, and replace all
hard-coded `"TheSuperHackers"` string literals in `GameSettingsService.cs`
(read, write, parse, and serialize sites) with this constant.

Phase 3: Correct the documentation

This phase aligns the documentation with the actual code behavior.

Document AdditionalSections, the TheSuperHackers section, and the settings.json flow

Correct the preservation claims and cover the previously undocumented paths.

• Update docs/features/game-settings/index.md to describe the top-level AdditionalSections mechanism, in addition to the per-section AdditionalProperties it already mentions.
• Describe how the TheSuperHackers INI section is preserved on save through merge, not replace.
• Add a short section that describes the GeneralsOnline settings.json persistence flow, including unknown-key preservation, the dedicated write lock, and the atomic write.
• Do not reference the non-existent docs/dev/game-settings-architecture.md path. Keep the correction inside the existing document.

🤖 Prompt for AI agents

Update `docs/features/game-settings/index.md` to match the actual, corrected
code behavior from Phase 1 and Phase 2.

- Describe the top-level `AdditionalSections` mechanism, in addition to the
per-section `AdditionalProperties` mechanism the document already mentions.
- Describe how the `TheSuperHackers` INI section is preserved on save through
merge, not replace.
- Add a short section describing the GeneralsOnline `settings.json` persistence
flow, including unknown-key preservation, the dedicated write lock, and the
atomic write.
- Do not reference the non-existent `docs/dev/game-settings-architecture.md`
path. Keep all corrections inside the existing
`docs/features/game-settings/index.md` document.

Research

The game settings feature is located in GenHub/GenHub/Features/GameSettings/GameSettingsService.cs, with models under GenHub.Core/Models/GameSettings/ and the contract in GenHub.Core/Interfaces/GameSettings/IGameSettingsService.cs.

Two persistence paths exist:

• The Options.ini path (LoadOptionsAsync/SaveOptionsAsync). SaveOptionsAsync is guarded by a static SemaphoreSlim _optionsIniWriteSemaphore. Unknown INI content is preserved through the AdditionalSections dictionary on IniOptions and the per-section AdditionalProperties dictionaries.
• The GeneralsOnline settings.json path (LoadGeneralsOnlineSettingsAsync/SaveGeneralsOnlineSettingsAsync). This path has no lock, no read-merge, no atomic write, and no unknown-key preservation. SaveGeneralsOnlineSettingsAsync performs a blind overwrite.

The TheSuperHackers INI section is handled by SaveTheSuperHackersSettingsAsync. It reads Options.ini, then does options.AdditionalSections["TheSuperHackers"] = SerializeTheSuperHackersSettings(settings). SerializeTheSuperHackersSettings emits only 14 modeled keys. The parser recognizes about 30 keys. So about 16 recognized-but-unmodeled keys are dropped on save. The read step is unguarded and the write step is guarded, so a TOCTOU gap exists.

Important discrepancies found during exploration:

• The ticket cites GameLauncher.cs:1422, GameSettingsMapper.cs:455-473, GameSettingsViewModel.CreateGeneralsOnlineSettings, and docs/dev/game-settings-architecture.md. None of these exist. GameLauncher.cs has zero references to GeneralsOnlineSettings. GameSettingsMapper.cs (247 lines) has no AdditionalSections logic. No file constructs a GeneralsOnlineSettings from a GameProfile and calls SaveGeneralsOnlineSettingsAsync; that API is currently unused. The real preservation bug is in GameSettingsService.SaveTheSuperHackersSettingsAsync. The real documentation file is docs/features/game-settings/index.md.
• No [JsonExtensionData] usage exists anywhere in the repository. A temp-file-then-File.Move atomic write pattern exists in GenHub/GenHub/Features/Storage/Services/CasStorage.cs and can serve as a reference.

This plan fixes the actual defects in code and corrects the documentation. It does not add new launch-time or ViewModel wiring, because that code does not exist and adding it is a separate feature.

---

🚀 Next Steps

🤖 All AI agent prompts combined

Task: 1

Implement safe persistence for the GeneralsOnline `settings.json` file in
`GenHub/GenHub/Features/GameSettings/GameSettingsService.cs` and the related
model classes. The goal is to eliminate the blind-overwrite, lock-free,
non-atomic write path.

- Add a `private static readonly SemaphoreSlim` field dedicated to the
`settings.json` path, separate from `_optionsIniWriteSemaphore`.
- In `SaveGeneralsOnlineSettingsAsync`, acquire this new semaphore with `await
WaitAsync()` around the full read-merge-write sequence, and release it in a
`finally` block, following the same template used by `SaveOptionsAsync`.
- In `LoadGeneralsOnlineSettingsAsync`, acquire the same semaphore around the
file read and release it in a `finally` block, so a read cannot observe a
partial write.
- Preserve the existing `BeginScope`, `try/catch`, `OperationResult<T>` return,
and log-level conventions unchanged.
- In `SaveGeneralsOnlineSettingsAsync`, before writing, read the existing
`settings.json` file if it exists and parse it into a `JsonNode` (or
`JsonObject`). Overlay the modeled properties from the incoming
`GeneralsOnlineSettings` object onto the parsed node, keeping any on-disk keys
the model does not define. If the file does not exist, serialize the incoming
object as the base content.
- Write the merged JSON to a temporary file in the target directory, then move
it over the final path with an atomic replace, using the
temp-file-then-`File.Move` pattern from `CasStorage.StoreObjectAsync` as
reference. Keep creating the target directory if missing, and keep using
`_jsonSerializerOptions` for serialization.
- Add a `[JsonExtensionData]` dictionary property (for example
`Dictionary<string, JsonElement>`) to `TheSuperHackersSettings` in
`GenHub.Core/Models/GameSettings/TheSuperHackersSettings.cs`. Since
`GeneralsOnlineSettings` inherits `TheSuperHackersSettings`, this single
property covers both models.
- Pass `_jsonSerializerOptions` to
`JsonSerializer.Deserialize<GeneralsOnlineSettings>` in
`LoadGeneralsOnlineSettingsAsync` so load and save use identical options.
- Keep existing behavior: missing file returns defaults, and a null deserialize
result returns defaults.
===============================================================================

Task: 2

Fix the data-loss and race-condition defects in the `TheSuperHackers` INI
section handling in `GameSettingsService.cs`. The goal is to stop dropping
recognized-but-unmodeled keys on save and to close the load-modify-write race.

- In `SaveTheSuperHackersSettingsAsync`, stop assigning
`options.AdditionalSections["TheSuperHackers"]` to a brand-new dictionary.
Instead, if an existing `TheSuperHackers` dictionary is present in
`options.AdditionalSections`, merge the modeled keys produced by
`SerializeTheSuperHackersSettings` into that existing dictionary, preserving all
other existing keys. If no existing section is present, use the serialized
modeled keys as the section content.
- Introduce internal helper methods that read and write `Options.ini` without
acquiring `_optionsIniWriteSemaphore`. Update the existing public
`LoadOptionsAsync`/`SaveOptionsAsync` to delegate to these helpers.
- In `SaveTheSuperHackersSettingsAsync`, acquire `_optionsIniWriteSemaphore`
exactly once for the whole sequence, then call the unguarded load helper, merge
the section as described above, and call the unguarded write helper, all inside
the same lock. Release the semaphore in a `finally` block. Do not acquire the
semaphore twice in one call path, because `SemaphoreSlim` is not reentrant.
- Preserve the existing error-propagation convention that returns
`OperationResult<T>.CreateFailure(result.Errors)` for nested failures.
- Add a `SectionName = "TheSuperHackers"` constant to
`GenHub.Core/Constants/GameSettingsTheSuperHackersConstants.cs`, and replace all
hard-coded `"TheSuperHackers"` string literals in `GameSettingsService.cs`
(read, write, parse, and serialize sites) with this constant.
===============================================================================

Task: 3

Update `docs/features/game-settings/index.md` to match the actual, corrected
code behavior from Phase 1 and Phase 2.

- Describe the top-level `AdditionalSections` mechanism, in addition to the
per-section `AdditionalProperties` mechanism the document already mentions.
- Describe how the `TheSuperHackers` INI section is preserved on save through
merge, not replace.
- Add a short section describing the GeneralsOnline `settings.json` persistence
flow, including unknown-key preservation, the dedicated write lock, and the
atomic write.
- Do not reference the non-existent `docs/dev/game-settings-architecture.md`
path. Keep all corrections inside the existing
`docs/features/game-settings/index.md` document.

💡 Iterate on the plan with: `@coderabbitai` <feedback>

Example Feedback
- `@coderabbitai` You can skip phase 3. Add a simple unit test case for phase 2.
- `@coderabbitai` For design choice 1 go ahead with option 3 and replan.

---

💬 Have feedback or questions? Drop into our discord!