toneequal: unlocked pipe_order test lets a second pipe strand luminance_valid

Found: by the gui_data lock-check scan
(tools/dt-lockcheck/dt-lockcheck.py, key toneequal:pipe_order)
Tree: dt-lockcheck at d5bf02a1ed; src/iop is unchanged from 037dee3956.

Summary

toneeq_process() guards its cache-invalidation block with a broken double-checked lock:
the condition is tested outside the module GUI lock and never re-tested inside. Two screen
pipes can therefore both run the block. When the second one runs it after the preview pipe
has already published its result, it clears g->luminance_valid over a mask that is fully
computed, and nothing puts the flag back — the preview branch holds the only
luminance_valid = TRUE in the file. Every GTK reader gated on that flag goes inert until
some other event makes the preview pipe run again.

No reorder is needed. g->pipe_order starts at 0, so every pipe sees the mismatch on
the module's first darkroom run.

The defect

1036:    if(g->pipe_order != piece->module->iop_order)   // read, no lock held
1037:    {
1038:      dt_iop_gui_enter_critical_section(self);
1039:      g->ui_preview_hash = DT_INVALID_HASH;
1040:      g->pipe_order = piece->module->iop_order;      // condition never re-tested
1041:      g->luminance_valid = FALSE;
1042:      g->histogram_valid = FALSE;
1043:      dt_iop_gui_leave_critical_section(self);
1044:      dt_preview_data_invalidate(&g->pd);
1045:    }

The lock serialises the writes but not the decision to make them, so two threads that both
read the stale pipe_order both enter. Nothing serialises them earlier: each processing
job holds only its own pipe's mutex (src/develop/develop.c:663, released at :939),
while piece->module = module (src/develop/pixelpipe_hb.c:537) gives every pipe's piece
the same dt_iop_module_t, hence the same gui_data and the same gui_lock. The block
sits before the per-pipe branch at :1047, so full, preview and preview2 all reach it.

The interleaving

1. Two pipes evaluate :1036 and both see the mismatch.
2. The preview pipe takes the section, records pipe_order, clears both validity flags,
   leaves, and goes down the dt_pipe_is_preview branch at :1115.
3. It clears again at :1130-1131, computes the mask at :1134, commits the preview hash
   at :1135 and publishes g->luminance_valid = TRUE at :1138.
4. The second pipe — descheduled between :1036 and :1038 — now enters and re-runs the
   stale invalidation, clearing luminance_valid at :1041 and discarding the preview's
   freshly committed hash at :1044.
5. The full branch (:1099-1114) computes its own mask at :1111 and commits its own
   cache key at :1112. It never writes luminance_valid — :1138 is the only site
   in the file that sets it TRUE.

The flag is left FALSE over a computed mask.

Step 4 needs the second pipe to be descheduled inside a two-statement window for the
duration of the first pipe's compute_luminance_mask(), so the per-run probability is
low. It is not exotic: that computation is OpenMP-parallel and saturates every core, so a
thread that loses its slice right there is competing with those workers to get it back.

Why both pipes see the mismatch on the first run

g->pipe_order is initialised to 0 in gui_init() (:1376), and a module running in a
pipe never has iop_order == 0 — _ioppr_reset_iop_order() assigns orders from 100
upwards in steps of 100 (src/common/iop_order.c:1246-1253). So the precondition holds
on the first run after the module's GUI is built, with no module reorder involved. It
becomes reachable again after an order resynchronisation (dt_ioppr_resync_modules_order()
from the history path, src/libs/history.c:639) or a GUI re-init.

What the user loses

line	site	effect
:1479	update_histogram()	histogram never recomputed — the gate is !histogram_valid && luminance_valid
:1807	auto_adjust_exposure_boost()	quad button aborts with "wait for the preview to finish recomputing" (:1809)
:1873	auto_adjust_contrast_boost()	same message
:2062	mouse_moved()	cursor exposure stops tracking
:2174	scrolled()	:2181 returns 1 — scroll-over-image correction is consumed and silently discarded
:2277, :2291, :2308	gui_post_expose()	readout freezes, then falls back to "? EV" (:2311)
:2336	gui_post_expose()	nearest-node highlight off

Not affected: the on-canvas luminance mask overlay, which is gated by g->mask_display at
:1157 and drawn from the full pipe's own buffer (:1062); and the processed image,
which is unaffected either way.

The least discoverable symptom is the scroll: the event is marked handled, so nothing
happens and nothing says why.

How it heals

On the next preview-pipe run for any reason. Every call to invalidate_luminance_cache()
ends in dt_iop_refresh_all() (:637), and it is reached from gui_update() (:1729),
gui_changed() (:1746, :1750, :1756) and both auto-adjust quads — so any parameter
or history change repairs it, and once pipe_order matches, :1036 is false and the
block is skipped. What it does not heal on is hovering, scrolling or redrawing, which is
exactly what a user does with this module before touching a slider.

Suggested fix

Re-test the condition after taking the lock. dt_preview_data_invalidate() must stay
outside the section: it re-enters the same lock at src/develop/preview_data.c:226, and
gui_lock is created non-recursive (dt_pthread_mutex_init(&module->gui_lock, NULL),
src/develop/imageop.c:1441), so folding it in deadlocks the pipe thread against itself
and freezes the darkroom. This is the trap dev-doc/GUI_Threading.md warns about under
"The Lock Is Not Recursive".

    dt_iop_gui_enter_critical_section(self);
    const gboolean reordered = (g->pipe_order != piece->module->iop_order);
    if(reordered)
    {
      g->ui_preview_hash = DT_INVALID_HASH;
      g->pipe_order = piece->module->iop_order;
      g->luminance_valid = FALSE;
      g->histogram_valid = FALSE;
    }
    dt_iop_gui_leave_critical_section(self);
    if(reordered) dt_preview_data_invalidate(&g->pd);

A structural alternative, not required to close this defect: the block conflates two
caches. ui_preview_hash is the full pipe's own cache key, while luminance_valid,
histogram_valid and g->pd describe the preview pipe's mask. Having whichever pipe
notices the reorder first invalidate the other's state is what makes the missing re-test
harmful rather than merely redundant.

What this fix does not close

A second publication race on the same flag, which the fix leaves untouched: the GTK thread
clears luminance_valid at :632 while the preview pipe is between its clear at :1131
and its publish at :1138, and the preview then publishes TRUE over a mask computed
from parameters the user has already superseded, committing a hash at :1135 that makes
it look fresh. That is a producer-side generation problem — the publisher does not check
whether it was invalidated while it was computing — and needs a generation counter or
equivalent, not a lock. Same family as #22068, which covers the consumer side of this
module's flag protocol.

(The pipe_order block gives the pipe threads a route to the same mirror ordering, but
only while :1036 is true; the fix above closes that route, because reaching :1131
means having already passed :1040.)

Collateral: histogram_valid

histogram_valid is cleared by the same block at :1042 and its only restoration is
update_histogram() at :1486, which is itself gated on luminance_valid at :1479. So
the stuck flag holds the histogram down too, and the fix above repairs both. This is not a
separate defect at those sites — both writes are inside critical sections.

histogram_valid does have unlocked GTK reads of its own, at :1807, :1873 and :2639
(the last in area_draw(), outside that function's only section at :2612-2615), racing
the pipe writes at :1042 and :1130. Those are the consumer-side shape #22068 already
documents for luminance_valid and belong there, not here. They are also much weaker:
g->histogram itself is written only on the GTK thread (:1482, under the lock), so the
worst case is one frame drawn from a histogram just marked stale — no torn buffer, no
use-after-free.

Debated and rejected

• "Both pipes write the same iop_order, so the block is idempotent and the race is
  formal." Two of three assessment rounds stopped here. The harm is not in pipe_order's
  value but in the three other fields the block writes, one of which only the preview
  branch can restore.
• "!dt_pipe_processing(dev->full.pipe) in the readers covers it." It gates on the full
  pipe being busy; the stuck state outlives the full pipe's run.
• "dt_dev_pixelpipe_cache_invalidate_later() at :1140 forces a repairing re-run." It
  invalidates pixelpipe cache entries on piece->pipe, runs on the preview branch before
  the stale clear in the harmful ordering, and schedules no preview job.
• "The stuck flag also defeats the full pipe's own mask cache at :1108, on every pan and
  zoom." Raised and withdrawn. The ROI is part of the hash
  (src/develop/pixelpipe_cache.c:168), so pan and zoom recompute regardless, and with ROI
  and parameters unchanged the pixelpipe cache serves the output without calling
  toneeq_process() at all.

Start at toneeq_process() and the pipe_order block identified by tools/dt-lockcheck/dt-lockcheck.py. Read dev-doc/GUI_Threading.md, especially the non-recursive lock guidance, and compare the lock usage with src/develop/preview_data.c:226. Done means the invalidation decision is synchronized without moving dt_preview_data_invalidate() into the critical section, and the lock-check finding is addressed.