fix(history): retain failed downloads and recent-page dismissals

[kshivang]

Consolidated review of #1421 and #1422

This ready-for-dev PR consolidates two legitimate, independent fixes from @manishakuhar, prepared oldest-first on current dev:

• fix(downloads): retain failed removals for retry #1421: failed completed-download deletion and rejected active-download cancellation now retain tracking and return failure so callers can retry; missing files remain idempotent.
• fix(history): preserve dismissals during recent-page loading #1422: recent-page Clear/Remove dismissals are recorded for in-flight disk loads, including browser-history bootstrap, while later visits remain visible.

The changes do not overlap in source files, but were reviewed and validated together to avoid integrating either stale branch base. Author attribution is preserved on both commits via -x cherry-picks.

Validation

• ./gradlew :composeApp:desktopTest --tests 'ai.rever.boss.components.plugin.panels.left_top.DownloadDataProviderImplTest' --tests 'ai.rever.boss.dashboard.RecentPagesDismissalDuringLoadTest' --no-daemon
• ./gradlew :composeApp:ktlintCheck :composeApp:detekt --no-daemon

Both passed on current dev (9.5.22). Related merged #821 and open #1077 were checked; #1422 is follow-up work, not a duplicate. #1407 and #1195 were also checked for adjacent download scope.

[github-actions]

Claude diff review of 4f21b91 (no tests executed).

Review of PR #1477 @ 4f21b91 (→ dev)

Scope reviewed: the diff only. I have not run the build or tests and cannot see the surrounding bodies of removeDownload, scheduleSave, saveImmediately, mergeRecordedPages, mergeDismissedSuggestions, FileSystemUtils.cleanupPartialFile, so findings that depend on those are marked uncertain.

---

Confirmed defects

1. DownloadDataProviderImpl: a rejected cancel now creates a permanently unremovable entry plus orphaned bytes

composeApp/src/desktopMain/.../DownloadDataProviderImpl.kt (~lines 98–115):

val cancelled = engine.cancel(id)
val settled = downloadManager.getDownload(id)
check(cancelled || settled == null || settled.isTerminal) {
    "The download is still active. Could not cancel it; retry removal."
}

The code this replaces existed precisely because the engine may never emit a follow-up event after a rejected cancel — the deleted comment says so verbatim: "on a rejected cancel no event arrives at all", which is why it also called FileSystemUtils.cleanupPartialFile(...) unconditionally.

Under the new code, if engine.cancel(id) returns false and the tracked status never transitions to terminal (exactly the case the old comment describes: the engine has already dropped the id, so no listener event will ever update DownloadManager), then:

• check fails on every attempt → removal fails forever, with no escape hatch;
• the partial file is never cleaned up (the cleanupPartialFile fallback is now unreachable on that branch).

The new test encodes this as desired behaviour, and in doing so demonstrates the dead end: FakeDownloadEngine(liveIds = emptySet()) means the engine does not know d4, cancel returns false, and the manager keeps DOWNLOADING — so the asserted "retry" in the test name will fail identically on every retry (DownloadDataProviderImplTest.kt, rejected cancellation keeps the active download and its bytes for retry, ~lines 83–98). Suggest distinguishing "engine rejected because it never owned the id" (safe to drop + clean up) from "engine refused while actively writing" (retain), or adding a forced-removal path.

2. Path.of / Files.deleteIfExists turn previously tolerated states into permanent failures

Same file, lines ~97 and ~112:

Files.deleteIfExists(Path.of(download.destinationPath))

• Path.of throws InvalidPathException (unchecked) for paths the old File(...) constructor accepted (embedded NUL, Windows-illegal characters in a path persisted by an older build). Combined with the new "filesystem failure retains the entry" contract, such a row can never be removed from the UI.
• Non-transient IOExceptions (AccessDeniedException on a read-only directory, Windows sharing violation from an external process) are now also permanent, while the user-visible message asserts the opposite: "retry removal."
• The logger.warn(LogCategory.FILE, "Failed to delete file", …) call was dropped, so these failures now have no log record at all — only a Result whose surfacing I cannot verify from the diff.

3. check() used for an expected runtime race

Lines ~106–108. check signals a programmer-contract violation and throws IllegalStateException; here it models a normal engine race and its message is written as user-facing copy. If any upstream layer treats IllegalStateException as a bug (crash reporting, rethrow outside the runCatching that the tests imply exists), this becomes a crash rather than a failed operation. An explicit typed failure (Result.failure(DownloadStillActiveException(...))) is more honest and greppable.

4. RecentPagesLoadGuard ticket can leak, and the leak grows unboundedly

RecentBrowserPagesManager.kt:

internal val initialLoad: Job = loadGuard.begin().let { ticket -> scope.launch { loadAsync(ticket) } }

and

internal suspend fun loadAsync(
    ticket: RecentPagesLoadGuard.Ticket = loadGuard.begin(),
    …
) = withContext(Dispatchers.IO) {
    try { … } catch (e: Exception) { … } finally { loadGuard.end(ticket) }
}

begin() happens outside the withContext block, but end() is inside it. If the launched job or the caller's job is already cancelled when withContext is entered, the block body never runs, the finally never executes, and the ticket stays in active permanently. Every subsequent loadGuard.remove(...) then appends another predicate to that dead ticket's removals list (RecentPagesLoadGuard.kt:21), which is never cleared except by clear() — unbounded list growth for the process lifetime, and any future load sharing that stale state is irrelevant only because the ticket is per-load. Fix: register the ticket inside the same try scope as the end, or use a try/finally around the launch in initialLoad.

5. Cheap-but-eager default argument does filesystem resolution on every load

historyFile: File = BossDirectories.resolve("browser-history.json") is evaluated on function entry, i.e. also on the common path where settingsFile.exists() and the history file is never touched. Minor, but it is a behavioural change from the previous lazily-resolved val browserHistoryFile = BossDirectories.resolve(...) inside the bootstrap branch, and resolve may create directories.

---

Uncertain observations

6. Removal predicates use raw URL equality while the manager has a canonical key notion

removePage:

loadGuard.remove({ it.url == url }) {
    _recentPages.update { pages -> pages.filter { it.url != url } }
}

The same file canonicalises elsewhere (promoKeys / canonicalUrlKey(it.url)). For the settings-file path the loaded URL is byte-identical to the recorded one, so exact match is fine; for the bootstrap path the rows come from browser-history.json and may differ only in canonical form (trailing slash, fragment, scheme case). If so, a dismissal during a bootstrap load will not suppress the loaded row and the dismissed page reappears — the exact bug this PR sets out to fix. Cannot confirm without canonicalUrlKey/mergeRecordedPages semantics.

7. The guard protects pages but not _dismissedSuggestions

clearAll() calls loadGuard.clear { _recentPages.value = emptyList() }, then mutates _dismissedSuggestions outside the lock, while loadAsync merges loadedDismissals into that flow with no ticket involvement. Correctness therefore rests entirely on mergeDismissedSuggestions being union-like. That is probably true today, but the asymmetry is undocumented and any future "undismiss" operation would race exactly the way removePage/clearAll used to race the page list.

8. Deleting a directory at destinationPath is now enshrined by test

failed file removal retains tracking and can be retried asserts assertFalse(directory.exists()) after the child file is removed — i.e. a "remove download" action deletes a directory if destinationPath points at one. File.delete() behaved the same way, so this is not a regression, but there is still no containment check that destinationPath lies under the downloads root before the unlink. Worth a look given the path originates from Chromium.

9. Possibly missing import

DownloadDataProviderImplTest.kt now uses assertNotNull (lines ~95, ~223, in the new tests). The import hunk only adds java.nio.file.Files, and the visible context stops at kotlin.test.assertFalse — the position where assertNotNull would sort. Worth confirming the import already exists; otherwise this does not compile.

10. Nested runBlocking inside cancel() in the new fakes

FakeDownloadEngine(emptySet(), onCancel = {
    runBlocking { manager.updateStatus("race", DownloadStatus.COMPLETED) }
})

cancel() is invoked from inside the provider's coroutine, so this blocks a dispatcher thread while awaiting updateStatus. If DownloadManager guards state with a Mutex and the provider context has limited parallelism, this is a deadlock candidate rather than a deterministic test. Cannot confirm without provider(...)'s context and updateStatus's implementation.

---

Test gaps

1. The stated motivation is untested. The comment "Register before launch: a Clear before the IO coroutine starts must also win" is the whole reason begin() was hoisted out of scope.launch, yet all four cases in RecentPagesDismissalDuringLoadTest gate on withTimeout(2_000) { readStarted.await() } — the dismissal always lands after the read has begun. The pre-start window has no coverage.
2. No unit tests for RecentPagesLoadGuard itself. No coverage for: remove after end (must be a no-op — AGENTS.md explicitly claims "not permanent URL tombstones"), clear followed by remove on the same ticket, two concurrent tickets where only one sees a removal, or predicate accumulation.
3. No test that tickets are released. Nothing asserts that a removal issued after a load completes does not filter a subsequent load — the property the new AGENTS.md paragraph promises.
4. retireVisits was rewritten to route through the guard (predicate extracted, loadGuard.remove(retire) { … }) with no new test; in particular nothing verifies the removed counter still matches the committed list now that the update runs under the guard lock.
5. Dismissal set not asserted. verifyOutcome checks only recentPages and the serialized file for URL substrings; dismissedSuggestions behaviour across the load race is unverified even though clearAll mutates it outside the guard.
6. Download side: no test for AccessDeniedException/permission failure (only DirectoryNotEmptyException), none for an invalid destinationPath (InvalidPathException), and none asserting what happens on the second rejected-cancel retry — which is where finding ⬆️ actions:(deps): Bump supabase/setup-cli from 2 to 3 #1 bites.

Test-hygiene risks in RecentPagesDismissalDuringLoadTest

• It mutates process-wide singleton state (manager.settingsFile) and restores private fields by reflection:

  RecentBrowserPagesManager::class.java.getDeclaredField(name).apply { isAccessible = true }

  A rename of _recentPages/_dismissedSuggestions fails at runtime with NoSuchFieldException inside the finally, leaving the singleton pointed at a deleted temp directory for every later test in the JVM. There is no compile-time coupling to catch this.

• The four tests each hold the singleton hostage across a paused IO read; any other test in the same JVM touching RecentBrowserPagesManager concurrently (or a maxParallelForks > 1 configuration) will interfere.

• withTimeout(2_000) { while (…) yield() } is a busy loop with a load-sensitive deadline — a plausible CI flake source.

---

Non-blocking notes

• clearAll(): Job — the returned Job is presumably ignored at UI call sites, so save failures remain silent; also note the asymmetry that clearAll captures val target = settingsFile while bootstrapFromBrowserHistory's saveImmediately() and scheduleSave() still read the mutable field late. That capture exists only to make the test's field-swap safe — a test-shaped seam in production code, as are the read/historyFile parameters.
• RecentPagesLoadGuard.end has an inferred Boolean return (active.remove(ticket)); an explicit Unit return would stop callers from reading meaning into it.
• AGENTS.md states "Keep the guard lock away from disk IO" — the diff does honour this (read(...) is invoked outside publish), and publish/remove/clear hold the monitor only across non-suspending StateFlow updates. Good.

[github-actions]

Claude diff review of 4f21b91 (no tests executed).

Review of PR #1477 @ 4f21b91 → dev

Diff-only review. I did not run anything; everything below is inferred from the diff text alone, and functions whose bodies are outside the diff (FileSystemUtils.cleanupPartialFile, saveImmediately, flushPendingSaves, canonicalUrlKey, mergeRecordedPages, DownloadManager.updateStatus, the runCatching wrapper in removeDownload) are treated as unverified. Line numbers are approximate, derived from the hunk headers.

---

A. DownloadDataProviderImpl.kt (hunk @@ -91,42 +92,27 @@)

A1. Path.of(...) introduces an unchecked InvalidPathException where File(...) never threw — confirmed behavior change

// ~line 95
Files.deleteIfExists(Path.of(download.destinationPath))

File(String) accepts essentially any string; Path.of throws InvalidPathException for paths containing a NUL byte, or reserved/invalid characters on Windows (:, ?, *, …), e.g. a destinationPath persisted by an older build, imported from another OS, or produced from a hostile Content-Disposition filename. Under the old code such an entry removed cleanly (exists() → false → warn-free proceed). Now the whole removeDownload fails, and since the tracking entry is only dropped after this block, the entry becomes permanently unremovable from the UI — there is no force/force-forget path anywhere in the diff. Same applies at the second call site (~line 112, settled.destinationPath).

A2. Any filesystem failure now blocks list removal, with no escape hatch — regression risk

The comment states the intent explicitly ("Other filesystem failures must retain the entry so the user can retry or reveal it"), so this is deliberate, but the practical consequences are worth weighing before merge:

• Windows AccessDeniedException when the finished file is open in another application (very common for PDFs/media) → the user cannot remove the row from their download list until they close the other app.
• Read-only/no-write-permission parent directory, vanished network share mount point, immutable flag → same.

Previously this was logger.warn(...) + removal (the deleted lines 94-98). Coupling "remove this row from my list" to "delete the file from disk succeeded" is a UX decision; if it stands, a "remove entry only" affordance or a bounded retry seems needed. Not covered by the new tests either way (the new tests only prove the entry is retained, never that a user can eventually get rid of it without fixing the filesystem).

A3. Diagnostics regression + check used for an expected runtime race — confirmed

// ~lines 105-108
val cancelled = engine.cancel(id)
val settled = downloadManager.getDownload(id)
check(cancelled || settled == null || settled.isTerminal) {
    "The download is still active. Could not cancel it; retry removal."
}

The deleted code logged both the benign race (debug, with id) and the genuinely odd case (warn, with id + status). Both are gone. The replacement throws IllegalStateException whose message carries no id and no status, so if the caller only propagates Result.failure, this failure mode is now unobservable in logs. Additionally, check is the Kotlin idiom for programmer error, and the message is written as end-user copy ("retry removal") — if that string is surfaced in UI, an IllegalStateException is the wrong carrier; if it is not surfaced, the user gets a silent failure. Recommend an explicit domain exception plus retained logger.warn with id/status.

A4. The check re-reads the manager synchronously after engine.cancel — uncertain, but likely to make removal flaky

engine.cancel is a boolean command; the manager's state is presumably updated by an asynchronous engine listener (that is exactly what the deleted comment described: "the download reached a terminal state between the lookup and the cancel"). If cancel is rejected because the engine already released the download but the listener event has not landed yet, settled.isTerminal is still false and the check fires — i.e. the old, explicitly documented benign race is now converted into a user-visible failure, and if the engine never emits a terminal event for a handle it already released, the entry is stuck forever (A1/A2 again). I cannot confirm the engine/listener ordering from this diff; please confirm, and consider a short re-read/await instead of a single immediate poll.

A5. Deletion contract is asymmetric between branches — uncertain

// ~lines 111-115
if (settled?.status == DownloadStatus.COMPLETED) {
    Files.deleteIfExists(Path.of(settled.destinationPath))   // failure aborts removal
} else {
    FileSystemUtils.cleanupPartialFile(download.destinationPath)  // failure presumably swallowed
}

The comment claims "the same deletion contract", but only the COMPLETED branch actually enforces it. If cleanupPartialFile is best-effort (its body is not in the diff, and the old code relied on it being tolerant of Chromium still holding the file), then a partial file that cannot be deleted still drops the tracking entry — the exact outcome the new COMPLETED contract forbids. Worth making explicit one way or the other.

---

B. RecentPagesLoadGuard.kt / RecentBrowserPagesManager.kt

B1. The guard's removal identity differs from the merge's identity — most likely real defect

// RecentBrowserPagesManager.kt ~line 479
loadGuard.remove({ it.url == url }) {
    _recentPages.update { pages -> pages.filter { it.url != url } }
}

// RecentPagesLoadGuard.kt ~line 40
loaded.filterNot { page -> ticket.removals.any { it(page) } }

The predicate is an exact string comparison on url, but the thing it is now filtering is the disk-loaded row set, and the merge it protects (mergeRecordedPages, plus promoKeys) is keyed by canonicalUrlKey. So if the persisted row's URL differs from the string passed to removePage by anything canonicalization normalizes (trailing slash, scheme/host case, fragment, www.), the dismissal filters the in-memory copy but not the loaded copy, and mergeRecordedPages re-introduces the dismissed page. That is precisely the bug this PR sets out to fix, surviving for non-identical URL spellings. The new tests use byte-identical URLs throughout (removed/retained literals in RecentPagesDismissalDuringLoadTest.kt ~lines 34-40), so this case is untested. Confirming canonicalUrlKey's aggressiveness would settle whether this is a defect or a non-issue; as written the guard and the merge disagree on page identity.

B2. begin() outside the coroutine has no cancellation-time release — leak hazard

// RecentBrowserPagesManager.kt ~line 242
internal val initialLoad: Job = loadGuard.begin().let { ticket -> scope.launch { loadAsync(ticket) } }

end(ticket) only runs in loadAsync's finally. If the launched coroutine never enters its body (scope already cancelled, dispatcher shut down, or the job cancelled before first dispatch), the ticket stays in active permanently. remove() then appends a lambda to that zombie ticket's removals on every removePage/retireVisits for the process lifetime — an unbounded list of closures, each capturing a URL/cutoff. Registering before launch is the right call for correctness (the comment is accurate), but it needs initialLoad.invokeOnCompletion { loadGuard.end(ticket) } (or equivalent) to make release unconditional.

B3. mutation/merge run while holding the monitor — uncertain, low probability

remove, clear and publish (RecentPagesLoadGuard.kt ~lines 17-43) all invoke the caller-supplied lambda inside synchronized(lock), and every one of those lambdas writes a MutableStateFlow. Setting a StateFlow value can resume collectors; a collector running on an immediate/unconfined dispatcher that re-enters removePage/clearAll on the same thread is fine (monitors are reentrant), but a collector on another thread that blocks waiting for work needing this lock would deadlock. The AGENTS.md claim ("no filesystem work holds this lock") is satisfied by the diff, but publish does hold the lock across updateAndGet, which retries under contention. Cheap mitigation: compute surviving under the lock, run merge outside it.

B4. Cleared bootstrap still writes and logs a wrong count — confirmed, minor

// RecentBrowserPagesManager.kt ~lines 325-333
if (recentPages.isNotEmpty()) {
    loadGuard.publish(ticket, recentPages) { surviving -> _recentPages.update { ... } }
    saveImmediately()
    logger.debug(..., "Bootstrapped pages from browser history", mapOf("count" to recentPages.size))
}

The emptiness check is on the unfiltered list. When the ticket was cleared (or every entry was removed), publish merges nothing, yet the code still performs an immediate disk write and logs count = <pre-filter size> — a misleading log line and a needless write on the "Clear during startup" path. Gate on the publish result instead.

B5. clearAll captures the target file while other save paths presumably do not — minor, test-only today

// ~lines 573-575
val target = settingsFile
return scope.launch { saveImmediately(target) }

scheduleSave()/bootstrap's saveImmediately() (no argument) appear to read the settingsFile field at save time. With settingsFile being a mutable var that the new test reassigns, two in-flight saves can target two different files. Harmless in production (the field never changes), but it makes the test's ordering assumptions load-bearing. I could not verify the saveImmediately(File) overload exists — its signature is not in the diff.

B6. Dismissal set is still unguarded

_dismissedSuggestions is merged/updated outside the ticket mechanism (loadAsync ~line 272, clearAll ~line 571). This appears safe because the operation is union-only/monotonic, but it means a dismissal recorded during load is not covered by the new invariant, and nothing in the new tests asserts it survives (see C2).

---

C. Test gaps

1. No unit test for RecentPagesLoadGuard itself. All coverage is end-to-end through the singleton. Untested guard behaviors that the implementation deliberately encodes: remove skipping already-cleared tickets (active.filterNot { it.cleared }, ~line 20); clear wiping accumulated removals (~line 28); two concurrent tickets with different removal sets; end making the ticket inert (the AGENTS.md "not permanent tombstones" claim — nothing asserts a subsequent load re-surfaces a page removed during an earlier load).
2. Dismissal persistence during load is unasserted. RecentPagesDismissalDuringLoadTest.verifyOutcome (~lines 84-98) checks recentPages and the pages payload only; it never inspects dismissedSuggestions, so a regression that drops the dismissal record (letting the page return as padding) would pass.
3. URL canonicalization variants are untested (see B1).
4. Download side: no test for a cancel that is accepted while the download stays non-terminal (the cancelled == true + settled.isTerminal == false path, which skips the check and deletes the partial), and no test for an undeletable/invalid destinationPath shaped like A1.
5. Test-isolation hazard (uncertain): RecentPagesDismissalDuringLoadTest joins initialLoad (which reads the real user file), reassigns the shared settingsFile, and restores private MutableStateFlows by reflection (~lines 101-108). Any other test class in the same JVM touching RecentBrowserPagesManager, or a parallel runner, will interfere; the four cases also depend on flushPendingSaves() observing a save that recordPageVisit may schedule from a separate coroutine (~lines 61-66), which looks order-sensitive.
6. Nested runBlocking inside FakeDownloadEngine.onCancel (DownloadDataProviderImplTest.kt ~lines 255, 275, and the new onCancel hook ~line 341) blocks the dispatcher thread from inside engine.cancel, which is itself called from inside a coroutine. If DownloadManager.updateStatus ever needs that thread (or a mutex held by the outer coroutine), this deadlocks rather than fails. A synchronous test double or an unconfined test scheduler would be safer.
7. already missing completed file can be removed repeatedly (~lines 233-244) does not test what its name says: the second removeDownload takes the unknown-id path because the entry was already dropped, so only the first call exercises deleteIfExists on a missing file.

---

D. Documentation

AGENTS.md (+2485-2493) accurately describes the mechanism, with one caveat: "Release tickets after loading" is only true when the load coroutine actually runs (see B2), and "Clear and removal predicates update that ticket alongside the in-memory mutation" is literally true but the identity used for those predicates differs from the merge's (see B1).

---

Suggested blocking items

• B1 (predicate vs. canonical-key identity) — confirm or fix; it undermines the PR's stated goal.
• A1 (InvalidPathException → permanently unremovable entry) and A2/A4 (no escape hatch when deletion/cancel keeps failing).
• B2 (unconditional ticket release).

Nice-to-have before merge

A3 (restore id/status logging), A5 (make the two branches' contracts consistent), B3 (don't hold the monitor across flow mutation), B4, and test gaps C1–C3.

[github-actions]

Claude diff review of 5dd4c9d (no tests executed).

Review of PR #1477 @ 5dd4c9d

Scope note: this is a diff-only review. I could not inspect removePage's full body, scheduleSave/saveImmediately/flushPendingSaves signatures, FileSystemUtils.cleanupPartialFile, or the runCatching/Result wrapper in removeDownload; several findings below are conditional on those. I did not run or verify any tests.

---

Confirmed defects / behavior regressions

1. Files.deleteIfExists(Path.of(...)) converts unrelated failures into a permanently unremovable entry

composeApp/src/desktopMain/.../DownloadDataProviderImpl.kt (new ~L95‑97, and the race branch ~L112‑114)

Files.deleteIfExists(Path.of(download.destinationPath))

Two distinct behavior changes here, only one of which is documented by the new comment ("Other filesystem failures must retain the entry"):

• Path.of throws, not just delete. Path.of(String) throws InvalidPathException (unchecked) for malformed paths (NUL bytes, illegal characters on Windows). The old code (File(download.destinationPath)) never threw for a bad path string. Because the new code is inside whatever runCatching produces the Result the tests assert on, a download row with a malformed destinationPath is now permanently unremovable — every retry re-throws. That is a hard regression with no user escape hatch.
• No force-remove path. Previously a failed delete logged a warning and still dropped the tracking entry. Now any IOException (file open/locked on Windows, read-only volume, DirectoryNotEmptyException, AccessDeniedException) blocks list removal forever. The new test only covers the case where the user can make the delete succeed (failed file removal retains tracking and can be retried, DownloadDataProviderImplTest.kt new L211‑232). There is no "remove from list only" affordance in the diff, so a user whose file is locked by another process is stuck with the row. If "retain on failure" is the intended product contract, it needs an escape hatch and/or a distinction between transient and permanent failures.

2. Diagnostics were deleted, not replaced

Same file, removed L104‑120: the debug("Download settled before the cancel reached the engine") and warn("Engine did not accept cancel…", id, status) calls are gone, replaced by

check(cancelled || settled == null || settled.isTerminal) {
    "The download is still active. Could not cancel it; retry removal."
}

The warn was the only signal that the engine is rejecting cancels; now the failure is an IllegalStateException message with no id/status context and (as far as the diff shows) no log line. Recommend logging with id/status before throwing. Also note check is being used for an expected race outcome rather than a programmer-error assertion, and the message is phrased as user-facing copy — if any caller surfaces Result.exceptionOrNull()?.message this is UI text buried in an assertion.

3. Inconsistent deletion contract between the completed and the partial path

Same file, new L113‑116: when cancellation succeeds, FileSystemUtils.cleanupPartialFile(...) runs and the entry is removed regardless of whether the partial bytes were actually deleted (best-effort, per the existing util), while a completed file's delete failure now aborts removal. So "remove" can still silently leave untracked partial garbage on disk while refusing to remove a row whose final file is merely locked. Worth deciding on one contract; at minimum document the asymmetry.

4. Guard ticket can leak, and leaked tickets accumulate predicates without bound

RecentBrowserPagesManager.kt new L240‑241:

internal val initialLoad: Job = loadGuard.begin().let { ticket -> scope.launch { loadAsync(ticket) } }

begin() runs outside the coroutine; end(ticket) runs only in loadAsync's finally (new ~L288‑290). If the launched coroutine is cancelled before its body starts (scope cancellation at shutdown, or scope.launch on a cancelled/replaced scope), loadAsync never executes and the ticket stays in active forever. Every subsequent removePage/retireVisit then does active.forEach { it.removals.add(predicate) } (RecentPagesLoadGuard.kt L21), so Ticket.removals grows unboundedly for the process lifetime — a slow leak plus O(n) cost on each dismissal. Fix: scope.launch { ... }.also { it.invokeOnCompletion { loadGuard.end(ticket) } }, or move begin() inside the coroutine and accept the (now-closed) ordering concern differently.

5. RecentPagesLoadGuard runs caller-supplied mutations while holding the lock — the documented invariant is only convention

RecentPagesLoadGuard.kt L17‑43: remove, clear and publish all invoke their mutation/merge lambda inside synchronized(lock). Today those lambdas are MutableStateFlow writes, but MutableStateFlow.value = / update can resume collector continuations inline (unconfined, or Main.immediate when called from the UI thread). That means arbitrary collector code (recomposition, listeners) can execute under this lock. AGENTS.md (new L2485‑2492) asserts "Keep the guard lock away from disk IO" as an invariant, but nothing in the type enforces it — the lock is held across a caller-provided lambda. This is a latent deadlock/latency hazard rather than a demonstrated bug, but the invariant should be enforced (e.g. compute under lock, mutate outside) or the risk documented at the lambda parameters.

---

Uncertain observations (need local verification)

6. Possible compile breaks from signatures not shown in the diff

• RecentPagesDismissalDuringLoadTest.kt L72 calls manager.removePage(removed).join(). The diff never changes removePage's return type (only its body, ~L476‑481) and its own comment says the mutation is "applied on the caller's thread, not inside scope.launch". If removePage still returns Unit, this test does not compile. Only clearAll() was changed to return Job (L565, L574). Please confirm removePage returns a Job/awaitable.
• DownloadDataProviderImplTest.kt newly uses assertNotNull (modified L95, new L221, etc.) but the diff adds only import java.nio.file.Files. Confirm kotlin.test.assertNotNull is already imported.
• saveImmediately(target) (L575) and flushPendingSaves() (test L38) must exist with those shapes; neither is in the diff.

7. clearAll() mutates _dismissedSuggestions outside the guard lock

RecentBrowserPagesManager.kt L566‑573: loadGuard.clear { pages = empty } holds the lock, then _dismissedSuggestions.update { it + promoKeys } runs unlocked. An in-flight load's dismissal merge (L272‑276) is not guarded at all. This appears safe because both sides are union-only (mergeDismissedSuggestions), so nothing is lost — but that safety is implicit and undocumented, and it depends on mergeDismissedSuggestions semantics I can't see. Worth a comment stating the union invariant, since a future non-monotonic change there would silently reintroduce the "Clear doesn't stick" bug for promo cards.

8. Production API widened for test seams

loadAsync(ticket = …, read: suspend (File) -> String = { it.readText() }, historyFile: File = BossDirectories.resolve("browser-history.json")) (L252‑256). Injecting a read function and a history path into a production singleton is workable but means production now has a code path (ticket default loadGuard.begin()) that only tests exercise, while the real wiring passes an explicit ticket. Low risk, but the two entry shapes diverge, so a bug in one is not covered by tests of the other (see gap 12).

9. Double publication on the error path

L280‑285: on decode failure after publish has already run, bootstrapFromBrowserHistory(ticket, …) is called with the same ticket and publishes again. Re-filtering is idempotent and the merge is union-like, so I believe this is benign — but two publish calls per ticket is an unstated assumption of RecentPagesLoadGuard.publish, which reads mutable ticket state each time. Worth asserting single-use or documenting re-entrancy.

10. Race window for settled == null + successful cancel

New L110‑116: when settled == null (engine listener already removed the entry) the code falls into the else branch and calls cleanupPartialFile(download.destinationPath). If the download actually completed in that window, the entry is gone and we call the partial-cleanup helper on a final file path. This is pre-existing behavior (the old code called cleanupPartialFile unconditionally), so not a regression — but the new settled?.status == COMPLETED branch was added precisely to handle "completion wins the race", and it does not cover the settled == null variant of the same race.

---

Test gaps

11. No direct unit tests for RecentPagesLoadGuard (RecentPagesLoadGuard.kt, 44 new lines, all new logic). Untested branches: remove skipping already-cleared tickets (L21), clear wiping removals (L28‑31), multiple concurrent tickets, end removing a ticket so later dismissals are not recorded, and publish with a predicate that matches nothing. All four scenarios currently go through the singleton integration test, which cannot deterministically exercise multi-ticket interleavings.

12. The production wiring is not asserted. The stated reason for loadGuard.begin().let { scope.launch { … } } (L240‑241) is "a Clear before the IO coroutine starts must also win." No test covers that ordering — the tests always call loadAsync with the default ticket after the load coroutine has already begun (RecentPagesDismissalDuringLoadTest.kt L57‑66 waits on readStarted before dismissing). The regression this PR is named for is therefore only covered for the mid-read window, not the pre-start window.

13. Ticket release is not asserted. No test checks that active is empty after loadAsync completes/throws/is cancelled (i.e. the leak in finding 4 would not be caught).

14. DownloadDataProviderImpl: no test for a malformed destinationPath (InvalidPathException, finding 1), and no test for a delete failure that the user cannot resolve, which is the case that produces a stuck row. The settled == null race (finding 10) is also untested; settled cancellation can remove tracking even after engine releases handle (new L269‑282) drives CANCELLED, not entry-removal-to-null.

15. Global-state hazard in the new test (RecentPagesDismissalDuringLoadTest.kt): it mutates the production singleton's settingsFile (L46) and restores two private MutableStateFlows by reflection (L96‑105). Concerns:

    • If any assertion or manager.clearAll().join() inside the outer finally (L84‑90) throws, manager.settingsFile = original (L89) never runs and every later test/production write in the JVM targets a deleted temp dir. Restore settingsFile first in the finally.
    • Reflection on "_recentPages" / "_dismissedSuggestions" string literals breaks silently on rename (runtime NoSuchFieldException, not a compile error).
    • Four tests in this class serially mutate one singleton; any other test in the same JVM touching RecentBrowserPagesManager concurrently will make file.readText() assertions (L88‑89 of verifyOutcome) flaky.
    • withTimeout(2_000) { while (…) yield() } (L74‑76) busy-spins the runBlocking dispatcher waiting for recordPageVisit to land on another dispatcher — a real wall-clock dependency that will be the first thing to flake on loaded CI.

---

Suggested minimum before merge

1. Guard Path.of (or keep File) so a malformed destinationPath cannot make a row permanently unremovable; decide and document the escape hatch for unresolvable delete failures (finding 1).
2. Restore a log line with id/status on the rejected-cancel path (finding 2).
3. Release the guard ticket via invokeOnCompletion so a cancelled startup coroutine cannot leak it (finding 4).
4. Add direct RecentPagesLoadGuard unit tests and an assertion that active drains (gaps 11, 13).
5. Reorder the test finally to restore settingsFile before anything that can throw (gap 15).