fix(keyring): stop concurrent writers wiping the shared secrets file - #5436
Conversation
Co-authored-by: Medulla <medulla@tinyhumans.ai>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 15 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: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe keyring backends now share cross-process sidecar locking, atomic file replacement, and corruption quarantine. Mutations lock the complete read-modify-write cycle. Tests use isolated workspaces and cover concurrency, replacement, permissions, cleanup, and corrupt files. ChangesKeyring file safety
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f5977788c5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Co-authored-by: Medulla <medulla@tinyhumans.ai>
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 66bd6b337c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Co-authored-by: Medulla <medulla@tinyhumans.ai>
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
Co-authored-by: Medulla <medulla@tinyhumans.ai>
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 51d691cbb9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // quarantines corrupt ciphertext. Hold the same lock as writers for | ||
| // either case so a delayed quarantine cannot rename a replacement a | ||
| // concurrent `set` just published. | ||
| let _guard = file_store::lock_for_write(&self.path)?; |
There was a problem hiding this comment.
When a production desktop using EncryptedFileBackend and a dev/file-backed Medulla process share a workspace during legacy migration, this locks secrets.enc.lock while FileBackend::set locks dev-keychain.json.lock, so they do not serialize. The migration can read the old plaintext map, the plaintext writer can publish a new entry, and then the migration can rename that newer file to .migrated while writing its stale snapshot to secrets.enc, making the new credential disappear from both active stores. The fresh evidence beyond the earlier same-backend migration race is the distinct lock path used by FileBackend; migration must also coordinate on the legacy file's lock or use one workspace-wide lock.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/openhuman/security/keyring/encrypted_file_backend.rs (1)
326-332: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
EncryptedFileBackend::read_mapmixes reading with filesystem mutation, which causes both findings in this file.read_mapperforms the legacy migration and the corruption quarantine while serving a read. That single design choice forcesgetonto the exclusive writer lock and lets the write path persist an empty map after a decryption failure. Separating the read from the recovery closes both.
src/openhuman/security/keyring/encrypted_file_backend.rs#L326-L332: giveread_mapa write policy sosetanddeletepropagate a decryption failure as an error instead of writing a map built from an emptyHashMap.src/openhuman/security/keyring/encrypted_file_backend.rs#L307-L318: once the mutating work is factored out of the read path, letgetread without the exclusive lock and acquire it only when a migration or a quarantine is actually required.🤖 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/openhuman/security/keyring/encrypted_file_backend.rs` around lines 326 - 332, The read_map recovery behavior must be separated from ordinary reads: update read_map and the set/delete write paths at src/openhuman/security/keyring/encrypted_file_backend.rs:326-332 so decryption failures propagate during writes instead of allowing an empty map to be persisted; update get at src/openhuman/security/keyring/encrypted_file_backend.rs:307-318 to read without the exclusive lock and acquire it only when performing legacy migration or corruption quarantine.
🧹 Nitpick comments (5)
src/openhuman/security/keyring/backend.rs (1)
145-145: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider replacing the
for_writeboolean with a named enum.
self.read_map(true)andself.read_map(false)do not state their meaning at the call site. The two branches differ in whether a corrupt file is quarantined and the call fails, or is silently treated as empty. An inverted flag reintroduces the wipe this PR fixes. A two-variant enum such asCorruptionPolicy::QuarantineandCorruptionPolicy::TreatAsEmptymakes each call site self-describing.🤖 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/openhuman/security/keyring/backend.rs` at line 145, Replace the boolean parameter on read_map with a two-variant corruption-policy enum, such as CorruptionPolicy::Quarantine and CorruptionPolicy::TreatAsEmpty. Update every read_map call site to pass the named variant matching its existing behavior, preserving quarantine-and-fail versus silently treating corruption as empty.src/openhuman/security/keyring/file_store.rs (3)
69-98: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd grep-friendly diagnostics to the lock and write paths.
lock_for_writeandwrite_atomicemit no log lines. Onlyquarantine_corruptlogs. The lock blocks indefinitely, so a contended or stale lock produces a silent hang with no evidence in logs. Add debug logs for lock acquisition and release, and for the rename that publishes the new file. Use the[keyring]prefix already used in this module.🔧 Suggested diagnostics
file.lock_exclusive().map_err(|e| { KeyringError::Backend(format!( "could not lock the keyring at {}: {e}", lock_path.display() )) })?; + log::debug!( + "[keyring] acquired write lock at {}", + lock_path.display() + ); Ok(WriteLock { _file: file })pub struct WriteLock { /// Kept alive purely for its lock; `fs2` releases on close. _file: File, + path: PathBuf, +} + +impl Drop for WriteLock { + fn drop(&mut self) { + log::debug!("[keyring] released write lock at {}", self.path.display()); + } }Based on learnings and coding guidelines: "New or changed flows must include verbose, grep-friendly diagnostics for entry/exit, branches, external calls, retries, state transitions, and errors" and domain logging under
src/openhumanuses a bracketed domain prefix.🤖 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/openhuman/security/keyring/file_store.rs` around lines 69 - 98, Add verbose `[keyring]` debug logs in `lock_for_write` for lock acquisition start, successful acquisition, and release via `WriteLock` cleanup; add a debug log in `write_atomic` immediately before or after the rename that publishes the new file. Include the relevant paths and use the existing module logging conventions, while preserving current locking and atomic-write behavior.Sources: Coding guidelines, Learnings
141-159: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueConsider syncing the parent directory after the rename.
file.sync_all()makes the temp file contents durable. The rename itself is not durable until the parent directory is synced. After a crash, the file can revert to the previous version even thoughsetreturnedOk. The old version stays intact, so this is a lost-update window, not corruption.If you want the write to be durable on return, open the parent directory and call
sync_allon it after the rename. Directory sync is a no-op or unsupported on Windows, so guard it with#[cfg(unix)].🤖 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/openhuman/security/keyring/file_store.rs` around lines 141 - 159, Update the keyring write flow after std::fs::rename in the visible write function to sync the parent directory before returning success. On Unix, open the directory containing path and call sync_all, propagating any failure as KeyringError::Backend; guard this directory-sync step with #[cfg(unix)] while preserving existing temporary-file cleanup and rename error handling.
33-33: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winReplace
fs2with the standard library file-locking API.The pinned Rust 1.96.1 toolchain supports
File::lockandFile::unlock. Replace all currentfs2::FileExtcalls and removefs2; usefs4only if older Rust support is required.🤖 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/openhuman/security/keyring/file_store.rs` at line 33, Replace the fs2::FileExt import and all locking calls in the file-store implementation with the standard library File::lock and File::unlock APIs supported by Rust 1.96.1. Remove the fs2 dependency and related usage, preserving the existing lock and unlock behavior; introduce fs4 only if compatibility with older Rust versions is required.src/openhuman/security/keyring/file_store_tests.rs (1)
109-136: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe 200 ms wait can make this test slow or weak on loaded CI.
rx.recv_timeout(Duration::from_millis(200))asserts that the contender did not acquire the lock. The assertion is safe in the failure direction, because a broken lock makes the contender send immediately. The cost is a fixed 200 ms per run. That is acceptable.One coverage gap remains: no test asserts that
write_atomicremoves the temp file when the rename fails. Consider adding a case that makes the rename fail, for example by pointingpathat an existing directory.🤖 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/openhuman/security/keyring/file_store_tests.rs` around lines 109 - 136, Add a test covering the rename-failure cleanup path in write_atomic by using a destination path that points to an existing directory, forcing rename to fail, then assert the operation errors and its temporary file is removed. Reuse the existing temporary-directory setup and naming conventions from the surrounding file-store tests.
🤖 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 `@src/openhuman/security/keyring/backend.rs`:
- Around line 156-165: Update the parse-error branch in the keyring backend to
inspect the Option returned by quarantine_corrupt and report whether the file
was moved aside or remained in place when quarantine fails. Preserve the
existing error context, and update
file_backend_set_refuses_to_overwrite_an_unparseable_file to assert the revised
wording.
In `@src/openhuman/security/keyring/file_store.rs`:
- Around line 202-205: Update quarantine_corrupt to generate collision-resistant
target names using nanosecond precision or a unique counter instead of
timestamp() seconds, ensuring repeated quarantines preserve all corrupted files.
Document that suffix must match the source extension because
Path::with_extension replaces the existing extension.
In `@src/openhuman/security/keyring/store.rs`:
- Around line 198-207: Update fallback_workspace_dir so TEST_DIR uses a fresh,
collision-resistant temporary-directory name rather than only
std::process::id(), preventing reuse of stale keyring files across processes.
Adjust the surrounding cleanup comment to accurately state the directory’s
actual lifetime and cleanup behavior.
- Around line 180-184: Add grep-friendly diagnostics to the workspace resolver
surrounding the environment match: log resolver entry and exit, and log distinct
staging, default, and test branch selections with their outcomes without
including secrets or full paths. Also cover the related directory-creation flow
around the referenced lines, preserving existing behavior while ensuring
failures retain their error diagnostics.
- Around line 169-170: Initialize an isolated keyring workspace before any
keyring backend use in the integration-test setup, including setting the
test-specific OPENHUMAN_WORKSPACE and calling init_workspace. Update the
relevant test setup rather than fallback_workspace_dir, and ensure this runs
before keyring::set or keyring::get is invoked.
In `@src/openhuman/security/keyring/tests.rs`:
- Around line 157-165: Replace the meaningless original-buffer comparison in the
quarantine test with an assertion on the contents of the single quarantined
file. Use the filename collected in quarantined to read that file from
dir.path(), and verify its bytes equal the corrupt payload written by the test
while preserving the existing count assertion.
---
Outside diff comments:
In `@src/openhuman/security/keyring/encrypted_file_backend.rs`:
- Around line 326-332: The read_map recovery behavior must be separated from
ordinary reads: update read_map and the set/delete write paths at
src/openhuman/security/keyring/encrypted_file_backend.rs:326-332 so decryption
failures propagate during writes instead of allowing an empty map to be
persisted; update get at
src/openhuman/security/keyring/encrypted_file_backend.rs:307-318 to read without
the exclusive lock and acquire it only when performing legacy migration or
corruption quarantine.
---
Nitpick comments:
In `@src/openhuman/security/keyring/backend.rs`:
- Line 145: Replace the boolean parameter on read_map with a two-variant
corruption-policy enum, such as CorruptionPolicy::Quarantine and
CorruptionPolicy::TreatAsEmpty. Update every read_map call site to pass the
named variant matching its existing behavior, preserving quarantine-and-fail
versus silently treating corruption as empty.
In `@src/openhuman/security/keyring/file_store_tests.rs`:
- Around line 109-136: Add a test covering the rename-failure cleanup path in
write_atomic by using a destination path that points to an existing directory,
forcing rename to fail, then assert the operation errors and its temporary file
is removed. Reuse the existing temporary-directory setup and naming conventions
from the surrounding file-store tests.
In `@src/openhuman/security/keyring/file_store.rs`:
- Around line 69-98: Add verbose `[keyring]` debug logs in `lock_for_write` for
lock acquisition start, successful acquisition, and release via `WriteLock`
cleanup; add a debug log in `write_atomic` immediately before or after the
rename that publishes the new file. Include the relevant paths and use the
existing module logging conventions, while preserving current locking and
atomic-write behavior.
- Around line 141-159: Update the keyring write flow after std::fs::rename in
the visible write function to sync the parent directory before returning
success. On Unix, open the directory containing path and call sync_all,
propagating any failure as KeyringError::Backend; guard this directory-sync step
with #[cfg(unix)] while preserving existing temporary-file cleanup and rename
error handling.
- Line 33: Replace the fs2::FileExt import and all locking calls in the
file-store implementation with the standard library File::lock and File::unlock
APIs supported by Rust 1.96.1. Remove the fs2 dependency and related usage,
preserving the existing lock and unlock behavior; introduce fs4 only if
compatibility with older Rust versions is required.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cee6c78a-f6b6-4616-9ba3-11d39568a20e
📒 Files selected for processing (8)
src/openhuman/security/keyring/README.mdsrc/openhuman/security/keyring/backend.rssrc/openhuman/security/keyring/encrypted_file_backend.rssrc/openhuman/security/keyring/file_store.rssrc/openhuman/security/keyring/file_store_tests.rssrc/openhuman/security/keyring/mod.rssrc/openhuman/security/keyring/store.rssrc/openhuman/security/keyring/tests.rs
| match serde_json::from_slice::<HashMap<String, String>>(&bytes) { | ||
| Ok(map) => Ok(map), | ||
| Err(e) if for_write => { | ||
| file_store::quarantine_corrupt(&self.path, "json"); | ||
| Err(KeyringError::Backend(format!( | ||
| "dev-keychain.json at {} could not be parsed ({e}); it was moved aside \ | ||
| rather than overwritten", | ||
| self.path.display() | ||
| ))) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
The error text claims the file was moved even when the quarantine fails.
quarantine_corrupt returns None when the rename fails, and that result is discarded. The error message then states "it was moved aside rather than overwritten" while the corrupt file is still in place. A user who follows the message looks for a .corrupt. file that does not exist.
Branch on the returned Option and report the actual outcome.
🛠️ Proposed fix
Err(e) if for_write => {
- file_store::quarantine_corrupt(&self.path, "json");
- Err(KeyringError::Backend(format!(
- "dev-keychain.json at {} could not be parsed ({e}); it was moved aside \
- rather than overwritten",
- self.path.display()
- )))
+ let outcome = match file_store::quarantine_corrupt(&self.path, "json") {
+ Some(target) => format!("it was moved to {}", target.display()),
+ None => "it could not be moved aside and is still in place".to_string(),
+ };
+ Err(KeyringError::Backend(format!(
+ "dev-keychain.json at {} could not be parsed ({e}); {outcome} \
+ rather than overwritten",
+ self.path.display()
+ )))
}The existing test asserts on the substring "moved aside", so update file_backend_set_refuses_to_overwrite_an_unparseable_file in src/openhuman/security/keyring/tests.rs to match the new wording.
🤖 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/openhuman/security/keyring/backend.rs` around lines 156 - 165, Update the
parse-error branch in the keyring backend to inspect the Option returned by
quarantine_corrupt and report whether the file was moved aside or remained in
place when quarantine fails. Preserve the existing error context, and update
file_backend_set_refuses_to_overwrite_an_unparseable_file to assert the revised
wording.
| pub fn quarantine_corrupt(path: &Path, suffix: &str) -> Option<PathBuf> { | ||
| let stamp = chrono::Utc::now().timestamp(); | ||
| let target = path.with_extension(format!("{suffix}.corrupt.{stamp}")); | ||
| match std::fs::rename(path, &target) { |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
chrono 0.4 DateTime timestamp_nanos_opt method availability
💡 Result:
The method timestamp_nanos_opt() is available in the chrono crate as of version 0.4.31 [1]. It was introduced to provide a non-panicking alternative to the original timestamp_nanos() method [2][3], which has been deprecated since version 0.4.31 [4][5]. The timestamp_nanos_opt() method returns an Option<i64>, providing None if the DateTime value is out of the representable range for nanosecond precision (which spans approximately 584 years) [4]. Key details: - Availability: Introduced in chrono 0.4.31 [1]. - Functionality: Returns the number of non-leap-nanoseconds since January 1, 1970 UTC as an Option<i64> [4]. - Migration: The timestamp_nanos() method, which would panic on out-of-range values, is now deprecated in favor of timestamp_nanos_opt() [4][5][1]. Users are encouraged to replace calls to the older method with this new version to handle potential range errors safely [4][3].
Citations:
- 1: https://github.com/chronotope/chrono/releases/tag/v0.4.31
- 2: Add
timestamp_nanos_opt, deprecatetimestamp_nanoschronotope/chrono#1275 - 3: Add
timestamp_nanos_opt, deprecatetimestamp_nanoschronotope/chrono#1275 - 4: https://docs.rs/chrono/latest/chrono/struct.DateTime.html
- 5: https://docs.rs/chrono/latest/src/chrono/datetime/mod.rs.html?search=
Prevent quarantine filename collisions.
timestamp() has one-second granularity. Two quarantines within the same second can compute the same target. On Unix, the second std::fs::rename replaces the first target and destroys the earlier quarantined bytes.
Use nanosecond precision or append a unique counter, such as TEMP_COUNTER. Also document that suffix must match the source extension because with_extension replaces the existing extension.
🤖 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/openhuman/security/keyring/file_store.rs` around lines 202 - 205, Update
quarantine_corrupt to generate collision-resistant target names using nanosecond
precision or a unique counter instead of timestamp() seconds, ensuring repeated
quarantines preserve all corrupted files. Document that suffix must match the
source extension because Path::with_extension replaces the existing extension.
| match std::env::var("OPENHUMAN_APP_ENV").as_deref() { | ||
| Ok("staging") => home.join(".openhuman-staging"), | ||
| _ => home.join(".openhuman"), | ||
| }; | ||
| openhuman_dir | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add branch-level diagnostics for workspace resolution.
The changed fallback logic has separate staging, default, and test paths. The shown code logs only a directory-creation failure. Add grep-friendly logs for the selected branch and the resolver entry and exit. Log branch names and outcomes, not secrets or full paths.
As per coding guidelines, “New or changed flows must include verbose, grep-friendly diagnostics for entry/exit, branches, external calls, retries, state transitions, and errors, while never logging secrets or full PII.”
Also applies to: 208-216
🤖 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/openhuman/security/keyring/store.rs` around lines 180 - 184, Add
grep-friendly diagnostics to the workspace resolver surrounding the environment
match: log resolver entry and exit, and log distinct staging, default, and test
branch selections with their outcomes without including secrets or full paths.
Also cover the related directory-creation flow around the referenced lines,
preserving existing behavior while ensuring failures retain their error
diagnostics.
Source: Coding guidelines
| /// redirected. The directory is per-process and never cleaned up, because the | ||
| /// backend outlives any single test (`BACKEND` is a `OnceLock`) — the OS clears | ||
| /// it with the rest of the temp dir. | ||
| #[cfg(test)] | ||
| fn fallback_workspace_dir() -> PathBuf { | ||
| static TEST_DIR: OnceLock<PathBuf> = OnceLock::new(); | ||
| TEST_DIR | ||
| .get_or_init(|| { | ||
| let dir = | ||
| std::env::temp_dir().join(format!("openhuman-test-keyring-{}", std::process::id())); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Use a unique temporary directory name.
TEST_DIR is never cleaned, but its name uses only std::process::id(). A later process can reuse the PID and reopen the previous run's keyring files. The system temporary directory is not guaranteed to remove this child directory. Tests can then depend on stale secrets or prior state.
Use a fresh unique temporary-directory name for each process. Update the cleanup comment to match the actual behavior.
🤖 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/openhuman/security/keyring/store.rs` around lines 198 - 207, Update
fallback_workspace_dir so TEST_DIR uses a fresh, collision-resistant
temporary-directory name rather than only std::process::id(), preventing reuse
of stale keyring files across processes. Adjust the surrounding cleanup comment
to accurately state the directory’s actual lifetime and cleanup behavior.
| // The bytes survived under the quarantine name, so nothing is unrecoverable. | ||
| let quarantined: Vec<_> = std::fs::read_dir(dir.path()) | ||
| .unwrap() | ||
| .filter_map(Result::ok) | ||
| .map(|e| e.file_name().to_string_lossy().into_owned()) | ||
| .filter(|name| name.contains(".corrupt.")) | ||
| .collect(); | ||
| assert_eq!(quarantined.len(), 1, "expected one quarantined file"); | ||
| assert_ne!(original, b"{ truncated by a racing writer".to_vec()); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the quarantined contents instead of comparing unrelated buffers.
Line 165 compares original (the serialized JSON map written at line 143) with the corrupt literal that the test itself wrote at line 147. Those two values can never be equal, so the assertion always passes and proves nothing.
The comment above states the intent: the corrupt bytes must survive under the quarantine name. Read the quarantined file and assert its contents to test that claim.
💚 Proposed fix
// The bytes survived under the quarantine name, so nothing is unrecoverable.
- let quarantined: Vec<_> = std::fs::read_dir(dir.path())
+ let quarantined: Vec<_> = std::fs::read_dir(dir.path())
.unwrap()
.filter_map(Result::ok)
- .map(|e| e.file_name().to_string_lossy().into_owned())
- .filter(|name| name.contains(".corrupt."))
+ .map(|e| e.path())
+ .filter(|p| p.to_string_lossy().contains(".corrupt."))
.collect();
assert_eq!(quarantined.len(), 1, "expected one quarantined file");
- assert_ne!(original, b"{ truncated by a racing writer".to_vec());
+ assert_eq!(
+ std::fs::read(&quarantined[0]).unwrap(),
+ b"{ truncated by a racing writer".to_vec(),
+ "the corrupt bytes must survive under the quarantine name"
+ );
+ assert!(
+ !original.is_empty(),
+ "the original file should have held the first secret"
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // The bytes survived under the quarantine name, so nothing is unrecoverable. | |
| let quarantined: Vec<_> = std::fs::read_dir(dir.path()) | |
| .unwrap() | |
| .filter_map(Result::ok) | |
| .map(|e| e.file_name().to_string_lossy().into_owned()) | |
| .filter(|name| name.contains(".corrupt.")) | |
| .collect(); | |
| assert_eq!(quarantined.len(), 1, "expected one quarantined file"); | |
| assert_ne!(original, b"{ truncated by a racing writer".to_vec()); | |
| // The bytes survived under the quarantine name, so nothing is unrecoverable. | |
| let quarantined: Vec<_> = std::fs::read_dir(dir.path()) | |
| .unwrap() | |
| .filter_map(Result::ok) | |
| .map(|e| e.path()) | |
| .filter(|p| p.to_string_lossy().contains(".corrupt.")) | |
| .collect(); | |
| assert_eq!(quarantined.len(), 1, "expected one quarantined file"); | |
| assert_eq!( | |
| std::fs::read(&quarantined[0]).unwrap(), | |
| b"{ truncated by a racing writer".to_vec(), | |
| "the corrupt bytes must survive under the quarantine name" | |
| ); | |
| assert!( | |
| !original.is_empty(), | |
| "the original file should have held the first secret" | |
| ); |
🤖 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/openhuman/security/keyring/tests.rs` around lines 157 - 165, Replace the
meaningless original-buffer comparison in the quarantine test with an assertion
on the contents of the single quarantined file. Use the filename collected in
quarantined to read that file from dir.path(), and verify its bytes equal the
corrupt payload written by the test while preserving the existing count
assertion.
store.rs: main's thread-scoped test_scope module supersedes the branch's earlier fallback_workspace_dir split (the PR's only store.rs change was that same test-isolation refactor), so take main's version. README: merge both — main's aligned key-files table plus the PR's file_store.rs row, and the PR's lock_for_write cross-process-lock paragraph alongside main's test-isolation note. Co-authored-by: Medulla <medulla@tinyhumans.ai>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b2ec573023
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let stamp = chrono::Utc::now().timestamp(); | ||
| let target = path.with_extension(format!("{suffix}.corrupt.{stamp}")); |
There was a problem hiding this comment.
When two corrupt payloads are quarantined within the same second—for example, two Linux/macOS cores with different cached encryption keys alternately replacing and rejecting secrets.enc—both operations select the same target path. On these platforms std::fs::rename replaces an existing destination, so the second quarantine silently destroys the first recovery copy, potentially discarding the only ciphertext containing the user's original secrets. Include a subsecond timestamp, random nonce, or collision-retry sequence in the quarantine name.
Useful? React with 👍 / 👎.
1 participant
What
Both file keyring backends keep every secret in one file, so a
setof one key is a read-modify-write over all of them. That cycle had no cross-process guard, and a corrupt read on the write path degraded to an empty map. Together those turn a routine race into a wipe.Reported as: signing into Medulla, rebuilding, and finding yourself signed out. The app session is one entry in
{workspace}/dev-keychain.json, alongside everything else.Three defects, all in
security/keyring/:FileBackend/EncryptedFileBackendheld only aparking_lot::Mutex. More than one process routinely addresses the same workspace — a desktop core, amedullaTUI embedding the same core, and anycargo testrun that inheritedOPENHUMAN_WORKSPACE. The later writer persists the map it read before the earlier writer landed; the earlier secret is gone.dev-keychain.json.tmp/secrets.enc.tmp), so two writers wrote the same file and one renamed the other's half-written buffer into place.FileBackend::read_mapcaught the parse error and returned an empty map. Thesetthat followed then persisted a map holding nothing but the key being written — every other secret replaced by one.On the machine this was found on,
dev-keychain.jsonhad grown to 1 MB / 6558 entries, 6554 of them tempdir-scoped test junk, sharing that read-modify-write cycle with the live app session.Changes
New
security/keyring/file_store.rsholds the primitives both backends now share:lock_for_write— exclusive advisory lock (fs2, already a dependency) on a sidecar<path>.lock. On the sidecar rather than the secrets file becausewrite_atomicreplaces that file by rename, so a lock on the old inode would guard nothing. Held across the read and the write.write_atomic—0600, staged through a temp name unique to this process and call,sync_allbefore rename, temp removed on failure.quarantine_corrupt— one implementation of the move-aside thatEncryptedFileBackendalready had.FileBackend::read_mapnow takes afor_writeflag. Reads still degrade to empty (a missing token means "sign in again", which is recoverable; failing every unrelated lookup is not). Writes quarantine and return an error.Test isolation. Under
cfg(test)the file backends' fallback directory is a per-process scratch dir under the system temp dir, andOPENHUMAN_WORKSPACEis not consulted at all. Test code could previously reach a developer's live store, and did — the Medulla TUI exports that variable into every process it spawns, socargo testfrom such a shell wrote its per-test secrets into the developer's real keyring. A test that wants a specific location still callsinit_workspacefirst; only the fallback moved.Validation
cargo test --lib security::keyring— 111 passed (was 108; 3 new backend regressions, plus 9 infile_store_tests.rs).cargo clippy --libclean;cargo fmt --checkclean.file_backend_set_refuses_to_overwrite_an_unparseable_file,file_backend_get_treats_an_unparseable_file_as_empty,file_backend_concurrent_sets_all_survive,lock_serializes_concurrent_holders,lock_survives_the_file_being_replaced,temp_paths_are_unique_per_call.Notes
No wire-surface change: no controllers, no agent tools, no RPC.
README.mdfor the module is updated with the locking rule and the corrupt-file policy.The Medulla side of this (not exporting the core's workspace into spawned coding harnesses in the first place) is a separate PR against
tinyhumansai/medulla.Summary by CodeRabbit
Bug Fixes
Tests