yuris/spacetimedb-defold

Fork 0

auth: exchange a provider credential for a SpacetimeDB token (#31) #64

Merged

yuris merged 5 commits from feat/31-auth-exchange into main

2026-08-20 15:03:33 +00:00

yuris commented

2026-08-20 13:58:17 +00:00

Owner

Closes #31. Phase 3 of the v1.1.0 milestone (#59) — the headline feature, and
the largest gap against the peer SDK. Purely additive: a new module plus one new
seam. No existing signature changes.

3a — the endpoint spike, settled: branch C

Run against the flake-pinned standalone 2.8.2 before writing any code. There is
no exchange endpoint, and no way to point the server at a third-party issuer.

/v1/identity                  POST 200    <- anonymous mint, the only one
/v1/identity/token            GET/POST 404
/v1/token                     GET/POST 404
/v1/auth                      GET/POST 404
/v1/identity/exchange         GET/POST 404

spacetimedb-standalone start --help exposes exactly --jwt-pub-key-path and
--jwt-priv-key-path — a single keypair, no issuer list, no JWKS URL. So a
Google/Steam/Apple JWT cannot be made acceptable as ?token= by configuration on
this version, and branch A is out along with branch B.

Therefore the exchange target is the game's own backend, which verifies the
provider credential and obtains a SpacetimeDB identity — matching the peer SDK's
provider-agnostic posture. This changes nothing structurally: opts.url is a
configurable URL, so if a future server version grows an endpoint, callers point
at it instead.

Incidental, recorded but deliberately not pursued: a malformed bearer token
panicked a tokio worker (spacetimedb-jsonwebtoken-9.3.0/src/decoding.rs:199,
"entered unreachable code") and took the server process down. Upstream behaviour
in a pinned dependency, out of scope here — but one more argument for branch C's
shape, since a third-party credential should never be presented at the
SpacetimeDB edge.

3b — the `http` seam

stdb/init.lua gains M.http_seam(), placed immediately after M.sys_storage
and mirroring it exactly — a factory returning a table of closures, so tests
substitute a table and skip the engine. This is the SDK's first HTTP
(grep -rn "http\." stdb/ returned nothing before this), so the shape is set
here.

Callback contract: (err, status, body). err is a string only for a
transport failure; any HTTP status, 4xx and 5xx included, arrives as a status
with err = nil.

The res.status == 0 transport-failure convention was verified against engine
source, not assumed — and this is where it could have gone wrong:

engine/dlib/src/dlib/http/http_service.cpp:637-648 — on RESULT_OK or
RESULT_NOT_200_OK it sends the real status; otherwise SendResponse(request, 0, ...).
Invalid URL → 0 at :536/:552; cannot-create-connection → 0 at :658.
script_http.cpp:158 passes it through untouched.
Defold's own engine test asserts it: test_http.lua.raw:79-87 expects
status == 0 for an unresolvable host while a real 404 asserts 404, and
test_http_timeout.lua.raw expects 0 for every timeout.
The HTML5/XHR path reports 0 too (script_http_js.cpp OnHttpError), so one
detection covers both targets.
Defold's published http docs do not document this convention; source and
the engine's own tests are the authority.

Two traps that would have made a naive check wrong, both now recorded in the
seam's comment so nobody re-derives them:

res.response is "" on transport failure, not nil — never key off it.
res.error is not a transport indicator; it is set only to
"Failed to write to temp file" for a failed download-to-disc.

3c — `stdb/auth.lua`

One public function, auth.exchange(opts, cb). cb fires exactly once.

Each decision, with its reason:

The credential travels in the request body, never in an Authorization
header. The exchange target is not the resource server; a header would invite
a proxy to forward it as one. Body is json.encode({ token = credential }),
header is Content-Type: application/json, and the tests assert there is no
other header and no other field.
storage defaults to nil — no caching. Deliberately unlike connect,
which defaults to sys_storage(). An identity token is the SDK's own to
persist; a provider-exchanged token is the game's credential, and the SDK must
not silently write it to disk. The caller opts in.
Cache key is the caller-supplied provider string. No JWT parsing anywhere
— so no base64url decoder ships, and there is no path by which an unverified
claim could be mistaken for an authorization decision. Stored as
{ [provider] = token }, merged on save so a second provider cannot evict
the first.
A cache hit short-circuits and makes no request.
The cached token is not validated locally. Verifying one needs a round trip,
and connect already handles a rejected token by reconnecting, so a stale entry
surfaces as a failure at connect. Clearing the cache is the caller's move —
documented loudly in the README. No expiry parsing, which would need the JWT
decode this scope excludes.
4xx does not retry. The load-bearing failure decision: a rejected credential
will not be accepted on the second ask, so retrying would mean three more
rejections and a 3.5s wait before the game learns the player is not signed in.
5xx and transport failures do retry.
The retry shares nothing with connection.backoff_delay — four lines of its
own arithmetic. The issue is explicit that the exchange's retry must not be
entangled with the socket reconnect; sharing the function would make one knob
move both.
Timeout is per attempt, not total, armed after the request and skipped if
the request already answered — because the seam is free to call back
synchronously (the tests do), and a timeout armed first would fire before the
request was ever made. A per-attempt settled flag discards a late answer; a
separate finished flag keeps cb to one call.
Never logs the credential or the token. stdb/auth.lua contains no print
at all, and no response body is ever interpolated into a message — a body may
itself carry a token. Errors name only a status and a count.
Missing url/provider/token/json raise at level 3, blaming
exchange's caller, matching the err() helpers in cache/protocol/schema/
tables. An empty-string provider counts as missing: "" would collapse
every provider into one cache entry.
A malformed body and a tokenless body are the same error. The decode is
pcalled so a bad body never escapes as a Lua error; to a caller both mean the
endpoint returned no token. A non-string token is also not a token.

json is required, no default, matching connect. timer is defaulted at call
time rather than load time, because bare Lua has no timer global and a module
that errored on require would take the unit suite with it. http resolves
lazily to avoid a circular require with stdb/init.lua.

3d — tests

New dev/test/test_auth.lua, 377 lines; the runner discovers it by glob, so no
registration. Suites went 6 → 7, 0 failed. Everything runs with no engine and
no clock: the http and timer seams drive retries, backoffs and timeouts
synchronously rather than sleeping.

Beyond the issue's acceptance list, the ones worth naming:

the backoff doubling asserted rather than assumed (0.5, 1, 2), and that
three retries means exactly three backoffs and no fourth;
timer queue order proving the timeout is per attempt: timeout, backoff,
timeout, … 4 timeouts + 3 backoffs = 7 entries;
a timed-out attempt whose answer arrives late — discarded, and cannot fire
the callback twice;
a timeout followed by a real transport failure reports the transport failure,
so the error names what actually happened last;
an exhausted 5xx reports its status, not "transport failure", because there
was no transport failure;
4xx makes exactly one request and schedules zero timers, so there is no
hidden backoff wait either;
two providers cached side by side, neither clobbering the other;
no storage seam → two exchanges make two requests, i.e. nothing is written.

The redaction test collects every string the SDK can produce — callback
errors, raised errors, anything printed — across every failure mode plus the
success paths, including response bodies that themselves carry the secrets, and
asserts none contains the credential or the token. It also guards against passing
vacuously: it asserts the sweep actually collected >8 composed strings, and that
the token really did reach the caller, so the check cannot be clean merely
because the module returned nothing.

Checks

lua dev/test/runner.lua          7 passed, 0 failed   (6 before)
stylua --check stdb dev/test     exit 0, silent

Cache-hit check — zero requests on the second call:

ok cache

Redaction and no-4xx-retry check:

ok redaction, requests=1: stdb.auth: exchange rejected with status 401

Conformance was not re-run: this adds no wire behaviour to the socket path. The
unit suite is the proof.

Also in this branch

readme: consumer archive is 12 entries now, not 11 — auth.lua ships inside
stdb/, so the README's measured archive figures went stale the moment this
module landed (54→56 tracked, 11→12 shipped, both re-measured). Phase 5's archive
audit compares against those numbers, so correcting them here keeps that gate
honest.

Closes #31. Phase 3 of the v1.1.0 milestone (#59) — the headline feature, and the largest gap against the peer SDK. Purely additive: a new module plus one new seam. No existing signature changes. ## 3a — the endpoint spike, settled: branch C Run against the flake-pinned standalone 2.8.2 before writing any code. **There is no exchange endpoint, and no way to point the server at a third-party issuer.** ``` /v1/identity POST 200 <- anonymous mint, the only one /v1/identity/token GET/POST 404 /v1/token GET/POST 404 /v1/auth GET/POST 404 /v1/identity/exchange GET/POST 404 ``` `spacetimedb-standalone start --help` exposes exactly `--jwt-pub-key-path` and `--jwt-priv-key-path` — a single keypair, no issuer list, no JWKS URL. So a Google/Steam/Apple JWT cannot be made acceptable as `?token=` by configuration on this version, and branch A is out along with branch B. **Therefore the exchange target is the game's own backend**, which verifies the provider credential and obtains a SpacetimeDB identity — matching the peer SDK's provider-agnostic posture. This changes nothing structurally: `opts.url` is a configurable URL, so if a future server version grows an endpoint, callers point at it instead. Incidental, recorded but deliberately not pursued: a malformed bearer token panicked a tokio worker (`spacetimedb-jsonwebtoken-9.3.0/src/decoding.rs:199`, "entered unreachable code") and took the server process down. Upstream behaviour in a pinned dependency, out of scope here — but one more argument for branch C's shape, since a third-party credential should never be presented at the SpacetimeDB edge. ## 3b — the `http` seam `stdb/init.lua` gains `M.http_seam()`, placed immediately after `M.sys_storage` and mirroring it exactly — a factory returning a table of closures, so tests substitute a table and skip the engine. This is the SDK's **first** HTTP (`grep -rn "http\." stdb/` returned nothing before this), so the shape is set here. Callback contract: `(err, status, body)`. `err` is a string **only** for a transport failure; any HTTP status, 4xx and 5xx included, arrives as a status with `err = nil`. **The `res.status == 0` transport-failure convention was verified against engine source, not assumed** — and this is where it could have gone wrong: - `engine/dlib/src/dlib/http/http_service.cpp:637-648` — on `RESULT_OK` or `RESULT_NOT_200_OK` it sends the real status; otherwise `SendResponse(request, 0, ...)`. Invalid URL → 0 at `:536`/`:552`; cannot-create-connection → 0 at `:658`. - `script_http.cpp:158` passes it through untouched. - Defold's **own engine test** asserts it: `test_http.lua.raw:79-87` expects `status == 0` for an unresolvable host while a real 404 asserts 404, and `test_http_timeout.lua.raw` expects 0 for every timeout. - The HTML5/XHR path reports 0 too (`script_http_js.cpp` `OnHttpError`), so one detection covers both targets. - Defold's published `http` docs do **not** document this convention; source and the engine's own tests are the authority. Two traps that would have made a naive check wrong, both now recorded in the seam's comment so nobody re-derives them: - `res.response` is `""` on transport failure, **not** `nil` — never key off it. - `res.error` is **not** a transport indicator; it is set only to "Failed to write to temp file" for a failed download-to-disc. ## 3c — `stdb/auth.lua` One public function, `auth.exchange(opts, cb)`. `cb` fires exactly once. Each decision, with its reason: - **The credential travels in the request body, never in an `Authorization` header.** The exchange target is not the resource server; a header would invite a proxy to forward it as one. Body is `json.encode({ token = credential })`, header is `Content-Type: application/json`, and the tests assert there is no other header and no other field. - **`storage` defaults to nil — no caching.** Deliberately unlike `connect`, which defaults to `sys_storage()`. An identity token is the SDK's own to persist; a provider-exchanged token is the *game's* credential, and the SDK must not silently write it to disk. The caller opts in. - **Cache key is the caller-supplied `provider` string. No JWT parsing anywhere** — so no base64url decoder ships, and there is no path by which an unverified claim could be mistaken for an authorization decision. Stored as `{ [provider] = token }`, **merged** on save so a second provider cannot evict the first. - **A cache hit short-circuits** and makes no request. - **The cached token is not validated locally.** Verifying one needs a round trip, and `connect` already handles a rejected token by reconnecting, so a stale entry surfaces as a failure at `connect`. Clearing the cache is the caller's move — documented loudly in the README. No expiry parsing, which would need the JWT decode this scope excludes. - **4xx does not retry.** The load-bearing failure decision: a rejected credential will not be accepted on the second ask, so retrying would mean three more rejections and a 3.5s wait before the game learns the player is not signed in. 5xx and transport failures do retry. - **The retry shares nothing with `connection.backoff_delay`** — four lines of its own arithmetic. The issue is explicit that the exchange's retry must not be entangled with the socket reconnect; sharing the function would make one knob move both. - **Timeout is per attempt, not total**, armed *after* the request and skipped if the request already answered — because the seam is free to call back synchronously (the tests do), and a timeout armed first would fire before the request was ever made. A per-attempt `settled` flag discards a late answer; a separate `finished` flag keeps `cb` to one call. - **Never logs the credential or the token.** `stdb/auth.lua` contains no `print` at all, and no response body is ever interpolated into a message — a body may itself carry a token. Errors name only a status and a count. - **Missing `url`/`provider`/`token`/`json` raise** at level 3, blaming `exchange`'s caller, matching the `err()` helpers in `cache`/`protocol`/`schema`/ `tables`. An empty-string `provider` counts as missing: `""` would collapse every provider into one cache entry. - **A malformed body and a tokenless body are the same error.** The decode is `pcall`ed so a bad body never escapes as a Lua error; to a caller both mean the endpoint returned no token. A non-string `token` is also not a token. `json` is required, no default, matching `connect`. `timer` is defaulted at call time rather than load time, because bare Lua has no `timer` global and a module that errored on require would take the unit suite with it. `http` resolves lazily to avoid a circular require with `stdb/init.lua`. ## 3d — tests New `dev/test/test_auth.lua`, 377 lines; the runner discovers it by glob, so no registration. Suites went **6 → 7, 0 failed**. Everything runs with no engine and no clock: the `http` and `timer` seams drive retries, backoffs and timeouts synchronously rather than sleeping. Beyond the issue's acceptance list, the ones worth naming: - the backoff **doubling** asserted rather than assumed (0.5, 1, 2), and that three retries means exactly three backoffs and no fourth; - timer queue **order** proving the timeout is per attempt: timeout, backoff, timeout, … 4 timeouts + 3 backoffs = 7 entries; - a timed-out attempt whose answer arrives **late** — discarded, and cannot fire the callback twice; - a timeout followed by a real transport failure reports the transport failure, so the error names what actually happened last; - an exhausted 5xx reports its **status**, not "transport failure", because there was no transport failure; - 4xx makes exactly one request and schedules **zero** timers, so there is no hidden backoff wait either; - two providers cached side by side, neither clobbering the other; - no storage seam → two exchanges make two requests, i.e. nothing is written. **The redaction test** collects every string the SDK can produce — callback errors, raised errors, anything printed — across every failure mode plus the success paths, including response bodies that themselves carry the secrets, and asserts none contains the credential or the token. It also guards against passing vacuously: it asserts the sweep actually collected >8 composed strings, and that the token really did reach the caller, so the check cannot be clean merely because the module returned nothing. ## Checks ``` lua dev/test/runner.lua 7 passed, 0 failed (6 before) stylua --check stdb dev/test exit 0, silent ``` Cache-hit check — zero requests on the second call: ``` ok cache ``` Redaction and no-4xx-retry check: ``` ok redaction, requests=1: stdb.auth: exchange rejected with status 401 ``` Conformance was not re-run: this adds no wire behaviour to the socket path. The unit suite is the proof. ## Also in this branch `readme: consumer archive is 12 entries now, not 11` — `auth.lua` ships inside `stdb/`, so the README's measured archive figures went stale the moment this module landed (54→56 tracked, 11→12 shipped, both re-measured). Phase 5's archive audit compares against those numbers, so correcting them here keeps that gate honest.

yuris added 2 commits

2026-08-20 13:58:17 +00:00

auth: exchange a provider credential for a SpacetimeDB token (#31 ) 92a488d2b1

The largest gap against the peer SDK: `connect` alone gives every player an
anonymous identity, which is useless the moment a player has an account. New
`stdb/auth.lua` with one function, `auth.exchange(opts, cb)`, plus the SDK's
first HTTP seam. Purely additive - `stdb/` is frozen, and nothing existing
changes shape.

The exchange target is the game's own backend, not the database. That is not a
preference, it is what 2.8.2 leaves us: `/v1/identity/token`, `/v1/token`,
`/v1/auth` and `/v1/identity/exchange` all answer 404, and the only identity
endpoint that exists - POST /v1/identity - mints an ANONYMOUS one. Pointing the
server at a third-party OIDC issuer is not on offer either:
`spacetimedb-standalone start` exposes a single --jwt-pub-key-path /
--jwt-priv-key-path keypair, no issuer list and no JWKS URL. So something the
game controls has to verify the credential and obtain the identity. `url` is
only a URL, so a later server version growing an endpoint changes nothing here.

Load-bearing decisions:

A 4xx does NOT retry. A rejected credential will not be accepted on the second
ask, so retrying it buys three more rejections and makes the game wait 3.5
seconds before telling the player they are not signed in. Transport failures
and 5xx do retry - those are the network and the server failing, not the
credential. A 5xx that outlives every retry reports its status rather than
"transport failure", because there was no transport failure to report.

`storage` defaults to nil, meaning no caching, which is the OPPOSITE of
`connect`'s default of sys_storage(). An identity token is the SDK's own to
persist; a provider-exchanged token is the game's credential, and the SDK must
not silently write it to a save file. The caller opts in, and the stored table
is merged per provider rather than overwritten so two providers coexist.

No JWT parsing, anywhere. The cache key is `provider`, a caller-supplied name -
the caller already knows which provider it used. That keeps a base64url decoder
out of the SDK and, more importantly, keeps an unverified claim from ever being
mistaken for an authorization decision. The consequence is stated loudly in the
README: a cached token is NOT validated locally, and clearing the cache is the
caller's move.

The retry shares nothing with connection.backoff_delay. Reusing it would make
one knob move both the credential exchange and the socket reconnect, and #31 is
explicit that the two must not be entangled. Four lines of arithmetic is a
cheaper price than that coupling.

The per-attempt timeout is armed AFTER the request and skipped if the attempt
already settled: the seam is free to answer synchronously (every unit test
does), and a timeout armed first would fire before the request was ever made.

Neither the credential nor the exchanged token reaches an error string or a log
line, and a response body is never interpolated into a message. auth.lua
contains no `print` at all. test_auth.lua collects every string the module
produces - callback errors, raised errors, prints - and fails if either secret
appears; a leak here would be invisible until it showed up in a crash report.

The http seam mirrors sys_storage exactly - a factory returning closures - so
tests substitute a plain table and skip the engine. `res.status == 0` as the
transport-failure signal is not assumed: Defold's engine forces status to a
literal 0 on any dmHttpClient failure (dlib/http/http_service.cpp, error branch
of the RESULT_OK/RESULT_NOT_200_OK check) and its own test asserts status 0 for
an unresolvable host, while a real 404 asserts 404. Two things that would have
made a naive check wrong and are now in the seam's comment: res.response is ""
rather than nil on failure, and res.error is set only for a failed
download-to-disc write, never for a transport error.

Suite goes 6 -> 7 files, 0 failed. Verified by mutation, not just by passing:
making 4xx retryable, overwriting the cache instead of merging, interpolating
the body into an error, and printing the credential each fail a named assertion.

readme: consumer archive is 12 entries now, not 11

ci / ci (pull_request) Successful in 1m26s

Details

a6e761a258

auth.lua is a new module inside the shipped `stdb/`, so both halves of the
figure move: 54 -> 56 tracked entries, 11 -> 12 after `.gitattributes` strips
`dev/`. Measured, not adjusted by hand: `git ls-tree -r -t HEAD | wc -l` and
`git archive HEAD | tar -t | wc -l`. The Phase 5 archive audit compares against
this number, so a stale one there reads as a regression.

yuris referenced this pull request

2026-08-20 14:15:21 +00:00

v1.1.0 release checklist #59

yuris force-pushed feat/31-auth-exchange from a6e761a258

ci / ci (pull_request) Successful in 1m26s

Details

to ef50c913d6

ci / ci (pull_request) Successful in 20s

Details

2026-08-20 14:41:01 +00:00

Compare

yuris commented

2026-08-20 14:41:30 +00:00

Author

Owner

Review actioned — all six findings. Verdict: NEEDS ANOTHER REVIEW, because
B1's fix touches stdb/init.lua and because my own verification then found a
second blocking defect that no unit test could have seen.

Thank you for B1 in particular. Constructing the real seam rather than trusting
fake_storage is exactly the move, and it found a defect that made the
documented configuration silently useless.

B1 (BLOCKING) — fixed in `sys_storage`, not by retreating from the docs

You were right that the suite structurally could not see this: fake_storage
returns whatever was saved unconditionally, so it modelled a seam the SDK does
not ship. Through the documented stdb.sys_storage("auth") every load missed —
re-exchanging the player's credential on every launch — and the merge re-read
through the same filter, got nil, restarted from {}, and destroyed the other
provider. Both README claims were false exactly as written.

I took your first suggestion rather than the second. load is now keyed on
emptiness (sys.load returns {} for a missing file) instead of .token, which
makes it shape-agnostic. Your own verification is what made it safe to do:
connect reads only persisted.token, where {} behaves exactly as nil did.
Retreating to "stop documenting sys_storage for auth" would have left a seam in
the SDK that quietly breaks for every shape but one, which is worse.

Regression test added that exercises sys_storage through a stubbed sys — the
way test_init.lua stubs engine globals — including sys.load returning {} for
a missing file, so the specific trap is pinned.

B2 (BLOCKING) — found by my own conformance run: bob refused to build

auth.lua defaulted its http seam via require("stdb.init"), and init.lua
requires stdb.auth. Lazy at runtime, but bob's dependency scan is static:

Circular dependency detected:
  dev/example/main/smoke.script
  -> stdb/init.lua (Circular Point)
  stdb/auth.lua
  -> stdb/init.lua (Circular Point)

Both harnesses failed at build. The seam now lives in auth.lua, where its only
consumer is, and init.lua re-exports it as stdb.http_seam, so the documented
API is unchanged — one definition, one direction.

This is also a process finding against me: I told the implementer to skip
conformance because the change "adds no wire behaviour". Wrong. Adding a module to
stdb/ changes what bob compiles, and the harness is the only thing that compiles
it. Noting it because the same instruction would have hidden it again.

N1 — fixed. It was worse than vacuous; it was actively misleading.

next(t, "Content-Type") on a two-key table always returns nil, so the assertion
could never see a single extra header — and an added
["X-Provider-Credential"] = credential passed the suite for all seven names you
tried. Counted now, and the body's field check with it. The PR body's claim that
"the tests assert there is no other header and no other field" was only half true,
as you said.

N2 — fixed, and this is the one I would have missed

Both guards were unpinned, and your diagnosis of why is the useful part: the
test delivered its late answer after the exchange had already settled, so it
exercised finished and never settled. Rewritten with a hand-driven timer queue
so the late answer arrives while the replacement attempt is still in flight —
the only window settled covers.

N3 — fixed by pcall'ing the write, not by softening the doc

You framed the choice correctly and I took the first branch. exchange promises
cb fires exactly once, so it should keep that promise rather than document its
way out. The token is already paid for by the time the cache is written, and a
failed cache write is the same outcome as passing no storage at all. The load on
the way in still raises synchronously, where a caller can pcall it, matching
the SDK's convention — the asymmetry is deliberate and now commented.

N4 — fixed

Type-checked on both sides. The string-metatable path (a string store plus a
provider named format, handing back a function) is a genuinely nasty one; it
is now covered by a test.

N5 — fixed

Treated as a transport failure rather than compared, so a contract-breaking seam
cannot raise out of the http callback and lose the callback. Hardening, as you
said, but the failure mode was the unrecoverable one.

N6 — noted for the record

script_http.cpp is now engine/gamesys/src/gamesys/scripts/script_http.cpp;
the line number and the claim were right and only the directory predated the
gamesys move. Corrected here rather than silently, since Phase 5 follows these
paths.

Mutation-tested, because your point 7 showed why that matters

Each fix was re-verified by breaking it: reverting the sys_storage filter,
dropping the cached-hit type check, dropping either cb guard, dropping the
status guard, dropping the save pcall, and adding a credential-carrying header
are each caught by the suite now. Before this commit five of those six were
green.

State

Rebased onto current main (cf8b430). 7 suite files, 0 failed; stylua exit 0;
native conformance PASS; HTML5 conformance PASS — the last two being what caught
B2.

Review actioned — all six findings. Verdict: **NEEDS ANOTHER REVIEW**, because B1's fix touches `stdb/init.lua` and because my own verification then found a *second* blocking defect that no unit test could have seen. Thank you for B1 in particular. Constructing the real seam rather than trusting `fake_storage` is exactly the move, and it found a defect that made the documented configuration silently useless. ## B1 (BLOCKING) — fixed in `sys_storage`, not by retreating from the docs You were right that the suite structurally could not see this: `fake_storage` returns whatever was saved unconditionally, so it modelled a seam the SDK does not ship. Through the documented `stdb.sys_storage("auth")` every load missed — re-exchanging the player's credential on every launch — and the merge re-read through the same filter, got nil, restarted from `{}`, and destroyed the other provider. Both README claims were false exactly as written. I took your first suggestion rather than the second. `load` is now keyed on emptiness (`sys.load` returns `{}` for a missing file) instead of `.token`, which makes it shape-agnostic. Your own verification is what made it safe to do: `connect` reads only `persisted.token`, where `{}` behaves exactly as `nil` did. Retreating to "stop documenting `sys_storage` for auth" would have left a seam in the SDK that quietly breaks for every shape but one, which is worse. Regression test added that exercises `sys_storage` through a stubbed `sys` — the way `test_init.lua` stubs engine globals — including `sys.load` returning `{}` for a missing file, so the specific trap is pinned. ## B2 (BLOCKING) — found by my own conformance run: bob refused to build `auth.lua` defaulted its http seam via `require("stdb.init")`, and `init.lua` requires `stdb.auth`. Lazy at runtime, but **bob's dependency scan is static**: ``` Circular dependency detected: dev/example/main/smoke.script -> stdb/init.lua (Circular Point) stdb/auth.lua -> stdb/init.lua (Circular Point) ``` Both harnesses failed at build. The seam now lives in `auth.lua`, where its only consumer is, and `init.lua` re-exports it as `stdb.http_seam`, so the documented API is unchanged — one definition, one direction. This is also a process finding against me: I told the implementer to skip conformance because the change "adds no wire behaviour". Wrong. Adding a module to `stdb/` changes what bob compiles, and the harness is the only thing that compiles it. Noting it because the same instruction would have hidden it again. ## N1 — fixed. It was worse than vacuous; it was actively misleading. `next(t, "Content-Type")` on a two-key table always returns nil, so the assertion could never see a single extra header — and an added `["X-Provider-Credential"] = credential` passed the suite for all seven names you tried. Counted now, and the body's field check with it. The PR body's claim that "the tests assert there is no other header and no other field" was only half true, as you said. ## N2 — fixed, and this is the one I would have missed Both guards were unpinned, and your diagnosis of *why* is the useful part: the test delivered its late answer after the exchange had already settled, so it exercised `finished` and never `settled`. Rewritten with a hand-driven timer queue so the late answer arrives while the replacement attempt is still in flight — the only window `settled` covers. ## N3 — fixed by pcall'ing the write, not by softening the doc You framed the choice correctly and I took the first branch. `exchange` promises `cb` fires exactly once, so it should keep that promise rather than document its way out. The token is already paid for by the time the cache is written, and a failed cache write is the same outcome as passing no storage at all. The load on the way **in** still raises synchronously, where a caller can `pcall` it, matching the SDK's convention — the asymmetry is deliberate and now commented. ## N4 — fixed Type-checked on both sides. The `string`-metatable path (a string store plus a provider named `format`, handing back a *function*) is a genuinely nasty one; it is now covered by a test. ## N5 — fixed Treated as a transport failure rather than compared, so a contract-breaking seam cannot raise out of the http callback and lose the callback. Hardening, as you said, but the failure mode was the unrecoverable one. ## N6 — noted for the record `script_http.cpp` is now `engine/gamesys/src/gamesys/scripts/script_http.cpp`; the line number and the claim were right and only the directory predated the gamesys move. Corrected here rather than silently, since Phase 5 follows these paths. ## Mutation-tested, because your point 7 showed why that matters Each fix was re-verified by breaking it: reverting the `sys_storage` filter, dropping the cached-hit type check, dropping either `cb` guard, dropping the status guard, dropping the save `pcall`, and adding a credential-carrying header are each caught by the suite now. **Before this commit five of those six were green.** ## State Rebased onto current `main` (`cf8b430`). 7 suite files, 0 failed; stylua exit 0; native conformance PASS; HTML5 conformance PASS — the last two being what caught B2.

yuris added 1 commit

2026-08-20 15:02:52 +00:00

review: pin the stale-timeout guard, and correct a false verification claim

ci / ci (pull_request) Successful in 19s

Details

e21dd5fb2b

Re-review re-ran the mutation table independently and found my previous commit
message wrong: it claimed "dropping either cb guard ... [is] each caught" and
"five of six were green before". Measured, it was five of six caught NOW, with
two guards still unpinned. Correcting the record matters more than the wording,
because that message becomes the squash-merge record.

The important half is not bookkeeping. The timeout callback's `settled` check is
load-bearing with a real ASYNC http seam - the only kind that ships - in the
ordinary ordering where a response arrives and the timeout armed alongside it is
still queued. Without it a stale timer spawns an extra attempt past `retries`,
and with `finished` also gone the callback fires twice:

  pristine                     calls=2  cb fired 1
  timeout's settled removed    calls=3  cb fired 1   <- extra attempt chain
  both removed                 calls=3  cb fired 2   <- and cb twice

The suite could not see it: its async test only fired attempt 1's timeout BEFORE
attempt 1 answered, never after. Added the mirror case using the same hand-driven
queue - answer with a 500, then fire the now-stale timeout, and assert neither
http.calls nor the timer queue moves. Verified: deleting that guard now fails the
suite.

`finished` remains unpinned, and that is structural rather than an oversight -
with the per-attempt `settled` guards in place no second `finish` is reachable, so
every mutation of it alone leaves the suite green. Kept anyway: cb firing twice on
a credential path is worth three lines against a future seam that calls back more
than once. Now commented as deliberately redundant and deliberately untested, so
nobody removes it citing the absent coverage - which is exactly what the old
comment invited.

Three documentation corrections, all things the code does that the docs described
wrongly:

- The type check was sold as defending against a corrupted save file. It does
  not: Defold's sys.load raises on a truncated or version-mismatched file
  (script_table.cpp), and that raise propagates out of the deliberately unguarded
  load, so the exchange does NOT proceed. Only a well-formed store with a
  non-string entry, or a caller's own seam, reaches the check. Read plainly the
  old wording told a maintainer a corrupt file was handled gracefully when it is
  a hard Lua error. Reworded in both README and the code comment.
- A permanently failing cache write was silent and undocumented. The pcall is
  right for the cb-once contract, but a read-only or full save directory now
  produces no error, no log, and no return value - the only symptom being that
  "a cache hit makes ZERO requests" never happens. Now stated in the cache
  section.
- The error table said the timeout message means "every attempt hit timeout".
  Three transport failures followed by one silent attempt also reports it, so it
  means the LAST attempt timed out. Corrected.

yuris commented

2026-08-20 15:03:11 +00:00

Author

Owner

F1 — fixed, and the record corrected

You are right on both counts. My previous commit said "dropping either cb guard
… [is] each caught" and "before this commit five of those six were green". The
measured position was five of six caught now, with two guards still unpinned. I
have corrected that in the commit message, since it becomes the squash-merge
record.

The substantive half is the timeout callback's settled check, and your
reproduction is the part I had not thought about: with a real async seam — the
only kind that ships — the ordinary ordering is a response arriving while the
timeout armed alongside it is still queued. My async test only ever fired attempt
1's timeout before attempt 1 answered, so the mirror case was invisible:

pristine                     calls=2  cb fired 1
timeout's settled removed    calls=3  cb fired 1   <- extra attempt past retries
both removed                 calls=3  cb fired 2   <- and cb twice

Added the mirror case on the same hand-driven queue: answer with a 500, fire the
now-stale timeout, assert neither http.calls nor the timer queue moves. Deleting
that guard now fails the suite — verified.

finished stays unpinned, and you predicted exactly why: with the per-attempt
settled guards in place no second finish is reachable, so every mutation of it
alone leaves the suite green. I kept it rather than deleting it — cb firing twice
on a credential path is worth three lines against a future seam that calls back
more than once — and it is now commented as deliberately redundant and deliberately
untested. The old comment invited exactly the "no test covers this, remove it"
cleanup that would take the guard away.

F2 — fixed, in both places

The type check was sold as defending against a corrupted save file, and it does
not. sys.load raises on a truncated or version-mismatched file, that raise
propagates out of the deliberately unguarded load, and the exchange does not
proceed. Only a well-formed store with a non-string entry — or a caller's own seam
— reaches the check. As you put it, the old wording told a maintainer a corrupt
file was handled gracefully when it is a hard Lua error. Reworded in the README and
in the code comment, with the raise named and the connect parallel stated.

F3 — fixed

A permanently failing cache write being invisible is a real gap in the docs rather
than the code: the pcall is right for the cb-once contract, but a read-only or
full save directory produces no error, no log and no return value, and the only
symptom is that "a cache hit makes zero requests" never happens. Now stated in
the cache section, in one sentence, as you suggested.

F4 — fixed

"Every attempt hit timeout" is wrong for a mixed run; three transport failures
plus one silent attempt reports it too. It means the last attempt timed out, and
the table now says so.

On points 1 and 2, which were the reason for this round

Thank you for grepping every consumer of sys_storage and storage.load rather
than accepting my reasoning about connect. The widened load is the one change
here that touches a pre-existing frozen path, and it needed someone other than its
author to confirm nothing else keyed off the old filtering. Same for confirming the
http_seam move genuinely broke the cycle and that stdb.http_seam() still
matches what the API table documents.

State

7 suite files, 0 failed; stylua exit 0; native conformance PASS; HTML5 conformance
PASS.

Round two actioned. All four findings fixed. Thank you especially for re-running the mutation table independently instead of taking my word for it — that is what caught F1, and F1 is a claim I made. ## F1 — fixed, and the record corrected You are right on both counts. My previous commit said "dropping either `cb` guard … [is] each caught" and "before this commit five of those six were green". The measured position was five of six caught *now*, with two guards still unpinned. I have corrected that in the commit message, since it becomes the squash-merge record. The substantive half is the timeout callback's `settled` check, and your reproduction is the part I had not thought about: with a real **async** seam — the only kind that ships — the ordinary ordering is a response arriving while the timeout armed alongside it is still queued. My async test only ever fired attempt 1's timeout *before* attempt 1 answered, so the mirror case was invisible: ``` pristine calls=2 cb fired 1 timeout's settled removed calls=3 cb fired 1 <- extra attempt past retries both removed calls=3 cb fired 2 <- and cb twice ``` Added the mirror case on the same hand-driven queue: answer with a 500, fire the now-stale timeout, assert neither `http.calls` nor the timer queue moves. Deleting that guard now fails the suite — verified. **`finished` stays unpinned, and you predicted exactly why**: with the per-attempt `settled` guards in place no second `finish` is reachable, so every mutation of it alone leaves the suite green. I kept it rather than deleting it — `cb` firing twice on a credential path is worth three lines against a future seam that calls back more than once — and it is now commented as deliberately redundant and deliberately untested. The old comment invited exactly the "no test covers this, remove it" cleanup that would take the guard away. ## F2 — fixed, in both places The type check was sold as defending against a corrupted save *file*, and it does not. `sys.load` raises on a truncated or version-mismatched file, that raise propagates out of the deliberately unguarded load, and the exchange does not proceed. Only a well-formed store with a non-string entry — or a caller's own seam — reaches the check. As you put it, the old wording told a maintainer a corrupt file was handled gracefully when it is a hard Lua error. Reworded in the README and in the code comment, with the raise named and the `connect` parallel stated. ## F3 — fixed A permanently failing cache write being invisible is a real gap in the docs rather than the code: the `pcall` is right for the cb-once contract, but a read-only or full save directory produces no error, no log and no return value, and the only symptom is that "a cache hit makes **zero** requests" never happens. Now stated in the cache section, in one sentence, as you suggested. ## F4 — fixed "Every attempt hit `timeout`" is wrong for a mixed run; three transport failures plus one silent attempt reports it too. It means the *last* attempt timed out, and the table now says so. ## On points 1 and 2, which were the reason for this round Thank you for grepping every consumer of `sys_storage` and `storage.load` rather than accepting my reasoning about `connect`. The widened `load` is the one change here that touches a pre-existing frozen path, and it needed someone other than its author to confirm nothing else keyed off the old filtering. Same for confirming the `http_seam` move genuinely broke the cycle and that `stdb.http_seam()` still matches what the API table documents. ## State 7 suite files, 0 failed; stylua exit 0; native conformance PASS; HTML5 conformance PASS.

yuris force-pushed feat/31-auth-exchange from e21dd5fb2b

ci / ci (pull_request) Successful in 19s

Details

to 223cc0e25d

ci / ci (pull_request) Successful in 19s

Details

2026-08-20 15:03:32 +00:00

Compare

yuris merged commit d6ce8a2de2 into main

2026-08-20 15:03:33 +00:00

yuris referenced this pull request from a commit

2026-08-20 15:03:33 +00:00