Repository navigation
Conversation
A WRITE request owns the file opened by its handler. Seek and partial-write failures previously returned without closing it, leaving retirement to backend finalizers instead of request completion. Close once on both failure exits, preserve the primary error identity and NFS status, and include any close failure as diagnostic context. Keep the normal success and close-error paths unchanged; use APIs compatible with the declared Go version. Loopback regressions use real temporary files and cover open, seek, partial-write, success and close-error paths. Restoring the previous handler fails the owned-close and closed-file assertions. Full normal tests, a full race run, vet and the consuming proxy race regression pass; an earlier repeated full race attempt hit client source-port collisions. No claim is made that every backend physically retires a resource after a failed Close.
Clamp large reads with unsigned offset comparisons so reads beyond EOF do not underflow into a larger backend buffer. Do not cast unrepresentable protocol offsets into a negative ReaderAt offset. Report known EOF even when a complete or zero-byte backend read returns nil, reuse the post-read attributes, and advertise the actual READ cap in FSINFO. Real-file loopback tests cover Count/Data/EOF, stat/read error statuses, file close, a fixed three-call client bound, and the advertised limit. Normal/race3, all-package normal/race and vet pass; four production omissions fail the intended assertions and restored race3 passes. Correct the existing full-read test to accept complete data with EOF while rejecting short or unrelated-error reads. Old client repeated-read cursor behavior after EOF, concurrent file mutation, minimum-Go runtime and kernel clients are not qualified here.
A terminal NFS READ can return data with EOF. The old client's sequential Read kept its previous cursor on that result, replaying consumed bytes on a later read and rereading the old prefix after subsequent file growth. Select the published correction in willscott/go-nfs-client#6 for this module's ordinary build/test path. Add real filesystem/TCP server-client regressions for zero and nonzero initial positions, cursor advancement, next-read no-replay, sequential append after EOF, and position-independent ReadAt. Reuse the existing server fixture instead of a separate wire implementation. Focused normal/race count3, all-package normal/race, vet and module tidying pass. Restoring the old native client reproduces six cursor, six replay and six growth failures across count3; restored race count3 passes. One earlier full-normal run encountered an existing WRITE fixture source-port collision, retained separately. The temporary public-fork replacement applies only when this repository is the main module. Downstream consumers must select the corrected client explicitly; upstream default-branch landing and the canonical pin remain separate delivery obligations. No minimum-Go execution, kernel-client, concurrent-mutation, aggregate-pressure or release qualification is implied.
Verbose root-package tests can race in the NFS client logger. The READ tests start RPC receive goroutines; a later TestNFS turns debugging on by writing the logger's unsynchronized flag. Closing the connection does not join that reader, so changing the flag in a later test is unsafe even without parallel tests. Parse test flags and configure verbose client logging in TestMain before any test starts. Production NFS code and default nonverbose logging remain unchanged. The Go 1.19.13 root race run fails at the logger update before this change and passes afterwards. The root suite also passes normal and race runs on Go 1.26.8. A repeated shuffled race run reported no logger race but hit existing random source-port bind collisions, which this fixture correction does not address.
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.
A READ beyond a known file’s end currently subtracts unsigned offsets in the wrong direction. For example, a 32,769-byte request at offset 8 in a seven-byte file reaches the backend with a 16 MiB buffer instead of returning an empty EOF reply. The handler can also omit EOF when a backend returns the complete remaining data with no error, and FSINFO advertises a 1 GiB read size although the handler already enforces a 16 MiB cap.
Compare offsets before subtracting, clamp to the nonnegative remaining size, and avoid converting protocol offsets above MaxInt64 into negative ReaderAt offsets. Derive known EOF from the post-read regular-file attributes while retaining backend EOF, serialize the same attributes, and advertise the existing MaxRead cap. The ordinary maximum is not increased and the correction adds no retry loop or background work.
Correct terminal EOF exposes a separate sequential-client bug: File.Read in the old client advances its cursor only when the read error is nil, so it can replay data already returned together with EOF and reread the old prefix after later file growth. Select the published correction from willscott/go-nfs-client#6 for this repository’s main-module build. The new real-filesystem/TCP tests check cursor progress, no replay, appended data, and position-independent ReadAt from both zero and nonzero initial offsets. This does not suppress or replace the original read error.
The server boundary, status, close, EOF and FSINFO regressions pass normal/race count3; four production-only omission controls fail their intended assertions. Restoring the previous native client selection reproduces cursor, replay and growth failures under both normal and race count3; the corrected selection and restored tests pass. All native packages pass normal and race runs, vet and tidy-diff. One earlier full-normal run encountered an existing WRITE-fixture source-port collision; repeated full-suite stability is not claimed.
Verbose root-package tests also previously changed the client logger’s unsynchronized debug flag after READ-test RPC receivers had started. Configure the same verbose setting in TestMain before any test runs; production NFS code is unchanged. The original verbose Go 1.19.13 root race run failed on the logger update; the corrected matching root race run and Go 1.26.8 normal/race root runs passed. Root READ/WRITE and paired-client paths, plus the changed client READ tests, were exercised on Linux/amd64 with Go 1.19.13. A repeated shuffled race run still hit existing random source-port bind collisions and was not green. The minimum-Go check does not cover the entire module’s helper test framework, which uses newer testing APIs, and capability-dropped full-client tests retain a privileged source-port bind limitation.
This branch includes the unchanged WRITE-retirement prerequisite from #4 because it is not yet on master. It contains no additional WRITE-handler changes; that prerequisite closes the request-owned file on seek/write errors while preserving the primary NFS status. The READ correction remains dependent on its reviewed server base.
The temporary client replace applies only when go-nfs is the main module. Dependency consumers must explicitly select a corrected client too; Go does not inherit dependency replace directives. Upstream client/default-branch landing and downstream canonical pins remain separate integration work. Minimum-Go qualification is limited to the root-package and changed-client paths above. These tests do not qualify concurrent file mutation, arbitrary third-party or kernel clients, aggregate memory/descriptor pressure, or deployment. This PR is review-ready, not a claim of those outcomes or of human acceptance.