yuris/spacetimedb-defold
1
0
Fork 0
Code Issues 1 Pull requests Projects Releases 9 Packages Wiki Activity Actions

connection: settle handle-free requests when the socket dies #47

Merged
yuris merged 2 commits from feat/41-callback-loss into main 2026-08-20 02:47:45 +00:00
Conversation 1 Commits 2 Files changed 5 +575 -53
yuris commented 2026-08-20 02:00:31 +00:00
Owner
Copy link

Closes #41. Also fixes a worse bug found while auditing c.pending: two concurrent one-off queries delivered the wrong rows to the wrong consumer.

The reported bug

PR #38 made a dying socket settle every in-flight Call handle with connection lost. Procedures and one-off queries got no such treatment because they have no handle — their c.pending entries were dropped when the socket was replaced and the callback was never invoked. A consumer who disabled a button until a procedure returned left it disabled forever, with nothing reported anywhere.

The rule now implemented is: the moment a request stops being answerable, its outcome reaches at least one surface the caller already holds — the Call or Subscription handle if it has one, the (err, ...) callback if it does not.

A socket "going away" is three paths, not one

The first cut of this PR put the sweep in set_send, which is reached only from open(). That covered a socket being replaced and missed a socket being retired: conn.close sets closing = true and schedule_reconnect bails on "closing or not reconnect_enabled", so on db:close(), or on any disconnect with reconnect = false, the sweep never ran at all and #41's own motivating scenario reproduced verbatim:

procedure callback: NEVER CALLED
one_off callback:   NEVER CALLED
Call handle state:  pending (is_pending() still true)
subscribe_query:    state='sent' — neither is_active() nor is_ended()
pending leaked:     4

The sweep is now M.settle_pending(c, replay), called from set_send with replay = true, and from conn.close and the terminal branch of schedule_reconnect with replay = false. Same three paths, measured after:

close() / reconnect=false : procedure "connection lost", one_off "connection lost",
                            Call state=error, sub state=error is_ended()=true, pending 0
reconnect (unchanged)     : procedure "connection lost", one_off "connection lost",
                            Call state=error, sub state=sent (the replay re-issues it), pending 0

Purely additive: M.settle_pending is new, no existing signature or behaviour changed.

The exclusion set is "answered by the replay that is about to happen"

Only one of the three exempt kinds is contingent on a replay, so the list is split in two rather than special-cased on a string:

local ANSWERED_ELSEWHERE = { reducer = true, unsubscribe = true }
local ANSWERED_BY_REPLAY = { subscribe = true }
  • reducer — exempt on both paths. Its outcome is on the Call settled in the same sweep, and the legacy callback's no-fire shape is frozen (#38 asserts it). resubscribe_all never re-issues a reducer call, so a replay has nothing to do with it either way.
  • unsubscribe — exempt on both paths. The set left query_sets when the drop was issued and the server's copy died with the socket, so the drop took effect. That is a statement about the socket dying, not about what replaces it; and the set is out of query_sets, so no replay could resurrect it.
  • subscribe — exempt only under replay. "resubscribe_all re-issues the set under its original id, so the rows come back" is a claim about the future, and it is the only one. After a terminal close nothing re-issues anything, so the silence becomes the lie instead of the report being one.

Still exclusion rather than a whitelist, so a request kind added later is reported without anyone remembering either list exists.

A raising callback no longer costs the other callers their answer

pending is detached before the sweep begins, so a callback that raised out of the loop left every later caller unrecoverable — entries already gone, no later sweep able to find them. Four procedures in flight, the first callback raising: 1 of 4 callbacks reached, 0 entries left, set_send never completed.

Each entry now settles under pcall and the first error is re-raised once the sweep is complete: 4 of 4 reached, and the error still propagates. Considered and rejected:

  • swallow it — hides a consumer bug in the one function whose entire job is to stop failures being silent.
  • emit an event — a new event type to carry what an error already carries, and it would route Lua errors somewhere Lua errors do not go.

Re-raising also keeps the behaviour a single raising callback already had, where the error propagated out of set_send anyway; the change is strictly that the other callers get told first. Structural work is ordered before the sweep — c.send is swapped, closing is set, the socket is disconnected — so a raising callback cannot leave a socket open or a transport unswapped. Later errors are dropped: Lua 5.1 carries one error value, and the first is the one whose cause is still live.

Reentrancy is unchanged and covered: a callback that calls db:close() or issues a new request mid-sweep still lets the sweep finish, and the re-issued request survives it.

Three correlated handlers were missing the p.kind check their siblings enforce

InitialSubscription and TransactionUpdate guarded on p.kind; subscription_applied, SubscriptionError and ProcedureResult took whatever was parked under that request_id. A ProcedureResult bearing a reducer's id fired the frozen (err, update) callback with 'not a TransactionUpdate' as update and consumed the Call's entry, orphaning it so no later settle could reach it and is_pending() lied for good.

Only reachable if the server misroutes an id, but it is the exact bug class this PR set out to fix. One clause each — except SubscriptionError, which takes subscribe or unsubscribe: a live 2.8.2 server answers UnsubscribeMulti with SubscriptionError for an unknown query_id and for a double unsubscribe, both already covered by a test. Narrowing it to subscribe alone leaks the entry and swallows the unsubscribe callback.

An unmatched OneOffQueryResponse was the one handler that could emit nothing

ProcedureResult emits whether or not it matched, and an unknown tag emits unhandled, so this was the only correlated handler that could consume a whole frame silently — every misroute, unknown echo, uppercase echo and double answer vanished, in a file whose documented posture is that failures are loud or absent. The emit moved outside if p then, carrying message_id and a matched flag as one_off_result / one_off_failed.

That also subsumes the case-fold question. Deleting the fold is now safe rather than merely tidy: an uppercase echo surfaces immediately as matched = false instead of hanging a consumer.

The design decision, and why the other option was rejected

Option 2 (settle the plain callbacks) over Option 1 (give procedures and one-off queries handles). Option 1 would freeze two more public types into 1.0 to carry an outcome that (err, value, msg) and (err, tables) already express — both requests are one-shot with exactly one result. The rule is "every terminal outcome reaches at least one surface the caller holds", not "every request grows a handle".

call_reducer's legacy callback still does not fire, and #38's assertion is unchanged and passing. The earlier phrasing of why — "the asymmetry is between mechanisms, not between what a caller can find out" — does not survive testing and has been corrected in the file and the README. A caller who used only the legacy callback and discarded the returned Call learns nothing about their specific request, only that the connection dropped, through a connection-level event that names no request. The honest statement is that the reducer's outcome is reachable, on a surface the same call already returned; reachable is not delivered, and that difference is the whole point of #41. The freeze is still right — firing the callback with a nil update would break consumers — so the README now says to keep the Call if you need to know about your own request.

The bug found by the audit

one_off_query keyed c.pending on c.last_one_off — "the query sent last". With two in flight at once:

  • the first response was dispatched to the second caller's callback — wrong rows to the wrong consumer, not merely a leak
  • the second response reached nobody
  • one entry leaked for the connection's lifetime

Now keyed on the echoed message_id; c.last_one_off is deleted.

Verified on the wire, since the new correlation depends on it: [0,0,1,42] and [0,0,0,255] in flight on one socket came back as "0000012a" and "000000ff" — %02x per byte, lowercase, no prefix. Independently re-verified more broadly during review: [222,173,190,239] echoes deadbeef (all letters, still lowercase), it is hex::encode of exactly the bytes sent at any length, a failing query echoes its own id too, and message_id arrives as a string in every case.

Also

  • conn.ws was assigned in init.lua and read nowhere in stdb/ or dev/. Deleted.
  • The ~30-line sweep docblock restated this PR body; trimmed to the invariant and the two exclusion lists.
  • README: the claim that the callback fires "when the socket dies under them" was unconditionally false on the close path. Now true, and scoped — plus the reachable-vs-delivered distinction, the query-set rule, and the five event types the events table never listed (unsubscribed, procedure_result / procedure_failed, one_off_result / one_off_failed, unhandled).

Left alone, with evidence:

  • duplicate message_id — live 2.8.2 answers both one-offs sent under the same id, and the second write would overwrite the first's entry. Not reachable through the public API: allocate_request_id is monotonic and never reset, including across reconnects (verified: two one-offs get ids 1 and 2 and park separately). The unmatched path above now reports it if a server ever does echo a duplicate.
  • request_id >= 2^32 aliases the one-off key — verified: 4294967295 -> ffffffff, 4294967296 -> 00000000, aliasing id 0. Needs 4.3e9 requests on one connection, and the general form (request_id above u32 on the wire) is pre-existing across every request kind.
  • the type(msg.message_id) == "string" guard — dead for absence, because decode_server already raises OneOffQueryResponse missing required field "message_id" first, correctly loud. It earns its keep for a non-string: a numeric message_id passes decode and now emits one_off_result with matched = false instead of vanishing. Kept.

Verification

6 passed, 0 failed; stylua --check exit 0; conformance PASS twice back-to-back on isolated port 4500 with a private STDB_STATE_DIR, no #39 phase-2 flake.

Mutation-tested, 16 mutants, 16/16 die:

mutation failure
close() no longer settles close() reaches a procedure's callback: expected connection lost, got unset
reconnect = false bail no longer settles reconnect = false settles on the socket's death: expected connection lost, got unset
replay distinction removed (subscribe always exempt) a subscribe IS told, because no replay is coming: expected connection lost, got unset
replay distinction inverted (subscribe never exempt) a subscribe is not failed: resubscribe_all re-issues the same set: expected false, got true
reducer dropped from the exclusion set trips #38's own pre-existing assertion: callback for the dead socket never fires: expected false, got true
unsubscribe dropped from the exclusion set nor an unsubscribe: the set was dropped when the drop was issued: expected false, got true
pcall removed from the sweep and every other caller is settled anyway, not abandoned mid-sweep: expected 4, got 1
the raised error swallowed instead of re-raised the raised error still reaches whoever asked for the close: expected error, call succeeded
last error kept instead of the first the first one, unwrapped exactly as the callback raised it: expected raised by callback 1, got raised by callback 4
subscription handle no longer failed on a terminal close and its handle's on_error: expected connection lost, got unset
is_ended guard dropped, so an ended set is resurrected a terminal close leaves it ended rather than re-marking it failed: expected ended, got error
ProcedureResult kind guard removed a ProcedureResult carrying a reducer's id does not fire the reducer callback: expected false, got true
subscription_applied kind guard removed an applied frame carrying a procedure's id leaves the procedure alone: expected unset, got nil
SubscriptionError narrowed to subscribe only an error answering an unsubscribe still reaches its callback: expected true, got false
one-off emit moved back inside if p a response nobody is waiting for is still emitted: expected 1, got 0
matched hardcoded true flagged as matching no request: expected false, got true

The seven mutants from the first round of this PR (settle removed, c.pending cleared after the sweep instead of detached first, one-off send key made constant, response side ignoring the echoed message_id, ProcedureResult not clearing its entry, OneOffQueryResponse not clearing its entry) still die with their original messages. The one that previously survived — case-folding the one-off key — no longer applies: the fold is deleted, and the unmatched path is what makes an uppercase echo observable.

Closes #41. **Also fixes a worse bug found while auditing `c.pending`: two concurrent one-off queries delivered the wrong rows to the wrong consumer.** ## The reported bug PR #38 made a dying socket settle every in-flight `Call` handle with `connection lost`. Procedures and one-off queries got no such treatment because they have no handle — their `c.pending` entries were dropped when the socket was replaced and the callback was never invoked. A consumer who disabled a button until a procedure returned left it disabled forever, with nothing reported anywhere. The rule now implemented is: **the moment a request stops being answerable, its outcome reaches at least one surface the caller already holds** — the `Call` or `Subscription` handle if it has one, the `(err, ...)` callback if it does not. ## A socket "going away" is three paths, not one The first cut of this PR put the sweep in `set_send`, which is reached only from `open()`. That covered a socket being **replaced** and missed a socket being **retired**: `conn.close` sets `closing = true` and `schedule_reconnect` bails on "closing or not reconnect_enabled", so on `db:close()`, or on any disconnect with `reconnect = false`, the sweep never ran at all and #41's own motivating scenario reproduced verbatim: ``` procedure callback: NEVER CALLED one_off callback: NEVER CALLED Call handle state: pending (is_pending() still true) subscribe_query: state='sent' — neither is_active() nor is_ended() pending leaked: 4 ``` The sweep is now `M.settle_pending(c, replay)`, called from `set_send` with `replay = true`, and from `conn.close` and the terminal branch of `schedule_reconnect` with `replay = false`. Same three paths, measured after: ``` close() / reconnect=false : procedure "connection lost", one_off "connection lost", Call state=error, sub state=error is_ended()=true, pending 0 reconnect (unchanged) : procedure "connection lost", one_off "connection lost", Call state=error, sub state=sent (the replay re-issues it), pending 0 ``` Purely additive: `M.settle_pending` is new, no existing signature or behaviour changed. ## The exclusion set is "answered by the replay that is about to happen" Only one of the three exempt kinds is contingent on a replay, so the list is split in two rather than special-cased on a string: ```lua local ANSWERED_ELSEWHERE = { reducer = true, unsubscribe = true } local ANSWERED_BY_REPLAY = { subscribe = true } ``` - **`reducer`** — exempt on *both* paths. Its outcome is on the `Call` settled in the same sweep, and the legacy callback's no-fire shape is frozen (#38 asserts it). `resubscribe_all` never re-issues a reducer call, so a replay has nothing to do with it either way. - **`unsubscribe`** — exempt on *both* paths. The set left `query_sets` when the drop was issued and the server's copy died with the socket, so the drop took effect. That is a statement about the socket dying, not about what replaces it; and the set is out of `query_sets`, so no replay could resurrect it. - **`subscribe`** — exempt *only* under `replay`. "`resubscribe_all` re-issues the set under its original id, so the rows come back" is a claim about the future, and it is the only one. After a terminal close nothing re-issues anything, so the silence becomes the lie instead of the report being one. Still exclusion rather than a whitelist, so a request kind added later is reported without anyone remembering either list exists. ## A raising callback no longer costs the other callers their answer `pending` is detached before the sweep begins, so a callback that raised out of the loop left every later caller **unrecoverable** — entries already gone, no later sweep able to find them. Four procedures in flight, the first callback raising: 1 of 4 callbacks reached, 0 entries left, `set_send` never completed. Each entry now settles under `pcall` and the **first** error is re-raised once the sweep is complete: 4 of 4 reached, and the error still propagates. Considered and rejected: - **swallow it** — hides a consumer bug in the one function whose entire job is to stop failures being silent. - **emit an event** — a new event type to carry what an error already carries, and it would route Lua errors somewhere Lua errors do not go. Re-raising also keeps the behaviour a single raising callback already had, where the error propagated out of `set_send` anyway; the change is strictly that the other callers get told first. Structural work is ordered before the sweep — `c.send` is swapped, `closing` is set, the socket is disconnected — so a raising callback cannot leave a socket open or a transport unswapped. Later errors are dropped: Lua 5.1 carries one error value, and the first is the one whose cause is still live. Reentrancy is unchanged and covered: a callback that calls `db:close()` or issues a new request mid-sweep still lets the sweep finish, and the re-issued request survives it. ## Three correlated handlers were missing the `p.kind` check their siblings enforce `InitialSubscription` and `TransactionUpdate` guarded on `p.kind`; `subscription_applied`, `SubscriptionError` and `ProcedureResult` took whatever was parked under that `request_id`. A `ProcedureResult` bearing a reducer's id fired the frozen `(err, update)` callback with `'not a TransactionUpdate'` as `update` **and** consumed the `Call`'s entry, orphaning it so no later settle could reach it and `is_pending()` lied for good. Only reachable if the server misroutes an id, but it is the exact bug class this PR set out to fix. One clause each — except `SubscriptionError`, which takes `subscribe` **or** `unsubscribe`: a live 2.8.2 server answers `UnsubscribeMulti` with `SubscriptionError` for an unknown `query_id` and for a double unsubscribe, both already covered by a test. Narrowing it to `subscribe` alone leaks the entry and swallows the unsubscribe callback. ## An unmatched `OneOffQueryResponse` was the one handler that could emit nothing `ProcedureResult` emits whether or not it matched, and an unknown tag emits `unhandled`, so this was the only correlated handler that could consume a whole frame silently — every misroute, unknown echo, uppercase echo and double answer vanished, in a file whose documented posture is that failures are loud or absent. The emit moved outside `if p then`, carrying `message_id` and a `matched` flag as `one_off_result` / `one_off_failed`. That also subsumes the case-fold question. Deleting the fold is now safe rather than merely tidy: an uppercase echo surfaces immediately as `matched = false` instead of hanging a consumer. ## The design decision, and why the other option was rejected **Option 2 (settle the plain callbacks) over Option 1 (give procedures and one-off queries handles).** Option 1 would freeze two more public types into 1.0 to carry an outcome that `(err, value, msg)` and `(err, tables)` already express — both requests are one-shot with exactly one result. The rule is *"every terminal outcome reaches at least one surface the caller holds"*, not *"every request grows a handle"*. **`call_reducer`'s legacy callback still does not fire**, and #38's assertion is unchanged and passing. The earlier phrasing of why — "the asymmetry is between *mechanisms*, not between what a caller can find out" — does not survive testing and has been corrected in the file and the README. A caller who used only the legacy callback and discarded the returned `Call` learns nothing about their specific request, only that the connection dropped, through a connection-level event that names no request. The honest statement is that the reducer's outcome is **reachable**, on a surface the same call already returned; *reachable is not delivered*, and that difference is the whole point of #41. The freeze is still right — firing the callback with a nil `update` would break consumers — so the README now says to keep the `Call` if you need to know about your own request. ## The bug found by the audit `one_off_query` keyed `c.pending` on `c.last_one_off` — "the query sent last". With two in flight at once: - the first response was dispatched to the **second** caller's callback — wrong rows to the wrong consumer, not merely a leak - the second response reached nobody - one entry leaked for the connection's lifetime Now keyed on the echoed `message_id`; `c.last_one_off` is deleted. **Verified on the wire**, since the new correlation depends on it: `[0,0,1,42]` and `[0,0,0,255]` in flight on one socket came back as `"0000012a"` and `"000000ff"` — `%02x` per byte, lowercase, no prefix. Independently re-verified more broadly during review: `[222,173,190,239]` echoes `deadbeef` (all letters, still lowercase), it is `hex::encode` of exactly the bytes sent at any length, a *failing* query echoes its own id too, and `message_id` arrives as a string in every case. ## Also - `conn.ws` was assigned in `init.lua` and read nowhere in `stdb/` or `dev/`. Deleted. - The ~30-line sweep docblock restated this PR body; trimmed to the invariant and the two exclusion lists. - README: the claim that the callback fires "when the socket dies under them" was unconditionally false on the close path. Now true, and scoped — plus the reachable-vs-delivered distinction, the query-set rule, and the five event types the events table never listed (`unsubscribed`, `procedure_result` / `procedure_failed`, `one_off_result` / `one_off_failed`, `unhandled`). Left alone, with evidence: - **duplicate `message_id`** — live 2.8.2 answers both one-offs sent under the same id, and the second write would overwrite the first's entry. Not reachable through the public API: `allocate_request_id` is monotonic and never reset, including across reconnects (verified: two one-offs get ids 1 and 2 and park separately). The unmatched path above now reports it if a server ever does echo a duplicate. - **`request_id` >= 2^32 aliases the one-off key** — verified: `4294967295` -> `ffffffff`, `4294967296` -> `00000000`, aliasing id 0. Needs 4.3e9 requests on one connection, and the general form (request_id above u32 on the wire) is pre-existing across every request kind. - **the `type(msg.message_id) == "string"` guard** — dead for absence, because `decode_server` already raises `OneOffQueryResponse missing required field "message_id"` first, correctly loud. It earns its keep for a non-string: a numeric `message_id` passes decode and now emits `one_off_result` with `matched = false` instead of vanishing. Kept. ## Verification `6 passed, 0 failed`; `stylua --check` exit 0; conformance PASS twice back-to-back on isolated port 4500 with a private `STDB_STATE_DIR`, no #39 phase-2 flake. Mutation-tested, 16 mutants, **16/16 die**: | mutation | failure | |---|---| | `close()` no longer settles | `close() reaches a procedure's callback: expected connection lost, got unset` | | `reconnect = false` bail no longer settles | `reconnect = false settles on the socket's death: expected connection lost, got unset` | | replay distinction removed (`subscribe` always exempt) | `a subscribe IS told, because no replay is coming: expected connection lost, got unset` | | replay distinction inverted (`subscribe` never exempt) | `a subscribe is not failed: resubscribe_all re-issues the same set: expected false, got true` | | `reducer` dropped from the exclusion set | trips #38's own pre-existing assertion: `callback for the dead socket never fires: expected false, got true` | | `unsubscribe` dropped from the exclusion set | `nor an unsubscribe: the set was dropped when the drop was issued: expected false, got true` | | `pcall` removed from the sweep | `and every other caller is settled anyway, not abandoned mid-sweep: expected 4, got 1` | | the raised error swallowed instead of re-raised | `the raised error still reaches whoever asked for the close: expected error, call succeeded` | | last error kept instead of the first | `the first one, unwrapped exactly as the callback raised it: expected raised by callback 1, got raised by callback 4` | | subscription handle no longer failed on a terminal close | `and its handle's on_error: expected connection lost, got unset` | | `is_ended` guard dropped, so an ended set is resurrected | `a terminal close leaves it ended rather than re-marking it failed: expected ended, got error` | | `ProcedureResult` kind guard removed | `a ProcedureResult carrying a reducer's id does not fire the reducer callback: expected false, got true` | | `subscription_applied` kind guard removed | `an applied frame carrying a procedure's id leaves the procedure alone: expected unset, got nil` | | `SubscriptionError` narrowed to `subscribe` only | `an error answering an unsubscribe still reaches its callback: expected true, got false` | | one-off emit moved back inside `if p` | `a response nobody is waiting for is still emitted: expected 1, got 0` | | `matched` hardcoded true | `flagged as matching no request: expected false, got true` | The seven mutants from the first round of this PR (settle removed, `c.pending` cleared after the sweep instead of detached first, one-off send key made constant, response side ignoring the echoed `message_id`, `ProcedureResult` not clearing its entry, `OneOffQueryResponse` not clearing its entry) still die with their original messages. The one that previously survived — case-folding the one-off key — no longer applies: the fold is deleted, and the unmatched path is what makes an uppercase echo observable.
connection: settle handle-free requests when the socket dies (#41)
All checks were successful
ci / ci (pull_request) Successful in 19s
Details
861d0afae2 
PR #38 made a dead socket settle every in-flight Call, but call_procedure and
one_off_query have no handle, so their c.pending entries were dropped with the
old socket and the callback was never invoked. A consumer that disabled a
button until a procedure returned left it disabled forever, with nothing
reported anywhere.

set_send now invokes the plain callback with ("connection lost") for every
remaining pending request except the three kinds whose outcome is already
reachable: reducer (its Call settles, and the legacy callback's no-fire shape is
frozen), subscribe (nothing terminal happened - resubscribe_all re-issues the
set under its original id) and unsubscribe (the set was dropped the moment the
drop was issued). Excluding rather than whitelisting keeps it default-loud: a
request kind added later is reported without anyone having to remember the list.

Handles for procedures and one-off queries - the other option on the issue -
were rejected. They would freeze two more public types into 1.0 to carry an
outcome the existing (err, ...) callback already expresses, since both requests
are one-shot with exactly one result. The rule kept is "every terminal outcome
reaches at least one surface the caller holds", not "every request grows a
handle". That is also why call_reducer's legacy callback still does not fire -
its outcome is on the Call the same function returns - and why this settle would
stay if procedures ever did grow a handle: taking it away would be the breaking
change.

pending is detached before the sweep rather than cleared after it, so a callback
that re-issues its request on the fresh socket - c.send is already the new one -
does not have its retry wiped by this function's own cleanup.

Found while auditing every write of c.pending against every clear: one-off
queries correlated on c.last_one_off, i.e. "the query sent last". Two in flight
at once crossed their answers - the first response reached the second caller's
callback, the second reached nobody, and one entry leaked for the life of the
connection. They now key on the message_id the server echoes back, which is what
that field is for. Verified against live 2.8.2: two concurrent one-offs come
back as "0000012a" and "000000ff", the hex of exactly the bytes sent.
yuris commented 2026-08-20 02:13:22 +00:00
Author
Owner
Copy link

Review — automated reviewer, posted for the record

Verdict: request changes

Closes #41: partially — reconnect path only; issue #41's own motivating harm still reproduces on the close path

The one-off correlation fix is correct and its wire premise independently re-verified (and holds more broadly than the author tested). The #41 fix is right in mechanism but incomplete in reach: the sweep fires only when a socket is REPLACED, never when one is RETIRED. 2 blockers, 4 should-fixes, 4 nits. Blockers 1+2 share one ~15-line additive fix.

Sanity

  • suite — 6 passed, 0 failed
  • stylua — --check stdb/ dev/test/ exit 0
  • conformance — PASS twice back-to-back on isolated port 4400; no #39 phase-2 flake observed
  • mutants — 7/7 claimed mutants die with the EXACT reported messages; mutant 8 (case-fold) survives as claimed. Mutant 2 confirmed to trip #38's pre-existing assertion at test_connection.lua:202 ('callback for the dead socket never fires: expected false, got true'). Mutation table is accurate.
  • ports — used only 4400 with private STDB_STATE_DIR=/tmp/stdb-review47; 3000 and 4335 untouched; server stopped; checkout left clean

c.pending kind enumeration

site kind handle swept
connection.lua:300 send_query_set subscribe p.sub false
connection.lua:318 legacy M.subscribe subscribe — false
connection.lua:379 unsubscribe unsubscribe — false
connection.lua:405 call_reducer reducer p.call false
connection.lua:423 call_procedure procedure — true
connection.lua:461 one_off_query one_off — true
  • inverse check: nothing is swept that should not be
  • double fire: structurally impossible — reducer is the only kind carrying p.call and it is excluded from the callback branch

Blockers

Blocker 1 — db:close() and reconnect=false never sweep at all

stdb/init.lua:258-263, stdb/init.lua:206, stdb/init.lua:218, stdb/connection.lua:562

Problem: set_send is the sweep's only caller and is only reached from open(). conn.close sets closing=true and disconnects; schedule_reconnect bails on 'closing or not reconnect_enabled'. So on a terminal close, or any disconnect with reconnect=false, the sweep never executes.

Evidence:

procedure callback: NEVER CALLED; one_off callback: NEVER CALLED; Call handle state: pending (is_pending() still true); pending leaked: 3
  • #41's motivating scenario reproduces verbatim — button disabled until a procedure returns, stuck forever, nothing reported
  • falsifies connection.lua:245's claim that is_pending() is 'honest in every case'
  • answers the review's subscribe question: a subscribe_query in flight when the connection is CLOSED is left state='sent' — neither is_active() nor is_ended() — with on_applied and on_error both permanently silent

Fix: Extract the sweep into M.settle_pending(c, replay) and call it from conn.close too. The exclusion set is really 'answered by the replay that is about to happen', so it must depend on whether one follows: answered = ANSWERED_ELSEWHERE[p.kind] and (replay or p.kind ~= 'subscribe'). Purely additive, no API change.

Blocker 2 — one raising settled callback abandons every later settle, unrecoverably

stdb/connection.lua:570-577, stdb/connection.lua:568

Problem: Four procedures in flight, first callback raises: only 1 of 4 callbacks reached, set_send did not complete, and the other three callers are unrecoverable because pending was already detached at :568 — no later sweep can find them.

Evidence:

callbacks reached (want 4): 1; pending after (entries gone): 0; set_send completed: false

Fix: pcall each callback inside the sweep, save the first error, re-raise after the loop completes. Every caller is told and the error still propagates loudly.

should fix

[
{
"id": 3,
"title": "three handlers skip the p.kind check their siblings enforce",
"refs": [
"stdb/connection.lua:655",
"stdb/connection.lua:757",
"stdb/connection.lua:810"
],
"problem": "InitialSubscription (:622) and TransactionUpdate (:702/:713) guard on p.kind; subscription_applied, SubscriptionError and ProcedureResult take whatever is parked under that request_id.",
"evidence": "A ProcedureResult bearing a reducer's request_id: legacy reducer callback fired: true (received update = 'not a TransactionUpdate'); Call state: pending; pending entry consumed: 0",
"consequences": [
"the frozen (err, update) callback is handed a non-TransactionUpdate as update",
"the Call is orphaned — its entry is gone, so set_send can never settle it and is_pending() lies forever"
],
"reachability": "pre-existing; only reachable if the server misroutes an id, since request ids are unique per connection across all kinds. But it is the exact bug class this PR set out to fix, in the file this PR is auditing.",
"fix": "One clause each: p.kind == 'procedure' at :810; p.kind == 'subscribe' or 'unsubscribe' at :655; p.kind == 'subscribe' at :757."
},
{
"id": 4,
"title": "an unmatched OneOffQueryResponse reports nothing at all",
"refs": [
"stdb/connection.lua:780-794"
],
"problem": "The only correlated handler that can consume a frame and emit nothing. ProcedureResult:817 emits unconditionally whether or not p was found; handle_frame:833 emits 'unhandled' for an unknown tag. So every misroute, unknown echo, uppercase echo or double-answer vanishes without trace — in a file whose documented posture is that failures are loud or absent.",
"evidence": "events emitted for an id nobody sent: 0; original query still parked: 1",
"fix": "Mirror ProcedureResult: move the emit outside the 'if p then', carrying message_id and a matched flag.",
"note": "This subsumes the case-fold question entirely — with a loud unmatched path an uppercase echo surfaces immediately instead of hanging a consumer."
},
{
"id": 5,
"title": "README overclaims the settle",
"refs": [
"README.md:389-391",
"README.md:787-788"
],
"problem": "'it does fire, with ("connection lost"), when the socket dies under them' is unconditionally false on the close path (blocker 1). Same for both changed table rows.",
"fix": "Either fix blocker 1 so the prose becomes true, or scope it to 'when the socket dies and the SDK reconnects'. Also add the reachable-vs-delivered distinction, and note that a consumer relying solely on call_reducer's callback should keep the returned Call."
},
{
"id": 6,
"title": "sweep docblock restates the PR body",
"refs": [
"stdb/connection.lua:506-556"
],
"problem": "~30 lines of rationale in-file. Worth trimming to the invariant plus the exclusion table — the argument belongs in the PR, the invariant belongs in the file.",
"severity_note": "style/maintenance, lowest of the should-fixes"
}
]

nits

[
{
"title": "duplicate message_id silently overwrites",
"detail": "Live 2.8.2 answers BOTH one-offs sent under the same message_id, each echoing the same id (my wire TEST B). In the SDK the second write at :461 overwrites the first's entry: caller 1's callback dropped, caller 1's rows delivered to caller 2, second response finds nothing. Not reachable via the public API — allocate_request_id (:45-49) is monotonic and never reset, including across reconnects — but the server does not dedupe."
},
{
"title": "request_id >= 2^32 aliases the one-off key",
"detail": "The key is 4 bytes, so request_id 4294967296 produces oneoff:00000000, aliasing id 0. Verified: ids 4294967295/4294967296 -> keys oneoff:ffffffff, oneoff:00000000. Needs 4.3e9 requests on one connection. The general form (request_id above u32 on the wire) is pre-existing across every request kind."
},
{
"title": "type(msg.message_id) == 'string' guard is dead for absence",
"detail": "protocol.decode_server already raises 'OneOffQueryResponse missing required field "message_id"' before the handler runs — correctly loud. The guard earns its keep only for a non-string, e.g. a numeric message_id, which passes decode and is correctly ignored."
},
{
"title": "conn.ws is assigned and never read",
"detail": "init.lua:212 assigns it; grep finds no reader anywhere in stdb/ or dev/. Dead as written."
}
]

wire verification

{
"method": "own isolated SpacetimeDB 2.8.2 on port 4400, raw Node WebSocket client, v1.json.spacetimedb subprotocol",
"result": "CONFIRMED, and more broadly than the PR tested",
"observations": [
"sent [0,0,1,42] -> echo "0000012a"; sent [0,0,0,255] -> echo "000000ff"; sent [222,173,190,239] -> echo "deadbeef"",
"all lowercase: true; all match /^[0-9a-f]{8}$/: true; three concurrent one-offs all answered",
"deadbeef is the decisive case — all letters, echoed lowercase",
"it is hex::encode of EXACTLY the bytes sent, any length: [1]->"01", []->"", 16 bytes->32 chars",
"a FAILING query echoes its own id too, so the error path is correlated correctly",
"message_id arrives as typeof string in every case"
],
"conclusion": "the re-key at :461/:783 is right and the big-endian arithmetic at :456-459 matches the echo"
}

reentrancy

{
"issues_new_request": {
"sweep_continues": true,
"pending_consistent": true,
"later_skipped": false
},
"calls_close": {
"sweep_continues": true,
"detail": "all 3 of 3 callbacks fired",
"pending_consistent": true,
"later_skipped": false,
"judgment": "correct — each caller deserves its answer even after someone asked to close"
},
"raises": {
"sweep_continues": false,
"pending_consistent": "detached, entries lost",
"later_skipped": "YES, unrecoverably",
"finding": "blocker 2 — the reviewer's instinct was correct"
}
}

frozen contract judgment

{
"verdict": "the asymmetry is the right thing to freeze, but the stated justification does not survive testing",
"tested_claim": "'between mechanisms, not between what a caller can find out' is FALSE as written — a caller who used only call_reducer's legacy callback and discarded the returned Call learns nothing about their specific request, only that the connection dropped, via a connection-level event that names no request",
"defensible_restatement": "the reducer's outcome is REACHABLE, on a surface the same call already returned. Reachable is not delivered, and that difference is the whole point of #41.",
"freeze_is_still_correct": "firing the legacy callback with a nil update would break consumers, and test_connection.lua:202 guards a real contract",
"option_1_rejection": "also right — two new public types to carry what (err, ...) already says, on the PR that freezes the API, is a bad trade"
}

## Review — automated reviewer, posted for the record **Verdict:** request changes **Closes #41:** partially — reconnect path only; issue #41's own motivating harm still reproduces on the close path The one-off correlation fix is correct and its wire premise independently re-verified (and holds more broadly than the author tested). The #41 fix is right in mechanism but incomplete in reach: the sweep fires only when a socket is REPLACED, never when one is RETIRED. 2 blockers, 4 should-fixes, 4 nits. Blockers 1+2 share one ~15-line additive fix. ### Sanity - **suite** — 6 passed, 0 failed - **stylua** — --check stdb/ dev/test/ exit 0 - **conformance** — PASS twice back-to-back on isolated port 4400; no #39 phase-2 flake observed - **mutants** — 7/7 claimed mutants die with the EXACT reported messages; mutant 8 (case-fold) survives as claimed. Mutant 2 confirmed to trip #38's pre-existing assertion at test_connection.lua:202 ('callback for the dead socket never fires: expected false, got true'). Mutation table is accurate. - **ports** — used only 4400 with private STDB_STATE_DIR=/tmp/stdb-review47; 3000 and 4335 untouched; server stopped; checkout left clean ### `c.pending` kind enumeration | site | kind | handle | swept | |---|---|---|---| | `connection.lua:300 send_query_set` | subscribe | p.sub | false | | `connection.lua:318 legacy M.subscribe` | subscribe | — | false | | `connection.lua:379 unsubscribe` | unsubscribe | — | false | | `connection.lua:405 call_reducer` | reducer | p.call | false | | `connection.lua:423 call_procedure` | procedure | — | true | | `connection.lua:461 one_off_query` | one_off | — | true | - inverse check: nothing is swept that should not be - double fire: structurally impossible — reducer is the only kind carrying p.call and it is excluded from the callback branch ### Blockers #### Blocker 1 — db:close() and reconnect=false never sweep at all `stdb/init.lua:258-263`, `stdb/init.lua:206`, `stdb/init.lua:218`, `stdb/connection.lua:562` **Problem:** set_send is the sweep's only caller and is only reached from open(). conn.close sets closing=true and disconnects; schedule_reconnect bails on 'closing or not reconnect_enabled'. So on a terminal close, or any disconnect with reconnect=false, the sweep never executes. **Evidence:** ``` procedure callback: NEVER CALLED; one_off callback: NEVER CALLED; Call handle state: pending (is_pending() still true); pending leaked: 3 ``` - #41's motivating scenario reproduces verbatim — button disabled until a procedure returns, stuck forever, nothing reported - falsifies connection.lua:245's claim that is_pending() is 'honest in every case' - answers the review's subscribe question: a subscribe_query in flight when the connection is CLOSED is left state='sent' — neither is_active() nor is_ended() — with on_applied and on_error both permanently silent **Fix:** Extract the sweep into M.settle_pending(c, replay) and call it from conn.close too. The exclusion set is really 'answered by the replay that is about to happen', so it must depend on whether one follows: answered = ANSWERED_ELSEWHERE[p.kind] and (replay or p.kind ~= 'subscribe'). Purely additive, no API change. #### Blocker 2 — one raising settled callback abandons every later settle, unrecoverably `stdb/connection.lua:570-577`, `stdb/connection.lua:568` **Problem:** Four procedures in flight, first callback raises: only 1 of 4 callbacks reached, set_send did not complete, and the other three callers are unrecoverable because pending was already detached at :568 — no later sweep can find them. **Evidence:** ``` callbacks reached (want 4): 1; pending after (entries gone): 0; set_send completed: false ``` **Fix:** pcall each callback inside the sweep, save the first error, re-raise after the loop completes. Every caller is told and the error still propagates loudly. ### should fix [ { "id": 3, "title": "three handlers skip the p.kind check their siblings enforce", "refs": [ "stdb/connection.lua:655", "stdb/connection.lua:757", "stdb/connection.lua:810" ], "problem": "InitialSubscription (:622) and TransactionUpdate (:702/:713) guard on p.kind; subscription_applied, SubscriptionError and ProcedureResult take whatever is parked under that request_id.", "evidence": "A ProcedureResult bearing a reducer's request_id: legacy reducer callback fired: true (received update = 'not a TransactionUpdate'); Call state: pending; pending entry consumed: 0", "consequences": [ "the frozen (err, update) callback is handed a non-TransactionUpdate as update", "the Call is orphaned — its entry is gone, so set_send can never settle it and is_pending() lies forever" ], "reachability": "pre-existing; only reachable if the server misroutes an id, since request ids are unique per connection across all kinds. But it is the exact bug class this PR set out to fix, in the file this PR is auditing.", "fix": "One clause each: p.kind == 'procedure' at :810; p.kind == 'subscribe' or 'unsubscribe' at :655; p.kind == 'subscribe' at :757." }, { "id": 4, "title": "an unmatched OneOffQueryResponse reports nothing at all", "refs": [ "stdb/connection.lua:780-794" ], "problem": "The only correlated handler that can consume a frame and emit nothing. ProcedureResult:817 emits unconditionally whether or not p was found; handle_frame:833 emits 'unhandled' for an unknown tag. So every misroute, unknown echo, uppercase echo or double-answer vanishes without trace — in a file whose documented posture is that failures are loud or absent.", "evidence": "events emitted for an id nobody sent: 0; original query still parked: 1", "fix": "Mirror ProcedureResult: move the emit outside the 'if p then', carrying message_id and a matched flag.", "note": "This subsumes the case-fold question entirely — with a loud unmatched path an uppercase echo surfaces immediately instead of hanging a consumer." }, { "id": 5, "title": "README overclaims the settle", "refs": [ "README.md:389-391", "README.md:787-788" ], "problem": "'it does fire, with (\"connection lost\"), when the socket dies under them' is unconditionally false on the close path (blocker 1). Same for both changed table rows.", "fix": "Either fix blocker 1 so the prose becomes true, or scope it to 'when the socket dies and the SDK reconnects'. Also add the reachable-vs-delivered distinction, and note that a consumer relying solely on call_reducer's callback should keep the returned Call." }, { "id": 6, "title": "sweep docblock restates the PR body", "refs": [ "stdb/connection.lua:506-556" ], "problem": "~30 lines of rationale in-file. Worth trimming to the invariant plus the exclusion table — the argument belongs in the PR, the invariant belongs in the file.", "severity_note": "style/maintenance, lowest of the should-fixes" } ] ### nits [ { "title": "duplicate message_id silently overwrites", "detail": "Live 2.8.2 answers BOTH one-offs sent under the same message_id, each echoing the same id (my wire TEST B). In the SDK the second write at :461 overwrites the first's entry: caller 1's callback dropped, caller 1's rows delivered to caller 2, second response finds nothing. Not reachable via the public API — allocate_request_id (:45-49) is monotonic and never reset, including across reconnects — but the server does not dedupe." }, { "title": "request_id >= 2^32 aliases the one-off key", "detail": "The key is 4 bytes, so request_id 4294967296 produces oneoff:00000000, aliasing id 0. Verified: ids 4294967295/4294967296 -> keys oneoff:ffffffff, oneoff:00000000. Needs 4.3e9 requests on one connection. The general form (request_id above u32 on the wire) is pre-existing across every request kind." }, { "title": "type(msg.message_id) == 'string' guard is dead for absence", "detail": "protocol.decode_server already raises 'OneOffQueryResponse missing required field \"message_id\"' before the handler runs — correctly loud. The guard earns its keep only for a non-string, e.g. a numeric message_id, which passes decode and is correctly ignored." }, { "title": "conn.ws is assigned and never read", "detail": "init.lua:212 assigns it; grep finds no reader anywhere in stdb/ or dev/. Dead as written." } ] ### wire verification { "method": "own isolated SpacetimeDB 2.8.2 on port 4400, raw Node WebSocket client, v1.json.spacetimedb subprotocol", "result": "CONFIRMED, and more broadly than the PR tested", "observations": [ "sent [0,0,1,42] -> echo \"0000012a\"; sent [0,0,0,255] -> echo \"000000ff\"; sent [222,173,190,239] -> echo \"deadbeef\"", "all lowercase: true; all match /^[0-9a-f]{8}$/: true; three concurrent one-offs all answered", "deadbeef is the decisive case — all letters, echoed lowercase", "it is hex::encode of EXACTLY the bytes sent, any length: [1]->\"01\", []->\"\", 16 bytes->32 chars", "a FAILING query echoes its own id too, so the error path is correlated correctly", "message_id arrives as typeof string in every case" ], "conclusion": "the re-key at :461/:783 is right and the big-endian arithmetic at :456-459 matches the echo" } ### reentrancy { "issues_new_request": { "sweep_continues": true, "pending_consistent": true, "later_skipped": false }, "calls_close": { "sweep_continues": true, "detail": "all 3 of 3 callbacks fired", "pending_consistent": true, "later_skipped": false, "judgment": "correct — each caller deserves its answer even after someone asked to close" }, "raises": { "sweep_continues": false, "pending_consistent": "detached, entries lost", "later_skipped": "YES, unrecoverably", "finding": "blocker 2 — the reviewer's instinct was correct" } } ### frozen contract judgment { "verdict": "the asymmetry is the right thing to freeze, but the stated justification does not survive testing", "tested_claim": "'between mechanisms, not between what a caller can find out' is FALSE as written — a caller who used only call_reducer's legacy callback and discarded the returned Call learns nothing about their specific request, only that the connection dropped, via a connection-level event that names no request", "defensible_restatement": "the reducer's outcome is REACHABLE, on a surface the same call already returned. Reachable is not delivered, and that difference is the whole point of #41.", "freeze_is_still_correct": "firing the legacy callback with a nil update would break consumers, and test_connection.lua:202 guards a real contract", "option_1_rejection": "also right — two new public types to carry what (err, ...) already says, on the PR that freezes the API, is a bad trade" }
connection: settle in-flight requests on close, not only on reconnect (#41)
All checks were successful
ci / ci (pull_request) Successful in 1m30s
Details
86dc51a34e 
The first cut put the sweep in set_send, which is only reached from open(). That
covered a socket being REPLACED and missed one being RETIRED: conn.close sets
closing = true and schedule_reconnect bails on "closing or not
reconnect_enabled", so on db:close(), or any disconnect with reconnect = false,
the sweep never ran and #41's own motivating scenario reproduced verbatim - a
procedure callback that never fired, a Call stuck at is_pending(), a
subscribe_query left at state='sent', neither is_active() nor is_ended(), with
on_applied and on_error both permanently silent, and four entries leaked.

The sweep is now M.settle_pending(c, replay), called from set_send with
replay = true and from conn.close and schedule_reconnect's terminal branch with
replay = false. Purely additive; no existing signature or behaviour changes.

The exclusion set is really "answered by the replay that is about to happen", so
it splits in two. reducer and unsubscribe are exempt on BOTH paths: a reducer's
outcome is on the Call settled in the same sweep and its legacy callback's
no-fire shape is frozen, and an unsubscribe took effect the moment the server's
copy died with the socket - neither claim mentions a replay, and resubscribe_all
re-issues neither. subscribe is the only contingent one: "the set comes back
under its original id" is a claim about the future, true on the reconnect path
and false after a close, where the silence becomes the lie instead.

A raising callback no longer costs the others their answer. pending is detached
before the sweep, so a callback that raised out of the loop left every later
caller unrecoverable - measured 1 of 4 reached, 0 entries left. Each entry now
settles under pcall and the FIRST error is re-raised once the sweep completes:
4 of 4 reached, and the bug still propagates. Swallowing it would hide a
consumer bug in the one function whose job is to stop failures being silent, and
an event would route a Lua error where Lua errors do not go. Structural work is
ordered before the sweep, so a raising callback cannot leave a socket open.

Three correlated handlers took whatever was parked under a request_id while
their siblings guarded on p.kind. A ProcedureResult bearing a reducer's id fired
the frozen (err, update) callback with a decoded return value where a
TransactionUpdate belongs AND consumed the Call's entry, orphaning it so no
later settle could reach it. subscription_applied and ProcedureResult now check
kind; SubscriptionError takes subscribe OR unsubscribe, because a live 2.8.2
server answers UnsubscribeMulti with SubscriptionError and narrowing it to
subscribe alone leaks the entry and swallows the documented callback.

An unmatched OneOffQueryResponse was the only correlated handler that could
consume a frame and emit nothing, so every misroute, unknown echo, uppercase
echo and double answer vanished. The emit moved outside `if p`, carrying
message_id and a matched flag. That is also what makes deleting the case-fold
safe: an uppercase echo now surfaces at once instead of hanging a consumer.

README: the claim that the callback fires "when the socket dies under them" was
unconditionally false on the close path. Now true, plus the reachable-vs-
delivered distinction - a caller holding only call_reducer's legacy callback
learns nothing about their own request - and the five event types the events
table never listed. conn.ws was assigned in init.lua and read nowhere; deleted.
The 30-line sweep docblock restated the PR body; trimmed to the invariant.

16 mutants, 16/16 die. 6 passed, 0 failed; stylua clean; conformance PASS twice
back-to-back on an isolated port.
yuris merged commit bf7c75bab0 into main 2026-08-20 02:47:45 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
area:defold
Defold engine integration

  
area:docs
Documentation and examples

  
area:infra
Flake, CI, secretspec

  
area:protocol
Wire protocol conformance

  
area:testing
Test harness and fixtures

  
kind:bug
Something broken

  
kind:chore
Maintenance

  
phase:breadth
UnsubscribeMulti, procedures, BSATN

  
phase:core
Core SDK layers (protocol/cache/connection)

  
phase:hardening
Reconnect, token persistence, errors

  
phase:slice
Vertical slice: first playable end-to-end

No labels
area:defold
area:docs
area:infra
area:protocol
area:testing
kind:bug
kind:chore
phase:breadth
phase:core
phase:hardening
phase:slice
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
yuris/spacetimedb-defold!47
No description provided.