Skip to content

P68: keep reconnect failure in its session and finish cleanup - #713

Merged
trissim merged 6 commits into
mainfrom
fix/session-reconnect-state
Oct 16, 2026
Merged

trissim merged 6 commits into
mainfrom
fix/session-reconnect-state

Conversation

@trissim

@trissim trissim commented Oct 16, 2026 •

Copy link
Copy Markdown

P68. A failed session handshake already records FailedSessionDisposition and the real agent error. reconnect() then threw a second, generic ValueError through transcript publication and view release. Remove that second failure. The unchanged 30-second settle budget belongs to AgentSession; the timeout message derives from it. Failure and explicit close share one retirement task. Login reports success only after readiness.

No ValueError or RuntimeError catch remains in reconnect. OSError and jsonrpc.APIError identify transport/agent failures. Only the route/preflight block handles RouteChanged and RelationViolationError (MaintenanceBarrier.require_open raises the latter). Unexpected errors still propagate.

Application resources no longer depend on a later Unmount event: the existing AsyncResourceRelease completes view, observer and process-pool cleanup even if a view fails to release. It preserves errors rather than swallowing them.

Separate rename-only commit 9dbe41f migrates the entire Toad SessionLoadPlan family and runtime field/parameter consumers. Old-family/source-field search across src and tests returns zero. Depends on Core #1026 (8b6620b32); do not install this Toad with an older Core. ACP metadata spellings are unchanged. Core and Toad must be integrated with current main before pinning the pair.

Source reasoning: AgentSession owns readiness, failure and retirement; ToadApp owns observer and preparation lifetime. NRA before/after scans enumerate production and dependency declarations, writes, calls, checks and imports; evidence is /home/ts/.cache/agent-scratch/forms-migration/p68-plan-{before,after}.json. The earlier source pass covered 1,826 modules, 10,719 classes, 10,683 indexed; 36 declarations outside the index remain unresolved, with no parse errors. Dynamic callback resolution is not a behavioral proof. Existing source sites and supplied blocked observer stacks were read.

Actual production modules import with Core8b6620b32; diff check passes; added function-local imports0 and added banned names0. No automated tests. Independent P55 forms work stays in #670; this is a narrow main-based repair.

Opening line decided once (943e71b)

Commit 943e71b (its message says "Preserve stopped P68 verification for Claude handoff"; it contains only this change) makes ScreenContent.render the single place that chooses the opening line. If agent.session.disposition.current_failure() is set it passes Thread disconnected: {message}: {details} with visible=True; otherwise it shows the content state's declared line(). label() now shows exactly the text and visibility it is given; it no longer overrides them. Opening, Ready and Failed content declare only their text, and their three render overrides are deleted. All consumers of render/label are in screens/main.py (compose, finish, attached, the AgentFail handler).

Real UI check

Isolated Xvfb :253, plain st, isolated AGENT_COMMS_ROOT with copied saved sessions, Toad at this head, Core 8b6620b32. Two threads open (p68-failed, p68-healthy), both ready. SIGKILL of p68-failed's ACP child gives "Agent returned a failure code: -9". Its project directory was then moved away and session.reconnect() was called, so the restart failed with a real OSError. Nothing escaped reconnect. The failed tab, opened by a real click, shows Thread disconnected: Failed to start agent: [Errno 2] No such file or directory: '…/project-a'. The healthy thread stayed ready, its ACP process stayed alive and it shows no label. Ctrl+Q exited cleanly, with no new Terminal_crash file in the default or isolated log directories. No provider input was sent.

Still misleading on a failed session (not changed here): the footer says "Agent is not ready. Please wait while the agent connects…", and the old transcript status line still says "Ready".

🤖 Generated with Claude Code

@trissim
trissim marked this pull request as ready for review October 16, 2026 21:55
Copilot AI balanced review requested due to automatic review settings October 16, 2026 21:55

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.

AgentSession.reconnect_after_login() restarts the session and raises
LoginNotReady with the session's own recorded failure when the restart
did not become ready. Conversation calls it instead of reconnect() and
no longer checks readiness itself; its existing ValueError handler
reports the failure and the success flash runs only for a ready session.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@trissim

trissim commented Oct 16, 2026

Copy link
Copy Markdown
Author

Ratchet fix at a31e33a: the post-login readiness check moved from Conversation to AgentSession.reconnect_after_login(), which raises LoginNotReady (a ValueError) carrying the session's recorded failure. Conversation calls that one method, so its existing handler reports the failure and the success flash runs only for a ready session. debt_ratchet --root src/toad/ against main b0d6aba: exit 0, no positive deltas, Conversation GodClassExcess 905 -> 905 (/home/ts/.cache/agent-scratch/rts-results/p68-toad-ratchet-claude-session.json). The real-UI kill/reconnect check ran on 943e71b, and this commit does not change that path; the terminal-login path was not run live.

🤖 Generated with Claude Code

@trissim
trissim merged commit d4a7cf9 into main Oct 16, 2026
2 checks passed
@trissim
trissim deleted the fix/session-reconnect-state branch October 16, 2026 22:05
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