Repository navigation
Conversation
…sync A single failed request right after a reconnect surfaced the location recovery prompt even though the directory was fine. Retry the sync a bounded number of times before declaring the location missing, and extract the sync logic into a createLocation creator (same pattern as context/session-retention) so the behavior is testable.
|
The following comment was made by an LLM, it may be inaccurate: |
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reproduced #55250 on the base: after a network drop, with a proxy returning 502 for GET /api/agent for about 1s after the event stream reconnects, the composer is replaced by "Session location unavailable". With this PR, the same run keeps the composer and the prompt doesn't appear. I ran the issue's reproduction script from source on both checkouts. The new tests pass, test/context/ passes, and packages/tui type-checks.
Public API impact: none. createLocation is a new export internal to the TUI package, and LocationProvider's context shape is unchanged.
Concerns:
- Retrying, not classifying. The fix retries the whole sync and then treats any remaining failure as a missing location. A resource failure that lasts longer than about 1.5s (a persistent 502, a broken plugin model as in #54943, a version mismatch as in #49352) still replaces the composer with the location prompt. A directory that really is missing now takes about 1.5s longer to report. The longer-term fix the issue points to is to tell
location.getfailures apart from resource-load failures (indata.location.sync, or by checkingcausein the session route). Even if the retry stays for now, it would help to say this limitation explicitly. - The retry loop and the timing-based tests have inline comments.
| ) | ||
| return | ||
| if (remaining > 0) { | ||
| await new Promise((resolve) => setTimeout(resolve, input.delay ?? RETRY_DELAY)) |
There was a problem hiding this comment.
Supersession (generation/ref) is only checked right after a failure. After this wait, the loop calls data.location.sync(target) again even if a newer sync has started or the target has changed, so a superseded sync still sends one more round of requests before it notices. Pending retries also keep running after the provider is disposed. Re-checking generation !== attempt (and whether the owner is disposed) right after the wait would make "a newer sync cancels an older one's pending retries" true for the requests too, not only for the error.
| const scope = setup({ failures: 1 }) | ||
| try { | ||
| scope.location.set({ directory }) | ||
| await sleep(150) |
There was a problem hiding this comment.
Both tests depend on real timers: a 20ms retry delay checked after fixed sleeps of 150ms and 250ms. The persistent-failure case needs 4 full sync rounds plus 3 delays to finish within 250ms, which may not hold on a loaded CI runner. Could you wait for the condition, e.g. poll until vcsCalls() reaches the expected count or the error is set, with a generous timeout, or inject the delay function so the test controls it? Then the outcome wouldn't depend on scheduler timing.
Re-check that the attempt still owns the location (and the provider is alive) after the backoff wait, so a superseded sync cannot fire another round and pending retries stop on disposal. Tests now wait on conditions instead of fixed sleeps.
|
Addressed both inline points in 1286317:
Also added the retry-vs-classify limitation to the PR description as suggested; classifying |
|
Cross-referencing from #55940, where the combined implementation is now described. The classification half belongs in #55940 and the retry half here, but they are only complete together: as noted in this PR's description, a location that stays unreachable past the retry window still surfaces the recovery prompt. Splitting the state into The retry here also works better with that split: it only retries what can succeed on a retry. A Tests for both halves are described on #55940, and the combined implementation is on |
|
Consolidating into #55940 — ggrabarz's |
|
Merged PR #57594 supersedes this one — it landed the same behaviour (only the server's typed Two things are worth adding on top of it, both of which are now open:
No need to reconcile these with anything here — they are additive and touch different files. |
Issue for this PR
Closes #55250
Type of change
What does this PR do?
After a reconnection the location sync can transiently fail while the server/proxy is still settling (the issue reproduces with
/api/agentanswering 502 for about a second after the event stream reconnects). Any single failed request in that sync set the location error, so the session view was replaced by the "location unavailable" recovery prompt even though the directory was fine.This gives the sync a small bounded retry (3 retries, 500ms apart) before declaring the location missing. Supersession and stale-target behavior is unchanged: a newer sync still cancels an older one's pending retries and retained error. As part of making this testable, the sync/retry logic moves into a
createLocationcreator consumed byLocationProvider-- the same pattern ascontext/session-retention.Limitation (per review discussion): this retries rather than classifying failures -- a location that stays unreachable past the retry window (~1.5s) still surfaces the recovery prompt, and a genuinely missing directory takes that long to report. Distinguishing
location.getfailures from resource-load failures (indata.location.sync, or by inspectingcausein the session route) is the follow-up the issue points to.How did you verify your code works?
packages/tui/test/context/location.test.ts:cd packages/tui && bun test test/context/location.test.ts-> 2 pass / 0 failcd packages/tui && bun test test/context/-> 78 pass / 2 fail; the two failing cases (switching agents restores their model and variant within the session,agent and model drafts are isolated across sessions and survive navigation) also fail on a clean checkout ofdevwithout this change (pre-existing, unrelated).cd packages/tui && bun run typecheck-> cleanScreenshots / recordings
Not a visual change.
Checklist