Skip pages that pass validation but fail to read during concatenation - #1105
Conversation
|
Not sure about this one, since we're silently skipping a file that the user uploaded, right? |
Yes that's correct, that was happening, sorry I missed it the first time. I fixed it now, the exhibit marks itself broken when this happens, so the warning shows it instead of it silently disappearing. My guess is it may be tied to the image conversion itself, I saw a few JPG conversion failures right before the crash in at least one session, though I'm not sure yet if that's the actual cause or something else going on, like a timing or file-handling issue. |
|
great, with that clarification + update will leave for @BryceStevenWilley to finish review unless he prefers me to finish |
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The changes are narrowly scoped, add test coverage for the new behavior, and primarily improve failure handling without introducing risky logic changes.
Review effort: Lite
Findings: 1
Open (2)
What changed in this PR
This PR improves resilience of exhibit PDF generation in al_document.py by handling pdf_concatenate failures that can occur even after pages/exhibits pass earlier validation, preventing “no valid files to concatenate” crashes and surfacing failures to users via the existing “broken” warning mechanism.
Changes:
- Catch
DAErrorfrompdf_concatenateduring exhibit/page concatenation and skip the failing output instead of crashing. - Track exhibits that fail during processing (
_failed_during_processing) so they appear as broken (and therefore show up in the warning banner). - Update warning text and add unit tests covering the new failure mode and broken-exhibit reporting.
| File | Description |
|---|---|
| docassemble/AssemblyLine/al_document.py | Adds DAError handling around concatenation, tracks processing failures as “broken,” and updates the warning banner wording. |
| docassemble/AssemblyLine/test_al_document.py | Adds tests for concatenation failure behavior and updates assertions for the new warning text. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There were few "no valid files to concatenate" crashes even after the last fix. The page already passed the check, but the file still failed when the code went to use it, probably a storage write that wasn't fully finished. That specific step was not handled, so it crashed the whole document instead of just skipping that one piece.
This catches the failure right where it happens and skips it instead of crashing, across the exhibit, the exhibit list, and the combined exhibit doc.
Also added: when an exhibit fails this way, it now marks itself as broken so the existing warning message shows it to the user, instead of it just quietly disappearing from the download. Updated the warning text too, since it used to say the file didn't upload correctly, but the upload was fine, it's the processing afterward that failed.
It stops the crash and tells the user something went wrong.