Skip to content

fix(waterfall): immutable per-generation adapter — no more racing in-flight jobs or stale draws#33

Merged
jeandet merged 2 commits into
SciQLop:mainfrom
jeandet:fix/waterfall-snapshot
Jun 12, 2026
Merged

fix(waterfall): immutable per-generation adapter — no more racing in-flight jobs or stale draws#33
jeandet merged 2 commits into
SciQLop:mainfrom
jeandet:fix/waterfall-snapshot

Conversation

@jeandet

@jeandet jeandet commented Jun 12, 2026

Copy link
Copy Markdown
Member

Stacked on #32 — merge that first; this PR's diff then collapses to its own commit (6ab2e66).

QCPWaterfallGraph was the one plottable that broke the pipeline's immutable-snapshot contract: it handed the resampler a single long-lived mutable QCPWaterfallDataAdapter, then mutated it freely — draw() reassigned offsets/norm-factors/gain on every paint and setDataSource() swapped the inner source, all while pool jobs read those members through keyAt()/valueAt(). Two user-visible consequences:

  • data race / use-after-free: an L1 build mid-iteration could observe a half-written QVector or keep iterating an inner source that new data had just destroyed (the job's shared_ptr kept only the adapter alive, not the source);
  • stale draws: setGain/setOffsets/setNormalize invalidated no cache, so a parameter change was a visual no-op until something else dirtied the line cache — while autoscale (getValueRange, which DID refresh the adapter) already used the new value, drawing lines inconsistent with the axis range.

The adapter is now an immutable snapshot (const members; co-owns its inner source) and QCPWaterfallGraph::rebuildAdapter() builds a fresh one on every data or parameter change, pushing it through QCPMultiGraph::setDataSource — which already invalidates line/L1/L2 caches. draw() no longer mutates anything; in-flight jobs finish against their own generation.

Reproducers: TestWaterfall::parameterChangesApplyWithoutDraw (failed before: gain changes were invisible to the data source until the next draw) and TestWaterfall::concurrentParameterAndDataChangesAreSafe (ASan regression guard hammering data+parameter changes against in-flight L1 jobs; the race is timing-dependent, so this guards rather than proves). Full suite ASan-clean; all 15 waterfall tests pass.

Part of the 2026-06-09 audit (C4/H6 — one design fix covers both). Stack: #32this → range scans → small sweep.

🤖 Generated with Claude Code

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR strengthens async pipeline “immutable snapshot” semantics for plottables, primarily by making QCPWaterfallGraph stop mutating a shared adapter while background resample jobs are running. It also extends the pipeline safety work (scheduler shutdown behavior, pipeline destruction guard, and colormap transform capture) and adds/updates regression tests for the identified race/UAF scenarios.

Changes:

  • Make QCPWaterfallDataAdapter an immutable per-generation snapshot and rebuild/swap adapters on every data/parameter change (no adapter mutation in draw()).
  • Harden async infrastructure: drop queued jobs on scheduler destruction; prevent TOCTOU between “destroyed” checks and queued delivery; capture colormap gap-threshold by value and re-bake on change.
  • Add/adjust tests to cover waterfall snapshot semantics, concurrent safety, and fix test-side lifetime issues (dangling spans / stack views).

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
tests/auto/test-waterfall/test-waterfall.h Adds new waterfall snapshot/race regression test declarations.
tests/auto/test-waterfall/test-waterfall.cpp Adds tests for immediate parameter effect and concurrent parameter/data changes while async jobs run.
tests/auto/test-scatter-rhi/test-scatter-rhi.cpp Fixes test to use owning setData instead of dangling std::span views.
tests/auto/test-pipeline/test-pipeline.h Adds new pipeline safety regression test declarations.
tests/auto/test-pipeline/test-pipeline.cpp Adds scheduler-dtor and colormap queued-job-after-delete regression tests.
tests/auto/test-datasource2d/test-datasource2d.cpp Drains pipeline after viewData to avoid stack-use-after-return in async jobs.
src/plottables/plottable-waterfall.h Makes adapter immutable; moves waterfall graph to rebuildAdapter() model and removes draw() mutation path.
src/plottables/plottable-waterfall.cpp Implements immutable adapter creation/swap on parameter/data changes; removes adapter mutation from draw() and range logic.
src/plottables/plottable-colormap2.h Changes gap-threshold API to setter function; stores threshold as a plain double.
src/plottables/plottable-colormap2.cpp Installs a self-contained resample transform capturing threshold by value; re-bakes transform on threshold changes.
src/datasource/pipeline-scheduler.h Adds shutdown flag to prevent running queued jobs during destruction.
src/datasource/pipeline-scheduler.cpp Drops queued work and rejects new submissions during scheduler destruction; prevents epilogue from starting new work.
src/datasource/async-pipeline.h Replaces atomic destroyed flag with a mutex-guarded destroy guard held across check+invoke.
src/datasource/async-pipeline.cpp Sets destroy guard under mutex in destructor; removes redundant destroyed-check in deliverResult.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +134 to 138
void QCPWaterfallGraph::rebuildAdapter()
{
if (!mOriginalSource) return;
if (!mOriginalSource)
return;
if (mNormDirty)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Confirmed — this was a regression of the snapshot rework: the pre-PR code cleared the inner source through the mutable adapter, so setDataSource(nullptr) used to empty the graph. Worse, range queries consult mOriginalSource (reporting "no data") while draw() used the stale installed adapter, so the two disagreed. Fixed in 6467bcf: the null-source path now drops the adapter and forwards null to QCPMultiGraph::setDataSource, which already resets the caches and pipeline source — same state as a freshly constructed graph. Reproducer: clearingSourceEmptiesTheGraph (failed before the fix).

@jeandet jeandet force-pushed the fix/waterfall-snapshot branch from 6ab2e66 to 5099672 Compare June 12, 2026 16:51
jeandet added a commit to jeandet/NeoQCP that referenced this pull request Jun 12, 2026
rebuildAdapter() early-returned on a null source, so setDataSource(nullptr)
left the previous adapter installed in QCPMultiGraph: the graph kept
rendering the old data while getKeyRange() already reported no data.
Pre-snapshot code cleared the inner source through the mutable adapter, so
this was a regression of the immutable-adapter rework.

Reproducer: clearingSourceEmptiesTheGraph (review follow-up on PR SciQLop#33).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
jeandet and others added 2 commits June 12, 2026 21:53
…flight jobs or stale draws

QCPWaterfallGraph was the one plottable that broke the pipeline's
immutable-snapshot contract: it handed the resampler a single long-lived
mutable QCPWaterfallDataAdapter, then mutated it freely — draw() reassigned
offsets/norm-factors/gain on every paint and setDataSource() swapped the
inner source, all while pool jobs read those members through
keyAt()/valueAt(). Two user-visible consequences:

- data race / use-after-free: an L1 build mid-iteration could observe a
  half-written QVector or keep iterating an inner source that new data had
  just destroyed (the job's shared_ptr kept only the adapter alive, not
  the source);

- stale draws: setGain/setOffsets/setNormalize invalidated no cache, so a
  parameter change was a visual no-op until something else dirtied the
  line cache — while autoscale (getValueRange, which DID refresh the
  adapter) already used the new value, drawing lines inconsistent with
  the axis range.

The adapter is now an immutable snapshot (const members; co-owns its inner
source) and QCPWaterfallGraph::rebuildAdapter() builds a fresh one on every
data or parameter change, pushing it through QCPMultiGraph::setDataSource —
which already invalidates line/L1/L2 caches. draw() no longer mutates
anything; in-flight jobs finish against their own generation.

Reproducers: TestWaterfall::parameterChangesApplyWithoutDraw (failed before:
gain changes were invisible to the data source until the next draw) and
TestWaterfall::concurrentParameterAndDataChangesAreSafe (ASan regression
guard hammering data+parameter changes against in-flight L1 jobs; the race
is timing-dependent, so this guards rather than proves). Full suite
ASan-clean; all 15 waterfall tests pass.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
rebuildAdapter() early-returned on a null source, so setDataSource(nullptr)
left the previous adapter installed in QCPMultiGraph: the graph kept
rendering the old data while getKeyRange() already reported no data.
Pre-snapshot code cleared the inner source through the mutable adapter, so
this was a regression of the immutable-adapter rework.

Reproducer: clearingSourceEmptiesTheGraph (review follow-up on PR SciQLop#33).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@jeandet jeandet force-pushed the fix/waterfall-snapshot branch from 6467bcf to 852cfa2 Compare June 12, 2026 19:55
@jeandet jeandet merged commit cfa000b into SciQLop:main Jun 12, 2026
5 checks passed
@jeandet jeandet deleted the fix/waterfall-snapshot branch June 12, 2026 20:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants