Repository navigation
runsc: don't create a lock file when loading a missing container - #16255
Open
NahumLitvin wants to merge 1 commit into
Open
NahumLitvin wants to merge 1 commit into
NahumLitvin wants to merge 1 commit into
Conversation
Contributor
Author
|
@konstantin-s-bogom long time no reviewed :) |
Contributor
Author
|
@tamird please review |
Contributor
At a glance I don't love papering over the race but I'll have clanker review later today. |
load() takes the container lock through gofrs/flock, which opens the lock file with O_CREATE. When the state file is gone, the read fails with ENOENT and the newly created lock file stays behind. This happens in two ways. Load globs for the state file and then locks, so a concurrent delete in between leaves a lock file. An Exact load of an ID that has no state file leaves one with no race at all. These files pile up in the runsc root, and every later Load globs that directory. We saw a root with 176 containers and 32k orphan lock files, where each runsc call spent about 45ms in the glob. Open the lock file without O_CREATE in load(), and report a missing lock file as os.ErrNotExist, like a missing state file. LockForNew and the other lock() callers still create it. This needs gofrs/flock v0.11.0 for SetFlag. Assisted-by: Claude Code Signed-off-by: Nahum Litvin <nahuml@wix.com>
NahumLitvin
force-pushed
the
runsc-state-lock-leak
branch
from
October 24, 2026 12:28
9bddf83 to
ecbf248
Compare
NahumLitvin
changed the title
Contributor
Author
|
@tamird giving it my best try :) reworked it so load() never creates the lock file. the race can still happen, it just returns not-found now with nothing left on disk. also turned out an Exact load of a missing id leaked with no race at all, added a test for it. |
Collaborator
|
I'll be sending a PR shortly that refactors state file loading and avoids taking a lock at all for read-only operations (e.g. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
We found a runsc root on one of our nodes with 176 state files and 32,062
.lockfiles. All but 183 of the lock files had no state file, and each orphan belonged to a different, already deleted pod. Everyrunsc stateorevents --statson that node took about 60ms, of which roughly 45ms was the glob infindContainerIDwalking the directory. Copying 32k empty lock files into a test root tookrunsc statefrom about 60ms to 300ms.The cause is that
load()creates the lock file. It locks through gofrs/flock, which opens withO_CREATE, so loading a container whose state file is gone leaves a new lock file behind. The common trigger is a race:Loadglobs for the state file, a concurrentrunsc deletedestroys the container, and thenload()recreates the lock. AnExactload of a missing ID does the same with no race at all. On our nodes the leak went from a handful a day to about 12k a day per node, because each leaked file makes the glob, and so the race window, longer.This change opens the lock file without
O_CREATEinload(). A missing lock file now reads asos.ErrNotExist, the same as a missing state file, sorunsc delete --forceandrestorekeep their not-found handling. A load can still lose to a delete. It now returns not-found without leaving anything on disk.LockForNewand the otherlock()callers still create the file.SetFlagneeds gofrs/flock v0.11.0, so this bumps it from v0.8.0. The releases in between only add Go modules, Illumos support and the options.TestLoadAfterDestroyruns the racy order andTestLoadExactMissingloads a missing ID. Both fail on master and pass with the change. To run them:bazel test //runsc/container:state_file_test.I used Claude Code to help investigate and write this, and I reviewed and tested the result.