[PR #388] [MERGED] Mark speed_up_quit() as unsafe with safety contract #407

New issue

Closed

opened 2026-10-26 09:49:58 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-26 09:49:58 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/deepseek-ai/3FS/pull/388
Author: @Copilot
Created: 1/20/2026
Status: ✅ Merged
Merged: 1/20/2026
Merged by: @SF-Zhou

Base: main ← Head: copilot/strengthen-speed-up-quit-docs

📝 Commits (2)

7ee291a Initial plan
788ddf1 Mark speed_up_quit as unsafe and add safety documentation

📊 Changes

3 files changed (+13 additions, -4 deletions)

View changed files

📝 src/storage/chunk_engine/src/bin/bench.rs (+2 -1)
📝 src/storage/chunk_engine/src/core/engine.rs (+10 -2)
📝 src/storage/chunk_engine/src/cxx.rs (+1 -1)

📄 Description

Engine::speed_up_quit() intentionally leaks Arc pointers via Arc::into_raw() to avoid shutdown overhead. This is only safe when the process exits immediately—calling it elsewhere (tests with repeated engine creation, hot-reload, refactored shutdown paths) causes operational memory leaks.

Changes

Mark function as unsafe: Forces explicit acknowledgment at call sites
Add # Safety documentation: Documents the "must only call before process exit" contract
Update call sites: Wrap all calls in unsafe blocks with // SAFETY comments explaining why each call is safe

Example

/// # Safety
///
/// This function **must only be called** when the process is guaranteed to exit immediately afterwards.
/// Calling it in any other context (e.g., during a restart, in a test) will cause permanent memory leaks.
pub unsafe fn speed_up_quit(&self) {
    let _ = Arc::into_raw(self.meta_cache.clone());
    let _ = Arc::into_raw(self.writing_list.clone());
}

// At call site:
// SAFETY: This benchmark exits immediately after this call.
unsafe { engine.speed_up_quit(); }

Original prompt

This section details on the original issue you should resolve

<issue_title>[Safety/Docs] Strengthen contract around speed_up_quit() intentional leak</issue_title>
<issue_description>Description
The Engine::speed_up_quit() method intentionally leaks memory for meta_cache and writing_list by calling Arc::into_raw().

pub fn speed_up_quit(&self) {
// There is a memory leak and should only be called when the process exits.
let _ = Arc::into_raw(self.meta_cache.clone());
let _ = Arc::into_raw(self.writing_list.clone());
}

Observation

1.As the current comment states, this method operates under the critical assumption that it is "only called when the process exits."

2.In the current synchronous, single-process shutdown flow, this is an intentional trade-off and not an immediate problem.

Risk & Why Documentation/Guard is Needed
This assumption is fragile and could be easily broken by future code changes, turning this intentional leak into a real operational issue. Concrete future scenarios include:

Tests/Benchmarks: If this method is called in tests or benchmarks where engine instances are created and destroyed repeatedly.

Service Internal Restart: If the "fast quit" logic is reused for a service hot-reload or internal restart mechanism.

Graceful Shutdown Flow Changes: If worker cleanup or shutdown flow is refactored, potentially calling this method on a path where process exit is not guaranteed.

Proposal (Minimal Behavior Change)
To safeguard correct usage, we should make the function's contract much more explicit and robust through documentation and potentially a debug guard.

1.Enhanced Documentation via # Safety Section:

Add a formal # Safety section to the function's doc-comment. This is the standard Rust convention to declare functions that have strict calling preconditions.

Explicitly state that it must only be called on the immediate path to process exit and that misuse causes permanent leaks.

Mention that it's an internal method intended for a single, specific call site (e.g., the process exit handler).

2.Improved Function Name (Optional, but Highly Effective):

Renaming the function to reflect its dangerous contract is the strongest guard. For example: leak_arcs_for_immediate_process_exit_only().

3.Debug Assertion Guard (Optional):

While a guard like debug_assert!(std::process::id() == ...) is complex, a simple debug_assert! or conditional logging under cfg!(debug_assertions) could help catch misuse during development.

Example Code

/// Intentionally leaks Arc pointers to avoid shutdown overhead.
///
/// # Safety
///
/// This function must only be called when the process is guaranteed to exit immediately afterwards.
/// Calling it in any other context (e.g., during a restart, in a test) will cause permanent memory leaks of the Arc-managed data.
/// This is an internal method intended to be used exactly once in the process exit path.
pub fn speed_up_quit(&self) {
// There is a memory leak and should only be called when the process exits.
let _ = Arc::into_raw(self.meta_cache.clone());
let _ = Arc::into_raw(self.writing_list.clone());
}

These clarifications will help prevent subtle bugs as the codebase evolves. What are your thoughts?</issue_description>

<agent_instructions>make this function as unsafe at https://github.com/deepseek-ai/3FS/blob/main/src/storage/chunk_engine/src/core/engine.rs#L171 and add SAFETY comment.</agent_instructions>

Comments on the Issue (you are @copilot in this section)
@SF-Zhou Thanks for the detailed issue. I agree with your analysis. In the short term, I’ll mark the function as `unsafe` and add a `// SAFETY` comment to make the contract explicit. Appreciate the contribution.

Fixes deepseek-ai/3FS#387

✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/deepseek-ai/3FS/pull/388 **Author:** [@Copilot](https://github.com/apps/copilot-swe-agent) **Created:** 1/20/2026 **Status:** ✅ Merged **Merged:** 1/20/2026 **Merged by:** [@SF-Zhou](https://github.com/SF-Zhou) **Base:** `main` ← **Head:** `copilot/strengthen-speed-up-quit-docs` --- ### 📝 Commits (2) - [`7ee291a`](https://github.com/deepseek-ai/3FS/commit/7ee291a845b2b23e6945f0e0598bcb3b0dbcb549) Initial plan - [`788ddf1`](https://github.com/deepseek-ai/3FS/commit/788ddf160abe89b253d5f11736cdb5b43eac80f0) Mark speed_up_quit as unsafe and add safety documentation ### 📊 Changes **3 files changed** (+13 additions, -4 deletions) <details> <summary>View changed files</summary> 📝 `src/storage/chunk_engine/src/bin/bench.rs` (+2 -1) 📝 `src/storage/chunk_engine/src/core/engine.rs` (+10 -2) 📝 `src/storage/chunk_engine/src/cxx.rs` (+1 -1) </details> ### 📄 Description `Engine::speed_up_quit()` intentionally leaks `Arc` pointers via `Arc::into_raw()` to avoid shutdown overhead. This is only safe when the process exits immediately—calling it elsewhere (tests with repeated engine creation, hot-reload, refactored shutdown paths) causes operational memory leaks. ## Changes - **Mark function as `unsafe`**: Forces explicit acknowledgment at call sites - **Add `# Safety` documentation**: Documents the "must only call before process exit" contract - **Update call sites**: Wrap all calls in `unsafe` blocks with `// SAFETY` comments explaining why each call is safe ## Example ```rust /// # Safety /// /// This function **must only be called** when the process is guaranteed to exit immediately afterwards. /// Calling it in any other context (e.g., during a restart, in a test) will cause permanent memory leaks. pub unsafe fn speed_up_quit(&self) { let _ = Arc::into_raw(self.meta_cache.clone()); let _ = Arc::into_raw(self.writing_list.clone()); } // At call site: // SAFETY: This benchmark exits immediately after this call. unsafe { engine.speed_up_quit(); } ```  <details> <summary>Original prompt</summary> > > ---- > > *This section details on the original issue you should resolve* > > <issue_title>[Safety/Docs] Strengthen contract around speed_up_quit() intentional leak</issue_title> > <issue_description>Description > The Engine::speed_up_quit() method intentionally leaks memory for meta_cache and writing_list by calling Arc::into_raw(). > > pub fn speed_up_quit(&self) { > // There is a memory leak and should only be called when the process exits. > let _ = Arc::into_raw(self.meta_cache.clone()); > let _ = Arc::into_raw(self.writing_list.clone()); > } > > Observation > > 1.As the current comment states, this method operates under the critical assumption that it is "only called when the process exits." > > 2.In the current synchronous, single-process shutdown flow, this is an intentional trade-off and not an immediate problem. > > Risk & Why Documentation/Guard is Needed > This assumption is fragile and could be easily broken by future code changes, turning this intentional leak into a real operational issue. Concrete future scenarios include: > > Tests/Benchmarks: If this method is called in tests or benchmarks where engine instances are created and destroyed repeatedly. > > Service Internal Restart: If the "fast quit" logic is reused for a service hot-reload or internal restart mechanism. > > Graceful Shutdown Flow Changes: If worker cleanup or shutdown flow is refactored, potentially calling this method on a path where process exit is not guaranteed. > > Proposal (Minimal Behavior Change) > To safeguard correct usage, we should make the function's contract much more explicit and robust through documentation and potentially a debug guard. > > 1.Enhanced Documentation via # Safety Section: > > Add a formal # Safety section to the function's doc-comment. This is the standard Rust convention to declare functions that have strict calling preconditions. > > Explicitly state that it must only be called on the immediate path to process exit and that misuse causes permanent leaks. > > Mention that it's an internal method intended for a single, specific call site (e.g., the process exit handler). > > 2.Improved Function Name (Optional, but Highly Effective): > > Renaming the function to reflect its dangerous contract is the strongest guard. For example: leak_arcs_for_immediate_process_exit_only(). > > 3.Debug Assertion Guard (Optional): > > While a guard like debug_assert!(std::process::id() == ...) is complex, a simple debug_assert! or conditional logging under cfg!(debug_assertions) could help catch misuse during development. > > Example Code > > /// Intentionally leaks Arc pointers to avoid shutdown overhead. > /// > /// # Safety > /// > /// This function **must only be called** when the process is guaranteed to exit immediately afterwards. > /// Calling it in any other context (e.g., during a restart, in a test) will cause permanent memory leaks of the Arc-managed data. > /// This is an internal method intended to be used exactly once in the process exit path. > pub fn speed_up_quit(&self) { > // There is a memory leak and should only be called when the process exits. > let _ = Arc::into_raw(self.meta_cache.clone()); > let _ = Arc::into_raw(self.writing_list.clone()); > } > > These clarifications will help prevent subtle bugs as the codebase evolves. What are your thoughts?</issue_description> > > <agent_instructions>make this function as unsafe at https://github.com/deepseek-ai/3FS/blob/main/src/storage/chunk_engine/src/core/engine.rs#L171 and add SAFETY comment.</agent_instructions> > > ## Comments on the Issue (you are @copilot in this section) > > <comments> > <comment_new><author>@SF-Zhou</author><body> > Thanks for the detailed issue. I agree with your analysis. In the short term, I’ll mark the function as `unsafe` and add a `// SAFETY` comment to make the contract explicit. Appreciate the contribution.</body></comment_new> > </comments> > </details>  - Fixes deepseek-ai/3FS#387  --- ✨ Let Copilot coding agent [set things up for you](https://github.com/deepseek-ai/3FS/issues/new?title=✨+Set+up+Copilot+instructions&body=Configure%20instructions%20for%20this%20repository%20as%20documented%20in%20%5BBest%20practices%20for%20Copilot%20coding%20agent%20in%20your%20repository%5D%28https://gh.io/copilot-coding-agent-tips%29%2E%0A%0A%3COnboard%20this%20repo%3E&assignees=copilot) — coding agent works faster and does higher quality work when set up for your repo. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-26 09:49:58 +00:00