You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
[Bug]: config.yaml is rewritten in place, and an interrupted save can silently empty or truncate the inventory #337
Saving config.yaml is neither atomic nor durable, and a damaged file is then accepted without complaint on the next load. Together those can lose an inventory.
A save is not durable.PhysicalTextFileStore.WriteAllTextAsync calls File.WriteAllTextAsync, which never flushes to disk, so a save that has returned successfully can still be lost or left partial if the machine loses power shortly afterwards.
A save is not atomic.File.WriteAllTextAsync opens with FileMode.Create, so the file is truncated before any bytes go back. Sampling the size during one rpk servers add on a 960 KB config (30,000 resources) catches it at 0 and 8192 on most runs, plus one intermediate size. The window is short: about a millisecond per save on my machine, so a process being killed inside it is unlikely. The realistic paths to a damaged file are power loss (where the missing flush is what bites) and a write that fails part way, such as running out of space, which leaves the truncated file behind.
The only backups are the .bak.<timestamp> copies taken during schema migrations, plus the optional Git integration when it is configured. BackupOriginalAsync writes through the same unflushed, in-place path, so the one existing safety net is written unsafely too.
A damaged file loads without an error, and the next save makes it permanent.
Truncating that config at 20 offsets gives three outcomes (on this synthetic file, where every entry is exactly 32 bytes, so the mix depends on the entry layout):
Outcome
Count
What the user sees
Plausible smaller inventory
11
Up to 29,687 of 30,000 resources, no error, exit 0
Empty inventory
7
Hardware (0), no error, exit 0
Unhandled exception
2
YamlDotNet stack trace, SIGABRT, exit 134
The first outcome is the most common and the most dangerous, because nothing about the file looks wrong. A file cut at an entry boundary is byte for byte a valid smaller config. Cut mid-name, the last entry survives with a truncated name: at one offset the 256th server loads as srv, which could collide with a real resource.
In the empty case the file still held 255 complete entries:
8,192 bytes holding 255 recoverable resources became 71 bytes, with no backup.
The CLI says nothing: exit 0, empty stderr, because LoadAsync runs in CliBootstrap.RegisterInternals before any logging provider is registered, and before config.SetExceptionHandler is installed, which is also why the crash outcome escapes as a raw .NET exception with a core dump rather than a formatted message. The Web app does log it (fail: DocMigrator.Yaml.YamlMigrationDeserializer[0] Failed to deserialize YamlRoot) but still serves an empty inventory with no warning in the UI. I confirmed that against the 2.1.0 image.
Until the first write, the surviving bytes are still visible on the /yaml page, so a user who notices can recover by hand.
Proposal
Two halves of one failure chain. They would be two PRs, and neither covers the whole problem on its own.
1. Treat a swallowed deserialization failure as fatal.
Today the load goes: read the file, hand it to the migration deserializer, build a YamlRoot. When that last step throws, the exception is caught, logged and turned into nothing, which becomes an empty YamlRoot. LoadAsync then clears the in-memory resources and connections and adds nothing back, and the app carries on as though the inventory really were empty. That is how 255 valid entries became Hardware (0) with exit 0 above.
The change would be to stop rather than continue: report that the config looks damaged and refuse to load it, so nothing overwrites the file and the user can still recover from it.
The distinction between "does it parse" and "did it load cleanly" matters here. The 8,192-byte cut ends in - kind: Se, which is perfectly valid YAML, just a string. The failure happens one step later, mapping that entry onto a Resource. A syntax check would wave it through, so the guard has to key on the deserialization failure itself.
This half is cheap, portable, testable in the existing ./Tests style, and covers corruption from any source, including a bad hand edit. It catches the empty and crash outcomes. It cannot catch a file cut at an entry boundary, because nothing in that file says entries are missing, so it does not help with the most common outcome. This continues what #155 set out to do for migrations.
2. Write atomically, and flush. Write a temp file in the same directory, flush it to disk, then rename over config.yaml. This is the only half that prevents the silently shrunken inventory, and the flush is what makes a completed save survive power loss. Doing it inside PhysicalTextFileStore.WriteAllTextAsync also covers the /yaml editor's save (YamlFileComponent.razor:169) and the migration backup.
Two things I am not sure about, because they touch how people mount the config:
A rename replaces the file, so the new one takes its mode from the umask. File.GetUnixFileMode and SetUnixFileMode can carry a user's 600 across on Linux, but ownership cannot be carried the same way, which matters in Docker where config.yaml is often root owned while the process runs as app.
It would also need to clean up the temp file on failure: when GIT_TOKEN is set the Web app stages every non-ignored change in the config directory, so a leftover file would be committed. The .bak.<timestamp> backups already have that property.
On testing: the atomic write is hard to test the way this project prefers. Forcing a crash at the right moment is not deterministic, and permission or disk-full tricks behave differently across platforms. A fake ITextFileStore would exercise the stand-in rather than the real write path. I would cover what is observable (content correct after a write, no leftover temp files) and rely on the rename guarantee for the rest. The load-side guard is straightforward to test: a config with an entry that cannot be deserialized should produce an error rather than a silently reduced inventory.
Happy to implement either or both, in whichever order you prefer.
Where does this occur?
Both / Not sure
How do you run RackPeek?
Docker
RackPeek version
2.1.0 (reproduced with a CLI built from the RackPeek-2.1.0 tag, rpk --version reporting v2.1.0; the Web UI check used the Docker image built from the same code)
Steps to reproduce (optional)
RPK_YAML_DIR must be absolute: a relative value resolves against the binary's directory, not the working directory, and silently creates a fresh empty config there.
# 1. a large config, 30k resources, about 960 KB
export CFG=$(mktemp -d)
{ echo "version: 3"; echo "resources:";
for i in $(seq -w 0 29999); do echo "- kind: Server"; echo " name: srv$i"; done;
echo "connections: []"; } > "$CFG/config.yaml"
# 2. sample the size while one save runs
# (if you see no intermediate sizes, raise the loop count)
( for i in $(seq 1 60000); do stat -c %s "$CFG/config.yaml"; done > /tmp/sizes.txt ) &
RPK_YAML_DIR="$CFG" rpk servers add srv99999
wait
sort -n /tmp/sizes.txt | uniq -c # 0 and 8192 appear on most runs
# 3. a cut that loads as empty
export BAD=$(mktemp -d)
head -c 8192 "$CFG/config.yaml" > "$BAD/config.yaml" # 255 complete entries remain
RPK_YAML_DIR="$BAD" rpk summary # Hardware (0), exit 0, nothing on stderr
RPK_YAML_DIR="$BAD" rpk servers add brand-new # succeeds
wc -c "$BAD/config.yaml" # 71
# 4. the more common outcome: a cut in the middle of a name
export PARTIAL=$(mktemp -d)
head -c 8208 "$CFG/config.yaml" > "$PARTIAL/config.yaml"
RPK_YAML_DIR="$PARTIAL" rpk summary # Hardware (256), no error
RPK_YAML_DIR="$PARTIAL" rpk servers get srv # the 256th entry, name truncated to "srv"
# 5. a cut at an entry boundary is indistinguishable from a smaller config
export BOUNDARY=$(mktemp -d)
head -c 8182 "$CFG/config.yaml" > "$BOUNDARY/config.yaml"
RPK_YAML_DIR="$BOUNDARY" rpk summary # Hardware (255), valid YAML, nothing to detect
@Timmoth thanks for #343. Step 3 of the reproduction above still ends with a 71-byte config.yaml on current staging (5f99c51): rpk summary now exits 5 with "nothing has been changed", but by then the file has already been replaced with an empty config, so rpk servers add brand-new succeeds. The only .bak left holds the empty file too (both commands ran within the same second), so the inventory is gone.
A damaged v4 config is refused and left untouched, as intended. The difference is that the step 3 file says version: 3, so it takes the migration path, and that path saves before the new check runs:
ResourceYamlMigrationService.DeserializeAsync turns a failed deserialize into new YamlRoot() (line 35) and passes it to postMigrationAction (line 38), which is SaveRootAsync. That writes version: 0 / resources: [] / connections: [] before the root.Version <= 0 check in LoadUnderLockAsync (line 292) throws.
The migration writes a .bak of the original first, but backup names have one-second resolution. A second load in the same second migrates the empty file and overwrites that .bak with it. This happens with two quick CLI commands, and in Docker when the first page load comes within a second of container start.
The existing test a_file_that_is_not_a_rackpeek_config_is_rejected already hits this path: hello: world is rewritten to the empty version: 0 config, but the test only checks the output. Adding an assertion that the file is unchanged makes it fail on staging. Skipping the post-migration save when Deserialize returns null lets the version check fire first, and the rest of the suite, including the legacy version-less migration test, still passes.
Also, on the single-file bind mount mentioned in the issue (-v ./config.yaml:/app/config/config.yaml): on staging every save now fails with IOException: Device or resource busy from the rename, and with a config from 2.1.0 the migration save on load fails too, so / returns 500. 2.1.0 saved fine with the same mount. The fallback suggested in the issue (a direct write when the rename fails) is one way to keep that setup working. Directory mounts, which is what the docs show, are unaffected.
What happened?
Saving
config.yamlis neither atomic nor durable, and a damaged file is then accepted without complaint on the next load. Together those can lose an inventory.A save is not durable.
PhysicalTextFileStore.WriteAllTextAsynccallsFile.WriteAllTextAsync, which never flushes to disk, so a save that has returned successfully can still be lost or left partial if the machine loses power shortly afterwards.A save is not atomic.
File.WriteAllTextAsyncopens withFileMode.Create, so the file is truncated before any bytes go back. Sampling the size during onerpk servers addon a 960 KB config (30,000 resources) catches it at 0 and 8192 on most runs, plus one intermediate size. The window is short: about a millisecond per save on my machine, so a process being killed inside it is unlikely. The realistic paths to a damaged file are power loss (where the missing flush is what bites) and a write that fails part way, such as running out of space, which leaves the truncated file behind.The only backups are the
.bak.<timestamp>copies taken during schema migrations, plus the optional Git integration when it is configured.BackupOriginalAsyncwrites through the same unflushed, in-place path, so the one existing safety net is written unsafely too.A damaged file loads without an error, and the next save makes it permanent.
Truncating that config at 20 offsets gives three outcomes (on this synthetic file, where every entry is exactly 32 bytes, so the mix depends on the entry layout):
The first outcome is the most common and the most dangerous, because nothing about the file looks wrong. A file cut at an entry boundary is byte for byte a valid smaller config. Cut mid-name, the last entry survives with a truncated name: at one offset the 256th server loads as
srv, which could collide with a real resource.In the empty case the file still held 255 complete entries:
One unreadable entry discards the whole file, not just that entry. The next save writes the reduced state back:
8,192 bytes holding 255 recoverable resources became 71 bytes, with no backup.
The CLI says nothing: exit 0, empty stderr, because
LoadAsyncruns inCliBootstrap.RegisterInternalsbefore any logging provider is registered, and beforeconfig.SetExceptionHandleris installed, which is also why the crash outcome escapes as a raw .NET exception with a core dump rather than a formatted message. The Web app does log it (fail: DocMigrator.Yaml.YamlMigrationDeserializer[0] Failed to deserialize YamlRoot) but still serves an empty inventory with no warning in the UI. I confirmed that against the 2.1.0 image.Until the first write, the surviving bytes are still visible on the
/yamlpage, so a user who notices can recover by hand.Proposal
Two halves of one failure chain. They would be two PRs, and neither covers the whole problem on its own.
1. Treat a swallowed deserialization failure as fatal.
Today the load goes: read the file, hand it to the migration deserializer, build a
YamlRoot. When that last step throws, the exception is caught, logged and turned into nothing, which becomes an emptyYamlRoot.LoadAsyncthen clears the in-memory resources and connections and adds nothing back, and the app carries on as though the inventory really were empty. That is how 255 valid entries becameHardware (0)with exit 0 above.The change would be to stop rather than continue: report that the config looks damaged and refuse to load it, so nothing overwrites the file and the user can still recover from it.
The distinction between "does it parse" and "did it load cleanly" matters here. The 8,192-byte cut ends in
- kind: Se, which is perfectly valid YAML, just a string. The failure happens one step later, mapping that entry onto aResource. A syntax check would wave it through, so the guard has to key on the deserialization failure itself.This half is cheap, portable, testable in the existing
./Testsstyle, and covers corruption from any source, including a bad hand edit. It catches the empty and crash outcomes. It cannot catch a file cut at an entry boundary, because nothing in that file says entries are missing, so it does not help with the most common outcome. This continues what #155 set out to do for migrations.2. Write atomically, and flush. Write a temp file in the same directory, flush it to disk, then rename over
config.yaml. This is the only half that prevents the silently shrunken inventory, and the flush is what makes a completed save survive power loss. Doing it insidePhysicalTextFileStore.WriteAllTextAsyncalso covers the/yamleditor's save (YamlFileComponent.razor:169) and the migration backup.Two things I am not sure about, because they touch how people mount the config:
-v ./config.yaml:/app/config/config.yaml) fails with "device or resource busy", and creating a temp file needs write permission on the config directory. A CLI-only setup does not need that in the steady state today, although it already does on the migration path, since.bakfiles are written there. The Web app always needs it, because it creates.dataprotectionin the same directory. Given '/app/config/config.yaml' is denied #101, Access to /app/config/config.yaml error #144 and [Bug]: When using BIND volume: Unhandled exception. System.UnauthorizedAccessException: Access to the path '/app/config/config.yaml' is denied. #176 ("Access to the path '/app/config/config.yaml' is denied" with bind mounts) and [Bug]: Access to /tmp is denied on docker/ubuntu #312 (Path.GetTempFileName()failing in a hardened container), I would fall back to the current direct write whenever the temp file or the rename fails, so nothing that works today breaks.File.GetUnixFileModeandSetUnixFileModecan carry a user's600across on Linux, but ownership cannot be carried the same way, which matters in Docker whereconfig.yamlis often root owned while the process runs asapp.It would also need to clean up the temp file on failure: when
GIT_TOKENis set the Web app stages every non-ignored change in the config directory, so a leftover file would be committed. The.bak.<timestamp>backups already have that property.On testing: the atomic write is hard to test the way this project prefers. Forcing a crash at the right moment is not deterministic, and permission or disk-full tricks behave differently across platforms. A fake
ITextFileStorewould exercise the stand-in rather than the real write path. I would cover what is observable (content correct after a write, no leftover temp files) and rely on the rename guarantee for the rest. The load-side guard is straightforward to test: a config with an entry that cannot be deserialized should produce an error rather than a silently reduced inventory.Happy to implement either or both, in whichever order you prefer.
Where does this occur?
Both / Not sure
How do you run RackPeek?
Docker
RackPeek version
2.1.0 (reproduced with a CLI built from the
RackPeek-2.1.0tag,rpk --versionreporting v2.1.0; the Web UI check used the Docker image built from the same code)Steps to reproduce (optional)
RPK_YAML_DIRmust be absolute: a relative value resolves against the binary's directory, not the working directory, and silently creates a fresh empty config there.