chore(stack): land the merged stack that never reached main - #22
Merged
Conversation
Result finalization (I4). writer.Close is where S3 and ClickHouse wait for the upload and return its error; ignoring it marked queries SUCCEEDED with nothing behind them. Its error now fails the query, drops the partial result and reports STORAGE_FINALIZE_FAILED. State persistence (I2, I4). A failed terminal write was logged once and the query then left the registry, so metadata could stay RUNNING for ever. Writes are retried with backoff, the registry entry is released only afterwards so the lease stays alive meanwhile, and a query that still cannot be recorded is left to the owner reaper. Owner fencing. The reaper wrote unconditionally and could replace a SUCCEEDED record, ResultRef included, with FAILED/OWNER_LOST. It now skips records this instance owns, ignores queries younger than two heartbeat intervals, and writes through UpdateQueryIfState. Every state change is checked against CanTransitionTo, which had no callers at all. Queries left behind by a previous process under the same instance ID are recovered once at startup. Drain admission (I5). The draining check sat outside the registration step, so a query could register after the shutdown loop had already seen zero in-flight. Both now happen under one lock, query goroutines are tracked in a WaitGroup, and CountInFlight takes the larger of the local registry and the MetaStore, so a restarted node does not claim to be quiesced. Idempotency (I3). The key is released when submission fails afterwards, is reclaimed when it points at a record that no longer exists, is re-armed against FinishedAt so it expires with the result, is stored on the record, and is resolved before Ping so a duplicate resolves while the database is down. Input validation. result_format reached os.Create as part of a file name; "../../etc/cron.d/x" created, truncated and then removed that file. Options are validated against a whitelist before any side effect, the fs backend works through os.Root and confines stored locators, and the S3 backend confines keys to its result prefix. Cancellation and timeouts. context.Canceled now yields CANCELED and context.DeadlineExceeded a QUERY_TIMEOUT failure instead of a blanket DB_EXEC_FAILED, and the storage writer receives the execution context so a stopped query aborts its upload rather than uploading in the background. Also: DML and DDL are rejected by default through a tokenizing guard, with allow_writes to opt out; concurrent executions can be capped; GC runs under a cluster lock and keeps metadata when the result could not be deleted; pools are opened outside the pool lock and really are recreated when the DSN, engine or max_conns changes, with the replaced pool drained rather than closed under running queries; reload reports the sections it had to ignore; Watch validates the query, replays the current state and ends after a terminal one; driver errors carrying the DSN stay in the log and callers get typed domain errors instead.
REST answered with the domain model, so `timeout`, `result_ttl` and `db_exec_duration` went out in nanoseconds while api/openapi/dbbridge.yaml promised `timeout_ms`, `result_ttl_seconds` and `db_exec_duration_ms`. The API accepted milliseconds and answered in nanoseconds, so no generated client could read a response. Explicit wire types now carry the documented names and units, and unset timestamps are omitted rather than sent as the year one. Errors. Everything except draining was a 500 (CodeInternal in Connect) whose body carried err.Error(); for a connection failure that text is the DSN with host, user and parameters. Both transports now map ValidationError to 400/InvalidArgument, NotFoundError to 404/NotFound, DrainingError and UnavailableError to 503/Unavailable and ResourceExhaustedError to 429/ResourceExhausted, and answer everything else with "internal error" plus a request ID that ties the response to the full log line. Connect also stops turning unset timestamps into large negative milliseconds. Timeouts. middleware.Timeout(60s) covered the whole router, which cut result downloads and sync submissions off after a minute and killed every WebSocket connection at the same age. It now applies only to the short-lived routes. Both listeners get ReadHeaderTimeout, IdleTimeout and MaxHeaderBytes, and the submission body goes through MaxBytesReader with a 413 on overflow; neither ReadTimeout nor WriteTimeout is set globally, they would break the streams. WebSocket. Origin checking was disabled outright, so any page in a browser inside the perimeter could open a connection and read other people's query events. Origins are now checked against server.ws_allowed_origins. Each watch message used to spawn a goroutine and register a channel that lived until the connection closed, with no cap and no check on the query ID; subscriptions are now tracked per connection, deduplicated, capped at 32, released on unwatch and on disconnect, and an unknown query ID is reported instead of hanging. Also: a Range starting past the end is a 416 with `bytes */<total>` rather than a 206 with a malformed `bytes 0--1/*` header, and X-Forwarded-For is only trusted for the configured number of proxy hops.
There was no authentication on any transport and no securityScheme in the OpenAPI document. Anyone who could reach the port could run arbitrary SQL under the service credentials of every configured database, stop other people's queries, read their results and reload the process. internal/authn holds static bearer tokens loaded from auth.tokens, with the value normally taken from an environment variable so it never has to sit in a ConfigMap. Comparison is constant time across all tokens with no early exit. Scopes are read (status, stats, download, list, watch), write (submit, stop) and admin (reload, can-stop); admin implies the others. The package sits outside internal/transport because the core stamps a query with the subject that submitted it and must not import a transport. REST gates every /v1 route on its scope, the Connect handler gets an interceptor that covers unary and streaming calls alike, and an unmapped RPC defaults to admin so a new method is never accidentally public. The health probes stay open; a configured auth section that resolves to no usable token is a startup failure rather than a silent pass-through. Knowing a query ID was also enough to read anyone's SQL, status, stats and result. QueryRecord now carries the submitting subject, and status, stats, download, stop and watch check it. Admin acts across subjects; a record written before subject binding has no owner and is admin-only. A foreign query answers 404, not 403, so the API does not confirm that an ID exists. Finally, /metrics and /v1/admin/* move to their own listener when server.admin_addr is set: the metric labels enumerate every configured db_id and the admin routes reload the process, so neither belongs on the public port.
result_format: parquet was serialized as JSONL, so clients received a .parquet file with JSON lines inside and `format: "parquet"` in the metadata. It now produces a real Parquet file: the head of the stream is sampled to infer a type per column, everything past the bounded sample is streamed, SQL NULL round-trips through optional fields, and a round-trip test opens the result with a parquet reader. Two properties are documented in the encoder: columns come out in alphabetical order because parquet-go sorts schema group fields, and duplicate SQL column names get a numeric suffix. The ClickHouse ResultStore was written but never registered in the executable, so storage_backend: clickhouse failed with "unknown storage backend" only after the SQL had already run. It is now built from storage.clickhouse, registered, and closed on shutdown. A default_storage that cannot be built is rejected at config load, not after the first query. The FS store is only created when the configuration asks for it: doing it unconditionally meant MkdirAll on every start, which fails under a read-only root filesystem even when results go to S3. ResultRef.Checksum was always empty. It is now a sha256 computed from the same bytes on their way to storage, so it costs one pass and never needs the result read back.
Watchers lived in a per-process map and Redis Pub/Sub only carried STOP_QUERY, so a subscription opened through any instance other than the owner never received an event, which breaks I2 for WebSocket and WatchQuery. Events now travel over the control channel as QUERY_EVENT and are fanned out to the local subscriptions of every instance. An instance ignores its own announcements, and publishing runs in its own goroutine with a bounded queue so it can never stall the query that produced the event. Stats were written once, at completion, so a query that ran for an hour reported nothing until it was already over. The encoder now calls back every thousand rows, and the manager persists and announces the row and byte counts from there, throttled to one write every couple of seconds with the first batch reported immediately. The instance lifecycle gains STOPPABLE, which spec section 9 defines but which was never reachable: a draining instance advances to it once it holds zero in-flight owned queries, /v1/admin/can-stop and CanIBeStopped report it, and readiness keeps returning 503 through it so the node stays out of rotation until termination. Telemetry had two parallel metric stacks: the domain metrics were on prometheus/client_golang while an OTel MeterProvider was raised beside them and never given a single instrument, so nothing reached OTLP. The instruments are OTel now and Prometheus is one of the readers behind them, which keeps /metrics serving the same names, labels and histogram buckets while OTLP finally carries the same data. Go runtime metrics stay on the Prometheus collector, which is what exposes the full runtime/metrics ruleset. Execution spans also lost their parent, because I1 detaches the execution context from the request. The submitting span context is carried over as a span link, so a trace can still be followed from transport to execution without pretending the two share a lifetime.
…ort tls
The container had no USER and the pod no securityContext, so the process ran
as root: any file-handling defect became "write anywhere in the container".
It now runs as UID 10001 with a read-only root filesystem, all capabilities
dropped, no privilege escalation and the RuntimeDefault seccomp profile, with
emptyDir volumes for the result directory and /tmp because a read-only root
leaves nowhere else to write.
The config loader had no variable substitution, so `dbbridge-$(POD_NAME)` in
the ConfigMap stayed a literal and both replicas reported the same owner,
which makes leases, remote cancellation and owner-loss detection meaningless.
`${VAR}` is now expanded from the environment and an unset variable is a
startup error rather than an empty string. A bare `$VAR` is left alone, so
DSNs and passwords may contain a dollar sign.
That same mechanism moves the Redis password, the S3 credentials and the API
tokens out of the ConfigMap and into a Secret.
TLS is configurable for all three listeners; without a certificate gRPC still
runs as cleartext HTTP/2, but that now has to be acknowledged with
server.tls.allow_h2c or it logs a warning on every start.
The dev stack no longer publishes Redis without a password, and the MinIO
bucket is no longer made anonymously readable, which had published every
query result to anyone who could guess an object key. Metrics move to the
admin listener, so Prometheus scrapes port 8081 and the public Service does
not carry it.
Every test ran on the in-memory MetaStore, the local filesystem and fake drivers, so the paths that only exist for the real backends had no coverage at all: Lua scripts and WATCH transactions in Redis, lease key expiry, pgx and MySQL type mapping, the S3 multipart uploader and the Close-waits-for-upload contract behind I4. test/integration runs those against Redis, PostgreSQL, MySQL and MinIO under testcontainers, behind the `integration` build tag and a separate CI job so a Docker problem is never mistaken for a code failure. It covers a full round trip per backend, multi-node idempotency, a subscription and a stop crossing instances, owner-loss reaping, the GC lock, and a heartbeat storm that must not clobber a terminal write. govulncheck reported two reachable advisories: GO-2026-6061 in grpc and GO-2026-5970 in x/text. Both are patched, and the check now runs in `make ci` and in the CI workflow. That check also raises the floor on the toolchain, because most of what it finds is in the standard library rather than in a dependency: go.mod asks for 1.26.6, which is the first release without any of the advisories reachable from this code. CI installs exactly what go.mod declares, so leaving it at 1.26.0 meant shipping a binary built against a stdlib with two dozen known issues in net/http, crypto/tls, crypto/x509 and html/template. spec.md gains the security section it never had - authentication, authorization and subject binding, the statement policy, TLS and trusted proxies, request and concurrency limits, listener isolation and error sanitization - which was a gap in the specification rather than a divergence from it. It also states what a reload cannot apply, and records that leases live outside the query record and that query events travel the control channel.
The concurrency semaphore bounds how many queries run at once, but nothing bounded how fast they could be submitted, so one client could still churn through pool connections, storage writes and idempotency keys as fast as the process could accept or reject them. server.rate_limit gives each caller a token bucket, keyed by authenticated subject where there is one and by the resolved client address otherwise, on both REST and Connect. Idle buckets are evicted so the map cannot grow without bound. The health probes are exempt: the kubelet calls them on a schedule and must not be starved by a noisy caller, nor consume its budget.
The interface sketches in sections 5.1 through 5.3 predate the implementation and had drifted: the service methods are named after the RPCs in section 7, a driver takes a DSN string and a pool size, a RowStream reports column names because no supported driver offers portable type metadata before the first row, and a ResultStore Writer mints the ResultRef rather than receiving one, since only the backend knows the locator it is about to write to. Section 12 now matches the tree, including the two departures from the original sketch and why they were made: idempotency has no package of its own, and internal/state is flat. Section 13 records what the code actually depends on, including why the config layer is plain yaml.v3 rather than viper or koanf, and section 11 describes the single metrics pipeline and the span link that stands in for the parent I1 makes impossible.
testcontainers does not read the docker CLI context, so on a colima host it reported "rootless Docker not found" and every integration test failed before starting a container. The target now spells out both variables it needs when the current context is colima: DOCKER_HOST is the host-side socket it connects to, and TESTCONTAINERS_DOCKER_SOCKET_OVERRIDE is that same socket as seen inside the VM, which is what its reaper container bind-mounts. Both stay empty on Docker Desktop and in CI, where the default socket is already correct for each. Also stops logging a draining 503 as an ERROR. It is expected operation and would fill the log on every rolling deploy; an unreachable database is already logged where it is detected, with the driver detail the response deliberately drops.
chore(test): make the container suite runnable under colima
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.
Ten merged pull requests, #12 through #21, that never reached
main.Every PR in that stack was based on the branch below it rather than on
main, and GitHub only retargets a base when the base branch is deleted. The branches were kept, so each merge landed in its own base: #12 intofix/state-consistency, #13 intofix/core-invariants, and so on up to #21 intodocs/spec-code-alignment. The work rolled up the stack instead of down intomain, which received only #11.This brings the whole accumulated chain over in one step. The diff is 81 files, and the only commit
mainholds that this branch does not is the merge node of #11, whose content (672d6e2) is already here, so the merge is conflict-free.What is in it
3cb1caefix(manager): close the gaps in the mandatory invariants3f449cafix(transport): align responses with the contract and bound the surfacecb77494feat(authn): require credentials and bind queries to their subject0c7432bfeat(storage): implement parquet, register clickhouse, checksum results4c4e8a6feat(manager): deliver events across instances and report live progress359f8a9chore(deploy): run unprivileged, keep secrets out of the config, support tls55d41b2chore(test): cover real backends, patch dependencies, document securityf7f9299feat(server): cap the request rate per caller64cdda2docs(spec): reconcile interface sketches and layout with the code2a04130chore(test): make the container suite runnable under colimaEach commit carries its own description in the pull request it came from; nothing is new here beyond the merge itself.
Operational notes for whoever deploys this
/metricsnow requires theadminscope wherever it is mounted, because its labels enumerate every configureddb_idand the traffic volume per database. Any external scraper needs a token; the shipped Prometheus config has one.authon binds new records to a subject and leaves older ones reachable only byadmin. Switch it on while the instance is idle, or expect queries submitted before the switch to answer404to their owners untilresult_ttlexpires.authis reported underreport.ignoredby a reload rather than applied, so revoking a token takes a restart.deploy/k8s/secret.yamlis nowsecret.example.yaml. Copy it, fill it in and apply the copy;kubectl apply -f deploy/k8s/no longer picks up placeholder tokens that would fail validation.Verification
make ciin full (fmt-check, vet, test, race, lint, buf lint, govulncheck) - green on the tip.make test-containerswith-raceagainst real Redis, PostgreSQL, MySQL and MinIO - 8/8.A compose stack was exercised by hand: scope enforcement on the public and admin listeners, Prometheus scraping the gated
/metricswith a token, DML rejection, S3 materialization, and idempotency keys staying inside their subject.After merging
The intermediate branches (
fix/state-consistencythroughdocs/spec-code-alignment) hold merge nodes that carry no content of their own. They can be deleted once this lands.