From 9c4c28da9d5fed1f0c75ff0abd5e247d9fd32810 Mon Sep 17 00:00:00 2001 From: Daniel Rainer Date: Fri, 8 Aug 2025 19:53:36 +0200 Subject: [PATCH 1/4] Use updated file id This restores behavior from before f438e80f9b9dcd6a7d31e60582c35f02ef6569c5. The file id changes when data is written to the file, so it needs to be updated with data obtained after the updates to the file are completed. --- src/history.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/history.rs b/src/history.rs index 4f66d2957..c303ff5c3 100644 --- a/src/history.rs +++ b/src/history.rs @@ -711,7 +711,7 @@ fn save_internal_via_appending(&mut self, history_path: &wstr) -> std::io::Resul // write. // We don't update `self.file_contents` since we only appended to the file, and everything we // appended remains in our new_items - self.history_file_id = file_id; + self.history_file_id = file_id_for_file(locked_history_file.get()); drop(locked_history_file); From 20e268a75c31f6677149ba9341bd51d6e9a46ba6 Mon Sep 17 00:00:00 2001 From: Daniel Rainer Date: Fri, 8 Aug 2025 19:56:52 +0200 Subject: [PATCH 2/4] Clear file state earlier This should not result in behavioral changes in the code, but it eliminates some redundant variables and is a step in refactoring the function such that early returns via `?` become sound. Remove the `drop` since the lock will be dropped at this point anyway, there is no need to be explicit about it. --- src/history.rs | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/src/history.rs b/src/history.rs index c303ff5c3..62094b42a 100644 --- a/src/history.rs +++ b/src/history.rs @@ -661,8 +661,10 @@ fn save_internal_via_appending(&mut self, history_path: &wstr) -> std::io::Resul LockedFile::new(LockingMode::Exclusive(WriteMethod::Append), history_path)?; // Check if the file was modified since it was last read. - let file_id = file_id_for_file(locked_history_file.get()); - let file_changed = file_id != self.history_file_id; + // If someone has replaced the file, forget our file state. + if file_id_for_file(locked_history_file.get()) != self.history_file_id { + self.clear_file_state(); + } // We took the exclusive lock. Append to the file. // Note that this is sketchy for a few reasons: @@ -713,13 +715,6 @@ fn save_internal_via_appending(&mut self, history_path: &wstr) -> std::io::Resul // appended remains in our new_items self.history_file_id = file_id_for_file(locked_history_file.get()); - drop(locked_history_file); - - // If someone has replaced the file, forget our file state. - if file_changed { - self.clear_file_state(); - } - res } From b0ae11e769ad5877c53e16640acabc5913d8ff6e Mon Sep 17 00:00:00 2001 From: Daniel Rainer Date: Fri, 8 Aug 2025 20:14:06 +0200 Subject: [PATCH 3/4] Write only once when appending items Building a buffer in advance and writing it once all items are serialized into the buffer makes for simpler code, makes it easier to ensure that `self.first_unwritten_new_item_index` is only updated if writing succeeded, and it actually matches the previous behavior of the code in most realistic cases, since previously there was only more than one `write_all` call if the serialized items took up more than `HISTORY_OUTPUT_BUFFER_SIZE` bytes (64 * 1024), which seems unlikely to occur during normal use, where mostly just a single item will be appended. --- src/history.rs | 24 +++++++----------------- 1 file changed, 7 insertions(+), 17 deletions(-) diff --git a/src/history.rs b/src/history.rs index 62094b42a..01c44b042 100644 --- a/src/history.rs +++ b/src/history.rs @@ -683,30 +683,20 @@ fn save_internal_via_appending(&mut self, history_path: &wstr) -> std::io::Resul // So far so good. Write all items at or after first_unwritten_new_item_index. Note that we // write even a pending item - pending items are ignored by history within the command // itself, but should still be written to the file. - // TODO: consider filling the buffer ahead of time, so we can just lock, splat, and unlock? - let mut res = Ok(()); // Use a small buffer size for appending, we usually only have 1 item let mut buffer = Vec::new(); - while self.first_unwritten_new_item_index < self.new_items.len() { - let item = &self.new_items[self.first_unwritten_new_item_index]; + let mut new_first_index = self.first_unwritten_new_item_index; + while new_first_index < self.new_items.len() { + let item = &self.new_items[new_first_index]; if item.should_write_to_disk() { append_history_item_to_buffer(item, &mut buffer); - res = flush_to_file( - &mut buffer, - locked_history_file.get_mut(), - HISTORY_OUTPUT_BUFFER_SIZE, - ); - if res.is_err() { - break; - } } // We wrote or skipped this item, hooray. - self.first_unwritten_new_item_index += 1; + new_first_index += 1; } - if res.is_ok() { - res = flush_to_file(&mut buffer, locked_history_file.get_mut(), 0); - } + flush_to_file(&mut buffer, locked_history_file.get_mut(), 0)?; + self.first_unwritten_new_item_index = new_first_index; // Since we just modified the file, update our history_file_id to match its current state // Otherwise we'll think the file has been changed by someone else the next time we go to @@ -715,7 +705,7 @@ fn save_internal_via_appending(&mut self, history_path: &wstr) -> std::io::Resul // appended remains in our new_items self.history_file_id = file_id_for_file(locked_history_file.get()); - res + Ok(()) } /// Saves history. From 5de43a4e861b36eace391a147c4acb920132a6df Mon Sep 17 00:00:00 2001 From: Daniel Rainer Date: Fri, 8 Aug 2025 20:20:23 +0200 Subject: [PATCH 4/4] Call `fsync` after appending to history file This is an attempt to eliminate history file corruption as described in https://github.com/fish-shell/fish-shell/issues/10300. In particular, issues raised in https://github.com/fish-shell/fish-shell/issues/10300#issuecomment-2567063654 can benefit from such synchronization. --- src/history.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/history.rs b/src/history.rs index 01c44b042..cdd9abb9e 100644 --- a/src/history.rs +++ b/src/history.rs @@ -48,6 +48,7 @@ expand::{expand_one, ExpandFlags}, fds::wopen_cloexec, flog::{FLOG, FLOGF}, + fs::fsync, history::file::{append_history_item_to_buffer, HistoryFileContents}, io::IoStreams, operation_context::{OperationContext, EXPANSION_LIMIT_BACKGROUND}, @@ -696,6 +697,7 @@ fn save_internal_via_appending(&mut self, history_path: &wstr) -> std::io::Resul } flush_to_file(&mut buffer, locked_history_file.get_mut(), 0)?; + fsync(locked_history_file.get())?; self.first_unwritten_new_item_index = new_first_index; // Since we just modified the file, update our history_file_id to match its current state