fix: record why an async publish failed, and restore the retry #1529 disabled - #2171
Open
netomi wants to merge 2 commits into
Open
fix: record why an async publish failed, and restore the retry #1529 disabled#2171netomi wants to merge 2 commits into
netomi wants to merge 2 commits into
Conversation
Two things behind #1450's second complaint - that a publish which fails leaves the version inactive while the CLI reports success. An OutOfMemoryError is an Error, and publishAsync caught Exception, so the failure that prompted the report walked straight past the one handler that would have recorded it: even an instance with scanning enabled marked nothing, and the only trace was Spring's SimpleAsyncUncaughtExceptionHandler line, which names the method rather than the extension it was publishing. Catch Throwable, log which version it was and that the version stays inactive, mark the scan errored where there is one, and rethrow. The retry has not run since #1529. That PR extracted the body of the public @async @retryable publishAsync into a private doPublish and moved @retryable with it - onto a method reached by self-invocation, where no proxy can apply it. The annotation, and the comment above the file-resource cleanup that exists to make an attempt repeatable, have been describing something that does not happen. Put it back on the public methods where the advice can see it, with includes = Exception.class so that an Error fails on the first attempt rather than being retried into a JVM that has just run out of room - which also matches what spring-retry retried before the move. The new test builds the handler behind real async and retry advice, because that is where this behaviour lives: with @retryable back on the private method, retriesAFailedPublish sees one attempt instead of four and fails. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Async publishing is a deliberate design decision - storing, signing and checksumming a package are slow and have no business holding the upload request open. What was missing is that when that work fails, nothing writes down why. The row keeps active = false, and an operator looking at it has only a stack trace in the log naming publishAsync rather than the version it was working on, to be correlated by hand (#1450). Add extension_version.publish_error, set when an attempt fails and cleared when a version is activated, so it always describes the latest attempt. It holds the failure's type and message and nothing deeper: a cause chain carries paths, host names and connection strings into a column that tooling reads back, and the log keeps the full account anyway. Recorded in its own transaction, because the one the failure happened in is on its way to being rolled back and would take the record with it. The user-facing side of the same gap: enrichWithReviewStatus is driven entirely by scan records, so on an instance that does not run scanning an inactive version reported "Your extension is being reviewed" indefinitely - for a version nothing was reviewing, or ever would. A recorded failure now outranks every scan state there. The reason itself is not part of that response; the publisher gets told to contact the operator, who has the column. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Follow-up to #2170, covering the second half of #1450: a version whose publish fails just stays inactive.
Async publishing is a deliberate design decision — storing, signing and checksumming a package are slow and have no business holding the upload request open. The gap is that when that work fails, nothing writes down why.
Nothing recorded the reason
A failed attempt left the row at
active = falseand that was the whole of it. The only account was Spring'sSimpleAsyncUncaughtExceptionHandlerline, which namesPublishExtensionVersionHandler.publishAsyncand not the extension it was working on — so an operator finding an inactive version had to correlate it against a stack trace by hand, which is exactly what the reporter did.V1_75addsextension_version.publish_error, set when an attempt fails and cleared when a version is activated, so it always describes the latest attempt rather than accumulating history.Two deliberate choices in it:
REQUIRES_NEW), because the transaction the failure happened in is on its way to being rolled back and would take the record with it.The user-facing side of the same gap
UserAPI.enrichWithReviewStatusis driven entirely by scan records. On an instance that does not run scanning — the reporter's setup —scanResultis null, so an inactive version reported "Your extension is being reviewed" indefinitely, for a version nothing was reviewing or ever would.A recorded failure now outranks every scan state there.
rejectedis the closest of the three statuses that vocabulary has (the version will not become live without someone intervening) whereunder_reviewpromises attention nothing is giving it. The recorded reason itself stays out of that response — it names server internals and there is nothing in it a publisher could act on — so they are pointed at the operator, who has the column.The Error walked past the catch
publishAsynccaughtExceptionin order to callmarkScanAsErrored.OutOfMemoryErroris anError, so the very failure that prompted #1450 slipped past it — even on an instance running scanning, nothing was recorded. Now it catchesThrowable, records the reason, names the version in the log, and marks the scan where there is one.The retry has not run since #1529
doPublishcarries@Retryable, and the comment above its file-resource cleanup — "Delete file resources in case publishAsync is retried" — exists to make an attempt repeatable. Neither has done anything for a while.Before #1529 the method was:
Public, called from
ExtensionServicethrough the proxy, so both annotations applied. #1529 extracted the body into a privatedoPublishand moved@Retryableonto it — a method reached only by self-invocation, where no proxy can ever apply it. Nothing failed; the retry just quietly stopped happening. (@EnableResilientMethodsis onRegistryApplication, and the@Retryableon the publiccreateExtensionVersiondoes still work, which is what makes this easy to miss.)Moved back onto the public methods, with one deliberate change:
includes = Exception.class, so anErrorfails on the first attempt instead of being retried three more times into a JVM that has just said it has no room — the exact scenario in #1450. That also matches what was retried before the move: spring-retry'sSimpleRetryPolicydefaults toException, notThrowable.Tests
PublishExtensionVersionHandlerRetryTestbuilds the handler behind real async and retry advice, rather than constructing it directly the way the existing handler test does — the behaviour under test is the advice, so a bare object cannot show it. A synchronous executor makes@Asyncrun inline so nothing has to wait.recordsWhyThePublishDidNotFinish— the reason reaches the version.recordsTheFailureTypeWhenItCarriesNoMessage— the type alone, not a barenullappended to it.retriesAFailedPublish— a storage failure is attempted four times (one plus three retries).doesNotRetryAnError— anOutOfMemoryErroris attempted once.recordsAnErrorAgainstTheScan— the scan is marked errored with theOutOfMemoryErrornamed.Plus, on the service: the reason is written onto the managed row, a purged row is ignored rather than turning a failed publish into a second failure, and activation clears an earlier failure.
I checked the retry test actually bites rather than trusting a green run: with
@Retryableback on the privatedoPublish, it fails with one attempt instead of four. The migration was applied against a real Postgres 16 to confirm the column lands nullable with no default — which also meansimport-db-dump.shcorrectly treats it as needing no backfill.Full server suite green (1175 tests), formatter clean.
Not in here
Surfacing the recorded reason through the admin API, so an operator can read it without reaching for
psql. The column is the prerequisite; where it gets displayed is a separate decision.🤖 Generated with Claude Code