fix: zero client error/warn noise on expected server restarts#507
Merged
Conversation
…ay quiet) A server restart produced a burst of error/warn logs on recovery. The dominant source was EditorPane's 3s disk-sync poll, which logged at error on every failed fetch with no backoff and no connectivity awareness; the WsClient also logged each failed reconnect attempt at error. None of this is surprising — the client already knows the server is gone via the WS-derived connection status — so it should be silent. Principle applied: severity reflects *surprise*, and surprise is relative to known connectivity. Expected transport failures during a known outage are not logged; only genuinely unexpected failures are. - api.ts: add isTransientNetworkError() — one reusable classifier for fetch transport failures (TypeError / AbortError), distinct from HTTP ApiError. Make ApiError a real Error subclass (was a plain object that stringified to "[object Object]" and escaped log dedupe). Reuse the classifier in App.tsx's bootstrap retry, replacing an inline engine-specific string match. - EditorPane: pause the disk-sync poll while connection status !== 'ready' (resumes automatically on reconnect); classify the poll and file-load catches so transient transport failures stay silent while unexpected errors still log. - ws-client: a failed reconnect attempt is expected during a restart — log at debug; giving up after max attempts is a degraded state — log at warn (was error in both cases). Tests: TDD — poll pauses when disconnected and resumes when ready; transient failures don't log error/warn but unexpected ones do; isTransientNetworkError and ApiError unit coverage; ws-client reconnect stays off error/warn. Existing EditorPane stores gain the connection slice; api mocks gain the new export. Generated with Amplifier Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
…nd 2) Addresses findings from adversarial review of 9c1dbacc: - MAJOR: classifying any TypeError as transient could silently swallow real bugs (e.g. a null-deref while processing a *successful* response). Now api.request() throws a dedicated NetworkError on transport failures (fetch rejection or connection lost mid-body), and isTransientRequestFailure() matches only NetworkError / AbortError / gateway 502-504 ApiError. A bare TypeError is a real bug and surfaces again. - MAJOR: HTTP 5xx during the shutdown/boot window while the WS still reads 'ready' logged at error. Gateway statuses (502/503/504) are now classified transient, and the EditorPane catches re-check live connection status via a ref (the closure value can be stale when the server dies mid-poll). - MINOR: the same log-at-error-on-transient pattern existed in EditorPane's terminal-cwd fetch and autocomplete catches — same classification applied. - MINOR: mount auto-restore now waits for connection 'ready' instead of firing into a down server and failing silently (restoredRef stays unset so it retries when ready); transient auto-restore failures covered by tests. - ApiError.toJSON() preserves message under JSON.stringify (Error.message is non-enumerable); ws reconnect-noise test restores the WebSocket global. Two tests asserting fetch-rejects-with-generic-Error logged errors were updated: real fetch() rejects only with TypeError/AbortError, so those scenarios are transport failures and now intentionally stay silent; the "unexpected error still logs" guarantee is covered by HTTP-500 and malformed-body tests instead. Generated with Amplifier Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
…nd 3) Addresses the two MAJORs from round-2 review of 8e4684d5: - MAJOR: WsClient permanently gave up after ~10 reconnect attempts (~35-45s). With the disk-sync poll and editor restore now gated on connection 'ready', a restart slower than the backoff budget left the editor silently wedged until page reload. The client now falls back to a slow steady retry (15s) after the fast budget is exhausted — warning exactly once — and recovers whenever the server returns. Test proves: warn once, no error, keeps retrying, reconnects. - MAJOR: autosave punched through the zero-noise guarantee — typing during an outage logged editor_autosave_failed at error 5s later. Autosave, manual save, and open-external now classify transient failures silently, and a new reconnect effect re-schedules the pending save once the connection returns (through the normal debounce path, so the disk-sync poll can still win and raise the conflict UI first — no conflict handling bypassed). Minors from the same review: - isTransientRequestFailure doc caveat: some endpoints 503 in steady state (fresh-agent runtime-unavailable) — do not gate those calls with it. - connectionStatusRef assignment moved from render into an effect. - Poll-backstop coupling made explicit: test proves a transiently failed mount-restore is recovered by the next disk-sync poll tick. - Stale local name isTransientFetchFailure renamed. Generated with Amplifier Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
… round 4) Addresses the three MAJORs from round-3 review of 7677aa9e: - MAJOR: the retry-on-reconnect autosave raced the disk-sync poll's conflict detection (5s debounce vs 3s poll on a cold-started server) and could silently overwrite external changes made during the outage. The retry now stats the file first and stands down on an mtime mismatch, letting the poll raise the conflict UI. Test proves: dirty pending + disk changed + reconnect => no write, conflict banner shown. - MAJOR: File System Access saves never updated lastSavedContent, so the retry effect saw FSA files as permanently dirty and rewrote the user's local file on every reconnect flap or effect re-run. Both FSA save paths now record lastSavedContent, and the retry effect skips FSA files entirely (their writes never touch the server). FSA write failures (permissions/quota) are also no longer suppressed by the connectivity guard — the compound predicate was centralized into isExpectedOutageFailure() and scoped to server calls only, with branch-scoped catches in scheduleAutoSave/performSave. - MAJOR (dev): the Vite proxy converted only ECONNREFUSED to 503; a backend killed mid-request surfaced ECONNRESET as a default 500, which is correctly non-transient and logged at error. The proxy now maps the transport-level errno family (ECONNREFUSED/ECONNRESET/EPIPE/ETIMEDOUT/EHOSTUNREACH) to 503. connectionStatusRef is also written during render again (documented): the post-paint effect write widened the stale-'ready' window. Minors: disconnect() resets slowRetryAnnounced (symmetric with onopen); slow branch honors minDelayMs; ws test pins the degraded-state warn to exactly one; the hand-rolled api mocks gained the gateway-status arm so they track real classifier semantics (the ws-bootstrap 503 test now exercises the retry). Generated with Amplifier Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
… 4 minor) Round-4 review MINOR-2: when the backend dies while streaming a response body, the Vite proxy's error handler ran res.end() with headers already sent, which terminated the chunked stream cleanly — the client saw a *successful* truncated response, parsed garbage, and a later save could 400 and log an error during an expected restart. Destroy the connection instead so the client sees a transport failure (NetworkError -> classified transient -> silent). Also rebased onto origin/main (9f3a503, #505); no semantic overlap (#505 touches server/coding-cli timestamp flooring, not files-router or the client paths changed here). Full client suite re-verified green on the new base. Generated with Amplifier Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c9ce725d5f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+700
to
+701
| if (statResult.exists && statResult.modifiedAt !== lastKnownMtime.current) return | ||
| scheduleAutoSave(pendingContent.current) |
There was a problem hiding this comment.
When a user has an unsaved edit during an outage and the file is deleted externally before the server reconnects, /api/files/stat returns exists: false; this guard treats that as “unchanged” and immediately schedules autosave, recreating the deleted file without showing the conflict flow. Since the retry is specifically meant to avoid overwriting disk changes during the outage, !exists should also stand down or surface a conflict when the pane was editing an existing file.
Useful? React with 👍 / 👎.
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
2 participants
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.
Problem
Killing and restarting the backend flooded client logs with
editor_stat_poll_failed: "Failed to fetch"at error severity — an expected event logged as a surprise. EditorPane's 3s disk-sync poll ignored connectivity, WsClient logged every failed reconnect at error and permanently gave up after ~10 attempts, and ApiError was a plain object that stringified to[object Object]and escaped log dedupe.Principle
Severity reflects surprise, and surprise is relative to known connectivity (
state.connection.status, WS-derived). Expected transport failures during a known outage stay silent; genuinely surprising conditions (HTTP 500, malformed responses, local FSA write failures, real bugs) still log at error.Changes
request()throws a dedicatedNetworkErroron transport failures (fetch rejection or connection lost mid-body);ApiErroris a realErrorsubclass (withtoJSON());isTransientRequestFailure()matches only NetworkError / AbortError / gateway 502-504.Verification