refactor: Move the file when doing Dump for FileSharedMMap

[zhanglei1949]

When dumping for a FileSharedMMap, we just move the underlying file to a new position.
It is OK since the file itself must be a copy of other files.

Fix #208

[greptile-apps]

Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.

[qodo-code-review]

ⓘ You are approaching your monthly quota for Qodo. Upgrade your plan

Review Summary by Qodo

Optimize FileSharedMMap::Dump() with atomic rename fallback

✨ Enhancement

Walkthroughs

Description

• Optimize FileSharedMMap::Dump() to use atomic rename when possible
• Attempt fast O(1) rename on same filesystem before copying
• Fall back to copy_file for cross-filesystem moves
• Add Close() calls to properly release resources after dump operations

Diagram

flowchart LR
  A["Dump called"] --> B{"path_.empty()?"}
  B -->|Yes| C["Call MMapContainer::Dump"]
  C --> D["Close"]
  B -->|No| E["Sync"]
  E --> F{"path == path_?"}
  F -->|Yes| G["Close"]
  F -->|No| H["Save src_path"]
  H --> I["Close"]
  I --> J{"Try rename"}
  J -->|Success| K["Return"]
  J -->|EXDEV| L["Copy file"]
  L --> M["Unlink source"]
  J -->|Other error| N["Throw exception"]

Loading

File Changes

1. src/storages/container/file_mmap_container.cc ✨ Enhancement +20/-11

Optimize dump with atomic rename and proper cleanup

• Replace file copy operation with atomic rename as primary strategy
• Add Close() calls after Dump operations to release mmap resources
• Implement cross-filesystem fallback using copy_file + unlink when rename fails with EXDEV
• Improve error handling by throwing exception for rename failures other than EXDEV

src/storages/container/file_mmap_container.cc

---

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (3)   📘 Rule violations (0)   📎 Requirement gaps (0)   🖥 UI issues (0)   🎨 UX Issues (0)

🐞\ ≡ Correctness (1) ☼ Reliability (1) ⚙ Maintainability (1)

1. Missing errno/rename headers 🐞 ≡

Description

FileSharedMMap::Dump uses ::rename(), errno, and EXDEV but
src/storages/container/file_mmap_container.cc does not include headers that guarantee these
declarations, which can cause compilation failures on stricter toolchains.

Code

src/storages/container/file_mmap_container.cc[R150-158]

+  // Try atomic rename first. On the same filesystem this is O(1) and
+  // involves no data copying — only a directory-entry update.
+  if (::rename(src_path.c_str(), path.c_str()) == 0) {
+    return;
+  }
+
+  if (errno != EXDEV) {
+    THROW_IO_EXCEPTION("Failed to rename file: " + src_path + " -> " + path);
  }
-  Close();

Evidence

The implementation now references ::rename(), errno, and EXDEV, but the translation unit’s includes
do not include <cstdio>/<stdio.h> or <cerrno>/<errno.h>, so these identifiers are not guaranteed to
be declared.

src/storages/container/file_mmap_container.cc[150-158]
src/storages/container/file_mmap_container.cc[16-26]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`FileSharedMMap::Dump()` uses `::rename()`, `errno`, and `EXDEV` but the `.cc` file doesn't include headers that guarantee their declarations, which can break compilation depending on platform/toolchain.

### Issue Context
This was introduced by the refactor that added a rename-first move path.

### Fix Focus Areas
- src/storages/container/file_mmap_container.cc[16-26]
- src/storages/container/file_mmap_container.cc[150-158]

### Proposed fix
Add explicit includes at the top of `file_mmap_container.cc`, e.g.:
- `#include <cstdio>` (for `::rename`)
- `#include <cerrno>` (for `errno`/`EXDEV`)
(or the C equivalents `<stdio.h>` / `<errno.h>`).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Unchecked unlink failure 🐞 ☼

Description

On the EXDEV fallback path, Dump() ignores the return value of ::unlink, so a failed delete silently
leaves the source backing file behind even though Dump intends to move/remove it.

Code

src/storages/container/file_mmap_container.cc[R160-164]

+  // Cross-filesystem fallback: copy then remove the source.
+  // copy_file tries copy_file_range (kernel-side, no userspace buffer) first,
+  // then falls back to a 64 KB read/write loop.
+  file_utils::copy_file(src_path, path, /*overwrite=*/true);
+  ::unlink(src_path.c_str());

Evidence

After copying to the destination, the code calls ::unlink() without checking its return value or
errno, so the cleanup step can fail silently.

src/storages/container/file_mmap_container.cc[160-164]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
In the cross-filesystem fallback, `Dump()` does `copy_file()` then `::unlink()` but ignores unlink failure. This can leave the original backing file in place without notifying the caller.

### Issue Context
The refactor changes Dump semantics from copy to move; for EXDEV it implements move as copy+delete. If delete fails, the move is incomplete.

### Fix Focus Areas
- src/storages/container/file_mmap_container.cc[160-164]

### Proposed fix
Check `::unlink(src_path.c_str())` return value:
- If it returns non-zero, throw `THROW_IO_EXCEPTION` (ideally include `errno`/`strerror(errno)` in the message).
- Optionally consider whether to remove the destination file on failure to avoid ending up with two copies (depends on desired semantics).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Stale Dump documentation 🐞 ⚙

Description

The FileSharedMMap::Dump doc comment still describes copy_file + fallback-to-fwrite and
inode-independence, but the implementation now renames/moves the backing file and no longer falls
back, which can mislead callers and future maintenance.

Code

src/storages/container/file_mmap_container.cc[R146-164]

+  // Save path before Close() clears it.
+  std::string src_path = std::move(path_);
+  Close();  // munmap; all dirty pages already flushed by Sync() above.
+
+  // Try atomic rename first. On the same filesystem this is O(1) and
+  // involves no data copying — only a directory-entry update.
+  if (::rename(src_path.c_str(), path.c_str()) == 0) {
+    return;
+  }
+
+  if (errno != EXDEV) {
+    THROW_IO_EXCEPTION("Failed to rename file: " + src_path + " -> " + path);
  }
-  Close();
+
+  // Cross-filesystem fallback: copy then remove the source.
+  // copy_file tries copy_file_range (kernel-side, no userspace buffer) first,
+  // then falls back to a 64 KB read/write loop.
+  file_utils::copy_file(src_path, path, /*overwrite=*/true);
+  ::unlink(src_path.c_str());

Evidence

The header comment explicitly documents a copy-based dump with fallback behavior, while the
implementation is now rename-first (move) with EXDEV copy+unlink and no catch/fallback path.

include/neug/storages/container/file_mmap_container.h[65-78]
src/storages/container/file_mmap_container.cc[146-164]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`FileSharedMMap::Dump()` documentation no longer matches behavior after the move-to-rename refactor, and it still claims inode-independence and fallback-to-fwrite.

### Issue Context
Callers/readers may rely on or be guided by these comments when reasoning about snapshot aliasing and failure handling.

### Fix Focus Areas
- include/neug/storages/container/file_mmap_container.h[65-78]
- src/storages/container/file_mmap_container.cc[146-164]

### Proposed fix
Revise the comment to describe the new behavior:
- `Sync()` then attempt atomic `rename()` when possible.
- On `EXDEV`, perform `copy_file()` then `unlink()`.
- Clarify that this is a move (source path removed) rather than producing an independent inode copy, and clarify what exceptions are thrown on failures.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

ⓘ The new review experience is currently in Beta. Learn more