[fix] Do not delete data in odemis-settings.yaml when data cannot be …

[tepals]

…serialized

Persistent data is saved in /etc/odemis-settings.yaml, when the data could not be serialized it would empty the file and then fail to write new data. Now we only write new data if it can be serialized.

[coderabbitai]

📝 Walkthrough

Walkthrough

The change makes persistence writes safer by serializing in-memory persistent data to a string before modifying the settings file. Serialization is attempted first; if it fails, the exception is logged and the function returns without touching the file. Only after successful serialization does the code truncate the settings file, write the serialized string, flush, and attempt an fsync (logging a debug message if fsync isn't available). Any exceptions during the write sequence are logged. The settings file is therefore only overwritten after successful serialization.

Sequence Diagram(s)

sequenceDiagram
    participant Odemisd
    participant Serializer
    participant SettingsFile
    participant Filesystem
    participant Logger

    Odemisd->>Serializer: serialize(_persistent_data) -> data_str
    alt serialization fails
        Serializer-->>Odemisd: raise exception
        Odemisd->>Logger: log exception ("serialization failed")
        Odemisd-->>SettingsFile: return (no change)
    else serialization succeeds
        Serializer-->>Odemisd: data_str
        Odemisd->>SettingsFile: open file (r+)
        Odemisd->>SettingsFile: truncate()
        Odemisd->>SettingsFile: write(data_str)
        Odemisd->>SettingsFile: flush()
        Odemisd->>Filesystem: attempt fsync()
        alt fsync unavailable
            Filesystem-->>Odemisd: raise NotImplemented / OSError
            Odemisd->>Logger: log debug ("fsync unavailable, continuing")
        else fsync succeeds
            Filesystem-->>Odemisd: fsync ok
        end
        alt write or fs operations fail
            SettingsFile-->>Odemisd: raise exception
            Odemisd->>Logger: log exception ("write failed")
        else all good
            SettingsFile-->>Odemisd: close()
        end
    end

Loading

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly describes the main fix: preventing deletion of data in odemis-settings.yaml when serialization fails, which directly matches the changeset's primary objective.
Description check	✅ Passed	The description is directly related to the changeset, explaining the problem (data loss on serialization failure) and the solution (only write if serialization succeeds).

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

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

Tip

Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs).
Share your feedback on Discord.

---

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]

🧹 Nitpick comments (1)

src/odemis/odemisd/main.py (1)

165-166: Unnecessary bytes check.

yaml.dump() returns a str by default; it only returns bytes when the encoding parameter is explicitly set. Since YamlExtraDumper doesn't configure an encoding, data_str will always be a string. This check is harmless but could be removed for clarity.

♻️ Suggested simplification

         try:
             # Overwrite file only after successful serialization
             self._settings.truncate(0)
             self._settings.seek(0)
-            if isinstance(data_str, (bytes, bytearray)):
-                data_str = data_str.decode()
             self._settings.write(data_str)
             self._settings.flush()

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/odemis/odemisd/main.py` around lines 165 - 166, Remove the unnecessary
bytes/bytearray decoding branch around the yaml.dump output: since
yaml.dump(...) (used with YamlExtraDumper) returns str by default, delete the
conditional that checks isinstance(data_str, (bytes, bytearray)) and the
subsequent data_str = data_str.decode() so that the code treats data_str as a
string directly; locate the handling of data_str immediately after the yaml.dump
call and remove that decoding logic.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/odemis/odemisd/main.py`:
- Around line 165-166: Remove the unnecessary bytes/bytearray decoding branch
around the yaml.dump output: since yaml.dump(...) (used with YamlExtraDumper)
returns str by default, delete the conditional that checks isinstance(data_str,
(bytes, bytearray)) and the subsequent data_str = data_str.decode() so that the
code treats data_str as a string directly; locate the handling of data_str
immediately after the yaml.dump call and remove that decoding logic.

---

ℹ️ Review info

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b6dea6a and 3bf845e.

📒 Files selected for processing (1)

• src/odemis/odemisd/main.py

[coderabbitai]

🧹 Nitpick comments (1)

src/odemis/odemisd/main.py (1)

167-170: Consider narrowing the exception type for fsync.

Per the static analysis hint, catching a blind Exception here is overly broad. The fsync() call raises OSError when not supported (e.g., for certain file-like objects). Using OSError would be more precise and avoid masking unexpected errors.

Suggested fix

             try:
                 os.fsync(self._settings.fileno())
-            except Exception:
+            except OSError:
                 logging.debug("fsync not available for settings file")

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/odemis/odemisd/main.py` around lines 167 - 170, The try/except around
calling os.fsync(self._settings.fileno()) is catching all Exception types;
change the except to catch OSError specifically so only expected filesystem/IO
errors (e.g., unsupported fileno/fsync) are handled and other unexpected
exceptions are not swallowed—update the block around
os.fsync(self._settings.fileno()) to except OSError and keep the logging.debug
message.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/odemis/odemisd/main.py`:
- Around line 167-170: The try/except around calling
os.fsync(self._settings.fileno()) is catching all Exception types; change the
except to catch OSError specifically so only expected filesystem/IO errors
(e.g., unsupported fileno/fsync) are handled and other unexpected exceptions are
not swallowed—update the block around os.fsync(self._settings.fileno()) to
except OSError and keep the logging.debug message.

---

ℹ️ Review info

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3bf845e and 974fbbd.

📒 Files selected for processing (1)

• src/odemis/odemisd/main.py