Skip to content

Guard end_scan() in fly_scan's finally so StartScan cannot wedge - #187

Open
xmap wants to merge 1 commit into
tomography:masterfrom
xmap:fix/end-scan-must-not-wedge-startscan
Open

Guard end_scan() in fly_scan's finally so StartScan cannot wedge#187
xmap wants to merge 1 commit into
tomography:masterfrom
xmap:fix/end-scan-must-not-wedge-startscan

Conversation

@xmap

@xmap xmap commented Aug 21, 2026

Copy link
Copy Markdown

On 2026-08-20 at APS 2-BM, /local1 on the detector host filled during a
1501-projection helical scan. The HDF5 file plugin reported a write error,
tomoscan correctly started to abort the scan, and then the IOC stayed wedged
for two and a half hours.

Mechanism. fly_scan() calls self.end_scan() from a finally: block.
An exception raised inside end_scan() therefore propagates out of
fly_scan() unhandled and kills the scan thread. In this incident, the
2-BM override of end_scan() does fid.create_dataset('exchange/web_camera_frame', data=frame)
with no guard, and the truncated scan file raised ValueError: Unable to synchronously create dataset (address of object past end of allocation).
That killed the thread before super().end_scan() could run, and the base
end_scan() is the only place that puts StartScan back to 0 and clears
scan_is_running. StartScan is a busy record; a client that started the
scan with ca_put_callback waits for it to return to 0, and it never did.
The operator's AbortScan did nothing either, because abort_scan() works
by making wait_camera_done() raise into the scan thread's finally, and
that thread was already dead.

This wraps the self.end_scan() call in fly_scan()'s finally block in
its own try/except, logs the traceback, and on failure runs a new private
method, _end_scan_after_failure(), that puts a distinguishable failure
literal to ScanStatus, puts StartScan to 0, and clears
scan_is_running. Each PV write is wrapped separately so a failing put
cannot stop the others.

Related. #181 already covers the general problem that ScanStatus
reads 'Scan complete' on every exit path from fly_scan(), including the
three existing failure handlers, so a client watching the PV cannot tell a
good scan from a bad one. This PR does not attempt that fix. It only adds
one more distinguishable value, 'Scan cleanup failed', for a path #181
does not name: an exception raised inside end_scan() itself. The literal
deliberately is not 'Scan aborted', since that string is already used
elsewhere in this file as a log message only and never reaches the PV.

Scope. Only tomoscan/tomoscan.py, only the finally block in
fly_scan() plus the new _end_scan_after_failure() private method. No
other method is touched, and the three existing except handlers above the
finally block are unchanged. This does not address the abort, timeout, or
overwrite-refused cases in #181; those still write 'Scan complete'
because end_scan() runs there without raising.

Testing. This repo has no test coverage of end_scan() or fly_scan(),
and building an EPICS harness for it is out of scope for this change. To
reproduce: make end_scan() raise (either directly, or by reproducing the
disk-full scenario in tomoscan_2bm.py) and confirm StartScan still
reaches 0 instead of staying at 1.

An exception raised inside end_scan() propagates out of fly_scan()'s
finally block unhandled, which kills the scan thread before StartScan
is put back to 0 and scan_is_running is cleared. A client that started
the scan with ca_put_callback and is waiting on StartScan then waits
forever, and abort_scan() cannot rescue it, since abort_scan() works
by raising into the scan thread, which is already dead.

end_scan() is not currently guarded against this anywhere in its call
chain: a derived class's override can raise for reasons that have
nothing to do with the base class's own cleanup (a full disk truncating
the scan file, for one). This wraps the call, logs the traceback, and
runs a last-resort teardown that puts a distinguishable failure literal
to ScanStatus, puts StartScan to 0, and clears scan_is_running, each PV
write guarded on its own so one failing put cannot block the rest.

The ScanStatus literal ('Scan cleanup failed') is deliberately not
'Scan complete', which end_scan() writes unconditionally on every exit
path today (see issue tomography#181), and deliberately not 'Scan aborted',
which is already a log-only message elsewhere in this file. It only
distinguishes cleanup failure from a normal successful ending; it does
not address the abort/timeout/overwrite cases tomography#181 also names.

Untested locally: this repo has no test coverage of end_scan() or
fly_scan(), and building an EPICS harness for it is out of scope here.
To reproduce, make end_scan() raise (or run the scenario that raised
in tomoscan_2bm.py) and confirm StartScan still reaches 0.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.

1 participant