Stamp a format version on the data directory - #245
Conversation
github-actions
Bot
added
documentation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 51144de94a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| func stamp(path string) error { | ||
| if err := os.WriteFile(path, []byte(strconv.Itoa(Version)+"\n"), 0o600); err != nil { |
There was a problem hiding this comment.
When the initial os.WriteFile creates format but fails during the write—for example because the disk or quota is full—it can leave an empty or partial file behind. After the underlying condition is fixed, every subsequent startup sees that file as an invalid version and refuses to use the directory; similarly, a power loss can leave durable log data paired with a non-durable marker. Write and sync a temporary file, then rename it and sync the directory so an interrupted stamp remains absent rather than permanently invalid.
Useful? React with 👍 / 👎.
| // freshly mounted volume is not empty — lost+found is there before kvs ever runs. | ||
| func holdsData(dir string) bool { | ||
| for _, name := range []string{LogName, RaftName} { | ||
| if _, err := os.Stat(filepath.Join(dir, name)); err == nil { |
There was a problem hiding this comment.
If os.Stat returns an error other than os.ErrNotExist while checking an existing kvs.log or raft path—for example a transient filesystem I/O error—this loop treats the path as absent and stamps the directory as current. Once that happens, a later startup can accept and read the previously unversioned data, defeating the compatibility guard this package introduces. Return and propagate non-absence errors instead of collapsing every stat failure into “no data.”
Useful? React with 👍 / 👎.
| path := filepath.Join(dir, FormatName) | ||
|
|
||
| //nolint:gosec // The path is the data directory the operator named; that is the feature. | ||
| raw, err := os.ReadFile(path) |
There was a problem hiding this comment.
When the format path contains an unexpectedly large file—for example after an accidental replacement, a bad restore, or modification of a writable volume—os.ReadFile allocates memory proportional to its size before validating the value. A multi-gigabyte or large sparse marker can therefore terminate the process through memory exhaustion instead of producing the promised clean format refusal. Read only a small bounded amount sufficient for a version and reject any additional content.
Useful? React with 👍 / 👎.
cd38af1 to
6d921ab
Compare
|
Codex review, three comments, all taken in 6d921ab.
Each has a check: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6d921ab871
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| defer func() { _ = file.Close() }() | ||
|
|
||
| return io.ReadAll(io.LimitReader(file, maxFormatBytes)) |
There was a problem hiding this comment.
When a marker is longer than 64 bytes but its first 64 bytes are 1 followed by whitespace, LimitReader silently discards the remaining content and TrimSpace makes the truncated prefix look like the current version, so Ensure accepts a corrupted or future-format directory instead of failing closed. The fresh evidence after the earlier bounded-read comment is that this new implementation caps allocation but never reads a 65th byte to detect truncation; read one extra byte and return a format error when it exists.
Useful? React with 👍 / 👎.
| return fmt.Errorf("open data dir: %w", err) | ||
| } | ||
|
|
||
| if err := handle.Sync(); err != nil { |
There was a problem hiding this comment.
For the Windows binaries configured in .goreleaser.yml, os.Open(dir) produces a read-only directory handle and File.Sync maps to FlushFileBuffers, which requires write access, so this returns an access-denied error for every newly stamped directory. Because the rename has already installed format, the first Open or clustered startup fails while an immediate retry succeeds; use a platform-supported durability operation or explicitly handle the unsupported directory-sync error.
Useful? React with 👍 / 👎.
6d921ab to
6f12890
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6f128906f6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // caller tells a missing file from an unreadable one, so the error is returned unwrapped. | ||
| func readFormat(path string) ([]byte, error) { | ||
| //nolint:gosec // The path is the data directory the operator named; that is the feature. | ||
| file, err := os.Open(path) |
There was a problem hiding this comment.
When format is a FIFO, os.Open blocks waiting for a writer, and ReadAll can continue waiting for EOF even after version bytes arrive. Thus an accidentally or deliberately replaced marker can hang every startup indefinitely instead of producing the promised format refusal; inspect the path type first or open/read it in a way that cannot block on special files.
Useful? React with 👍 / 👎.
| // freshly mounted volume is not empty — lost+found is there before kvs ever runs. | ||
| func holdsData(dir string) (bool, error) { | ||
| for _, name := range []string{LogName, RaftName} { | ||
| switch _, err := os.Stat(filepath.Join(dir, name)); { |
There was a problem hiding this comment.
When an unversioned kvs.log or raft entry is a symlink whose target is temporarily absent—for example storage mounted separately—os.Stat follows it and returns ErrNotExist, so this check stamps the directory as format 1. If the target later becomes available, the old unversioned data is then accepted as current; use Lstat to distinguish an absent entry from an existing dangling symlink and fail closed.
Useful? React with 👍 / 👎.
kvs replays whatever it finds in --data-dir. If a future version lays those bytes out differently, the replay reads them anyway and the damage surfaces later, looking like corruption rather than a version mismatch. Nothing on disk said which version wrote it. A data directory now carries a format file naming its version, and every path that opens one checks it first: kvs.Open for the single-node and library cases, cluster.Start for a clustered node, which never goes through Open. A version this build does not understand stops startup with both numbers and what to do about it. The marker sits beside the data rather than inside it because the Raft store's files belong to a library that will not carry our header. One version on the directory covers our append log and its files together, which also means it has to move when that library changes its own layout. There is no conversion between versions and none is promised. Refusing to start is the feature; a half-working replay is what it replaces. The filenames the check looks for now live in one place, so a log this build writes cannot be one the next build fails to notice.
Three ways the version check could fail to do its job. A stat that failed for any reason other than the file being absent was read as "no data here", which stamps this build's version onto a directory that may hold an older one's keyspace - the silent acceptance this package exists to prevent. The stamp was written in place, so an interrupted write left a file holding half a version, and half a version is refused forever. And the file was read whole before being looked at, so replacing it with something enormous ends the process instead of being told it is not a version. Now the stat failure is returned, the stamp goes through a temporary file that is synced and renamed with the directory synced after it, and the read stops at 64 bytes.
6f12890 to
8b4c631
Compare
Two ways the check could still accept a directory it exists to refuse, and one place a fix for either would have to be written twice. The read stopped at 64 bytes, which bounds the allocation but says nothing about the file: a marker holding this version followed by more than 64 bytes of anything else was trimmed down to its prefix and taken for that version. It now reads one byte past the cap and refuses anything that reaches it, quoting the prefix it did read so the message still says what was on disk. The search for data written before versioning followed symlinks, so an entry pointing at a volume that is not mounted yet answered "not there". The directory was stamped on the strength of that, and the unversioned keyspace behind the link would be accepted as this format the moment the volume came back. Lstat tells an absent entry from one whose target is away, and the second is data. The append log and this package each carried their own copy of the same open-sync-close on the directory. It belongs here, where the directory does, so there is one of it to change - the flush fails on Windows, where a read-only directory handle cannot be flushed, and a fix that lands in one copy fixes nothing. The test standing in for a path that cannot be stat'd used a symlink loop, which Lstat reads without complaint. It takes the search bit off the directory instead, and skips where that changes nothing.
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Summary
kvs replays whatever it finds in
--data-dir, and nothing on disk said which version wrote it.A future release that lays those bytes out differently would be read anyway, and the damage
would surface later looking like corruption rather than a version mismatch.
A data directory now carries its format version, and every path that opens one checks it before
reading anything.
Type of Change
Not marked breaking: no released version writes a data directory at all.
--data-dirarrivedwith #236 and has never been tagged. A directory left behind by an untagged build is refused
by this change, which is deliberate — see below.
Changes
internal/datadir—Ensure(dir)stamps<data-dir>/formaton a new directory and refusesone whose version this build does not understand. Three refusals, each with its own advice: a
version that is not ours, a
formatfile that does not hold a number, and data present withno
formatfile at all. The last one is the untagged-build case.kvs.Openandcluster.Startboth call it. A clustered node never goes throughOpen, soone call site would have left the Raft path unguarded.
library that will not carry our header, so one version on the directory covers our append log
and its files together. That also means the version has to move when that library changes its
own layout, which the docs now say.
kvs.logandraftare named in one place now, so a log this build writes cannot be one thenext build fails to notice.
content/docs/compatibility.mdandcontent/docs/clustering.md— the on-disk paragraph waswritten when there was no version to describe. What is promised is the refusal, not the
contents: kvs does not convert between formats and does not promise to.
Testing
make allpassesSix cases in
internal/datadir, plus one throughkvs.Openend to end. Also checked againstthe real binary: a fresh directory gets
formatholding1, and editing it to2makeskvs servestop with both numbers in the message.The public API surface is unchanged —
testdata/api-surface.txtdoes not move in this PR,which is why the new package is under
internal/.Related Issues
None.