Skip to content

fix(exports): close failed Parquet output streams - #227

Merged
Miracle656 merged 1 commit into
Miracle656:mainfrom
Wang1rrr:fix/parquet-writer-cleanup
Oct 18, 2026
Merged

Miracle656 merged 1 commit into
Miracle656:mainfrom
Wang1rrr:fix/parquet-writer-cleanup

Conversation

@Wang1rrr

@Wang1rrr Wang1rrr commented Oct 9, 2026

Copy link
Copy Markdown
Contributor

Summary

If a Parquet export fails while querying rows, appending a row, or closing the writer, the handler attempts to unlink the temporary file while its output descriptor remains open. Retain ownership of the underlying stream, finish pending writes, and wait for descriptor closure before removing the file and forwarding the original error. Run the truncation probe before opening the file.

The regression suite uses real temporary files and the real Parquet library, with controlled database and writer failures. It also reads a successful capped download back through the Parquet reader to check compatibility.

Related issue

Follow-up to the export endpoint in #149 and export validation in #199. No existing open issue or PR was found for writer cleanup on failure.

Type of change

  • Bug fix
  • New feature
  • Refactor
  • Docs
  • Tests
  • CI / tooling

Validation

Windows, Node.js 24.14.0, based on 4428d53:

  • The new lifecycle suite produced 4 failures and 2 passing controls on the unmodified implementation.
  • Focused Parquet lifecycle and CSV export suites: 13 tests passed.
  • npm run db:generate (same schema and shared dependency installation)
  • npx tsc --noEmit
  • npm run typecheck
  • npm run build
  • npm test: 54 suites, 650 tests passed, including coverage thresholds.

Tests verify descriptors are closed and files are removed before an error response completes. No live database or RPC is used. The change covers file-generation failures; cancellation during the later download is outside this scope. AI assistance: implemented with Codex and validated locally.

Checklist

  • I have read CONTRIBUTING.md
  • npx tsc --noEmit passes
  • npm run build passes
  • I added / updated tests where relevant
  • I updated docs where relevant (file format, endpoint, and configuration are unchanged)

Copilot AI balanced review requested due to automatic review settings October 9, 2026 16:21

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@Miracle656 Miracle656 left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right fix, and it cleans up after itself rather than only closing the handle.

A failed Parquet export previously left the write stream open and the partial file on disk. Both matter and for different reasons: the open handle is a descriptor leak that accumulates silently until the process hits its limit — at which point unrelated work starts failing, which is a miserable thing to debug — and the orphaned partial file is worse than useless, because a half-written Parquet is still a plausible-looking file that someone or something may later pick up.

output.destroy() then fs.promises.unlink(output.path) with the ENOENT swallowed is the correct order and the correct tolerance: a stream that never created its file must not turn a real export error into a confusing cleanup error on the way out.

The test asserting the error still reaches the error handler is the one I would have asked for — cleanup code that accidentally swallows the original failure is a common way to make an outage undiagnosable.

726 tests pass, tsc --noEmit clean, verified against current main. Integration tests is pre-existing red on main.

Merging.

@Miracle656
Miracle656 merged commit 4553874 into Miracle656:main Oct 18, 2026
1 check passed
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.

3 participants