Skip to content

[Bugfix][Store] Isolate fileread worker count assertions from runtime threads - #4210

Open
Aionw wants to merge 1 commit into
kvcache-ai:mainfrom
Aionw:fix/fileread-worker-count-test
Open

Aionw wants to merge 1 commit into
kvcache-ai:mainfrom
Aionw:fix/fileread-worker-count-test

Conversation

@Aionw

@Aionw Aionw commented Sep 18, 2026

Copy link
Copy Markdown
Collaborator

Description

Fix the flaky FilereadWorkerPoolTest.AcceptsTypedTrailingWhitespaceAndCaches introduced in #4166 and observed in the unrelated #4161 CI run:
https://github.com/kvcache-ai/Mooncake/actions/runs/35190325962/job/105191941422?pr=4161

The test counts every thread under /proc/self/task and requires the process-wide count to return to its original baseline after pool destruction. CI enables USE_ETCD and STORE_USE_ETCD, loading a Go shared library whose runtime can create unrelated background threads asynchronously. Joining all pool workers does not restore that process-wide baseline.

Add a narrow friend test peer to inspect the pool's own worker vector. Preserve both assertions: "2 " creates two workers, and changing the environment to "3" still creates two workers because the configuration is cached. Keep both construction/destruction scopes; remove process-wide thread polling and the now-unnecessary Linux-only guard. No production behavior or public API change.

Searched existing issues/PRs for the test name, fileread_worker_pool, and WaitForProcessThreadCount; no dedicated fix found. #4179 reorganizes store paths but does not address this assertion race.

Module

  • Mooncake Store (mooncake-store)

Type of Change

  • Bug fix

How Has This Been Tested?

Test commands:

CCACHE_DIR="$HOME/.cache/ccache/mooncake" \
CCACHE_BASEDIR="$(git rev-parse --show-toplevel)" \
cmake --build build --target fileread_worker_pool_test \
  fileread_worker_pool_config_test transfer_task_test -j 12

ctest --test-dir build \
  -R '^(fileread_worker_pool_config_test|fileread_worker_pool_test|transfer_task_test)$' \
  --output-on-failure
ctest --test-dir build -R '^fileread_worker_pool_test$' \
  --repeat until-fail:1000 --output-on-failure
# ETCD_WRAPPER_SO points to an existing local Go-built libetcd_wrapper.so.
LD_PRELOAD="$ETCD_WRAPPER_SO" ctest --test-dir build \
  -R '^fileread_worker_pool_test$' --repeat until-fail:1000 --output-on-failure

# Use clang-format 20 for the repository's version-pinned formatting hook.
uv tool run --from clang-format==20.1.8 sh -c \
  './scripts/code_format.sh --check --staged'
uv tool run --from clang-format==20.1.8 sh -c \
  'prek run --files mooncake-store/include/transfer_task.h mooncake-store/tests/fileread_worker_pool_test.cpp'

Test results:

  • Unit tests pass: all three focused CTest targets.
  • Integration tests pass (not run).
  • Manual testing done: controlled before/after runtime-loading experiment.

Before the fix, the local CPU build (etcd disabled) passed 100/100 separate executions. Loading an existing libetcd_wrapper.so with LD_PRELOAD reproduced 13 failures in 100 separate executions: nine at line 76 (the exact CI assertion), four at line 70 (the equivalent first-pool assertion). After the fix, both plain and Go-runtime-preloaded CTest repetitions passed 1000/1000. This is a controlled reproduction, not a full recreation of the CI build; CUDA and etcd build options remain disabled locally. Existing build configuration uses ccache launchers and disables sccache.

All applicable touched-file hooks passed with prek (pre-commit unavailable) and clang-format 20.1.8. The initial formatting attempt with the host's clang-format 22 was rejected by the version-pinned script; rerunning with 20 passed.

Checklist

  • I have performed a self-review of my own code (human review pending; draft PR).
  • I have formatted my code using ./scripts/code_format.sh.
  • I have run pre-commit on the files changed in this PR and all hooks pass (prek equivalent).
  • I have updated the documentation (not applicable: test-only behavior).
  • I have added tests to prove my changes are effective (updated existing regression test and repeated the reproducer).
  • For changes >500 LOC: I have filed an RFC issue (not applicable).

AI Assistance Disclosure

  • No AI tools were used.
  • AI tools were used.

An OpenAI coding assistant investigated CI logs, reproduced the failure, implemented this focused fix, and ran the checks. This PR is intentionally a draft pending the human submitter's line-by-line review and confirmation that they understand and can defend the change end to end.

@fcczzz fcczzz left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants