subscriptions, reducers: return handles instead of bare ids #38

Merged

yuris merged 1 commit from feat/29-handles into main 2026-08-19 17:26:55 +00:00

Conversation 1 Commits 1 Files changed 5 +986 -42

yuris commented 2026-08-19 16:52:56 +00:00

Owner

Copy link

Closes #29.

subscribe_query returned a bare query_id and call_reducer a bare request_id, with one (err, ...) callback each. Both now return a handle; both old forms keep working.

local sub = db:subscribe_query({ "SELECT * FROM message" })
sub:on_applied(fn)  sub:on_error(fn)  sub:is_active()  sub:is_ended()  sub:unsubscribe(cb)  sub.query_id

local call = db:call_reducer("send_message", { "yuris", "hello" })
call:on_ok(fn)  call:on_error(fn)  call:on_out_of_energy(fn)  call.request_id

Why each matters: a subscription that errored was silently dropped and the holder had no way to discover it except by having caught the callback at the time. And OutOfEnergy is the account's problem while a failed reducer is the module's — handlers.TransactionUpdate already separated all three outcomes internally and then flattened them into one error string.

Backwards compatibility

git diff on the test file removes exactly 5 lines, every one of them reading an id off the return value (rid.request_id, qid.query_id, …). No assertion label, expected value, or callback shape changed. The legacy Subscribe/InitialSubscription path, the flattened error strings, and every procedure assertion are byte-identical and still pass.

dev/example/main/smoke.script was not edited — it stores whatever subscribe_query returns and hands it back to unsubscribe, which now accepts either a handle or a raw id.

Design decisions worth an opinion

• Late attach fires immediately. The applied message can arrive before the caller attaches on_applied, so callbacks are retained and fire on attach — and on_applied is a list, firing again on every reconnect replay, not a one-shot.
• A reconnect walks an applied handle back to sent, so is_active() is false across the gap, because resubscribe_all drops the cache. sent is set before c.send so a transport that raises mid-replay cannot leave a handle claiming to be active.
• A set ends when the drop is issued, not when UnsubscribeMultiApplied lands. It is already out of query_sets and therefore unreplayable, and a second unsubscribe in that window must not send a second frame.
• The ended-guard lives in M.unsubscribe, not only on the handle method, so both entry points behave identically. An unknown raw id still goes out on the wire rather than faulting.
• TransactionUpdate's p.callback(...) is now nil-guarded — a callback-less call_reducer is normal with handles and would otherwise fault.

Verification

5 passed, 0 failed; stylua clean. +240 assertions in a new Handles section.

Falsifiability by mutation testing — 12 behaviours mutated in place, suite re-run, then restored:

mutant	failure
handle not marked applied	expected applied, got sent
late fire removed from on_applied	attached after the fact still fires: expected true, got false
ended-guard removed	dropping an already-ended set is a no-op: expected nil, got 3
raw id no longer resolves its handle	and the handle for it ends too: expected true, got false
raw-id wire fallback removed	connection.lua:279 attempt to index local 'sub' (a nil value)
OutOfEnergy not routed separately	expected out_of_energy, got error
SubscriptionError not propagated	expected no such table: \ghost`, got nil`
replay allocates a new handle	replay puts the same handle back in flight: expected sent, got applied
unsubscribe reply re-applies the handle	expected ended, got applied

Worth noting honestly: the first pass left two mutants surviving — the raw-id wire fallback and the unsubscribe reply re-applying the handle. Two tests were added specifically to kill them.

Deliberately not done

No timeouts. A pending call still lives until the connection drops. A timeout needs an owner in the update loop and a defined interaction with reconnect replay — a behaviour change, not a handle feature. The peer Godot SDK declares one and left it unimplemented, which is the warning. Recorded in the set_send docstring, the README, the code and the commit message.

No coroutine/await layer — separate opinion, separate issue.

Closes #29. `subscribe_query` returned a bare `query_id` and `call_reducer` a bare `request_id`, with one `(err, ...)` callback each. Both now return a handle; both old forms keep working. ```lua local sub = db:subscribe_query({ "SELECT * FROM message" }) sub:on_applied(fn) sub:on_error(fn) sub:is_active() sub:is_ended() sub:unsubscribe(cb) sub.query_id local call = db:call_reducer("send_message", { "yuris", "hello" }) call:on_ok(fn) call:on_error(fn) call:on_out_of_energy(fn) call.request_id ``` Why each matters: a subscription that **errored** was silently dropped and the holder had no way to discover it except by having caught the callback at the time. And `OutOfEnergy` is the account's problem while a failed reducer is the module's — `handlers.TransactionUpdate` already separated all three outcomes internally and then flattened them into one error string. ## Backwards compatibility `git diff` on the test file removes exactly **5 lines**, every one of them reading an id off the return value (`rid.request_id`, `qid.query_id`, …). No assertion label, expected value, or callback shape changed. The legacy `Subscribe`/`InitialSubscription` path, the flattened error strings, and every procedure assertion are byte-identical and still pass. `dev/example/main/smoke.script` was **not** edited — it stores whatever `subscribe_query` returns and hands it back to `unsubscribe`, which now accepts either a handle or a raw id. ## Design decisions worth an opinion - **Late attach fires immediately.** The applied message can arrive before the caller attaches `on_applied`, so callbacks are retained and fire on attach — and `on_applied` is a *list*, firing again on every reconnect replay, not a one-shot. - **A reconnect walks an applied handle back to `sent`**, so `is_active()` is false across the gap, because `resubscribe_all` drops the cache. `sent` is set *before* `c.send` so a transport that raises mid-replay cannot leave a handle claiming to be active. - **A set ends when the drop is *issued*, not when `UnsubscribeMultiApplied` lands.** It is already out of `query_sets` and therefore unreplayable, and a second `unsubscribe` in that window must not send a second frame. - **The ended-guard lives in `M.unsubscribe`**, not only on the handle method, so both entry points behave identically. An unknown raw id still goes out on the wire rather than faulting. - `TransactionUpdate`'s `p.callback(...)` is now nil-guarded — a callback-less `call_reducer` is normal with handles and would otherwise fault. ## Verification `5 passed, 0 failed`; stylua clean. +240 assertions in a new Handles section. Falsifiability by **mutation testing** — 12 behaviours mutated in place, suite re-run, then restored: | mutant | failure | |---|---| | handle not marked applied | `expected applied, got sent` | | late fire removed from `on_applied` | `attached after the fact still fires: expected true, got false` | | ended-guard removed | `dropping an already-ended set is a no-op: expected nil, got 3` | | raw id no longer resolves its handle | `and the handle for it ends too: expected true, got false` | | raw-id wire fallback removed | `connection.lua:279 attempt to index local 'sub' (a nil value)` | | `OutOfEnergy` not routed separately | `expected out_of_energy, got error` | | `SubscriptionError` not propagated | `expected no such table: \`ghost\`, got nil` | | replay allocates a new handle | `replay puts the same handle back in flight: expected sent, got applied` | | unsubscribe reply re-applies the handle | `expected ended, got applied` | Worth noting honestly: the **first** pass left two mutants surviving — the raw-id wire fallback and the unsubscribe reply re-applying the handle. Two tests were added specifically to kill them. ## Deliberately not done **No timeouts.** A pending call still lives until the connection drops. A timeout needs an owner in the update loop and a defined interaction with reconnect replay — a behaviour change, not a handle feature. The peer Godot SDK declares one and left it unimplemented, which is the warning. Recorded in the `set_send` docstring, the README, the code and the commit message. No coroutine/await layer — separate opinion, separate issue.

yuris added 1 commit 2026-08-19 16:52:56 +00:00

subscriptions, reducers: return handles instead of bare ids (#29) ... 4f8fe6cbb4

Two async operations handed back a number and took one `(err, ...)`
callback, and both lost information the wire already carried.

A `subscribe_query` caller held a `query_id` with no way to ask whether
it was still live. That matters because of reconnect: query sets are
replayed under their ORIGINAL id, so a held id stays valid across a
drop - except for a set the server REJECTED, which is dropped and never
replayed. The only notice of that was the callback at the time; a holder
who missed it had no way to find out. `sub.state` now walks
pending -> sent -> applied -> ended | error, so the drop is
discoverable afterwards, and the replay path re-sends the SAME handle
object rather than a fresh one, which is what keeps a live handle live
and re-fires its on_applied.

A `call_reducer` caller got one error string for three distinct wire
outcomes. TransactionUpdate already separated Committed, Failed(message)
and OutOfEnergy internally and then flattened the last two. An exhausted
account is not a broken module - one is the player's billing problem, the
other a bug to report - and a game may well surface them differently.
on_ok / on_error / on_out_of_energy keep them apart.

Registering a callback for an outcome that already arrived fires it
immediately. The alternative is the classic promise footgun: nothing
orders registration against the socket, so an applied frame can and does
land before the caller reaches its next line, and a "too late" callback
would silently never run.

Additive, because 1.0 freezes both forms:

  - the `(err, ...)` callback argument is untouched, flattened error
    string included. The only test lines that changed read the id off
    the return value (`rid.request_id`, `qid.query_id`); every legacy
    assertion is byte-identical and still passes.
  - `unsubscribe` takes a handle OR a raw numeric query_id, so
    dev/example/main/smoke.script needed no edit: it stores whatever
    subscribe_query returned and passes it straight back.
  - dropping an already-ended set is a no-op returning nil rather than an
    error. A caller cannot be expected to race its own unsubscribe
    against a SubscriptionError.

Deliberately out of scope, both from the issue: no timeouts, and no
coroutine/await layer. A pending call still lives until the connection
drops. A timeout needs a real owner in the update loop and a defined
interaction with reconnect replay, which is a behaviour change rather
than a handle feature - the peer Godot SDK declares `reducer_call_timeout`
and leaves it unimplemented, which is a fair warning. `await` is a
separate opinion and a separate issue.

yuris commented 2026-08-19 17:07:35 +00:00

Author

Owner

Copy link

Review — automated reviewer, posted for the record

All verification complete. Here is the review.

---

Review: PR #38 — handles for subscriptions and reducer calls

Verdict: the design is right and the compatibility claims all hold, but there is one blocker — a reachable sequence that leaves a handle permanently claiming is_active() == true for a subscription the server has dropped. That is precisely the "worse than not having the handle" failure mode. It is a two-line fix and I have verified it.

Everything below was checked by running code. Line refs are to feat/29-handles @ 4f8fe6c.

What I verified as correct

Conformance passes, and phase 3 genuinely travels the handle path. nix develop .#conformance passed twice on the unmodified branch. Because "it passed" is not proof the handle branch ran, I instrumented smoke.script phase 3 with type and state assertions and re-ran the full harness in a real Defold engine against the live server:

SMOKE: REVIEW: handle state=sent query_id=1 is_active=false
SMOKE: query set 1 subscribed
SMOKE: REVIEW: handle applied->ended via the handle branch
SMOKE: PHASE3 OK unsubscribed query set 1; cache drained

The run asserts type(...) == "table", the presence of is_active/unsubscribe, is_active() true after the server's real SubscribeMultiApplied, and is_ended() true after unsubscribe. Any of those failing would have failed the run. The author's claim that smoke.script needed no edit is correct, and phase 3 exercised M.unsubscribe's type(query_id) == "table" branch, not the raw-id fallback.

Legacy compatibility is real, exactly as claimed. The diff removes exactly 5 lines, all id reads. Checking out origin/main's test_connection.lua against the branch's connection.lua fails only at test_connection.lua:95 — one of those 5. Applying just the 5 id reads and nothing else: 5 passed, 0 failed. No assertion label, expected value, or callback shape changed. The flattened error string is unchanged for all three outcomes — connection.lua:517 is untouched context in the diff, and Committed → (nil, msg), Failed → (message, msg), OutOfEnergy → ("out of energy", msg) all still hold.

All three reconnect claims hold (connection.lua:216, 363-376): an applied handle goes back to sent, on_applied is a list that fires again per replay, and replay reuses the same handle object. Marking sent before c.send is the right call and the raising-transport test earns its keep.

No double-fire, and late-attach arguments are identical to the eager path. I probed the classic bug directly. Re-entrant registration from inside a firing callback fires exactly once (fire snapshots #list in Lua 5.1's numeric for, and the late-attach branch covers the new entry). Argument parity, eager vs. late, for on_ok / on_error / on_out_of_energy / on_error on a subscription: identical arity and identical values in all four cases. register's outcome == "error" special case is correct.

error → reconnect correctly does not replay (resubscribe_all returns 0, state stays error) — though nothing in the suite asserts it. ended → error is benign: both are is_ended(), is_active() stays false. So the subscription_fail path needs no guard; only subscription_apply can un-end a handle.

---

Blocker 1 — an unsubscribe issued before the subscribe reply permanently corrupts the handle

stdb/connection.lua:212 parks a back-reference p.sub in c.pending. M.unsubscribe (274-296) clears c.query_sets[id] and sets sub.state = "ended" — but leaves that in-flight pending entry alone. When the subscribe reply lands, subscription_applied (465-467) unconditionally calls subscription_apply(p.sub, msg), which sets state = "applied" (135-139).

The real 2.8.2 server answers both frames. Captured live, subscribe and unsubscribe sent in one tick:

SubscribeMultiApplied(request_id=10, query_id={"id":5})
UnsubscribeMultiApplied(request_id=11, query_id={"id":5})

Replaying those real captured frames through the unmodified SDK, driven by the most ordinary code a game can write:

local sub = db:subscribe_query({ "SELECT * FROM message" })
sub:unsubscribe()

after unsubscribe    state=ended is_active=false is_ended=true
  <- SubscribeMultiApplied    state=applied is_active=true
  <- UnsubscribeMultiApplied  state=applied is_active=true

on_applied fired AFTER unsubscribe: 1
cached rows (server drained them):  0
in conn.query_sets:                 false
replayed on reconnect:              0

FINAL: sub.state=applied  is_active()=true  is_ended()=false

The handle reports applied / is_active() == true / is_ended() == false forever for a set that does not exist. It is unrecoverable: the set is out of query_sets, so no future frame or reconnect can correct it. on_applied also fires after teardown, so a consumer that repopulates UI from the applied message repopulates it after being told to stop.

There is a second independent route: unsubscribe during the reconnect gap, while the replayed set is sent. Same corrupt terminal state.

Pre-handles this sequence was merely odd (the callback fired for rows that were then deleted). The permanent lying state is new, and is_active() is exactly the question the handle exists to answer.

Fix — one guard at the single shared mutation point, rather than chasing c.pending from M.unsubscribe:

local function subscription_apply(sub, msg)
	-- An unsubscribe issued while the subscribe was still in flight leaves the
	-- reply behind it: the server answers BOTH, verified against 2.8.2. Applying
	-- it would resurrect an ended handle into a permanent `is_active()` lie,
	-- since the set is already out of `query_sets` and can never be corrected.
	if sub:is_ended() then
		return
	end
	sub.state = "applied"

Verified: both routes now end at state=ended is_active=false is_ended=true, and the author's suite is still 5 passed, 0 failed with the test file untouched. Worth adding as a test — it is the one reachable transition the new suite misses, and ended → applied is the transition that matters most.

Should-fix 2 — pending leak and a swallowed callback when SubscriptionError answers an UnsubscribeMulti

connection.lua:542 clears c.pending[request_id] only when p.kind == "subscribe". A live server really does answer an unsubscribe with SubscriptionError:

SEND {"UnsubscribeMulti":{"request_id":2,"query_id":{"id":77}}}
RECV SubscriptionError {"request_id":{"some":2},"query_id":{"some":77},
                        "error":"Subscription not found: (…)"}

It also does this for a double unsubscribe of a real set (request_id 4, query_id 1 → Subscription not found), so this is not an exotic path. Result: c.pending[request_id] is never cleared — a leak for the life of the connection — and the unsubscribe callback never fires despite the docstring promising it.

This guard predates the PR, but the PR adds orphan_conn:unsubscribe(77) as a blessed test case, and 1.0 freezes it. Measured: pending after the error reply: 1, unsubscribe callback fired: false, kind still parked: unsubscribe.

Fix: drop the kind discriminator — if p then. Verified: pending goes to 0, the callback fires with the server's error, suite still green.

Should-fix 3 — a Call handle is stranded at pending forever when the socket dies

M.set_send (385-389) wipes c.pending, orphaning every in-flight Call. Measured after a reconnect with two calls in flight:

outcome after the socket died     pending  pending
pending entries                   0
any registration ever fires       false
indistinguishable from in-flight  true

The docstring frames this as "exactly the fate its callback already had". That is true but it understates the regression: pre-handles the loss was invisible, whereas now the handle advertises outcome == "pending" — a status field that will never change and cannot be distinguished from still-in-flight. Worse, on_ok/on_error attached afterwards silently never run, which is the precise failure the late-attach rule (connection.lua:62-65) exists to prevent.

This needs no timeout machinery. The socket's death is a definite terminal event that set_send already knows about and currently discards:

for _, p in pairs(c.pending) do
	if p.call and p.call.outcome == "pending" then
		call_settle(p.call, "error", "connection lost")
	end
end
c.pending = {}

Verified: outcome becomes error, registrations fire, and the legacy (err, update) callback is deliberately not invoked — it never fired here before, and that shape is frozen. Suite still 5 passed, 0 failed; stylua --check clean. This is more valuable than a timeout and should land before the freeze.

Should-fix 4 — a callback passed to a no-op unsubscribe is silently dropped

M.unsubscribe:276-278 returns nil early on an ended set without touching callback. Measured on a rejected set: returned nil, callback never fires. The no-op-instead-of-error decision is right, but silently discarding the callback contradicts the PR's own stated principle. Either fire it immediately with the terminal reason (consistent with every on_*) or document that a no-op never calls back.

Should-fix 5 — no way to detach on_applied, and repeat-firing makes that matter

Repeat-firing is the right contract to freeze: resubscribe_all empties the cache, so a consumer holding derived state must be told again; one-shot would silently break every consumer after its first reconnect. Keep it.

But there is no off, no once, and on_applied returns self — so there is not even a token to hold, and returning self forecloses returning a canceller later. The natural "(re)bind whenever the set is live" pattern inside an event handler degrades quietly:

registrations / total invocations        5  20
invocations for the LAST reconnect alone 5

Unbounded list growth and N invocations per logical event, with no escape hatch and nothing in the API to hint at one. Add sub:off_applied(fn) (or have on_applied return a detacher) before freezing. At minimum, document that registration is permanent and belongs outside event handlers.

Should-fix 6 — the two vocabularies diverge more than the domain justifies

Subscription  is_active is_ended on_applied on_error unsubscribe   sub.state
Call          on_ok on_error on_out_of_energy                      call.outcome

on_applied vs on_ok is defensible: a subscription is long-lived and repeating, a call is one-shot, so different verbs carry real information. Two things are not defensible and both get frozen:

• sub.state vs call.outcome — one concept, two names, for no domain reason. Converge on state. They already share the pending and error values.
• Call has no predicates. Subscription gets is_active()/is_ended(); a Call holder must string-compare call.outcome == "pending" — exactly what predicates exist to avoid. call:is_pending() is one line and makes the pair coherent.

Nits

• sub.message retains the entire applied payload (connection.lua:137), and the connection holds the handle via query_sets, so it is retained whether or not the consumer keeps the handle. Measured: ~2.0 MB retained for a 4000-row set, of which ~1.9 MB is sub.message. Inherent to the late-attach contract and not a growing leak (re-stored, not accumulated, per replay), but Defold ships to mobile and this deserves a line in the README.
• sub.message is overloaded: subscription_fail (144) overwrites it with the SubscriptionError frame, so the field means two different things and a reader cannot tell which. Verified. A separate sub.applied_message would be clearer.
• README :437 — "Each returns a request_id unless noted" is now false for two of the seven methods. And yes: document the handles as classes in the API tables, not only in prose. sub.query_id, sub.state, call.request_id, call.outcome are frozen public fields and belong in a table alongside the methods.
• TransactionUpdateLight never clears c.pending (529-532), so a light update for our own call leaks the entry and pins outcome = "pending" forever. Pre-existing, and smoke.script already asserts the server does not do this to the caller — but handles turn an invisible leak into a visible stuck state. Cheap to close alongside fix 3.
• resubscribe_all called twice without a socket swap stacks pending subscribe entries and fires on_applied once per stacked entry (measured: 3 fires for one logical reconnect). init.lua always calls set_send first, so this is only reachable by calling the public resubscribe_all directly. Low risk, worth knowing.
• If c.send raises inside subscribe_query, the exception propagates before the handle is returned, yet the set is already registered in query_sets and will be replayed. The caller can never unsubscribe what it cannot reference. Pre-existing in shape, marginally worse now.

On the deliberate omission of timeouts

Correct call, and correctly argued: a timeout needs an owner in the update loop and a defined interaction with replay. Shipping half a timeout is worse. But the current state is not "no timeout" — it is "a pending call silently becomes unobservable garbage", which fix 3 resolves without any timeout at all. Land fix 3 and the omission becomes genuinely safe to freeze, because every terminal outcome the SDK can know about is then reported. I would not reserve a timeout argument in the handle API; the natural home is a connection-level option, and adding an argument later is backwards-compatible anyway.

Does it close #29?

Yes on scope, with one acceptance criterion not met. All four required unit-test areas are present and genuine: state transitions including the error path, no-op unsubscribe on an ended set, each of the three reducer outcomes reaching its own callback, and the legacy form unchanged. Both "existing callback forms keep working byte for byte" and "no timeout, no await" constraints are honoured, and I verified the first independently. The shape matches the issue's sketch exactly.

Not met: the issue's own framing is that the handle makes subscription liveness observable. Blocker 1 means is_active() can be observably wrong, permanently, for an ordinary two-line call sequence. That is the acceptance criterion in spirit, and it is the one thing that must not ship into a frozen API.

Recommendation: land fixes 1–3 (about 15 lines total, all verified — author's suite untouched and green, stylua clean, conformance passing at engine level with the patch applied), settle the state/outcome naming and Call predicates while they are still free to change, then merge. Fixes 4–6 and the README items are cheap and worth doing in the same pass since 1.0 freezes them.

I did not edit, push, comment on, or merge anything; all work was done in a throwaway clone at /tmp/review-38.

## Review — automated reviewer, posted for the record All verification complete. Here is the review. --- # Review: PR #38 — handles for subscriptions and reducer calls **Verdict:** the design is right and the compatibility claims all hold, but there is **one blocker** — a reachable sequence that leaves a handle permanently claiming `is_active() == true` for a subscription the server has dropped. That is precisely the "worse than not having the handle" failure mode. It is a two-line fix and I have verified it. Everything below was checked by running code. Line refs are to `feat/29-handles` @ `4f8fe6c`. ## What I verified as correct **Conformance passes, and phase 3 genuinely travels the handle path.** `nix develop .#conformance` passed twice on the unmodified branch. Because "it passed" is not proof the handle branch ran, I instrumented `smoke.script` phase 3 with type and state assertions and re-ran the full harness in a real Defold engine against the live server: ``` SMOKE: REVIEW: handle state=sent query_id=1 is_active=false SMOKE: query set 1 subscribed SMOKE: REVIEW: handle applied->ended via the handle branch SMOKE: PHASE3 OK unsubscribed query set 1; cache drained ``` The run asserts `type(...) == "table"`, the presence of `is_active`/`unsubscribe`, `is_active()` true after the server's real `SubscribeMultiApplied`, and `is_ended()` true after `unsubscribe`. Any of those failing would have failed the run. **The author's claim that `smoke.script` needed no edit is correct**, and phase 3 exercised `M.unsubscribe`'s `type(query_id) == "table"` branch, not the raw-id fallback. **Legacy compatibility is real, exactly as claimed.** The diff removes exactly 5 lines, all id reads. Checking out `origin/main`'s `test_connection.lua` against the branch's `connection.lua` fails only at `test_connection.lua:95` — one of those 5. Applying just the 5 id reads and nothing else: `5 passed, 0 failed`. No assertion label, expected value, or callback shape changed. The flattened error string is unchanged for all three outcomes — `connection.lua:517` is untouched context in the diff, and `Committed → (nil, msg)`, `Failed → (message, msg)`, `OutOfEnergy → ("out of energy", msg)` all still hold. **All three reconnect claims hold** (`connection.lua:216`, `363-376`): an applied handle goes back to `sent`, `on_applied` is a list that fires again per replay, and replay reuses the same handle object. Marking `sent` *before* `c.send` is the right call and the raising-transport test earns its keep. **No double-fire, and late-attach arguments are identical to the eager path.** I probed the classic bug directly. Re-entrant registration from inside a firing callback fires exactly once (`fire` snapshots `#list` in Lua 5.1's numeric `for`, and the late-attach branch covers the new entry). Argument parity, eager vs. late, for `on_ok` / `on_error` / `on_out_of_energy` / `on_error` on a subscription: identical arity and identical values in all four cases. `register`'s `outcome == "error"` special case is correct. **`error → reconnect`** correctly does not replay (`resubscribe_all` returns 0, state stays `error`) — though nothing in the suite asserts it. **`ended → error`** is benign: both are `is_ended()`, `is_active()` stays false. So the `subscription_fail` path needs no guard; only `subscription_apply` can un-end a handle. --- ## Blocker 1 — an unsubscribe issued before the subscribe reply permanently corrupts the handle `stdb/connection.lua:212` parks a back-reference `p.sub` in `c.pending`. `M.unsubscribe` (`274-296`) clears `c.query_sets[id]` and sets `sub.state = "ended"` — but leaves that in-flight pending entry alone. When the subscribe reply lands, `subscription_applied` (`465-467`) unconditionally calls `subscription_apply(p.sub, msg)`, which sets `state = "applied"` (`135-139`). The real 2.8.2 server answers **both** frames. Captured live, subscribe and unsubscribe sent in one tick: ``` SubscribeMultiApplied(request_id=10, query_id={"id":5}) UnsubscribeMultiApplied(request_id=11, query_id={"id":5}) ``` Replaying those **real captured frames** through the unmodified SDK, driven by the most ordinary code a game can write: ```lua local sub = db:subscribe_query({ "SELECT * FROM message" }) sub:unsubscribe() ``` ``` after unsubscribe state=ended is_active=false is_ended=true <- SubscribeMultiApplied state=applied is_active=true <- UnsubscribeMultiApplied state=applied is_active=true on_applied fired AFTER unsubscribe: 1 cached rows (server drained them): 0 in conn.query_sets: false replayed on reconnect: 0 FINAL: sub.state=applied is_active()=true is_ended()=false ``` The handle reports `applied` / `is_active() == true` / `is_ended() == false` **forever** for a set that does not exist. It is unrecoverable: the set is out of `query_sets`, so no future frame or reconnect can correct it. `on_applied` also fires *after* teardown, so a consumer that repopulates UI from the applied message repopulates it after being told to stop. There is a second independent route: `unsubscribe` during the reconnect gap, while the replayed set is `sent`. Same corrupt terminal state. Pre-handles this sequence was merely odd (the callback fired for rows that were then deleted). The **permanent lying state is new**, and `is_active()` is exactly the question the handle exists to answer. **Fix** — one guard at the single shared mutation point, rather than chasing `c.pending` from `M.unsubscribe`: ```lua local function subscription_apply(sub, msg) -- An unsubscribe issued while the subscribe was still in flight leaves the -- reply behind it: the server answers BOTH, verified against 2.8.2. Applying -- it would resurrect an ended handle into a permanent `is_active()` lie, -- since the set is already out of `query_sets` and can never be corrected. if sub:is_ended() then return end sub.state = "applied" ``` Verified: both routes now end at `state=ended is_active=false is_ended=true`, and the author's suite is still `5 passed, 0 failed` with the test file untouched. Worth adding as a test — it is the one reachable transition the new suite misses, and `ended → applied` is the transition that matters most. ## Should-fix 2 — `pending` leak and a swallowed callback when `SubscriptionError` answers an `UnsubscribeMulti` `connection.lua:542` clears `c.pending[request_id]` only when `p.kind == "subscribe"`. A live server really does answer an unsubscribe with `SubscriptionError`: ``` SEND {"UnsubscribeMulti":{"request_id":2,"query_id":{"id":77}}} RECV SubscriptionError {"request_id":{"some":2},"query_id":{"some":77}, "error":"Subscription not found: (…)"} ``` It also does this for a **double unsubscribe of a real set** (`request_id 4, query_id 1` → `Subscription not found`), so this is not an exotic path. Result: `c.pending[request_id]` is never cleared — a leak for the life of the connection — and the unsubscribe callback never fires despite the docstring promising it. This guard predates the PR, but the PR adds `orphan_conn:unsubscribe(77)` as a *blessed* test case, and 1.0 freezes it. Measured: `pending after the error reply: 1`, `unsubscribe callback fired: false`, `kind still parked: unsubscribe`. **Fix:** drop the `kind` discriminator — `if p then`. Verified: pending goes to 0, the callback fires with the server's error, suite still green. ## Should-fix 3 — a `Call` handle is stranded at `pending` forever when the socket dies `M.set_send` (`385-389`) wipes `c.pending`, orphaning every in-flight `Call`. Measured after a reconnect with two calls in flight: ``` outcome after the socket died pending pending pending entries 0 any registration ever fires false indistinguishable from in-flight true ``` The docstring frames this as "exactly the fate its callback already had". That is true but it understates the regression: pre-handles the loss was invisible, whereas now the handle *advertises* `outcome == "pending"` — a status field that will never change and cannot be distinguished from still-in-flight. Worse, `on_ok`/`on_error` attached afterwards silently never run, which is the precise failure the late-attach rule (`connection.lua:62-65`) exists to prevent. This needs **no timeout machinery**. The socket's death is a definite terminal event that `set_send` already knows about and currently discards: ```lua for _, p in pairs(c.pending) do if p.call and p.call.outcome == "pending" then call_settle(p.call, "error", "connection lost") end end c.pending = {} ``` Verified: `outcome` becomes `error`, registrations fire, and the legacy `(err, update)` callback is deliberately **not** invoked — it never fired here before, and that shape is frozen. Suite still `5 passed, 0 failed`; `stylua --check` clean. This is more valuable than a timeout and should land before the freeze. ## Should-fix 4 — a callback passed to a no-op `unsubscribe` is silently dropped `M.unsubscribe:276-278` returns `nil` early on an ended set without touching `callback`. Measured on a rejected set: `returned nil`, callback never fires. The no-op-instead-of-error decision is right, but silently discarding the callback contradicts the PR's own stated principle. Either fire it immediately with the terminal reason (consistent with every `on_*`) or document that a no-op never calls back. ## Should-fix 5 — no way to detach `on_applied`, and repeat-firing makes that matter Repeat-firing **is** the right contract to freeze: `resubscribe_all` empties the cache, so a consumer holding derived state must be told again; one-shot would silently break every consumer after its first reconnect. Keep it. But there is no `off`, no `once`, and `on_applied` returns `self` — so there is not even a token to hold, and returning `self` forecloses returning a canceller later. The natural "(re)bind whenever the set is live" pattern inside an event handler degrades quietly: ``` registrations / total invocations 5 20 invocations for the LAST reconnect alone 5 ``` Unbounded list growth and N invocations per logical event, with no escape hatch and nothing in the API to hint at one. Add `sub:off_applied(fn)` (or have `on_applied` return a detacher) before freezing. At minimum, document that registration is permanent and belongs outside event handlers. ## Should-fix 6 — the two vocabularies diverge more than the domain justifies ``` Subscription is_active is_ended on_applied on_error unsubscribe sub.state Call on_ok on_error on_out_of_energy call.outcome ``` `on_applied` vs `on_ok` is *defensible*: a subscription is long-lived and repeating, a call is one-shot, so different verbs carry real information. Two things are not defensible and both get frozen: - **`sub.state` vs `call.outcome`** — one concept, two names, for no domain reason. Converge on `state`. They already share the `pending` and `error` values. - **`Call` has no predicates.** `Subscription` gets `is_active()`/`is_ended()`; a `Call` holder must string-compare `call.outcome == "pending"` — exactly what predicates exist to avoid. `call:is_pending()` is one line and makes the pair coherent. ## Nits - **`sub.message` retains the entire applied payload** (`connection.lua:137`), and the *connection* holds the handle via `query_sets`, so it is retained whether or not the consumer keeps the handle. Measured: **~2.0 MB retained** for a 4000-row set, of which ~1.9 MB is `sub.message`. Inherent to the late-attach contract and not a growing leak (re-stored, not accumulated, per replay), but Defold ships to mobile and this deserves a line in the README. - **`sub.message` is overloaded**: `subscription_fail` (`144`) overwrites it with the `SubscriptionError` frame, so the field means two different things and a reader cannot tell which. Verified. A separate `sub.applied_message` would be clearer. - **README `:437`** — "Each returns a `request_id` unless noted" is now false for two of the seven methods. And yes: **document the handles as classes in the API tables**, not only in prose. `sub.query_id`, `sub.state`, `call.request_id`, `call.outcome` are frozen public fields and belong in a table alongside the methods. - **`TransactionUpdateLight` never clears `c.pending`** (`529-532`), so a light update for our own call leaks the entry and pins `outcome = "pending"` forever. Pre-existing, and `smoke.script` already asserts the server does not do this to the caller — but handles turn an invisible leak into a visible stuck state. Cheap to close alongside fix 3. - **`resubscribe_all` called twice without a socket swap** stacks pending subscribe entries and fires `on_applied` once per stacked entry (measured: 3 fires for one logical reconnect). `init.lua` always calls `set_send` first, so this is only reachable by calling the public `resubscribe_all` directly. Low risk, worth knowing. - **If `c.send` raises inside `subscribe_query`**, the exception propagates before the handle is returned, yet the set is already registered in `query_sets` and will be replayed. The caller can never unsubscribe what it cannot reference. Pre-existing in shape, marginally worse now. ## On the deliberate omission of timeouts Correct call, and correctly argued: a timeout needs an owner in the update loop and a defined interaction with replay. Shipping half a timeout is worse. **But** the current state is not "no timeout" — it is "a pending call silently becomes unobservable garbage", which fix 3 resolves without any timeout at all. Land fix 3 and the omission becomes genuinely safe to freeze, because every terminal outcome the SDK *can* know about is then reported. I would not reserve a `timeout` argument in the handle API; the natural home is a connection-level option, and adding an argument later is backwards-compatible anyway. ## Does it close #29? **Yes on scope, with one acceptance criterion not met.** All four required unit-test areas are present and genuine: state transitions including the error path, no-op unsubscribe on an ended set, each of the three reducer outcomes reaching its own callback, and the legacy form unchanged. Both "existing callback forms keep working byte for byte" and "no timeout, no await" constraints are honoured, and I verified the first independently. The shape matches the issue's sketch exactly. Not met: the issue's own framing is that the handle makes subscription liveness **observable**. Blocker 1 means `is_active()` can be observably wrong, permanently, for an ordinary two-line call sequence. That is the acceptance criterion in spirit, and it is the one thing that must not ship into a frozen API. **Recommendation:** land fixes 1–3 (about 15 lines total, all verified — author's suite untouched and green, `stylua` clean, conformance passing at engine level with the patch applied), settle the `state`/`outcome` naming and `Call` predicates while they are still free to change, then merge. Fixes 4–6 and the README items are cheap and worth doing in the same pass since 1.0 freezes them. I did not edit, push, comment on, or merge anything; all work was done in a throwaway clone at `/tmp/review-38`.

yuris force-pushed feat/29-handles from 4f8fe6cbb4 to 928fa1d59f 2026-08-19 17:25:07 +00:00 Compare

yuris merged commit 573909a81c into main 2026-08-19 17:26:55 +00:00

yuris referenced this pull request from a commit 2026-08-19 17:26:56 +00:00

subscriptions, reducers: return handles instead of bare ids (#38)

yuris referenced this pull request 2026-08-19 17:27:25 +00:00

Procedure and one-off-query callbacks are silently lost when the socket dies #41

yuris referenced this pull request 2026-08-19 18:26:34 +00:00

v1.0.0 release checklist #24

yuris referenced this pull request 2026-08-20 01:45:51 +00:00

v1.0.0 release checklist #24

yuris referenced this pull request from a commit 2026-08-20 01:59:05 +00:00

connection: settle handle-free requests when the socket dies (#41)

yuris referenced this pull request 2026-08-20 02:00:31 +00:00

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

yuris referenced this pull request 2026-08-20 02:13:22 +00:00

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

yuris referenced this pull request 2026-08-21 05:46:14 +00:00

stdb: connect({ protocol = "v2" }), opt-in #84

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

No description provided.