stdb: fix five silent failures, and delete three required arguments #111

Merged

yuris merged 2 commits from dx-pass into main 2026-08-22 09:34:34 +00:00

Conversation 2 Commits 2 Files changed 19 +826 -142

yuris commented 2026-08-22 09:15:23 +00:00

Owner

Copy link

A DX pass over the whole SDK surface. Four read-only audits (connect, the
read side, the call side, the first-five-minutes path) turned up five
failures that were silent, three required arguments that did not need to
exist, and a README that never explained where the schema came from.

Everything below was executed, not reasoned about. Details in the commit
message; this is the reviewer's map.

Five silent failures

What	Why it was silent
db:ping callback was inverted	cb(seconds) on success, so stdb.await(db.ping, db) raised the latency as an error. Both existing tests passed under the bug because they only asserted err == nil.
Stale schema dropped columns	decode_typed_bsatn never checked for leftover bytes. A column added at the end vanished forever; one added in the middle misaligned the reader and produced a plausible wrong number.
await hung on a dropped socket	call_reducer settles its Call instead of firing the callback (ANSWERED_ELSEWHERE), and await threw the handle away. Same for subscribe across a reconnect replay.
await_event unhooked its sibling	Restoring db.on_event on resolve dropped a later waiter's wrapper out of the chain. Only reproduces once a second coroutine exists.
await_event hung when already connected	It waited for a future firing. "Wait until connected, then resubscribe" is the natural use and the exact case that broke.

Three arguments deleted

schema = gen accepts the whole generated module, so the four correlated
options become one. json defaults to the engine global. with_schema
collapses to one assignment, and stdb.builder(opts) reaches the options
the fluent setters could not.

The json one needed checking rather than assuming. The comment in
init.lua about bob's static dependency scan is about require, not about
globals, so nothing forced the argument.

Verification

Beyond lua dev/test/runner.lua (13/13) and stylua:

• Published the README's own module to a scratch database and ran the SDK
  against it. join_game arrived through on_insert, move_player came
  back as a real on_update from 0,0 to 12.5,40, and find on the
  identity pk resolved. Scratch database deleted afterwards.
• Ran spacetime init as documented. Caught two errors in my own draft
  that way: a missing export default, without which the module does not
  publish, and the wrong file path.
• Generated a schema off the live server and diffed it against the
  README's illustrative snippet. Matches field for field, including the
  empty typespace.
• Confirmed send_message-style snake_casing from the live schema
  endpoint rather than asserting it.

Reviewer notes

Three behaviour changes could break a caller who depended on the old
shape, and each has a test pinning the new one:

1. db:ping's callback is (err, seconds). Anyone reading the first
   argument as a number now gets nil.
2. db.tables.<unknown> raises instead of returning nil. A schemaless
   connection therefore raises on every table name.
3. A row with bytes left over raises instead of decoding short.

Number 3 is the one worth arguing about. It turns a silent wrong answer
into a hard failure, which I think is right for a developer-time mistake,
and decode_rows runs in a pass that writes nothing so it cannot leave
half-applied state. Say if you would rather it reported through
undescribed instead.

Not done

db.db and ctx.db are still a third and second alias for
db.tables, and ctx.db.tables.message is a nil index. Deleting
c.db touches contract §2, so I left it.

A DX pass over the whole SDK surface. Four read-only audits (connect, the read side, the call side, the first-five-minutes path) turned up five failures that were silent, three required arguments that did not need to exist, and a README that never explained where the schema came from. Everything below was executed, not reasoned about. Details in the commit message; this is the reviewer's map. ## Five silent failures | What | Why it was silent | | --- | --- | | `db:ping` callback was inverted | `cb(seconds)` on success, so `stdb.await(db.ping, db)` raised the latency as an error. Both existing tests passed under the bug because they only asserted `err == nil`. | | Stale schema dropped columns | `decode_typed_bsatn` never checked for leftover bytes. A column added at the end vanished forever; one added in the middle misaligned the reader and produced a plausible wrong number. | | `await` hung on a dropped socket | `call_reducer` settles its Call instead of firing the callback (`ANSWERED_ELSEWHERE`), and `await` threw the handle away. Same for subscribe across a reconnect replay. | | `await_event` unhooked its sibling | Restoring `db.on_event` on resolve dropped a later waiter's wrapper out of the chain. Only reproduces once a second coroutine exists. | | `await_event` hung when already connected | It waited for a future firing. "Wait until connected, then resubscribe" is the natural use and the exact case that broke. | ## Three arguments deleted `schema = gen` accepts the whole generated module, so the four correlated options become one. `json` defaults to the engine global. `with_schema` collapses to one assignment, and `stdb.builder(opts)` reaches the options the fluent setters could not. The `json` one needed checking rather than assuming. The comment in init.lua about bob's static dependency scan is about `require`, not about globals, so nothing forced the argument. ## Verification Beyond `lua dev/test/runner.lua` (13/13) and stylua: - Published the README's own module to a scratch database and ran the SDK against it. `join_game` arrived through `on_insert`, `move_player` came back as a real `on_update` from 0,0 to 12.5,40, and `find` on the identity pk resolved. Scratch database deleted afterwards. - Ran `spacetime init` as documented. Caught two errors in my own draft that way: a missing `export default`, without which the module does not publish, and the wrong file path. - Generated a schema off the live server and diffed it against the README's illustrative snippet. Matches field for field, including the empty `typespace`. - Confirmed `send_message`-style snake_casing from the live schema endpoint rather than asserting it. ## Reviewer notes Three behaviour changes could break a caller who depended on the old shape, and each has a test pinning the new one: 1. `db:ping`'s callback is `(err, seconds)`. Anyone reading the first argument as a number now gets nil. 2. `db.tables.<unknown>` raises instead of returning nil. A schemaless connection therefore raises on every table name. 3. A row with bytes left over raises instead of decoding short. Number 3 is the one worth arguing about. It turns a silent wrong answer into a hard failure, which I think is right for a developer-time mistake, and `decode_rows` runs in a pass that writes nothing so it cannot leave half-applied state. Say if you would rather it reported through `undescribed` instead. ## Not done `db.db` and `ctx.db` are still a third and second alias for `db.tables`, and `ctx.db.tables.message` is a nil index. Deleting `c.db` touches contract §2, so I left it.

yuris added 1 commit 2026-08-22 09:15:23 +00:00

stdb: fix five silent failures, and delete three required arguments ... f3a5ed9c34

A DX pass over the whole surface, driven by an audit of connect, the read
side, the call side and the first-five-minutes path.

FIXED, each of which failed silently before:

  * `db:ping` called back `(seconds)` on success and `(nil, err)` on
    failure, inverted against every other async entry point here, so
    `stdb.await(db.ping, db)` raised the latency as an error. Now
    `(err, seconds)`. The two existing tests passed under the bug because
    they only asserted `err == nil`; naming the parameters in order is
    what pins it.
  * `decode_typed_bsatn` read as many columns as the def named and never
    checked for leftover bytes, so a generated schema behind the server
    dropped a trailing column forever, and a column added in the MIDDLE
    misaligned the reader and produced a plausible wrong number. Both
    sibling decoders already made this check (protocol_v2.lua:445, :785).
    Safe to raise here: decode_rows runs in apply_database_update's PASS
    ONE, which writes nothing.
  * `stdb.await` discarded the handle `fn` returned, so a reducer awaited
    across a dropped socket parked its coroutine forever (the Call is
    settled instead of the callback firing, ANSWERED_ELSEWHERE), and a
    subscribe awaited across a reconnect replay did the same
    (ANSWERED_BY_REPLAY). It now wires the handle's on_error/on_applied
    too, settle-once.
  * `stdb.await_event` restored `db.on_event` unconditionally on resolve,
    which dropped a LATER waiter's wrapper out of the chain and hung it.
    It now detaches only while it is still the head.
  * `stdb.await_event(db, "connected")` waited for a FUTURE firing, so it
    parked forever on an already-connected db. That is the natural "wait
    until connected, then resubscribe" after a reconnect has landed. It
    now answers from `db:is_connected()`.

DELETED, three arguments a caller had to type for no reason:

  * `schema = gen` takes the whole generated module and `connect` reads
    `typespace`/`reducers`/`procedures` off it. Four correlated lines
    become one, and dropping `reducers` no longer fails later, wire-side,
    at the first call. The four-field spelling is unchanged.
  * `json` defaults to the engine's own global in `schema.fetch` and
    `auth.exchange`. Verified the bob static-scan constraint does not
    force the argument: `json` is an ambient global, read the way
    `websocket` and `zlib` already are, not a module this SDK requires.
  * `Builder:with_schema` collapses to one assignment, and
    `stdb.builder(opts)` seeds the options the fluent setters cannot
    reach, which also makes init.lua's existing read of `_opts.on_event`
    live rather than dead.

BETTER ERRORS, where the old one named neither the cause nor the fix:

  * `db.tables.mesage` raises and lists the real names, like
    `db.reducers` already did. Covers a typo, a `schema` never passed,
    and a table the generated module predates.
  * an empty reducer/procedure section names the option that was omitted
    instead of trailing "known: " with nothing after it.
  * `stdb.url` rejects a scheme-less host at the callsite rather than
    letting it fail inside the engine's websocket.
  * the editor command no longer defaults `database` to this repo's dev
    fixture, which generated a schema from the wrong module successfully
    and silently.
  * `stdb.query` is re-exported, so the typed query builder is reachable
    without reading stdb/*.lua to learn it exists.

DOCS. The README quickstart is eight steps, and the two it was missing
are the two a new reader needs first: writing and publishing a module,
and generating the schema. Every command and snippet in it was executed
against a live 2.8.2 server, including publishing the README's own module
to a scratch database and driving it from the SDK. The example is a
`player` table rather than a chat, since a game replicates positions and
a moving row is what demonstrates that on_update needs a primary key.

ROT. `evt.reason` in the example was never set by the SDK, so a
disconnect printed nil. A dead `json` option on `connect`, a stale
comment about a README bug since fixed, and three references to
stdb/protocol.lua, deleted in 4.0.0, one of them stamped into every
generated schema module.

Tests: 13/13, stylua clean, headless example round trip PASS.

yuris commented 2026-08-22 09:28:51 +00:00

Author

Owner

Copy link

Review — PR #111 (dx-pass), correctness

Verdict: REQUEST CHANGES — two blocking items. The five claimed fixes are real
and four of them are pinned properly; the two blockers are a discriminator that
does not discriminate in the one case the PR says it handles, and a new failure
path that trades a silent hang for a silent death.

Environment: lua dev/test/runner.lua → 13 passed, 0 failed. stylua --check stdb dev/test → clean. Runner is Lua 5.1.5.

---

1. BLOCKING — the schema = gen discriminator misfires on a module with a table named columns

stdb/init.lua:292:

if type(schema) == "table" and type(schema.tables) == "table" and schema.tables.columns == nil then

and the comment above it, stdb/init.lua:287-289:

-- is the one ambiguity, and `columns` settles it - every table def has one
-- (`M.schema.new` rejects a def without it), a section map never does.

"A section map never does" is false. gen.tables is keyed by table name, so a
server module with a table called columns makes gen.tables.columns a def —
non-nil — and the whole generated module is then passed through as if it were a
schema map. Reproduced against the real stdb.connect:

gen+tbl'columns'  cache.schema keys = {procedures,reducers,tables,typespace}  typespace=false
   db.tables.message -> false  stdb: no such table "message"; known: procedures, reducers, tables, typespace

For comparison, the case the comment was worried about is fine:

gen+tbl'tables'   cache.schema keys = {message,tables}  typespace=true
   db.tables.message -> true  message

Why it matters: the four section names become the "tables", typespace is lost,
every real relation is undecodable, and every subscribed row arrives as an
undescribed event. columns is not an exotic table name (information_schema
has one), and this is precisely the ambiguity the PR claims to have settled.

Smallest fix — discriminate on the sections, which typetags.build always emits
all four of (stdb/typetags.lua:520), instead of on a name inside tables:

if type(schema) == "table" and type(schema.tables) == "table"
   and schema.reducers ~= nil and schema.procedures ~= nil and schema.typespace ~= nil then

Verified against the same six cases: correct for all of them. The residual
ambiguity shrinks from "a module with one table named columns" to "a module with
four tables named tables, reducers, procedures and typespace". (A total
fix would be an explicit gen = ... option; setmetatable-tagging
schema.new's output does not work, since M.new returns the caller's own
gen.tables table — stdb/schema.lua:100.)

---

2. BLOCKING — stdb.await no longer hangs on a dropped socket; it now fails silently instead

stdb/await.lua:88-92:

		if state == "YIELDED" then
			coroutine.resume(co)

coroutine.resume's return value is discarded, so the error(err, 0) at
stdb/await.lua:134 — the entire point of wiring the handle — is swallowed
whenever the settle arrives asynchronously, which is every real dropped socket.
Reproduced end to end with a real connection.new + call_reducer +
settle_pending:

start:               true
status after yield:  suspended
status after drop:   dead      -- error was consumed by settle's resume, nothing raised

Nothing raised, nothing logged, settle_pending returned clean, the coroutine's
remaining statements never ran. The good news the PR claims is real (the
coroutine does resume rather than parking forever); the bad news is that "the
socket died" reaches no surface at all. stdb/await.lua:50-52 promises the
opposite: "a caller who forgets to pcall an await that raises finds out
immediately". It does not.

Note this is why the new test at dev/test/test_init.lua:637-651 passes: its stub
late-fires synchronously inside registration, so settle takes the
state == "RUNNING" branch and the raise happens on the caller's own stack. The
asynchronous path is never exercised for an error.

Smallest fix, in settle:

		if state == "YIELDED" then
			local ok, e = coroutine.resume(co)
			if not ok then
				error(e, 0)
			end

This makes the failure land where every other SDK raise from inside a frame
already lands (see stdb/init.lua:373-383, which re-raises for exactly this
reason).

---

3. BLOCKING — README §8 documents a pcall idiom that cannot work on the HTML5 target

README.md, step 8:

A failure raises inside the coroutine, so one `pcall` covers the whole sequence.

On plain Lua 5.1 a pcall cannot be yielded across. Demonstrated on the repo's
own interpreter (Lua 5.1.5) with a real call_reducer:

real dropped-socket await -> false  attempt to yield across metamethod/C-call boundary

So pcall(function() ... stdb.await(...) ... end) fails at the first await that
actually waits. It works under LuaJIT (yield-across-pcall is a LuaJIT extension),
which covers Defold's 64-bit native targets, but [INFERENCE] not the HTML5/Lua 5.1
target this README promises support for two sections earlier ("The SDK itself is
pure Lua and runs on both native and HTML5 targets").

Smallest fix: drop the sentence, or qualify it — "a failure raises inside the
coroutine; catch it with a pcall inside the awaited step, not around the whole
coroutine.wrap body, since Lua 5.1 cannot yield across pcall."

The same claim exists at stdb/await.lua:49-53 (pre-existing, not part of this
diff) and is wrong there for the same reason; worth fixing in the same pass.

---

4. NIT — the handle-error path is pinned only against a stub, and only in its inline branch

dev/test/test_init.lua:637-651 and :659-671 assert await's wiring against
hand-built { on_error = ... } / { on_applied = ... } tables. Nothing in the
suite awaits a real conn:call_reducer and then drops the socket, so the actual
claim — "ANSWERED_ELSEWHERE no longer parks the coroutine" — is not pinned:
Call:on_error could stop firing from settle_one
(stdb/connection.lua:846-848) and these tests would still pass. I verified by
hand that the real path works today, so this is coverage, not a defect.

The replay test (:653-676) is the good one — it genuinely yields, parks, and
proves the second fire is inert. The error test should do the same: build a
connection.new, stdb.await(conn.call_reducer, conn, ...) inside a
coroutine.create, assert it is suspended, then connection.settle_pending(conn, false) and assert the outcome. (That test will also catch finding 2.)

5. NIT — await attaches to a Subscription permanently

stdb/await.lua:120-124 registers an on_applied closure and never detaches it.
Subscription:on_applied registration is permanent by contract
(stdb/connection.lua:339-343), so every awaited subscribe_query leaves one
inert closure that is invoked on every reconnect replay for the life of the
subscription. Harmless (the results guard short-circuits) but unbounded per
await. Fix: capture the closure and handle:off_applied(fn) inside settle.

6. NIT — M.builder(opts) aliases the caller's table

stdb/init.lua:616 stores the caller's opts as _opts, and with_token
(:630) / with_schema (:638) then write into it. build() copies
(:667-670), but the seed the caller still holds has been mutated. One-line fix:
shallow-copy in M.builder rather than in build.

7. NIT — three new guards ship with no test

• stdb/init.lua:70-75 — stdb.url's scheme and non-empty-database raises. No
  test matches needs a ws or non-empty name.
• stdb/spacetimedb.editor_script:273-281 — the new "no database to generate
  from" raise. dev/test/test_fixtures.lua:1507 already loadfiles the editor
  script and calls get_commands(), so this is cheap to pin.

8. NIT — the duck-type comment should name the one on_error with a different shape

stdb/await.lua:110-112 says a Call has on_error and a Subscription has both.
SubBuilder:on_error (stdb/connection.lua:630) is a third: same name, but its
fn is called (ctx, err), so settle(err, msg) would read a ctx as the error.
No awaitable function returns a SubBuilder today, so this is documentation, not
a bug — but it is the one counterexample to the sentence as written.

9. NIT — stdb/auth.lua:10 still shows json = json, in the header example

while :98 now says it is "only needed off-engine". Drop it from the example for
consistency with the change.

---

Checked, fine — do not redo

• decode_typed_bsatn's new raise cannot half-apply the cache. Read
  cache.decode_rows (stdb/cache.lua:372) and apply_database_update
  (:524-594) directly: PASS ONE builds batches and writes nothing; every store
  write is in PASS TWO (:598+), which never reaches a decoder. A raise from the
  decoder leaves the store untouched for the whole frame. The comment's claim
  holds.
• The raise is not swallowed under frame_budget. stdb/init.lua:373-383
  re-raises coroutine.resume's error with level 0 and keeps the queue. (Contrast
  finding 2, where the same pattern does not re-raise.)
• No legitimate trailing-byte case found. row_list partitions rows_data
  exactly for both hints (FixedSize slices at stdb/protocol_v2.lua:521-534,
  RowOffsets validated at :535+), so a well-matched def consumes a row exactly.
  Event-table rows are decoded against the table's own def
  (stdb/connection.lua:1181-1187) — same def, same rows. One-off query rows are
  never decoded by the SDK, and the documented caller-side decode of a
  projection against a full def now raises instead of silently swapping columns
  (stdb/connection.lua:777-783) — an improvement, and the intended one.
• Settle-once is once under every ordering I could construct. results is set
  before any resume, so late-fire-during-registration, callback-then-handle,
  handle-then-callback, and replay-after-settle all land on the early return. The
  replay test at dev/test/test_init.lua:653-676 covers the double-fire case
  through a real yield.
• Nothing else returned by an awaited function is handle-shaped.
  one_off_query, call_procedure, unsubscribe and ping all return a plain
  numeric request_id (stdb/connection.lua:759, :791, :724); unsubscribe
  returns nil for an already-ended set. Only call_reducer (Call) and
  subscribe_query (Subscription) return tables. SubHandle has neither method.
• db.tables's new raising __index breaks nothing. No pairs, nil-check
  or rawget against the facade map anywhere in stdb/ or dev/.
  tables.dispatch (stdb/tables.lua:261) indexes it, but only with names taken
  from cache.schema, which is the same map the facades are built from;
  subscribe_to_all_tables (stdb/connection.lua:666) iterates cache.schema,
  not the facades. build_ctx aliases the map (:86, :160), it does not probe
  it.
• json defaulting mixes no implementations. _G.json is read in exactly two
  places (stdb/schema.lua:206, stdb/auth.lua:121); each uses it only for its
  own response body, opts.json remains the seam in both, and connect takes no
  json at all. The editor script never calls schema.fetch (it carries its own
  decoder), so the editor VM's missing json global is not reachable from the new
  default. Both new tests restore _G.json before their assertions
  (dev/test/test_auth.lua:551, dev/test/test_fixtures.lua:1466), and the
  runner hands each file its own json in a custom env (dev/test/runner.lua:50),
  so unsetting _G.json does not disturb the test file's own reference.
• The ping inversion is genuinely pinned, not just ported.
  dev/test/test_connection_handles.lua:1265-1267 and :1626-1628 assert
  assert_nil(ping_err) plus type(ping_seconds) == "number" plus
  latency() == ping_seconds; re-inverting cb puts the number in ping_err and
  fails. (The error branch, cb(err) at stdb/connection.lua:1475, has no test —
  minor.)
• The reducer-accessor message assertion is real. stdb/connection.lua:59-69
  emits pass reducers = gen.reducers to stdb.connect, which the new match at
  dev/test/test_connection_handles.lua:1677 finds; the old known: $ match would
  no longer match this string, so it was replaced rather than weakened. The
  typo-path assertions (no such reducer + the real name listed) are untouched.
• test_tables.lua and test_auth.lua got stronger, not weaker. Both replaced
  assert_nil with a raise plus two content assertions on the message; the json
  entry dropped from the required-field loop is replaced by a positive
  default-resolution test and a negative no-json-anywhere test.
• chat_client.lua's evt.code is correct: stdb/init.lua:444 is the only
  emitter of disconnected and it carries code and nothing else.
• README Lua snippets run as written against the real signatures:
  stdb.query.tables(gen) (re-exported at stdb/init.lua:48, validated at
  stdb/query.lua:489), :where + a builder passed to subscribe_query,
  Table:count/:find/:iter/:on_insert/:on_update/:on_delete
  (stdb/tables.lua:171-251), db.identity (stdb/connection.lua:958),
  stdb.url("ws://...", "arena") under the new scheme check, schema.fetch
  without json, stdb.await(self.db.reducers.join_game, { ... }) against the
  accessor's (args, callback) shape, and /my_schema.lua matching
  OUT in the editor script. The hand-written gen example matches the real
  generated shape ({ params, types } per reducer, stdb/typetags.lua:341;
  identity/f32/string are all real scalar tags,
  stdb/protocol_v2.lua:171-190). The TypeScript module and spacetime CLI
  invocations were not verified — no server toolchain reachable from here.

# Review — PR #111 (`dx-pass`), correctness **Verdict: REQUEST CHANGES** — two blocking items. The five claimed fixes are real and four of them are pinned properly; the two blockers are a discriminator that does not discriminate in the one case the PR says it handles, and a new failure path that trades a silent hang for a silent death. Environment: `lua dev/test/runner.lua` → 13 passed, 0 failed. `stylua --check stdb dev/test` → clean. Runner is Lua 5.1.5. --- ## 1. BLOCKING — the `schema = gen` discriminator misfires on a module with a table named `columns` `stdb/init.lua:292`: ```lua if type(schema) == "table" and type(schema.tables) == "table" and schema.tables.columns == nil then ``` and the comment above it, `stdb/init.lua:287-289`: ``` -- is the one ambiguity, and `columns` settles it - every table def has one -- (`M.schema.new` rejects a def without it), a section map never does. ``` "A section map never does" is false. `gen.tables` is keyed by **table name**, so a server module with a table called `columns` makes `gen.tables.columns` a def — non-nil — and the whole generated module is then passed through as if it were a schema map. Reproduced against the real `stdb.connect`: ``` gen+tbl'columns' cache.schema keys = {procedures,reducers,tables,typespace} typespace=false db.tables.message -> false stdb: no such table "message"; known: procedures, reducers, tables, typespace ``` For comparison, the case the comment *was* worried about is fine: ``` gen+tbl'tables' cache.schema keys = {message,tables} typespace=true db.tables.message -> true message ``` Why it matters: the four section names become the "tables", `typespace` is lost, every real relation is undecodable, and every subscribed row arrives as an `undescribed` event. `columns` is not an exotic table name (`information_schema` has one), and this is precisely the ambiguity the PR claims to have settled. Smallest fix — discriminate on the sections, which `typetags.build` always emits all four of (`stdb/typetags.lua:520`), instead of on a name inside `tables`: ```lua if type(schema) == "table" and type(schema.tables) == "table" and schema.reducers ~= nil and schema.procedures ~= nil and schema.typespace ~= nil then ``` Verified against the same six cases: correct for all of them. The residual ambiguity shrinks from "a module with one table named `columns`" to "a module with four tables named `tables`, `reducers`, `procedures` and `typespace`". (A total fix would be an explicit `gen = ...` option; `setmetatable`-tagging `schema.new`'s output does not work, since `M.new` returns the caller's own `gen.tables` table — `stdb/schema.lua:100`.) --- ## 2. BLOCKING — `stdb.await` no longer hangs on a dropped socket; it now fails silently instead `stdb/await.lua:88-92`: ```lua if state == "YIELDED" then coroutine.resume(co) ``` `coroutine.resume`'s return value is discarded, so the `error(err, 0)` at `stdb/await.lua:134` — the entire point of wiring the handle — is swallowed whenever the settle arrives asynchronously, which is every real dropped socket. Reproduced end to end with a real `connection.new` + `call_reducer` + `settle_pending`: ``` start: true status after yield: suspended status after drop: dead -- error was consumed by settle's resume, nothing raised ``` Nothing raised, nothing logged, `settle_pending` returned clean, the coroutine's remaining statements never ran. The good news the PR claims is real (the coroutine does resume rather than parking forever); the bad news is that "the socket died" reaches no surface at all. `stdb/await.lua:50-52` promises the opposite: *"a caller who forgets to `pcall` an await that raises finds out immediately"*. It does not. Note this is why the new test at `dev/test/test_init.lua:637-651` passes: its stub late-fires **synchronously** inside registration, so `settle` takes the `state == "RUNNING"` branch and the raise happens on the caller's own stack. The asynchronous path is never exercised for an error. Smallest fix, in `settle`: ```lua if state == "YIELDED" then local ok, e = coroutine.resume(co) if not ok then error(e, 0) end ``` This makes the failure land where every other SDK raise from inside a frame already lands (see `stdb/init.lua:373-383`, which re-raises for exactly this reason). --- ## 3. BLOCKING — README §8 documents a `pcall` idiom that cannot work on the HTML5 target `README.md`, step 8: ``` A failure raises inside the coroutine, so one `pcall` covers the whole sequence. ``` On plain Lua 5.1 a `pcall` cannot be yielded across. Demonstrated on the repo's own interpreter (Lua 5.1.5) with a real `call_reducer`: ``` real dropped-socket await -> false attempt to yield across metamethod/C-call boundary ``` So `pcall(function() ... stdb.await(...) ... end)` fails at the first await that actually waits. It works under LuaJIT (yield-across-pcall is a LuaJIT extension), which covers Defold's 64-bit native targets, but [INFERENCE] not the HTML5/Lua 5.1 target this README promises support for two sections earlier ("The SDK itself is pure Lua and runs on both native and HTML5 targets"). Smallest fix: drop the sentence, or qualify it — "a failure raises inside the coroutine; catch it with a `pcall` *inside* the awaited step, not around the whole `coroutine.wrap` body, since Lua 5.1 cannot yield across `pcall`." The same claim exists at `stdb/await.lua:49-53` (pre-existing, not part of this diff) and is wrong there for the same reason; worth fixing in the same pass. --- ## 4. NIT — the handle-error path is pinned only against a stub, and only in its inline branch `dev/test/test_init.lua:637-651` and `:659-671` assert `await`'s wiring against hand-built `{ on_error = ... }` / `{ on_applied = ... }` tables. Nothing in the suite awaits a real `conn:call_reducer` and then drops the socket, so the actual claim — "`ANSWERED_ELSEWHERE` no longer parks the coroutine" — is not pinned: `Call:on_error` could stop firing from `settle_one` (`stdb/connection.lua:846-848`) and these tests would still pass. I verified by hand that the real path works today, so this is coverage, not a defect. The replay test (`:653-676`) is the good one — it genuinely yields, parks, and proves the second fire is inert. The error test should do the same: build a `connection.new`, `stdb.await(conn.call_reducer, conn, ...)` inside a `coroutine.create`, assert it is `suspended`, then `connection.settle_pending(conn, false)` and assert the outcome. (That test will also catch finding 2.) ## 5. NIT — `await` attaches to a Subscription permanently `stdb/await.lua:120-124` registers an `on_applied` closure and never detaches it. `Subscription:on_applied` registration is permanent by contract (`stdb/connection.lua:339-343`), so every awaited `subscribe_query` leaves one inert closure that is invoked on every reconnect replay for the life of the subscription. Harmless (the `results` guard short-circuits) but unbounded per await. Fix: capture the closure and `handle:off_applied(fn)` inside `settle`. ## 6. NIT — `M.builder(opts)` aliases the caller's table `stdb/init.lua:616` stores the caller's `opts` as `_opts`, and `with_token` (`:630`) / `with_schema` (`:638`) then write into it. `build()` copies (`:667-670`), but the seed the caller still holds has been mutated. One-line fix: shallow-copy in `M.builder` rather than in `build`. ## 7. NIT — three new guards ship with no test - `stdb/init.lua:70-75` — `stdb.url`'s scheme and non-empty-database raises. No test matches `needs a ws` or `non-empty name`. - `stdb/spacetimedb.editor_script:273-281` — the new "no database to generate from" raise. `dev/test/test_fixtures.lua:1507` already `loadfile`s the editor script and calls `get_commands()`, so this is cheap to pin. ## 8. NIT — the duck-type comment should name the one `on_error` with a different shape `stdb/await.lua:110-112` says a Call has `on_error` and a Subscription has both. `SubBuilder:on_error` (`stdb/connection.lua:630`) is a third: same name, but its `fn` is called `(ctx, err)`, so `settle(err, msg)` would read a ctx as the error. No awaitable function returns a `SubBuilder` today, so this is documentation, not a bug — but it is the one counterexample to the sentence as written. ## 9. NIT — `stdb/auth.lua:10` still shows `json = json,` in the header example while `:98` now says it is "only needed off-engine". Drop it from the example for consistency with the change. --- ## Checked, fine — do not redo - **`decode_typed_bsatn`'s new raise cannot half-apply the cache.** Read `cache.decode_rows` (`stdb/cache.lua:372`) and `apply_database_update` (`:524-594`) directly: PASS ONE builds `batches` and writes nothing; every store write is in PASS TWO (`:598+`), which never reaches a decoder. A raise from the decoder leaves the store untouched for the whole frame. The comment's claim holds. - **The raise is not swallowed under `frame_budget`.** `stdb/init.lua:373-383` re-raises `coroutine.resume`'s error with level 0 and keeps the queue. (Contrast finding 2, where the same pattern does *not* re-raise.) - **No legitimate trailing-byte case found.** `row_list` partitions `rows_data` exactly for both hints (`FixedSize` slices at `stdb/protocol_v2.lua:521-534`, `RowOffsets` validated at `:535+`), so a well-matched def consumes a row exactly. Event-table rows are decoded against the table's own def (`stdb/connection.lua:1181-1187`) — same def, same rows. One-off query rows are never decoded by the SDK, and the documented caller-side decode of a *projection* against a full def now raises instead of silently swapping columns (`stdb/connection.lua:777-783`) — an improvement, and the intended one. - **Settle-once is once under every ordering I could construct.** `results` is set before any resume, so late-fire-during-registration, callback-then-handle, handle-then-callback, and replay-after-settle all land on the early return. The replay test at `dev/test/test_init.lua:653-676` covers the double-fire case through a real yield. - **Nothing else returned by an awaited function is handle-shaped.** `one_off_query`, `call_procedure`, `unsubscribe` and `ping` all return a plain numeric `request_id` (`stdb/connection.lua:759`, `:791`, `:724`); `unsubscribe` returns `nil` for an already-ended set. Only `call_reducer` (Call) and `subscribe_query` (Subscription) return tables. `SubHandle` has neither method. - **`db.tables`'s new raising `__index` breaks nothing.** No `pairs`, `nil`-check or `rawget` against the facade map anywhere in `stdb/` or `dev/`. `tables.dispatch` (`stdb/tables.lua:261`) indexes it, but only with names taken from `cache.schema`, which is the same map the facades are built from; `subscribe_to_all_tables` (`stdb/connection.lua:666`) iterates `cache.schema`, not the facades. `build_ctx` aliases the map (`:86`, `:160`), it does not probe it. - **`json` defaulting mixes no implementations.** `_G.json` is read in exactly two places (`stdb/schema.lua:206`, `stdb/auth.lua:121`); each uses it only for its own response body, `opts.json` remains the seam in both, and `connect` takes no `json` at all. The editor script never calls `schema.fetch` (it carries its own decoder), so the editor VM's missing `json` global is not reachable from the new default. Both new tests restore `_G.json` before their assertions (`dev/test/test_auth.lua:551`, `dev/test/test_fixtures.lua:1466`), and the runner hands each file its own `json` in a custom env (`dev/test/runner.lua:50`), so unsetting `_G.json` does not disturb the test file's own reference. - **The `ping` inversion is genuinely pinned, not just ported.** `dev/test/test_connection_handles.lua:1265-1267` and `:1626-1628` assert `assert_nil(ping_err)` plus `type(ping_seconds) == "number"` plus `latency() == ping_seconds`; re-inverting `cb` puts the number in `ping_err` and fails. (The error branch, `cb(err)` at `stdb/connection.lua:1475`, has no test — minor.) - **The reducer-accessor message assertion is real.** `stdb/connection.lua:59-69` emits `pass reducers = gen.reducers to stdb.connect`, which the new match at `dev/test/test_connection_handles.lua:1677` finds; the old `known: $` match would no longer match this string, so it was replaced rather than weakened. The typo-path assertions (`no such reducer` + the real name listed) are untouched. - **`test_tables.lua` and `test_auth.lua` got stronger, not weaker.** Both replaced `assert_nil` with a raise plus two content assertions on the message; the `json` entry dropped from the required-field loop is replaced by a positive default-resolution test and a negative no-json-anywhere test. - **`chat_client.lua`'s `evt.code`** is correct: `stdb/init.lua:444` is the only emitter of `disconnected` and it carries `code` and nothing else. - **README Lua snippets run as written** against the real signatures: `stdb.query.tables(gen)` (re-exported at `stdb/init.lua:48`, validated at `stdb/query.lua:489`), `:where` + a builder passed to `subscribe_query`, `Table:count`/`:find`/`:iter`/`:on_insert`/`:on_update`/`:on_delete` (`stdb/tables.lua:171-251`), `db.identity` (`stdb/connection.lua:958`), `stdb.url("ws://...", "arena")` under the new scheme check, `schema.fetch` without `json`, `stdb.await(self.db.reducers.join_game, { ... })` against the accessor's `(args, callback)` shape, and `/my_schema.lua` matching `OUT` in the editor script. The hand-written `gen` example matches the real generated shape (`{ params, types }` per reducer, `stdb/typetags.lua:341`; `identity`/`f32`/`string` are all real scalar tags, `stdb/protocol_v2.lua:171-190`). The TypeScript module and `spacetime` CLI invocations were not verified — no server toolchain reachable from here.

yuris added 1 commit 2026-08-22 09:34:08 +00:00

stdb: action the PR 111 review ... 4285c43544

Three blocking findings from the review, all reproduced before fixing:

The `schema = gen` discriminator read `tables.columns` as proof the argument
was a bare schema map, so a server module with a table actually named
`columns` had its four section names taken as the tables and its typespace
dropped, making every relation undecodable. Discriminate on the section set
instead, which `typetags.build` always emits all four of.

`stdb.await` discarded `coroutine.resume`'s return value on the async settle
path, so the `error(err, 0)` that is the whole point of wiring the handle went
nowhere: a dropped socket raised nothing, logged nothing, and left the
coroutine's remaining statements unrun. Re-raise, as the frame_budget path
already does. The existing test hid this because its stub late-fires
synchronously; the new one builds a real connection, parks a real await, then
drops the socket, and fails against the old code.

The README and await.lua both documented wrapping a whole `coroutine.wrap`
body in one `pcall`. Lua 5.1 cannot yield across `pcall` and the HTML5 target
is plain Lua 5.1, so that idiom fails at the first await that actually waits.

Also taken from the nits: `await` now detaches its own `on_applied` closure
once settled, since Subscription registration is permanent and would otherwise
leak one inert callback per await for the life of the subscription;
`M.builder` copies its seed so `with_token`/`with_schema` stop mutating the
caller's table; tests for `stdb.url`'s two guards and the editor script's
no-database raise; `json` dropped from auth.lua's header example.

yuris commented 2026-08-22 09:34:28 +00:00

Author

Owner

Copy link

Review actioned. All three blocking findings fixed, four of the six nits taken.

Blocking

1. schema = gen discriminator. Took the reviewer's fix: discriminate on the four sections (tables + reducers + procedures + typespace all present), not on tables.columns. New test builds a module whose tables include one named columns and asserts the typespace survives and the real tables are still the tables. Verified it fails against the old discriminator (expected table, got nil on the typespace).

2. stdb.await swallowing the async error. Took the reviewer's fix in settle: local ok, e = coroutine.resume(co); if not ok then error(e, 0) end. Replaced nothing; added a test that genuinely parks, per finding 4: real connection.new + stdb.await(conn.call_reducer, ...) inside coroutine.create, assert suspended, then connection.settle_pending(conn, false) and assert the raise reaches the caller, the message is connection lost, and the coroutine is dead. Verified explicitly that this test FAILS against the pre-fix code (expected error, call succeeded) and passes after.

3. README pcall claim. Fixed in README.md step 8 and in stdb/await.lua's docstring. Both now say to pcall around the awaited step rather than the whole coroutine.wrap body, because Lua 5.1 cannot yield across pcall and the HTML5 target is plain Lua 5.1. No em dashes, no bold, no mid-sentence colons.

Nits taken

• 5, permanently attached on_applied. await now captures the closure and detaches it once settled. Placed after the yield rather than inside settle, because on_applied can late-fire during registration, before the local holding the closure has been assigned. Pinned by extending the existing replay stub with an off_applied that records which function was detached.
• 6, M.builder aliasing. The seed is shallow-copied in M.builder. build() keeps its own copy, which is what lets a builder be built twice.
• 7, untested guards. Two assert_errors for stdb.url's scheme and empty-database raises. One for the editor script's no-database raise, driven through commands[1].run with a stubbed editor global, asserting on the message content.
• 8, duck-type comment. Names SubBuilder:on_error as the one same-name-different-shape case and why it never reaches there.
• 9, stdb/auth.lua:10. json = json, dropped from the header example.

Nits skipped

• 4, the coverage complaint, is what the new async test in finding 2 answers. Kept the existing synchronous stub test alongside it: it pins the inline branch, which is a different path through settle.

Verification

• lua dev/test/runner.lua — 13 passed, 0 failed
• stylua stdb dev/test — clean, and the suite still passes after formatting
• headless example round-trip — EXAMPLE: PASS, including the cross-client light frame

Re-review

Not warranted. Every fix is the reviewer's own suggested change, each pinned by a test whose failure against the pre-fix code was verified rather than assumed. The one judgement call worth naming is the off_applied placement above, which is more conservative than doing it inside settle. No new problems surfaced while fixing, and nothing changed behaviour beyond what the review asked for. Merging.

Review actioned. All three blocking findings fixed, four of the six nits taken. ## Blocking **1. `schema = gen` discriminator.** Took the reviewer's fix: discriminate on the four sections (`tables` + `reducers` + `procedures` + `typespace` all present), not on `tables.columns`. New test builds a module whose tables include one named `columns` and asserts the typespace survives and the real tables are still the tables. Verified it fails against the old discriminator (`expected table, got nil` on the typespace). **2. `stdb.await` swallowing the async error.** Took the reviewer's fix in `settle`: `local ok, e = coroutine.resume(co); if not ok then error(e, 0) end`. Replaced nothing; added a test that genuinely parks, per finding 4: real `connection.new` + `stdb.await(conn.call_reducer, ...)` inside `coroutine.create`, assert `suspended`, then `connection.settle_pending(conn, false)` and assert the raise reaches the caller, the message is `connection lost`, and the coroutine is `dead`. Verified explicitly that this test FAILS against the pre-fix code (`expected error, call succeeded`) and passes after. **3. README `pcall` claim.** Fixed in `README.md` step 8 and in `stdb/await.lua`'s docstring. Both now say to `pcall` around the awaited step rather than the whole `coroutine.wrap` body, because Lua 5.1 cannot yield across `pcall` and the HTML5 target is plain Lua 5.1. No em dashes, no bold, no mid-sentence colons. ## Nits taken - **5, permanently attached `on_applied`.** `await` now captures the closure and detaches it once settled. Placed after the yield rather than inside `settle`, because `on_applied` can late-fire during registration, before the local holding the closure has been assigned. Pinned by extending the existing replay stub with an `off_applied` that records which function was detached. - **6, `M.builder` aliasing.** The seed is shallow-copied in `M.builder`. `build()` keeps its own copy, which is what lets a builder be built twice. - **7, untested guards.** Two `assert_error`s for `stdb.url`'s scheme and empty-database raises. One for the editor script's no-database raise, driven through `commands[1].run` with a stubbed `editor` global, asserting on the message content. - **8, duck-type comment.** Names `SubBuilder:on_error` as the one same-name-different-shape case and why it never reaches there. - **9, `stdb/auth.lua:10`.** `json = json,` dropped from the header example. ## Nits skipped - **4**, the coverage complaint, is what the new async test in finding 2 answers. Kept the existing synchronous stub test alongside it: it pins the inline branch, which is a different path through `settle`. ## Verification - `lua dev/test/runner.lua` — 13 passed, 0 failed - `stylua stdb dev/test` — clean, and the suite still passes after formatting - headless example round-trip — `EXAMPLE: PASS`, including the cross-client light frame ## Re-review Not warranted. Every fix is the reviewer's own suggested change, each pinned by a test whose failure against the pre-fix code was verified rather than assumed. The one judgement call worth naming is the `off_applied` placement above, which is more conservative than doing it inside `settle`. No new problems surfaced while fixing, and nothing changed behaviour beyond what the review asked for. Merging.

yuris merged commit f2c3c3a8eb into main 2026-08-22 09:34:34 +00:00

yuris deleted branch dx-pass 2026-08-22 09:34:34 +00:00

yuris referenced this pull request from a commit 2026-08-22 09:34:35 +00:00

stdb: fix five silent failures, and delete three required arguments (#111)

yuris referenced this pull request 2026-08-22 09:58:08 +00:00

deps: take extension-websocket 4.2.4, and drop the HTML5 patch #112

yuris referenced this pull request from a commit 2026-08-22 09:58:08 +00:00

test: regenerate the schema fixture, and pin the one ctx.db mistake

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

Subscribe

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!111

No description provided.