fix: object_store: stop throwing errors when checking generation file

[amananas]

A new routine is added to File class, to check whether a file exists or not. It will allow file existence check, without trying to open the file and thus throw an error when the use case is valid.

This new routine is added to Generation wasUpdated routine: if the generation file does not exist, the routine will exit without error, instead of trying to open the file and report an error.

Fixes #753

Summary by CodeRabbit

• New Features

  • Added a file-existence check so callers can determine whether a file exists without opening it.

• Bug Fixes

  • Update detection now treats missing files as needing updates, improving change-detection correctness.

• Tests

  • Expanded tests to validate file-existence checks before performing file operations.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

📝 Walkthrough

Walkthrough

Added File::exists(const std::string&) to check file existence (platform-specific: stat on POSIX, _access on Windows). Generation::wasUpdated() now returns true immediately when the generation file is missing. Tests updated to use the new existence check before constructing File objects.

Changes

Cohort / File(s)	Summary
File existence API src/lib/object_store/File.h, src/lib/object_store/File.cpp	Add public static bool File::exists(const std::string &path); POSIX uses stat(...) == 0, Windows uses _access(..., 0) == 0.
Generation update check src/lib/object_store/Generation.cpp	wasUpdated() now calls File::exists(path) and short-circuits to true when the generation file is missing (avoids attempting to open a non-existent file).
Tests updated src/lib/object_store/test/FileTests.cpp	Test cases augmented to assert file existence via File::exists() before creating File instances (platform-specific path separators handled).

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐇 I sniff the path and tap my paw,
If no file sleeps beneath the straw,
I skip the open, let logs unwind,
A quiet hop, no noise behind. 🥕

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

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

✅ 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 identifies the main change: adding a check for generation file existence to prevent throwing errors, which directly addresses the PR objectives.
Linked Issues check	✅ Passed	The PR successfully implements a solution to issue #753 by adding File::exists() method and using it in Generation::wasUpdated() to check file existence before opening, preventing spurious error logs.
Out of Scope Changes check	✅ Passed	All changes directly support the objective of preventing spurious generation file errors: File::exists() method, its integration into Generation::wasUpdated(), and test coverage are all in scope.

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

✨ Finishing touches

• 📝 Generate docstrings

---

---

📜 Recent review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8f49d40 and 1bb911f.

📒 Files selected for processing (4)

• src/lib/object_store/File.cpp
• src/lib/object_store/File.h
• src/lib/object_store/Generation.cpp
• src/lib/object_store/test/FileTests.cpp

🧰 Additional context used

🧠 Learnings (1)

📓 Common learnings

Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 815
File: src/lib/SoftHSM.cpp:6543-6546
Timestamp: 2025-09-11T16:54:00.370Z
Learning: SoftHSM PR `#815` (pkcs11.h 3.1.0 update, CK_RSA_AES_KEY_WRAP_PARAMS fix): Maintainer bukka prefers to keep scope minimal (header sync and param rename only); broader OAEP parameter enforcement or backend plumbing is out of scope for this PR and should be handled in a follow-up issue.

🧬 Code graph analysis (4)

src/lib/object_store/File.cpp (1)

src/lib/object_store/test/FileTests.cpp (2)

• exists (359-381)
• exists (359-359)

src/lib/object_store/Generation.cpp (1)

src/lib/object_store/File.cpp (2)

• exists (149-157)
• exists (149-149)

src/lib/object_store/test/FileTests.cpp (1)

src/lib/object_store/File.cpp (2)

• exists (149-157)
• exists (149-149)

src/lib/object_store/File.h (2)

src/lib/object_store/File.cpp (2)

• exists (149-157)
• exists (149-149)

src/lib/object_store/test/FileTests.cpp (2)

• exists (359-381)
• exists (359-359)

🔇 Additional comments (4)

src/lib/object_store/Generation.cpp (1)

91-96: LGTM! Early existence check prevents spurious error logs.

The pre-check correctly avoids the ERROR_MSG in the File constructor when the generation file doesn't exist. Returning true (indicating "was updated") is consistent with the existing fallback behavior when the file is invalid (lines 104-106 and 130-132).

Note: There's an inherent TOCTOU window between File::exists() and the subsequent File constructor, but this is acceptable since the existing code already handles invalid files gracefully, and the primary goal is reducing log noise rather than providing atomicity.

src/lib/object_store/File.cpp (1)

146-157: LGTM! Clean implementation with consistent cross-platform semantics.

The implementation correctly uses stat() on POSIX and _access() with mode 0 (existence check) on Windows. Both approaches check file existence without requiring read permissions, providing consistent behavior across platforms.

src/lib/object_store/File.h (1)

50-53: LGTM! Well-documented static utility method.

The declaration is appropriate: static since no instance state is needed, and const std::string& avoids unnecessary copies. The documentation clearly explains the intended use case.

src/lib/object_store/test/FileTests.cpp (1)

77-104: LGTM! Good test coverage for the new File::exists() method.

The tests appropriately verify both cases:

• Non-existent file returns false (lines 79, 82)
• Existing file returns true (lines 97, 100)

Platform-specific path separators are correctly handled.

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}

---

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: 1

🧹 Nitpick comments (2)

src/lib/object_store/File.h (1)

50-54: Consider passing path by const reference for efficiency.

The path parameter is passed by value, creating an unnecessary string copy.

Apply this diff to pass by const reference:

-	static bool exists(std::string path);
+	static bool exists(const std::string& path);

src/lib/object_store/File.cpp (1)

149-149: Parameter should be passed by const reference.

As noted in the File.h review, the path parameter creates an unnecessary copy. Apply the same change here to match the header declaration once updated.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 70e0d4c and dda968a.

📒 Files selected for processing (3)

• src/lib/object_store/File.cpp (1 hunks)
• src/lib/object_store/File.h (1 hunks)
• src/lib/object_store/Generation.cpp (1 hunks)

🧰 Additional context used

🧠 Learnings (1)

📓 Common learnings

Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 815
File: src/lib/SoftHSM.cpp:6543-6546
Timestamp: 2025-09-11T16:54:00.370Z
Learning: SoftHSM PR #815 (pkcs11.h 3.1.0 update, CK_RSA_AES_KEY_WRAP_PARAMS fix): Maintainer bukka prefers to keep scope minimal (header sync and param rename only); broader OAEP parameter enforcement or backend plumbing is out of scope for this PR and should be handled in a follow-up issue.

🧬 Code graph analysis (3)

src/lib/object_store/File.cpp (1)

src/lib/object_store/test/FileTests.cpp (2)

• exists (355-377)
• exists (355-355)

src/lib/object_store/File.h (2)

src/lib/object_store/File.cpp (2)

• exists (149-165)
• exists (149-149)

src/lib/object_store/test/FileTests.cpp (2)

• exists (355-377)
• exists (355-355)

src/lib/object_store/Generation.cpp (2)

src/lib/object_store/File.cpp (2)

• exists (149-165)
• exists (149-149)

src/lib/object_store/test/FileTests.cpp (2)

• exists (355-377)
• exists (355-355)

🔇 Additional comments (1)

src/lib/object_store/Generation.cpp (1)

91-96: LGTM - Effectively prevents spurious error logs.

The early existence check successfully avoids attempting to open non-existent generation files, directly addressing issue #753. Returning true when the file doesn't exist is consistent with the existing behavior at lines 104-107 and 130-133, where invalid files also return true.

[bukka]

It makes sense but please also add test to FileTests.cpp