mirror of
https://github.com/deepseek-ai/3FS.git
synced 2026-11-05 00:47:47 +00:00
[GH-ISSUE #387] [Safety/Docs] Strengthen contract around speed_up_quit() intentional leak #228
Labels
No labels
pull-request
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
deepseek-ai/3FS#228
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Originally created by @red1239109-cmd on GitHub (Jan 20, 2026).
Original GitHub issue: https://github.com/deepseek-ai/3FS/issues/387
Originally assigned to: @SF-Zhou, @Copilot on GitHub.
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?
@SF-Zhou commented on GitHub (Jan 20, 2026):
Thanks for the detailed issue. I agree with your analysis. In the short term, I’ll mark the function as
unsafeand add a// SAFETYcomment to make the contract explicit. Appreciate the contribution.