GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — [connectors] Report the error that stopped the circuit thread in /status by blp · Pull Request #7867 · feldera/feldera

Conversation

@blp

@blp blp commented Nov 5, 2026 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

Fixes #2986.

When the circuit thread stopped with an error that is_fatal_controller_error does not classify as fatal, such as a fault-tolerance replay failure, /status reported only "Pipeline has been terminated." (PipelineTerminated). The cause was only in the log. Commit 0b927cc already made the pipeline fail in this case; this change makes it fail with the correct error.

File Change
controller/stats.rs New GlobalControllerMetrics::fatal_error: the first error that stopped the circuit thread.
controller.rs The circuit thread records its error before finish() sets Terminated, so an observer that sees Terminated also sees the cause.
server.rs terminated_status reports the recorded error with its own error code. The error formatting moves into controller_error_status, which the Failed branch also uses. This also corrects its log message, which named SERVICE_UNAVAILABLE as the code it converted to.

Tests

  • terminated_status_reports_the_fatal_error: /status mapping.
  • replay_failure_is_recorded_as_fatal_error: runs a fault-tolerant pipeline from a file, changes the file, restarts, and expects the pipeline to stop with ReplayFailure recorded.

Both tests fail when the fix is disabled.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@blp blp added bug Something isn't working ft Fault tolerant, distributed, and scale-out implementation connectors Issues related to the adapters/connectors crate rust Pull requests that update Rust code labels Nov 5, 2026
@blp blp self-assigned this Nov 5, 2026
mihaibudiu
mihaibudiu approved these changes Nov 5, 2026
View reviewed changes
@mythical-fred-oss

mythical-fred-oss Bot commented Nov 5, 2026

Copy link
Copy Markdown
Contributor

Review summary. No hard gate fails. The one finding is the misplaced doc comment noted inline.

What I ran

Command Result
cargo test --locked -p dbsp_connectors --lib -- terminated_status 3/3 pass
cargo test --locked -p static-connectors --test connector_suites -- replay_failure_is_recorded pass, and 25/25 on reruns (not flaky)

What I checked

  • Manager compatibility: nothing consumes the PipelineTerminated error code. The pipeline-manager automaton treats any non-200, non-503 /status response as fatal, so returning ReplayFailure (500) instead of PipelineTerminated (500) is safe. The 503→500 remap in controller_error_status prevents an error from being shown as Unavailable.
  • Ordering: set_fatal_error runs before finish() stores Terminated, with Release/Acquire ordering, so an observer that sees Terminated also sees the error.
  • Panic path: it doesn't record a fatal_error, but that's fine because ControllerPanic already moves the pipeline to Failed.

Missing (low): no test checks that suspended == true takes precedence over a recorded fatal_error in terminated_status. That is a one-line addition to terminated_status_reports_the_fatal_error if you want it.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
mythical-fred-oss[bot]
mythical-fred-oss Bot reviewed Nov 5, 2026
View reviewed changes
Comment thread crates/connectors/src/controller/stats.rs
/// Records the reason before the state, so that an observer that sees
/// [PipelineState::Terminated] also sees why.
/// The error that stopped the circuit thread, if one did.
pub fn fatal_error(&self) -> Option<Arc<ControllerError>> {

@mythical-fred-oss mythical-fred-oss Bot Nov 5, 2026

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bug (medium, docs): These methods were inserted between set_suspended and its doc comment. Now pub fn fatal_error has a doc that starts "Terminates the pipeline because a suspend completed successfully…", and set_suspended has no doc. Please move the three /// lines above this one back above fn set_suspended.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@blp blp Nov 5, 2026

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 82a00e6: the doc comment is back above set_suspended, and fatal_error has only its own doc.

-- Claude Code, replying at @blp's request.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@blp
[connectors] Report the error that stopped the circuit thread in /sta…
82a00e6 
…tus.

When the circuit thread stopped with an error that the server does not
classify as fatal, such as a fault-tolerance replay failure, /status
reported only "Pipeline has been terminated." (PipelineTerminated).  The
cause was only in the log.

The circuit thread now records the error before it terminates the
pipeline, and /status reports that error with its own error code, for
example ReplayFailure.

Fixes #2986.

Signed-off-by: Ben Pfaff <[email protected]>
@blp
blp force-pushed the ft-failure-fails-pipeline branch from b843e3d to 82a00e6 Compare November 5, 2026 23:09
@blp
blp added this pull request to the merge queue Nov 5, 2026
Merged via the queue into main with commit 4876dba Nov 6, 2026
1 check passed

Uh oh!

There was an error while loading. Please reload this page.
@blp
blp deleted the ft-failure-fails-pipeline branch November 6, 2026 04:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mythical-fred-oss mythical-fred-oss[bot] mythical-fred-oss[bot] left review comments
@mihaibudiu mihaibudiu mihaibudiu approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@blp blp

Labels

bug Something isn't working connectors Issues related to the adapters/connectors crate ft Fault tolerant, distributed, and scale-out implementation rust Pull requests that update Rust code

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

When fault tolerance detects a failure, pipeline should become Failed

Uh oh!

There was an error while loading. Please reload this page.

2 participants

@blp @mihaibudiu
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.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.