st: chunked pread with EINTR retry and honest short-read errors#331
Open
nbeerbower wants to merge 2 commits into
Open
st: chunked pread with EINTR retry and honest short-read errors#331nbeerbower wants to merge 2 commits into
nbeerbower wants to merge 2 commits into
Conversation
Two latent bugs in every st.h reader, both hit in the field: - a single pread caps at ~2^31 bytes on Linux, so any tensor past 2.1 GB (bf16 embed/unembed tensors of large models qualify) came back silently truncated with perror printing '... : Success' (errno untouched by a short read) — the same misleading-error symptom glm.c fixed for its own reads in JustVugg#236; - no EINTR retry. st_pread_full loops in ST_PREAD_CHUNK pieces (1 GB default, override for tests), retries EINTR, and reports offset + progress on failure. All five read sites converted; behavior on well-formed files is byte-identical (GLM oracle re-verified on this branch: 32/32). tests/test_st_pread builds with -DST_PREAD_CHUNK=7 so a 96-byte tensor exercises the multi-chunk loop, and forks a child against a shard truncated after st_init (init's static bounds check means the pread path only fires when a file shrinks under a live handle) asserting exit(1) with a 'short read' message and no 'Success'. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ed POSIX Same fix pattern as test_stops: mkdtemp with a CWD-relative template (MinGW resolves Windows paths; /tmp is not one), and the fork/pipe/ truncate-based truncation subtest compiles only where those exist — Windows still runs the cross-platform chunk-loop content check. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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
1 participant
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.
Same class of bug you fixed in glm.c for #236, still present in the shared
st.hreaders — plus one you haven't hit yet but we did:preadcaps at ~2^31 bytes on Linux, so any tensor over 2.1 GB comes back silently truncated, withperrorprinting... : Success(short reads don't set errno). We hit this in the field with 2.47 GB bf16 embed/unembed tensors; nothing inst.hguards against it today.st_pread_fullloops inST_PREAD_CHUNKpieces (1 GB default), retries EINTR, and reports offset+progress honestly on failure. All five read sites converted; well-formed files read byte-identically (GLM oracle on this branch: 32/32).Verification:
tests/test_st_pread(added toTEST_BINS) compiles with-DST_PREAD_CHUNK=7so a 96-byte tensor exercises the multi-chunk loop with exact content checks, and forks a child against a shard truncated afterst_init(your init-time bounds check already covers static truncation — nice — so the pread path only fires when a file shrinks under a live handle), assertingexit(1)with a "short read" message and no "Success".make check70/70, 0 warnings.🤖 Generated with Claude Code
https://claude.ai/code/session_016KLDCYJSyHxd39ChTr4T27