Repository navigation
SOLR-18010: Replace standalone security.json through a synced temporary file on POSIX - #5118
Open
nick-boss-tech wants to merge 13 commits into
Open
nick-boss-tech wants to merge 13 commits into
nick-boss-tech wants to merge 13 commits into
Conversation
nick-boss-tech
added a commit
to nick-boss-tech/solr
that referenced
this pull request
Oct 13, 2026
… gate jobs, two settling runs, draft fixes Verdicts: apache#5116 SATISFIED; apache#5115, apache#5117 and apache#5118 NOT SATISFIED with blocking items named. Round 2 assignment posted.
Contributor
Author
…keep its permissions, drop internal metadata
nick-boss-tech
force-pushed
the
solr-18010-submit
branch
from
October 21, 2026 18:41
5713074 to
57ad783
Compare
…ry file on POSIX Edits to a standalone security.json now write a temporary file in the same directory, sync it, and move it over the existing file, so a crash during an edit leaves the previous file in place. The in-place write is kept for file systems without POSIX attributes, symbolic links, a SOLR_HOME that cannot hold a temporary file, a refused move, and a file owned by another user. A failed write, sync or permission copy fails the edit and keeps the existing file. This branch was rebased onto upstream main; the earlier commits in this branch are kept in history.
…le errors Only a SOLR_HOME that denies writes falls back to an in-place write when the temporary file cannot be created. Other failures, such as running out of inodes, fail the edit and keep the existing file, since an in-place write would truncate it first. The replacement file takes the group of the existing security.json, the same as it already keeps the owner and mode. If the group cannot be set, the edit is written in place, as for a file owned by another user.
…ike test filesystem The reader test renames over a file it holds open. The Windows-like test filesystem tracks open files by inode and fails its bookkeeping assertion in that case (seed 75F671E94C57A51). The class runs without that filesystem. On NFS a read can fail with a stale handle while the file is replaced; the reader skips those reads and still checks every successful one.
…eader test to its own class Renaming over a hard-linked file breaks the link, so a second name for the same file kept the old content. Such files are written in place, as before. The reader test replaces a file while it is open, which the Windows-like test filesystem cannot track, so it moves to SecurityConfHandlerReaderTest, which runs without that filesystem.
The module check requires the call in any test file that uses Mockito.
nick-boss-tech
added a commit
to nick-boss-tech/solr
that referenced
this pull request
Oct 23, 2026
nick-boss-tech
marked this pull request as ready for review
October 23, 2026 02:52
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.
🤖 AI text below 🤖 (reviewed by Nick Shanin)
https://issues.apache.org/jira/browse/SOLR-18010
What happens today
A reader can see an empty or partial
security.json, and a crash mid-write leaves one behind.persistConfat the merge base opens the file with truncation before it writes the new content. #5065 serializes edits inside one process. It does not protect the file on disk.What this change does
An edit writes a temp file, syncs it, and renames it over
security.json. Readers see the old file or the new one. The new file keeps the owner, group and mode (code).Other projects use a similar pattern. ZooKeeper's
AtomicFileOutputStreamsyncs, then renames. Kafka'satomicMoveWithFallbackrenames, then syncs the parent directory.Choices to check
Renaming over
security.jsondrops its ACLs and SELinux labels. Is that acceptable?Main writes this file in place, so a crash can truncate it. A rename cannot leave a partial file. The cost is that the new file is a new inode, and Java on Linux cannot read or copy ACLs or SELinux labels. The reference guide says so.
How to decide:
security.jsonhave an ACL or a custom SELinux label? Check withgetfaclandls -Z. If no, rename costs nothing.Recommendation: rename. A partial file after a crash is worse than a label that has to be set again, and step 2 covers setups that need one.
Is losing ACLs and SELinux labels after an edit acceptable, with the step documented?
When an owner, a group or a move blocks the rename, should the edit write in place or fail?
Bind-mounted files, files owned by another user, and groups that cannot be set all save on main today, because main writes in place. The fallback brings back main's truncation risk, but only for those files.
How to decide:
Recommendation: keep the fallback, with the logged warning. Those setups keep working, and their risk is no worse than main.
Should these edits keep the in-place fallback, or fail?
A process killed during a write leaves a temporary file behind. Should Solr delete leftovers at startup?
Context: an edit that succeeds or fails with an error leaves no temporary file (the tests check this). Only a kill during the write leaves one, named
security.json.<uuid>.tmp. In 75 of 100 kill trials it was left behind. It is as large as the file it was replacing, owner-only, and Solr never reads it.How to decide:
Recommendation: leave the files in this PR and add the sweep as a follow-up. A startup delete changes what Solr removes on boot, and it needs tests of its own. This PR changes the write path only.
Is leaving the leftovers now, with the sweep in a follow-up, acceptable?
Proof
The temp-file test fails on
c34abaec94and passes on46869c4d8b(2026-10-23; production code unchanged sinced8388c6519. The last commit,d742eb99dd, changes one line of a test file, an Error Prone fix).SecurityConfHandlerTest: 7 of 8 pass on Linux. The hard-link test skips on NFS and passes on local disk. The temp-file test fails onc34abaec94, where the temp file is0644while it is written. On main, the reader test and the concurrent-persist test fail.SecurityConfHandlerFailurePathsTest: 8 of 8 pass on Linux. It makes each failure step fail through a test file system that wraps the default one: the temp-file sync, the directory sync, the move, the permission copy, the owner check, and the group copy with its failure. Removing the temp-file sync, the owner check and the group copy from production fails 6 of the 8 tests. Removing the permission copy fails its test, and making a refused move throw fails its test.SecurityConfHandlerReaderTest: 1 of 1 on Linux.BasicAuthStandaloneTest: 2 of 2.d742eb99dd: 19 tests, 6 pass, 13 skipped, 0 failures, 0 errors. The skips are the POSIX-only tests: all 8 failure-path tests, 4 of 8 inSecurityConfHandlerTest, and the reader test. The other 6 pass, including the concurrency and edit tests. Windows writes in place, as on main.Limits
Not covered: power loss, a real group change, and the symlink case.
LinkPermission "hard"only. Leaving the policy as it is is the recommendation.SecurityConfHandlerReaderTestruns without Lucene's Windows-like test filesystem, which fails when a file is replaced while another handle is open. On NFS a read can fail with a stale handle during a replace. The test skips those reads, and a production reader gets an error, not partial content.Changelog: changelog/unreleased/SOLR-18010-security-json-atomic-replace.yml
AI-generated description. Nick Shanin directed the work and takes responsibility for this contribution.