Skip to content

repo-commit: preserve existing .file object inode on consume commit#3603

Open
electricface wants to merge 1 commit into
ostreedev:mainfrom
electricface:fix-consume-inode-stability
Open

repo-commit: preserve existing .file object inode on consume commit#3603
electricface wants to merge 1 commit into
ostreedev:mainfrom
electricface:fix-consume-inode-stability

Conversation

@electricface

@electricface electricface commented Jun 29, 2026

Copy link
Copy Markdown

repo-commit: preserve existing object inode when staging to objects/

When ostree commit --consume is used and an object with the same
checksum already exists in objects/, rename_pending_loose_objects() was
unconditionally renaming the staging copy over it.  On Linux, renameat(2)
atomically replaces the destination for two regular files, silently
changing the inode of the existing repo object.

Fix this by checking whether the object already exists in objects/
before renaming.  If it does, the content is identical by definition
(the object store is content-addressed by SHA256), so we can simply
unlink the staging copy and keep the existing object with its original
inode.

Exception: .commitmeta objects are keyed by commit checksum rather than
their own content, so they can be updated in place (e.g. when GPG
signatures are added or deleted via ostree gpg-sign).  These are always
renamed unconditionally.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request modifies rename_pending_loose_objects in src/libostree/ostree-repo-commit.c to preserve existing .file objects and their inodes instead of replacing them with staging copies when they already exist in the objects directory. The review feedback correctly identifies a critical bug where errno is not reset before calling glnx_fstatat_allow_noent, which could lead to an incorrect evaluation of file existence if errno was previously set to ENOENT.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread src/libostree/ostree-repo-commit.c Outdated
@electricface electricface force-pushed the fix-consume-inode-stability branch from da62630 to 96a6854 Compare June 29, 2026 08:17
Comment thread src/libostree/ostree-repo-commit.c Outdated
Comment on lines +1808 to +1809
* All other object types (.commit, .dirtree, .dirmeta, .commitmeta,
* etc.) always go through rename so their content can be updated.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

All objects are immutable, it does not make sense to me to just special case file objects.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Except for commitmeta, but...I don't think commitmeta goes through staging at all.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thanks for the review! I wanted to follow up on the .commitmeta point, though I may be misreading the code — could you help confirm?

Looking at ostree_repo_write_commit_detached_metadata() in ostree-repo-commit.c:

int dest_dfd;
if (self->in_transaction)
dest_dfd = self->commit_stagedir.fd;
else
dest_dfd = self->objects_dir_fd;

It seems like when a pull is in progress (which runs inside a transaction), .commitmeta is written to the staging directory and would therefore go through rename_pending_loose_objects(). This is also consistent with the test failure we observed: without the .commitmeta exception, test-commit-sign.sh test 7 ("pull sig deleted") failed because the old .commitmeta in objects/ was preserved and the deleted GPG signature was never removed on the client side.

So my understanding is that .commitmeta does go through staging — at least during a pull. But I might be missing something about the overall flow. Could you take a look and let me know if I'm wrong?

As for the broader point about applying the optimization to all immutable object types (not just .file): that makes sense to me, and the current version does exactly that — all types except .commitmeta get the "skip rename if already exists" treatment. Happy to adjust if you see it differently.

Comment thread src/libostree/ostree-repo-commit.c Outdated
@electricface electricface force-pushed the fix-consume-inode-stability branch from 96a6854 to 11b7327 Compare June 30, 2026 12:03
When ostree commit --consume is used and an object with the same
checksum already exists in objects/, rename_pending_loose_objects() was
unconditionally renaming the staging copy over it.  On Linux, renameat(2)
atomically replaces the destination for two regular files, silently
changing the inode of the existing repo object.

Fix this by checking whether the object already exists in objects/
before renaming.  If it does, the content is identical by definition
(the object store is content-addressed by SHA256), so we can simply
unlink the staging copy and keep the existing object with its original
inode.

Exception: .commitmeta objects are keyed by commit checksum rather than
their own content, so they can be updated in place (e.g. when GPG
signatures are added or deleted via ostree gpg-sign).  These are always
renamed unconditionally.
@electricface electricface force-pushed the fix-consume-inode-stability branch from 11b7327 to 9fd0804 Compare June 30, 2026 12:45
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