Repository navigation
Conversation
When send_task failed, import_users_f returned the task as if it had been sent, so the import stayed In Progress with nothing reporting the failure. It now logs the error and marks the task Failed with the reason, only while it is still in progress, so a worker's final status is not overwritten. The response still carries the task, so the portal's widget follows it and shows the failure. The message says it is not known whether the task reached the queue: a failed send doesn't prove the broker never got it, and nothing confirms delivery either way. Parent issue: sequentech/meta#14271 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0129Cs8ygVE7UH6N1XzXk8WP
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review: 2
Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
Notify when the import action rejects and reflect local failure status while a widget has no task id. Once assigned, retain the persisted task status so a worker success is never replaced by local failure. Parent issue: sequentech/meta#14271
This branch has not been deployed
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.
Parent issue: https://github.com/sequentech/meta/issues/14271
A voter import stayed Pending after RabbitMQ was drained during node consolidation:
import_users_fignored thesend_taskerror and returned HTTP 200 with the task still In Progress, so nothing reported the failure.Change
packages/harvest/src/routes/users.rs,import_users_f. Whensend_taskfails:{err:?}).update_export_fail. That update applies only while the task is stillIN_PROGRESS, so a worker's final status is never overwritten.The Portal also shows the existing import-error notification if the action rejects (including failure to persist the enqueue error). A widget without a task id now follows its local failure status; once a task id is assigned, persisted worker status remains authoritative.
Successful sends behave as before.
What a failed send means
InvalidChannelState). Harvest didn't record the error it swallowed in the incident; what is known is the broker shutdown and the missing send and worker-receipt markers. The isolated broker check below reproducedInvalidChannelStateafter the same kind of shutdown, which supports, but doesn't prove, that this was the error. Other send errors, such as I/O failures while writing, could happen after the broker received the task.Findeton/rusty-celery@b4145925) doesn't wait for publisher confirms, so a successful send doesn't confirm the task was queued either.Recovery after the broker comes back
This PR doesn't change recovery. Harvest's live and ready probes already call
broker.reconnect(windmill/src/services/probe.rs), which rebuilds the connection and channels once the broker is back. This existing recovery is consistent with the later import succeeding without a Harvest restart.Testing
Follow-up Portal regression check (2026-10-18): isolated React/jsdom harness using actual Widget components on all three branches reproduced the baseline Pending state and verified local FAILED, preservation of persisted SUCCESS, and later worker SUCCESS. Executed import handlers with a synthetic rejected mutation and no election event: the new error notification is shown. No shared services were used. Both fresh independent reviewers re-reviewed the correction with no remaining actionable findings.
cargo check -p harvest --lockedpasses on this branch.Synthetic broker check (pinned rusty-celery and lapin, against a throwaway RabbitMQ 3.12.11 container, not the shared one):
docker stopclosed the connection withconnection_forced ... 'shutdown', as in the incident;send_taskreturnedErr(BrokerError(AMQPError(InvalidChannelState(Error))));broker.reconnect(what the probes call) ran, and then succeeded;Hasura behaviour (checked against a throwaway Hasura v2.33.1): a 5xx action response reaches the client as a generic
internal error, so the failure is reported through the task the portal already tracks, not through an HTTP error. A 5xx is still returned if the task can't be marked Failed; the portal'scatchthen marks its widget failed.Reviews: two independent reviews, one of task status and API/UI propagation, one of AMQP recovery, delivery ambiguity and the backports. Both passed after their findings were fixed.
Not done: no end-to-end import against a real Harvest and RabbitMQ.
Not in this PR
Keeping RabbitMQ from being evicted by node consolidation during elections (a PodDisruptionBudget or
karpenter.sh/do-not-disrupt) belongs in the infrastructure repo.🤖 Generated with Claude Code
https://claude.ai/code/session_0129Cs8ygVE7UH6N1XzXk8WP