Skip to content

fix(android): harden camera capture hang fix (post-#256 review findings)#257

Merged
tstapler merged 2 commits into
mainfrom
backlog/stelekit-fix-camera-capture-hang
Jul 24, 2026
Merged

fix(android): harden camera capture hang fix (post-#256 review findings)#257
tstapler merged 2 commits into
mainfrom
backlog/stelekit-fix-camera-capture-hang

Conversation

@tstapler

@tstapler tstapler commented Jul 24, 2026

Copy link
Copy Markdown
Owner

Summary

PR #256 (merged, shipped in v0.71.4) already fixed the core reported bug — tapping capture hanging forever. This PR is a follow-up hardening pass: a 4-agent parallel code review of #256's changes surfaced two real correctness gaps and a security/UX regression that #256 didn't cover. This PR is rebased directly on main and contains only the incremental delta on top of #256 — it does not re-implement anything already shipped.

What Changed

  • App.kt's CapturePreviewDialog.onSave had no try/catch/finally around its scope.launch body. An uncaught Throwable from imageImportService.import() (or anything downstream of it) would either crash the Android process (uncaught Throwable on a plain rememberCoroutineScope() with no CoroutineExceptionHandler — see this repo's own documented failure mode) or, short of a crash, skip the isCaptureImporting reset — reintroducing the exact "stuck forever" bug fix(android): prevent camera capture dialog from hanging forever #256 fixed, one step downstream. Now wrapped in try/catch(Throwable)/finally.
  • AndroidCameraProvider.capturePhoto()'s withTimeout(10_000L) didn't actually cover ProcessCameraProvider.getInstance() or bindToLifecycle() — both sat before the timeout block, so a wedged provider/bind could still hang this capture path indefinitely. Widened the timeout to bound the whole pipeline, matching the original bug's acceptance criterion that both duplicated capture paths never hang.
  • CameraViewfinderDialog.android.kt passed raw Throwable.message (which can include OutOfMemoryError diagnostic strings, since fix(android): prevent camera capture dialog from hanging forever #256 broadened the catch-all to Throwable) straight to the user-facing snackbar at four call sites, unlike every other error path in the codebase which routes through DomainError.toUiMessage(). Now logs the raw detail and surfaces a generic message. Also wrapped the click handler's result.fold in try/catch(Throwable) so a throw from the caller-supplied onCapture/onError/onDismiss callbacks can't crash the process.
  • Added an explicit code comment at both withTimeout(10_000L) sites documenting a known residual risk: Kotlin cancellation is cooperative and cannot preempt a truly HAL-wedged takePicture() or a synchronous BitmapFactory decode mid-call.
  • Added a regression test (ExifOrientationFixerTest) locking in the EXIF-fix dispatcher hop, with an honest KDoc about what it does and doesn't cover.

Test plan

  • ./gradlew :kmp:compileDebugKotlinAndroid / :kmp:compileTestKotlinJvm — both clean
  • ./gradlew :kmp:testDebugUnitTest --tests dev.stapler.stelekit.platform.sensor.ExifOrientationFixerTest — 12/12 passed
  • ./gradlew :kmp:testDebugUnitTest --tests dev.stapler.stelekit.platform.sensor.AndroidCameraFrameSourceTest — 3/3 passed (no regression to the concurrent QR-scan camera session)
  • ./gradlew :kmp:jvmTest --tests dev.stapler.stelekit.ui.CaptureAndImportTest — 10/10 passed
  • ./gradlew :kmp:jvmTest --tests dev.stapler.stelekit.platform.sensor.MotionSensorProviderTest — 10/10 passed
  • ./gradlew :kmp:detekt — no violations
  • Manual on-device verification — not performed in this sandbox; no physical device/emulator camera available

Known residual gaps (not addressed here, called out for visibility)

  • No automated end-to-end test exercises the real capturePhoto()/takePhotoAndProcess() call sites (CameraX is not cheaply fakeable under Robolectric) — documented as a known residual risk in this task's validation plan.
  • AndroidCameraProvider.capturePhoto() still has no reachable production caller (pre-existing, tracked as a follow-up pipeline-unification ticket).

Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com
https://claude.ai/code/session_01VrCZ7tzBPrWhhj7oJXCXKX

tstapler and others added 2 commits July 24, 2026 15:44
Both duplicated capture paths (CameraViewfinderDialog.android.kt's
takePhotoAndProcess and AndroidCameraProvider.capturePhoto()) already
wrap ExifOrientationFixer.fixOrientation in withContext(PlatformDispatcher.IO)
inside the widened withTimeout(10_000L) — verified by re-reading both files,
this was already fixed by a prior commit on this branch. What was missing was
a regression test proving the dispatcher hop, mirroring the existing
snapshotSensorData hang test. Also re-ran AndroidCameraFrameSourceTest (QR
scan) and CaptureAndImportTest (capture-and-import + withImportTimeout) to
confirm no regression — both still green.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VrCZ7tzBPrWhhj7oJXCXKX
Four-agent parallel review of PR #256 surfaced two real correctness
issues, one incomplete fix, and a test-quality gap; addressed all four:

- App.kt's CapturePreviewDialog.onSave scope.launch had no try/catch or
  finally: an uncaught Throwable from imageImportService.import() (or
  anything downstream of it) would kill the Android process, and even
  short of a crash would skip the isCaptureImporting reset — reintroducing
  the exact "stuck forever" bug this PR fixes, one step downstream of the
  capture dialog. Now wrapped in try/catch(Throwable)/finally.
- AndroidCameraProvider.capturePhoto()'s withTimeout(10_000L) didn't
  actually cover ProcessCameraProvider.getInstance() or bindToLifecycle()
  — both sat before the timeout block, so a wedged provider/bind could
  still hang this capture path indefinitely, the acceptance criterion's
  explicit "both duplicated capture code paths" requirement. Widened the
  timeout to bound the whole pipeline.
- CameraViewfinderDialog.android.kt passed raw Throwable.message (which
  can now include OOM diagnostic strings, since the outer catch was
  broadened to Throwable) straight to the user-facing snackbar at four
  call sites, unlike every other error path in the codebase which routes
  through DomainError.toUiMessage(). Now logs the raw detail via Logger
  and surfaces a generic message. Also wrapped the click handler's
  result.fold in try/catch(Throwable) so a throw from the caller-supplied
  onCapture/onError/onDismiss callbacks can't crash the process.
- Corrected the new dispatcher-hop test's KDoc, which overclaimed that it
  would catch a regression in the production call sites — it validates
  the withContext(IO) pattern in isolation, not the wiring.
- Added an explicit code comment at both withTimeout(10_000L) sites
  documenting the known residual risk: Kotlin cancellation is cooperative
  and cannot preempt a truly HAL-wedged takePicture() or a synchronous
  BitmapFactory decode mid-call.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VrCZ7tzBPrWhhj7oJXCXKX
@tstapler
tstapler force-pushed the backlog/stelekit-fix-camera-capture-hang branch from 6f27d7a to 3e18def Compare July 24, 2026 22:46
@tstapler tstapler changed the title fix(android): camera capture dialog no longer hangs forever on picture capture fix(android): harden camera capture hang fix (post-#256 review findings) Jul 24, 2026
@tstapler
tstapler marked this pull request as ready for review July 24, 2026 22:47
@github-actions

Copy link
Copy Markdown
Contributor

JVM Load Benchmark (Desktop)

Synthetic in-memory benchmark measuring load performance for the desktop (JVM) app.
Comparing ba44a6e9 (this PR) vs 3e560563 (baseline)
Graph config: xlarge — 230 pages

Metric This PR Baseline Delta
Phase 1 TTI ↓ 0ms 0ms 0 (0%)
Phase 2 background ↓ 1ms 1ms 0 (0%)
Phase 3 index ↓ 1ms 1ms 0 (0%)
Total ↓ 2ms 2ms 0 (0%)
Write p95 (baseline) ↓ 19ms 17ms +2ms (+12%) ⚠️
Write p95 (under load) ↓ n/a n/a
Jank factor ↓ n/a n/a
↓ lower is better
Flamegraphs (this PR) **Allocation** — object allocation pressure (JDBC/SQLite churn)

Alloc flamegraph not available

CPU — method-level hotspots by on-CPU time

CPU flamegraph not available

Top allocation hotspots (this PR) `37.2%` byte[]_[k] `7.2%` java.lang.String_[k] `6.6%` int[]_[k] `6.3%` java.util.LinkedHashMap$Entry_[k] `3.9%` java.lang.Object[]_[k]
Top CPU hotspots (this PR) `97.6%` /usr/lib/x86_64-linux-gnu/libc.so.6 `0.9%` /tmp/sqlite-3.51.3.0-fcfafaeb-3f19-4187-a745-0237b15f3f22-libsqlitejdbc.so `0.3%` __libc_pwrite `0.1%` fsync `0.1%` SR_handler

@github-actions

Copy link
Copy Markdown
Contributor

Android Load Benchmark

Instrumented benchmark on an API 30 x86_64 emulator — 500-page synthetic graph.

Comparing ba44a6e9 (this PR) vs a6b9e512 (baseline)
Device: API 30 x86_64 emulator — 530 pages loaded

Graph Load

Metric This PR Baseline Delta
Phase 1 TTI ↓ 35ms 32ms +3ms (+9%) ⚠️
Phase 3 index ↓ 4157ms 3672ms +485ms (+13%) ⚠️

Interactive Write Latency (during Phase 3)

Metric This PR Baseline Delta
Write p95 (baseline) ↓ 8ms 7ms +1ms (+14%) ⚠️
Write p95 (during phase 3) ↓ 13ms 17ms -4ms (-24%) ✅
Jank factor ↓ 1.63x 2.43x -0.8x (-33%) ✅
Concurrent writes ↑ 20 18 +2ms (+11%) ✅

SAF I/O Overhead (ContentProvider vs direct File read)

Measures Binder IPC cost added by ContentResolver per readFile() call.
Real SAF via ExternalStorageProvider will be higher on device; this is a lower bound.

Metric This PR Baseline Delta
Direct read / file ↓ 0.0ms 0.0ms 0 (0%)
Provider read / file ↓ 0.2ms 0.2ms +0ms (+18%) ⚠️
IPC overhead ratio ↓ 6x 5x +1x (+20%) ⚠️
↓ lower is better · ↑ higher is better

@tstapler
tstapler merged commit 6369b87 into main Jul 24, 2026
33 checks passed
tstapler pushed a commit that referenced this pull request Jul 25, 2026
🤖 I have created a release *beep* *boop*
---


##
[0.71.5](v0.71.4...v0.71.5)
(2026-07-24)


### Bug Fixes

* **android:** harden camera capture hang fix
(post-[#256](#256) review
findings) ([#257](#257))
([6369b87](6369b87))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.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