Skip to content

fix(conversion): clean up temp dirs for unsupported buffers#105

Open
Co-Messi wants to merge 2 commits intorun-llama:mainfrom
Co-Messi:codex/buffer-conversion-cleanup
Open

fix(conversion): clean up temp dirs for unsupported buffers#105
Co-Messi wants to merge 2 commits intorun-llama:mainfrom
Co-Messi:codex/buffer-conversion-cleanup

Conversation

@Co-Messi
Copy link
Copy Markdown
Contributor

Problem

convertBufferToPdf() writes input bytes to a temporary directory before delegating to convertToPdf(). If the delegated conversion returns passthrough content for an unsupported buffer format, that temporary directory is left behind.

What changed

  • cleaned up the temporary directory in convertBufferToPdf() when delegated conversion returns passthrough content or an error
  • added a conversion test covering an unsupported-but-recognized buffer format returning passthrough content

Why this fixes it

This keeps the buffer conversion temp directory tied to the lifetime of the attempted conversion instead of only the successful PDF path.

Verification

  • npm test -- src/conversion/convertToPdf.test.ts
  • npm run format:check
  • npm run build

Fixes #104

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6f08dd1d74

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/conversion/convertToPdf.ts Outdated
const result = await convertToPdf(tmpPath, password);

if ("content" in result || "code" in result) {
await fs.rm(tmpDir, { recursive: true, force: true });
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Handle cleanup failures without masking conversion results

convertBufferToPdf now awaits fs.rm directly when returning passthrough content or a conversion error, so a temp-dir deletion failure (e.g., EPERM/EBUSY on Windows or locked files) will throw and replace the original content/ConversionError result. This is a regression from previous behavior and can make non-PDF buffer parsing fail for cleanup-only reasons; cleanup should be best-effort (catch/log) so conversion outcomes are preserved.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

agreed with this comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Changed this so temp-dir cleanup stays best-effort. If fs.rm fails now, convertBufferToPdf() still returns the original passthrough/error result instead of turning cleanup into the primary failure. I also added a regression test for the cleanup-failure case.

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.

[Bug] convertBufferToPdf leaks temp files for unsupported buffers

2 participants