Non-blocking accuracy fix, surfaced by the #229 delta review. The code is correct — this is about a comment that would mislead the next person to touch it.
#229 added if (this.destroyed) return; guards to webSocketMessage, webSocketClose, webSocketError and alarm(). The field doc, commit message and test-file header all justify the absence of behavioral tests by saying each handler's pre-existing if (!vaultName) return short-circuits first, making the guards currently unreachable.
That reasoning is only true for one of the four:
alarm() (~vault-do.ts:2249) — accurate. Reads config straight from storage, which is null after deleteAll(), so :2251 returns. deleteAlarm() also means nothing is scheduled. Genuinely dead.webSocketMessage (~:1804) — goes through ensureStateForWake, whose warm fast-path (~:2016) returns the cached this.config.name without touching storage, and handleDestroy never clears this.config/stateLoaded. So on a warm instance the name is non-null and the null check does not fire. It is unreachable only because destroy closed every socket, so no message can arrive.webSocketClose / webSocketError (~:1838 / ~:1846) — have no !vaultName check at all, and are genuinely reachable post-destroy (closing sockets with 1001 fires the close handler). They are safe because the work is benign: warm-return, a fast-returning rehydration check, and a map delete.
So three different reasons are documented as one. The guards remain correct and worth keeping — the decision not to write a vacuous "nothing bad happens" test was also right.
Why fix it: this is the primitive the account-delete cascade builds on. A future change that clears this.config in handleDestroy, or adds real work to a socket-close path, invalidates the stated invariant while the comment still claims protection. Someone will read "the null check covers us," and for three of these handlers that was never true.
Fix: one-line correction to the doc/comment naming the actual reason per handler — null-check for alarm(), no-sockets-remain for webSocketMessage, benign-work for the close/error pair. Can ride the next PR touching vault-do.ts (PR-2b of the vault-delete train is a natural home).
Non-blocking accuracy fix, surfaced by the #229 delta review. The code is correct — this is about a comment that would mislead the next person to touch it.
#229 added
if (this.destroyed) return;guards towebSocketMessage,webSocketClose,webSocketErrorandalarm(). The field doc, commit message and test-file header all justify the absence of behavioral tests by saying each handler's pre-existingif (!vaultName) returnshort-circuits first, making the guards currently unreachable.That reasoning is only true for one of the four:
alarm()(~vault-do.ts:2249) — accurate. Reads config straight from storage, which is null afterdeleteAll(), so :2251 returns.deleteAlarm()also means nothing is scheduled. Genuinely dead.webSocketMessage(~:1804) — goes throughensureStateForWake, whose warm fast-path (~:2016) returns the cachedthis.config.namewithout touching storage, andhandleDestroynever clearsthis.config/stateLoaded. So on a warm instance the name is non-null and the null check does not fire. It is unreachable only because destroy closed every socket, so no message can arrive.webSocketClose/webSocketError(~:1838 / ~:1846) — have no!vaultNamecheck at all, and are genuinely reachable post-destroy (closing sockets with 1001 fires the close handler). They are safe because the work is benign: warm-return, a fast-returning rehydration check, and a map delete.So three different reasons are documented as one. The guards remain correct and worth keeping — the decision not to write a vacuous "nothing bad happens" test was also right.
Why fix it: this is the primitive the account-delete cascade builds on. A future change that clears
this.configinhandleDestroy, or adds real work to a socket-close path, invalidates the stated invariant while the comment still claims protection. Someone will read "the null check covers us," and for three of these handlers that was never true.Fix: one-line correction to the doc/comment naming the actual reason per handler — null-check for
alarm(), no-sockets-remain forwebSocketMessage, benign-work for the close/error pair. Can ride the next PR touchingvault-do.ts(PR-2b of the vault-delete train is a natural home).