fix(dispatcher): treat a missing tarball object as a permanent error - #336
Draft
VascoSch92 wants to merge 1 commit into
Draft
Conversation
When an automation's TarballUpload row is live but its object is absent from storage, store.read raises FileNotFoundError. That is a plain Exception, so it escaped the PermanentDispatchError handler in dispatch step 4: _execute_run_safe marked the run FAILED with a bare "Internal error" and left the automation enabled, so it re-failed on every schedule tick (C24 saw 12 and 16 consecutive failures). Reclassify it as TarballNotFoundError, which already subclasses PermanentDispatchError. The existing handler then disables the automation and surfaces the real cause as error_detail, naming the missing storage path and telling the user to recreate the automation. This makes the failure legible and bounded; it does not address why the object went missing (MinIO durability and the non-transactional delete in the prompt-edit path are tracked separately). Ref: OSS-9505
Contributor
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.
Fixes the customer-visible half of OSS-9505 ([C24] HappyFox #OS00000160): automation "run now" failing with an opaque
Internal errorthat repeated on every schedule tick.This is fix (2) from the issue. Fixes (1) (crash-safe tarball regeneration) and (3) (MinIO durability, PLTF-3455) are not in this PR.
Warning
Do not merge as-is. Review turned up an over-trigger in this approach — see "Known defect" below. Pushing a narrowing commit.
The bug
_download_internal_tarballalready raisesTarballNotFoundErrorwhen theTarballUploadrow is missing, but the subsequentstore.read(upload.storage_path)was unguarded. When the row is live but its object is gone,s3.pymapsNoSuchKeytoFileNotFoundError— a plainException, which sails past theexcept PermanentDispatchErrorhandler in dispatch step 4 and lands in_execute_run_safe. Two consequences, both of which C24 hit:FAILEDwith the literal string"Internal error"and no detail;The fix
Wrap that one read and re-raise as
TarballNotFoundError, which already subclassesPermanentDispatchError. No new machinery — the existing handler disables the automation and surfacesstr(exc)aserror_detail. The message names the missing storage path and the remedy (recreate the automation), since the original bytes are not retained anywhere and delete-and-recreate is the only recovery.Known defect (being fixed)
s3.py::_handle_client_error(lines 262-276) maps everyClientErrortoFileNotFoundError, not justNoSuchKey:NoSuchBucket,AccessDenied, and anelsecatch-all covering 500InternalError, 503ServiceUnavailable,SlowDownthrottling and expired credentials all land there too.So as committed, this PR treats a transient storage hiccup as permanent. On C24 that is not hypothetical — their MinIO is OOM-crash-looping, so a read landing during a restart returns 5xx, which would now permanently disable a healthy automation. Today those runs simply fail and recover on the next tick, so this would be a regression for the exact customer it targets.
Narrowing: raise an
ObjectNotFoundError(FileNotFoundError)subclass only at the genuine-absence sites (s3.py:267-268,local.py:64,google_cloud.py:102,137) and catch that here instead of bareFileNotFoundError. Backward compatible — the seven existingexcept FileNotFoundErrorhandlers keep behaving identically via the parent class, and every otherClientErrorstays transient.Tests
Two tests, both verified to fail without the source change:
test_raises_tarball_not_found_for_missing_object— unit-level: liveCOMPLETEDrow, store raisesFileNotFoundError, asserts the reclassification and that the original error is chained as__cause__.test_disables_automation_on_missing_tarball_object— end-to-end through_execute_run: asserts the automation ends upenabled=False, thaterror_detailnames the real cause and is not"Internal error", and that execution never entered the sandbox.Full suite: 1148 passed. Pre-commit (ruff format/lint, pycodestyle, pyright) clean.
Notes for the issue thread
The attached
reconcile_tarballs.pycannot discriminate the two candidate root causes the way step 4 of its runbook claims. It saysRECORD_SOFT_DELETEDimplies the edit/rollback bug — but in that scenariosource_upload.deleted_atis only flushed, never committed, so the rollback reverts the soft-delete andtarball_pathtogether. The surviving state is a live,COMPLETEDrow with no object: verdictMISSING_OBJECT, identical to MinIO loss.RECORD_SOFT_DELETEDis effectively unreachable via that path, since both writes share one transaction. Breadth (are never-edited automations affected) remains a valid signal; worth also pulling 5xx responses onPATCH /automations/{id}from C24.Separate latent bug in the same area:
preset_router.py:378doesexcept FileNotFoundError: return None, so a transient storage error during a prompt edit makes regeneration silently no-op. The PATCH returns 200 and thepromptcolumn updates, but the tarball keeps the old baked prompt and nothing surfaces the divergence.