GeezerVison

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

GeezerVison is showing github.com — [python] Report a pipeline restart instead of an empty view in the aggtest harness by bestouff · Pull Request #7816 · feldera/feldera

Conversation

bestouff commented Nov 3, 2026

Copy link

Copy Markdown

Contributor

Fixes the misleading failure in #7688 and the post-ingestion variant of #6986.

A pipeline pod that restarts after ingestion holds empty tables, reports Running, and answers every ad-hoc query with an empty result. The harness then blamed the first view it queried:

ASSERTION ERROR: failed view: varchar_arg_min_gby
Received: []

Completion tokens only catch a restart during ingestion. This change records the incarnation UUID after start and, when a view check fails, compares it again. A changed UUID raises PipelineRestartedError, which names both incarnations and the input record count of the new one.

Scenario	Before	After
View really wrong	AssertionError with the diff	unchanged
Pod restarted after ingestion	AssertionError, `Received: []`	`PipelineRestartedError: incarnation A became B, it now holds 0 input records`

Unit test in python/tests/unit/test_aggtst_restart_detection.py. With the check disabled the restart test fails, so it catches the regression.

Sorry, something went wrong.

Uh oh!

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

All reactions


          [python] Report a pipeline restart instead of an empty view in the ag…

…gtest harness

A pipeline pod that restarts after ingestion holds empty tables, reports
Running, and answers every ad-hoc query with an empty result. The harness
then blamed the first view it queried, for example in #7688:

    ASSERTION ERROR: failed view: varchar_arg_min_gby
    Received: []

Completion tokens only catch a restart during ingestion (#6986).
Record the incarnation UUID after start and, when a view check fails,
compare it again. A changed UUID raises PipelineRestartedError, which
names both incarnations and the input record count of the new one.

mythical-fred-oss Bot reviewed

Nov 3, 2026

View reviewed changes

Comment thread

python/tests/runtime_aggtest/aggtst_base.py

		view.validate(pipeline)
		except AssertionError as e:
		raise_if_restarted(pipeline, incarnation, e)
		raise

mythical-fred-oss Bot Nov 3, 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): an empty expected result hides the restart. The incarnation is compared only when a view fails. About 98 aggtest views expect [] (grep -rn "\.data = \[\]" tests/runtime_aggtest). After a post-ingestion restart, those views pass even though nothing was really checked. If every view in a pipeline expects [], the whole test goes green. Suggest also comparing the incarnation once after the for view in views loop, so a restart always fails the test:

for view in views:
    ...
raise_if_restarted(pipeline, incarnation, AssertionError("all views validated"))

(Or split the helper into a check_incarnation that does not need a cause.)

Sorry, something went wrong.

Uh oh!

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

All reactions

mythical-fred-oss Bot reviewed

Nov 3, 2026

View reviewed changes

Comment thread

python/tests/runtime_aggtest/aggtst_base.py

		tokens only catch a restart during ingestion (feldera#6986), so the view
		check compares the incarnation UUID recorded after start.
		"""
		metrics = pipeline.stats().global_metrics

mythical-fred-oss Bot Nov 3, 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.

pitfall (medium): a failing stats() call replaces the view diff. This code runs right after a suspected restart, so the pod may still be coming up and stats() can fail. If it raises (FelderaAPIError or a connection error), that error escapes and the outer handler reports RUNTIME ERROR: <api error> instead of the view diff. That makes the result worse than before this PR. Wrap the call in a try/except, and on failure re-raise the original AssertionError, maybe with a note that the incarnation could not be checked.

Sorry, something went wrong.

Uh oh!

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

All reactions

mythical-fred-oss Bot commented Nov 3, 2026

Copy link

Copy Markdown

Contributor

What I ran

Check	Result
`uv run --locked pytest tests/unit/test_aggtst_restart_detection.py`	3 passed
`pre-commit run --files <changed>`	passed (ruff, ruff-format)
runtime_aggtest against a live pipeline	not run (needs a manager and compiler)

Gates: tests ✅, no flakiness introduced ✅, commit message ✅, no deps/docs impact ✅.

Missing (medium): untested cases

A restart where every view expects []. It passes today; see the inline comment.
stats() raising inside raise_if_restarted. The view diff is lost.
run_pipeline wiring: no test checks that PipelineRestartedError reaches the caller inside RuntimeExecutionError, or that a plain mismatch still re-raises the original AssertionError. The unit tests cover only the helper.

Otherwise the approach looks good to me once the two inline issues are fixed.

All reactions

Sorry, something went wrong.

Uh oh!

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


          [python] Catch a restart behind views that expect no rows, keep the d…

46981cb

…iff when the probe fails

Review follow-up on the aggtest restart check:

- Compare the incarnation once more after all views validated. About
  98 aggtest views expect no rows, and those pass on an empty new
  incarnation, so a restart could turn a pipeline green.
- A failed stats() probe right after a suspected restart no longer
  replaces the view diff. The harness prints a warning and re-raises
  the original assertion.
- Unit tests now drive run_pipeline with a fake pipeline, so the
  wiring is covered: restart reported, plain mismatch kept, restart
  behind empty views, failed probe, and the passing case.

bestouff commented Nov 3, 2026

Copy link

Copy Markdown

Contributor Author

Both inline findings are addressed in the second commit, and the wiring is now covered:

Finding	Change
Views that expect no rows hide a restart	`check_incarnation` runs once more after all views validated
A failed `stats()` probe replaces the view diff	`raise_if_restarted` prints a warning and returns, the caller re-raises the original assertion
Only the helper was tested	Tests drive `run_pipeline` with a fake pipeline: restart reported, plain mismatch kept, restart behind empty views, failed probe, passing case

Removing the post-loop check fails test_restart_fails_views_that_expect_no_rows; narrowing the probe guard fails test_failed_probe_keeps_the_view_diff. The lateness_tests suite also ran green against the CI amd64 instance with this harness.

All reactions

Sorry, something went wrong.

Uh oh!

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

This was referenced Nov 3, 2026

Runtime Integration Tests (amd64): aggregate_tests4/varchar_arg_min_gby fails empty after 650s+ compile under slow CI #7688

Closed

Runtime Integration Tests (amd64): pipeline stopped mid-test, RuntimeExecutionError "Pipeline must be running to wait for completion" #6986

Closed

mihaibudiu approved these changes

Nov 3, 2026

View reviewed changes

bestouff commented Nov 3, 2026

Copy link

Copy Markdown

Contributor Author

What ?!? Approved like this, nothing to change, not even a slight comment ?

All reactions

Sorry, something went wrong.

Uh oh!

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

mihaibudiu commented Nov 3, 2026

Copy link

Copy Markdown

Contributor

I think you just crave human connection

blp reacted with laugh emoji

All reactions

😄 1 reaction

Sorry, something went wrong.

Uh oh!

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

bestouff commented Nov 4, 2026

Copy link

Copy Markdown

Contributor Author

Well, PR reviews are one of the only few pieces of human interaction we got in this job.

All reactions

Sorry, something went wrong.

Uh oh!

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

bestouff added this pull request to the merge queue

Nov 4, 2026

github-merge-queue Bot removed this pull request from the merge queue due to failed status checks

Nov 4, 2026

Uh oh!

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

mythical-fred-oss Bot mentioned this pull request

Nov 4, 2026

Java Tests job fails when sccache remote service is unreachable (Timed out waiting for server startup) #7833

Open

mythical-fred-oss Bot commented Nov 4, 2026

Copy link

Copy Markdown

Contributor

Verdict: infrastructure flake, not this PR. High confidence.


Failed job	Java Tests / Execute Java Tests
Failing test	`SaturatingOperationsTests.saturatingOperations` (via `BaseSQLTests.compileAndTestRust`)
Root cause	log:612-615 — `sccache: error: Timed out waiting for server startup. Maybe the remote service is unreachable?`, which makes the `rustc -vV` probe exit 101 before any Rust code compiles or runs

This PR only touches python/tests/runtime_aggtest/aggtst_base.py and python/tests/unit/test_aggtst_restart_detection.py — nothing in SQL-compiler or the sccache CI config. Only 1 of 2742 Java tests errored, and it errored on the compiler toolchain probe, not on any assertion. The same sccache: ... Timed out waiting for server startup message already shows up on #6763 against the production sccache backend, so the remote sccache service itself appears to be flaky right now.

Filed #7833 with the evidence and assigned it to the sccache config owner.

Next step: re-queueing should pass — nothing here points at this PR's diff.

All reactions

Sorry, something went wrong.

Uh oh!

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

bestouff added this pull request to the merge queue

Nov 4, 2026

Merged via the queue into main with commit 2afeaba

Nov 4, 2026

1 check passed

Uh oh!

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

bestouff deleted the issue7688 branch

November 4, 2026 16:57

Reviewers

mythical-fred-oss[bot]

mythical-fred-oss[bot] left review comments

mihaibudiu

mihaibudiu approved these changes

Uh oh!

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

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

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

2 participants

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.