fix: stop masking fsync failures in the image store - #447
Merged
Conversation
four sync() catch {} sites in the image/blob write path swallowed fsync
errors — a durability failure (disk full, IO error) would go unnoticed
until a later read found a corrupt or missing layer. convert each to
catch |err| log.warn(...) so the failure is observable. content
addressing still guards correctness; this adds the early signal.
deliberately left untouched: idempotent schema migrations, best-effort
temp-file cleanup on error paths, best-effort client writes to a
possibly-gone peer, and sleep catch unreachable (not reachable on
external input). those aren't masking real faults — churning them adds
risk without value.
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.
summary
third of the four-PR hardening pass — the targeted error-handling sweep.
the audit found ~264
catch {}and ~165catch unreachable, but thelarge majority are legitimately benign (idempotent migrations,
best-effort temp cleanup, writes to disconnected peers, sleeps that only
fail on cancellation). this PR fixes the ones that actually mask a
real fault: silently-swallowed fsync failures in the image store.
what's in here
four
sync() catch {}→catch |err| log.warn(...)in the blob/layerwrite path:
image/layer/create.zig— layer tar fsyncimage/registry/blob_transfer.zig— downloaded blob fsyncimage/store/blob_runtime.zig— blob copy + blob store fsync (×2)a swallowed fsync means a durability failure (disk full, IO error) goes
unnoticed until a later read finds a corrupt/missing layer. content
addressing still guards correctness on read; this adds the early signal
at write time.
deliberately not touched (and why)
addColumnIfMissing ... catch {}) —"column already exists" is the expected benign case
is harmless
already gone; logging every disconnect would be noise
sleep(...) catch unreachable— fails only on cancellation, notreachable from external input
a blanket sweep of all 264 sites would be high-churn and risky for no
real safety gain; this stays surgical.
verification
YOQ_SKIP_SLOW_TESTS=1 zig build test— 2235 passed, 0 failedtools/panic_audit.shclean;zig fmt --checkclean