Skip to content

fix(storage): create the relay and approval tables with the schema, never cache a failed create - #1177

Merged
AbirAbbas merged 2 commits into
mainfrom
fix/relay-table-race
Nov 10, 2026
Merged

AbirAbbas merged 2 commits into
mainfrom
fix/relay-table-race

Conversation

@AbirAbbas

@AbirAbbas AbirAbbas commented Nov 10, 2026 •

Copy link
Copy Markdown
Contributor

What broke

#1174 added three tables that the control-plane store created lazily, on first use, with CREATE TABLE IF NOT EXISTS: execution_relay, run_owners and execution_approval_questions. Against a real postgres:16 control plane, the existing-deployments smoke suite logged:

ERROR:  duplicate key value violates unique constraint "pg_type_typname_nsp_index"
create execution_relay: ERROR: duplicate key value ... (SQLSTATE 23505)   "failed to record the run's owner"

Three problems compounded:

  1. A race. Two requests reaching first use at the same moment both ran CREATE TABLE IF NOT EXISTS, and on Postgres the loser fails on the catalog's unique index.
  2. The error was cached forever. relayTableOnce and approvalQuestionTableOnce stored the error per db handle. After one lost race, relay writes, run ownership and approval questions failed until the control plane restarted, so on a Postgres control plane nothing streamed to the run's owner.
  3. DDL on every run. runOwnerTable ran CREATE TABLE IF NOT EXISTS run_owners on every call, and the run-watched lookup consults it for ordinary executions.

The fix

  • The three tables are now created at startup, with the rest of the schema, in autoMigrateSchema, for SQLite and Postgres. The DDL is unchanged and each table has a single definition, so databases that already have the tables are untouched.
  • A guard remains for stores built without migration. It serializes creation per db, records only success, and runs no DDL in steady state. On Postgres it treats SQLSTATE 23505 or 42P07 as success when the table exists afterwards, because a peer created it.
  • Relay, ownership and approval semantics are unchanged. No columns were added.

Tests

  • R1: a migrated store has all three tables before any call.
  • R2: a failed create is not cached, and the next call succeeds. This includes the Postgres 23505 and 42P07 paths, through a driver seam.
  • R3: steady state issues no CREATE TABLE.
  • R4: 32 concurrent first uses pass under -race, with exactly one owner row per run.
  • S9: the lazy-creation tests now use a store that skips migration. They asserted that the tables do not exist after migration, which is the opposite of this contract. Their assertions are otherwise kept.
  • Existing tests: relay, approval, run-watched and handler tests all pass.

Proof against real deployments

The existing-deployments smoke suite ran on a candidate built from main plus this fix, run 42759516796, with 15 of 15 jobs green:

  • Postgres and SQLite: an ordinary run sends no watched header and leaves zero relay and owner rows. 8 concurrent owned executions, the first owned work a fresh control plane sees, are all watched with exactly one owner row each. A positive control with a nested call records its relay line.
  • Without this fix: the same Postgres check failed with pg_type_typname_nsp_index, and ownership was lost.
  • Old SDKs against the new control plane: Python 0.1.130 and 0.1.145, and Go v0.1.130, all pass.
  • New SDKs against control planes nobody upgraded: v0.1.145 and v0.1.57 both pass.
  • Migrations and rollback: migrations from stable v0.1.145 apply cleanly on SQLite and Postgres, and the v0.1.145 binary still starts and serves on the migrated store.

The coverage commit after that run adds tests only. The patch-coverage gate now passes.

🤖 Generated with Claude Code

Create execution_relay, run_owners, and execution_approval_questions at
startup in LocalStorage.autoMigrateSchema for both SQLite and PostgreSQL
using single DDL definitions shared with the lazy fallback guards.

Guard fallback creation per *sqlDatabase with a mutex and a positive-only
table set so concurrent goroutines in one process never race CREATE TABLE,
runOwnerTable issues no DDL in steady state, and a failed CREATE TABLE is
never cached. On PostgreSQL, treat SQLSTATE 23505 and 42P07 as success
when the table exists after a concurrent peer finished creating it.

Found by the existing-deployments smoke suite against postgres:16:
"create execution_relay: duplicate key value violates unique constraint
pg_type_typname_nsp_index", after which the cached error left run
ownership broken until restart.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
AbirAbbas added a commit that referenced this pull request Nov 10, 2026
The candidate control plane in this suite is built from this branch, so
merging the fix here runs Checks A-D against main plus #1177.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Nov 10, 2026 •

Copy link
Copy Markdown
Contributor

📊 Coverage gate

Thresholds from .coverage-gate.toml: per-surface ≥ 84%, aggregate ≥ 85%, max per-surface regression ≤ 1.0 pp, max aggregate regression ≤ 0.50 pp.

Surface Current Baseline Δ
control-plane 88.20% 87.40% ↑ +0.80 pp 🟡
sdk-go 93.30% 92.00% ↑ +1.30 pp 🟢
sdk-python 94.77% 93.73% ↑ +1.04 pp 🟢
sdk-typescript 92.44% 90.42% ↑ +2.02 pp 🟢
web-ui 84.90% 84.79% ↑ +0.11 pp 🟡
aggregate 86.12% 85.75% ↑ +0.37 pp 🟡

✅ Gate passed

No surface regressed past the allowed threshold and the aggregate stayed above the floor.

@github-actions

github-actions Bot commented Nov 10, 2026 •

Copy link
Copy Markdown
Contributor

📐 Patch coverage gate

Threshold: 80% on lines this PR touches vs origin/main (from .coverage-gate.toml:thresholds.min_patch).

Surface Touched lines Patch coverage Status
control-plane 62 100.00% ✅
sdk-go 0 — ➖ no changes
sdk-python 0 — ➖ no changes
sdk-typescript 0 — ➖ no changes
web-ui 0 — ➖ no changes

✅ Patch gate passed

Every surface whose lines were touched by this PR has patch coverage at or above the threshold.

AbirAbbas added a commit that referenced this pull request Nov 10, 2026
Check C hung until its job timed out: the owned burst's bare `wait` also
waited on the test nodes the shell runs in the background. Wait on the
burst's own PIDs.

On control plane v0.1.57 every node, stable SDKs included, registered and
ran its sync and async executes, then failed the heartbeat-survival step
for not being "active". That step now uses the same SMOKE_ACCEPT_HEALTH as
registration, with the advancing heartbeat as the survival proof.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The patch-coverage gate measured 74% on #1177's touched control-plane lines.
Drive each uncovered branch through real behaviour: an uninitialized store,
a seam that fails only the second or third CREATE so the run_owners or
approval-question error surfaces, autoMigrateSchema wrapping that error,
nil and non-SQLState errors, and the race probe when the table is still
absent.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@AbirAbbas
AbirAbbas marked this pull request as ready for review November 10, 2026 05:24
@AbirAbbas
AbirAbbas requested a review from a team as a code owner November 10, 2026 05:24
@AbirAbbas
AbirAbbas merged commit 186f695 into main Nov 10, 2026
30 checks passed
@AbirAbbas
AbirAbbas deleted the fix/relay-table-race branch November 10, 2026 05:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant