Fix race condition on large invoices - #9
Open
valeriansaliou wants to merge 1 commit into
Open
Conversation
…voice PDF is fully flushed to disk
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
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.
Seeing on Crisp billing system frequent race condition (about ~80 at last billing cycle) where the invoice PDF gets written to disk by this library, and then immediately read for Factur-X stamping. But the PDF file is not yet fully written, and fails parsing.
We did not see this error before adding Factur-X, or maybe it got silenced as users were not complaining about seeing invalid invoices uploaded to S3. The Factur-X is very strict, so it throws right away.
Seeing errors like
Error: Failed to parse PDF document (line:145 col:24 offset=2367): Failed to parse invalid PDF objectfrom the Factur-X library. Re-playing the event shows that it works (re-generating the invoice and re-stamping Factur-X). So I suspected a race condition on the return ofMicroInvoice.generate().Claude Code identified a race condition on using the
closevsendevent (we need to wait that all buffers are flushed to disk, that is, the file is fully closed to return the Promise).Claude managed to reproduce it w/o the fix, and confirm that the fix works.
Claude Code output & fix:
The cause
The promise resolves on this.document.on("end", ...). this.document is a PDFKit document, which is a Readable stream. Its "end" event fires when the last chunk has been read out of the document — i.e. handed to the pipe. It says nothing about whether fs.createWriteStream(filePath) has flushed those bytes through the libuv threadpool and closed the fd.
So the ordering is:
Between 1 and 3 you can fs.readFile() the path and get a truncated file — which parses as invalid PDF (the xref table and %%EOF trailer are at the end of the file, so a truncated PDF is exactly the failure mode you're seeing). Bigger invoices = more buffered data = wider window.
Second, smaller issue: this.document.end() is called on line 307, before the listeners are attached on line 314. PDFKit defers the actual flush, so in practice it works, but it's fragile. And reject() passes no error, plus write-stream errors (ENOSPC, EACCES, bad directory) are never observed at all — a failed write resolves or hangs rather than rejecting.
Confirmed — and it's not subtle
I built a 120-line invoice, awaited generate({type: "file", path}), then immediately readFileSync'd it:
On the current release the awaited promise resolved with nothing yet flushed to disk. Your intermittent "invalid PDF data" is exactly this — you're racing the write stream, and whether you get 0 bytes, a truncated file, or (luckily) a complete one depends on scheduling and invoice size.
What changed in lib/index.ts
The non-file branch (generate() returning the raw PDFKit.PDFDocument) is untouched, and tsc --noEmit passes.
One note on your call site: since generate() is overloaded, the {type: "file"} path returns Promise — so return new MicroInvoice(...).generate(...) is correct as long as the caller awaits it. Once you pick up this fix, awaiting it will actually mean what you expect.