Skip to content

fix(distillfs): prevent duplicate watcher panic - #47

Merged
tianyuzhou95 merged 1 commit into
inclusionAI:mainfrom
tianyuzhou95:codex/fix-distillfs-watcher-panic
Sep 3, 2026
Merged

fix(distillfs): prevent duplicate watcher panic#47
tianyuzhou95 merged 1 commit into
inclusionAI:mainfrom
tianyuzhou95:codex/fix-distillfs-watcher-panic

Conversation

@tianyuzhou95

Copy link
Copy Markdown
Collaborator

Summary

  • Claim watcher ownership atomically so repeated starts cannot launch multiple watcher goroutines for one daemon lifecycle.
  • Pass the current stop and completion channels into the watcher so cleanup cannot follow later mutations of the Daemon fields.
  • Add a regression test that repeatedly starts one watcher and verifies a single clean shutdown.

Reproduction

On an unmodified main checkout at 5970730a, the regression test deterministically started multiple watchers and failed with panic: close of closed channel from distillfs.(*Daemon).watch.func1, matching the production stack.

With this change applied, the same test passed 100 consecutive runs and 100 consecutive runs under the race detector on the test node.

Verification

  • go test -race ./pkg/imagemanager/distillfs -run '^TestDaemon_StartWatchOnlyOnce$' -count=100
  • make check-fmt
  • make vet
  • make test

Start each daemon watcher with an atomic ownership claim so repeated
start requests cannot launch multiple goroutines for the same lifecycle.
Capture the completion and stop channels when the watcher starts so its
cleanup cannot follow later mutations of the daemon fields.

Add a regression test that starts the watcher repeatedly and verifies a
single clean shutdown instead of a close-of-closed-channel panic.

Signed-off-by: Tianyu Zhou <albert.zty@antgroup.com>
@tianyuzhou95
tianyuzhou95 requested a review from hky1999 September 3, 2026 03:39

@hky1999 hky1999 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 — approving.

Verified beyond the diff itself: TestDaemon_StartWatchOnlyOnce passes -race -count=20 locally, and I also cherry-picked this commit onto a branch carrying #42 and ran the full Firecracker E2E suite there (including the daemon kill -9 recovery cases) — no regressions.

The fix is the minimal correct shape: CompareAndSwap makes watcher ownership atomic so repeated startWatch calls can never stack a second goroutine, and snapshotting stopChan/kickStop into the goroutine means the deferred cleanup closes exactly the channels this watcher was started with — later Daemon field mutations can no longer divert it onto a different channel, which is precisely the panic: close of closed channel production stack. The exiting watcher still resets watcherActive before the remount path can reacquire it, so the single-watcher invariant holds across restart.

CI is 13/13 green on c599e784b, the base is the current main head, and there is no overlap with #42. Ready to merge from our side — landing this before #42's rebase also removes one moving part from it.

@tianyuzhou95
tianyuzhou95 merged commit 3441008 into inclusionAI:main Sep 3, 2026
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants