Skip to content

fix(daemon): address cwd-scoped sessions by their store key, not their public name - #2068

Open
NicolasBataille wants to merge 1 commit into
callstack:mainfrom
NicolasBataille:fix/2031-session-state-dir
Open

fix(daemon): address cwd-scoped sessions by their store key, not their public name#2068
NicolasBataille wants to merge 1 commit into
callstack:mainfrom
NicolasBataille:fix/2031-session-state-dir

Conversation

@NicolasBataille

Copy link
Copy Markdown

Refs #2031, #1394 (deliberately not Closes — see below)

Status of #2031

#2031 is closed, by b1ee2a7 (#2057), which is in main. That fix covers the reported
mechanism: a replaced-but-still-running daemon keeping the device claim, now classified
owner-daemon-superseded and reconciled like a dead owner. I re-read that diff; it does what the
issue's headline asks.

The follow-up comment on the same issue describes a different mechanism, and that one still
reproduces on main. This PR fixes that half.

Root cause

resolveEffectiveSessionName scopes an implicit session to the caller's worktree: a session opened
without --session is named default and stored under cwd:<hash>:default. The two are
not interchangeable, because passing --session sets sessionExplicit, which disables scoping —
so --session default addresses the literal key default, a different session.

Three surfaces built caller-facing text from SessionState.name:

  • buildDeviceInUseBySessionErrorDevice is already in use by session "default".
  • buildSessionRecoveryHintagent-device close --session default
  • publicSessionInfo (session list) → sessionStateDir: <state>/sessions/default

Each names something unreachable. Reproduced on main (iOS simulator, this clone):

$ agent-device open <app> --platform ios --udid <udid>          # cwd-scoped `default`
$ agent-device open <app> --platform ios --udid <udid> --session qa
Error (DEVICE_IN_USE): Device is already in use by session "default".
Hint: ... first run agent-device close --session default.

$ agent-device close --session default                          # follow the hint
Error (SESSION_NOT_FOUND): No active session                    # <- the hint's own command

$ agent-device open ... --session qa                            # retry
Error (DEVICE_IN_USE): ... by session "default".                # still held

That is the comment's report exactly: a DEVICE_IN_USE naming a session the documented recovery
cannot close. The same mismatch makes session list report a sessionStateDir and runnerLogPath
under <state>/sessions/default — a directory that is never created — while the session's real
artifacts, runner.log included, sit in <state>/sessions/cwd_<hash>_default, which is what open
already answers with. An operator following session list to read a runner log finds nothing there.

This is the #1394 family: a session's identity and its address diverged, and the surfaces that
report identity kept publishing it as an address.

Fix

  • SessionStore.entries() exposes the key alongside the record.
  • findNewSessionDeviceConflict reads the conflicting session through it and passes the key to
    buildDeviceInUseBySessionError, which reports it in the message, in details.session, and in
    the hint.
  • buildSessionRecoveryHint takes the session's address explicitly. The two other callers
    (session-selector.ts, request-lock-policy.ts) act on the session the current request named,
    where the name is the address, so they pass session.name and their text is unchanged.
  • session list resolves sessionStateDir/runnerLogPath from the store key.

Explicitly named sessions are unaffected everywhere; the DEVICE_IN_USE help-conformance sample is
byte-identical (its fixture session is named checkout).

Tests

Three new files; all five assertions were observed red against the pre-fix code.

  • src/daemon/session-recovery-hints.test.ts — device-in-use, selector-conflict and recording
    recoveries all name the address; an explicitly named session addresses itself unchanged.
  • src/daemon/handlers/__tests__/session-open-device-in-use.test.ts — the producer reports the
    address in message, details and hint.
  • src/daemon/handlers/__tests__/session-inventory-scoped-paths.test.tssession list keeps the
    public name but resolves paths to cwd_<hash>_default.

pnpm check:quick, pnpm test:unit (8093 passed) and pnpm check:layering are green.

Live check

Same sequence as the repro above, after the fix:

Error (DEVICE_IN_USE): Device is already in use by session "cwd:8bea844ab16aa9b3:default".
Hint: ... first run agent-device close --session cwd:8bea844ab16aa9b3:default.

$ agent-device close --session "cwd:8bea844ab16aa9b3:default"
Closed: cwd:8bea844ab16aa9b3:default
$ agent-device open ... --session qa
Opened: <app>

session list now reports .../sessions/cwd_8bea844ab16aa9b3_default, which exists.

The issue's original close→plain-open sequence does not reproduce on main — a plain close
followed by a plain open in the same worktree succeeds. Its --device "iPhone 17" variant now
also raises AMBIGUOUS_MATCH when two simulators share that name, which is separate and correct.

What a maintainer might push back on

  • Exposing the scoped key to users. close --session cwd:<hash>:default is ugly, and it is
    a hash the caller never chose. It is the value that works today, and a hint that works beats a
    hint that reads nicely. The alternative — teaching close/open to resolve a public name
    against scoped keys within the caller's scope — is a bigger change to resolveEffectiveSessionName
    and its sessionExplicit contract, and belongs in its own PR. Say the word and I will draft it
    instead.
  • entries() on SessionStore. The key can also be reconstructed as
    session.sessionScope ? cwd:<id>:<name> : name, which needs no new accessor — but that
    reconstruction is wrong for exactly the case session list omits cwd-scoped sessions opened via replay's internal open dispatch #1394 documents, a session whose sessionScope was
    never propagated. Reading the key the store actually used cannot be wrong.
  • The two unchanged callers. They now pass session.name explicitly, which looks like
    boilerplate. Making the parameter optional would hide the question at the sites most likely to be
    wrong next; the comment at each site records why the name is the address there.
  • Scope. This is a follow-up to a closed issue. If the maintainers prefer, it can be re-filed as
    its own issue first — the reproduction above is self-contained.

…r public name

An implicitly cwd-scoped session is NAMED `default` and STORED under
`cwd:<hash>:default`. Three surfaces built caller-facing text from the name, so
each pointed at something that does not exist:

- `DEVICE_IN_USE` reported `session "default"` and its recovery hint said
  `agent-device close --session default`. `--session` marks the session
  explicit, which disables cwd scoping, so following the hint addressed a
  different, absent session: `SESSION_NOT_FOUND`, and the device stayed held —
  the residual half of callstack#2031 that callstack#2057's superseded-daemon reconciliation does
  not cover.
- `session list` reported `sessionStateDir` and `runnerLogPath` under
  `<state>/sessions/default`, a directory that is never created; the session's
  real artifacts sit in `<state>/sessions/cwd_<hash>_default`, which is what
  `open` already answers with.

`SessionStore.entries()` exposes the key alongside the record, and
`buildSessionRecoveryHint` takes the session's address explicitly. Call sites
that only hold the session the current request named pass its name — the address
there — so their text is unchanged, as are explicitly named sessions everywhere.

Live before/after on an iOS simulator: `open` (cwd-scoped default), then
`open --session qa` on the same device. Before, the hint's own `close --session
default` answered `SESSION_NOT_FOUND` and the retry failed again; after, the
hint names `cwd:8bea844ab16aa9b3:default`, that close releases the device, and
the retry opens.

Refs callstack#2031, callstack#1394
@thymikee

Copy link
Copy Markdown
Member

[P1] Thread the resolved session address through every recovery producer instead of falling back to SessionState.name. The new comments in session-selector.ts and request-lock-policy.ts claim the current session name is its address, but the ordinary implicit route resolves cwd:<hash>:default, loads that store entry, then passes only the record whose public name is default; selector and lock conflicts therefore still emit the same unreachable close --session default recovery this PR fixes elsewhere. The Maestro route also already holds the resolved session name. Pass the store key/address through these APIs and add production-route regressions, not only helper tests. session list must likewise expose the actual address (while preserving name if useful), otherwise its primary discovery surface still tells operators only default, which cannot be reused explicitly. Prefer the store’s existing address owner or a narrow {address, session} reference over a broad raw entries() accessor. Exact-head CI is still absent; do not apply ready-for-human.

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.

2 participants