Fix shutdown abort, transfer error reporting, log query params, and transferProgress payload by jordlee · Pull Request #54 · crsdk/alpha-sdk-api

jordlee · 2026-08-13T20:34:01Z

Five server-side fixes found while running a full transfer/shutdown sweep against real hardware (ILCE-7M5 CFCA6014E092, USB, SDK V2.02.00, server 3.0.0, darwin-arm64), plus the docs and codegen they imply. One commit per issue.

Commit	Issue
`6010751`	#42 — shutdown SIGABRT (exit 134)
`9152cd8`	#46 — `lines`/`level` never parsed
`3d96992`	#49 — `0x8D03` reported as a hard `400`
`1cd65d9`	#47 — `transferProgress` missing documented fields
`d780362`	regenerate `schema.d.ts` (codegen drift from the spec edit)
`18ab402`	docs for the three behaviour changes

Note: this PR originally also carried a fix for #33 (Linux OpenCV bundling). That commit has been dropped in favour of #51, which does the same thing and is verified on actual Linux hardware rather than by simulation. #53 covers the RUNPATH half properly in CMake, which is a better fix than the patchelf approach I had. No overlap remains — this PR touches no file that #51 or #53 touch. One refinement suggestion left as a comment on #51.

Two findings that go beyond the issues as filed

0x8D03 is not a latch (#49, and it re-frames #40). It means "a transfer is already in progress", and clears the moment that transfer completes. Reproduced deterministically: start a 67 MB transfer, issue a second request 0.3 s later → 0x8D03; wait for the first to finish, retry → 202. On the Pi in #40 this looked like "one stuck download latches the whole subsystem", but every later request was correctly reporting "busy" because the stuck transfer never finished. There is no separate latching bug — and the 400 "Failed to start file download" wording is what produced the wrong causal model.

#46 was not just an unimplemented handler. parseRequest() stripped the query string off the path and discarded it, so the handler's comment — "could be parsed from request.path" — described something impossible. Query strings are now retained on HttpRequest; routing still matches the bare path and was re-verified across 11 endpoints with and without a query.

Compatibility

transferProgress keeps filename alongside the new savedPath, because mcp/src/tools/files.ts reads it. The MCP client was exercised end to end (connect → list_captures → download_capture → poll_events → get_last_capture) against this build and still resolves saved paths; the downloaded file was byte-exact against the card listing. Other additions are additive fields (retryable, returned).

Regression sweep

Routing across 11 endpoints with and without query strings, plus edge cases (?, ?lines=, ?=novalue, duplicate keys, &&); all three transfer types still 202; 0x8D02 still 400 with its original message; 4/4 SSE events spec-complete; 75 properties returned; live view enable → start → frame returning a 1024×680 JPEG. No regressions.

Three apparent failures during the sweep were investigated and are not caused by these changes: af-area-position returning 400 (live view not streaming — correct), live-view/frame returning 404 (test skipped start), and unknown-camera properties/all returning 200 instead of 400 — the separate, already-filed item in #47.

What is not verified

POST /api/server/shutdown aborts with SIGABRT (exit 134) — joinable std::thread destroyed #42's race is not proven eliminated. Seven clean shutdowns across four paths (SIGINT, SIGTERM, triple rapid SIGINT, SIGINT with a camera connected, HTTP ×2), but a thread race cannot be proven absent by repetition, and no before/after baseline of the exit-134 crash was captured on this machine.
POST /api/server/shutdown aborts with SIGABRT (exit 134) — joinable std::thread destroyed #42 changes signal handling: the handler now records the signal and main() performs the teardown, adding ≤200 ms to shutdown latency (measured 0.24–0.33 s end to end). This was not optional — a mutex in stop() reached from a signal handler risks deadlocking Ctrl-C — but it is a behavioural change worth attention.
Spec/behaviour mismatches found in full 47-endpoint hardware sweep #47 is only partly closed. contentId/fileId answer "which file", not "which artifact": a full file, thumbnail and screennail of the same content emit an identical identity, so a client requesting more than one cannot tell the events apart. The spec has no field for this, so this PR matched the spec rather than exceeding it. Detail in Spec/behaviour mismatches found in full 47-endpoint hardware sweep #47 and Transfer requests are neither queued nor deduplicated — N identical downloads write N copies #50.

🤖 Generated with Claude Code

stop() opened with `if (!m_running) return;` above the join, so the second caller — ~CameraWebServer(), after the detached POST /api/server/shutdown thread had already cleared the flag — returned early and skipped it. m_serverThread was then destroyed while still joinable: std::terminate, SIGABRT, exit 134 after an otherwise complete teardown. - Serialize stop() and stopLiveViewBroadcasting() on a new m_shutdownMutex and always join. A bare exchange() would still have left both callers able to join the same std::thread concurrently, which is itself undefined. - stopLiveViewBroadcasting() had the identical early-return-above-join shape and is fixed the same way. - The signal handler no longer calls stop(). It performed stream I/O, thread joins and mutex acquisition inside a signal context — already unsafe, and with the new mutex it could deadlock and wedge Ctrl-C. It now records the signal and main() performs the teardown. Verified on hardware (macOS, ILCE-7M5): SIGINT, SIGTERM, triple rapid SIGINT, SIGINT with a camera connected, and two HTTP shutdowns — all exit 0, exactly one teardown each, with ReleaseDevice() returning cleanly. Refs #42 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

…ver/logs The handler hardcoded maxLines = 100 and minLevel = "info", with a comment saying the values "could be parsed from request.path". They could not: parseRequest() stripped the query string off the path and discarded it, so no handler could ever observe a query parameter. - Retain the query string on HttpRequest (routing still matches the bare path) and add HttpRequest::queryParam(). - Parse ?lines= (clamped to the retained buffer; non-numeric falls back to the default) and ?level= (unrecognised falls back to info). - Report `returned` alongside `total`, so a filtered response is not mistaken for data loss. Verified: ?lines=5 returns 5, ?level=warn returns warn entries only, ?lines=3&level=warn combines both, and ?lines=notanumber / ?level=bogus fall back gracefully. Routing was re-checked across 11 endpoints with and without query strings. Refs #46 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

CrError_RemoteTransfer_GetContentsDataDisable does not mean the requested transfer failed — it means one is already in flight, and it clears as soon as that transfer completes. Reporting it as 400 "Failed to start file download" is what led #40 to conclude that a single stuck transfer permanently "latches" the RemoteTransfer subsystem; in fact every later request was correctly reporting "busy" because the stuck one never finished. - Add classifyTransferError(), shared by the full-file and thumbnail/screennail paths so the two cannot drift. - Map 0x8D03 to 409 with a message naming the state and the remedy, plus retryable: true. 0x8D02 keeps its existing 400 and message. - Log the outcome at warn/error. These paths previously logged nothing beyond the request line, which is a large part of why #40 was hard to diagnose. Verified: a second request issued 0.3s into a 67MB transfer returns 409, and the identical request returns 202 once that transfer completes. Refs #49, refs #40 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

The event carried only {percent, notify, filename} — none of the cameraId, contentId, fileId or savedPath the spec documents — so a client written against the spec could not tell which transfer had completed. - Track the in-flight transfer's content and file id, and emit them along with cameraId and savedPath, from both the SDK callback and the polling fallback. - Hold that identity in dedicated members rather than reading it from m_pendingTransfers. Once a real callback has been seen, transferPollLoop() clears that list on every tick, so it is already empty by the time the callback for any transfer after the first arrives; reading it there reported contentId 0 for every transfer but the first. - Keep `filename` alongside the new `savedPath`. mcp/src/tools/files.ts reads it, and removing it would break the MCP client. - Correct the event catalog: drop the stale claim that the macOS SDK callback is unreliable — it is not on V2.02, where no synthetic events were observed — and document notify, filename and synthetic. Verified: three sequential transfers each emitted the correct distinct contentId with every documented field present, and the MCP client still resolves the saved path end to end. Refs #47 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

api/openapi.yaml is the source of truth for mcp/src/api, so the transferProgress description change in the previous commit left schema.d.ts stale and would have failed the codegen-drift gate. Comment-only: one line, no type or tool changes. 71/71 MCP tests pass. Refs #47 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

…nd the log query params Brings the docs in line with the behaviour changes in this PR (and satisfies the spec-docs sync gate, which api/openapi.yaml changes trip). - events: transferProgress now shows cameraId / contentId / fileId / savedPath, notes that filename is retained for existing consumers, and explains the synthetic flag rather than repeating the stale claim that the macOS callback is unreliable. - sd-card: document that the SDK permits one transfer at a time and that a concurrent request returns a retryable 409, distinct from the 0x8D02 400. - server: document the lines and level params properly, including clamping and fallback behaviour, and the new `returned` field. Refs #46, refs #47, refs #49 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

jordlee and others added 6 commits August 13, 2026 13:51

jordlee force-pushed the fix/shutdown-transfer-errors-log-params-opencv branch from 0ee2cd9 to 18ab402 Compare August 13, 2026 20:51

jordlee changed the title ~~Fix shutdown abort, transfer error reporting, log query params, transferProgress payload, and Linux OpenCV bundling~~ Fix shutdown abort, transfer error reporting, log query params, and transferProgress payload Aug 13, 2026

This was referenced Aug 13, 2026

Fix Linux OpenCV bundling in .mcpb and guard against silent reship #51

Merged

Linux: OpenCV never bundled into .mcpb — opencvLibDir() paths cannot match what extract.ts writes #33

Closed

jordlee merged commit 680a18c into main Aug 13, 2026
9 checks passed

jordlee mentioned this pull request Aug 13, 2026

/api/server/logs: documented 'lines' and 'level' query params are never parsed #46

Closed

Navigation Menu

Uh oh!

Fix shutdown abort, transfer error reporting, log query params, and transferProgress payload - #54

Fix shutdown abort, transfer error reporting, log query params, and transferProgress payload#54
jordlee merged 6 commits into
mainfrom
fix/shutdown-transfer-errors-log-params-opencv

jordlee commented Aug 13, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

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

Uh oh!

Two findings that go beyond the issues as filed

Compatibility

Regression sweep

What is not verified

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

jordlee commented Aug 13, 2026 •

edited

Loading