fix(kb): deleting a document mid-upload leaked bytes and spammed 404 dialogs - #1059
Merged
Merged
Conversation
…dialogs Found by deleting an `uploading` document in dev. Two independent defects; a third (more serious) is deliberately NOT in this change — see below. 1. FIVE "Not found" DIALOGS PER DELETE. The polling loop already tolerates up to five consecutive 404s before giving up, and the component already handled the resulting DOCUMENT_NOT_FOUND cleanly — but the global errorInterceptor pops a dialog for every failed request BEFORE any caller's catch runs. So correct handling was invisible and the user got one dialog per tolerated retry. The poll's reads now set SUPPRESS_ERROR_TOAST, which exists for exactly this case. Also made the poll actually stoppable. `deleteDocument` already dropped the id from `pollingDocuments`, but that signal was display-only — the running loop never read it, so it kept asking about a deleted row until its 404 budget ran out. `pollDocumentStatus` now takes an `isCancelled` callback and the component passes that same set membership, so the existing removal finally means something. POLL_CANCELLED is handled beside DOCUMENT_NOT_FOUND: both are ordinary outcomes, and reloading the list on either would race the optimistic delete and flash the row back. 2. A SILENT, CUMULATIVE BYTE-CAP LEAK. The request-time reservation is released on every abandon path except deletion: ingestion reaching terminal, a client-reported upload failure, and the stale sweep. A deleted document reaches none of them, so its reservation was stranded forever. `soft_delete_document` now releases it through `release_reservation_if_managed`, whose `settle_once` stamp makes it exactly-once against the other three paths. This is the worst shape a bug can have: invisible, cumulative and delayed. Each cancelled upload permanently shaved bytes off that assistant's allowance, surfacing months later as "uploads stopped working" with no failure anywhere near the deletes that caused it — which is what byte_cap.release's own docstring warns about. NOT FIXED HERE, by agreement: neither ingestion pipeline checks whether a document is `deleting`. Grepped both — no such guard exists. If the S3 PUT completes after the delete, the event fires and the managed consumer ingests the document into Bedrock and writes `complete` over `deleting`, so deleted content becomes answerable again and the row returns. That is a data-correctness bug touching the live ingest path and deserves its own change with its own mutation guards, not a ride-along. Tests: 6 new (tests/routes/test_document_delete_releases_bytes.py), covering release on delete, exactly-once under a re-delete, legacy left untouched, an already-settled complete document not double-credited, a zero-size imported row as a no-op, and a missing document changing nothing. Mutation guard verified: removing the release call fails two of them with reservedBytes still charged. 55 backend tests green across the document suites, ruff clean, tsc clean.
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.
Found by deleting an
uploadingdocument in dev. Two defects fixed; a third, more serious one is deliberately left out and described at the bottom.1. Five "Not found" dialogs per delete
The polling loop already tolerated up to five consecutive 404s, and the component already handled the resulting
DOCUMENT_NOT_FOUNDcleanly. The noise came from somewhere else entirely: the globalerrorInterceptorpops a dialog for every failed request before any caller'scatchruns. So the code was handling it correctly and invisibly, while the user got one dialog per tolerated retry.The poll's reads now set
SUPPRESS_ERROR_TOAST— the token that exists for precisely this case, per its own doc comment.And the poll is now actually stoppable.
deleteDocumentalready removed the id frompollingDocuments, but that was a display-only signal — the running loop never read it, so it kept asking about a deleted row until its 404 budget ran out.pollDocumentStatustakes anisCancelledcallback and the component passes that same set membership, so the removal that was already there finally means something.POLL_CANCELLEDis handled alongsideDOCUMENT_NOT_FOUND: both are ordinary outcomes rather than faults, and reloading the list on either would race the optimistic delete and flash the deleted row back into view.2. A silent, cumulative byte-cap leak
This is the one worth caring about.
The request-time byte reservation is released on every abandon path except deletion — ingestion reaching a terminal state, a client-reported upload failure, and the stale sweep. A deleted document reaches none of them, so its reservation was stranded permanently.
soft_delete_documentnow releases it viarelease_reservation_if_managed, whosesettle_oncestamp keeps it exactly-once against the other three paths.It had the worst shape a bug can have: invisible, cumulative, and delayed. Every cancelled upload permanently shaved bytes off that assistant's allowance, and it would surface months later as "uploads stopped working" with no failure anywhere near the deletes that caused it.
byte_cap.release's own docstring warns about exactly this.Deliberately NOT fixed here
Neither ingestion pipeline checks whether a document is
deleting. I grepped both the managed consumer and the legacy handler; no such guard exists.So if the S3 upload completes after the delete — entirely likely, since delete is instant and the PUT may still be in flight — the S3 event fires, the managed consumer ingests the document into Bedrock, and writes
completeoverdeleting. Deleted content becomes answerable again and the row comes back to life.That is a data-correctness bug on the live ingestion path. It deserves its own change with its own mutation guards rather than riding along with a toast fix, so it was scoped out by agreement.
Tests
6 new in
backend/tests/routes/test_document_delete_releases_bytes.py:completedocument not double-credited (would drive counters negative)Mutation guard verified: removing the
release_reservation_if_managedcall fails two of them withreservedBytesstill charged.55 backend tests green across the document suites ·
ruffclean ·tsc --noEmitclean.