[GH-ISSUE #391] speed_up_quit() usage in tests contradicts documented safety contract #229

New issue

Closed

opened 2026-10-26 09:47:27 +00:00 by bsakaguchi · 1 comment

bsakaguchi commented

2026-10-26 09:47:27 +00:00

Owner

Originally created by @red1239109-cmd on GitHub (Jan 22, 2026).
Original GitHub issue: https://github.com/deepseek-ai/3FS/issues/391

Originally assigned to: @SF-Zhou, @Copilot on GitHub.

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!

Originally created by @red1239109-cmd on GitHub (Jan 22, 2026). Original GitHub issue: https://github.com/deepseek-ai/3FS/issues/391 Originally assigned to: @SF-Zhou, @Copilot on GitHub. 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!

bsakaguchi closed this issue

2026-10-26 09:47:28 +00:00

bsakaguchi commented

2026-10-26 09:47:30 +00:00

Author

Owner

@SF-Zhou commented on GitHub (Jan 22, 2026):

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.

@SF-Zhou commented on GitHub (Jan 22, 2026): 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.

bsakaguchi referenced this issue

2026-10-26 09:48:48 +00:00

[PR #231] [MERGED] fix OfflineTargetReq.targetId in ResyncWorker, fix #229 #337