[PR #392] [MERGED] Clarify speed_up_quit() test usage violates documented safety contract #409

New issue

Closed

opened 2026-10-26 09:50:01 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-26 09:50:01 +00:00

Owner

📋 Pull Request Information

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

Base: main ← Head: copilot/fix-speed-up-quit-testing

📝 Commits (2)

43fd77d Initial plan
558e55c Update speed_up_quit() test comment to clarify exceptional testing usage

📊 Changes

1 file changed (+4 additions, -1 deletions)

View changed files

📝 src/storage/chunk_engine/src/core/engine.rs (+4 -1)

📄 Description

Engine::speed_up_quit() is documented to only be called when the process exits immediately, yet test_engine_normal calls it with a misleading SAFETY comment claiming the test exits afterward—the test process continues running.

Changes

Updated the SAFETY comment at line 801 to explicitly acknowledge:

This is an exceptional case for testing the function itself
Normal usage must only call speed_up_quit() immediately before process exit
The test intentionally violates the safety contract to verify behavior
Memory leaks persist for the remainder of the test process

// Before
// SAFETY: This is a test that exits immediately after this call.
unsafe { engine.speed_up_quit(); }

// After
// SAFETY: This is an exceptional case for testing the function itself.
// In normal usage, speed_up_quit() should ONLY be called immediately before process exit.
// This test intentionally violates that contract to verify the function's behavior,
// but this will cause memory leaks that persist for the remainder of the test process.
unsafe { engine.speed_up_quit(); }

Aligns test documentation with the function's safety contract (lines 175-177) which explicitly warns against calling in non-exit contexts like tests.

Original prompt

This section details on the original issue you should resolve

<issue_title>speed_up_quit() usage in tests contradicts documented safety contract</issue_title>
<issue_description>Hi maintainers 👋
First of all, thank you for the great engine design and extensive test coverage.

While reading the code and tests, I noticed a small but potentially confusing inconsistency regarding Engine::speed_up_quit().

Background

speed_up_quit() is documented as:

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.

However, in test_engine_normal, the function is called here:

// SAFETY: This is a test that exits immediately after this call.
unsafe { engine.speed_up_quit(); }

But the test process does not exit at this point — subsequent test cases continue running in the same process.

Why this might be an issue

Even though this may be practically harmless today (tests are short-lived), it creates a mismatch between:

the strong safety contract documented on speed_up_quit(), and

how it is actually used in tests.

This could be confusing for future contributors, or encourage similar usage in non-exit contexts.

Possible options (happy to follow maintainers’ preference)

Remove the speed_up_quit() call from tests

Gate speed_up_quit() behind #[cfg(not(test))]

Or explicitly document that test usage is an intentional exception

I’m not proposing a specific change yet — just wanted to surface this for discussion and alignment.

Thanks again for the solid engineering work!</issue_description>

<agent_instructions>https://github.com/deepseek-ai/3FS/blob/main/src/storage/chunk_engine/src/core/engine.rs#L802 按照讨论的结论更新这里的注释，表明这是为了测试所做的意外情况，正常使用过程中不应该在进程结束前这样做。</agent_instructions>

Comments on the Issue (you are @copilot in this section)
@SF-Zhou This is an interesting observation 😄. Actually, to verify that calling this function is relatively safe (i.e., it doesn’t cause the process to crash or corrupt data), I need to test it in unit tests. I understand the inconsistency you mentioned with the documentation. I think adding a comment in the unit test would be better, clarifying that the call to speed_up_quit() here is only for testing purposes, and in normal usage you should not call this function before the process ends.

Fixes deepseek-ai/3FS#391

✨ 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/392 **Author:** [@Copilot](https://github.com/apps/copilot-swe-agent) **Created:** 1/22/2026 **Status:** ✅ Merged **Merged:** 1/22/2026 **Merged by:** [@SF-Zhou](https://github.com/SF-Zhou) **Base:** `main` ← **Head:** `copilot/fix-speed-up-quit-testing` --- ### 📝 Commits (2) - [`43fd77d`](https://github.com/deepseek-ai/3FS/commit/43fd77dbdf02383ad17fb41094a3d3c798321c67) Initial plan - [`558e55c`](https://github.com/deepseek-ai/3FS/commit/558e55caa99f070121f2cd3d1b949040d42c1f9b) Update speed_up_quit() test comment to clarify exceptional testing usage ### 📊 Changes **1 file changed** (+4 additions, -1 deletions) <details> <summary>View changed files</summary> 📝 `src/storage/chunk_engine/src/core/engine.rs` (+4 -1) </details> ### 📄 Description `Engine::speed_up_quit()` is documented to only be called when the process exits immediately, yet `test_engine_normal` calls it with a misleading SAFETY comment claiming the test exits afterward—the test process continues running. ## Changes Updated the SAFETY comment at line 801 to explicitly acknowledge: - This is an exceptional case for testing the function itself - Normal usage must only call `speed_up_quit()` immediately before process exit - The test intentionally violates the safety contract to verify behavior - Memory leaks persist for the remainder of the test process ```rust // Before // SAFETY: This is a test that exits immediately after this call. unsafe { engine.speed_up_quit(); } // After // SAFETY: This is an exceptional case for testing the function itself. // In normal usage, speed_up_quit() should ONLY be called immediately before process exit. // This test intentionally violates that contract to verify the function's behavior, // but this will cause memory leaks that persist for the remainder of the test process. unsafe { engine.speed_up_quit(); } ``` Aligns test documentation with the function's safety contract (lines 175-177) which explicitly warns against calling in non-exit contexts like tests.  <details> <summary>Original prompt</summary> > > ---- > > *This section details on the original issue you should resolve* > > <issue_title>speed_up_quit() usage in tests contradicts documented safety contract</issue_title> > <issue_description>Hi maintainers 👋 > First of all, thank you for the great engine design and extensive test coverage. > > While reading the code and tests, I noticed a small but potentially confusing inconsistency regarding Engine::speed_up_quit(). > > Background > > speed_up_quit() is documented as: > > 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. > > However, in test_engine_normal, the function is called here: > > // SAFETY: This is a test that exits immediately after this call. > unsafe { engine.speed_up_quit(); } > > > But the test process does not exit at this point — subsequent test cases continue running in the same process. > > Why this might be an issue > > Even though this may be practically harmless today (tests are short-lived), it creates a mismatch between: > > the strong safety contract documented on speed_up_quit(), and > > how it is actually used in tests. > > This could be confusing for future contributors, or encourage similar usage in non-exit contexts. > > Possible options (happy to follow maintainers’ preference) > > Remove the speed_up_quit() call from tests > > Gate speed_up_quit() behind #[cfg(not(test))] > > Or explicitly document that test usage is an intentional exception > > I’m not proposing a specific change yet — just wanted to surface this for discussion and alignment. > > Thanks again for the solid engineering work!</issue_description> > > <agent_instructions>https://github.com/deepseek-ai/3FS/blob/main/src/storage/chunk_engine/src/core/engine.rs#L802 按照讨论的结论更新这里的注释，表明这是为了测试所做的意外情况，正常使用过程中不应该在进程结束前这样做。</agent_instructions> > > ## Comments on the Issue (you are @copilot in this section) > > <comments> > <comment_new><author>@SF-Zhou</author><body> > This is an interesting observation 😄. Actually, to verify that calling this function is relatively safe (i.e., it doesn’t cause the process to crash or corrupt data), I need to test it in unit tests. I understand the inconsistency you mentioned with the documentation. I think adding a comment in the unit test would be better, clarifying that the call to speed_up_quit() here is only for testing purposes, and in normal usage you should not call this function before the process ends.</body></comment_new> > </comments> > </details>  - Fixes deepseek-ai/3FS#391  --- ✨ 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:50:01 +00:00

closed this issue
added the
pull-request
label

No labels

pull-request

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

deepseek-ai/3FS#409

No description provided.

Rows
Columns