fix(client): restore-flag resilience, persist-empty guard v2, per-tab layout salvage#519
Open
danshapiro wants to merge 4 commits into
Open
fix(client): restore-flag resilience, persist-empty guard v2, per-tab layout salvage#519danshapiro wants to merge 4 commits into
danshapiro wants to merge 4 commits into
Conversation
Root cause: consumeTerminalRestoreRequestId() in terminal-restore.ts was a one-shot consume -- it deleted the armed requestId from the module-level Set on first read. TerminalView's onReconnect mid-restore re-drive (TerminalView.tsx) reuses the SAME createRequestId when a server restart lands before terminal.created is ever received, but if the restore flag had already been read once (e.g. an earlier attempt within this pane's lifecycle, or a component remount that resets local ref caches back to their defaults), the SECOND terminal.create for that requestId silently carried restore:false. The rust server then mints a brand-new session for an existing pane, and the pane's history becomes invisible -- exactly the incident symptom (every amplifier terminal restored as a fresh session after two server restarts landed 52s apart, the second mid-restore). Fix: consumeTerminalRestoreRequestId() is now a non-destructive peek. The armed flag persists across any number of interrupted restore rounds and is only removed by an explicit clearTerminalRestoreRequestId() call, invoked at the two points where a requestId's restore fate is actually settled: (1) the pane anchors -- terminal.created is received for that requestId, or (2) the requestId is abandoned in favor of a freshly-minted one (the three SESSION_IDENTITY_MISMATCH / dead_live_handle / INVALID_TERMINAL_ID reconnect paths). TerminalView.tsx's redundant local ref-cache (restoreRequestIdRef/restoreFlagRef) is removed -- peeking is now safe and idempotent, so there is no longer any need to memoize the result per requestId, and removing it eliminates a possible source of stale/masked reads across component remounts. This design was chosen over an alternative (re-arming the flag only in the onReconnect mid-restore re-drive branch) because the invariant needs to hold regardless of which exact code path re-attempts sendCreate for an unanchored requestId -- module-level peek/clear correctness doesn't depend on guessing the trigger mechanism. It also keeps the existing one-shot contract test file's *shape* intact (same exported function names), so the blast radius stays limited to behavior, not call sites, across the several test files that mock @/lib/terminal-restore. Tests: - test/unit/lib/terminal-restore.test.ts: real module contract (peek survives repeated reads; clearTerminalRestoreRequestId resolves it; no-op on unarmed ids). RED under old one-shot consume, GREEN now. - test/unit/client/components/TerminalView.restore-flag-persistence.test.tsx (new): uses the REAL terminal-restore module (not mocked) to prove the end-to-end regression -- a shell-mode pane (no sessionRef, the exact case App.tsx's terminal.inventory re-arm can't reach) that never anchors keeps sending restore:true across a simulated remount; anchoring clears the flag and post-anchor reconnects never resend restore:true; a fresh user-created pane never carries restore:true. RED under old code (round 2 got restore:false), GREEN now. - Updated terminal-restore mocks in TerminalView.lifecycle.test.tsx, TerminalView.codex-identity.test.tsx, and 4 e2e flow test files to add the new clearTerminalRestoreRequestId export (no behavior change to those existing suites; all still pass). 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
Root cause: the v1 destructive-empty-tabs guard (`distrustEmptyTabs`) was a one-shot latch, armed only for a session that booted via recovery (corrupt/partial persisted layout), and PERMANENTLY disarmed the instant ANY non-empty tabs state was observed. This left two gaps that both landed in the same incident (a browser refresh after a mid-restore server crash showed ZERO tabs): 1. A perfectly normal boot (persisted layout parses fine, tabs.length > 0 from the very first render) never armed the latch at all. Any LATER emptying of tabs via a non-user path (a cross-tab hydrateTabs merge tombstoning the only tab, or any other bug) sailed through with zero protection and destructively overwrote the persisted layout. 2. Even a genuine recovery-boot session lost all protection forever the moment it observed one non-empty tabs snapshot -- so a later, unrelated emptying in that same session was equally unprotected. Neither case logged anything actionable, so the destructive write was unprovable after the fact. Fix (v2): the guard is now a stateless, permanent rule evaluated on every flush, indefinitely -- never overwrite a non-empty persisted layout with an empty tabs array unless THIS SPECIFIC write was authorized by a genuine user action. `userClosedTabsIntent` is set by matching the literal 'tabs/removeTab' action type (removeTab is dispatched ONLY from the closeTab thunk -- close/close-others/close-to-right all funnel through it, confirmed the sole call site), consumed-and-reset at the top of every flush() so a later, unrelated dirty cycle can't inherit a stale intent. Matched by literal string rather than importing the action creator to avoid a circular import (tabsSlice already imports loadPersistedLayout / markTabsLoadRecovery from this module). A rolling backup (`freshell.layout.v3.bak`, LAYOUT_BACKUP_STORAGE_KEY) is now written before ANY write that would replace a non-empty (or unparseable, hence unprovably-empty) persisted layout with an empty one -- whether the guard refuses the write or a genuine user close authorizes it. This is the last line of defense: even if some future bug slips past the intent check, the prior layout is recoverable. Guard refusals now log at `error` level (previously `warn`) with a structured reason, closing the observability gap that made this incident class unprovable. Tests (test/unit/client/store/persistTabsEmptyGuard.test.ts): - New load-bearing test: a normal (non-recovery) boot followed by tabs emptied via a non-user hydrateTabs cross-tab merge. RED under v1 (the latch never armed for a non-recovery boot, so the write proceeded destructively); GREEN under v2 (guard refuses regardless of boot type, backup written). - Existing transient-parse-failure and genuine-close-all tests updated (without weakening) to also assert the rolling backup is written in both the refused and the authorized-empty-write cases. - Existing non-empty-throughout test extended to assert the backup path is never entered when tabs stay non-empty (no spurious backup writes). 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
…reator The destructive-empty-tabs guard in persistMiddleware.ts sets userClosedTabsIntent by matching the literal string 'tabs/removeTab' (to avoid a circular import with tabsSlice, which already imports loadPersistedLayout/markTabsLoadRecovery from persistMiddleware). Nothing coupled that literal to the real removeTab action creator, so a future rename of removeTab or the tabs slice's name would silently defang the guard with no test failure to catch it. Add a regression test that imports the real removeTab action creator from tabsSlice and asserts removeTab(id).type === 'tabs/removeTab', making the literal-string/action-creator coupling explicit and test-enforced. 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
Root cause investigation update: the originally-reported cause (zTabMode as a `z.enum([...])` rejecting extension-provided modes like 'amplifier') was already fixed on main in commit 46d4ba4 ("fix: keep amplifier tabs restorable (#506)"), which loosened zTabMode/zCodingCliProvider to `z.string().min(1)`. That fix is present on this branch's base and is not reverted here. The underlying architectural defect remains, however: parsePersistedTabsRaw and parsePersistedLayoutRaw validate the entire `tabs` array atomically via `z.array(zTab)`. Any single tab that fails schema validation for ANY reason (a non-string `mode`, a missing `id`, or any future foreign field shape) still causes `safeParse` to fail for the whole payload, and `parsePersistedLayoutRaw` returns null - wiping every other valid tab on the next page load. This adds two defense layers to close that gap: 1. Sanitize: `mode` and `codingCliProvider` are compat-only fields. An invalid value (wrong type, empty string) is now sanitized to `undefined` via `z.preprocess` instead of failing the tab. 2. Salvage: tabs are now validated one at a time (`salvageTabs`) instead of atomically via `z.array(zTab)`. A structurally invalid tab (e.g. missing `id`) is dropped and logged via the client logger; every other valid tab in the same payload survives. Whole-payload `null` is now reserved for genuinely unparseable JSON, a non-object root, or a version newer than this build supports. crossTabSync.ts calls parsePersistedLayoutRaw at three sites (dispatch hydrate, initial local-persisted-at read, broadcast persisted-at read). Traced the merge semantics in tabsSlice's hydrateTabs reducer: a tab dropped by salvage is simply absent from the remote tab list, and hydrateTabs preserves any local tab whose id isn't present in the remote list (as long as it isn't tombstoned). So a corrupt remote tab can never overwrite or erase a good local copy of the same tab - salvage is safe to apply uniformly, with no special-casing needed at the crossTabSync boundary. Tests (RED verified before implementation, including a stash/pop check that the new tabsPersistence.test.ts integration test fails against the pre-fix parser): - persistedState.test.ts: sanitizes a non-string mode without dropping the tab; drops a structurally invalid tab while preserving the rest (parsePersistedTabsRaw and parsePersistedLayoutRaw); confirms whole-payload null is unchanged for bad JSON / future versions. - tabsPersistence.test.ts: integration test through handleUiCommand's tab.create fold + persistMiddleware + parsePersistedLayoutRaw, reproducing the exact "extension sends a malformed mode" flow and confirming all tabs (not just the malformed one) survive reload. Verification: focused suite (9 files / 129 tests) green, full `npm run test:unit` (313 files / 3767 tests) green, `npm run typecheck:client` clean, `npm run lint` clean (0 errors, pre-existing unrelated warnings only). 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
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.
Summary
Incident-fix family from the July 19-20 tab-loss investigations (three production incidents, all root-caused):
userClosedTabsIntentrule), with a rolling.bakbackup and structured recovery logging. Includes a coupling test (6364a4c) pinning the'tabs/removeTab'action string to the creator.mode:'amplifier'against the older strict enum).Test plan
Generated with Amplifier