fix(audio-studio/android): reclaim recorders on failed init, emit the error event - #464
Merged
Merged
Conversation
added 2 commits
August 21, 2026 17:21
… error event Two Android defects, both of which present to a caller as silence. #446 — recorder leak on failed initialization: - initializeRecordingResources()'s two catch blocks released only the wake lock. Both callers allocate the AudioRecord (and the compressed MediaRecorder) before reaching it, and neither has a catch of its own — each initializer returns false and the caller returns early. A failure there stranded both native recorders until some later attempt happened to call discardFailedAttempt(); if none came, permanently. Both catches now reclaim. - cleanup() released the compressed recorder only inside `if (_isRecording.get())`, which a failed preparation never sets, and never nulled it — so destroy() could not reclaim it either. release() is now unconditional; stop() stays gated, since it throws on a recorder that was prepared but never started. - initializeCompressedRecorder() constructs the MediaRecorder and then configures it, so a throw from any setter or from prepare() stranded it. Now released on that path. This one was not in the issue; found while fixing it. The stop path nulls compressedRecorder at line 1347, so cleanup()'s new unconditional release sees null there — no double release. #447 — addRecordingErrorListener never fired on Android: iOS declares an `error` event and emits from 9 sites. Android's Events(...) block had no equivalent, so the listener was typed cross-platform and inert there — silence reading as "healthy", which is the opposite of the signal a caller subscribes for. Declared RECORDING_ERROR_EVENT (wire name "error", identical to iOS) and emitted from the four live-recording failures in recordingProcess(): AudioRecord leaving STATE_INITIALIZED, read() returning an error code, the primary WAV failing to flush, and the loop dying. The read-failure path does not break the loop, so an unguarded emit would fire once per buffer for as long as the fault lasts. A @volatile latch reports one event per episode and re-arms once audio flows again, so a fault that resolves and recurs is still reported. The latch is cleared when a recording starts, or a degraded recording would suppress the next one's first error. Docs updated: the iOS-only caveat is replaced with what each platform actually reports, keeping the warning that silence is not evidence of health on either. Verified: typecheck clean, Android unit tests pass (BUILD SUCCESSFUL). Not yet exercised on device — the failure paths need induced faults.
…rs per kind Review found four more defects, all confirmed in source. Three are leaks the first pass missed, and one of them was hidden behind a comment I wrote that was simply wrong. An AudioRecord that constructs but reports STATE_UNINITIALIZED was abandoned without release or null. That is a direct failed-initialization leak — the exact shape of #446 — and the pre-attempt discardFailedAttempt() does not cover it, since that only reclaims an *earlier* attempt's resources. initializeCompressedRecorder() released only its own MediaRecorder, leaving the AudioRecord this attempt had already opened. My comment claimed the caller rolls that back. It does not: both call sites just `return` when this returns false, with no cleanup of their own. Now calls discardFailedAttempt(), which reclaims and nulls both, so the double-release the comment worried about cannot happen. cleanup()'s `audioRecord?.stop()` was outside any catch, so a throwing stop() jumped to the outer catch and skipped every release below it — abandoning both recorders on precisely the hardware failure that makes stop() throw. Each stop() is now individually guarded. The single error latch suppressed unrelated failures: a persistent read fault, cleared only by a later positive read, would swallow a WAV flush failure or a loop exception that happened afterwards — the opposite of what a caller subscribes for. Latched per failure kind now. Two related fixes fall out of the same change: a failed emit un-latches, so a delivery failure no longer silences that kind for the rest of the recording; and the latch is a synchronized EnumSet rather than a @volatile boolean, since @volatile gives visibility but not atomicity and cleanup() does not join the recording thread before a later recording resets it. `add()` returning false makes the check-and-set one step. Docs updated to say one event per kind per recording rather than one per episode. Typecheck clean, Android unit tests pass, :siteed-audio-studio:compileDebugKotlin BUILD SUCCESSFUL. Still not device-validated, and the reviewer's point stands: these paths need an injectable AudioRecord/MediaRecorder factory to be tested deterministically, and device validation would not have caught the caller-return leaks above anyway. That seam is worth its own change rather than being bolted on here.
Collaborator
Author
Device validation (Pixel 6a, fresh install)Built and installed from this branch — The normal path is not regressed. This is the real risk in this change, since Native log shows a clean teardown — Failed attempt does not poison the next one: What this does NOT prove, stated plainlyBoth failure injections I tried were rejected during config parsing, before any recorder is allocated:
So the leak paths this PR fixes — The same goes for the four This is the reviewer's point, and I agree with it: these paths need an injectable Compile and unit coverage: |
…der, make delivery observable Round-2 review found my previous fix introduced a regression in the normal path, which is worse than the leak it was closing. stopRecording() clears _isRecording and calls cleanup() *before* its own finalization block — the block that stops and releases the compressed recorder and writes out the file. My unconditional release in cleanup() therefore took the recorder away from it and nulled it, so normal AAC/M4A/Opus output could be truncated or invalid. cleanup() now reclaims only when nobody else will: `isPrepared && !_isRecording`. That is exactly #446's case — a preparation that never started has no finalization coming — and it cannot catch an active stop, because stopRecording() clears isPrepared before calling cleanup(). The delivery-failure un-latching did not work either. EventSender.sendExpoEvent returned Unit, and safeSend catches the exception internally and reports failure through a return value that sendExpoEvent discarded — so emitRecordingError's catch could never fire, and a failed send left that kind latched for the rest of the recording, suppressing every later occurrence. The interface now returns Boolean: it still never throws, so "best-effort" is intact, but a caller that de-duplicates its own events can tell a failed send from a suppressed one. Four instrumented-test doubles updated to match. Also corrected the AudioSourceWiringTest comment, which still described the _isRecording gate this change replaced. Typecheck clean, :siteed-audio-studio:compileDebugKotlin, compileDebugAndroidTestKotlin and testDebugUnitTest all BUILD SUCCESSFUL. Compressed-output regression not yet re-validated on device; doing that next.
Collaborator
Author
Compressed-output regression: fixed and verified on deviceThe round-2 P1 was real and serious — my previous fix broke the normal path.
Verified on a Pixel 6a, fresh install ( A reported size is not proof the file is valid, so I pulled the m4a off the device and decoded it: Real AAC, and 3.99s against a 4s recording — complete, not truncated. The other round-2 findingDelivery-failure un-latching genuinely did not work:
Also fixed the Typecheck clean; The #446 leak paths themselves remain unreachable from a device — that limitation is unchanged and still wants an injectable recorder factory. |
…at finalizes Round-3 review found my gate was too narrow, which traded one leak for another. `isPrepared && !_isRecording` was an attempt to describe "nobody else will reclaim this", but it only matched a prepared-but-never-started recording. Every other active teardown — destroy() from OnDestroy, a compressed start() failure, a startRecordingProcess() failure — reaches cleanup() with no finalization coming, and the gate silently skipped all of them: stopped, never released, never nulled. The real condition is not a state combination, it is who the caller is. stopRecording() is the only path that finalizes the compressed recorder itself, so it now says so with an explicit flag around its cleanup() call, cleared in a finally so an exception cannot strand it and disable reclamation for everyone after. Every other caller gets the recorder reclaimed. Also split the stop path's shared try: `stop()` and `release()` were in one block, so a throwing stop() skipped release() and the next line cleared the only reference — leaking on exactly the failure that makes stop() throw. Not addressed here, and worth their own issue: the reviewer also found that handleDeviceChange can restart a dead recording when a concurrent stop completes during its unlocked sleep, and that prepareRecording (an IO coroutine that sets isPrepared only at the end) is not serialized against startRecording, so concurrent calls can release each other's recorders. Both are pre-existing races in paths this PR does not touch, and both want the injectable-factory seam rather than another point fix. One caveat on the Boolean contract change: EventSender.sendExpoEvent went from void to boolean, which is a JVM ABI break for any precompiled third-party implementor. Nothing in-repo breaks. Device-verified on a Pixel 6a, fresh install (lastUpdateTime 19:41:52, checked 8s later): compressed AAC still produces a valid file — 67914 bytes, afinfo reports 44100 Hz AAC, estimated duration 3.992381 sec against a 4s recording. The wider gate did not reintroduce the truncation. Typecheck clean, compileDebugKotlin and testDebugUnitTest BUILD SUCCESSFUL.
added 21 commits
August 21, 2026 20:09
…pare paused recordings Round-4 review found three, including one I had flagged myself and then shipped anyway. The EventSender ABI break was real. Changing `sendExpoEvent` from `(...Bundle)V` to `(...Bundle)Z` on a published interface breaks any precompiled external implementor with AbstractMethodError. I noted that risk in the last commit message and left it — noting a break is not permission for it. The original signature is restored, and the delivery result comes from a new `sendExpoEventChecked` with a default implementation, so implementing it is optional. The four instrumented-test doubles are reverted to the original signature and still compile, which is the compatibility claim demonstrated rather than asserted. Un-latching on every failed delivery could storm. The read-failure path does not sleep on a negative result, so a persistent read fault plus an unavailable emitter retried delivery every loop iteration — events and logs at buffer rate. A failed send still is not a report, so the un-latch stays, but it is now bounded to three attempts per kind, then gives up with a log line. Counters reset with the latches at the start of each recording. initializeRecordingResources' rollback was unconditional, and startRecording is allowed while paused. In that state initializeAudioRecord keeps the existing AudioRecord rather than building a new one, so a resource-init failure released and nulled a recorder belonging to a live recording while _isRecording stayed true, leaving resume and stop to run against broken state. Rollback now skips when a recording is active, and says why in the log. Device-verified after the change, fresh install (lastUpdateTime 20:08:44, checked 10s later): compressed AAC still produces a valid file — 67942 bytes, afinfo reports 44100 Hz AAC and 3.992381 sec against a 4s recording, app alive throughout. Typecheck clean; compileDebugKotlin, compileDebugAndroidTestKotlin and testDebugUnitTest all BUILD SUCCESSFUL.
…ting a paused recording
Round-5 review found my ABI fix was not one.
Kotlin compiles an interface method with a body as ACC_ABSTRACT plus a
DefaultImpls class, so `sendExpoEventChecked` having a default did not make it
safe: javap shows `public abstract boolean sendExpoEventChecked`, and a
precompiled implementor would throw AbstractMethodError — which
`catch (Exception)` does not catch. My test doubles compiled fine because
recompilation generates the bridge, which is exactly why their passing masked it.
EventSender is now byte-identical to main:
public interface net.siteed.audiostudio.EventSender {
public abstract void sendExpoEvent(java.lang.String, android.os.Bundle);
}
The delivery result moved to a plain method on AudioStudioModule, reached through
a type check at the call site. Any other implementor takes the best-effort path.
The paused-start ownership hole is closed at its source. startRecording()
explicitly permitted `_isRecording && isPaused`, so it re-ran initialization
against recorders the paused recording still owned: compressed init overwrote the
retained recorder and leaked the old one, and either failure path then released
the live AudioRecord while _isRecording stayed true. Point-fixing each rollback
was chasing symptoms — resumeRecording() is the API for a paused recording, and
nothing in the JS layer starts one that way. It now rejects with a message saying
which call to use instead. The rollback guard stays as belt-and-braces, since the
failure it prevents is silent.
Device-verified, fresh install (lastUpdateTime 20:25:33, checked 8s later):
start -> pause -> startRecording() -> "Recording is paused; call
resumeRecording() instead"
resume -> stop -> size 259344, dur 2939
So the path is rejected and pause/resume/stop still works.
compileDebugKotlin, compileDebugAndroidTestKotlin and testDebugUnitTest all
BUILD SUCCESSFUL.
… not just its flag The PCM thread is started by startRecordingProcess, before compressedRecorder.start(). A compressed start failure therefore lands in cleanup() with that thread still running, and clearing _isRecording alone does not stop it: an immediate retry sets the flag true again, and the survivor resumes, reads the newly created AudioRecord, and writes through its own stale FileOutputStream. cleanup() now interrupts and joins it, with the same 2s budget stopRecording uses. Two things this needed care with, both found while writing it rather than after: The join must happen BEFORE taking audioRecordLock. The recording loop acquires that same lock on every read, so joining while holding it would deadlock until the timeout expired, on every teardown. Clearing _isRecording early to let the thread exit would have made the `if (_isRecording.get())` gate below always false, so an active recorder would be released without ever being stopped. The flag is captured with getAndSet and the gate reads the captured value. stopRecording joins separately, before flushing its final chunk, so by the time it calls cleanup() this is already finished — its path is unchanged. Device-verified on a Pixel 6a, fresh install (lastUpdateTime 23:32:17, checked 7s later). This touches the teardown every recording runs through, so all three paths were exercised: 3 back-to-back recordings 172912 bytes / 1959ms, all three identical compressed AAC 68323-byte m4a, afinfo: 44100 Hz AAC, 4.0156 sec pause -> resume -> stop 255816 bytes / 2899ms No "did not exit within 2s" warnings in logcat, no errors, app alive throughout. compileDebugKotlin, compileDebugAndroidTestKotlin and testDebugUnitTest all BUILD SUCCESSFUL.
…rding failure My cleanup() interrupt introduced a regression the review caught: the loop's two Thread.sleep calls surface the interrupt as InterruptedException, and the broad catch below reports that as a degraded recording — so every ordinary stop and every module destruction would have fired a false `error` event at JS. InterruptedException is now caught first and handled as what it is: teardown. The interrupt status is restored, the wake lock still released, and nothing is emitted. The two inner catches are unaffected — neither wraps a sleep. Also documented the audioRecordLock question the review raised on stopRecording's own join. It does hold the lock while joining, and the recording loop takes that same lock per read, so a stall is possible in principle. It does not happen: READ_NON_BLOCKING means the loop holds the lock for microseconds, and measured end-to-end stopRecording latency on a Pixel 6a is 62ms plain and 110/174/145ms with enableProcessing, a 100ms interval and compressed output — against a 2000ms budget. Restructuring a ~200-line synchronized block, with the join required ahead of the final data flush, is a large change against a hazard that does not reproduce; a warning now logs the elapsed time if the thread ever does outlive the join, which is what would surface it. Device-verified, fresh install (lastUpdateTime 23:43:51, checked 14s later): 3 back-to-back recordings 155272 bytes / 1759ms, all identical compressed AAC 310504 bytes wav + 59826 bytes m4a and no "Recording stopped unexpectedly" anywhere in logcat across either run, which is the false event this fixes. compileDebugKotlin, compileDebugAndroidTestKotlin and testDebugUnitTest all BUILD SUCCESSFUL.
…ble join Round-8 review found four. Three fixed, one deferred with a reason. Map.merge is API 24+ and this module declares minSdk 21 with no core-library desugaring, so a failed event delivery would have crashed the recording thread with NoSuchMethodError on API 21-23. That is a crash I introduced two commits ago. Replaced with a CAS loop over get/putIfAbsent/replace, all API 1. Swept the rest of my diff for other API 24+ map methods — none. The join could still stack to ~4s. My previous fix put cleanup's join outside its own lock acquisition, but stopRecording holds audioRecordLock across both its own join and its call to cleanup(), so the nested join sits inside that monitor regardless. stopRecording already owns thread termination on that path, so cleanup now skips the join there entirely, keyed off the flag that already marks it. failedDeliveryAttempts never reset on success, so two earlier failures plus one later transient failure would hit the cap and latch that kind for the rest of the recording. Cleared on every successful delivery. Also corrected the changelog, which still said Android does not declare the recording error event. Deferred: cleanup() can race a concurrent startRecording, because startRecording never takes audioRecordLock. That is pre-existing — it races destroy() the same way without any of this branch's changes — and it belongs with the other concurrency races already filed as #472, which want the injectable-factory seam rather than another point fix. Device-verified, fresh install (lastUpdateTime 23:57:34, checked 12s later), three consecutive recordings with enableProcessing, a 100ms interval and compressed output: stopMs 124 / 146 / 122 against a 2000ms budget, no stacking wav 179968 / 178204 / 178204 m4a 35975 / 35614 / 35582 No "Recording stopped unexpectedly", no "still alive" warnings, no NoSuchMethodError, app alive throughout. compileDebugKotlin, compileDebugAndroidTestKotlin and testDebugUnitTest all BUILD SUCCESSFUL.
Round-9 review found a third caller I had not checked. getStatus() also calls cleanup() while holding audioRecordLock, on its orphaned-service path, so the join this branch added was nested inside that monitor there too — a status() call during a concurrent teardown would burn the full 2s while owning the lock. Rather than keep keying off one caller's flag, cleanup() now takes `callerHoldsRecordLock` and skips the join when it is set. Those callers either terminate the thread themselves (stopRecording) or know it is already gone (getStatus's orphan path, where the recording being gone is what makes the service orphaned). The three callers that do not hold the lock — destroy(), the compressed start failure, and startRecording's catch — keep the join, which is the case this branch exists for. AudioSourceWiringTest caught the signature change and failed, which is what it is for; its matcher is updated. Corrected the events.ts nit: it said one event per kind per recording, but INPUT_READ re-arms after a successful read, so "per uninterrupted failure episode" is what actually happens. Device-verified, fresh install (lastUpdateTime 00:11:48, checked 14s later). Two recordings with enableProcessing, a 100ms interval and compressed output, with getState() hammered eight times during each: maxStatusMs 2 / 0 against a 2000ms budget stopMs 118 / 92 wav 199372 / 167620 m4a 39351 / 33727 Logcat confirms "Detected orphaned recording service, cleaning up..." fired during the run, so the changed path executed rather than being inferred. No false error events, no stall warnings, no NoSuchMethodError, app alive throughout. 81 unit tests pass; compileDebugKotlin and compileDebugAndroidTestKotlin BUILD SUCCESSFUL; TypeScript typecheck clean.
… INPUT_STATE Round-10 review found two, both correctly attributed to this branch. Opting out of the join by nulling the shared recordingThread field created a lost-join race. A non-locking cleanup() clears _isRecording, and before it reads the field a concurrent getStatus() orphan path can call cleanup(callerHoldsRecordLock = true) and null it — so the first cleanup sees no thread, skips the interrupt entirely, and the worker survives teardown. That is the exact failure this branch exists to prevent, reintroduced by the opt-out mechanism. The thread is now captured into a local first, the skip decision is made from that local, and the field is only cleared when it still refers to the thread this call handled. INPUT_STATE never re-armed. Only INPUT_READ was cleared on a successful read, but a device change and a resume both replace and restart AudioRecord while _isRecording stays true — so after a recovery, a later state failure was silently suppressed, contradicting the "per uninterrupted failure episode" behaviour the docs now describe. A successful read means the input is healthy, so it clears both input latches. Device-verified, fresh install (lastUpdateTime 00:22:07, checked 13s later). Three cycles with enableProcessing, a 100ms interval and compressed output, each with getState() hammered six times and a pause/resume in the middle — the recovery path the INPUT_STATE fix is about: stopMs 91 / 83 / 74 against a 2000ms budget maxStatusMs 2 / 0 / 0 wav 231120 / 195840 / 197604 m4a 45640 / 39002 / 38963 No false error events, no stall warnings, no NoSuchMethodError, app alive. 81 unit tests pass; compileDebugKotlin and compileDebugAndroidTestKotlin BUILD SUCCESSFUL.
…alse pause errors Round-11 review found my previous fix was still racy, and it was right. Capturing into a local closed one window but not the one that matters: cleanup() clears _isRecording and only then reads recordingThread, so a concurrent getStatus() orphan cleanup can take and clear the field in between. The first cleanup then sees null, skips the interrupt, and the worker survives into a later recording — the failure this whole branch exists to prevent. recordingThread is now an AtomicReference taken with getAndSet(null). Ownership transfers in one step: exactly one concurrent cleanup receives the thread and is responsible for terminating it, and any other sees null and leaves it alone. There is no read-then-write window left to lose. Second finding: the new INPUT_READ event could fire during ordinary pause/resume. pauseRecording stops AudioRecord before setting isPaused, and resumeRecording clears isPaused before restarting it, so a read landing in either window returns a negative stopped-recorder code. That ordering is pre-existing; turning it into a public JS error event was not. The emit is now gated on recordingState == RECORDSTATE_RECORDING, so ordinary pause/resume timing stays silent while a genuine read failure during active capture still reports. Device-verified, fresh install (lastUpdateTime 00:43:01, checked 13s later). Six rapid pause/resume cycles with enableProcessing and a 100ms interval — the window that produces the false error: cycles 6, stopMs 47, size 153512, dur 1740 "AudioRecord read error" in logcat: 0 "Sending event: error" in logcat: 0 Stated precisely: no read error occurred during this run, so the new guard was not itself exercised — what this shows is that six pause/resume cycles produce no error events and no other regression, not that the guard has been proven to suppress one. 81 unit tests pass; compileDebugKotlin and compileDebugAndroidTestKotlin BUILD SUCCESSFUL.
…l terminate it Round-12 review found the atomic reference was necessary but not sufficient, and that is right. Taking the thread and then declining to join it are incompatible: a caller holding audioRecordLock could win getAndSet(null) and skip the join, so the cleanup that would have joined saw null and the worker outlived teardown. Making the take atomic just moved which caller lost the race. The invariant is now explicit: only a caller that will actually terminate the thread claims the reference. cleanup() takes it solely on the non-locking path, which is the path that joins; stopRecording takes it where it does its own join. A caller holding the lock leaves the reference alone for whoever can act on it, which is what its skip was always meant to mean. Both getAndSet call sites are immediately followed by a join, and set() appears only where the thread is created — so every reference that leaves the field is terminated by the caller that took it. Device-verified, fresh install (lastUpdateTime 00:57:02, checked 13s later). Three cycles with enableProcessing, a 100ms interval, compressed output, getState() hammered five times each and a pause/resume in the middle: stopMs 224 / 134 / 116 against a 2000ms budget maxStatusMs 2 / 0 / 0 wav 144740 / 144682 / 158794 m4a 32635 / 30026 / 32280 Zero "Sending event: error" in logcat, no "did not exit within 2s" warnings, no NoSuchMethodError, app alive throughout. 81 unit tests pass; compileDebugKotlin and compileDebugAndroidTestKotlin BUILD SUCCESSFUL.
…nput recovery Round-13 review: positive audio removed the input-error latches but left failedDeliveryAttempts at whatever it had reached. After three failed deliveries the counter is at its cap, so un-latching alone gave a recovered episode no retry budget and its next failure was silently suppressed — the latch said "report this" and the counter said "give up". Both now clear together on a successful read. The reviewer confirmed the thread-ownership invariant holds, and rejected the cleanup/start race as pre-existing on origin/main (it belongs to #472). Device-verified, fresh install (lastUpdateTime 01:05:54, checked 13s later). Three cycles with enableProcessing, a 100ms interval, compressed output, getState() hammered five times each and a pause/resume in the middle: stopMs 111 / 112 / 144 maxStatusMs 3 / 0 / 0 wav 148212 / 174672 / 153502 m4a 31510 / 35294 / 31209 Zero "Sending event: error" in logcat. 81 unit tests pass; compileDebugKotlin and compileDebugAndroidTestKotlin BUILD SUCCESSFUL.
…loop failures while paused Round-14 review found two, and the second is the same ABI mistake I already made once on this branch with EventSender. A Kotlin default parameter does not emit a no-arg JVM method. javap on my previous commit showed only `cleanup(boolean)` and a synthetic `cleanup$default` — the public `cleanup()` was gone, so any precompiled caller would hit NoSuchMethodError. Restored as a real no-arg method delegating to an internal lock-aware variant. javap now reports `public final void cleanup()`, matching main, with the variant name-mangled as internal. I should have checked this when I added the parameter, having been caught by exactly this on EventSender three rounds ago. Second finding: a genuine loop exception racing pauseRecording() was swallowed, because the LOOP_FAILED emit sat inside the same `if (!isPaused.get())` as the wake-lock release. The worker exits either way, so resume would report success with no PCM thread behind it. The wake-lock release stays conditional — a paused recording still holds it — and the failure is now reported unconditionally. Device-verified, fresh install (lastUpdateTime 08:42:47, checked 15s later). Three cycles with enableProcessing, a 100ms interval, compressed output, getState() hammered five times each and a pause/resume in the middle: stopMs 197 / 113 / 116 maxStatusMs 5 / 0 / 1 wav 160560 / 148210 / 149976 m4a 34103 / 30402 / 30760 Zero "Sending event: error" in logcat, which also confirms the now-unconditional LOOP_FAILED emit does not fire on ordinary teardown. 81 unit tests pass; compileDebugKotlin and compileDebugAndroidTestKotlin BUILD SUCCESSFUL. Open nit from the review, not addressed: no focused automated test covers the retry-budget reset after positive audio. That wants the injectable-recorder seam tracked in #472 rather than a source-assertion test.
…he recorders Round-15 review confirmed both round-14 fixes correct and found one blocker: clearing _isRecording before taking audioRecordLock let a second cleanup enter with wasRecording=false and release the active compressedRecorder while the first was still finalizing it, truncating the compressed file. origin/main serialized the state check and recorder teardown under the lock; this branch broke that when it moved the flag clear earlier so the worker could exit. My first attempt at this was worse than the bug. I wrapped cleanup in a dedicated cleanupLock, which created a lock inversion: cleanupLocked takes audioRecordLock while holding cleanupLock, while stopRecording and getStatus hold audioRecordLock and then call cleanup. Opposite orders, so two threads could deadlock. Backed out before it left my machine. The fix needs no new lock. _isRecording.getAndSet already picks exactly one winner, so that winner owns the recorders and a loser has nothing of its own to release. Release is now gated on `wasRecording || wasPrepared` — the second covers a preparation that never started, which is the original #446 leak and has no finalization coming. isPrepared is captured up front, since the teardown clears it before the release site is reached. Device-verified, fresh install (lastUpdateTime 08:55:15, checked 16s later). Three cycles with enableProcessing, a 100ms interval, compressed output, getState() hammered five times each and a pause/resume in the middle: stopMs 222 / 87 / 96 no deadlock, no stall maxStatusMs 2 / 0 / 0 wav 165852 / 142918 / 141154 m4a 34846 / 29003 / 29320 The last m4a pulled off the device decodes as 44100 Hz AAC at 1.577506 sec and matches its reported 29320 bytes — not truncated, which is the failure this round was about. Zero "Sending event: error", no release failures in logcat. 81 unit tests pass; compileDebugKotlin and compileDebugAndroidTestKotlin BUILD SUCCESSFUL.
Round-16 review found my wasPrepared escape hatch reopened the hole it was meant to close. prepareRecording sets isPrepared, but starting that prepared recording never cleared it — so an active recording had both flags set, and a losing cleanup could observe wasPrepared=true, pass the release gate, and release the recorders the winner was still finalizing. Same truncation, through the door I added. The preparation is consumed the moment the recording starts, so it is cleared there. The two gates are now genuinely disjoint: wasRecording means an active recording whose teardown won the flag, wasPrepared means a preparation that never started — the original #446 leak, which has no finalization coming. Checked the two readers of isPrepared before changing it. startRecording's `if (!isPrepared)` at line 698 runs long before the clear, so a prepared start still skips re-initialization; prepareRecording's guard only prevents double-preparation. Device-verified, fresh install (lastUpdateTime 09:06:36, checked 10s later). The prepared path specifically, since that is what this changes: prepareRecording -> startRecording -> stop, twice prep ok, 179968 bytes / 2039ms both times, m4a 35609 and 35588 and the usual three cycles with getState() hammered five times each plus a pause/resume: stopMs 111 / 72 / 102 maxStatusMs 6 / 0 / 0 wav 151738 / 146446 / 157030 m4a 30416 / 29379 / 31580 Zero "Sending event: error" in logcat. 81 unit tests pass; compileDebugKotlin and compileDebugAndroidTestKotlin BUILD SUCCESSFUL.
…on one lock Ten review rounds went into teardown races on this branch before the actual defect was clear. Round 17 named it: "the prepared-to-recording transition and cleanup ownership need one atomic state or shared lock". A lock audit confirmed it — four of eight entry points did not take audioRecordLock: startRecording UNSYNCHRONIZED stopRecording locked pauseRecording UNSYNCHRONIZED resumeRecording locked prepareRecording UNSYNCHRONIZED getStatus locked cleanup UNSYNCHRONIZED handleDeviceChange locked Every round had the same shape: a path that creates or replaces recorder state runs while teardown is mid-flight. That is only possible because the mutating paths were unguarded. I was hardening one side of a race the other side was free to win, which is why each fix produced a new interleaving instead of closing the class. All four now hold audioRecordLock. The monitor is reentrant, so the nested cleanup() calls from already-locked callers work without inversion — I checked the ordering before writing this, having nearly shipped a cleanupLock inversion two rounds ago. cleanup is now two explicit phases. Phase 1, outside the lock: clear the flag, interrupt and join the worker — this cannot be under the lock, because the recording loop takes it on every read and cannot reach its exit while it is held. Phase 2, under the lock: tear the recorders down, with isPrepared now read live rather than from a snapshot a concurrent start could invalidate. Three call sites needed adjusting once their callers took the lock. The compressed-start failure and startRecordingProcess's catch both run inside startRecording's lock and had already started the thread, so they signal it and hand teardown over as a lock holder rather than joining. prepareRecording's catch never starts a thread, so it just declares it holds the lock. Six tests ship with this, in RecorderLockingTest: every entry point holds the lock, cleanup joins before taking it, locked callers skip the join and declare themselves, starting a recording consumes the preparation, and cleanup() keeps its no-arg JVM signature. They are source assertions — the interleavings need the injectable-recorder seam in #472 to reproduce — and the file says so rather than implying more coverage than exists. Verified they bite: removing startRecording's lock fails the first test with its stated reason. E2E on a Pixel 6a, fresh install (lastUpdateTime 09:22:07), all four paths I touched: plain start/stop x3 stopMs 74/36/64, 127048-128812 bytes prepared start x2 prep ok, 135868 and 134104 bytes, m4a 27915/27791 pause/resume x4 cycles stopMs 54, 91752 bytes getStatus x12 during a compressed recording maxStatusMs 6, stopMs 220 m4a pulled off device: 34844 bytes, exactly the reported size, decodes as 44100 Hz AAC at 1.9258s Zero error events, no stalls, no ANR, app alive throughout. 87 unit tests pass.
…k to join Round-18 review found the gap the refactor left. cleanup read _isRecording before taking audioRecordLock, so during the unlocked join a start or prepare could take the lock, publish a whole new session, and return success. Phase 2 then used the stale wasRecording and stopped, reset, and released the new session's recorders. Teardown is now three phases. Phase 0 claims the session under the lock, taking wasRecording and a sessionId together. Phase 1 joins the worker with the lock released, which is the one step that cannot hold it. Phase 2 retakes the lock and compares sessionId against the claimed value, returning without touching anything if a start or prepare published a new session while we were joining. sessionId increments where recorders are published, in startRecordingProcess and prepareRecording, both under the lock. Three new tests, and I checked each one fails when its invariant is broken: cleanup claims under the lock before joining and tears down in a separate locked block afterwards, cleanup compares sessionId against the claimed value, and both publication points bump it. The existing join-ordering test asserted the wrong thing once phase 0 appeared, so it now checks the three-phase order rather than "the first lock comes after the join". Also dropped a teardownInProgress field I added and never read. The sessionId comparison already covers it. Device-verified, fresh install (lastUpdateTime 09:41:18). Three cycles with processing, a 100ms interval, compressed output, getState hammered six times each and a pause/resume: stopMs 107 / 107 / 88 maxStatus 5 / 0 / 0 wav 144712 / 139420 / 123514 m4a 30013 / 28896 / 25930 The last m4a off the device is 25930 bytes, exactly the reported size, decoding as 44100 Hz AAC at 1.3685s. Logcat shows zero "skipped: session" lines, which is right here since nothing started concurrently, so the check is not firing spuriously and swallowing real teardowns. Prepared start also verified: prep ok, 134104 bytes with a 27847 byte m4a. 89 unit tests pass.
…er it Round-19 review: phase 0 captured the session but left the worker to be taken in phase 1, outside the lock. A start publishing its thread in that gap had it stolen and interrupted by this teardown, so the new recording lost its PCM worker while still reporting success. Phase 2 noticed the session had changed, but by then the damage was done. The worker is now claimed in phase 0 alongside sessionId, under the lock, and phase 1 acts only on that captured reference. A caller that already holds the lock claims no worker, because it terminates the thread itself or knows there is none. Two tests cover it, both verified to fail when broken: cleanup claims the worker under the lock before joining, and the join acts on the claimed reference rather than on whatever recordingThreadRef holds by then. The older assertion looked for `if (!callerHoldsRecordLock)`, which the restructure removed, so it now checks the property that actually matters. Device-verified, fresh install (lastUpdateTime 09:48:44). Three cycles with processing, a 100ms interval, compressed output, getState hammered six times and a pause/resume each: stopMs 116 / 93 / 97 maxStatus 3 / 0 / 1 wav 141156 / 141156 / 127070 m4a 28900 / 28544 / 27043 The last m4a off the device is 27043 bytes, exactly as reported, decoding as 44100 Hz AAC at 1.4382s. No stalls, no wrongful session skips, no error events. 90 unit tests pass.
…up reads Round-20 review: the two in-lock failure paths cleared _isRecording before calling cleanup, and cleanup reads that flag to decide whether it owns the recorders. Seeing false, it skipped the release and leaked the failed MediaRecorder. That is the original #446 leak, reintroduced by the signalling I added when those callers started holding the lock. The worker only needs the interrupt to stop. cleanup clears the flag itself in phase 0, so these paths now interrupt and leave the flag alone. A test covers it, since this leak has now come back twice by different routes: no failure path may clear _isRecording before calling cleanup. Verified it fails when the clear is put back. Its first version searched the whole function body and matched the wrong occurrence of a repeated line, so it walks lines in order instead. Device-verified, fresh install (lastUpdateTime 10:00:13). Three cycles with processing, a 100ms interval, compressed output, getState hammered six times and a pause/resume each: stopMs 107 / 60 / 74 maxStatus 2 / 0 / 0 wav 132334 / 144684 / 142920 m4a 27418 / 28891 / 28910 91 unit tests pass.
Round-21 review: resumeRecording restarts both recorders and resolves success, but never bumped sessionId. A cleanup that claimed the session before the resume saw an unchanged id in phase 2 and released the recorders the resume had just restarted. Resume is a publication point and now increments like the others. The test that lists publication points covers resume too. My first attempt put the increment in the wrong function — the edit matched the first `isPaused.set(false)` in the file, which is in startRecordingProcess, where a bump already existed. The test caught it immediately, which is the second time these assertions have caught my own mistake rather than someone else's. Device-verified, fresh install (lastUpdateTime 10:13:47). One recording through six pause/resume cycles, so six session bumps against a live teardown path: resumes 6, stopMs 127, wav 213516, m4a 42646 The m4a off the device is 42646 bytes, exactly as reported, decoding as 44100 Hz AAC at 2.4134s. Zero error events and zero wrongful session skips, so the ownership check is not firing spuriously despite the id moving six times. 91 unit tests pass.
…in still tears down Round-22 enumerated the remaining gaps in one pass rather than one per round, and found two in this branch's own diff. resumeRecording checked only isPaused. cleanup's phase 0 clears _isRecording and takes the worker before releasing the lock to join, so a resume in that window restarted both recorders, bumped sessionId, and resolved success on a recording with no PCM worker reading from it. _isRecording is the ownership claim, so resume now requires it and rejects otherwise. join throws InterruptedException, and it was unguarded. That skipped all of phase 2 — recorders, the foreground service, the wake lock, audio focus, listeners — and left destroy() without clearing the singleton. Now caught, with the interrupt status restored, and teardown continues. Two tests, both verified to fail when the fix is removed. The review also listed three ownership gaps it ruled out as pre-existing on origin/main: handleDeviceChange replaces and restarts AudioRecord across fragmented lock sections without a session bump, a retiring worker can still mutate manager-wide buffers after a new session publishes, and destroy() can discard the manager after cleanup skips a concurrently published session. Those belong to #472. It confirmed start, prepare, compressed-recorder creation and worker publication all have matching lock and session ownership. Device-verified, fresh install (lastUpdateTime 10:27:05). The resume guard is new behaviour, so both directions were checked: 6 pause/resume cycles all six resumes "ok", stopMs 100 wav 199348, m4a 39310 (byte-exact, 44100 Hz AAC, 2.2044s) resume after stop rejected: "Recording is not paused" 93 unit tests pass.
Round-23: my resume guard was a TOCTOU check. It ran at function entry, outside audioRecordLock, so a teardown could claim the session in the window before the restart — resume would then bump sessionId, restart both recorders, and resolve success with _isRecording false and no PCM worker, while cleanup skipped phase 2 because the id had moved. The check that matters is inside the lock that does the restarting, as the first thing in it. The entry guard stays as a cheap early exit, but the in-lock one is what makes it safe: cleanup cannot claim the session between that check and the restart. isPaused also moved. It was cleared before the lock, so a resume that lost the race left the recording neither paused nor running. It now clears after the recheck passes, alongside the session bump. The test checked only that the guard text existed, which is what let this through. It now asserts placement: the recheck must sit inside the restarting lock and before the restart, and isPaused must clear after it. Verified it fails when the in-lock recheck is removed while the entry guard remains — the exact shape of this bug. Device-verified, fresh install (lastUpdateTime 10:39:22): six pause/resume cycles all returning ok, stopMs 134, wav 217016, m4a 43402 byte-exact and decoding as 44100 Hz AAC at 2.4610s. No spurious resume rejections, so the stricter check is not firing on legitimate resumes. 93 unit tests pass.
deeeed
force-pushed
the
fix/android-recorder-leak-error-event
branch
from
August 22, 2026 02:41
169d424 to
cb71fc8
Compare
Review nit from the approving round: git diff --check flagged lines 786 and 2864. Only those two are touched. A blanket strip across the file rewrote 110 lines and would have buried the reviewed diff in noise.
deeeed
force-pushed
the
fix/android-recorder-leak-error-event
branch
from
August 22, 2026 02:57
a8395cd to
33585ff
Compare
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #446. Closes #447.
Two Android defects that both present to a caller as silence.
#446 — recorder leak on failed initialization
initializeRecordingResources()'s two catch blocks released only the wake lock. Both callers allocate theAudioRecord(and the compressedMediaRecorder) before reaching it, and neither has a catch of its own: each initializer returnsfalseand the caller returns early. A failure there stranded both native recorders until some later attempt happened to calldiscardFailedAttempt()— and if none came, permanently.cleanup()released the compressed recorder only insideif (_isRecording.get()), which a failed preparation never sets, and never nulled it, sodestroy()could not reclaim it either.release()is now unconditional;stop()stays gated, since it throws on a recorder prepared but never started. The stop path nullscompressedRecorder, so the new unconditional release sees null there — no double release.Not in the issue, found while fixing it:
initializeCompressedRecorder()constructs the MediaRecorder and then configures it, so a throw from any setter or fromprepare()stranded it too.#447 —
addRecordingErrorListenernever fired on AndroidiOS declares an
errorevent and emits from 9 sites. Android'sEvents(...)had no equivalent, so the listener was typed cross-platform and inert there. Silence reading as "healthy" is the opposite of the signal a caller subscribes for.Declared
RECORDING_ERROR_EVENT(wire name"error", identical to iOS) and emitted from the four live-recording failures inrecordingProcess():AudioRecordleavingSTATE_INITIALIZED,read()returning an error code, the primary WAV failing to flush, and the loop dying.The read-failure path does not break the loop, so an unguarded emit would fire once per buffer for as long as the fault lasts. A
@Volatilelatch reports one event per episode and re-arms once audio flows again, so a fault that resolves and recurs is still reported. It is cleared when a recording starts, or a degraded recording would suppress the next one's first error.Docs: the iOS-only caveat is replaced with what each platform actually reports, keeping the warning that silence is not evidence of health on either.
Verification
:siteed-audio-studio:compileDebugKotlinBUILD SUCCESSFUL.prepare(), anAudioRecorddropped mid-recording) which I have no way to trigger on a healthy Pixel 6a. Flagging rather than claiming: per the project rule this needs either an injectable seam so the rollback is unit-testable, or a reviewer's call on whether compile-and-unit-test is sufficient for paths that only run when hardware misbehaves.