Repository navigation
Conversation
Reviewer's GuideAPIUpdateConfig now starts a dedicated filesystem-migration worker, moving filesystem migration state handling out of its update flow.
Setdata validates the filesystem-operation version against the version owned by the new migration module.
ImplementationImplement the serialized filesystem-migration worker, including broker state loading, ordered resumable operations, completion tracking, and error reporting.
Make totals generation use a persistent lock, propagate contention and lock-file replacement failures, and publish readiness on the system topic.
TestsAdd coverage for the worker’s migration sequencing, progress persistence, recovery behavior, and broker state loading.
Test totals lock retention, contention handling, and continuation across individual log failures.
Remove filesystem-migration tests from UpdateConfig and verify that it starts the new worker with the expected process options.
Lock files and vendored codeIgnore the added generated or runtime artifact path.
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 9 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="packages/helpermodules/filesystem_migrations.py" line_range="70" />
<code_context>
+ log.debug(f"current file operation version: {file_operation_version}")
+ log.debug(f"target file operation version: {self.FILE_OPERATION_VERSION}")
+ for version in list(range(self.FILE_OPERATION_VERSION+1)):
+ operation_required = version not in file_operation_version
+ if version == 0:
+ # Alte Installationen haben Version 0 schon beim Prozessstart eingetragen.
</code_context>
<issue_to_address>
**Legacy progress blocks migrations**
When the retained `openWB/system/file_operation_version` topic contains the legacy integer format, `version not in file_operation_version` raises `TypeError`, so the worker exits without running or recording any filesystem migration.
Normalize the legacy integer progress value to the iterable format before checking migration versions.
</issue_to_address>
### Comment 2
<location path="packages/helpermodules/filesystem_migrations.py" line_range="102-114" />
<code_context>
+ # Die Lock-Datei erst nach dem gesamten Migrationslauf entfernen, auch bei Fehlern.
+ with LOCK_FILE.open("a") as lock_file:
+ try:
+ fcntl.flock(lock_file.fileno(), fcntl.LOCK_EX | fcntl.LOCK_NB)
+ except BlockingIOError:
+ log.info("Dateisystem-Migrationen laufen bereits. Kein zweiter Lauf gestartet.")
+ return
+
+ try:
+ FilesystemMigrations().update_filesystem()
+ except Exception:
+ log.exception("Dateisystem-Migrationen abgebrochen.")
+ pub_system_message({}, "Fehler bei der Aktualisierung der Dateisystem-Daten.", MessageType.ERROR)
+ raise
+ finally:
+ LOCK_FILE.unlink(missing_ok=True)
+
+
</code_context>
<issue_to_address>
**Workers run migrations concurrently**
When the lock path is deleted or replaced while a worker holds its original file descriptor, `run_migrations` unlinks `LOCK_FILE` without verifying that it still names the locked inode, allowing workers to lock different inodes and run migrations concurrently.
Stop unlinking the lock file during cleanup so workers continue contending on the same inode.
Also at `packages/helpermodules/measurement_logging/generate_totals_subprocess.py:106`.
</issue_to_address>
### Comment 3
<location path="packages/helpermodules/filesystem_migrations.py" line_range="54" />
<code_context>
+ """Liest den aktuellen Broker-Zustand im Migrationsprozess ein."""
+ broker = BrokerClient("filesystem-migrations", self.on_connect, self.on_message)
+ try:
+ broker.start_finite_loop()
+ finally:
+ broker.disconnect()
</code_context>
<issue_to_address>
**Delayed topics erase migration progress**
When the broker takes longer than one second to deliver retained migration topics, `start_finite_loop` returns with an incomplete topic snapshot, so the worker treats the missing version topic as a new installation, publishes an empty version list, and can rerun completed migrations.
Wait for the required retained topics before processing them, and do not initialize or publish missing progress when the wait ends without those topics.
Also at `packages/helpermodules/filesystem_migrations.py:65`.
</issue_to_address>
### Comment 4
<location path="packages/helpermodules/filesystem_migrations.py" line_range="86" />
<code_context>
+ """Generiere die Totals-Summen für Tage und Monate."""
+
+ self._update_topic("openWB/system/log_data_ready", False)
+ generate_totals()
+ self.all_received_topics["openWB/system/log_data_ready"] = True
+
</code_context>
<issue_to_address>
**Failed totals are marked ready**
When a daily or monthly totals save fails, including from a transient filesystem error, `generate_totals()` catches per-day and per-month exceptions and returns normally, so `upgrade_file_operation_0()` marks logs ready and the worker records version 0 even when a totals write failed. The missing totals are then not retried by this migration.
Propagate a failure or persist failed periods for retry, and only mark readiness/version complete once those failures are resolved.
</issue_to_address>
### Comment 5
<location path="packages/helpermodules/update_config.py" line_range="839" />
<code_context>
self._remove_invalid_topics()
self.__pub_missing_defaults()
self.__update_version()
+ self.__start_filesystem_migrations()
except Exception:
log.exception("Fehler bei der Aktualisierung des Brokers.")
</code_context>
<issue_to_address>
**Stale totals remain available**
When a scheduled five-minute handler runs after the worker starts but before it publishes `log_data_ready=False`, `UpdateConfig` signals completion while the old readiness value is still `True`, so `update_pv_monthly_yearly_yields` publishes totals from before the migration and users see stale PV yields.
Publish `log_data_ready=False` before launching the worker or signaling configuration completion.
Also at `packages/helpermodules/filesystem_migrations.py:85`, `packages/helpermodules/update_config.py:941`.
</issue_to_address>
### Comment 6
<location path="packages/helpermodules/setdata.py" line_range="15" />
<code_context>
+import pytest
+
+from helpermodules import filesystem_migrations
+from helpermodules.filesystem_migrations import FilesystemMigrations
+
+
</code_context>
<issue_to_address>
**Main-process logs are redirected**
When the main application imports `setdata` during startup, importing `setdata` imports `FilesystemMigrations`, which imports `generate_totals_subprocess`; that module runs `setup_generate_totals_logging(include_process_log=True)` at import time. It sets the main process's `process_log` logger to stop propagating to the root logger and sends its messages to `generate_totals.log` instead of `main.log`.
Avoid importing the totals subprocess module from the main process, or move its process-log setup out of module import and invoke it only in the migration subprocess.
Also at `packages/helpermodules/filesystem_migrations.py:21`.
</issue_to_address>
### Comment 7
<location path="packages/helpermodules/filesystem_migrations_test.py" line_range="149" />
<code_context>
+def test_file_operation_0_start_behavior(monkeypatch, file_operation_version, finished, expected_calls):
+ monkeypatch.setattr(FilesystemMigrations, "FILE_OPERATION_VERSION", 0)
+ updater = FilesystemMigrations()
+ updater.all_received_topics = {
+ "openWB/system/file_operation_version": file_operation_version,
+ "openWB/system/log_data_ready": finished,
</code_context>
<issue_to_address>
**Broker message handling can stay broken**
When the broker loop stops delivering messages through `updater.on_message`, in `test_filesystem_worker_reads_current_topics_before_migrating` (line 149), `read_topics` writes directly to `all_received_topics`, bypassing `on_message`; the no-migration assertion still passes if broker messages are never collected.
Have the broker mock deliver an MQTT message through the callback and assert that its topic is collected before migration decisions are made.
</issue_to_address>
### Comment 8
<location path="packages/helpermodules/filesystem_migrations_test.py" line_range="197" />
<code_context>
+ with pytest.raises(RuntimeError, match="migration failed"):
+ filesystem_migrations.run_migrations()
+
+ message.assert_called_once()
+ assert not lock_path.exists()
+ update.side_effect = None
</code_context>
<issue_to_address>
**Failure report contents go unchecked**
When the worker calls `pub_system_message` with incorrect report arguments, in `test_worker_reports_failure_and_releases_lock` (line 197), `message.assert_called_once()` passes even if the worker reports success or sends the wrong message and type, so the test can miss a broken failure report.
Assert the expected message text and `MessageType.ERROR` in the mock call.
</issue_to_address>
### Comment 9
<location path="packages/helpermodules/filesystem_migrations_test.py" line_range="138" />
<code_context>
+ with pytest.raises(RuntimeError, match="generator failed"):
+ updater._FilesystemMigrations__solve_breaking_changes_filesystem()
+
+ assert updater.all_received_topics["openWB/system/log_data_ready"] is False
+ assert updater.all_received_topics["openWB/system/file_operation_version"] == completed
+ next_migration.assert_not_called()
</code_context>
<issue_to_address>
**Unready state can go unpublished**
When the migration updates local readiness without publishing the retained system topic, in `test_file_operation_0_failure_leaves_logs_unready_and_blocks_next` (line 138), the assertion checks only the worker’s in-memory topic map, so the test passes if `log_data_ready` is set to `False` locally but the retained broker topic stays `True`.
Assert that the publisher was called with `openWB/system/log_data_ready` and the serialized `false` payload.
</issue_to_address>| """Liest den aktuellen Broker-Zustand im Migrationsprozess ein.""" | ||
| broker = BrokerClient("filesystem-migrations", self.on_connect, self.on_message) | ||
| try: | ||
| broker.start_finite_loop() |
There was a problem hiding this comment.
🟡 Medium · Delayed topics erase migration progress
When the broker takes longer than one second to deliver retained migration topics, start_finite_loop returns with an incomplete topic snapshot, so the worker treats the missing version topic as a new installation, publishes an empty version list, and can rerun completed migrations.
Wait for the required retained topics before processing them, and do not initialize or publish missing progress when the wait ends without those topics.
Also at packages/helpermodules/filesystem_migrations.py:65.
Prompt for AI agents
In `packages/helpermodules/filesystem_migrations.py` at line 54:
**Delayed topics erase migration progress**
When the broker takes longer than one second to deliver retained migration topics, `start_finite_loop` returns with an incomplete topic snapshot, so the worker treats the missing version topic as a new installation, publishes an empty version list, and can rerun completed migrations.
Wait for the required retained topics before processing them, and do not initialize or publish missing progress when the wait ends without those topics.
Also at `packages/helpermodules/filesystem_migrations.py:65`.| """Generiere die Totals-Summen für Tage und Monate.""" | ||
|
|
||
| self._update_topic("openWB/system/log_data_ready", False) | ||
| generate_totals() |
There was a problem hiding this comment.
🟡 Medium · Failed totals are marked ready
When a daily or monthly totals save fails, including from a transient filesystem error, generate_totals() catches per-day and per-month exceptions and returns normally, so upgrade_file_operation_0() marks logs ready and the worker records version 0 even when a totals write failed. The missing totals are then not retried by this migration.
Propagate a failure or persist failed periods for retry, and only mark readiness/version complete once those failures are resolved.
Prompt for AI agents
In `packages/helpermodules/filesystem_migrations.py` at line 86:
**Failed totals are marked ready**
When a daily or monthly totals save fails, including from a transient filesystem error, `generate_totals()` catches per-day and per-month exceptions and returns normally, so `upgrade_file_operation_0()` marks logs ready and the worker records version 0 even when a totals write failed. The missing totals are then not retried by this migration.
Propagate a failure or persist failed periods for retry, and only mark readiness/version complete once those failures are resolved.| self._remove_invalid_topics() | ||
| self.__pub_missing_defaults() | ||
| self.__update_version() | ||
| self.__start_filesystem_migrations() |
There was a problem hiding this comment.
🟡 Medium · Stale totals remain available
When a scheduled five-minute handler runs after the worker starts but before it publishes log_data_ready=False, UpdateConfig signals completion while the old readiness value is still True, so update_pv_monthly_yearly_yields publishes totals from before the migration and users see stale PV yields.
Publish log_data_ready=False before launching the worker or signaling configuration completion.
Also at packages/helpermodules/filesystem_migrations.py:85, packages/helpermodules/update_config.py:941.
Prompt for AI agents
In `packages/helpermodules/update_config.py` at line 839:
**Stale totals remain available**
When a scheduled five-minute handler runs after the worker starts but before it publishes `log_data_ready=False`, `UpdateConfig` signals completion while the old readiness value is still `True`, so `update_pv_monthly_yearly_yields` publishes totals from before the migration and users see stale PV yields.
Publish `log_data_ready=False` before launching the worker or signaling configuration completion.
Also at `packages/helpermodules/filesystem_migrations.py:85`, `packages/helpermodules/update_config.py:941`.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Disconnecting before retained state is confirmed can cause delayed delivery to be treated as a fresh installation, overwriting migration history and rerunning completed migrations.
2 open issues
What changed in this PR
Moves filesystem migrations out of broker configuration updates into a dedicated background worker, separating responsibilities and supporting ordered, resumable execution.
Changes:
- Runs migrations sequentially and persists completed versions through MQTT.
- Uses persistent file locks to prevent concurrent migration runs.
- Updates tests for worker startup, migration recovery, legacy progress conversion, and totals processing.
| File | Description |
|---|---|
packages/helpermodules/update_config.py |
Replaces inline filesystem migrations with worker startup. |
packages/helpermodules/update_config_test.py |
Verifies the worker launch arguments. |
packages/helpermodules/setdata.py |
Uses the migration worker’s version constant for validation. |
packages/helpermodules/measurement_logging/generate_totals_subprocess.py |
Retains lock files, propagates lock failures, and publishes readiness directly. |
packages/helpermodules/measurement_logging/generate_totals_subprocess_test.py |
Replaces wrapper tests with per-log failure-handling tests. |
packages/helpermodules/filesystem_migrations.py |
Adds the serialized worker with retained progress and retry handling. |
packages/helpermodules/filesystem_migrations_test.py |
Covers ordering, recovery, legacy progress, and broker-state loading. |
.gitignore |
Ignores persistent migration lock files. |
Review effort: Balanced
Comment on lines
+54
to
+56
| broker.start_finite_loop() | ||
| finally: | ||
| broker.disconnect() |
Comment on lines
+88
to
+91
| except BlockingIOError: | ||
| log.exception("generate_totals_subprocess läuft bereits. " | ||
| "Der zweite Prozess wird beendet.") | ||
| return | ||
| raise |
AlexanderHa98
force-pushed
the
feature_update_Dateisystem_Migrationen
branch
from
November 9, 2026 14:02
2e2a3b7 to
7758c0e
Compare
LKuemmel
approved these changes
Nov 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
filesystem_migrations.py
-> in eigene Datei verschoben, wegen Circular Import-Error
Summary by Sourcery
Run filesystem migrations as a serialized, resumable background process separate from broker configuration updates.
New Features:
Bug Fixes:
Enhancements:
Tests: