Skip to content

fix(download): bound trigger lifetime and retire cancelled input - #354

Merged
iuyo5678 merged 4 commits into
Tencent:mainfrom
NianJiuZst:codex/fix-download-deadline
Sep 27, 2026
Merged

iuyo5678 merged 4 commits into
Tencent:mainfrom
NianJiuZst:codex/fix-download-deadline

Conversation

@NianJiuZst

@NianJiuZst NianJiuZst commented Sep 26, 2026 •

Copy link
Copy Markdown
Contributor

Summary

A stalled native click acknowledgement can keep captureBrowserDownload pending after timeout or cancellation. Its listeners remain installed and handleDownload cannot reach the finally that releases the extension-wide download gate, so other sessions receive another bsk download is active.

Bound capture/trigger waiting, fence retired input at native dispatch, and clean up the operation's input and overlay resources within the shared one-second cleanup budget. Fixes #347.

Current head: b967b27c, synchronized with main aaf77145. The dispatch-effect metadata fix from #351 is now included in the base.

Reproduction and evidence

Original hang

The original deterministic reproduction on 8cbcc490 held the trigger promise, or the Input.dispatchMouseEvent(mousePressed) reply inside production handleDownload:

Probe Before this PR Current regression expectation
Capture timeout 100 ms; advance fake time to 10,000 ms Still pending; download listeners remain installed Request settles and removes listeners/timers
Abort with a press acknowledgement held First request remains pending; another session receives the global-gate error Bounded cleanup returns and releases the gate
Deliver an old acknowledgement after retirement Old action can continue No further input is sent by the retired trigger

The second-session test deliberately omits its capability directory and expects that ordinary validation error. It proves gate release, not completion of a second file transfer.

Review follow-up: three independently reproduced edges

The review identified three remaining cases. With the new regression probes added to pre-follow-up commit 04975a6d, all three failed with the observations below. They pass with b967b27c.

Controlled sequence Before the follow-up After the follow-up
Send cleanup release on attachment A; hold its reply; detach A and establish B; expire old cleanup B is detached; its attachment ID becomes undefined B remains attached; old cleanup cannot send input or detach B
Drop the connection after click preparation; hold reattachment; cancel and await handler return; finish reattachment Native input changes from [] to ["mouseMoved"] after return Native input remains []
Retain one unrelated hover bypass; acquire this click's bypass/passthrough; hold press reply; cancel Bypass count remains 2 instead of returning to 1 Count returns to 1 and this click's passthrough lease is removed before the press reply

These handler/driver regressions use production ChromiumCdp with controlled debugger API replies. The overlay cases also start a subsequent download and deliver the previous operation's late reply: the new operation's bypass and passthrough ownership remain intact. Pending/rejected passthrough-end replies are covered; they report cleanup_state: failed without preventing the independent bypass release.

Implementation

  1. Bound capture waiting. Race the trigger against capture failure and the capture deadline/caller cancellation. A completed file does not remove a pending trigger's deadline. Retire listeners and handle late promise rejection during teardown.
  2. Guard actual dispatch. ChromiumCdp.sendGuarded checks the operation signal in the callback immediately before the native sendCommand, after any connection setup. Root and child targets share this boundary. Record the press and its actual attachment only when it reaches this boundary, so a cancelled queued press does not cause a spurious cleanup release.
  3. Fence cleanup to its attachment. Input cleanup rechecks ownership before the timeout fallback. Guarded cleanup sends require the original live attachment and never reconnect. The driver's conditional detach(tabId, expectedAttachmentId) checks identity before invalidating or detaching anything. An already dispatched release is not replayed.
  4. Release overlay ownership independently. withClickOverlay registers one idempotent cleanup with the download operation. Outer retirement and a late inner finally join the same cleanup promise. Bypass and the click's unique passthrough lease release independently, preserving unrelated hover and subsequent click ownership.
  5. Keep cleanup bounded. Input, overlay and uniquely claimed download cleanup begin while the operation owns the download gate and share the existing 1,000 ms waiting budget. Rejection or expiration is reported as cleanup_state: failed; the handler can then release the gate.

Validation

Local environment: macOS, Node 26.10.0, pnpm 10.17.0, Vitest 4.1.6, Chrome 153.0.8010.54. Final validation used b967b27c.

Check Result
New download-lifecycle.test.ts 9 passed: replacement during cleanup; handler cancellation around reattachment; root/child native dispatch cancellation; stale cleanup send/detach refusal; overlay ownership with successful, pending and rejected end replies
Focused unit suites 239 passed across lifecycle, deadline, trigger, file transfer, interaction, Chromium CDP and command deadlines
Complete extension suite 2,341 passed; 122 skipped; opt-in/environment-dependent cases are not counted as passes
Four explicitly enabled real Chrome suites 7 passed, including all 3 download cancellation cases, existing click/overlay cases, DOM multi-click and download-effect metadata
TypeScript compile, extension build, changed-file Biome, git diff --check Passed

The download browser suite now tests both sides of dispatch:

  • Hold the reply after Chrome processes move/press: retirement sends no additional input when the old reply arrives. The press case sends one cleanup release and can complete one click.
  • Hold reattachment before the queued input is sent: cancel and await handler return, then release the attachment reply. Native input stays empty, and the page records zero mouse moves and zero export clicks.

Use the declared pnpm 10.17.0 from the repository root. Local runs selected it with npm exec --yes --package pnpm@10.17.0 -- pnpm .... Browser cases skip when BSK_CLICK_CHROME is unset.

pnpm install --frozen-lockfile
pnpm --filter @browser-skill/extension exec wxt prepare
pnpm --filter @browser-skill/extension exec vitest run \
  src/tools/__tests__/download-lifecycle.test.ts \
  src/tools/__tests__/download-deadline.test.ts \
  src/tools/__tests__/download-trigger.test.ts \
  src/tools/__tests__/file-transfer.test.ts \
  src/tools/__tests__/interaction.test.ts \
  src/browser-driver/__tests__/chromium-cdp.test.ts \
  src/browser-driver/__tests__/command-deadline.test.ts
BSK_CLICK_CHROME=/path/to/chrome \
  pnpm --filter @browser-skill/extension exec vitest run \
  src/tools/__tests__/download-deadline.browser.test.ts \
  src/tools/__tests__/click.browser.test.ts \
  src/tools/__tests__/dom-multiclick.browser.test.ts \
  src/tools/__tests__/download-effect-state.browser.test.ts
pnpm --filter @browser-skill/extension test
pnpm --filter @browser-skill/extension compile
pnpm --filter @browser-skill/extension build

Hosted status checked 2026-09-27 UTC for this head: all 9 reported PR checks passed (CI run). The frontend job reports the 9 lifecycle regressions passing and the extension total of 2,341 passed / 122 skipped. The dedicated download browser job reports 3 passed and emits:

DOWNLOAD_REATTACH_PROOF {"inputs":[],"moves":0,"startedExports":0}

The existing browser suite, Rust, Windows, Node scripts and CodeCC also passed.

Scope and limitations

  • The capture deadline still starts after pre-capture tab/selector resolution.
  • Cancellation cannot retract a command already sent to Chrome, undo page-side work, or guarantee physical browser cleanup when Chrome does not respond. A cleanup release can complete a click already pressed. Cleanup failures are reported rather than treated as successful rollback.
  • Only a uniquely claimed download is cancelled or removed; a page-side asynchronous export may continue independently.
  • Lifecycle tests use controlled debugger, tab, download and overlay APIs. Browser tests use real Chrome/CDP and page input events with adapted extension APIs and deliberately held replies. They do not freeze a real browser process, complete an exported file transfer, or exercise an installed CLI/daemon/extension end-to-end pipeline.

@iuyo5678

Copy link
Copy Markdown
Collaborator

Thanks for this PR and your other recent fixes. I’ve really appreciated the quality of the implementation notes and regression coverage. The explicit distinction between bounded waiting and actually undoing browser-side effects is particularly helpful here.

I reviewed 78bd77e on top of main 4061767. The original hang is still reproducible on main, and this change does release the global download gate in the stalled-acknowledgement case. The 43 focused unit tests and both Chrome cancellation tests also passed locally.

I think this is a worthwhile fix to land. Before merging, could we tighten up three remaining lifecycle edges?

1. Keep the fallback detach scoped to the original attachment

In download-trigger.ts, cleanup checks the attachment identity before starting, but the fallback detach() in the catch block does not repeat that check.

I reproduced this sequence using the production ChromiumCdp with controlled API replies:

  • Cleanup sends a release on attachment A, and its acknowledgement remains pending.
  • Another operation detaches A and establishes attachment B on the same tab.
  • The old cleanup times out and detaches B.

Could the fallback also be conditional on the original attachment identity? Ideally, cleanup sends and detach would enforce that identity at the point of execution. A regression covering replacement during cleanup would complement the existing replacement-before-cleanup test.

2. Carry cancellation through to the actual CDP dispatch boundary

The wrapper checks cancellation before calling sendToCdpTarget(), but the underlying driver may then await ensureAttached() before sending the command.

With the production download handler, I reproduced a connection drop after click preparation, followed by:

  • mouseMoved waiting for reattachment;
  • cancellation completing and the handler returning;
  • reattachment resolving;
  • the retired operation sending mouseMoved.

This differs from the existing browser tests, which hold an acknowledgement after Chrome has already processed the input. The reproduced late input was a move; I have not established an extra press or duplicate download.

Could the operation signal be checked after connection preparation and immediately before native dispatch? This would prevent commands that have not yet been sent from escaping retirement, without requiring cancellation of commands Chrome already received.

3. Include overlay bypass ownership in the independent cleanup

When withClickOverlay() has acquired bypassOverlay(true) and the press acknowledgement stalls, its internal finally still waits for that acknowledgement. The new outer deadline can return and release the download gate while the bypass remains active.

In a handler-level reproduction, the request returned and download listeners were removed, but the bypass count remained 1 until the old reply was delivered.

This leak also exists while main is stuck, so I’m not treating it as a newly introduced defect. However, now that subsequent work can proceed, it would be useful to retire this resource at the same boundary. Could the operation own an independently releasable, idempotent cleanup for the bypass/passthrough resources, so a late inner finally cannot release another operation’s ownership?

These look like focused follow-ups to the current approach rather than a reason to redesign it. I would keep the existing scope—including the documented pre-capture limitation—and address these three cases before merging. Thanks again for the careful work and the unusually clear reproduction evidence.

@NianJiuZst

Copy link
Copy Markdown
Contributor Author

Thank you for the careful review and the concrete reproductions. I reproduced all three cases and addressed them in b967b27c, after synchronizing the branch with main aaf77145.

  1. Attachment-scoped cleanup: the timeout fallback now rechecks the original attachment. The driver also enforces the expected attachment at cleanup dispatch and conditional detach; cleanup cannot reconnect to a replacement. The regression replaces A with B while A's cleanup release reply is pending and verifies that B survives both the timeout and the late reply.

  2. Cancellation at native dispatch: the operation signal is now checked immediately before sendCommand, after connection preparation, for both root and child targets. Press ownership is recorded at that same boundary, so an unsent press does not cause a cleanup release. The handler reproduction no longer emits the late move. A new real Chrome case holds reattachment, cancels and awaits handler return, then releases the attachment reply: no input is dispatched and the page records zero moves and zero export clicks.

  3. Independent overlay cleanup: the click registers one idempotent cleanup with the download operation. Outer retirement and the late inner finally share that cleanup; bypass and passthrough release independently within the existing cleanup budget. The regressions retain an unrelated hover reference, cancel with the press reply held, start another download, and then deliver the old reply. The new operation retains its resources. Pending/rejected passthrough-end replies also preserve independent bypass cleanup and surface cleanup_state: failed.

Validation on this head:

  • 9 new lifecycle tests pass; the three reported failures were first reproduced against the pre-follow-up implementation.
  • 2,341 extension tests passed, 122 skipped; the skipped cases are not counted as passes.
  • 7 explicitly enabled Chrome tests passed, including all 3 download cancellation cases.
  • TypeScript, the extension build, changed-file Biome and whitespace checks passed.

All 9 reported PR checks passed on this head (CI run). The frontend log includes all 9 lifecycle regressions, and the dedicated browser log reports 3 passed with DOWNLOAD_REATTACH_PROOF {"inputs":[],"moves":0,"startedExports":0}.

I also updated the PR body with the before/after evidence, reproducible commands, and verification limits. The pre-capture limitation remains unchanged. Queued click commands are fenced before native dispatch; already delivered input and page-side effects cannot be undone by cancellation.

@iuyo5678
iuyo5678 merged commit 09b4400 into Tencent:main Sep 27, 2026
9 checks passed
@iuyo5678

Copy link
Copy Markdown
Collaborator

LGTM

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.

[Bug] A stalled download click ignores timeout/cancellation and blocks subsequent downloads

2 participants