v0.1.2: a leaked USB claim and an unreadable chunk count by juancobo · Pull Request #2 · UCSB-AMPLab/pychdk

juancobo · 2026-09-13T18:01:42Z

Why

Integrating v0.1.1 into the Digitization Toolkit's CHDK camera backend put the library under review as a dependency rather than as a thing on its own, and that turned up two faults neither the release nor its own tests could have found.

What was wrong

A failed connection left a claim on the USB port with nothing to release it. ChdkDevice opened the transport and then the PTP session with no rollback, so a session that raised left the interface claimed and nothing tracked to close. A host retrying enumeration accumulated claims on a port until someone unplugged the camera. The same sequence was written out a second time in reconnect, where it is worse, because the device has already been dropped from tracking.

Fixing it took two passes. Wrapping the session open was not enough: the transport itself can claim the interface and then fail during endpoint discovery, before it considers itself open, so its own close() did nothing and the caller's rollback called a no-op. Ownership now begins when the claim succeeds, and closing releases whatever is actually held however far opening got.

The chunk count could not be recovered. remote_capture_get_data assembles a capture from chunks and returned only the bytes. That number matters because the toolkit has exactly one bench session with cameras, and a still that arrives in one chunk and a still that arrives in forty say different things about the wire. It is now readable after a capture, per device, without changing any return shape: widening shoot would have silently changed what MultiCam hands back, and that is the path a two-body rig actually shoots through.

The counter zeroes at the attempt rather than at the download, so zero means no chunk arrived. It reset at the download at first, which meant a capture refused outright still reported the previous capture's count. That is worse than having no number, because it is specific enough to be believed.

The same fault, four times

Reviewing the first fix found the same leak in reconnect. Copilot found a third instance in the context manager, where __enter__ opens and a failure there means Python never runs __exit__. A sweep for a fourth found it in MultiCam: building two cameras in a loop, a second that fails to open leaves the first claimed by a process that no longer holds a handle on it, curable only by unplugging. That is the two-camera case, which is the only way this rig is used.

Three call sites failing the same way independently is what a convention does, so the guarantee moved into the code:

PTPDevice.open() now guarantees a failure claims nothing. Everything after the interface claim releases it before the exception propagates, so a caller cannot leak an interface by forgetting a rollback, and a fifth call site cannot reintroduce it. The rollbacks in construction, reconnect and the context manager stay as defence in depth.
MultiCam() closes the cameras it has already opened when a later one fails, instead of orphaning them. The invariant above does not cover this: the first camera opened successfully, so it is an orphan rather than a partial open.

One related asymmetry is documented rather than fixed: a session open that reaches the camera and then fails reading the response may leave a session open on the camera. It holds no resource on our side, and the comment sits where the next person will read it.

Also

A test named for a timeout never reached one: its mock reported nothing running, so it exercised a script that ended without producing data. It is renamed for what it checks, and a real deadline test sits beside it, driven by a substituted clock rather than a wall-clock wait, asserting the exception type as proof of which path ran.

What this does not prove

No camera has run any of it. The protocol claims rest on CHDK's headers, and the tests encode that reading rather than a device's behaviour. Acceptance is the bench.

Tests: 183 at v0.1.1, 204 here.

ChdkDevice opened the transport and then the session with nothing in between, so a session that raised left the interface claimed and the caller holding no object to close it. Retrying enumeration piled up claims on a port until someone unplugged the camera — and unplugging is the one repair that costs a body's position on a copy stand. Opening now gives the claim back if anything past it fails, and leaves nothing in the tracking set for _cleanup_all to find. The rollback catches BaseException rather than Exception, since a KeyboardInterrupt arriving between the claim and the session is the same leak. reconnect had the identical sequence written out a second time and the identical leak, worse for happening mid-session with the device already discarded from tracking. It calls _open now, so there is one way to claim an interface and one rollback to keep right.

remote_capture_get_data assembled chunks and returned only the bytes, so a host had no way to say whether a still came over in one piece or forty. That is one of the things the bench exists to find out, and the assembly loop is the only place that knows it. The count is left on the protocol object as last_capture_chunks and read through a property of the same name on ChdkDevice, rather than returned. shoot() already returns bytes or None depending on four of its arguments, and MultiCam.shoot promises a list of pictures, one per camera — widening either return would change what every existing caller receives to carry a number most of them will not read. Per device is also the right granularity: after a MultiCam shot each camera's figure is on its own entry in MultiCam.cameras. It resets when a capture begins and rises as chunks land, so a capture that failed half way still says how far it got, which is more use at a bench than a number that only appears on success.

Cuts the release carrying the USB claim released on a failed construction and the chunk count a streamed capture arrived in, both found reviewing the backend that pins v0.1.1.

PTPDevice.open claims the interface and then goes looking for endpoints, but only sets _is_open once everything has succeeded, and close() keyed on that flag. A camera whose endpoints could not be found therefore held a claim the transport did not believe it had: close() returned immediately, and calling it explicitly changed nothing. One claim, no release, until someone unplugged the camera. Ownership is now tracked from the moment the claim succeeds, and close() gives back whatever is actually held rather than whatever opening finished. A device that never got as far as claiming still closes to nothing, and a second close does not release twice. The device-level rollback had the matching gap: transport.open sat outside the handler, so a transport that failed this way was never closed by its caller either. It is inside now, which is what makes the rollback a release rather than a no-op. Both paths into it, construction and reconnect, go through the same code.

The count reset inside remote_capture_get_data, which only runs once a capture is ready to download. So a capture refused outright, or one that never became ready, or one that timed out, left the previous capture's number standing. After a two-chunk still, a capture where no chunk arrived at all still said two. That is the failure the count was added to prevent, made worse: a number that is simply missing gets investigated, and a number that is wrong gets believed. At a bench it would have sent someone looking for a transfer problem in a capture that never transferred anything. _shoot_streaming zeroes it on entry, before the DNG refusal, so every streamed attempt starts from nothing and zero means no chunk arrived. remote_capture_get_data keeps its own reset for a caller using it directly. Both docstrings now say that a reader in another thread sees a partial count while a capture is in flight, which matters because MultiCam shoots on a pool.

@synchronized

…d test the deadline test_a_timed_out_capture_reports_nothing never reached a deadline. Its status response said no script was running, so the second capture ended on the script-ended path after three transactions and no elapsed time, raising RuntimeError where a timeout raises TimeoutError. It proved the counter resets on a failed capture, which is worth having, but its name was evidence for a path it never took — and its comment said the camera stayed busy while its mock said the opposite. Renamed for the path it actually exercises, and the deadline now has a test of its own. The clock is driven rather than waited on: a fake stands in for the time module inside the device, walking a scripted monotonic sequence and counting sleeps without taking them, so thirty seconds of deadline cost nothing. It asserts TimeoutError specifically, which the old setup could not have raised, and that the loop went round before expiring rather than falling straight out of it. Also records what close() does about concurrent callers, read out of pyusb rather than assumed: _ResourceManager holds a threading.RLock, managed_claim_interface and managed_release_interface are both @synchronized against it, and release calls the backend only for an interface still in its claimed set, removing it in a finally. So concurrent closes serialise and a second release does nothing, and we rely on that rather than adding a lock here. The docstring cites the file and both functions so the next reader can confirm it in a minute.

Copilot

🟡 Changes recommended

Unresolved moderate USB/session cleanup findings must be addressed before approval.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

This PR hardens USB/PTP cleanup, adds per-device capture chunk counts, expands regression tests, and bumps the release to 0.1.2.

Changes:

Adds rollback for failed USB and session setup.
Tracks remote-capture chunk counts without changing return shapes.
Adds cleanup, timeout, and counting tests.
Updates package metadata to version 0.1.2.

File summaries

File	Summary
`tests/test_usb_transport.py`	Adds partial-claim cleanup coverage.
`tests/test_device.py`	Tests rollback, chunk counts, and timeout behavior.
`tests/test_chdk.py`	Tests capture chunk-count behavior.
`src/pychdk/usb_transport.py`	Tracks and releases partial USB claims. Findings: moderate (1 vote) and nit (2 votes) cleanup issues remain.
`src/pychdk/device.py`	Adds connection rollback and chunk-count access. Three moderate findings remain, each with 1 vote, covering reconnect registration, concurrency safety, and session rollback.
`src/pychdk/chdk.py`	Implements capture chunk counting.
`src/pychdk/__init__.py`	Updates the package version.
`pyproject.toml`	Updates project metadata to 0.1.2.

Review details

Suppressed comments (4)

src/pychdk/device.py:177

Because reconnect() now delegates to _open(), every successful reconnect executes this registration again. atexit retains duplicate registrations, so repeated reconnects grow the shutdown callback list and invoke _cleanup_all once per reconnect; keep the registration per device while preserving the first-registration ordering.

            # Re-register so our cleanup runs before any pyusb finalizers
            # that were registered during device creation (atexit is LIFO).
            atexit.register(_cleanup_all)

src/pychdk/device.py:486

This docstring now promises that ChdkDevice.close() is safe with concurrent callers, but it still invokes the unsynchronized PTPSession.close(). Two callers can both pass _is_open, allocate different transaction IDs, and interleave command/response I/O; the pyusb lock mentioned in PTPDevice does not protect that session state. Add a per-device lifecycle lock around session/transport operations (including reconnect), or remove the concurrency guarantee.

        """Close the connection to the camera.

        Safe to call more than once, and safe against a concurrent
        closer on the same device. PTPDevice.close says why, and where
        in pyusb to check it.

src/pychdk/device.py:184

This rollback only closes the transport. PTPSession.open() sends OPEN_SESSION before setting its _is_open flag, so a timeout or read/protocol error after the camera accepts that command leaves the remote PTP session open while this path skips session.close(). A retry can then receive SESSION_ALREADY_OPEN even though the USB claim was released; make session-open rollback track and close an in-flight session before closing the transport.

            try:
                self._transport.close()
            except Exception:
                pass

src/pychdk/usb_transport.py:222

When endpoint discovery fails, open() never reaches line 187 where _finalize_called is set, but this rollback still disposes the device without disabling PyUSB's finalizer. The surrounding code explicitly disables that finalizer to prevent shutdown-time libusb access after the context is gone, so a partially opened device can still take the unsafe finalizer path; mark the device finalized after this cleanup (and cover the partial-open case).

        try:
            usb.util.dispose_resources(self._dev)
        except usb.core.USBError:

Files reviewed: 8/8 changed files
Comments generated: 1
Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

PTPDevice.__enter__ called open() and returned self, and Python does not call __exit__ when __enter__ raises. So a transport that claimed the interface and then failed finding endpoints held it with nothing outside the block able to give it back — the same leak as a failed construction, in the one path neither that fix nor the reconnect fix reached. __enter__ closes before re-raising. close() already releases whatever is actually held however far open() got, so this is the rollback the other two callers have, in the third place that needed it. Three call sites and one fault between them says the shape is wrong rather than the callers: open() can take a claim and then raise, and each caller is separately responsible for noticing. The fix that ends it is to give open() the guarantee instead — claim released before it propagates, so that a raise means nothing is held — which would make all three rollbacks redundant rather than mandatory. Not at the end of a release, but it is the change that stops a fourth.

Releasing a claim that open() took and then lost was a convention, and three call sites independently failed to honour it — construction, reconnect, and the context manager — each found separately, each fixed separately. That is what a convention does once there is more than one caller: the next one added will not know about it either. open() now carries the guarantee itself. Everything after the claim runs inside a handler that gives the interface back before the exception propagates, so "open raised" means "nothing is held" as a property of the function that does the claiming, not as something four callers have to remember. The existing rollbacks stay: they are harmless now rather than load-bearing, and taking them out is its own decision. PTPSession.__enter__ has the same shape and deliberately no rollback, which is now written down where it sits rather than left to be rediscovered: a failed session open holds no operating-system resource, since it raises when the camera refused the session. The one asymmetry is noted there too — a command that reached the camera followed by a failed response read could leave a camera-side session we would not close.

MultiCam built its cameras in a loop with no rollback, so a second camera that would not open left the first one open and claimed while the half-built MultiCam was thrown away. Nothing was then holding it: not the caller, who never received an object, and not _open_devices, which tracks weakly and simply loses the entry when the device is collected. The interface stayed claimed until the process ended. This is the two-camera case, which is the only way the rig is used, and at a bench it presents as a camera that worked a minute ago and now cannot be opened by anything, with no cure but unplugging it. An afternoon spent hunting a hardware fault that is not there is an afternoon of the only hardware time there is. The invariant added to PTPDevice.open does not cover this: the first camera opened successfully, so it is an orphan and not a partial open. Constructing now closes what it built before re-raising, which MultiCam.close already does defensively, one camera at a time.

Copilot

🟡 Changes recommended

Unresolved USB cleanup and concurrency issues remain in partial-open and concurrent-close paths.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (5)

src/pychdk/device.py:486

The new concurrent-close guarantee is not implemented at the high-level device. Two callers can both observe the session as open and issue CLOSE_SESSION transactions concurrently on the same transport; pyusb's internal claim lock does not serialize those PTP operations. Add a device-level lock covering session/transport teardown (and reconnect), or remove the concurrency guarantee from this documentation.

        """Close the connection to the camera.

        Safe to call more than once, and safe against a concurrent
        closer on the same device. PTPDevice.close says why, and where
        in pyusb to check it.

src/pychdk/multicam.py:21

This docstring still says previously opened cameras remain claimed until process exit, but the new except block calls self.close() and releases them before re-raising. Leaving this text unchanged documents the leak as the current behavior; please describe the rollback instead.

        A camera that fails to open partway down the list leaves the
        ones before it open and claimed, and the half-built MultiCam is
        discarded, so nothing is left holding them: not the caller, who
        never got an object, and not the cleanup registry, which tracks
        devices weakly. They stay claimed until the process ends.

src/pychdk/ptp.py:222

The comment says session opening raises only when the camera refuses OPEN_SESSION, but _send_command() and _receive_response() can also fail; the following lines already acknowledge a response-read failure. Please describe the absence of a host-side resource without implying refusal is the only failure mode.

        # raises only when the camera refused OPEN_SESSION, so there is
        # no session to close and nothing claimed. The one asymmetry,
        # should it ever matter: if the command reached the camera and
        # the response read failed, the camera may hold a session we
        # would not close, because close() returns early on _is_open.

src/pychdk/usb_transport.py:239

_claimed_intf is cleared even when release_interface raises. If the backend has not actually released the interface, every later close() becomes a no-op and the exception path has no way to retry, so a failed open can still leak the USB claim—the failure mode this change is intended to eliminate. Keep ownership tracked until cleanup has succeeded, or otherwise surface/handle the cleanup failure instead of discarding the state.

        if self._claimed_intf is not None:
            try:
                usb.util.release_interface(self._dev, self._claimed_intf)
            except usb.core.USBError:
                pass
            self._claimed_intf = None

src/pychdk/usb_transport.py:236

_claimed_intf is read in the condition and then read again as the release argument. Two concurrent close() calls can interleave so one sets it to None before the other evaluates the argument, causing release_interface(self._dev, None) and an uncaught TypeError; this contradicts the concurrent-close guarantee in the docstring. Snapshot the interface value before the check (or protect the check/release/clear sequence with a lock).

        if self._claimed_intf is None and not self._is_open:
            return
        if self._claimed_intf is not None:
            try:
                usb.util.release_interface(self._dev, self._claimed_intf)

Files reviewed: 11/11 changed files
Comments generated: 1
Review effort level: Lite

Setting _dev._finalize_called was the last statement in the try, so it ran only when opening succeeded. A failed open released the claim and disposed the resources but left pyusb's finalizer armed on a device we had already touched — and the comment immediately above says what that costs: libusb_open on a freed context at interpreter shutdown, and the process goes down with it. So the guarantee added last round, that a failed open holds nothing, was buying a released interface with a possible crash. That is the worse half of the trade: a leaked claim is cured by unplugging a camera, a SIGSEGV at shutdown is cured by nobody and reproduces on whichever machine happens to be unlucky. The rollback takes ownership first and releases second. Also narrows a docstring that claimed more than was checked. ChdkDevice.close said it was safe against a concurrent closer, citing the pyusb serialisation we verified — but that covers releasing the interface, not closing the session, which this method does first by sending a close over the wire with nothing guarding two callers from both sending one. It now says which half is guaranteed and which is the host's problem.

Copilot

🔵 Needs a closer look

Resource-cleanup and protocol changes, along with the remaining documentation nits, warrant human review.

Review details

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

src/pychdk/ptp.py:222

This comment says session opening raises only when the camera refuses OPEN_SESSION, but PTPSession.open() can also raise from bulk_write, response parsing, or a failed response read. The latter case is already acknowledged below and can leave a remote session open, so the comment should describe transport/protocol failures rather than implying only a refusal path.

src/pychdk/multicam.py:21

This docstring describes the old leak as current behavior: it says earlier cameras stay claimed until process exit, but the new except block immediately calls self.close() and the class promises to leave none open. Please describe this as the previous failure mode or state that the rollback closes the already-open cameras; otherwise the public documentation is misleading when diagnosing constructor failures.

        A camera that fails to open partway down the list leaves the
        ones before it open and claimed, and the half-built MultiCam is
        discarded, so nothing is left holding them: not the caller, who
        never got an object, and not the cleanup registry, which tracks
        devices weakly. They stay claimed until the process ends.

Files reviewed: 11/11 changed files
Comments generated: 0 new
Review effort level: Lite

…ot the failure modes The comment said a failed session open "raises only when the camera refused OPEN_SESSION". It does not: open writes a command, reads a response and parses it, and any of those can raise before the refusal check is ever reached. The conclusion was right — none of them holds an operating-system resource — but the word "only" drew a boundary in the wrong place, and the next sentence of the same comment described one of the cases it had just excluded. Third instance this release of one pattern: a verified fact summarised into a broader claim, the generalisation happening in the sentence rather than in the investigation. It is the sharpest of the three because this comment exists to mark a boundary, which is the one job it then got wrong. Rewritten the way the other two should have been. It names the property — a session open that raises holds no operating-system resource, whatever raised it — and then says what is not covered, which is the camera's own state: a session it may hold that we will never close, indistinguishable on this side from one that was never opened. No claim about the set of ways the thing can fail, because that set was never what mattered.

juancobo added 6 commits September 13, 2026 09:41

chore(release): 0.1.2

01f2f88

Cuts the release carrying the USB claim released on a failed construction and the chunk count a streamed capture arrived in, both found reviewing the backend that pins v0.1.1.

Copilot AI lite review requested due to automatic review settings September 13, 2026 18:01

Copilot started reviewing on behalf of juancobo September 13, 2026 18:02 View session

Copilot AI reviewed Sep 13, 2026

View reviewed changes

Comment thread src/pychdk/usb_transport.py

juancobo added 3 commits September 13, 2026 13:09

juancobo requested a lite review from Copilot September 13, 2026 18:14

Copilot started reviewing on behalf of juancobo September 13, 2026 18:14 View session

Copilot AI reviewed Sep 13, 2026

View reviewed changes

Comment thread src/pychdk/usb_transport.py

juancobo requested a lite review from Copilot September 13, 2026 18:23

Copilot started reviewing on behalf of juancobo September 13, 2026 18:24 View session

Copilot AI reviewed Sep 13, 2026

View reviewed changes

juancobo merged commit 3f1788e into main Sep 13, 2026
2 checks passed

Navigation Menu

v0.1.2: a leaked USB claim and an unreadable chunk count - #2

v0.1.2: a leaked USB claim and an unreadable chunk count#2
juancobo merged 11 commits into
mainfrom
release/v0.1.2

juancobo commented Sep 13, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

juancobo commented Sep 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why

What was wrong

The same fault, four times

Also

What this does not prove

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (4)

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (5)

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Suppressed comments (2)

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

juancobo commented Sep 13, 2026 •

edited

Loading