Skip to content

fix(recover): release a stray attacher of the session's tab instead of reporting healthy (#131) - #132

Merged
broven merged 1 commit into
mainfrom
131
Oct 7, 2026
Merged

broven merged 1 commit into
mainfrom
131

Conversation

@broven

@broven broven commented Oct 7, 2026

Copy link
Copy Markdown
Owner

Summary

  • recover no longer reports healthy while a client outside the session's executor process is the attacher of the session's tab. Before this change, that attacher refused every fresh attach (already attached by another client), recover returned straight from the state machine's healthy without checking, and session reset (which only reaps the executor) could not free it.
  • Clients now report their pid on the control ws (?pid=). recover gains a rung 0, DaemonState.release_foreign_attachers, which releases attacher ownership of the session's tabs held outside the live executor process. Each release is reported as an attach step. The resident executor's own attachment is kept, as designed.
  • The attach-conflict fix hint now says what each verb can free: recover releases an outside attacher; if the error survives a healthy recover, session reset frees an attacher inside the executor process. ADR-0013 is amended.
  • Known limits (also in the commit message):
    • Matching is per process, so an extra attacher inside the executor process still needs session reset.
    • A client too old to report its pid is treated as outside.
    • Rung 0 applies to every backend, per ADR-0013's backend-agnostic ladder.
    • The incident's trigger (PageBindTimeout, and connections that kept piling up per call) was not reproduced and is out of scope.

Closes #131

Test evidence

New E2E test, tests/daemon/e2e/test_issue131_stray_attacher.py (real Chrome for Testing + unpacked extension):

  • A stray process attaches the session's tab and stays alive for the whole test. A fresh attach is refused. recover reports the release, and after session reset a new outside process can attach.
    • It was red before the fix: recover cold-started the executor and released nothing.
  • Negative control: when the resident executor is the attacher, recover releases nothing.
bash tests/daemon/e2e/run.sh (full real-Chrome E2E)
tests/daemon/e2e/test_l3_parity.py ...                                   [ 78%]
tests/daemon/e2e/test_long_lived_session_degradation.py .                [ 79%]
tests/daemon/e2e/test_markdown_command_extension.py ...                  [ 82%]
tests/daemon/e2e/test_markdown_normalize_real_dom.py ...                 [ 85%]
tests/daemon/e2e/test_mute_session_groups.py .                           [ 86%]
tests/daemon/e2e/test_patch_extension.py ..                              [ 88%]
tests/daemon/e2e/test_repro_extension_reload_orphan.py ....              [ 92%]
tests/daemon/e2e/test_repro_zer123.py .                                  [ 93%]
tests/daemon/e2e/test_search_command_cdp.py ....                         [ 97%]
tests/daemon/e2e/test_userscripts_e2e.py ..                              [100%]

======================= 96 passed in 1379.27s (0:22:59) ========================
mise run test
[test:skill] ........................................................................ [ 85%]
[test:daemon] ........................................................................ [ 26%]
[test:skill] ..................................................                       [100%]
[test:skill] 338 passed in 6.00s
[test:skill] Finished in 6.77s
[test:daemon] ........................................................................ [ 33%]
[test:daemon] ........................................................................ [ 39%]
[test:daemon] ........................................................................ [ 46%]
[test:daemon] ...............................sssssss.................................. [ 53%]
[test:daemon] ........................................................................ [ 59%]
[test:daemon] ........................................................................ [ 66%]
[test:daemon] ........................................................................ [ 73%]
[test:daemon] ........................................................................ [ 79%]
[test:daemon] ........................................................................ [ 86%]
[test:daemon] ........................................................................ [ 92%]
[test:daemon] ........................................................................ [ 99%]
[test:daemon] ....                                                                     [100%]
[test:daemon] 990 passed, 94 skipped in 46.12s
[test:daemon] Finished in 46.98s
Finished in 47.01s

UI evidence

N/A: no UI change (CLI recover output gains an attach: step line).

Checklist

  • Test evidence includes the actual commands and output
  • UI evidence filled in or marked N/A

🤖 Generated with Claude Code

…f reporting healthy (#131)

`recover` short-circuited on the recovery state machine's `healthy` and never
looked at the single-attacher table, so a client outside the session's
executor that owned the session's tab kept refusing every fresh attach
("already attached by another client") while `recover` said healthy and
`session reset` (which only reaps the executor) could not free it.

- Clients report their pid on the control ws (`?pid=`); the daemon records
  it on `ClientState`.
- `recover` gains a rung 0: `DaemonState.release_foreign_attachers` drops
  attacher ownership of the session's tabs held outside the live executor
  process, each release reported as an `attach` step. The resident
  executor's own attachment is kept (by design).
- The attach-conflict fix hint now names what each verb can free:
  `recover` for an outside attacher, then `session reset` when the
  executor process itself is the attacher.
- ADR-0013 amended.

Known limits: matching is per process, so an extra attacher inside the
executor process still needs `session reset`; a client too old to report
its pid is treated as outside. The incident's trigger (PageBindTimeout and
per-call lingering connections) was not reproduced and is out of scope.

E2E: tests/daemon/e2e/test_issue131_stray_attacher.py (stray stays alive
throughout; resident-executor negative control). Full e2e: 96 passed.

Co-Authored-By: Claude <noreply@anthropic.com>
@broven
broven merged commit 2318cb8 into main Oct 7, 2026
1 check passed
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.

recover reports healthy four times while the session stays unusable (Target.attachToTarget ... already attached by another client)

1 participant