Fix seven startup, auth, filesystem and config failures that require an app restart to clear - #43
Merged
Merged
Conversation
Closes the bug reported in #37 and fixed in #40, plus two follow-ups found while reviewing that change. messages_fts is contentless (content=''), so SQLite stores no column values and "SELECT text_content ... LIMIT 1" always reads back NULL however many rows are indexed. The is-populated probe was therefore always false, the fts_version check above it was unreachable, and the rebuild branch (which opens with DROP TABLE) ran on every start. The rebuild then materialised every user and assistant message in a single .all(), so on a large history it exhausted the V8 heap and crash-looped the gateway before it could listen. Probe COUNT(*) instead, and page both the full rebuild and the incremental backfill through one keyset-cursor helper so peak memory stays flat regardless of history size. Paging rather than .iterate() is deliberate: better-sqlite3 refuses to run a write while an iterator is open on the same connection, throwing "This database connection is busy executing a query". Record a high-water mark (db_meta.fts_max_id) alongside the row count behind it. Most messages extract to no searchable text once tool_use and tool_result blocks are stripped, so they never receive an FTS rowid and the "missing from FTS" query re-selected every one of them on every launch. messages.id is AUTOINCREMENT, so ids are monotonic and are never reused even after a session delete, which is what makes a simple mark safe here. Two details that are load-bearing and were wrong in the first attempt: The mark advances to the highest id that currently exists, not to the last row the paged query returned. Messages are already indexed on write by indexMessageForSearch, so the NOT IN filter usually excludes the newest rows and the cursor never reaches them; a cursor-derived mark would freeze and leave every later message to be re-scanned forever, which is the cost the mark exists to avoid. The row count is read back live rather than accumulated, because that same write-time indexing means a running total drifts away from reality. An index that is empty but was previously populated is treated as lost and rebuilt, so a wiped index still heals itself without returning to a rebuild on every boot. Measured against a copy of a 145k-message database: peak heap 1218MB to 58MB, and startup cost after the first run drops from roughly 1500ms to 1ms.
…randing the app From #41, plus wiring the callback it introduced. waitForReady() gave up after a fixed 20s and threw. The throw reached start()'s catch, which incremented retries and scheduled another start(), but start() opens with "if (this.process) return" and on a readiness timeout the gateway process is still alive, so the retry returned immediately without doing anything. Nothing re-checked the socket afterwards, no second exception advanced the retry counter, and onError was never reached. A gateway that simply needed longer came up healthy moments later while the window stayed dead until the user quit and relaunched. Keep polling while the process is alive rather than against a wall-clock deadline, and report progress at the old 20s mark through a new onSlowStart callback instead of failing. Genuine failures are unaffected: the exit handler clears this.process, which trips the identity check already inside the loop, which throws and drives the existing restart path. onSlowStart is wired in main.ts so the tray actually says startup is still in progress. Without that the callback would be dead code and the user would still see nothing. The ceiling is 120s rather than ten minutes. This is reachable through ordinary slow startup work, and backfillFtsIndex() runs long before httpServer.listen(), so first launch after a large migration does pay a one-time cost. But a gateway that is alive and still not listening after two minutes is wedged, and surfacing an error with logs beats showing a window that looks fine and never connects. This change and the FTS fix before it need to ship together: before that fix the index rebuild ran on every launch, which is what pushed startup past the old 20s cliff in the first place.
…lt to the CLI Two faults that produce the same symptom: chats start failing with 401 some hours in, and only relaunching the app helps. From #42, extended to the Codex provider and to the env-propagation half reported in #38. 1. A failed refresh disabled refresh permanently scheduleTokenRefresh() retried three times over about 15 seconds, then cleared nextRefreshAt and returned, leaving the process with no timer armed at all. The current access token kept working until it expired, and every request after that returned 401. Both common triggers are transient. Waking from sleep fires an already-due timer before DNS is up (getaddrinfo ENOTFOUND console.anthropic.com). And the token endpoint rotates refresh_token, so concurrent ensureOAuthToken() calls at the top of overlapping turns spend the same one twice and all but the winner are rejected. The old retry loop could not help with the second case: it read the token once outside the loop and resent that same rotated value on all three attempts. Re-arm with a 30s to 10 minute backoff instead of clearing the timer, single-flight the refresh so concurrent callers share one request, and re-check token health when the timer fires in case ensureOAuthToken() already refreshed. ensureOAuthToken() only arms a background retry when none is pending, and does not touch the failure streak. That counter tracks consecutive retry cycles; incrementing it per turn pinned the backoff at its ceiling, so a user who kept typing while offline waited far longer for recovery than one who did not, and re-arming on every turn meant the background timer never fired at all while they were active. Retrying forever also means the user must not be told about it forever. A refresh that never reached the endpoint is a network problem, not an auth problem, so it no longer raises authRequired. An overnight offline stretch now surfaces one notification instead of one every five minutes. Genuine endpoint rejections still notify, and a prolonged outage notifies once when the backoff reaches its ceiling. Codex had all three faults in the same shape, and is more exposed to the concurrency race because it requires a rotated refresh_token on every response where the Claude path can fall back to reusing the old one. Fixed identically, with the token write moved inside the single-flight wrapper so it happens on exactly one path rather than once per concurrent caller racing a non-atomic write to auth.json. 2. Refreshed credentials never reached the spawned CLI The SDK does not merge options.env with process.env, it replaces it: in 0.3.220 the subprocess env is a destructuring default, so passing env wins outright. opts.env comes from cleanEnvForSdk(), which snapshots a login shell once and caches it for the life of the process, and that snapshot inherits whatever process.env held at the time. It therefore captures the token from gateway startup and never changes again. Everything this file does with process.env after a refresh was invisible to the CLI subprocess, which kept using the startup token until it expired. Put the current credential into the env the SDK will actually spawn with. opts.env is rebuilt per run, so mutating it cannot leak into the cached snapshot or another run. The api_key and dorabot_oauth branches each set their own variable and clear the competing one, so behaviour does not depend on a precedence order that lives in the CLI binary rather than the SDK. On a failed refresh the stored access token is used rather than unsetting the variable: a network blip does not invalidate a token that still has time left on it. cli_keychain deliberately does not inject. The CLI owns and refreshes that credential, it can belong to a different account, and silently changing which one is billed is worse than the expiry it would fix. Only a stale token is cleared out of the snapshot so it cannot shadow the keychain. This matters in practice: on an install where the CLI is logged in and dorabot's own tokens have long expired, injecting them would replace a working credential with a dead one.
scheduleReconnect() gave up after 30 attempts and set a terminal state reading "Connection failed after multiple attempts. Restart the app to try again." Once that fired nothing re-armed the timer, so the bridge stayed down for the life of the process and the only recovery was quitting the app. The situations that exhaust 30 attempts are exactly the ones that heal on their own. The gateway is a local process on a Unix socket: it comes back after a crash, after an upgrade replaces it, and after a machine wakes from a night asleep. On this machine's gateway log 2,905 of 3,609 disconnects are heartbeat_timeout, which is overwhelmingly sleep and wake, since timers do not fire while the machine is asleep and the server sweeps the stale client when it comes back. reconnectAttempt does reset on a successful auth, so reaching the limit needs a run of consecutive failures, which is precisely what a long sleep or a slow-starting gateway produces. Keep retrying with no attempt limit, and slow from the existing sub-10s backoff to a 60s poll once the fast attempts are clearly not working, so a gateway that is down for hours is retried once a minute rather than every eight seconds. This is the same shape as the token refresh that gave up permanently: a bounded retry treating a transient outage as terminal, recoverable only by relaunching. Both are now unbounded with a capped backoff. The user-visible consequence of the old behaviour was wider than a status label. Every RPC then failed with "Not connected to gateway", and anything gated on the connection silently stopped, including the file explorer's fs watch, which is started only when connected and whose failure was swallowed.
Three defects that combined to make the file explorer look frozen. The watch was created with recursive: false, so only the immediate contents of the open root produced events. Any create, delete or edit inside a subdirectory was invisible, which is most of a real project. It is now recursive; on macOS that is backed by FSEvents and events are already debounced 250ms, so the cost is a coalesced notification rather than one per file. The fs.change event carried the relative filename all along and the renderer discarded it, forwarding only the watch root. Listeners therefore could not tell which directory changed. The changed directory is now derived from the filename and passed through. FileExplorer reloaded only when the reported path equalled viewRoot, so even with the two fixes above every expanded subdirectory would have stayed stale. It now reloads whichever directory changed, provided that directory is one it has already loaded. The lookup goes through a ref rather than the dirs state so the subscription is not torn down and re-established every time a directory loads. fs.watch.start no longer swallows its rejection. When it failed silently the tree simply stopped updating with nothing logged anywhere, which presents as a broken explorer rather than a watcher that never started.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
… bricking on an unknown one provider.name was declared as 'claude' | 'codex' | 'minimax' | 'openrouter', config.set accepted 'claude', 'codex' or 'minimax', and the factory implemented 'claude' and 'codex'. Three lists, three different answers. Selecting one of the differences threw "Unknown provider: minimax" on every run, and because changing the setting back also needs a working run, there was no way out from inside the app. Delete minimax and openrouter along with the unused OpenRouterProviderConfig, and derive the type and the setter's validation from one PROVIDER_NAMES const so the three cannot drift again. Falling back is what actually unbricks an install: an existing config.json can still name a provider that no longer exists, so the factory now logs and uses claude instead of throwing. Deleting the type alone would have left anyone who had already selected minimax exactly as stuck as before.
verifyAuth() called the SDK with no env at all, so its subprocess inherited process.env while every real run receives opts.env, a cached login-shell snapshot. The check therefore ran against an environment no session would ever see, and could pass against a credential the real path never uses. That is how the UI could report the provider connected while every chat returned 401, which is the confusing half of the reports in #38. It now builds the env with cleanEnvForSdk() exactly as a run does and selects the credential through the same code, so a pass means a run would pass. Extracting getShellPath, getShellEnv and cleanEnvForSdk into src/sdk-env.ts is what makes that possible without a cycle: the provider cannot import agent.ts, which imports the provider. The credential selection that query() performed inline is now applyAuthEnv(), shared by both call sites, so the two cannot drift the way the provider name list did. agent.ts loses 107 lines and gains an import. Verified by A/B against main, reading the spawned child's real environment with ps eww and using CLAUDECODE as the discriminator, since cleanEnvForSdk deletes it and a raw process.env keeps it: origin/main child: CLAUDECODE leaked = true (raw process.env) this branch child: CLAUDECODE leaked = false (cleanEnvForSdk) Both still report authenticated on an install where the CLI owns the credential, so the check did not become stricter, only honest about what it is checking.
suitedaces
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Seven bugs that share one shape: something transient or misconfigured happens, a bounded retry runs out or a hard throw fires, and the process latches into a broken state that only quitting and relaunching clears.
Three were reported by @ankit-thebigred in #40, #41 and #42, following his diagnosis in #37. His analysis was correct in each case and those fixes are his, with follow-ups added where reviewing them turned up more. The rest came from chasing a "Not connected to gateway" report.
What is fixed
1. The search index was destroyed and rebuilt on every launch (ref #37, #40)
messages_ftsis contentless, soSELECT text_content ... LIMIT 1always reads back NULL and the "is it populated?" probe was never true. The rebuild branch, which opens withDROP TABLE, therefore ran every start, and it loaded every message into one array.Measured on a 145k-message database: peak heap 1218MB to 58MB, and startup after the first run ~1500ms to ~1ms. Under a 512MB heap cap the old path OOMs outright, which is the crash loop in #37.
Adds a high-water mark so messages with no searchable text are not re-scanned forever, and a lost-index self-heal.
2. A slow gateway start stranded the app permanently (ref #41)
waitForReady()threw at 20s, the catch scheduled a retry, andstart()returned immediately atif (this.process) returnbecause the process was still alive. A silent no-op: nothing re-checked the socket, the retry counter never advanced,onErrornever fired. Now polls while the process is alive and reports progress viaonSlowStart, which is also wired up so the tray says so.3. A failed OAuth refresh disabled refresh permanently (ref #42, #38)
The retry cleared the timer and returned, leaving nothing armed. Both common triggers are transient: waking from sleep with DNS not yet up, and concurrent turns spending the same rotating
refresh_token. The old loop could not help with the second, since it read the token once and resent that same rotated value three times.Now backs off 30s to 10min without ever disarming, single-flights the refresh, and does not raise
authRequiredfor network-class failures, so an overnight offline stretch produces one notification instead of one every five minutes.The same three faults were present in the Codex provider, which is more exposed because it requires a rotated
refresh_tokenon every response. Fixed there too.4. Refreshed credentials never reached the CLI (ref #38)
The SDK does not merge
options.envwithprocess.env, it replaces it.opts.envis a login-shell snapshot cached once at startup, so it captures the token from gateway start and never changes. Every refresh was invisible to the subprocess, which kept using the startup token until it expired.Credentials are now injected into the env the SDK actually spawns with. Note the
cli_keychainpath deliberately does not inject: that credential belongs to the CLI, can be a different account, and on an install where dorabot's own tokens have expired, injecting them replaces a working credential with a dead one.5. The bridge stopped reconnecting, and the file explorer went silent
scheduleReconnect()gave up after 30 attempts with "Restart the app to try again" and never re-armed. The situations that exhaust 30 attempts are the ones that heal on their own: on one machine 2,905 of 3,609 disconnects areheartbeat_timeout, overwhelmingly sleep and wake. Once it fired, every RPC failed withNot connected to gatewayand anything gated on the connection stopped, including the file explorer's watcher, whose failure was swallowed.Separately the watcher used
recursive: false, so changes in any subdirectory produced no event at all; the renderer discarded thefilenamethe event already carried, so nothing could tell which directory changed; and the explorer only reloaded when the path equalled the watch root. All three are fixed, andfs.watch.startno longer swallows its rejection.6. Selecting a provider that was never implemented bricked the app
ProviderNamedeclared four providers,config.setaccepted three, the factory implemented two. Choosing one of the differences threwUnknown provideron every run, and since changing the setting back also needs a working run, there was no way out from inside the app.minimaxandopenrouterare removed along with the unusedOpenRouterProviderConfig, and the type and the setter's validation now derive from onePROVIDER_NAMESconst so they cannot drift again. The factory falls back to claude with a log instead of throwing, because a stored config can still name a provider that no longer exists and deleting the type alone would leave those installs exactly as stuck.7.
verifyAuth()could report healthy while every session failedIt called the SDK with no
env, so its subprocess inheritedprocess.envwhile real runs receiveopts.env. The check ran against an environment no session would ever see. That is the confusing half of #38: the UI showing the provider connected while chats returned 401.It now builds the env with
cleanEnvForSdk()exactly as a run does and selects the credential through the same shared function, so a pass means a run would pass. This required extracting the env builder tosrc/sdk-env.ts, since the provider cannot importagent.ts, which imports the provider.agent.tsloses 107 lines.Verification
Six of the seven have runtime evidence rather than build-only checks.
backfillFtsIndexagainst copies of a real 1.1GB database. First run 1543ms, then 1ms; wipe the index and it self-heals; the mark advances correctly past 500 messages inserted live.subtype=success is_error=false, with the spawned child's environment probed viaps eww.mainwith the gateway down.mainstops at attempt 30 with no timer armed; this branch reaches 31 and stays armed, then reconnects on its own 44s after the gateway returns, with no app restart.mainover the real RPC with real files.mainfails 4 of 5 cases (subdir, 2-levels-deep, new subdir, delete); this branch passes all 5 with correct relative paths.minimaxconfig now resolves to claude instead of failing.CLAUDECODEas the discriminator sincecleanEnvForSdkdeletes it and a rawprocess.envkeeps it.mainleaks it, this branch does not, and both still report authenticated.Backend and desktop typecheck and both builds pass at every commit, so bisect stays clean.
fix(desktop): recover when the gateway is slow to startis the one without runtime evidence. It needs a genuinely slow gateway start inside Electron, which I could not force cleanly.Ordering
Commits 1 and 2 should land together.
backfillFtsIndex()runs long beforehttpServer.listen(), so the index fix is what stops startup crossing the readiness timeout that commit 2 makes survivable.Known and deliberately not included
_cliHasAuthis cached for the process lifetime, so logging into the CLI while dorabot runs is never noticed.gateway-managerincrementsretriesin both the exit handler and the catch, halving the retry budget on a real crash.runAgent()does not return after a successfulresultmessage. Reproduced onmainas well, so pre-existing.messages_fts_after_deletetrigger makesDELETE FROM messagesthrow on databases created in a particular window, breaking session deletion. Not created by current code, so fresh installs are unaffected.