Roll back config handler on persistence failure (#469.2)

[nitrobass24]

Summary

Addresses item #2 in #469.

`/server/config/set` previously mutated the in-memory `Config` before attempting `to_file`. If the write failed (disk full, permissions, …), the in-memory state ended up ahead of the on-disk state, and the LFTP hot-reload callback could fire on a value that never persisted — so after a restart the runtime would silently revert to the on-disk value, contradicting what the UI showed.

The handler now:

1. Captures the old native value via `getattr(inner_config, key)`.
2. Calls `inner_config.set_property(key, value)` (existing `ConfigError` → 400 preserved).
3. Attempts `self.__config.to_file(...)`.
4. On `OSError`: restores via `set_property(key, old_value)`, logs via `logger.exception`, returns a structured HTTP 500.
5. Only on write success: fires `__on_lftp_config_change` (when the key is in `_LFTP_TUNING_KEYS`).

Deviation from the issue's suggested approach

The original write-up suggested a copy-then-write-then-swap pattern. That doesn't actually work in this codebase because the `Config` instance is shared with the `Controller` via `Context` — swapping the handler's reference wouldn't propagate to the rest of the app, and the LFTP runtime would still read the old config. Capture-and-rollback is the equivalent semantically and works with the existing aliasing. Called out in the commit message.

Test plan

• `cd src/python && pytest tests/integration/test_web/test_handler/test_config.py` — 12 tests pass (3 new + 9 existing)
• New tests:
  • `test_set_persistence_failure_rolls_back` — patches `Config.to_file` to raise `OSError`, asserts 500, asserts old value restored, asserts LFTP callback not invoked
  • `test_set_persistence_failure_rolls_back_lftp_tuning_key` — same on an actual hot-reload key
  • `test_set_persistence_success_fires_lftp_callback` — positive baseline that successful writes still trigger the callback

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes
  • Configuration writes now handle disk write failures gracefully: on failure the in-memory setting is reverted to its prior value, a structured HTTP 500 error is returned, and automatic reload callbacks are only invoked after a successful persist.
• Tests
  • Added integration tests covering persistence failures, rollback behavior, callback suppression on failed writes, successful callback invocation, and concurrent-update edge cases.

[coderabbitai]

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 69d5c828-69c6-4301-9bee-fd0e1458afa3

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Refactors /server/config/set to record pre-mutation value, attempt disk persistence, roll back the in-memory value on Config.to_file OSError (unless a concurrent update replaced it), log the error, and run LFTP hot-reload only after a successful write. Tests and changelog updated.

Changes

Config set handler persistence with rollback

Layer / File(s)	Summary
Config set handler with error separation and rollback src/python/web/handler/config.py	Adds logging, a per-instance logger, and refactors __handle_set_config to record the old value, convert ConfigError to 400, persist via Config.to_file, on OSError conditionally rollback the in-memory property only if unchanged and log the exception, then trigger LFTP hot-reload only after successful persistence and return the appropriate response.
Integration tests and changelog documentation src/python/tests/integration/test_web/test_handler/test_config.py, CHANGELOG.md	Adds unittest.mock.patch import and four tests: failure rollback for normal keys, failure preventing LFTP callback for hot-reload keys, successful write firing LFTP callback once, and a concurrency case where rollback is skipped. Updates CHANGELOG with the fixed behavior.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related issues

• Hardening: error handling and persistence ordering in web handlers #469: Implements the hardening requested in the issue — record pre-mutation value, rollback on to_file OSError, and run LFTP hot-reload only after successful disk write.

Possibly related PRs

• nitrobass24/seedsync#433: Related changes to LFTP hot-reload plumbing; this PR adjusts when and how request_lftp_reconfigure is invoked and adds rollback on persistence failures.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and specifically describes the main change: rolling back the config handler when persistence fails, with reference to the related issue.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/config-handler-atomicity

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@CHANGELOG.md`:
- Around line 3-8: Add a new top-level versioned release entry in CHANGELOG.md
above the existing [Unreleased] section using semantic versioning (e.g., X.Y.Z)
and a YYYY-MM-DD date, and move the current Unreleased bullet ("Config handler
persistence atomicity — /server/config/set ...") into the Fixed section of that
new release; ensure the new release includes the standard sections: Changed,
Added, Fixed, Removed, Security (even if some are empty), and leave the existing
[Unreleased] header in place for future work.

In `@src/python/web/handler/config.py`:
- Around line 81-91: The rollback currently always restores old_value on
OSError, which can overwrite a newer concurrent update; change the except
OSError handler so it first reads the current value (e.g., current =
getattr(inner_config, key)) and only call inner_config.set_property(key,
old_value) if current == value (i.e., the in-memory value still equals the
failed-to-persist value); otherwise skip the rollback, log that a newer value is
present, and still return the 500 response. Reference inner_config.set_property,
getattr(inner_config, key), self.__config.to_file, self.__config_path and
self.__logger when making the change.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: b8f92149-8808-485a-88c8-c3a123b352f3

📥 Commits

Reviewing files that changed from the base of the PR and between eedf863 and 0f7a10a.

📒 Files selected for processing (3)

• CHANGELOG.md
• src/python/tests/integration/test_web/test_handler/test_config.py
• src/python/web/handler/config.py

[coderabbitai]

♻️ Duplicate comments (1)

src/python/web/handler/config.py (1)

81-105: ⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy lift

Serialize all config writes; this key-local rollback is still unsafe.

Because self.__config.to_file(...) persists the entire shared config, the guard at Lines 95-97 only covers “the same key changed.” A second request can successfully persist some other key while this request’s value is still present in memory, which also writes this request’s set_native value to disk; if this except block then restores old_value, memory falls behind disk again. There is also still a read-check-write race between Lines 95 and 97. This flow needs a lock shared by every writer of the same Config around mutate → to_file → rollback/callback, not another point-in-time equality check.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/python/web/handler/config.py` around lines 81 - 105, The current rollback
is unsafe because self.__config.to_file persists the whole shared config and the
point-in-time equality check can miss concurrent writer races; fix by
serializing writers with a shared lock around the entire mutate→persist→rollback
sequence: acquire a per-Config writer lock before calling
inner_config.set_property(key, value), keep the lock while calling
self.__config.to_file(self.__config_path), and only perform the rollback
(inner_config.set_property(key, old_value)) while still holding that same lock;
add or reuse a lock attribute (e.g., self.__config_lock or attach a lock to
self.__config) and use it in the try/except that currently surrounds
set_property and to_file, and log via self.__logger as before.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Duplicate comments:
In `@src/python/web/handler/config.py`:
- Around line 81-105: The current rollback is unsafe because
self.__config.to_file persists the whole shared config and the point-in-time
equality check can miss concurrent writer races; fix by serializing writers with
a shared lock around the entire mutate→persist→rollback sequence: acquire a
per-Config writer lock before calling inner_config.set_property(key, value),
keep the lock while calling self.__config.to_file(self.__config_path), and only
perform the rollback (inner_config.set_property(key, old_value)) while still
holding that same lock; add or reuse a lock attribute (e.g., self.__config_lock
or attach a lock to self.__config) and use it in the try/except that currently
surrounds set_property and to_file, and log via self.__logger as before.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 4634718e-c3fa-4c1b-8d11-b4cafcdb0f4d

📥 Commits

Reviewing files that changed from the base of the PR and between 0f7a10a and efd0876.

📒 Files selected for processing (2)

• src/python/tests/integration/test_web/test_handler/test_config.py
• src/python/web/handler/config.py