fix: simplify Android recording recovery#1135
Conversation
Size Report
Startup median (7 runs, lower is better):
Top changed chunks:
|
|
thymikee
force-pushed
the
fix/android-recording-durable-recovery-1130-latest
branch
2 times, most recently
from
8db06dd to
a3e137d
Compare
|
Reviewed #1135 at head |
thymikee
added
the
ready-for-human
🔬 Thermo-nuclear review — Android recording recovery (#1135)Reviewed on The direction is right. A device-side durable manifest with explicit ownership is a real improvement over the ownerless process scan, and pulling parsing/building into 🔴 Blocker — manifest ownership compares two different session-name schemesOwnership matching keys on // record-trace-android-recovery.ts:639
manifest.sessionName === activeSession.name && sessionScopesEqual(...)But
Concrete repro (the headline flow, in any cwd scope root — i.e. the default when run from a project dir without
So a normally-opened, cwd-scoped recording cannot be recovered after a restart — the exact scenario this PR hardens. It's currently green only because the two recovery tests avoid the Fix: key the manifest on the effective store key, consistently at both ends. 🟠 Code-judo — the
|
| current | pending | status |
|---|---|---|
| ✓ | – | live |
| – | ✓ | pending |
| ✓ | ✓ | rotating |
The parser even already infers it (readAndroidRecoveryManifestStatus, manifest.ts:230), and then spends four branches re-checking that the stored status agrees with presence (manifest.ts:85-93: invalid_current/pending/rotating_recording). Those disagreement checks exist only because status is stored redundantly. status is read in exactly two places — parse-time validation and resolve dispatch (grep confirms) — and written in three build functions.
Dropping the stored field lets you delete the enum type, the inference, the four validation branches, and collapse the three near-identical resolve*Candidate functions (recovery.ts:132-222) into one that dispatches on presence — behavior-preserving:
async function resolveAndroidRecoveryCandidate(deviceId, manifest): Promise<AndroidRecoveryResolution> {
if (manifest.pending) {
const adopted = await findLiveAndroidScreenrecordByPath(deviceId, manifest.pending.remotePath);
if (adopted && adopted !== 'uncertain') {
return live(manifest, { current: adopted, warning: manifest.current ? ROTATION : START });
}
if (!manifest.current) return adopted === 'uncertain' ? uncertain() : stale();
const liveness = await checkRecoverableAndroidScreenrecord(deviceId, manifest.current);
if (liveness === 'uncertain' || adopted === 'uncertain') return uncertain();
if (liveness === 'stale') return stale();
return live(manifest, {
current: manifest.current,
chunks: chunksThroughRemotePath(manifest.chunks, manifest.current.remotePath),
warning: liveness === 'finished' ? `${ROTATION} ${FINISHED}` : ROTATION,
});
}
if (!manifest.current) return stale();
const liveness = await checkRecoverableAndroidScreenrecord(deviceId, manifest.current);
if (liveness === 'live') return live(manifest, { current: manifest.current });
if (liveness === 'finished') return live(manifest, { current: manifest.current, warning: FINISHED });
return liveness === 'uncertain' ? uncertain() : stale();
}This is the single biggest lever in the PR — it removes a whole enum + its validation surface and ~90 lines of triplicated dispatch, and it's the main reason record-trace-android-recovery.ts nearly doubled (357 → 665). If you'd rather keep status on-device for human debuggability, that's a defensible call — but then at least drop the disagreement-validation (trust presence) and still merge the three resolve functions.
🟠 Reuse — the "pick the one owned manifest, else error" decision is duplicated
selectAndroidRecoveryManifest (recovery.ts:554-576) and blockAndroidManifestRecoveryForUncertainManifest (recovery.ts:500-529) both: filter by androidRecoveryManifestMatchesSession, summarizeAndroidActiveRecordings, then branch on matches.length === 0 (owner mismatch) / > 1 || manifests.length > 1 (ambiguous). That's the same ownership+multiplicity model written twice, kept in sync by hand. Extract one resolveOwnedManifest(manifests, activeSession): { owned } | { error }; the uncertain path just turns the "would-recover" result into a block with its own hint. Removes a duplicated invariant.
🟡 The prepareRemotePath / cleanupPreparedRemotePath hooks are a generic seam for one caller
startAndroidScreenrecordChunk gained a generic hooks object (android.ts) whose only job is to write the pending/rotating manifest before each path attempt. The contract is also unusual: prepareRemotePath returns an error string (not a thrown error, not a boolean) to abort the attempt, and cleanupPreparedRemotePath is fire-and-forget. It reads as a generic mechanism hiding one concrete assumption. It does keep chunk-start decoupled from recovery, so it's borderline — but consider inlining the manifest write at the (single) call sites, or at minimum renaming to make "returns an error to veto this path" explicit. Right now the seam costs more indirection than it buys.
🟡 Smaller structural cleanups
- Name the repeated union.
Promise<{kind:'live';manifest}|{kind:'stale'}|{kind:'uncertain'}>is written verbatim 3× (recovery.ts:135, 167, 189). One named type. (Moot if the resolve functions collapse per above.) - Duplicated chunk-default.
recording.chunks ?? [{ index: 1, remotePath: recording.remotePath }]and the.map(c => ({index, remotePath}))normalization appear in bothbuildAndroidRecoveryManifestandbuildAndroidRecoveryRotatingManifest(manifest.ts:160, 175). Extract atoManifestChunks(recording, extra?). - Double split.
checkRecoverableAndroidScreenrecordsplitsresult.stdouton newlines twice (recovery.ts:251-261) — computelinesonce.
✅ Rotation transaction — traced adversarially, no corruption defect
The behavioral change from stop-then-start to start-next-concurrently → finish-previous (with a sequential fallback and apply/commit/rollback/discard) is well-built. previousChunkFinished is honored so the previous chunk is stopped exactly once on every branch; the un-reverted rotating manifest after a persist-failure rollback is safe because recovery re-derives liveness from the device and chunksThroughRemotePath trims the stale trailing chunk; and finishCurrentAndroidRecordingChunk's defaulted remotePath/remotePid is never confused because rotation always passes explicit chunk args. Residual, low-severity notes:
- New orphan (low): if the final stop of the previous chunk fails after a successful concurrent start+persist (
chunks.ts:151),recordingalready points at the next pid, so the previousscreenrecordis never re-stopped — it self-terminates at the 180s limit and its file is still copied, so it self-heals. Vs. base (which never ran two processes) this is a genuinely new, if very unlikely, leak. - Overlap tradeoff: two encoders run for ~0.5–2s per rotation. The common "device rejects the 2nd encoder" case is handled by the fallback; the residual risk is
waitForAndroidRecordingReady'sMIN_RUNNING_POLLS=2early-return accepting a lingering-but-dead 2nd encoder — pre-existing probe weakness that concurrency makes newly reachable. - Cosmetic: a stop that races an in-flight rotation can append the "180s limit / may be truncated" warning even on a clean end (
android.ts:537).
Scope note: this concurrent-rotation change is a meaningful runtime-behavior shift bundled into a PR titled "recovery." It's the main driver of record-trace-android-chunks.ts 127 → 307. Given the manifest needs a live pending pid to probe, the coupling is understandable — but it's worth a line in the description, and ideally its own follow-up if you want to de-risk the two live-encoder window separately.
Verdict
Request changes. The ownership blocker breaks the primary open→record→restart→stop flow under cwd scoping and must be fixed (thread the effective sessionName as the ownership key + add the missing integration test). Separately, the status-derivation simplification is a high-value, behavior-preserving cleanup that removes an entire redundant state dimension and most of the file-size growth — strongly recommended. Everything else is polish. Nice work on the manifest extraction and the rotation transaction.
thymikee
removed
the
ready-for-human
thymikee
force-pushed
the
fix/android-recording-durable-recovery-1130-latest
branch
from
a3e137d to
a4833f8
Compare
1 participant
Summary
Validation