yuris/spacetimedb-defold

Fork 0

dev: isolate dev-server state per port and verify the pid holds it (#54) #60

Merged

yuris merged 3 commits from feat/54-dev-server-state into main

2026-08-20 14:12:29 +00:00

yuris commented

2026-08-20 13:30:48 +00:00

Owner

Note on this description. It was rewritten at head to describe the merged
mechanism. The first revision used ss -ltnp | grep -q and asserted that ss
was already used in this script; review showed the check was broken under
pipefail and the premise was false. Both are covered under Round one below,
since the reasoning is the useful part.

Two independent holes, both closed

The issue proposes either fix; only the first addresses data collision, which is
what actually bit people. Both are kept because they close different cases.

1. The default state dir is now keyed on $PORT. STATE was a single fixed
/tmp/spacetimedb-defold, and PIDFILE, DATA, KEYS, LOGFILE and HOME all
derive from it, so two ports shared all five. An explicit STDB_STATE_DIR still
overrides, unchanged.

2. server_pid() now verifies the recorded pid is our server for this port,
by reading its own argv:

if [ -r "/proc/$pid/cmdline" ]; then
  local cmd
  cmd=$(tr '\0' ' ' <"/proc/$pid/cmdline")
  [[ $cmd == *"--listen-addr 127.0.0.1:$PORT "* ]] || return 1
fi

It previously did only kill -0, which passes for a recycled pid. The trailing
space is load-bearing: it stops port 1234 matching a server on 12345. Guarded on
/proc being readable, so a host without it (macOS) degrades to the old
behaviour rather than failing every check — verified by masking /proc in an
unprivileged mount namespace.

Reading argv rather than asking who holds the port is deliberate, and it is what
round two of review confirmed:

No pipeline, so set -o pipefail cannot reach it — structurally, not by
careful quoting. See round one.
No new dependency. ss is iproute2, is not supplied by the flake, and is
absent from some minimal CI images.
True during the window before the process has bound, which keeps stop
able to kill a server that is still starting. A port check answers "is someone
serving?"; this answers "is this pid mine?", and the latter is what both
callers actually ask.

Both failures reproduced on `main` first, not inferred

Collision, with 6901 already up:

$ STDB_PORT=6902 dev/tools/dev-server.sh
[dev-server] server already running at http://127.0.0.1:6902 (pid 3809830)
curl: (7) Failed to connect to 127.0.0.1:6902 after 0 ms

3809830 is the 6901 server. Recycled pid, on main, pidfile holding a live pid
that holds no port:

[dev-server] server already running at http://127.0.0.1:6906 (pid 3812266)

Both are gone on this branch: two servers run concurrently on two ports each
minting its own identity, restart 6901 then stop 6902 leaves 6901 at 200 and
6902 refused, and the planted-pidfile case starts a real server instead of
falsely reporting one.

Round one: the `ss` check was broken, and the premise for it was false

The first revision used ss -ltnp | grep -q "127.0.0.1:$PORT[^0-9].*pid=$pid,".
Review found it blocking: grep -q exits on first match, ss then dies of
SIGPIPE, and pipefail reports 141 for a pipeline that matched — so
server_pid() calls a healthy server absent. It needs >64KiB of ss output
(~600 listening sockets) to surface, so it would have appeared only on a busy
host. Reproduced end to end with a same-format ss stand-in: start falsely
refused with "port held by another process", and stop printed "no server
running" while leaking a live server. This script already documents the
identical trap at the publish call.

Review also disproved the justification I had given: I stated ss -ltnp was
"already read for the stray-server message". It was not — main reads no sockets
anywhere, so that line would have introduced the repo's first iproute2
dependency. The claim came from the milestone plan and I repeated it without
checking, which is exactly what the re-capture instruction exists to catch.

Rather than patch the pipeline, the mechanism was replaced, which dissolved three
of the five findings at once (the pipefail defect, a shellcheck SC1087 on
$PORT[^0-9], and the dependency).

Verified at head

Round two attacked the new check specifically: healthy server identified with the
right pid across start/restart/stop; stale live pid and live-server-on-another-port
both rejected; the trailing-space anchor works in both directions and survives
--listen-addr being the final argv element (the kernel's terminating NUL is
translated too); the glob RHS is double-quoted so $PORT matches literally and
every metacharacter fails closed; /proc masked → check inert, degrading to
main's behaviour; no pipeline anywhere in server_pid; shellcheck exits 0
where the previous revision was the repo's only shellcheck error.

lua dev/test/runner.lua            6 passed, 0 failed
stylua --check stdb dev/test       exit 0, silent
shellcheck dev/tools/dev-server.sh exit 0
conformance.sh                     PASS, PHASE1-5, identity persisted
html5-conformance.sh               PASS, identity survived a reload

Also here

README.md's dev-server section documented the old fixed
/tmp/spacetimedb-defold, which this change invalidates. Corrected, and it now
names the default the reader actually gets: STDB_PORT defaults to 3000, so the
directory is /tmp/spacetimedb-defold-3000. The README ships in the consumer
archive, so it is cheapest to fix in the PR that caused it.

Out of scope, filed as #63

With two ports pointed at one explicit STDB_STATE_DIR, the second start
overwrites the pidfile with its own dead child, after which stop cannot find the
first server and leaks it. Reproduced. It requires deliberately sharing one state
dir, which is the configuration this PR's default now stops anyone reaching by
accident. Review also showed my first rationale for deferring it was wrong — the
pid is still discoverable in /proc, so a fallback scan is a cheaper fix than
"the fix has to be earlier" implied; #63 has been corrected accordingly.

Closes #54. First item of the v1.1.0 milestone (#59), landed alone and before any parallel work: every later phase spawns concurrent subagents in separate worktrees, and this bug bit three of them during the v1.0.0 push. > **Note on this description.** It was rewritten at head to describe the merged > mechanism. The first revision used `ss -ltnp | grep -q` and asserted that `ss` > was already used in this script; review showed the check was broken under > `pipefail` and the premise was false. Both are covered under *Round one* below, > since the reasoning is the useful part. ## Two independent holes, both closed The issue proposes either fix; only the first addresses data collision, which is what actually bit people. Both are kept because they close different cases. **1. The default state dir is now keyed on `$PORT`.** `STATE` was a single fixed `/tmp/spacetimedb-defold`, and `PIDFILE`, `DATA`, `KEYS`, `LOGFILE` and `HOME` all derive from it, so two ports shared all five. An explicit `STDB_STATE_DIR` still overrides, unchanged. **2. `server_pid()` now verifies the recorded pid is our server for this port**, by reading its own argv: ```sh if [ -r "/proc/$pid/cmdline" ]; then local cmd cmd=$(tr '\0' ' ' <"/proc/$pid/cmdline") [[ $cmd == *"--listen-addr 127.0.0.1:$PORT "* ]] || return 1 fi ``` It previously did only `kill -0`, which passes for a recycled pid. The trailing space is load-bearing: it stops port 1234 matching a server on 12345. Guarded on `/proc` being readable, so a host without it (macOS) degrades to the old behaviour rather than failing every check — verified by masking `/proc` in an unprivileged mount namespace. Reading argv rather than asking who holds the port is deliberate, and it is what round two of review confirmed: - **No pipeline**, so `set -o pipefail` cannot reach it — structurally, not by careful quoting. See round one. - **No new dependency.** `ss` is iproute2, is not supplied by the flake, and is absent from some minimal CI images. - **True during the window before the process has bound**, which keeps `stop` able to kill a server that is still starting. A port check answers "is someone serving?"; this answers "is this pid mine?", and the latter is what both callers actually ask. ## Both failures reproduced on `main` first, not inferred Collision, with 6901 already up: ``` $ STDB_PORT=6902 dev/tools/dev-server.sh [dev-server] server already running at http://127.0.0.1:6902 (pid 3809830) curl: (7) Failed to connect to 127.0.0.1:6902 after 0 ms ``` `3809830` is the 6901 server. Recycled pid, on `main`, pidfile holding a live pid that holds no port: ``` [dev-server] server already running at http://127.0.0.1:6906 (pid 3812266) ``` Both are gone on this branch: two servers run concurrently on two ports each minting its own identity, `restart 6901` then `stop 6902` leaves 6901 at 200 and 6902 refused, and the planted-pidfile case starts a real server instead of falsely reporting one. ## Round one: the `ss` check was broken, and the premise for it was false The first revision used `ss -ltnp | grep -q "127.0.0.1:$PORT[^0-9].*pid=$pid,"`. Review found it **blocking**: `grep -q` exits on first match, `ss` then dies of SIGPIPE, and `pipefail` reports 141 for a pipeline that *matched* — so `server_pid()` calls a healthy server absent. It needs >64KiB of `ss` output (~600 listening sockets) to surface, so it would have appeared only on a busy host. Reproduced end to end with a same-format `ss` stand-in: `start` falsely refused with "port held by another process", and `stop` printed "no server running" while leaking a live server. **This script already documents the identical trap** at the publish call. Review also disproved the justification I had given: I stated `ss -ltnp` was "already read for the stray-server message". It was not — `main` reads no sockets anywhere, so that line would have introduced the repo's first iproute2 dependency. The claim came from the milestone plan and I repeated it without checking, which is exactly what the re-capture instruction exists to catch. Rather than patch the pipeline, the mechanism was replaced, which dissolved three of the five findings at once (the pipefail defect, a `shellcheck` SC1087 on `$PORT[^0-9]`, and the dependency). ## Verified at head Round two attacked the new check specifically: healthy server identified with the right pid across start/restart/stop; stale live pid and live-server-on-another-port both rejected; the trailing-space anchor works in both directions and survives `--listen-addr` being the final argv element (the kernel's terminating NUL is translated too); the glob RHS is double-quoted so `$PORT` matches literally and every metacharacter fails closed; `/proc` masked → check inert, degrading to `main`'s behaviour; no pipeline anywhere in `server_pid`; `shellcheck` exits 0 where the previous revision was the repo's only shellcheck error. ``` lua dev/test/runner.lua 6 passed, 0 failed stylua --check stdb dev/test exit 0, silent shellcheck dev/tools/dev-server.sh exit 0 conformance.sh PASS, PHASE1-5, identity persisted html5-conformance.sh PASS, identity survived a reload ``` ## Also here `README.md`'s dev-server section documented the old fixed `/tmp/spacetimedb-defold`, which this change invalidates. Corrected, and it now names the default the reader actually gets: `STDB_PORT` defaults to 3000, so the directory is `/tmp/spacetimedb-defold-3000`. The README ships in the consumer archive, so it is cheapest to fix in the PR that caused it. ## Out of scope, filed as #63 With two ports pointed at **one explicit** `STDB_STATE_DIR`, the second start overwrites the pidfile with its own dead child, after which `stop` cannot find the first server and leaks it. Reproduced. It requires deliberately sharing one state dir, which is the configuration this PR's default now stops anyone reaching by accident. Review also showed my first rationale for deferring it was wrong — the pid is still discoverable in `/proc`, so a fallback scan is a cheaper fix than "the fix has to be earlier" implied; #63 has been corrected accordingly.

yuris added 1 commit

2026-08-20 13:30:48 +00:00

dev: isolate dev-server state per port and verify the pid holds it (#54 )

ci / ci (pull_request) Successful in 1m29s

Details

d9f1f75619

Two independent holes let one dev-server invocation report another's server.

The default state dir was a single fixed path, so a second invocation on a
different port read the FIRST server's pidfile, announced "already running",
and then failed to connect to a port nothing was listening on. Reproduced on
main: with 6901 up, `STDB_PORT=6902 dev-server.sh` printed
`already running at http://127.0.0.1:6902 (pid 3809830)` - 6901's pid - then
`curl: (7) Failed to connect to 127.0.0.1:6902`. Deriving the default from
$PORT makes PIDFILE, DATA, KEYS, LOGFILE and HOME structurally unable to
collide, since all five derive from STATE. An explicit STDB_STATE_DIR still
overrides.

Separately, server_pid() only did `kill -0`, which passes for a RECYCLED pid.
Also confirmed on main: a pidfile holding a live pid that holds no port made
the script report `already running (pid 3812266)` and start nothing. It now
additionally requires that pid to hold the port. Guarded on `command -v ss`
deliberately - without the guard a host lacking `ss` would fail every check
and break the script outright; missing `ss` now degrades to the old behaviour.
`ss -ltnp` is already read for the stray-server message, so it is not a new
dependency.

This lands first in the v1.1.0 milestone because every later phase runs
parallel subagents in separate worktrees, and this bug bit three of them
during the v1.0.0 push.

yuris added 1 commit

2026-08-20 13:51:44 +00:00

review: identify our server by argv, not by who holds the port

ci / ci (pull_request) Successful in 20s

Details

be71fa0807

Review found the `ss -ltnp | grep -q` check BLOCKING, and it was right: under
`set -euo pipefail`, `grep -q` exits on first match, `ss` then dies of SIGPIPE,
and pipefail reports 141 for a pipeline that MATCHED - so server_pid() calls a
healthy server absent. Needs >64KiB of `ss` output (~600 listening sockets), so
it would have surfaced only on a busy host. This file already documents the
identical trap at the publish call.

Replaced with a read of /proc/$pid/cmdline rather than repaired, because that
dissolves three of the five findings at once:

- No pipeline, so pipefail cannot be tripped at all - structurally, not by
  careful quoting.
- No new dependency. Review also disproved the premise I had asserted in the PR
  body: `ss` was NOT already used here. main reads no sockets anywhere, so that
  line would have introduced the repo's first iproute2 dependency, host-supplied
  rather than flake-pinned, and absent from some minimal CI images.
- It is true during the window before the process has bound the port, which
  keeps `stop` able to kill a server that is still starting. A port check
  answers "is someone serving?"; both callers actually ask "is this pid mine?".

The trailing space in `--listen-addr 127.0.0.1:$PORT ` stops port 6960 matching
a server on 69601. Guarded on /proc being readable so a host without it (macOS)
degrades to the old behaviour, same shape as the `command -v ss` guard it
replaces. shellcheck now exits 0, which also clears the SC1087 the braced-$PORT
finding raised.

Also corrects README's dev-server section, which still documented the old fixed
/tmp/spacetimedb-defold path this change invalidates.

Not fixed here, filed separately instead: with two ports pointed at ONE explicit
STDB_STATE_DIR, the second start overwrites the pidfile with its own dead child,
after which `stop` cannot find the first server and leaks it. Reproduced, and
unreachable by server_pid - the record itself is gone. It needs an explicit
shared state dir, which is the configuration this PR's default now eliminates.

yuris referenced this pull request

2026-08-20 13:52:23 +00:00

dev-server: two ports sharing one explicit STDB_STATE_DIR leak a server #63

yuris commented

2026-08-20 13:52:56 +00:00

Author

Owner

Review actioned. Verdict: NEEDS ANOTHER REVIEW — the mechanism changed, so
the new one deserves a fresh look rather than my own sign-off.

Thank you for finding the pipefail defect. It was correct, it was blocking, and
it would have surfaced only on a busy host.

Finding 1 (BLOCKING, pipefail SIGPIPE) — fixed, but not with the suggested patch

The diagnosis is right: grep -q exits on first match, ss dies of SIGPIPE,
and pipefail reports 141 for a pipeline that matched, so server_pid()
calls a healthy server absent. Doubly embarrassing given this file already
documents the identical trap at the publish call.

The here-string patch would have worked. I dropped ss entirely instead,
reading /proc/$pid/cmdline, because that dissolves findings 1, 4 and 5 at once
rather than fixing them one at a time:

if [ -r "/proc/$pid/cmdline" ]; then
  local cmd
  cmd=$(tr '\0' ' ' <"/proc/$pid/cmdline")
  [[ $cmd == *"--listen-addr 127.0.0.1:$PORT "* ]] || return 1
fi

No pipeline at all, so pipefail cannot be tripped structurally rather than
by careful quoting. Verified: no executable line in server_pid contains a
pipe, only ||.
No dependency — which matters because of your finding 5.
True before the process has bound the port, which is your finding 3's
concern about the stop path.
shellcheck exits 0, clearing finding 4's SC1087.

The trailing space in --listen-addr 127.0.0.1:$PORT is load-bearing: it stops
port 6960 matching a server on 69601. Observed argv confirms the format:
spacetimedb-standalone start --listen-addr 127.0.0.1:6960 --data-dir .... The
/proc guard degrades on macOS exactly as command -v ss did.

Re-verified after the change: recycled-pid case still rejected (starts a real
server instead of falsely reporting one); two ports concurrently still isolated,
restart 6901 then stop 6902 leaves 6901 at 200 and 6902 refused; unit suite
6 passed 0 failed; stylua exit 0; native and HTML5 conformance both PASS.

Finding 5 (the false premise) — you were right, and it was mine

I asserted in the PR body that ss -ltnp was "already read at :80-82 for the
stray-server message". It is not. main reads no sockets anywhere. That claim
came from the milestone plan and I repeated it without checking, which is exactly
what the re-capture instruction exists to catch. Moot now that ss is gone, but
worth recording that the premise was wrong rather than merely superseded.

Finding 2 (README path) — fixed

README.md:1505 still documented the old fixed /tmp/spacetimedb-defold, which
this change made false. Corrected, and it now states why the path is keyed on
the port. Good catch that the README ships in the consumer archive.

Finding 3 (stop leak) — partly fixed here, remainder filed as #63

The /proc check fixes the half you flagged as the design concern: stop can
now identify our own server during the window before it binds, which no port
check could.

The scenario you reproduced is not fixed, and I confirmed that honestly rather
than claiming it. With two ports pointed at one explicit STDB_STATE_DIR:
6913's pidfile held 3848953; 6919 failed the database lock and overwrote the
pidfile with its own dead child 3849062; stop on 6913 then said no server running while 6913 still answered 200. server_pid() cannot fix it — by that
point the pid it needs is gone from the pidfile. The fix has to be earlier
(refuse to start when the pidfile names a live server for another port, or key
the pidfile by port within the state dir).

Filed as #63 rather than expanded into this PR: it requires deliberately sharing
one state dir across two ports, which is the configuration this PR's default now
stops anyone reaching by accident.

Finding 4 (SC1087) — cleared

Folded in; shellcheck is now silent on this file, where the previous revision
was the repo's only shellcheck error.

Review actioned. Verdict: **NEEDS ANOTHER REVIEW** — the mechanism changed, so the new one deserves a fresh look rather than my own sign-off. Thank you for finding the pipefail defect. It was correct, it was blocking, and it would have surfaced only on a busy host. ## Finding 1 (BLOCKING, pipefail SIGPIPE) — fixed, but not with the suggested patch The diagnosis is right: `grep -q` exits on first match, `ss` dies of SIGPIPE, and `pipefail` reports 141 for a pipeline that *matched*, so `server_pid()` calls a healthy server absent. Doubly embarrassing given this file already documents the identical trap at the publish call. The here-string patch would have worked. I dropped `ss` entirely instead, reading `/proc/$pid/cmdline`, because that dissolves findings 1, 4 and 5 at once rather than fixing them one at a time: ```sh if [ -r "/proc/$pid/cmdline" ]; then local cmd cmd=$(tr '\0' ' ' <"/proc/$pid/cmdline") [[ $cmd == *"--listen-addr 127.0.0.1:$PORT "* ]] || return 1 fi ``` - **No pipeline at all**, so pipefail cannot be tripped structurally rather than by careful quoting. Verified: no executable line in `server_pid` contains a pipe, only `||`. - **No dependency** — which matters because of your finding 5. - **True before the process has bound the port**, which is your finding 3's concern about the `stop` path. - `shellcheck` exits 0, clearing finding 4's SC1087. The trailing space in `--listen-addr 127.0.0.1:$PORT ` is load-bearing: it stops port 6960 matching a server on 69601. Observed argv confirms the format: `spacetimedb-standalone start --listen-addr 127.0.0.1:6960 --data-dir ...`. The `/proc` guard degrades on macOS exactly as `command -v ss` did. Re-verified after the change: recycled-pid case still rejected (starts a real server instead of falsely reporting one); two ports concurrently still isolated, `restart 6901` then `stop 6902` leaves 6901 at 200 and 6902 refused; unit suite 6 passed 0 failed; stylua exit 0; native and HTML5 conformance both PASS. ## Finding 5 (the false premise) — you were right, and it was mine I asserted in the PR body that `ss -ltnp` was "already read at :80-82 for the stray-server message". It is not. `main` reads no sockets anywhere. That claim came from the milestone plan and I repeated it without checking, which is exactly what the re-capture instruction exists to catch. Moot now that `ss` is gone, but worth recording that the premise was wrong rather than merely superseded. ## Finding 2 (README path) — fixed `README.md:1505` still documented the old fixed `/tmp/spacetimedb-defold`, which this change made false. Corrected, and it now states *why* the path is keyed on the port. Good catch that the README ships in the consumer archive. ## Finding 3 (stop leak) — partly fixed here, remainder filed as #63 The `/proc` check fixes the half you flagged as the design concern: `stop` can now identify our own server during the window before it binds, which no port check could. The scenario you reproduced is *not* fixed, and I confirmed that honestly rather than claiming it. With two ports pointed at one explicit `STDB_STATE_DIR`: 6913's pidfile held 3848953; 6919 failed the database lock and overwrote the pidfile with its own dead child 3849062; `stop` on 6913 then said `no server running` while 6913 still answered 200. `server_pid()` cannot fix it — by that point the pid it needs is gone from the pidfile. The fix has to be earlier (refuse to start when the pidfile names a live server for another port, or key the pidfile by port within the state dir). Filed as #63 rather than expanded into this PR: it requires deliberately sharing one state dir across two ports, which is the configuration this PR's default now stops anyone reaching by accident. ## Finding 4 (SC1087) — cleared Folded in; `shellcheck` is now silent on this file, where the previous revision was the repo's only shellcheck error.

yuris added 1 commit

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

review: realizable port pair in the comment, and name the default state dir

ci / ci (pull_request) Successful in 1m33s

Details

d9ff2106db

Two documentation corrections from the re-review, neither behavioural.

The trailing-space example used 6960 against 69601, but 69601 is above 65535 so
nothing can listen there and that collision is unreachable. The anchor is
genuinely load-bearing - re-review proved it against a crafted argv on
--listen-addr 127.0.0.1:69121 - but the illustration needs two ports that can
both exist. Now 1234 against 12345.

README said state lives under /tmp/spacetimedb-defold-$STDB_PORT, but STDB_PORT
is unset for anyone following the README, so the sentence rendered as
/tmp/spacetimedb-defold- while the real directory is /tmp/spacetimedb-defold-3000.
The 3000 default appears nowhere else in the README as the dev-server's port. Now
stated.

yuris referenced this pull request

2026-08-20 14:12:13 +00:00

dev-server: two ports sharing one explicit STDB_STATE_DIR leak a server #63

yuris merged commit 2cad726e5c into main

2026-08-20 14:12:29 +00:00

yuris referenced this pull request from a commit

2026-08-20 14:12:29 +00:00

dev: isolate dev-server state per port and verify the pid holds it (#54) (#60)

yuris referenced this pull request

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

v1.1.0 release checklist #59

yuris referenced this pull request