Skip to content

fix(link-preview): keep composer fetches user-paced - #7211

Merged
tellaho merged 11 commits into
mainfrom
tho/link-preview-fetch-timeouts
Sep 8, 2026
Merged

fix(link-preview): keep composer fetches user-paced#7211
tellaho merged 11 commits into
mainfrom
tho/link-preview-fetch-timeouts

Conversation

@tellaho

@tellaho tellaho commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Category: fix
User Impact: Link previews can keep loading while a message is being composed, while sending still has a finite escape hatch and stalled network transports cannot occupy preview slots forever.

Problem: Native metadata and image deadlines could collapse slow previews into fallback cards while the user was still composing, and a shared image-host cooldown made pasted batches fail inconsistently after one rate limit. Solution: Keep preview resolution user-paced with no aggregate request deadline, bound transport inactivity (15s DNS/connect, 30s idle read), serialize image requests by host, and allow at most one server-directed cooldown wait of up to 30s across an image fetch and its redirects. The existing bounded post-Send preparation and immediate Skip paths remain unchanged.

File changes

desktop/src-tauri/src/commands/link_preview.rs
Removes aggregate native deadlines so composer metadata work can complete at the user's pace, while retaining DNS/connect/idle-read liveness bounds. Adds bounded host-paced image request coordination that releases its gate during cooldown, waits inline at most once for at most 30 seconds, and cannot renew that wait through redirects or the outer transient retry. Same-host image and favicon requests remain deliberately serialized to align with host rate limits.

desktop/src-tauri/src/commands/link_preview_rate_limit.rs
Adds a fixed-size striped host gate so concurrent image requests are serialized without retaining an unbounded attacker-controlled hostname map.

desktop/src-tauri/src/commands/link_preview_tests.rs
Moves native link-preview tests into a dedicated module and covers the user-paced metadata contract, bounded one-shot cooldown behavior, and gate release while a rate-limited request sleeps—including a different host sharing the same bounded gate stripe.

desktop/src-tauri/src/commands/link_preview_youtube.rs
Removes the thumbnail fetch deadline so YouTube previews follow the same composer lifecycle contract while using the shared bounded transport.

desktop/src/shared/lib/useResolvedLinkPreviews.ts
Adds development-only metadata outcome diagnostics with elapsed time and image/fallback state, without logging encoded image payloads.

Reproduction steps

  1. Open the desktop composer and paste several GitHub pull request links whose OpenGraph images share a host.
  2. Observe that image requests are paced by host instead of racing, and slow-but-progressing preview work remains pending rather than immediately becoming a completed favicon fallback.
  3. Send while preview work is still pending and confirm Preparing link preview remains bounded by the existing post-Send budget.
  4. Use Skip during preparation and confirm the message proceeds immediately.
  5. In a development build, inspect the console for [link-preview] metadata fetch completed diagnostics containing elapsed time and image state without base64 payloads.

Related issue

N/A — scoped from the linked Buzz implementation room.

Testing

At current head dfb394aafbee537e9ffb04ad3732d08f65f30b8e:

  • Production-bound paused-time metadata regression passed through fetch_link_preview_metadata; restoring the former 10-second aggregate wrapper makes it fail at the pending assertion.
  • Native link-preview module: 19/19 passed.
  • cargo check --manifest-path desktop/src-tauri/Cargo.toml passed.
  • Rust formatting and git diff --check passed.
  • Pre-push push-head-scope, org safety, differential file-size, branch-skew, and desktop-tauri-checks hooks passed.

At prior head 59e2dcf167b15c7a3e637ad2608008b7f9cef5f3:

  • Full Tauri Rust suite: 3,056 passed, 19 ignored; integration crates 7 + 3 passed.
  • Focused native link-preview suite: 26/26 passed.
  • The pasted multi-preview workflow was exercised in the desktop app and confirmed improved before draft publication.

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown

🔐 Codex Security Review

Status: review required for the current range.

The current range is 44316ff72f5f7de014c66b01cbf534298a70c249...274a2a3ad43a3e9811cafb078ba26f55cdc5c1e7.
A new review must complete for this exact range. When manual authorization
is required, a Block organization member must comment exactly
@buzz-security-review 274a2a3ad43a3e9811cafb078ba26f55cdc5c1e7 to authorize a new review.
Any previous review applies only to its recorded range.

@tellaho
tellaho marked this pull request as ready for review September 1, 2026 23:09
@tellaho
tellaho requested a review from a team as a code owner September 1, 2026 23:09

@jedwards27 jedwards27 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:bot: Jude’s code review agent

Requesting changes at exact head dfb394aafbee537e9ffb04ad3732d08f65f30b8e.

Blocking — prevent stalled fetches from permanently starving unrelated previews. createTaskScheduler has two global slots and releases a slot only when its task settles (desktop/src/shared/lib/useResolvedLinkPreviews.ts:110-134,175-189). This PR removes the native aggregate deadline (desktop/src-tauri/src/commands/link_preview.rs:59-77), while reqwest's 30-second read_timeout is an inactivity timeout (:228-245): a server can continuously drip chunks below that interval, and the bounded readers still take arbitrarily long to reach their byte limit (:261-287). Two such requests can therefore occupy both global slots indefinitely, preventing every later URL from starting. Removing a URL only suppresses the React continuation (useResolvedLinkPreviews.ts:651-669), and Skip completes send preparation without aborting the underlying shared job (desktop/src/features/messages/lib/linkPreviewPreparationStore.ts:302-318,321-329), so neither action restores capacity.

Author action: preserve user-paced preview behavior while ensuring abandoned or trickling native requests cannot monopolize global admission—for example, cancellable in-flight work tied to live consumers, or another bounded forward-progress policy. Add a deterministic regression proving that after two never-settling/trickling requests are removed or skipped, a third ordinary URL starts and resolves.

Both assigned review lanes independently confirmed this blocker and found no additional material defects. Exact-head CI is green across unit, desktop core/build, smoke/integration, security, and platform jobs. Local focused Rust validation could not pass Tauri build setup because desktop/src-tauri/binaries/buzz-acp-aarch64-apple-darwin is absent; that is a reviewer-tooling confidence gap, not requested author work. The latest Codex Security Review authorized successfully but skipped the actual review job, so this verdict relies on source tracing, lane agreement, and the completed CI suite.

@jedwards27 jedwards27 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:bot: Jude’s code review agent

Verdict: REQUEST CHANGES

Reviewed PR base 5aed49b505a7e27f3b0e34dafa53d6c4e8cdcd64 at exact live head dfb394aafbee537e9ffb04ad3732d08f65f30b8e.

High — two slow responses can permanently starve every later preview

createTaskScheduler has two process-global admissions and releases one only when the scheduled promise settles (desktop/src/shared/lib/useResolvedLinkPreviews.ts:110-134,137-190). This PR removes the native aggregate deadline (desktop/src-tauri/src/commands/link_preview.rs:59-77), while reqwest's 30-second read_timeout is an inactivity timeout, not an overall or forward-progress deadline (desktop/src-tauri/src/commands/link_preview.rs:219-245). Both response readers permit an unlimited sequence of chunks arriving within that idle interval (desktop/src-tauri/src/commands/link_preview.rs:261-287). Consequently, two servers sending a byte every <30 seconds can retain both renderer slots for an extremely long time (up to the byte caps), and every unrelated preview remains queued.

Removing those URLs from the composer only cancels the React continuation or an as-yet-undispatched callback; it does not cancel an already-started Tauri invocation (desktop/src/shared/lib/useResolvedLinkPreviews.ts:651-669). Likewise, post-Send Skip resolves the send task but leaves the shared preview job running (desktop/src/features/messages/lib/linkPreviewPreparationStore.ts:291-318,321-329). A user can therefore paste two hostile or merely broken links once, remove or Skip them, and still lose subsequent previews until the requests eventually complete or the app restarts. The former aggregate wrapper bounded this global-slot occupancy, so this is introduced by the PR rather than merely exposed by it.

Author action: preserve user-paced composing without allowing abandoned or trickle-progress requests to monopolize global admission. Make in-flight native work cancellable when it has no live composer/send consumer and release scheduler capacity on cancellation, or introduce another bounded forward-progress/stage policy that prevents indefinite slot ownership. Add a production-seam regression that starts two never-settling or progress-drip fetches, removes/Skips them, and proves a third ordinary URL starts and resolves.

Verification owner: author for the biting regression; reviewer will inspect/mutation-check the fix and rerun at the new exact head.

Review evidence and residual risk

  • Two independent review lanes and integration tracing reached the same starvation result. No other material systems, cooldown, redirect, SSRF, diagnostics-privacy, or UI-state defect was found in the changed paths.
  • Current-head CI is green, including Rust lint, unit tests, Desktop Core/Desktop, desktop E2E, security, Windows, and macOS build lanes. Green CI does not exercise the starvation sequence above.
  • git diff --check passed with a clean worktree at the pinned head.
  • Focused local Tauri Rust execution could not start because the checkout lacks desktop/src-tauri/binaries/buzz-acp-aarch64-apple-darwin; this is a reviewer-environment confidence gap, not author action. CI and the author's reported native results provide supporting evidence, but they do not cover the blocking global-admission regression.

@tellaho

tellaho commented Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

AI-generated update: The blocker was abandoned/trickling metadata work retaining both global fetch slots after composer removal or promoted-send Skip. Commit c801200 adds shared consumer ownership and renderer-to-native cancellation, preserves a short re-entry cache window, immediately reclaims orphaned slots when new demand arrives, and aborts unobserved promoted preparation on Skip/timeout/cancel. Validation at c801200: full Desktop JS suite 5,885/5,885; full Tauri workspace suite 3,060 passed, 19 ignored; TypeScript typecheck; scoped Biome check; Tauri rustfmt; pre-push desktop and Tauri gates.

@jedwards27 jedwards27 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:bot: Jude’s code review agent — request changes on c8012002caf58c5761484cde123b35ef4b017820.

The new head fixes the original global-slot starvation path, but introduces a shared-ownership bug in cancellation.

Blocking defect

P1 — a single consumer can release the shared metadata lease twice and cancel another live owner’s fetch.

Every coalesced load() increments PendingMetadataLoad.consumers, but each returned cancel closure directly invokes release() without a per-lease once fence (desktop/src/shared/lib/useResolvedLinkPreviews.ts:216-230,249-256,306-309). The send-preparation path invokes that same closure twice on abort: once synchronously from its abort listener and again from finally (desktop/src/features/messages/lib/linkPreviewPreparationStore.ts:118-129).

With a composer and promoted Send sharing one pending URL (consumers === 2), Skip/timeout/cancel can therefore decrement the count twice to zero. That schedules orphan cancellation and aborts the native fetch after one second even though the composer still owns and displays the URL. The remaining composer can stay loading because its existing effect has not acquired a replacement lease. Cancellation has, rather inconveniently, killed the wrong owner.

Author action: make each returned consumer lease idempotent (for example, a closure-local released flag/once wrapper). Add a production-path regression where two consumers share one pending fetch, the preparation consumer is cancelled through its actual abort-listener + finally path, and the other consumer remains live and resolves; only its final single release should abort the request.

Verification owner: author for the regression and patch; reviewer for exact-head mutation/re-review.

Reconciled evidence

Both lanes agree the previous starvation blocker is addressed: orphaned native work is cancelled, new demand immediately reclaims admission, re-entry has a bounded grace window, request IDs fence native cancellation, and the third-request production-seam regression bites when orphan reclamation is removed. The product/UI lane found no separate defect and mutation-checked that forward-progress regression.

The systems lane found the non-idempotent lease defect above, ran the exact-head full Desktop JS suite (5,885/5,885) and typecheck successfully, and confirmed that mutating the coalesced consumer cancellation leg to a no-op leaves the suite green—current tests do not bind this ownership invariant. I independently traced the double invocation and shared counter transition and agree it is author-actionable.

Confidence gaps (not additional blockers): reviewer-owned Tauri/native GUI execution was not completed in this shortened re-review. CI builds, lint, unit, security, and integration jobs checked so far are passing; remaining smoke/core jobs must reach their required terminal state independently.

@jedwards27 jedwards27 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: REQUEST CHANGES
Reviewed: 5aed49b505a7e27f3b0e34dafa53d6c4e8cdcd64..c8012002caf58c5761484cde123b35ef4b017820 (exact head c8012002caf58c5761484cde123b35ef4b017820)
Risk: high — shared renderer ownership now controls cancellation of long-lived Tauri/native network work and global scheduler capacity.

The original global-slot starvation path is addressed: orphaned loads can be reclaimed on new demand, abort propagates through a request-specific native cancellation path, and the hook-level regression fails when production orphan reclamation is removed. However, the replacement ownership contract has a concrete double-release bug.

Blocking finding — consumer cancellation is not idempotent. Every coalesced load() increments pending.consumers, but its returned cancel closure calls release() on every invocation (desktop/src/shared/lib/useResolvedLinkPreviews.ts:216-230,249-256,306-309). The preparation path invokes the same lease twice when its signal aborts: once in the abort listener and again unconditionally in finally (desktop/src/features/messages/lib/linkPreviewPreparationStore.ts:118-129). With a composer and promoted Send sharing one pending URL (two consumers), Skip/timeout/cancel can therefore decrement the count twice to zero, orphan the load, and abort it after the grace period even though the composer still owns it. The still-present composer can remain loading because no dependency change retriggers its effect.

Author action: make each returned consumer lease idempotent (for example, a closure-local released/once guard). Add a production-seam regression with two consumers on one pending fetch: abort the preparation consumer through its actual listener-plus-finally path, prove the other consumer remains live and resolves, then prove its final single release permits abort.

Validation: exact-head Desktop JS suite passed 5,885/5,885 and typecheck passed in independent review. A mutation replacing the returned coalesced consumer cancellation with a no-op left all 5,885 tests green, demonstrating that the current suite does not bind this ownership leg. Separate mutation evidence confirms the new third-request/orphan-reclamation regression bites its production call site. Source tracing found request-ID native cancellation, stale-settlement identity fencing, and the remaining native cancellation path coherent.

Verification owner: author for the regression and fix; reviewer for exact-head delta and mutation check.

Confidence gaps: independent Tauri package execution and a native GUI journey were not completed in the re-review window. Those are reviewer verification gaps, not additional author-actionable findings. Exact-head CI was still completing Desktop Core and two smoke shards at review time; completed unit, Rust lint, security, Windows/macOS builds, integration E2E, relay E2E, and two smoke shards were green.

— :bot: Jude’s code review agent

@tellaho

tellaho commented Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

AI-generated update: The blocker was a preparation consumer releasing its shared metadata lease in both the abort listener and finally, which could decrement shared ownership twice and cancel a composer-owned fetch. Commit 2f14491 makes every metadata lease release idempotent and lets aborted preparation unwind promptly without waiting on another live owner’s fetch. The production-seam regression exercises the actual preparation abort-listener + finally path, proves the composer lease stays live and resolves, and proves the final owner release permits native cancellation; removing the once-fence makes that regression fail. Validation at 2f14491: Desktop typecheck passed; full Desktop JS suite passed 5,886/5,886; scoped Biome and git diff --check passed; push hooks passed Desktop check, typecheck, and tests.

@jedwards27 jedwards27 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:bot: Jude’s code review agent

Verdict: APPROVE
Reviewed: 5aed49b505a7e27f3b0e34dafa53d6c4e8cdcd64..2f14491504690babd29fe383e8e53f4f5ac0f95e (exact head 2f14491504690babd29fe383e8e53f4f5ac0f95e)
Risk: high — shared renderer/native cancellation and preview-forward-progress lifecycle.

Behavior/contracts traced: composer and promoted-Send coalescing, consumer lease accounting, abort-listener plus finally duplicate release, orphan reclamation, native request cancellation identity, scheduler admission, stale settlement/cache fencing, Skip/timeout/remove/re-add behavior.

Findings: no unresolved author-actionable defect. The prior blocker is fixed: every metadata lease now has a closure-local once fence, so duplicate cancellation of the preparation lease cannot consume the composer’s lease. Preparation also races explicit abort settlement, allowing Skip/reset to complete while a legitimately shared native request remains alive.

Author action: none.
Verification owner: reviewer/tooling for remaining native GUI observation; exact-head CI/release gates own their outstanding jobs.

Validation at matching clean head: focused production-seam tests passed 29/29; full Desktop JS package passed 5,886/5,886; pnpm typecheck passed; git diff --check passed. A mutation removing the once fence made the new shared composer/preparation regression fail at the expected native-cancellation assertion, then restoration returned the worktree clean at the exact head.

Manual/native evidence: no native GUI journey was run. This is a confidence gap, not a discovered defect.

Residual risk: several exact-head CI jobs were still running at review time; completed Rust lint, security, Windows build, relay/backend/Postgres E2E checks were green. Codex Security authorized but its review job was skipped. Merge readiness remains with the named CI gates.

@wesbillman wesbillman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Carl, an automated reviewer, commenting via Wes’s GitHub account.

CHANGES REQUESTED at 2f14491504690babd29fe383e8e53f4f5ac0f95e (base 5aed49b505a7e27f3b0e34dafa53d6c4e8cdcd64). One blocking Send regression; one low-severity, non-blocking cooldown issue. The prior abandoned-fetch starvation and double-release findings are addressed in the reviewed paths.

P1: Keep the post-Send deadline and Skip when replacing an expired settled job

linkPreviewPreparationStore.ts:289-299 selects the fast path from the old job's settled flag, then calls prepareLinkPreview, which can replace that job after its five-minute TTL. The replacement's fresh metadata promise is then awaited without a task, timeout, or functional Skip.

A production trigger needs no remove/re-add: let a URL return transient image failure with Retry-After: 900, finish its fallback snapshot, and keep the URL in the composer. At the retry boundary, the metadata hook starts another fetch, while the composer retains its ready tag and skips renewing the old preparation (useComposerLinkPreviews.tsx:434-475). Press Send during that pending retry. MessageComposer.tsx:637-649 promotes the raw live candidate; the store chooses the settled fast path, replaces the expired job, and joins the pending fetch. resolvePreviewTags awaits it before message publication. The preparation owns a live lease, so clearing the composer does not cancel it as an orphan.

The stale fast-path predicate existed at base, but removing the native 10-second aggregate timeout in this PR newly removes the fallback bound on that metadata wait. A drip-fed response that stays within the idle-read timeout can now hold Send far beyond the promised 10-second budget with no preview Skip affordance.

Fix: decide whether the adopted work is already settled after job adoption/replacement, or use the existing finite task/Skip machinery for every promotion. Do not restore a composer-wide aggregate deadline. Add a production-bound regression that completes a transient fallback, advances beyond the job TTL into a pending metadata retry with the URL retained, then proves both deadline and Skip settle Send. The existing TTL and recent-settled tests do not cover this transition.

P3, non-blocking: A transport retry can renew the one-shot cooldown wait

link_preview.rs:466-471 returns retry_inline: true for transport errors even after consuming a cooldown wait. The outer retry helper creates a fresh invocation, resetting waited_for_cooldown. Thus 429 + 20s wait -> connection failure -> outer retry -> 429 + 20s wait consumes two waits rather than the promised one. Mirror the HTTP-status branch's !waited_for_cooldown guard and cover the outer retry boundary. This remains bounded to two invocations and cancellable, so it is not independently blocking.

Validation: exact-head/base source and diff review, with independent frontend and native lanes integrated. No PR code, tests, builds, or GUI workflow executed. Existing reviewer-reported test success addresses the prior lease bug, not the expired-settled promotion above. Review covered composer ownership, native cancellation, Send/Skip/reset, snapshot consumers, transport/SSRF and host pacing; wire formats and mobile/browser/CLI code are unchanged in this diff.

tellaho and others added 11 commits September 8, 2026 11:06
Co-authored-by: Carl <acda9e433d19dcd0e6b6840f7f4b98f3a56f1fab98049d444c087019e6d36560@buzz.block.builderlab.xyz>
Signed-off-by: Taylor Ho <taylorkmho@gmail.com>
Co-authored-by: Carl <acda9e433d19dcd0e6b6840f7f4b98f3a56f1fab98049d444c087019e6d36560@buzz.block.builderlab.xyz>
Signed-off-by: Taylor Ho <taylorkmho@gmail.com>
Co-authored-by: Carl <acda9e433d19dcd0e6b6840f7f4b98f3a56f1fab98049d444c087019e6d36560@users.noreply.github.com>
Signed-off-by: Taylor Ho <taylorkmho@gmail.com>
Co-authored-by: Carl <acda9e433d19dcd0e6b6840f7f4b98f3a56f1fab98049d444c087019e6d36560@buzz.block.builderlab.xyz>
Signed-off-by: Taylor Ho <taylorkmho@gmail.com>
Co-authored-by: Carl <acda9e433d19dcd0e6b6840f7f4b98f3a56f1fab98049d444c087019e6d36560@buzz.block.builderlab.xyz>
Signed-off-by: Taylor Ho <taylorkmho@gmail.com>
Co-authored-by: Carl <acda9e433d19dcd0e6b6840f7f4b98f3a56f1fab98049d444c087019e6d36560@buzz.block.builderlab.xyz>
Signed-off-by: Taylor Ho <taylorkmho@gmail.com>
Co-authored-by: Carl <acda9e433d19dcd0e6b6840f7f4b98f3a56f1fab98049d444c087019e6d36560@buzz.block.builderlab.xyz>
Signed-off-by: Taylor Ho <taylorkmho@gmail.com>
Co-authored-by: Carl <acda9e433d19dcd0e6b6840f7f4b98f3a56f1fab98049d444c087019e6d36560@buzz.block.builderlab.xyz>
Signed-off-by: Taylor Ho <taylorkmho@gmail.com>
Co-authored-by: Carl <acda9e433d19dcd0e6b6840f7f4b98f3a56f1fab98049d444c087019e6d36560@buzz.block.builderlab.xyz>

Signed-off-by: Taylor Ho <taylorkmho@gmail.com>
Co-authored-by: Carl <acda9e433d19dcd0e6b6840f7f4b98f3a56f1fab98049d444c087019e6d36560@buzz.block.builderlab.xyz>

Signed-off-by: Taylor Ho <taylorkmho@gmail.com>
Co-authored-by: Carl <acda9e433d19dcd0e6b6840f7f4b98f3a56f1fab98049d444c087019e6d36560@buzz.block.builderlab.xyz>

Signed-off-by: Taylor Ho <taylorkmho@gmail.com>
@tellaho
tellaho force-pushed the tho/link-preview-fetch-timeouts branch from 2f14491 to 274a2a3 Compare September 8, 2026 18:16
@tellaho

tellaho commented Sep 8, 2026

Copy link
Copy Markdown
Contributor Author

AI-generated update: The blocking regression was an expired settled preview job being classified before adoption, allowing its pending replacement to bypass the post-Send deadline and Skip task. Commits d8c06f6 and 274a2a3 classify adopted work instead, bind the expired-to-pending production I/O transition to deadline and Skip coverage, and prevent a transport failure after the one allowed cooldown wait from renewing that wait through the outer retry. Validation at 274a2a3 after rebasing onto current main: Desktop JS 6,456/6,456; Tauri 3,172 passed, 19 ignored; Desktop typecheck; scoped Biome; Tauri rustfmt; git diff check; all pre-push Desktop and Tauri gates.

@jedwards27 jedwards27 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:bot: Jude’s code review agent

Verdict: APPROVE
Reviewed: 44316ff72f5f7de014c66b01cbf534298a70c249..274a2a3ad43a3e9811cafb078ba26f55cdc5c1e7 (exact head 274a2a3ad43a3e9811cafb078ba26f55cdc5c1e7)
Risk: high — renderer/native preview ownership, post-Send publication liveness, and network retry pacing.

Behavior/contracts traced: expired preparation-job adoption and replacement; transient fallback → TTL expiry → pending metadata retry; promoted Send task identity; deadline, Skip, reset, and stale-settlement races; image-host cooldown state through redirects, transport failure, and the outer retry helper.

Findings: no unresolved author-actionable defect. Both prior findings are fixed.

  • prepareBackgroundLinkPreviews now adopts or replaces each candidate before testing whether the adopted job is settled (desktop/src/features/messages/lib/linkPreviewPreparationStore.ts:289-303). An expired settled job replaced by pending production I/O therefore enters the real task path, with functional Skip (:339-346), the finite post-Send timer (:349-352), and one-shot terminal settlement (:315-335,353-357). The production-seam regression drives fetch_link_preview_metadata and proves both timeout and Skip settle this exact transition (desktop/src/features/messages/lib/linkPreviewPreparationStore.test.mjs:258-306). Independent mutation of the old classify-before-adopt ordering caused that regression to hang until the reviewer bound, then restoration passed.
  • A transport failure after an inline cooldown now returns retry_inline: false via !waited_for_cooldown (desktop/src-tauri/src/commands/link_preview.rs:447-471). The outer retry helper consequently cannot create a fresh invocation and renew the wait (desktop/src-tauri/src/commands/link_preview_image_retry.rs:3-20). The paused-time regression binds the production fetch to the outer retry boundary and asserts one send attempt (desktop/src-tauri/src/commands/link_preview_tests.rs:258-292).

Author action: none.
Verification owner: the remaining Desktop Core CI job owns terminal merge-gate completion; reviewer tooling owns independent native execution and GUI observation.

Validation at matching clean head: two independent review lanes agreed the ownership, user-visible Send/Skip behavior, and retry state are coherent. Full Desktop JS passed 6,456/6,456; pnpm typecheck passed; git diff --check passed. HEAD and clean status were rechecked in the same shells. Exact-head CI is green across completed Rust lint/Windows, Desktop smoke 1–4, Windows/macOS builds, integration, DCO, Semgrep, and zizmor jobs; Desktop Core remained in progress when this review was filed.

Manual/native evidence: no native GUI journey. Focused local Tauri execution could not start because the checkout lacks desktop/src-tauri/binaries/buzz-acp-aarch64-apple-darwin; this is reviewer-environment confidence debt, not author work. Codex Security authorized the exact range but its review job was skipped.

Residual risk: exact-head native behavior is supported by source tracing, production-seam tests, author-reported Tauri results, and completed platform CI, but was not independently executed or observed in a native GUI during this round.

@jedwards27 jedwards27 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exact-head re-review of 274a2a3ad43a3e9811cafb078ba26f55cdc5c1e7 against base 44316ff72f5f7de014c66b01cbf534298a70c249: APPROVE.

The prior approval was treated as expired. I re-reviewed the new delta and the prior REQUEST_CHANGES findings rather than carrying clearance forward.

The expired-settled ownership bug is fixed: prepareBackgroundLinkPreviews now adopts/replaces the candidate before classifying its settled state (linkPreviewPreparationStore.ts:289-303). A replacement that enters production I/O is therefore represented as pending work, gets a post-Send deadline and functional Skip (:311-352), and is fenced to one terminal result across completion/timeout/Skip races (:313-357). Identity checks and delete-before-abort ordering prevent stale settlement from deleting or publishing over a replacement (:180-190,235-249,323-330). The send-owned controller remains independently cancellable and is released by the send flow (:270-287,373-378; useMentionSendFlow.ts:686-690).

The user-visible contract follows that state correctly: isPreparing and canSkip derive from the pending task set (linkPreviewPreparationStore.ts:65-69), so the composer overlay exposes truthful “Preparing link preview”/Skip behavior at the transient-fallback → expired-TTL → pending-retry boundary (ComposerUploadProgressOverlay.tsx:16-24). Timeout and Skip authorize send without claiming preview completion.

The cooldown change is also coherent. Once an invocation has already waited for cooldown, a transport failure returns retry_inline: false (link_preview.rs:447-471), preventing retry_transient_image_fetch from creating a fresh invocation and renewing the wait (link_preview_image_retry.rs:10-20). Redirects preserve the invocation-local state; the no-cooldown transport path retains its single outer retry.

Validation on a clean exact-head checkout:

  • full Desktop JavaScript suite: 6,456/6,456 passed
  • Desktop typecheck: passed
  • focused preparation-store file: 14/14 passed
  • git diff --check: passed
  • mutation restoring classify-before-adopt caused the production-I/O regression to stop settling and hit the reviewer timeout; restoring exact-head bytes returned it to passing
  • exact-head CI: Rust lint/results, Windows Rust, macOS/Windows Desktop builds, Desktop smoke shards 1–4, integration shards, Semgrep, zizmor, and DCO were green at review time; Desktop Core remained in progress

No author-actionable defect remains. Local native/Tauri execution and an independent Rust cooldown mutation were blocked by absent packaged buzz-acp/buzz-agent sidecars, and no native GUI journey was run. Those are reviewer/tooling confidence gaps, not author defects; CI owns terminal completion of Desktop Core. A new head invalidates this approval.

@wesbillman wesbillman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Carl, an automated reviewer, commenting via Wes’s GitHub account.

REVIEW CLEAR at 274a2a3ad43a3e9811cafb078ba26f55cdc5c1e7, against base 44316ff72f5f7de014c66b01cbf534298a70c249. No remaining actionable findings in this corrective re-review of the prior review.

  • Prior P1 addressed: preparation adoption now precedes settled classification. An expired settled job replaced by pending metadata therefore enters the existing finite Send task, rather than bypassing the deadline and Skip. I traced live candidates from the composer through promotion, the preparation overlay, and resolvePreviewTags before publication. Timeout retains available tags; Skip settles without previews; terminal fencing prevents late completion from changing the result. The new regression seeds an expired settled fallback, invokes production adoption with pending metadata IPC, and asserts deadline and Skip settlement. It covers the repaired admission seam, not the full elapsed-time composer UI journey.
  • Prior P3 addressed: the transport-error branch now returns retry_inline: !waited_for_cooldown. After a cooldown wait, the outer retry helper cannot restart the operation with a fresh wait allowance. The new production-seam witness calls both the outer helper and inner fetch path and asserts a single transport attempt. The earlier abandoned-fetch starvation and per-lease double-release repairs remain credited.
  • Contract and limits: preserve user-paced composer fetching without an aggregate composer deadline, transport connection/inactivity limits, a finite 10-second promoted-preview budget, immediate preview Skip, and shared-owner cancellation isolation. Reviewed exact source/diffs and test structure only, with an independent native lane integrated. No checkout, PR-code execution, builds, tests, CI monitoring, or GUI workflow performed. This re-review covers the corrective frontend/native paths and their Send/cancellation contracts; it does not reopen unrelated merged-main changes or unchanged mobile/browser/CLI implementations and wire formats. This is a clear COMMENTED review, not an approval.

@tellaho
tellaho merged commit 218633b into main Sep 8, 2026
61 checks passed
@tellaho
tellaho deleted the tho/link-preview-fetch-timeouts branch September 8, 2026 19:59
brow added a commit that referenced this pull request Sep 8, 2026
* origin/main: (29 commits)
  fix(acp): pace targeted overflow recovery on consumer capacity (#7325)
  fix(link-preview): keep composer fetches user-paced (#7211)
  feat(mesh): upgrade to mesh-llm 0.76.0-rc8 and recommend Qwen3.8 27B (#6189)
  fix(agent): route GPT-5+ model-service FQNs to Responses (#7358)
  fix(buzz-acp): wake held ACP threads and fence forked sessions (#7340)
  fix(mobile): style inline code with the app mono face (#6631)
  chore(release): release Buzz Desktop version 0.5.23 (#7381)
  fix(desktop): keep packaged frontendDist relative so Windows embeds assets (#7177)
  fix(sidebar): simplify unread indicators and emphasize priority activity (#7134)
  Add generic information-flow control core (#7293)
  feat(buzz-acp): update base prompt; add buzz context and skills to Pi agents (#7335)
  fix(desktop): restore mention chip identity icons (#7338)
  Persist video playback speed preference (#7336)
  Verify ACP relay events before prompt routing (#7010)
  fix(buzz-acp): bound busy-owner hold to prevent cross-channel starvation (#7337)
  feat(desktop): invite owned agents from standalone forums (#7125)
  fix(desktop): authorize remote mentions at publication (#7124)
  fix(acp): rename system tag to agent-instructions (#7332)
  fix(desktop): bind duplicate mention selections to exact recipients (#7133)
  refactor(relay): extract NIP-29 membership authorization (#7285)
  ...

Signed-off-by: Tom Brow <tomb@block.xyz>
jrobotham-square added a commit to jrobotham-square/buzz that referenced this pull request Sep 8, 2026
…stody

* origin/main:
  fix(acp): pace targeted overflow recovery on consumer capacity (block#7325)
  fix(link-preview): keep composer fetches user-paced (block#7211)
  feat(mesh): upgrade to mesh-llm 0.76.0-rc8 and recommend Qwen3.8 27B (block#6189)
  fix(agent): route GPT-5+ model-service FQNs to Responses (block#7358)
  fix(buzz-acp): wake held ACP threads and fence forked sessions (block#7340)
  fix(mobile): style inline code with the app mono face (block#6631)

Signed-off-by: Joel Robotham <jrobotham@squareup.com>
rileycrane pushed a commit that referenced this pull request Sep 8, 2026
* origin/main: (77 commits)
  fix(acp): pace targeted overflow recovery on consumer capacity (#7325)
  fix(link-preview): keep composer fetches user-paced (#7211)
  feat(mesh): upgrade to mesh-llm 0.76.0-rc8 and recommend Qwen3.8 27B (#6189)
  fix(agent): route GPT-5+ model-service FQNs to Responses (#7358)
  fix(buzz-acp): wake held ACP threads and fence forked sessions (#7340)
  fix(mobile): style inline code with the app mono face (#6631)
  chore(release): release Buzz Desktop version 0.5.23 (#7381)
  fix(desktop): keep packaged frontendDist relative so Windows embeds assets (#7177)
  fix(sidebar): simplify unread indicators and emphasize priority activity (#7134)
  Add generic information-flow control core (#7293)
  feat(buzz-acp): update base prompt; add buzz context and skills to Pi agents (#7335)
  fix(desktop): restore mention chip identity icons (#7338)
  Persist video playback speed preference (#7336)
  Verify ACP relay events before prompt routing (#7010)
  fix(buzz-acp): bound busy-owner hold to prevent cross-channel starvation (#7337)
  feat(desktop): invite owned agents from standalone forums (#7125)
  fix(desktop): authorize remote mentions at publication (#7124)
  fix(acp): rename system tag to agent-instructions (#7332)
  fix(desktop): bind duplicate mention selections to exact recipients (#7133)
  refactor(relay): extract NIP-29 membership authorization (#7285)
  ...

Signed-off-by: Sol <478bb5a31222ea2b28a3d1afb8b1d598940628f19c2a87efc3c4b822299eeec6@buzz.block.builderlab.xyz>

# Conflicts:
#	desktop/src-tauri/src/commands/media_download.rs
#	desktop/src-tauri/src/lib.rs
This was referenced Sep 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants