claimFinalName follow-ups from the #7 review: the untested refusal branch, and three claims the KDoc overstates #10

New issue

Closed

opened 2026-08-24 16:57:16 +02:00 by julian · 0 comments

julian commented

2026-08-24 16:57:16 +02:00

Owner

Four non-blocking items from @thisilike's approving review of #7 (review). None of them change what ships — one adds the coverage the new code is missing, three correct sentences that are narrower or wider than the truth.

1. The refusal branch has no coverage on the host CI uses

claimFinalName's fill-refused branch — refused, target.delete(), the strike, and the IllegalStateException("cannot claim a name …", refused) backstop — is the part added in answer to the first review round, and it is the one part nothing pins. The reviewer's instrumented run recorded zero refusals across 3000 rounds × 16 threads on Linux, which is expected (unlink of an open file just works there) and means the branch is exercised only incidentally, on a Windows desk, by Defender.

It is deterministic in three lines: hand the claim a temp that does not exist, and every fill is refused with NoSuchFileException. One test then covers the strike, the delete, the exhaustion, the cause riding along, and the "no corpse" property.

2. The accepted loss is wider than the KDoc scopes it to

One loss is accepted: a process death between the claim and the rename …

Process death is not the only way. target.delete()'s result is ignored, and the refusal actually observed — a scanner holding the fresh placeholder — is precisely the state in which a Windows delete also fails. When it does, the placeholder survives as a zero-byte file under the final name with no crash involved, poisoning that number exactly as the process-death case does.

Shipping impact is nil (Android unlinks an open file), so the code needs nothing. The sentence does: the loss is a claim this transfer could not release, whatever stopped it.

Same cause, second consequence: concurrent claims …' third assertion (assertEquals(landed.keys, downloads.list()!!.toSet())) is then a real flake on the one host where the refusal has been seen. Worth saying in the test rather than weakening the assertion, which is load-bearing on the hosts that matter.

3. Worth writing down: this path only ships to API 26–28

claimFinalName is reached from downloadToAppDownloads, the else of SDK_INT >= Q. Android 10 and up take downloadViaMediaStore, where the final name is settled by MediaStore's own insert — an atomic dedupe, no check-then-claim anywhere in it, so there is genuinely nothing to fix there. The KDoc argues from "two pages on the back stack", which reads as a statement about any device; the next person to touch the Q+ path deserves the qualifier where the reasoning lives.

4. "on the desk, where these tests run"

They also run on cth-ubuntu-latest, twice per push to main (lintDebug, testReleaseUnitTest). The reason for ATOMIC_MOVE is sound and stands — a test that must be green on both hosts has to exercise the move that ships — but the clause puts the tests in one place and there are two.

Docs and tests only; no behaviour change, so no versionName bump.

Corrections (added while working #11 — the text above is left as written)

Review of #11 found five claims in this issue that do not hold. Kept rather than edited away, so the reasoning the PR answers is still readable.

§1, "it is the one part nothing pins" — not exhaustive. The createNewFile → "cannot write to …" branch (SchemaViewModel.kt:381-385 on main, :430-436 on #11's head) has no coverage either, and pinning it wants an unwritable directory, which is a different fixture from a refused move. #11 does not add it.
§1, "3000 rounds × 16 threads" — not the committed test. That is the reviewer's instrumented run from #7. The test in the repo is repeat(200) with n = 8; it produces no refusals either, so the conclusion stands, but the numbers name something else.
§2, "under the final name" — wrong for the failed release. After a failed delete() the loop strikes that candidate and lands under a later number, so the corpse is not under this transfer's final name. True only for the process-death case. Also: the set is not closed at two — anything thrown between the claim and the release that the function does not catch strands the same corpse.
§3, MediaStore mechanism. The insert does not settle the final name; it settles a pending row under the fallback name. The served name is applied afterwards by resolver.update, which is the call that can collide and has its own fallback and read-back (displayNameOf). The conclusion — no check-then-claim to race — survives; the mechanism as written does not match the function it cites.
§4, "there are two [hosts]". CI runs on one, cth-ubuntu-latest, in both workflows. The desk is a desk, not a second CI host, and the ATOMIC_MOVE argument does not need a count either way.

#11 also goes past §1's plan: the exhaustion fixture pins the branch through a temp that does not exist, which downloadToAppDownloads never produces (it always creates its own), so the fill became a defaulted parameter and a second test refuses exactly one fill — the shape a device hits, and the only way to observe the release-and-retry the KDoc promises.

One more, from the review of `c855bed`

The bound was never "pinned elsewhere". #11's description claimed repeat(1000) was held by concurrent claims … rather than by the new exhaustion test. It is the other way round: repeat(200) leaves the concurrency test green and fails the exhaustion test, so that test is the only thing pinning the bound — and it reports a shrunk bound as a strike failure. Said in the test's own comment on #11's head.

Four non-blocking items from @thisilike's approving review of #7 ([review](https://git.imhof.cloud/OpsDeck/mobile/pulls/7#issuecomment-1110)). None of them change what ships — one adds the coverage the new code is missing, three correct sentences that are narrower or wider than the truth. ## 1. The refusal branch has no coverage on the host CI uses `claimFinalName`'s fill-refused branch — `refused`, `target.delete()`, the strike, and the `IllegalStateException("cannot claim a name …", refused)` backstop — is the part added in answer to the first review round, and it is the one part nothing pins. The reviewer's instrumented run recorded **zero** refusals across 3000 rounds × 16 threads on Linux, which is expected (unlink of an open file just works there) and means the branch is exercised only incidentally, on a Windows desk, by Defender. It is deterministic in three lines: hand the claim a `temp` that does not exist, and every fill is refused with `NoSuchFileException`. One test then covers the strike, the delete, the exhaustion, the cause riding along, and the "no corpse" property. ## 2. The accepted loss is wider than the KDoc scopes it to > One loss is accepted: a process death between the claim and the rename … Process death is not the only way. `target.delete()`'s result is ignored, and the refusal actually observed — a scanner holding the fresh placeholder — is precisely the state in which a Windows delete also fails. When it does, the placeholder survives as a zero-byte file under the final name with no crash involved, poisoning that number exactly as the process-death case does. Shipping impact is nil (Android unlinks an open file), so the code needs nothing. The sentence does: the loss is a claim this transfer could not release, whatever stopped it. Same cause, second consequence: `concurrent claims …`' third assertion (`assertEquals(landed.keys, downloads.list()!!.toSet())`) is then a real flake on the one host where the refusal has been seen. Worth saying in the test rather than weakening the assertion, which is load-bearing on the hosts that matter. ## 3. Worth writing down: this path only ships to API 26–28 `claimFinalName` is reached from `downloadToAppDownloads`, the `else` of `SDK_INT >= Q`. Android 10 and up take `downloadViaMediaStore`, where the final name is settled by MediaStore's own insert — an atomic dedupe, no check-then-claim anywhere in it, so there is genuinely nothing to fix there. The KDoc argues from "two pages on the back stack", which reads as a statement about any device; the next person to touch the Q+ path deserves the qualifier where the reasoning lives. ## 4. "on the desk, where these tests run" They also run on `cth-ubuntu-latest`, twice per push to main (`lintDebug`, `testReleaseUnitTest`). The reason for `ATOMIC_MOVE` is sound and stands — a test that must be green on both hosts has to exercise the move that ships — but the clause puts the tests in one place and there are two. --- Docs and tests only; no behaviour change, so no `versionName` bump. --- ## Corrections (added while working #11 — the text above is left as written) Review of #11 found five claims in this issue that do not hold. Kept rather than edited away, so the reasoning the PR answers is still readable. 1. **§1, "it is the one part nothing pins" — not exhaustive.** The `createNewFile` → `"cannot write to …"` branch (`SchemaViewModel.kt:381-385` on `main`, `:430-436` on #11's head) has no coverage either, and pinning it wants an unwritable directory, which is a different fixture from a refused move. #11 does not add it. 2. **§1, "3000 rounds × 16 threads" — not the committed test.** That is the reviewer's instrumented run from #7. The test in the repo is `repeat(200)` with `n = 8`; it produces no refusals either, so the conclusion stands, but the numbers name something else. 3. **§2, "under the final name" — wrong for the failed release.** After a failed `delete()` the loop strikes that candidate and lands under a later number, so the corpse is not under this transfer's final name. True only for the process-death case. Also: the set is not closed at two — anything thrown between the claim and the release that the function does not catch strands the same corpse. 4. **§3, MediaStore mechanism.** The insert does not settle the final name; it settles a *pending* row under the fallback name. The served name is applied afterwards by `resolver.update`, which is the call that can collide and has its own fallback and read-back (`displayNameOf`). The conclusion — no check-then-claim to race — survives; the mechanism as written does not match the function it cites. 5. **§4, "there are two [hosts]".** CI runs on one, `cth-ubuntu-latest`, in both workflows. The desk is a desk, not a second CI host, and the `ATOMIC_MOVE` argument does not need a count either way. #11 also goes past §1's plan: the exhaustion fixture pins the branch through a `temp` that does not exist, which `downloadToAppDownloads` never produces (it always creates its own), so the fill became a defaulted parameter and a second test refuses exactly one fill — the shape a device hits, and the only way to observe the release-and-retry the KDoc promises. ### One more, from the review of `c855bed` 6. **The bound was never "pinned elsewhere".** #11's description claimed `repeat(1000)` was held by `concurrent claims …` rather than by the new exhaustion test. It is the other way round: `repeat(200)` leaves the concurrency test green and fails the exhaustion test, so that test is the only thing pinning the bound — and it reports a shrunk bound as a strike failure. Said in the test's own comment on #11's head.

julian referenced this issue from a commit

2026-08-24 17:05:20 +02:00

test(download): pin the refusal branch, and stop the KDoc overstating three things

julian referenced this issue from a pull request that will close it,

2026-08-24 17:05:42 +02:00

refactor(download): pin the refusal branch, and stop the KDoc overstating three things #11