Skip to content

Fix deadlock, silent data loss and crashes in the sort pipeline - #13

Merged
lanrat merged 1 commit into
mainfrom
claude/extsort-bug-review-562a73
Sep 30, 2026
Merged

lanrat merged 1 commit into
mainfrom
claude/extsort-bug-review-562a73

Conversation

@lanrat

@lanrat lanrat commented Sep 30, 2026

Copy link
Copy Markdown
Owner
  • Run the build, sort and save stages in one errgroup, so a save error (toBytes failure, full disk) no longer deadlocks Sort() and a sort error no longer leaks the save goroutine and its temp file.
  • Report a read error on a chunk's first record instead of silently dropping the chunk, and treat a length header without its payload as io.ErrUnexpectedEOF instead of a clean end of chunk.
  • Return the recovered error from the legacy FromBytes wrapper (named results) instead of (nil, nil).
  • Convert panics in compareFunc, fromBytes and toBytes during the save and merge stages into ComparisonError, DeserializationError and SerializationError instead of crashing the process.
  • Close the temp reader before closing the result channels, fixing the "send on closed channel" panic when Close fails.
  • Create the temp file lazily, once a second chunk exists, and close it on every path. Creation errors are reported on the error channel, so the constructors never return a nil sorter.
  • Release the .extsort_ directory reference when the reader closes.
  • Temp dir selection: return an explicit TempFilesDir unchanged so New reports it when unusable, and skip default candidates that are missing or not writable (no /var/tmp, read-only root) instead of failing.

Add regression tests for each bug, and make TestDeserializationError, TestNilInputs and TestComparisonFunctionPanic reach FromBytes and the merge and require an error.

- Run the build, sort and save stages in one errgroup, so a save error
  (toBytes failure, full disk) no longer deadlocks Sort() and a sort
  error no longer leaks the save goroutine and its temp file.
- Report a read error on a chunk's first record instead of silently
  dropping the chunk, and treat a length header without its payload as
  io.ErrUnexpectedEOF instead of a clean end of chunk.
- Return the recovered error from the legacy FromBytes wrapper (named
  results) instead of (nil, nil).
- Convert panics in compareFunc, fromBytes and toBytes during the save
  and merge stages into ComparisonError, DeserializationError and
  SerializationError instead of crashing the process.
- Close the temp reader before closing the result channels, fixing the
  "send on closed channel" panic when Close fails.
- Create the temp file lazily, once a second chunk exists, and close it
  on every path. Creation errors are reported on the error channel, so
  the constructors never return a nil sorter.
- Release the .extsort_<pid> directory reference when the reader closes.
- Temp dir selection: return an explicit TempFilesDir unchanged so New
  reports it when unusable, and skip default candidates that are missing
  or not writable (no /var/tmp, read-only root) instead of failing.

Add regression tests for each bug, and make TestDeserializationError,
TestNilInputs and TestComparisonFunctionPanic reach FromBytes and the
merge and require an error.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@lanrat
lanrat merged commit 2a059cd into main Sep 30, 2026
1 check passed
@lanrat
lanrat deleted the claude/extsort-bug-review-562a73 branch September 30, 2026 20:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant