Conversation
crazy-max
reviewed
Sep 21, 2026
Comment on lines
64
to
+69
| if cerr := cw.Close(); cerr != nil { | ||
| bklog.G(ctx).WithError(cerr).Warnf("failed to close writer %q", ref) | ||
| } | ||
| if aerr := sr.cm.ContentStore.Abort(ctx, ref); aerr != nil { | ||
| bklog.G(ctx).WithError(aerr).Warnf("failed to abort writer %q", ref) | ||
| } |
Member
There was a problem hiding this comment.
Since the proxy Close() only calls CloseSend(), can't Abort() still race with server-side closure writing updatedat? The comment should also avoid saying an open data file causes ENOTEMPTY, since Linux allows unlinking open files.
Unique refs still make sense after #7167. Could we add a regression test that keeps the failed writer locked while fallback starts, verifying it succeeds with a different ref?
okhowang
force-pushed
the
fix/snap-id
branch
from
September 21, 2026 08:36
db562c2 to
b868c28
Compare
okhowang
force-pushed
the
fix/snap-id
branch
from
September 21, 2026 08:38
b868c28 to
32e0208
Compare
Reusing sr.ID() as the content store ingest reference makes a following
attempt collide with the previous one, because containerd releases the
ingest lock asynchronously: the gRPC proxy's Close() is a fire-and-forget
CloseSend(), and the local writer unlocks only after its fp.Sync() has
completed. So any failure of the overlay differ, which is followed by a
fallback to the containerd differ using the same reference, turns into a
hard failure:
mount callback failed on .../tmpmounts/containerd-mount...:
failed to open writer: ref moby/1/<id> locked for <d>: unavailable
This is independent of the kernel: it fires whenever the overlay differ
fails for any reason, which is why it is the more important half of the fix.
Generate a reference per attempt instead, keeping the cache record ID as a
prefix so ingests can still be traced back to the record they belong to.
Also abort the ingest when the fallback differs fail, as they do not clean
up an ingest that was opened with an explicit reference.
Additionally close the writer before aborting the ingest. Aborting removes
the ingest directory, while closing the writer still writes to it: the data
file is synced and the updatedat timestamp is rewritten. If the two run
concurrently, that timestamp can reappear after the directory has been
emptied, making the final rmdir fail with "directory not empty". The failed
abort rolls back the metadata transaction that drops the ingest bucket, so
the ingest is left behind in the db and its directory leaks on disk:
failed to abort writer "<ref>" error="unlinkat .../ingest/<digest>:
directory not empty"
This is not caused by the data file being open, as Linux unlinks open files
just fine, and the ordering is best-effort only: with the gRPC proxy Close()
is a CloseSend(), so the server can still be finishing its own close, and
rewriting updatedat, while the abort is handled. Such an abort failure is
only logged.
This ordering is only safe once references are unique, hence both changes
in a single commit.
Add a regression test that keeps the ingest of the failed attempt locked
while the fallback starts, verifying that the fallback succeeds because it
opens a different reference.
Signed-off-by: okhowang(王沛文) <okhowang@tencent.com>
okhowang
force-pushed
the
fix/snap-id
branch
from
September 21, 2026 08:51
32e0208 to
7da0dbc
Compare
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
2 participants
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.
relate #7166
Reusing sr.ID() as the content store ingest reference makes a following attempt collide with the previous one, because containerd releases the ingest lock asynchronously: the gRPC proxy's Close() is a fire-and-forget CloseSend(), and the local writer unlocks only after its fp.Sync() has completed. So any failure of the overlay differ, which is followed by a fallback to the containerd differ using the same reference, turns into a hard failure:
This is independent of the kernel: it fires whenever the overlay differ fails for any reason, which is why it is the more important half of the fix.
Generate a reference per attempt instead, keeping the cache record ID as a prefix so ingests can still be traced back to the record they belong to. Also abort the ingest when the fallback differs fail, as they do not clean up an ingest that was opened with an explicit reference.
Additionally close the writer before aborting the ingest. Aborting while the writer is still open always fails with "directory not empty", which rolls back the metadata transaction removing the ingest bucket and leaks the ingest directory on disk:
This ordering is only safe once references are unique, hence both changes in a single commit.