Hardening wave 2 — 27-H storage: IndexedDB that always settles, autosave that measures itself, and a mic that is given back - #208
Merged
Conversation
The hardening audit's M3, and the half of it this project had already MEASURED from the outside: storageUsage.js's `safeGet` exists because "idb.js settles only on the request's own onsuccess/onerror, so an aborted transaction leaves a promise pending FOREVER". This is the fix that finding was owed. - `tx.onabort` rejects, in all four wrappers. THE TIMING IS THE WHOLE POINT and is why the first version of the test passed for the wrong reason: abort a transaction with a request still in flight and that request errors FIRST, which bubbles to `tx.onerror`, so the old code happened to settle. Abort once every request has succeeded and `onabort` is the only event that fires - that is the case that hung, and it is what the seam now reproduces. - `withTimeout` bounds every operation at 10s. Rule 1 covers the aborts the browser reports; the bound covers the class it does not, where the request object simply never fires again. The error carries `timedOut` so a caller can branch without matching a string, and it logs through 27-B's diagnostics ring. - `open()` is cached, with the cache dropped on `onclose`, on `onversionchange`, on a failed open, and on the `InvalidStateError` a stale handle throws (which `withDb` retries once - that retry is what pays for the cache). Every op used to open its own connection and a storage scan makes a few hundred in a burst. - 10s IS MEASURED, not assumed: a 25MB put - larger than the Explorer's own import cap - takes ~480ms here, so the bound has ~20x headroom over the largest write the app can make. The suite asserts a 5x margin, so a change that makes writes genuinely slow turns red instead of silently failing a user's import. - storageUsage.js's comment said the fix was "still owed"; it now says it landed and why the 5s bounded read stays anyway (a panel must not wait 10s per key). Counterfactuals, each proven by breaking the code and watching the suite: - `tx.onabort` removed -> "an aborted transaction REJECTS rather than hanging" reads `still waiting in 5369ms`, which is the bug verbatim (3 checks red). - the `Promise.race` bound removed -> the stalled transaction reads `still waiting, timedOut=false` after 5263ms (2 checks red). - the `open()` cache removed -> "20 reads reuse one connection" reads `20 new opens`. - unit: the same three properties with no browser, including a `still waiting` race that says what an unbounded await does. Suites: storage-hardening NEW 10/10. Held green: autosave-object-flows, explorer-storage (239s for the pair). Unit 86 tests / 8 files (base 79 / 7). svelte-check 358/47, exactly the committed baseline. Build green with the dev server stopped. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Pcm6oYNMNNBGyUj8VEm5Um
…says when it fails
The audit's M3 (re-entrancy, no quota feedback) and M5 (a full GLTF export of the whole
scene on the main thread every 30s, worst exactly when the scene is biggest).
- ONE SAVE AT A TIME. `markDirty` rescheduled `saveSnapshot` unconditionally and a
snapshot is several awaits long, so on a scene whose export outlasts the debounce
every tick started a FRESH full export while the previous one ran, each parking and
unparking the same objects. A save asked for mid-write is now folded into the one in
flight and scheduled once when it finishes. `saveNow` deliberately does NOT fold - it
is the path whose promise is "it is on disk when I resolve", so it waits its turn.
- THE CADENCE ADAPTS. `exportScene` measures itself and `cadenceFor(ms)` - pure, and
exported so it can be asserted directly - turns that into the wait: 150ms or less
keeps 30s, then it doubles per doubling of the cost to a 5min cap. Derived from ONE
measurement rather than a stateful "double it, halve it", which oscillates. The
3-minute safety-net interval respects it too, or the backoff buys nothing.
- THE PROBE STRINGIFY IS GONE. `JSON.stringify(snapshot).length` serialised everything
and threw it away to learn a number, and then `idbPut` walked the same graph again.
`estimateSnapshotBytes` reads the `.length` of the handful of base64 strings that ARE
the bytes (GLTF buffers/images, animated-import file bytes) and estimates the rest
from counts. MEASURED at 0.010ms against the stringify's 9.0ms on an 8MB snapshot.
- A FAILED AUTOSAVE IS SAID OUT LOUD. A full disk reached `console.log` and stopped
there, so crash recovery had silently switched itself off with nothing to tell the
user - the worst shape a safety feature can fail in. Now a STICKY toast naming what it
means for recovery, carrying "Manage storage", cleared by the next successful save;
the reason also lands in 27-B's diagnostics bundle through a new `autosave` section
(cadence, last cost, last error - the single most useful line in a lost-work report).
`isQuotaError` tests all three spellings; Firefox's is a legacy numeric code.
- Clearing `dirty` is now conditional on `dirtyPulse` not having moved during the
export, the held-body `lastWritten` rule: a change made DURING a save is not in the
bytes that save wrote.
- The Storage panel renders the cadence in words, the last export's cost, and - only
when it has backed off - why. An adaptive interval nobody can see is indistinguishable
from autosave being broken.
Counterfactuals, each proven by breaking the code:
- re-entrancy guard removed -> three ticks during one save write 3 snapshots, 0 coalesced.
- the failure report removed -> all five quota checks red, `lastError` null.
- the cadence frozen at 30s -> "the live cadence is the one that measurement implies"
reads `509ms -> 30000ms`.
- the probe stringify restored -> "at least 20x cheaper" reads 2.270ms vs 2.1ms.
One suite trap worth the line: the panel was opened with a page-side
`import('/src/lib/storageUsage.js')`, which binds a SECOND module instance once vite has
timestamped the app's copy - it passed once and then failed in two counterfactual runs
for a reason that had nothing to do with the counterfactual. It goes through
`window.__stores` now.
Suites: storage-hardening 28/28 (10 -> 28). Held green: autosave-object-flows,
explorer-storage, diagnostics (4 suites, 296s). svelte-check 358/47. Unit 86/86.
Build green with the dev server stopped.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Pcm6oYNMNNBGyUj8VEm5Um
…s it that way
The audit's M4. It counted 136 bare `localStorage.setItem` calls in 25 files; the tree
has grown since, and the real number measured here is 507 call sites across 94 files.
WHY IT MATTERS, in one sentence: `setItem` throws synchronously in Safari private mode
and on a full quota, and most of these sit inside `$effect`s and store subscribers - so
the throw does not merely fail to persist a setting, it KILLS THAT SUBSCRIBER for the
session, and the UI it drives stops updating. The suite reproduces exactly that with the
wrapper removed: toggling a setting in a broken world leaves it stuck at its old value
and raises QuotaExceededError out of the subscriber. Reading is not safe either, which
is less well known - in a sandboxed iframe merely TOUCHING `window.localStorage` throws
SecurityError, which every `typeof localStorage === 'undefined'` guard in this codebase
misses, and there are about a hundred of them.
- `src/lib/safeStorage.js`, a leaf that imports NOTHING (it is reached from stores, from
components and from both sides of the history-cycle family, so any import here is a
future cycle - and it is what lets the unit layer test it with no browser).
get/set/remove per the spec, plus getItem/setItem/removeItem/clear/keys so the codemod
is ONE IDENTIFIER per line - a rename a reviewer can check by eye rather than 507
chances to move a semicolon.
- THE FALLBACK IS PER-KEY, which is what makes the promise honest: a setting whose write
failed is kept in memory, so it still APPLIES this session and reads back as what you
set; it just does not survive a reload. A SUCCESSFUL write drops the shadow again, or
a stale one outvotes the real value forever.
- `keys()` enumerates through `length`/`key(i)` rather than `Object.keys`, the form
dragWindow used: that happens to work on the real Storage exotic object and returns
METHOD NAMES on anything else implementing the interface.
- The codemod, plus two hand cases the regex could not see: units.js's
`const ls = typeof localStorage !== 'undefined' ? localStorage : null` alias, and
dragWindow's `Object.keys(localStorage)` sweep.
- `scripts/check-storage.cjs` + `npm run check:storage`, wired into ci.yml's `check` job.
Without it the codemod decays on the next feature, because the file you are editing
still shows you ninety-three examples of the old way. `src/app.html` is ALLOWED with
its reason spelled out: an inline <script> applying the saved theme before first
paint, which runs before any module exists to import.
- A `storage` diagnostics section, so a bundle says whether persistence is working.
`degraded` is the line worth having - settings applying but not surviving a reload is
otherwise completely invisible, and it is what "my preferences keep resetting" is.
BASELINE RATCHETED 358 -> 357. The codemod removed one pre-existing error for free:
commandsHandler called `localStorage.setItem('showGrid', false)` with a boolean, and
safeStorage takes `any` and coerces the way Storage does. Identified by diffing the full
error sets against a clean checkout, not guessed.
Counterfactuals, each proven by breaking the code:
- the try/catch removed -> 5 checks red, incl. the real bug: a setting toggled while
storage is broken reads `false, raised QuotaExceededError`.
- the memory fallback removed -> "the setting still APPLIES" reads back null.
- a bare `localStorage.setItem` added to viewPrefs.js -> check-storage exits 1 naming
the file and line; removed -> exits 0.
- unit: the no-storage, throwing-setItem and throwing-ACCESS worlds, plus a
side-by-side bare call that does throw in the same world.
Suites: storage-hardening 37/37 (28 -> 37). Held green: autosave-object-flows,
explorer-storage, settings-autorestore-colors, explorer-views, docking, units, packs,
packs-explorer, panel-shortcuts, sessions-packs, workspace-restore (13 suites, 954s).
PRE-EXISTING RED, A/B'd against this branch's own previous commit and failing
identically there: packs-drop (2 checks; the documented drag-drop-simulation cluster).
Unit 98/98 (86 -> 98, 10 files). svelte-check 357/47 against the new floor. Build green
with the dev server stopped.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Pcm6oYNMNNBGyUj8VEm5Um
The audit's M9. Mute only ever set `track.enabled = false`, and nothing in this module has ever called `stop()`. A disabled track is still a LIVE track: the tab keeps its recording indicator, the OS keeps the device claimed so nothing else can open it, and both stay that way for the life of the page after one press. That is a trust problem before it is a resource one - the indicator says "this page is listening" and it is not true. `leaveSession` never touched voice at all, so it survived leaving the session too. - `releaseMic()` stops every track, drops the `self` analyser, and CLOSES THE OUTGOING CALLS. The last part is not tidiness: a MediaConnection carries this stream, and `callPeer` skips a peer that already has one - so leaving a dead channel up would make the next re-acquire reach nobody. Closing means `ensureStream` re-calls everybody, which costs a renegotiation and is the only version that works. INCOMING calls are deliberately left alone: listening never needed a microphone, and turning your own mic off is not a request to stop hearing other people. - Called from: the mic toggle going OFF (immediately - you said so, and the indicator is what you are watching), the VR mic mode reaching 'off', and `leaveSession`. - PUSH-TO-TALK releases after a 3s IDLE GRACE rather than on the keyup. That is the one piece of policy here, and it is there because re-acquiring costs a `getUserMedia` AND a renegotiation with every peer: releasing instantly would make the second sentence of a conversation arrive late. A few seconds of indicator after you stop talking is active use; forever is the bug. - `releaseMic` also clears `micActive`, and THE TWO-PEER SECTION IS WHAT FOUND THAT: with the flag left true and no stream behind it, the toolbar claimed an open mic and the next press was read as "off", so the peer was never called at all. Measured as B seeing `incoming: 0` through a 20s wait. The state has to agree with the device. - THE SPEAKING POLL used to be armed once at init and run at ~7Hz for the life of the tab, with no microphone, no peers and nothing to measure. `syncPoll` arms it only while something is measurable (our stream, or any call) and stands it down otherwise, clearing `speakingPeers` when it does - nobody can be speaking when nothing is measured. - The AudioContext is deliberately NOT closed: `audioEngine` owns it for the whole app since #22 A1, so closing it here would silence music, sounds and pings. Counterfactuals, each proven by breaking the code: - `stop()` swapped back for `enabled = false` -> 5 checks red, reading `{"stream":true,"live":1,"enabled":0}` - a live-but-disabled track, which IS the bug. - the unconditional `setInterval` restored -> "nothing is claimed and nothing is polling" reads `polling:true` with no mic and no peers. - `releaseMic()` removed from `leaveSession` -> the mic survives leaving the session (`live:1, enabled:1`). Suites: storage-hardening 51/51 (37 -> 51, now two peers for section 5). Held green: voice-ptt, spatial-voice, autosave-object-flows, explorer-storage (5 suites, 495s on a freshly restarted server). PRE-EXISTING RED, A/B'd against BOTH this branch's previous commit and the lane base 87c9d72, failing identically on all three: net-reconnect ("B's new object reaches A after the heal"). svelte-check 357/47. Unit 98/98. Build green with the dev server stopped. One method note: after the A/B checkouts above, every suite died in setupPage's `waitForFunction` with `$peers` null inside Scene - the documented mid-session HMR churn, not a regression. A dev-server restart and a curl-grep for a new symbol cleared it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Pcm6oYNMNNBGyUj8VEm5Um
…write Found reviewing my own phase 2 diff (b5898ec). That commit added the right guard - "a change made DURING the export is not in the bytes just written" - and then read `markAtStart = get(dirtyPulse)` immediately before `idbPut`, which is AFTER the GLTF export has already finished. The export is the slow part and therefore the entire window the guard exists for, so as written it compared a stamp taken after the risky period against itself and cleared `dirty` unconditionally in every real case. It is read on the first line of `writeSnapshot` now. Not a lost-work bug in practice - the `markDirty` that raced the save also armed a fresh debounce, so the edit still reached disk 30s later - but `isDirty()` read false in between, and that store is what Settings and the window title's dirty asterisk consult. The honest version of the guard is the one that measures the right window. Suite: two checks in storage-hardening - an edit made while a snapshot is being written stays unsaved, and a quiet save still clears the flag (a guard that only asserted the first half would pass with `dirty` never cleared at all). Counterfactual: the unconditional `dirty = false` restored -> "an edit made while a snapshot is being written stays unsaved" reads `(false)`, 56 of 57. Suites: storage-hardening 57/57 (51 -> 57 checks; the 51 in f731555's body was a miscount - 57 is the measured number). svelte-check 357/47. Unit 98/98. check:storage clean. Build green with the dev server stopped. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Pcm6oYNMNNBGyUj8VEm5Um
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.
Wave 2's first half: phase 27-H storage hardening, five commits stacked on wave 1 (#206) at
87c9d72. Wave 2's other half (26-overload / 27-J) is not started, so this stands alone.What changes
b7f5ec1idb.js:tx.onabortrejects, every op bounded at 10s,open()cached with a stale-handle retryb5898ecautosave: re-entrancy guard, measured export, adaptive cadence, sticky quota toast, probe stringify gonef3d2dd0safeStorageleaf + a 507-site codemod +scripts/check-storage.cjsas a CI gatef731555leaveSession; the poll only runs while there is audio7646fc2Each commit body carries its own counterfactuals with measured numbers.
Evidence
storage-hardening(new): 57/57, five sections, section 5 two-peer.autosave-object-flows,explorer-storage,voice-ptt,spatial-voice,diagnostics, plus the codemod-cover set (settings-autorestore-colors,explorer-views,docking,units,packs,packs-explorer,panel-shortcuts,sessions-packs,workspace-restore,connect-states,explorer-files,animation-window,hud-editor×3,explorer-multiselect,mesh-toolbox-redesign,selection-extras).check-baseline.json.Pre-existing reds, each A/B'd against this branch's own base — none from this lane
packs-drop(the documented drag-drop-simulation cluster) ·net-reconnect("B's new object reaches A after the heal", also red on87c9d72) ·themes("dark is the authored look" — a tailwind-4 CSS output-format drift,oklch(…)where the check wantsrgb(…)).Deviations worth knowing
scripts/check-storage.cjs, not an addition todeps-check.cjs— deps-check is a network-dependent drift report that always exits 0 and CI never runs, so a gate inside it could never fire. The new script is offline, exits 1, names file+line, and sits in ci.yml'scheckjob beside the ratchet (npm run check:storage).debugForceNextTx('quota')seam inidb.jsrather than by filling the disk — a headless origin is granted tens of gigabytes. The seam raises a realDOMException(…, 'QuotaExceededError'), so the whole downstream path runs against the genuine exception.getUserMediaplus a renegotiation with every peer. An explicit voice-off releases immediately. ConstantPTT_IDLE_MS.releaseMic()also clearsmicActive— forced by the two-peer section, which measured the peer never being called because the toolbar claimed an open mic with no stream behind it.audioEnginehas owned it app-wide since Feature/vr-mode #22 A1; closing it here would silence music, sounds and pings.typeof localStorage === 'undefined'guards at call sites were left in place — now redundant and harmless.Owed on device, not fakeable in CI
Storage.prototype.setItem, which is where the browser actually fails).Conflict note
The codemod touched 94 files, two of which the not-yet-started 26-overload lane owns (
Controls.svelte,Toasts.svelte) — one identifier per line, nowhere nearpokeScene/ theloadingbatch. If that lane also ratchetscheck-baseline.json, take the lower number.🤖 Generated with Claude Code