Skip to content

ADE-60: Local runtime: ensureProject() bypasses isClosed()/retry, so first read after daemon recycle still fails raw#438

Merged
arul28 merged 2 commits into
mainfrom
ade-60-local-runtime-ensureproject-bypasses-isclosed-retry-so-first-read-after-daemon-recycle-still-fails-raw
May 30, 2026
Merged

ADE-60: Local runtime: ensureProject() bypasses isClosed()/retry, so first read after daemon recycle still fails raw#438
arul28 merged 2 commits into
mainfrom
ade-60-local-runtime-ensureproject-bypasses-isclosed-retry-so-first-read-after-daemon-recycle-still-fails-raw

Conversation

@arul28
Copy link
Copy Markdown
Owner

@arul28 arul28 commented May 30, 2026

Fixes ADE-60

Summary

  • Retry local runtime project registration once when the cached daemon connection is already closed or drops during projects.add.
  • Reset the stale connection before retrying so the next connect reaches the live daemon.
  • Add regression coverage for both the pre-call isClosed path and the projects.add dropped-connection path before a read action.

Validation

  • npx vitest run src/main/services/localRuntime/localRuntimeConnectionPool.test.ts
  • npm --prefix apps/desktop run typecheck
  • npx eslint src/main/services/localRuntime/localRuntimeConnectionPool.ts src/main/services/localRuntime/localRuntimeConnectionPool.test.ts

ADE PR: https://ade-app.dev/open?type=pr&repo=arul28%2FADE&number=438

Summary by CodeRabbit

Release Notes

  • Bug Fixes
    • Improved reliability of local runtime connections with automatic reconnection handling. When a cached runtime connection drops or becomes unusable, the system now automatically retries project registration up to two times, ensuring smoother operation during temporary connection interruptions.

Review Change Stack

Greptile Summary

This PR adds retry logic to ensureProject() so a stale/closed daemon connection detected either before or during the projects.add call is transparently recovered from, matching the existing pattern already in callActionForRootUncoalesced.

  • ensureProject now loops up to two times: on the first attempt it checks isClosed() before calling projects.add, and catches connection-dropped errors thrown during the call itself; on each failure it calls resetActiveConnection (which clears this.connection and projectsByRoot) and retries with a fresh connection.
  • Two regression tests are added: one covering the isClosed() pre-call path and one covering the mid-projects.add drop path that then continues all the way through a successful callActionForRoot.

Confidence Score: 5/5

Safe to merge — the retry logic is narrowly scoped to connection-dropped errors, follows the identical pattern already used in callActionForRootUncoalesced, and both new regression tests pass end-to-end through a successful action call.

The change is a focused two-attempt retry loop that only activates on connection-dropped errors in ensureProject. resetActiveConnection correctly clears all shared state (this.connection, activeConnection, projectsByRoot) before each retry, so no stale data can leak across attempts. The existing callActionForRootUncoalesced retry loop interacts cleanly with this: if ensureProject exhausts its retries it throws immediately, and if ensureProject succeeds the action-call retry path continues to work as before.

No files require special attention.

Important Files Changed

Filename Overview
apps/desktop/src/main/services/localRuntime/localRuntimeConnectionPool.ts Adds a 2-attempt retry loop to ensureProject() mirroring the existing isClosed/connection-drop handling in callActionForRootUncoalesced; resetActiveConnection correctly clears connection state and projectsByRoot before each retry.
apps/desktop/src/main/services/localRuntime/localRuntimeConnectionPool.test.ts Two new test cases cover the isClosed() pre-call path and the mid-call connection-drop path; both correctly mock createConnection to return fresh entries on retry and verify call counts, log events, and end-to-end results.

Sequence Diagram

sequenceDiagram
    participant C as callActionForRootUncoalesced
    participant EP as ensureProject
    participant Pool as ConnectionPool
    participant D1 as Daemon (stale)
    participant D2 as Daemon (fresh)

    C->>EP: ensureProject(rootPath)
    EP->>Pool: connect() → entry1 (stale)
    alt "isClosed() == true"
        EP->>Pool: resetActiveConnection(entry1)
        Note over EP: clears connection + projectsByRoot
    else projects.add drops
        EP->>D1: projects.add
        D1-->>EP: connection dropped
        EP->>Pool: resetActiveConnection(entry1)
    end
    EP->>Pool: connect() → entry2 (fresh)
    EP->>D2: projects.add
    D2-->>EP: project record
    EP-->>C: project (cached)
    C->>Pool: connect() → entry2 (same cached)
    C->>D2: ade/actions/call (projectId)
    D2-->>C: action result
Loading

Reviews (8): Last reviewed commit: "Refs ADE-60: fix: document unreachable p..." | Re-trigger Greptile

@linear-code
Copy link
Copy Markdown

linear-code Bot commented May 30, 2026

ADE-60

@vercel
Copy link
Copy Markdown

vercel Bot commented May 30, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
ade Ignored Ignored Preview May 30, 2026 8:34am

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 30, 2026

📝 Walkthrough

Walkthrough

The PR adds retry logic to the ensureProject method to handle dropped runtime daemon connections, allowing the connection pool to automatically reconnect and retry project registration instead of failing. Two new test cases validate this behavior across different failure scenarios.

Changes

Connection drop retry mechanism and tests

Layer / File(s) Summary
Retry logic for dropped runtime connections
apps/desktop/src/main/services/localRuntime/localRuntimeConnectionPool.ts
ensureProject wraps the cached connection check and projects.add call in a 2-attempt retry loop. It detects closed connections via isClosed(), logs local_runtime.ensure_project_connection_dropped, resets the active connection entry, and retries; other errors are rethrown immediately, and a final error is thrown after exhausting attempts.
Tests for connection drop retry scenarios
apps/desktop/src/main/services/localRuntime/localRuntimeConnectionPool.test.ts
Two test cases validate that when a cached runtime client drops before a read action, the pool reconnects and retries projects.add on a fresh client before performing the read action; and when the client is closed before project registration, the pool reconnects first so projects.add runs only on the new client. Both assert the dropped-connection warning log.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related issues

Suggested labels

desktop

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title directly and specifically describes the main change: adding retry logic to ensureProject() to handle closed daemon connections after daemon recycle.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch ade-60-local-runtime-ensureproject-bypasses-isclosed-retry-so-first-read-after-daemon-recycle-still-fails-raw

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@capy-ai
Copy link
Copy Markdown

capy-ai Bot commented May 30, 2026

Capy auto-review is paused for this organization because the monthly auto-review limit has been reached. Increase the limit or turn it off in billing settings to resume automatic reviews.

@arul28
Copy link
Copy Markdown
Owner Author

arul28 commented May 30, 2026

@copilot review but do not make fixes

@arul28 arul28 force-pushed the ade-60-local-runtime-ensureproject-bypasses-isclosed-retry-so-first-read-after-daemon-recycle-still-fails-raw branch 7 times, most recently from 4df2b3a to 46bf5aa Compare May 30, 2026 08:20
@arul28 arul28 force-pushed the ade-60-local-runtime-ensureproject-bypasses-isclosed-retry-so-first-read-after-daemon-recycle-still-fails-raw branch from 46bf5aa to d144f9d Compare May 30, 2026 08:34
@arul28 arul28 merged commit 171cb06 into main May 30, 2026
28 checks passed
@arul28 arul28 deleted the ade-60-local-runtime-ensureproject-bypasses-isclosed-retry-so-first-read-after-daemon-recycle-still-fails-raw branch May 30, 2026 08:59
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.

1 participant