Confine DB test harnesses; answer POST /api/queues honestly - #6
Merged
Merged
Conversation
A rolling node maintenance restarted NATS and left three Bento consumers (latest-kv, alarms, entity-greptimedb) holding dead subscriptions. All three reported Running 1/1 for hours; the halt was found only when a human ran `nats consumer info` by hand. Neither /ping nor /ready detects a subscription that is dead underneath a live connection, and greptimedb-freshness-guard stays green through it because a different consumer keeps writing rows to GreptimeDB. Adds nats-consumer-guard (CronJob + scripts ConfigMap) covering every JetStream durable, including the alarm-materializer one, which is created by the Go client and whose Deployment has no probes at all. Backlog depth is deliberately NOT the signal. latest-kv runs max_ack_pending=1 as a single-writer serialization mechanism, so under load it holds a large and often growing backlog while working perfectly — measured at a sustained 6,729 while its ack floor advanced 61,179 -> 100,526. A depth threshold alerts there, every tick. The guard checks push_bound (is anything subscribed to the deliver subject) and whether ack_floor.consumer_seq MOVES across two samples, and requires the fault in both samples so a rolling upgrade does not trip it. Verified against a real NATS 2.10.26 across all four branches, then in-cluster: a four-minute NATS outage left all five consumers unbound with every pod still Running 1/1 and its restart count unchanged, the guard failed the Job naming each one, and `rollout restart` returned the next tick to green. A single clean pod restart does NOT reproduce it — every consumer reconnects — which is why the failure looked intermittent. Also closes two landmines the guard would have surfaced: - the questdb durable was created when enabled but never removed when disabled, leaving an unbound consumer on a capped stream (it once reached 106k pending and never drained). Removed at the source rather than ignored. - every shipped overlay set latestKv.replicas: 2 against the documented single-writer invariant, including the example templates new deployments copy from. Pinned to 1 and covered by a test over all tracked values files. Documents the --reuse-values trap: it reuses the previous release's COMPUTED values, so new chart defaults are silently shadowed — which nearly shipped maxAckPending 1024 against the required 1. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three defects, all from the ADR-0002 / twin-state Phase 3 work, found by
reading the failing runs rather than assuming they were pre-existing noise.
Test flow-core — POST /api/entityViews returned 500
entity_id is a uuid column and ExtractEntityID yields "" for an absent field,
so a body without entityId handed the driver an empty string and failed with
`invalid input syntax for type uuid` — a malformed request answered as a server
fault. Now goes through dbutil.NullUUID, the same way customerId beside it
always has. Covered both ways: the realistic body round-trips its entityId, and
the omitted-entityId body stores NULL instead of 500-ing.
Test flow-core — nil-pointer panic in the async audit writer
The writer is a goroutine draining a queue, so an event can reach doWrite after
the pool it needs is gone: during shutdown, or in tests where the harness resets
the pool at cleanup while a write is still in flight. Killing the process to
avoid losing one audit row is the wrong trade — the stores are the source of
truth and this journal is secondary. Drops and counts the event instead.
Fresh-install smoke — latest-kv never started
When latest-kv became the single-writer doc-merge it gained
`max_ack_pending: ${LATEST_KV_MAX_ACK_PENDING}`. The Helm template was updated;
docker-compose was not. Bento treats a ${VAR} with no default as REQUIRED and
refuses to start on it, so the container never came up, the twin_state KV was
never populated, and the smoke failed three steps downstream on "NATS KV
twin_state did not hold both keys within 60s" with nothing pointing at the
cause. The compose bootstrap also created every durable with a shared
--max-pending 1024, including latest-kv's, which must be 1 — Bento asserts its
own value on bind, so that alone would have failed the attach.
Adds test_bento_env_contract.py: the chart and compose mount the SAME config
files, so every required var has to be satisfied twice. That contract is now
explicit instead of depending on whoever remembers both call sites.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… fan-out The latest-kv consumer became a doc-merge writing the whole device state under DEVICE.<tenant>.<device>; the DEVICE.<tenant>.<device>.telemetry.<key> fan-out it replaced no longer exists. The smoke still read the old per-key paths, so it failed with "twin_state did not hold both keys" — which reads as a dead latest-values pipeline rather than a stale assertion, and sent the last investigation looking at Bento instead of at this line. Now reads the single device document and requires BOTH keys to carry the published value inside it, which is what actually proves the merge. The failure message prints the document so the next mismatch is diagnosable from the log. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
29 harnesses opened FLOW_TEST_PG_DSN and ran `DROP TABLE IF EXISTS asset CASCADE` (and a dozen siblings) for a clean slate. Against the disposable Postgres CI starts as a service container that is fine. Against any database a human might point that variable at — a local dev stack, a shared scratch database, a pilot — it is a data-loss bug waiting for one careless export. They also collided with each other; CI hid that with `go test -p 1`, so the breakage only reached whoever ran `go test ./...` locally, where packages run in parallel by default. internal/testdb.Scoped creates a private schema and returns a DSN carrying `search_path=<schema>`, so the existing destructive DDL resolves inside it and cannot reach public. The harness bodies did not have to change — only the DSN they open. search_path is a CONNECTION PARAMETER, not a `SET`. sql.DB is a pool: `SET search_path` binds only the connection that served it, so under concurrency a later query lands on one still pointed at public — the confinement failing open, silently, exactly when load makes it matter. Verified against postgres:16: 64 concurrent queries, 0 unconfined. The migration then exposed two real defects that had been invisible: - internal/twin's tests only passed because internal/customer, internal/dashboard and friends had created `customer`, `dashboard`, `entity_view`, `device_profile` and `asset_profile` in the shared public schema earlier in the run. The expand CTE resolves a relation's endpoints against all of them. Once each package got its own schema that borrowed state vanished and the traversal 500'd. The isolation did not break those tests; it revealed what they leaned on. - The 0014/0015 migration tests queried information_schema and pg_constraint without pinning current_schema(). Those catalogues span the whole database, so once packages hold concurrent copies of `policy` the assertion counts them all and reports "found 2" for a correct migration. Measured: 2 of 5 parallel runs failed before, 0 of 6 after. tools/python/test_db_test_isolation.py enforces all three rules so none of them can quietly come back. Also: POST/PUT/DELETE /api/queues fell through to the SELECT and answered 200 with the queue LIST, so the UI reported success for a queue that was never created. The audit left this open as a product decision; the decision is not to implement the write. Queues and their consumers are Helm/k8s-owned here — the durables come from the nats-bootstrap hook and nothing in the data plane reads the `queue` table — so a row written through the API would configure nothing, which is a more expensive lie than a refusal. Answered honestly with 501 naming where queue configuration actually lives. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The up-down-up gate has been red on develop since 2026-08-10 and on main since
2026-08-13. It only ran again now because this branch touches a path under
flow-core/internal/migrations/, and the failure is real:
pq: cannot drop column security_status of table device because other objects
depend on it, view device_info_active_attribute_view depends on ...
device_info_active_attribute_view, device_info_active_ts_view and
device_info_view are all `SELECT d.*` over `device`, so they depend on every
column it has. Postgres refuses the DROP COLUMN, and 0008 could never roll back.
`DROP COLUMN ... CASCADE` would have "worked" by silently deleting all three
views and leaving nothing that recreates them — a rollback that quietly breaks
every device list query is worse than one that refuses.
The down migration now drops the views, drops the column, and rebuilds them from
the same definitions the baseline installs. Rebuilt after the column is gone,
`SELECT d.*` yields exactly the pre-migration shape.
That alone still left the cycle asymmetric, which the gate does not check and
which was only visible on inspection: a view's column list is fixed at creation
time, so after up -> down -> up the views came back WITHOUT security_status —
one column short, silently. Nothing in flow-core reads it through a view today
(every query goes straight to `device`), so this was a latent trap rather than a
live bug, but "the schema after a round trip differs from the schema before" is
exactly what this gate exists to prevent. The up migration now rebuilds the
views too, so the pair is symmetric.
Verified locally against postgres:16 by reproducing the gate step for step:
baseline -> force 1 -> up -> goto 1 -> up, then asserting the column, all three
views, their exposed columns, the CHECK constraint and the index are all back.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The mechanical migration wrapped every `sql.Open("postgres", dsn)` in
testdb.Scoped, including the one in internal/telemetry whose DSN points at
QuestDB. QuestDB speaks the Postgres wire protocol but has no schemas, so
CREATE SCHEMA and search_path both fail there and two working tests started
failing.
It stayed invisible locally because FLOW_TEST_QUESTDB_DSN is normally unset and
the tests skip; only CI sets it. Verified the fix the way I should have verified
the migration: against a real questdb:7.4.2, with both DSNs set, serially and in
parallel.
Confinement is also unnecessary there — the target is a dedicated QuestDB
instance, not a shared Postgres someone might also be using.
The isolation guard now rejects wrapping a non-Postgres DSN, and reads CODE
rather than raw source. A substring assertion over raw source matches the very
comment explaining why something must not be done — which it did here, flagging
the new "NOT testdb.Scoped" comment as an offender.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Test isolation
29 harnesses opened
FLOW_TEST_PG_DSNand ranDROP TABLE IF EXISTS asset CASCADEfor a clean slate — safe against CI's disposable service container,a data-loss bug against any database a human might point that variable at. They
also collided with each other; CI hid it with
go test -p 1, so the breakageonly reached whoever ran
go test ./...locally.internal/testdb.Scopedreturns a DSN carryingsearch_path=<throwaway schema>,so the existing destructive DDL resolves inside it. The harness bodies did not
change — only the DSN they open.
search_pathis a connection parameter, not aSET.sql.DBis a pool:SET search_pathbinds only the connection that served it, so under concurrencya later query lands on one still pointed at
public— the confinement failingopen, silently, exactly when load makes it matter. Verified against postgres:16:
64 concurrent queries, 0 unconfined.
Two real defects the isolation exposed
internal/twinwas borrowing tables from other packages. Its tests onlypassed because
internal/customer,internal/dashboardand friends hadcreated
customer,dashboard,entity_view,device_profileandasset_profilein the sharedpublicschema earlier in the run. Once eachpackage got a private schema the traversal 500'd. The isolation did not break
those tests; it revealed what they had been leaning on.
information_schemaandpg_constraintare not schema-scoped, so withconcurrent copies of
policythe assertion counted them all and reported"found 2" for a correct migration. 2 of 5 parallel runs failed before, 0 of
6 after.
tools/python/test_db_test_isolation.pyenforces all three rules.POST /api/queues
Fell through to the SELECT and answered 200 with the queue list, so the UI
reported success for a queue that was never created. The audit left this open as
a product decision; the decision is not to implement the write. Queues and
their consumers are Helm/k8s-owned here — the durables come from the
nats-bootstrap hook and nothing in the data plane reads the
queuetable — so arow written through the API would configure nothing, which is a more expensive
lie than a refusal. Answered with 501 naming where queue configuration lives.
Image verification
sha-0aec1c5deployed to the test cluster: consumer guard green, bothentityViewscases 200 (the endpoint that used to 500), no panics, noERROR-level logs.
🤖 Generated with Claude Code