JMR-dev/LibreMediaConverter
1
0
Fork 0
Code Issues 16 Pull Requests Actions Packages Projects Releases Wiki Activity

R35 — README promises conversions are "restored after a restart"; the measured behaviour is bounded retries then silent failure #44

New Issue
Closed
opened 2026-08-23 03:44:56 +00:00 by JMR-dev · 0 comments
JMR-dev commented 2026-08-23 03:44:56 +00:00 (Migrated from github.com)
Copy Link
Copy Source

Finding R35 from the overnight max-effort review (Fable lead, Opus sub-agents). Full report: scratchpad/overnight/REVIEW.md.

R35 — README promises conversions are "restored after a restart"; the measured behaviour is bounded retries then silent failure

severity: low
verdict: PLAUSIBLE (partly true post-D3; the denial path contradicts it)
where: README.md:106-107
scenario: D13's measured denial path ends in FOREGROUND_DENIED on a FAILED job that Reattachment excludes (D16, open): the user who does not watch the retries opens the app to an empty screen ~8.5h later. D3's reattachment does restore the common case, so the sentence is partly right in a way it was not at 903b43c.
evidence: FailureOutcome.kt:76,:98-99; Reattachment.kt:173; audit D13/D16 device evidence.
fix: Qualify ("restored when the app is reopened; a job the system refuses to restart in the background is retried and then reported"), or wait for D16 and say so. Decide which behaviour the README describes.
risk: The honest version is longer than one clause.

Mis-severity notes on recorded issues (not new findings)

  • D16 should be medium, for a stronger reason than the audit gives. There is no eleventh attempt a user could be watching: FOREGROUND_DENIED is reachable only when the app is in the background by construction (if it were foreground, setForeground would have succeeded), and once the process is recreated Reattachment excludes FAILED — so the message is written to a state no user can reach. The only surviving path is a process that stayed alive backgrounded with its ViewModel intact for the whole ~8h31m loop, which is not D13's scenario (D13 begins with process death). The bound still buys the end of the retrying, but the D13 fix should not be described as closing D13's user-facing cost. Combined with R7, carry D16 at medium. (Reviewer verified the attempt arithmetic against work-runtime sources: runAttemptCount is read before increment, so <10 yields 11 attempts / 10 waits ≈ 8h31m — the KDoc's numbers are right.)
  • D1's parked branch is separable. See R23: the overflow/clamp half of fix/allocatable-space fixes a live main bug and does not depend on the blocked measurement. The allocatable-vs-usable half stays parked as decided.

Branch verdicts

tools/api-37-emulator — mergeable with corrections. The re-derivation is sound; the strongest documentation in the repo (seven runs varying one thing, discriminator stated mechanically, inference labeled). Fix first: R4 (expect-49 — wrong the moment it merges, on the one check with no CI backstop), R16 (undocumented always-red exit), R27 (the one over-claim the tightening commit missed). On merge it falsifies: CLAUDE.md:73-76 (recorded by the branch, deliberately unapplied), status_check.yml:207-213 (NOT recorded — R19), memory files api-37-manual-release-check.md and emulators-segfault-on-this-host.md (NOT recorded; the second is already falsified by main), and three main local-emulator.md statements (patched by the branch itself). The audit's "not covered here" pointer survives.

fix/allocatable-space — stays parked; three landing risks recorded for when the measurement happens. (1) The JVM suite cannot see the change: allocatableBytes() is never invoked by any test (proved by instrumented port; 264/0/0 with it in place), and under Robolectric getUuidForPath throws an NPE the commit's throws-analysis does not name — vindicating its catch(Exception), but meaning fail-open makes the check silently vacuous wherever measurement breaks, with only an instrumented test (CI/Pixel-only) pinning refusal. (2) The branch predates hasSpaceForUnknownSize() (D5 fix): under its rule the floor for every unsizeable job moves from 128 MiB usable to ~628 MiB effective on the measured device — a decision the branch owner has not had the chance to make. (3) Rebase required: the branch's OutputPublisher predates the D4 publish rewrite, discardStaged and the sweep, and reintroduces a KDoc line main deliberately rewrote — hand-reconcile, take neither side. Otherwise the commit does what it claims; the audit's checklist (nullable seam, decided+tested IOException policy, UsableSpace lint-registry deletion) is satisfied item for item.

Adversarially checked and refuted (do not re-ticket)

  • SAF-bridge fd leak in getSafParameterForRead — refuted by decompiling the AAR (descriptor opened lazily by native safOpen).
  • Saved state clobbered by WorkInfo re-emission — refuted (distinctUntilChanged in WorkSpecDao).
  • hasRoomFor above the try as a D13 escape — refuted (InputQuery fully runCatching-guarded; usableSpace returns 0, never throws). [Lead reached the same refutation independently.]
  • BackgroundServiceStartNotAllowedException slipping the exact-class match — refuted (Processor uses ContextCompat.startForegroundService on the denied path).
  • Retry re-opening a previous attempt's partial — refuted (-y on every FFmpeg command; staged.delete before software fallback).
  • c2e6344 moving the progress throttle — refuted (setProgressAsync was already unthrottled at 903b43c; only the notification ever throttled).
  • Ambiguous meaningless for joins — refuted (a live join has no outputPath; Certain returned before aliasing is considered; the code comment already says this).
  • Forward clock skew defeating the sweep — refuted (sweep runs once per process start, before a jump could matter).
  • ConcatEngine list file orphaned on early throw — self-healing (jobId-named, swept); not a finding.
  • Media3 cancel racing quitSafely — window sub-millisecond; dropped rather than reported.
  • Renamed-away assertions — hunted for, none found; the one candidate (ConcatEngineTest's literal) was already fixed in 2a68f03 with a comment naming this failure mode.
  • Test stubs stubbing the seam under test — checked; they do not (AlwaysRoomPublisher overrides only hasSpaceFor; RecordingPublisher really deletes; PerJobStaging/WorkerOutputNaming drive real workers).

Known-and-recorded, correctly excluded

D15 (open), D16 (open — mis-severity note above), D1 (parked — separability note above), D12 (no-action), D5/D7 (recorded open in the audit — actually merged; that discrepancy is R3, a doc finding, not a code one).

Where the remaining risk actually sits

  1. The untested glue between well-tested pure cores and the framework: R1/R2/R8/R9 are all the same disease, and all JVM-fixable tonight.
  2. The D13 denial path's user-facing end (R7 + the D16 note): correct mechanics, no affordance and no message anyone will see. Needs product decisions, not just code.
  3. Documentation currency as a process: five of tonight's doc findings (R3, R12, R15, R20, R4) are the same failure — absolute numbers and status claims in unregenerated documents, written the same day they went stale. A convention (anchor every count to a commit, or derive it) is worth a ticket of its own.
  4. What only a device can verify: publish() against a real provider, the allocatable-space refusal assertion, FGS budget-exhaustion behaviour (SystemForegroundService catches the exception and the worker runs on with no foreground service — consequence unobserved), and the sweep's mtime-freshness premise for two-pass encodes. These are named, not closed.

Artifacts

  • scratchpad/overnight/tests/ — mutations.tsv (45 rows), muts/M*.sh (reproducible), logs/, clone (reverted clean).
  • scratchpad/overnight/lifecycle/ReviewEnumEscapeTest.kt.txt — R22's demonstration, inverts into the regression test.
  • scratchpad/overnight/{docs,resources}/REPORT-received.md — full sub-reviewer reports as received.
  • Real repo and all .claude/worktrees checkouts untouched throughout (verified: git status clean at 18c53a3).

Cut: below — held for manual review: product decision, CI/workflow, hardware, or PLAUSIBLE verdict.

🤖 Generated with Claude Code

_Finding **R35** from the overnight max-effort review (Fable lead, Opus sub-agents). Full report: `scratchpad/overnight/REVIEW.md`._ ### R35 — README promises conversions are "restored after a restart"; the measured behaviour is bounded retries then silent failure severity: low verdict: PLAUSIBLE (partly true post-D3; the denial path contradicts it) where: README.md:106-107 scenario: D13's measured denial path ends in FOREGROUND_DENIED on a FAILED job that Reattachment excludes (D16, open): the user who does not watch the retries opens the app to an empty screen ~8.5h later. D3's reattachment does restore the common case, so the sentence is partly right in a way it was not at 903b43c. evidence: FailureOutcome.kt:76,:98-99; Reattachment.kt:173; audit D13/D16 device evidence. fix: Qualify ("restored when the app is reopened; a job the system refuses to restart in the background is retried and then reported"), or wait for D16 and say so. Decide which behaviour the README describes. risk: The honest version is longer than one clause. --- ## Mis-severity notes on recorded issues (not new findings) - **D16 should be medium, for a stronger reason than the audit gives.** There is no eleventh attempt a user could be watching: FOREGROUND_DENIED is reachable only when the app is in the background by construction (if it were foreground, setForeground would have succeeded), and once the process is recreated Reattachment excludes FAILED — so the message is written to a state no user can reach. The only surviving path is a process that stayed alive backgrounded with its ViewModel intact for the whole ~8h31m loop, which is not D13's scenario (D13 begins with process death). The bound still buys the end of the retrying, but the D13 fix should not be described as closing D13's user-facing cost. Combined with R7, carry D16 at medium. (Reviewer verified the attempt arithmetic against work-runtime sources: runAttemptCount is read before increment, so <10 yields 11 attempts / 10 waits ≈ 8h31m — the KDoc's numbers are right.) - **D1's parked branch is separable.** See R23: the overflow/clamp half of fix/allocatable-space fixes a live main bug and does not depend on the blocked measurement. The allocatable-vs-usable half stays parked as decided. ## Branch verdicts **tools/api-37-emulator — mergeable with corrections.** The re-derivation is sound; the strongest documentation in the repo (seven runs varying one thing, discriminator stated mechanically, inference labeled). Fix first: R4 (expect-49 — wrong the moment it merges, on the one check with no CI backstop), R16 (undocumented always-red exit), R27 (the one over-claim the tightening commit missed). On merge it falsifies: CLAUDE.md:73-76 (recorded by the branch, deliberately unapplied), status_check.yml:207-213 (NOT recorded — R19), memory files api-37-manual-release-check.md and emulators-segfault-on-this-host.md (NOT recorded; the second is already falsified by main), and three main local-emulator.md statements (patched by the branch itself). The audit's "not covered here" pointer survives. **fix/allocatable-space — stays parked; three landing risks recorded for when the measurement happens.** (1) The JVM suite cannot see the change: allocatableBytes() is never invoked by any test (proved by instrumented port; 264/0/0 with it in place), and under Robolectric getUuidForPath throws an NPE the commit's throws-analysis does not name — vindicating its catch(Exception), but meaning fail-open makes the check silently vacuous wherever measurement breaks, with only an instrumented test (CI/Pixel-only) pinning refusal. (2) The branch predates hasSpaceForUnknownSize() (D5 fix): under its rule the floor for every unsizeable job moves from 128 MiB usable to ~628 MiB effective on the measured device — a decision the branch owner has not had the chance to make. (3) Rebase required: the branch's OutputPublisher predates the D4 publish rewrite, discardStaged and the sweep, and reintroduces a KDoc line main deliberately rewrote — hand-reconcile, take neither side. Otherwise the commit does what it claims; the audit's checklist (nullable seam, decided+tested IOException policy, UsableSpace lint-registry deletion) is satisfied item for item. ## Adversarially checked and refuted (do not re-ticket) - SAF-bridge fd leak in getSafParameterForRead — refuted by decompiling the AAR (descriptor opened lazily by native safOpen). - Saved state clobbered by WorkInfo re-emission — refuted (distinctUntilChanged in WorkSpecDao). - hasRoomFor above the try as a D13 escape — refuted (InputQuery fully runCatching-guarded; usableSpace returns 0, never throws). [Lead reached the same refutation independently.] - BackgroundServiceStartNotAllowedException slipping the exact-class match — refuted (Processor uses ContextCompat.startForegroundService on the denied path). - Retry re-opening a previous attempt's partial — refuted (-y on every FFmpeg command; staged.delete before software fallback). - c2e6344 moving the progress throttle — refuted (setProgressAsync was already unthrottled at 903b43c; only the notification ever throttled). - Ambiguous meaningless for joins — refuted (a live join has no outputPath; Certain returned before aliasing is considered; the code comment already says this). - Forward clock skew defeating the sweep — refuted (sweep runs once per process start, before a jump could matter). - ConcatEngine list file orphaned on early throw — self-healing (jobId-named, swept); not a finding. - Media3 cancel racing quitSafely — window sub-millisecond; dropped rather than reported. - Renamed-away assertions — hunted for, none found; the one candidate (ConcatEngineTest's literal) was already fixed in 2a68f03 with a comment naming this failure mode. - Test stubs stubbing the seam under test — checked; they do not (AlwaysRoomPublisher overrides only hasSpaceFor; RecordingPublisher really deletes; PerJobStaging/WorkerOutputNaming drive real workers). ## Known-and-recorded, correctly excluded D15 (open), D16 (open — mis-severity note above), D1 (parked — separability note above), D12 (no-action), D5/D7 (recorded open in the audit — actually merged; that discrepancy is R3, a doc finding, not a code one). ## Where the remaining risk actually sits 1. The untested glue between well-tested pure cores and the framework: R1/R2/R8/R9 are all the same disease, and all JVM-fixable tonight. 2. The D13 denial path's user-facing end (R7 + the D16 note): correct mechanics, no affordance and no message anyone will see. Needs product decisions, not just code. 3. Documentation currency as a process: five of tonight's doc findings (R3, R12, R15, R20, R4) are the same failure — absolute numbers and status claims in unregenerated documents, written the same day they went stale. A convention (anchor every count to a commit, or derive it) is worth a ticket of its own. 4. What only a device can verify: publish() against a real provider, the allocatable-space refusal assertion, FGS budget-exhaustion behaviour (SystemForegroundService catches the exception and the worker runs on with no foreground service — consequence unobserved), and the sweep's mtime-freshness premise for two-pass encodes. These are named, not closed. ## Artifacts - scratchpad/overnight/tests/ — mutations.tsv (45 rows), muts/M*.sh (reproducible), logs/, clone (reverted clean). - scratchpad/overnight/lifecycle/ReviewEnumEscapeTest.kt.txt — R22's demonstration, inverts into the regression test. - scratchpad/overnight/{docs,resources}/REPORT-received.md — full sub-reviewer reports as received. - Real repo and all .claude/worktrees checkouts untouched throughout (verified: git status clean at 18c53a3). --- **Cut:** `below` — held for manual review: product decision, CI/workflow, hardware, or PLAUSIBLE verdict. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
above-cut
Worked autonomously overnight: local, JVM-verifiable, no product decision
 accessibility
Barrier affecting people with disabilities
 backlog
Held for manual review: product/UX call, CI/workflow, hardware, or unverifiable here
 bug
Something isn't working
 confirmed
Reviewer demonstrated the defect
 documentation
Improvements or additions to documentation
 duplicate
This issue or pull request already exists
 enhancement
New feature or request
 good first issue
Good for newcomers
 help wanted
Extra attention is needed
 invalid
This doesn't seem right
 plausible
Reviewer could not fully demonstrate it; treat as unproven
 question
Further information is requested
 sev:high
High severity
 sev:low
Low severity
 sev:medium
Medium severity
 wontfix
This will not be worked on
No labels backlog documentation plausible sev:low
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
JMR-dev
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: JMR-dev/LibreMediaConverter#44