fix: make configuration writes strict and atomic#367
Conversation
|
Warning Review limit reached
Next review available in: 53 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughConfiguration writes now use prepared, validated payloads and conflict-aware storage updates. Local file storage adds non-blocking locking and canonical payload persistence. API routes, library handling, tests, and documentation cover partial writes, no-op detection, storage preservation, and HTTP 409 conflicts. ChangesConfiguration write flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ConfigRoutes
participant Configurator
participant ConfigMixin
participant ConfigStorage
Client->>ConfigRoutes: submit configuration write
ConfigRoutes->>Configurator: prepare_config_write(payload, partial)
ConfigRoutes->>ConfigMixin: set_config(prepared)
ConfigMixin->>ConfigStorage: commit_config_write(prepared)
ConfigStorage-->>ConfigMixin: changed or unchanged
ConfigMixin-->>Client: configuration response
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@reflexio/server/services/configurator/local_file_config_storage.py`:
- Around line 81-84: Update the missing-file branch in the configuration read
flow to tolerate a ConfigWriteConflict from save_config() when another process
initializes the file concurrently. Catch the conflict and read and return the
existing winner’s configuration, while preserving the current default-creation
path when no competing writer intervenes.
- Around line 113-116: Update the schema-cleanup branch around save_config so
normalization uses update_config_payload with a transform callback rather than
replacing the previously read payload via save_config. The transform must
re-read the current payload under the update lock, apply the same config
normalization, and preserve newer concurrent writes; retain the existing OSError
and ConfigWriteConflict handling.
- Around line 160-168: Update the transformed-payload flow in the local-file
storage override before _save_payload_to_local_dir: validate updated through the
same Config payload validation and canonicalization used by the base
implementation, then compare and persist the canonical result. Ensure malformed,
unknown, invalid, or non-JSON-compatible transform output is rejected rather
than written, while preserving the unchanged-payload result behavior.
In `@tests/server/services/configurator/test_config_storage_contract.py`:
- Around line 218-234: Strengthen
test_schema_cleanup_conflict_returns_valid_stored_config by assigning a
non-default value to a persisted field such as agent_context_prompt before
writing the payload. Assert the loaded configuration preserves that custom
value, while retaining the existing retired_field assertion to verify cleanup
was not performed during the lock conflict.
🪄 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: CHILL
Plan: Pro Plus
Run ID: 52e95da3-73a2-40aa-9392-29af66296e70
📒 Files selected for processing (9)
reflexio/lib/_config.pyreflexio/server/README.mdreflexio/server/routes/config.pyreflexio/server/services/configurator/base_configurator.pyreflexio/server/services/configurator/config_storage.pyreflexio/server/services/configurator/local_file_config_storage.pytests/lib/test_config_unit.pytests/server/api_endpoints/test_api_routes.pytests/server/services/configurator/test_config_storage_contract.py
- serve reads instead of raising ConfigWriteConflict when the config lock is held during a first load; write paths still fail fast - share CONFIG_UNCHANGED_MESSAGE between ConfigMixin and the config routes instead of string-matching the message inline - drop the unused PreparedConfigWrite.changed field (storage owns the authoritative changed result)
9394b06 to
fb9d0ad
Compare
The cleanup rewrite previously persisted the pre-lock snapshot, so a writer completing between the read and the lock could be silently overwritten. Re-run the normalization inside update_config_payload against the payload re-read under the lock instead, and pin the race plus user-value survival with contract tests.
There was a problem hiding this comment.
♻️ Duplicate comments (3)
reflexio/server/services/configurator/local_file_config_storage.py (2)
172-178: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winValidate and canonicalize transformed payloads before persistence.
Unlike the base implementation, this override writes the arbitrary transform output directly. A malformed transform can therefore persist unknown fields, invalid values, or non-JSON-compatible objects despite the storage contract requiring canonical
Configpayloads.🛡️ Proposed fix
- if updated == current: + config = Config.model_validate(updated) + canonical = config.model_dump(mode="json") + if canonical == current: return ConfigPayloadUpdateResult( payload=current, changed=False, ) - self._save_payload_to_local_dir(updated) - return ConfigPayloadUpdateResult(payload=updated, changed=True) + self._save_payload_to_local_dir(canonical) + return ConfigPayloadUpdateResult(payload=canonical, changed=True)🤖 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 `@reflexio/server/services/configurator/local_file_config_storage.py` around lines 172 - 178, Update the local storage update flow around _save_payload_to_local_dir so transformed payloads are first validated and canonicalized using the same Config payload contract as the base implementation. Compare the canonical result with current, persist only that canonical payload, and return it in ConfigPayloadUpdateResult while preserving the unchanged fast path.
124-127: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPreserve newer concurrent writes during schema cleanup.
The current implementation replaces the entire file via
save_config(config)using the payload read before locking. A writer completing between the read and lock acquisition will have its newer configuration silently overwritten.Perform normalization as an
update_config_payloadtransform so it re-reads and cleans the latest payload while holding the lock.♻️ Proposed fix to preserve concurrent writes
if config.model_dump(mode="json") != original_payload: + def _cleanup_transform(curr: dict[str, Any]) -> dict[str, Any]: + st = curr.get("storage_config") if isinstance(curr, dict) else None + if isinstance(st, dict) and st.get("type") == "disk": + curr = dict(curr) + curr["storage_config"] = self._default_storage_config().model_dump() + return validate_stored_config(curr).model_dump(mode="json") + try: - self.save_config(config=config) + self.update_config_payload(_cleanup_transform) except (OSError, ConfigWriteConflict):🤖 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 `@reflexio/server/services/configurator/local_file_config_storage.py` around lines 124 - 127, Update the schema-cleanup flow around save_config and replace the full-file save with an update_config_payload transform. Make the transform normalize the payload loaded while holding the lock, so concurrent writes completed after the initial read are re-read and preserved; retain the existing OSError and ConfigWriteConflict handling.tests/server/services/configurator/test_config_storage_contract.py (1)
234-246: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAssert that non-default user values survive the cleanup conflict.
This fixture is otherwise identical to the default config, so the test would still pass if
load_config()were to catch an unexpected error and silently fall back to returning defaults. Add a custom persisted field to guarantee the file was actually loaded.🧪 Proposed test strengthening
payload = storage.get_default_config().model_dump(mode="json") + payload["agent_context_prompt"] = "preserve me" payload["retired_field"] = "ignored" config_path = Path(storage.config_file) config_path.parent.mkdir(parents=True, exist_ok=True) config_path.write_text(json.dumps(payload), encoding="utf-8") lock_path = config_path.with_suffix(".json.lock") with lock_path.open("a+b") as lock_file: fcntl.flock(lock_file.fileno(), fcntl.LOCK_EX | fcntl.LOCK_NB) loaded = storage.load_config() assert loaded.storage_config == storage.get_default_config().storage_config + assert loaded.agent_context_prompt == "preserve me" assert json.loads(config_path.read_text())["retired_field"] == "ignored"🤖 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 `@tests/server/services/configurator/test_config_storage_contract.py` around lines 234 - 246, Strengthen the load_config test around storage.get_default_config() by setting a non-default persisted user value before writing the payload, then assert that value is present in loaded. Keep the retired_field preservation assertion and lock-conflict setup unchanged so the test verifies the file was loaded rather than silently returning defaults.
🤖 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 `@reflexio/server/services/configurator/local_file_config_storage.py`:
- Around line 172-178: Update the local storage update flow around
_save_payload_to_local_dir so transformed payloads are first validated and
canonicalized using the same Config payload contract as the base implementation.
Compare the canonical result with current, persist only that canonical payload,
and return it in ConfigPayloadUpdateResult while preserving the unchanged fast
path.
- Around line 124-127: Update the schema-cleanup flow around save_config and
replace the full-file save with an update_config_payload transform. Make the
transform normalize the payload loaded while holding the lock, so concurrent
writes completed after the initial read are re-read and preserved; retain the
existing OSError and ConfigWriteConflict handling.
In `@tests/server/services/configurator/test_config_storage_contract.py`:
- Around line 234-246: Strengthen the load_config test around
storage.get_default_config() by setting a non-default persisted user value
before writing the payload, then assert that value is present in loaded. Keep
the retired_field preservation assertion and lock-conflict setup unchanged so
the test verifies the file was loaded rather than silently returning defaults.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5ef7caca-5452-4544-a150-82270206985e
📒 Files selected for processing (10)
reflexio/lib/_config.pyreflexio/models/api_schema/retriever_schema.pyreflexio/server/README.mdreflexio/server/routes/config.pyreflexio/server/services/configurator/base_configurator.pyreflexio/server/services/configurator/config_storage.pyreflexio/server/services/configurator/local_file_config_storage.pytests/lib/test_config_unit.pytests/server/api_endpoints/test_api_routes.pytests/server/services/configurator/test_config_storage_contract.py
🚧 Files skipped from review as they are similar to previous changes (8)
- reflexio/models/api_schema/retriever_schema.py
- reflexio/server/README.md
- reflexio/server/services/configurator/config_storage.py
- reflexio/lib/_config.py
- tests/lib/test_config_unit.py
- reflexio/server/services/configurator/base_configurator.py
- reflexio/server/routes/config.py
- tests/server/api_endpoints/test_api_routes.py
1 participant
Summary
Changes
Configuration write contract
PreparedConfigWritefor side-effect-free validation and a storage-level read/transform/write result contract.CONFIG_UNCHANGED_MESSAGE) instead of an inline string match.Local-file storage
ConfigWriteConflict.API and documentation
ConfigWriteConflictto HTTP 409 withConfiguration update already in progress.Review follow-ups
PreparedConfigWrite.changedfield — storage owns the authoritative changed result.Test Plan
uv run pytest tests/lib/test_config_unit.py tests/server/api_endpoints/test_api_routes.py tests/server/services/configurator/test_config_storage_contract.py -q -o 'addopts='— 56 passeduv run python -c "import reflexio"127.0.0.1:8099was unavailableSummary by CodeRabbit