Skip to content

fix(desktop): recover from malformed connection catalogs - #13468

Closed
JDeffner wants to merge 2 commits into
pingdotgg:mainfrom
JDeffner:fix/desktop-catalog-recovery
Closed

JDeffner wants to merge 2 commits into
pingdotgg:mainfrom
JDeffner:fix/desktop-catalog-recovery

Conversation

@JDeffner

@JDeffner JDeffner commented Sep 24, 2026 •

Copy link
Copy Markdown

What Changed

A malformed connection-catalog.json currently fails the desktop catalog IPC call and prevents the client from registering its local connection. This change preserves the damaged file under a unique .corrupt.<pid>.<uuid> name, then uses the existing missing-catalog path to restore legacy records or start an empty catalog.

Recovery succeeds only after the damaged file is preserved. Read, base64, and decryption failures keep their existing error behavior. Catalog reads, writes, and clears share a semaphore to prevent recovery from moving a concurrent replacement. Writes flush and close the temporary file before replacing the previous catalog, with a writable handle for Windows.

Why

Fixes #4750.

Supporting investigation: Windows 10 incident evidence and isolated reproduction, including catalog contents, startup errors, database checks, and the distinction between confirmed recovery failure and the unproven corruption trigger.

The reported restart left a 1,552-byte, zero-filled catalog while the project and thread database remained readable. That document alone blocked startup. Preserving it and restoring the normal missing-catalog behavior lets the desktop reconnect without changing project or thread data. Remote connection details cannot be reconstructed from zero-filled bytes unless legacy records remain available.

Verification

  • Five regression cases failed against the original implementation, then passed with recovery: zero-filled, empty, invalid JSON, truncated JSON, and invalid envelope fields.
  • 102 focused repository tests passed across desktop catalog/legacy migration/IPC, web catalog storage, first-run logic, and connection registry suites. Two additional isolated desktop-to-renderer checks passed, including failed quarantine without a renderer overwrite.
  • Failure coverage includes write, flush, and rename failures, retry, concurrent reads and writes, legacy relay/SSH/bearer migration, and preserved read/decryption errors.
  • Desktop type checking, targeted lint/formatting, whitespace checks, and the full desktop build passed on Windows with Node 24.13.1.
  • Computer Use verified the built desktop against an isolated database snapshot and a copy of the damaged catalog. Existing threads and Settings loaded. The corrupt bytes remained unchanged in quarantine. Native Windows encryption passed inside the real desktop process; after a full app restart, the saved catalog remained unchanged and thread search worked.

No visual UI changes. Computer Use used accessibility and keyboard controls because screenshot capture was unavailable on this Windows 10 host. Physical power-loss behavior was not tested; flushing does not establish a power-loss guarantee.

Checklist

  • This PR is focused on connection catalog recovery and write durability.
  • I explained what changed and why.

Written with assistance from GPT-6 in Codex.

Summary by CodeRabbit

  • Bug Fixes
    • Malformed connection catalogs are now quarantined during recovery, preserving their original contents instead of treating them as empty.
    • Catalog reads, writes, and clears are coordinated to prevent conflicting operations during recovery.
    • Catalog updates are synchronized before replacing the existing catalog. If an update fails, the previous catalog remains available and the operation can be retried.

@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:M 30-99 changed lines (additions + deletions). labels Sep 24, 2026
@macroscopeapp

macroscopeapp Bot commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at 05709f3

Macroscope's review found this PR approvable — This is a narrowly scoped desktop persistence bug fix that safely quarantines malformed catalog envelopes, preserves the original bytes, serializes recovery with catalog operations, and flushes atomic writes. Existing valid-catalog and decryption-error behavior remains unchanged, with extensive targeted regression coverage.

No code changes detected at f7f8aa8. Prior analysis still applies.

You can add or adjust custom eligibility rules. Learn more.

@coderabbitai

coderabbitai Bot commented Sep 24, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Important

Review skipped

Review was skipped as selected files did not have any reviewable changes.

⚙️ Run configuration

Configuration used: Repository: pingdotgg/t3code/.coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 49f729d4-1402-4bbb-a2f8-c47378f5cc8b

📥 Commits

Reviewing files that changed from the base of the PR and between 05709f3 and f7f8aa8.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: pingdotgg/t3code/.coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: fcbc26ae-0897-4e01-9ed5-32e1ab1e8d47

📥 Commits

Reviewing files that changed from the base of the PR and between f26ee08 and 05709f3.

📒 Files selected for processing (2)
  • apps/desktop/src/app/DesktopConnectionCatalogStore.test.ts
  • apps/desktop/src/app/DesktopConnectionCatalogStore.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.


📝 Walkthrough

Walkthrough

The connection catalog store now serializes reads, writes, and clears. It quarantines malformed catalog files and syncs temporary files before replacing the catalog. Tests cover recovery, write failures, and retries.

Changes

Connection catalog store

Layer / File(s) Summary
Serialized catalog recovery
apps/desktop/src/app/DesktopConnectionCatalogStore.ts, apps/desktop/src/app/DesktopConnectionCatalogStore.test.ts
The store serializes catalog operations and quarantines malformed documents. Recovery errors identify quarantine failures. Tests cover migration, IPC reads and writes, concurrent operations, and preservation of corrupt file contents.
Synced catalog writes
apps/desktop/src/app/DesktopConnectionCatalogStore.ts, apps/desktop/src/app/DesktopConnectionCatalogStore.test.ts
The store syncs the temporary file before replacing the catalog and reports sync failures as write errors. Tests verify failure handling, preservation of the prior catalog, and successful retry.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Suggested reviewers: juliusmarminge

Merge Risk: ⚪ Minimal · up to 05709

The catalog recovery and write changes are ready to merge after normal checks.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Linked Issues check ❓ Inconclusive The PR implements the main [#4750] recovery work. It writes through a temporary file, syncs and closes the temporary handle, then renames it. It serializes reads, writes, and clears. It quarantines ma… Provide reviewable evidence for the unchanged-content write check and the UI path that presents catalog read failures. If either behavior already exists before this pull request, identify the relevant implementation and tests.
✅ Passed checks (4 passed)
Check name Status Explanation
Out of Scope Changes check ✅ Passed The changed production code and tests stay within [#4750]. They address atomic catalog writes, malformed-file quarantine, recovery serialization, legacy fallback, and failure handling. No unrelated pr…
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 2…
Title check ✅ Passed The title clearly and concisely describes the main change: recovery from malformed desktop connection catalogs.
Description check ✅ Passed The description explains what changed, why it changed, verification results, and the checklist status. The UI Changes section is omitted because the author states that the PR has no visual UI changes.
Full details: Linked Issues check

Explanation

The PR implements the main [#4750] recovery work. It writes through a temporary file, syncs and closes the temporary handle, then renames it. It serializes reads, writes, and clears. It quarantines malformed catalog documents under a unique name, preserves their bytes, and follows the missing-catalog path for legacy migration or an empty catalog. The tests cover malformed inputs, migration, retry after quarantine failure, concurrent access, and write-stage failures. The available evidence does not establish two remaining [#4750] requirements at the reviewed head: that unchanged serialized content skips a write, and that catalog read failures are surfaced in the UI. The summary only states that existing read-error behavior is retained.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR

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

@JDeffner

Copy link
Copy Markdown
Author

Additional validation on the affected Windows 10 installation:

  • Applied the catalog implementation from 05709f34f as a local backport to the installed 0.0.42 / 719a76ca1dbf / Electron 44.1.0 release. Only the compiled catalog-service region changed. The other 147 archived files, native unpacked files, executable, server, and UI were verified unchanged. The original archive, damaged catalog, and a consistent database snapshot were backed up first.
  • Tested a copy of that installed application before replacement. With the real database snapshot and corrupt-file copy, it recovered, loaded the existing threads, and exposed them through Computer Use. The database snapshot passed PRAGMA quick_check.
  • Closed the real desktop normally, replaced its application archive, and reopened the installed executable. The application itself quarantined the live 1,552-byte NUL-filled catalog through desktop:get-connection-catalog; the installer did not reset it manually.
  • The live quarantine matches the original byte for byte (SHA-256 07cb830e7cb76dc7d43b0fc8bc389e8225057335ffe80dda35b8c38e4b24f2c1). The installed archive matches the tested replacement. The local environment endpoint returns HTTP 200, version 0.0.42, platform Windows x64.

Two remaining observations distinguish the recovery result from other failures:

  1. The installation helper's final check initially failed because it attempted to parse an empty startup log. The archive had already been replaced and the app restarted successfully. The empty-log handling was corrected locally, and the installation hashes, quarantine, and backend health were then verified directly.
  2. The existing docker-desktop WSL environment still reports No space left on device while staging its runtime and a missing Node.js/node-pty prerequisite. T3 logs its fallback after repeated failures; the native Windows backend is healthy. That WSL problem remains unresolved by this catalog fix.

The final live UI could not be inspected with Computer Use: Windows returned GetCursorPos failed: Access is denied (0x80070005) after refreshing the window and retrying. The copied-installation UI check passed, but the live result above is based on the installed archive, the actual renderer catalog call, preserved file bytes, and backend health. No physical power-loss test or live remote-host connection test was performed.

@juliusmarminge juliusmarminge added the macroscope-review Opt PRs made by unvouched contributors in for Macroscope review. Vouched contributors auto-reviews label Oct 1, 2026 — with ChatGPT Codex Connector
@nasroykh

nasroykh commented Oct 1, 2026

Copy link
Copy Markdown

Another reproduction of #4750 on current main (148e6deea0, Windows 11 build 26300) after a power cut. Details are in #4750. I checked the same quarantine approach locally against main and it fixes the case. Would be good to see this land.

@maria-rcks

Copy link
Copy Markdown
Collaborator

Note

Written by claude-opus-5-5 on behalf of Maria

Hi! We are cleaning up open PRs, and this one appears to have been created with an older model (gpt-6). If this change is really important, we recommend rebuilding the PR with a newer model if possible.

@maria-rcks maria-rcks closed this Oct 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

macroscope-review Opt PRs made by unvouched contributors in for Macroscope review. Vouched contributors auto-reviews size:M 30-99 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

connection-catalog.json is written non-atomically every ~3s, and a corrupt document bricks the app permanently

4 participants