diff --git a/.github/workflows/e2e-nightly.yml b/.github/workflows/e2e-nightly.yml index 5f166de59..9c7912985 100644 --- a/.github/workflows/e2e-nightly.yml +++ b/.github/workflows/e2e-nightly.yml @@ -48,6 +48,12 @@ jobs: go-version: "1.27.2" cache: true + # A digest the docs pin is checked by nothing else, and three that did + # not exist shipped in the deployment guide (#2224). It asks the + # registries, so it runs here rather than in verify. + - name: Resolve the image digests the docs pin + run: make docs-images-check + - name: Start E2E environment run: make e2e-up diff --git a/.semgrep/go-advisory-lock.yml b/.semgrep/go-advisory-lock.yml new file mode 100644 index 000000000..1c50ef04a --- /dev/null +++ b/.semgrep/go-advisory-lock.yml @@ -0,0 +1,31 @@ +rules: + # A try-lock the platform holds across work goes onto the process's one lock + # session, internal/advisorylock (#2169). Each loop used to take its lock on + # a pooled connection of its own and hold it for the whole pass while the + # pass drew its queries from the same pool; with four such loops on a pool of + # five, the passes waited on the pool for their own queries, never released + # their locks, and starved the replica. connoauth's blocking per-call lock is + # the one session lock taken elsewhere, since a lock that waits cannot share + # a session. Test code takes what locks it likes, to stand in for another + # replica. + - id: advisory-try-lock-outside-advisorylock + languages: [go] + severity: ERROR + paths: + include: + - pkg + - internal + - cmd + exclude: + - "*_test.go" + - /internal/advisorylock + - /bench + pattern-regex: pg_try_advisory_lock\( + message: >- + Take a session advisory lock through internal/advisorylock: advisorylock.Try(ctx, db, + advisorylock.ID(key)) and defer the release it returns, and run the locked work on the pool. + A try-lock taken on a connection of its own pins that connection for as long as it is held, + and enough of them starve the pool their holders need to finish (#2169). + metadata: + category: reliability + issue: "#2169" diff --git a/CLAUDE.md b/CLAUDE.md index 52ef13b26..9e303d230 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -203,7 +203,7 @@ mcp-data-platform/ │ ├── connid/ # Connection identity: the instance a connection is stored under, the name a call binds it by, the toolkit serving it, and which half of the config owns it — one Resolver, distinct types │ ├── connview/ # Builds the list_connections view (configured + discovered) │ ├── contenttype/ # Media-type detection and normalization for every content write path -│ ├── database/ # Database utilities (migrate/ = golang-migrate runner + 207 embedded SQL migrations) +│ ├── database/ # Database utilities (migrate/ = golang-migrate runner + 209 embedded SQL migrations) │ ├── embedding/ # Text embedding generation for memory vector search │ ├── indexjobs/ # Postgres-backed, source-kind-agnostic background indexer │ ├── knowledge/ # Unified read path for platform knowledge (federation/ = live toolkit registry adapter) @@ -250,6 +250,7 @@ mcp-data-platform/ │ ├── apigwmetrics/ # The api gateway's outbound HTTP instrumentation as an http.RoundTripper: the connection/status labels, and the persona the call was authorized under, read off the request context the tool call carries (#1615). Holds no gateway types, and was extracted when pkg/toolkits/apigateway reached its package-size budget │ ├── outbound/ # The one transport chain every HTTP client the platform builds is made from (#1895): the User-Agent, a client span named `{kind} {method}` that continues the caller's trace and carries the W3C traceparent to the upstream (off per connection with `trace_propagation: false`), `http_client_requests_total{kind,connection,status_class}` and its histogram, and `egress_blocked_total{reason}` with one WARN when the egress guard under it refuses. `NewClient(Options)` or `Transport(base, opts)`; the recorder is the platform's one Metrics, installed by internal/platform/obs through `SetDefaultMetrics`, and `Metrics()` is how a retry loop counts beside the requests it issues. `.semgrep/go-outbound-client.yml` refuses an `http.Client` built anywhere else │ ├── opsobs/ # The recorder the platform's own operations report through (#1898): installed by internal/platform/obs like outbound's default, read by the authenticator chain (auth_attempts_total), the OIDC JWKS fetch, the HTTP gate's fail-open, browser sign-in and the domain operations (`Start(ctx, Op*)` opens a span under the call and counts domain_operations_total{operation,result}); also the label vocabulary those series use (AuthMethod*, AuthReason*, Denial*, Dependency*, Connection*, Op*, Sink*), kept off pkg/observability's exported surface +│ ├── advisorylock/ # The one session per process every PostgreSQL try-lock held across work is taken on (#2169): the background loops (catalog scan, maps, storage and webhook alerts, retention, capacity, the call catalog and audit sweeps, thumbnail sweeps, catalog import, resource reclassification), connlimit slots and the connoauth refresher. A lock on a pooled connection of its own pinned that connection for the whole pass while the pass drew from the same pool, and four such loops starved a pool of five. `Try(ctx, db, ID(key)|Pair(a,b))` returns a release; a key the process holds is refused, since PostgreSQL would grant it again; the session goes back to the pool with its last lock. `.semgrep/go-advisory-lock.yml` refuses a try-lock taken anywhere else │ ├── bgloop/ # The one way the platform runs work in the background (#1897): `Run` (a timer, a wake channel, a stop channel), `Consume` (a channel of events), `Unit` (one claimed unit of work) and `Listen` (a LISTEN connection's state). Every iteration is counted by result, timed and stamped on success (`background_loop_*{loop}`), gets a span whose context its body logs with, and a skip under another replica's lock is `ErrSkipped`; loop names are constants in names.go. `.semgrep/go-background-loop.yml` refuses a ticker or an infinite loop around a select anywhere else │ ├── objectobs/ # Every object operation on a bucket the platform owns (#1896): `Do` wraps one put/get/get_range/list/delete in a `storage.` client span and one `storage_operations_total{purpose,operation,result,reason}` observation, `Reason` reads a failure into access_denied/bucket_missing/quota_exceeded/not_found/other, and `WithPurpose` lets a caller writing through a shared client (the tile worker, an export, a script's output) report under its own purpose. Called by pkg/portal/s3adapter and the webhook objects; records through outbound.Metrics() │ ├── dbobs/ # The platform's PostgreSQL pool opened through otelsql (#1896): a `postgres.` span per statement inside a traced request (none outside one), `db_client_operation_duration_seconds{operation}` and `db_client_slow_statements_total{operation}`, where the operation is the statement keyword, `vector_search` for a pgvector ranking, or the transaction step. The SQL text is on the span only with `OTEL_TRACES_INCLUDE_DB_STATEMENT=true`; the slow threshold is `MCP_PLATFORM_DB_SLOW_STATEMENT_THRESHOLD` (1s) diff --git a/Makefile b/Makefile index c2535ec61..4886de00d 100644 --- a/Makefile +++ b/Makefile @@ -53,7 +53,7 @@ GOVET := $(GO) vet GOFMT := gofmt GOLINT := golangci-lint -.PHONY: all build test lint lint-full fmt clean install help docs-serve docs-build verify verify-release alert-rules-test builtin-pages-check \ +.PHONY: all build test lint lint-full fmt clean install help docs-serve docs-build docs-images-check verify verify-release alert-rules-test builtin-pages-check \ tools-check dead-code mutate patch-coverage patch-coverage-check coverage-summary doc-check acceptance acceptance-release acceptance-check acceptance-release-check release-tag-check schedule-lane schedule-lane-ui realdb-lane state-readers-check e2e-copy-check route-manifest-check posture-check preverify preverify-fast swagger swagger-check verify-checks verify-go verify-static verify-docker verify-ui vet-tags \ semgrep semgrep-diff semgrep-changed codeql sast osv embed-clean migrate-check \ frontend-install frontend-build frontend-build-content-viewer content-viewer-embed \ @@ -546,6 +546,10 @@ patch-coverage-check: @echo "Checking patch coverage..." @PATCH_COVERAGE_THRESHOLD=$(PATCH_COVERAGE_MIN) ./scripts/patch-coverage.sh +## docs-images-check: Resolve every image the docs pin to a digest against its registry (network and Docker; run by the E2E Nightly) +docs-images-check: + @./scripts/docs-image-digests.sh docs + ## doc-check: Fail on orphaned docs, unresolved doc links, or unregistered tool refs; warn on undocumented changes doc-check: @./scripts/doc-check.sh diff --git a/docs/images/screenshots/dark/admin-admin-connection-oauth-dark.webp b/docs/images/screenshots/dark/admin-admin-connection-oauth-dark.webp index c442cd95a..fb6b4cc9b 100644 Binary files a/docs/images/screenshots/dark/admin-admin-connection-oauth-dark.webp and b/docs/images/screenshots/dark/admin-admin-connection-oauth-dark.webp differ diff --git a/docs/images/screenshots/dark/admin-admin-connection-token-lifetime-dark.webp b/docs/images/screenshots/dark/admin-admin-connection-token-lifetime-dark.webp new file mode 100644 index 000000000..96252c648 Binary files /dev/null and b/docs/images/screenshots/dark/admin-admin-connection-token-lifetime-dark.webp differ diff --git a/docs/images/screenshots/light/admin-admin-connection-oauth-light.webp b/docs/images/screenshots/light/admin-admin-connection-oauth-light.webp index 705526f16..ab5cefa72 100644 Binary files a/docs/images/screenshots/light/admin-admin-connection-oauth-light.webp and b/docs/images/screenshots/light/admin-admin-connection-oauth-light.webp differ diff --git a/docs/images/screenshots/light/admin-admin-connection-token-lifetime-light.webp b/docs/images/screenshots/light/admin-admin-connection-token-lifetime-light.webp new file mode 100644 index 000000000..ea07f3ccc Binary files /dev/null and b/docs/images/screenshots/light/admin-admin-connection-token-lifetime-light.webp differ diff --git a/docs/llms-full.txt b/docs/llms-full.txt index 7091dbf55..0e4bc3693 100644 --- a/docs/llms-full.txt +++ b/docs/llms-full.txt @@ -483,7 +483,7 @@ The MCP SDK caps each Streamable HTTP request body at 4 MiB and rejects a larger | Field | Type | Default | Description | |-------|------|---------|-------------| | `database.dsn` | string | - | PostgreSQL connection string | -| `database.max_open_conns` | int | 25 | Maximum open database connections | +| `database.max_open_conns` | int | 25 | Maximum open database connections; at least 4, refused at startup below that (#2169). Every background try-lock (the locked loops, connlimit slots, the connoauth refresher) is held on one session per process, internal/advisorylock | Setting `dsn` enables audit logging, knowledge capture, session externalization, and OAuth persistence. Without it, these features degrade to in-memory or noop implementations. @@ -1384,7 +1384,7 @@ Every inbound HTTP request is measured once, outside every handler, under the ro `mcp_platform_config_info{toolkit_kinds,auth_methods,tracing,metrics_exporter,sampler_ratio} 1` names what the process runs with and never carries an address or a secret. `config_validation_warnings_total{code}` counts every configuration warning, at boot and when a persona written through the admin API raises one: `persona_tool_unregistered` (a `tools.allow` entry without a wildcard naming a tool the deployment does not register), `persona_incoherent`, `unused_connection`, `agent_instructions_unknown_tool`, `unrecognized_keys`, `deprecated_api_version`, `deprecated_key`, `placeholder_unexpanded`, `ephemeral_oauth_signing_key`, `portal_no_object_storage`, `resources_no_object_storage`, `memory_no_embedding`, `exclude_persona_unknown`, `deployment_id_unset`. Initialization logs one `platform initialized` line with the version and each `phase__ms`, and writes a `platform.startup` span with a child per phase. -`dependency_up{dependency}` (`database`, `semantic`, `query`, `object_storage`, `idp`, `renderer`) is 1 when the dependency answered its last ping; `/readyz` does not read it, so an upstream outage never takes every replica out of a load balancer. The pings re-run at most once per `server.state_probe_interval` (default `1m`), only when scraped, and never hold a scrape. `mcp_platform_connections{kind,state}` counts connections by the state the last call through each one found it in, and sends nothing to find out: `healthy` (the upstream answered), `unreachable` (no answer, or 502/503/504), `auth_failed` (HTTP 401/407 from an API, GraphQL, MCP or Trino upstream; an S3 key or signature refused), `configured` (no call has used it on this replica); read with `max by (kind, state)`. `mcp_platform_personas` counts the registered personas, `search_index_last_indexed_age_seconds{kind}` how long ago each search source last finished indexing. +`dependency_up{dependency}` (`database`, `semantic`, `query`, `object_storage`, `idp`, `renderer`) is 1 when the dependency answered its last ping; `/readyz` does not read it, so an upstream outage never takes every replica out of a load balancer. The replica's own pool is the exception: every connection in use with none handed out within 500ms for 30s answers `/readyz` 503 `database_pool_starved` (#2169). The pings re-run at most once per `server.state_probe_interval` (default `1m`), only when scraped, and never hold a scrape. `mcp_platform_connections{kind,state}` counts connections by the state the last call through each one found it in, and sends nothing to find out: `healthy` (the upstream answered), `unreachable` (no answer, or 502/503/504), `auth_failed` (HTTP 401/407 from an API, GraphQL, MCP or Trino upstream; an S3 key or signature refused), `configured` (no call has used it on this replica); read with `max by (kind, state)`. `mcp_platform_personas` counts the registered personas, `search_index_last_indexed_age_seconds{kind}` how long ago each search source last finished indexing. `domain_operations_total{operation,result}` and `domain_operation_duration_seconds{operation}` count the platform's own operations, each with a span of its name under the calling `tools/call`: `knowledge.apply`, `memory.capture`, `memory.manage`, `search.query`, `search.fetch`, `calls.record`, `calls.reuse`, `calls.promote`, `calls.sweep`, `configstore.read`, `configstore.write`, `resource.upload`, `resource.extract`, `table.register`, `prompt.serve`. `knowledge_changes_total{sink,result}` counts what `apply_knowledge` changed (`catalog`, `knowledge_page`, `agent_instructions`: `applied`/`failed`; `insight`: `created`/`approved`/`rejected`); `search_results_returned_total` sums the hits search returned; archive extraction counts `archive_members_extracted_total`, `archive_extracted_bytes_total` and `archive_refusals_total{reason}`. @@ -2038,7 +2038,7 @@ Lifecycle operations for existing memory records; create new records with `memor Reading memory back moved to the universal `search` tool (see Knowledge Tools): relevance ranking, entity-keyed lookup, and catalog lineage/graph traversal are all served there. The memory toolkit retains `memory_manage` for the write path. -The ranking it uses fuses embedding cosine similarity with a lexical full-text match signal, blended as `0.6 * semantic + 0.4 * lexical`, which improves recall on identifier-heavy content (entity URNs, column names, error codes) over pure vector search. The vector arm uses an `hnsw` ANN index on `memory_records.embedding`; the lexical arm a GIN index on `to_tsvector('english', content)`. When no embedding provider is available it degrades to lexical-only instead of erroring, which also surfaces rows whose embedding is NULL (saved during an outage). +The ranking it uses fuses embedding cosine similarity with a lexical full-text match signal (over `fts_name_words(content)`, which reads `-`, `_`, `.`, `/` and `:` as word breaks beside the text as written, #2230; the names, paths, file names, tags and titles of resources, assets, collections, prompts, knowledge pages, scripts and catalog datasets are read the same way, and a row or API/GraphQL operation not yet embedded is blended at a cosine of zero, so its lexical match outranks every non-match), blended as `0.6 * semantic + 0.4 * lexical`, which improves recall on identifier-heavy content (entity URNs, column names, error codes) over pure vector search. The vector arm uses an `hnsw` ANN index on `memory_records.embedding`; the lexical arm a GIN index on `to_tsvector('english', content)`. When no embedding provider is available it degrades to lexical-only instead of erroring, which also surfaces rows whose embedding is NULL (saved during an outage). Memory registers as a consumer of the shared index-jobs framework (`source_kind = memory`). The synchronous embed-on-write is preserved so a just-saved memory stays immediately recallable; a periodic reconciler backfills embeddings that were missed during an embedder outage (`embedding IS NULL`) or invalidated by a provider model swap (`embedding_model` differs from the current model), and the `memory` kind appears on the admin Indexing dashboard with an indexed/expected coverage ratio. Migration `000054_memory_hybrid_search` adds the `embedding_model` and `embedding_text_hash` breadcrumb columns plus the hnsw and GIN indexes. @@ -2942,7 +2942,7 @@ An external system posts events to `POST /hooks/{source}` (#1870). Each source i # Storage Capacity -Admin > Storage (#2148): every store the platform writes to, used and free, growth, fill date and level. Stores: the database (pg_database_size against `database_limit_bytes`, the operator's, since the platform cannot read the database server's disk), the SeaweedFS cluster (the master's `/dir/status` lists the volume servers, each one's `/status` reports its disks; `seaweedfs.volume_urls` replaces the master's list where it names addresses the platform cannot reach), each bucket (storage_usage summed over purposes, against a budget from the settings, which overrides MCP_PLATFORM_STORAGE_BUDGETS bucket by bucket and also feeds the storage_bucket_budget_bytes gauge), each replica's webhook spool volume (statfs of the spool directory, reported in webhook_spool_state fs_total_bytes/fs_free_bytes). One replica (advisory lock 4713210402) samples every store every 15 minutes (`MCP_PLATFORM_STORAGE_HISTORY_INTERVAL`) into `storage_samples`, kept `history_days` (180). Growth is the least-squares slope over 7 and 30 days in bytes per day, fitted about the means so a store holding one size reads exactly zero; the 7-day rate needs a day of history and the 30-day rate a week (#2201), and each store carries `history_hours`, `rate_7d_min_history_hours` and `rate_30d_min_history_hours` so the page says "Not enough history" with how much there is; the fill date uses the slower growing rate and names it. Settings `GET|PUT /api/v1/admin/settings/storage` (platform_settings section `storage`): database_limit_bytes, budgets, seaweedfs, history_days, used_percent (75/90), days_left (30/7), channels; an absent field keeps its value. Alerts post `storage::used::` and `storage::exhaustion::` and resolve once; a store a pass could not read (the cluster on a failed read, buckets whose capacity it gives, a spool whose replica missed recent reports) keeps its condition. `GET /api/v1/admin/storage` reads everything now (stores, database tables and indexes, buckets by purpose with last listing, reconcile orphaned/dangling, last scan, the SeaweedFS reading or its error, spools); `GET /api/v1/admin/storage/history?store=database|seaweedfs|bucket:|spool:&days=n` reads a store's samples. +Admin > Storage (#2148): every store the platform writes to, used and free, growth, fill date and level. Stores: the database (pg_database_size against `database_limit_bytes`, the operator's, since the platform cannot read the database server's disk), the SeaweedFS cluster (the master's `/dir/status` lists the volume servers, each one's `/status` reports its disks; `seaweedfs.volume_urls` replaces the master's list where it names addresses the platform cannot reach), each bucket (storage_usage summed over purposes, against a budget from the settings, which overrides MCP_PLATFORM_STORAGE_BUDGETS bucket by bucket and also feeds the storage_bucket_budget_bytes gauge), each replica's webhook spool volume (statfs of the spool directory, reported in webhook_spool_state fs_total_bytes/fs_free_bytes). One replica (advisory lock 4713210402) samples every store every 15 minutes (`MCP_PLATFORM_STORAGE_HISTORY_INTERVAL`) into `storage_samples`, kept `history_days` (180); a replica that finds a sample within nine tenths of the interval takes none (#2203). Growth is the least-squares slope over 7 and 30 days in bytes per day, fitted about the means so a store holding one size reads exactly zero; a store's first day (from its first kept sample, when nothing before it was pruned) is left out of every fit as setup, and after it the 7-day rate needs a day of history and the 30-day rate two weeks (#2201, #2203), and each store carries `history_hours`, `rate_7d_min_history_hours` and `rate_30d_min_history_hours` so the page says "Not enough history" with how much there is; the fill date uses the slower growing rate and names it. Settings `GET|PUT /api/v1/admin/settings/storage` (platform_settings section `storage`): database_limit_bytes, budgets, seaweedfs, history_days, used_percent (75/90), days_left (30/7), channels; an absent field keeps its value. Alerts post `storage::used::` and `storage::exhaustion::` and resolve once; a store a pass could not read (the cluster on a failed read, buckets whose capacity it gives, a spool whose replica missed recent reports) keeps its condition. `GET /api/v1/admin/storage` reads everything now (stores, database tables and indexes, buckets by purpose with last listing, reconcile orphaned/dangling, last scan, the SeaweedFS reading or its error, spools); `GET /api/v1/admin/storage/history?store=database|seaweedfs|bucket:|spool:&days=n` reads a store's samples. # Content Types and Viewers @@ -4385,7 +4385,7 @@ No upstream of this class is reachable from the project — every one is custome Signing uses the `jwt_*` keys with two defaults of the grant's own: `jwt_algorithm` defaults to `RS256` (`ES256` alike; `HS256` over `jwt_client_secret` accepted) and `jwt_audience` defaults to `oauth_token_url`. At least one of `jwt_issuer` (usually the application's client id) and `jwt_subject` (the integration user) is required, as under `signed_jwt`; an unset one is omitted from the assertion (#2061). The assertion carries `iss`, `sub`, `aud`, `iat`, `exp` and a random 128-bit hex `jti`, so two replicas exchanging in the same second never present the same assertion to an upstream that refuses replays. -The exchange POSTs `grant_type=urn:ietf:params:oauth:grant-type:jwt-bearer` and `assertion=` to `oauth_token_url`. A token from `client_credentials` or `jwt_bearer` whose response states no `expires_in` is reused for `oauth_token_lifetime` (duration string or seconds, default 15m, #1737/#2218); a stated `expires_in` always wins. `oauth_scope_placement` (#2061) says where `oauth_scope` goes: `param` (default) as the `scope` form parameter, `claim` as a `scope` claim in the signed assertion and not on the form (what Google reads), or `both`. `oauth_client_id` and `oauth_client_secret` are optional client authentication placed by `oauth_endpoint_auth_style`; with no secret, a client id goes as a form parameter and no Basic header is sent (an empty Basic credential is one an upstream may refuse). The request uses the same bounded, redirect-refusing, CA-bundle-honoring client as `client_credentials`. The access token is cached by `oauth2.ReuseTokenSource` and a fresh assertion is signed and exchanged as it nears expiry; a response with no `expires_in` is given a 15-minute lifetime (`DefaultUpstreamAccessTokenLifetime`) instead of the unbounded one `golang.org/x/oauth2` would infer, and a `refresh_token` in the response is dropped. `client_credentials` obtains its token under the same rule: both grants wrap their token source in one bounded-expiry stamp inside one `ReuseTokenSource`, so an upstream that omits `expires_in` is re-asked every 15 minutes rather than having its first access token presented for the life of the process (#1737). +The exchange POSTs `grant_type=urn:ietf:params:oauth:grant-type:jwt-bearer` and `assertion=` to `oauth_token_url`. A token from `client_credentials` or `jwt_bearer` whose response states no `expires_in` is reused for `oauth_token_lifetime` (duration string or seconds, default 15m, #1737/#2218); a stated `expires_in` always wins. The portal's api and graphql connection editors set it as **Token lifetime** under the OAuth block ("Used when the token endpoint states no expiry"); an empty field stores nothing (#2236). `oauth_scope_placement` (#2061) says where `oauth_scope` goes: `param` (default) as the `scope` form parameter, `claim` as a `scope` claim in the signed assertion and not on the form (what Google reads), or `both`. `oauth_client_id` and `oauth_client_secret` are optional client authentication placed by `oauth_endpoint_auth_style`; with no secret, a client id goes as a form parameter and no Basic header is sent (an empty Basic credential is one an upstream may refuse). The request uses the same bounded, redirect-refusing, CA-bundle-honoring client as `client_credentials`. The access token is cached by `oauth2.ReuseTokenSource` and a fresh assertion is signed and exchanged as it nears expiry; a response with no `expires_in` is given a 15-minute lifetime (`DefaultUpstreamAccessTokenLifetime`) instead of the unbounded one `golang.org/x/oauth2` would infer, and a `refresh_token` in the response is dropped. `client_credentials` obtains its token under the same rule: both grants wrap their token source in one bounded-expiry stamp inside one `ReuseTokenSource`, so an upstream that omits `expires_in` is re-asked every 15 minutes rather than having its first access token presented for the life of the process (#1737). Save-time refusals name the key: no `oauth_token_url`; neither `jwt_issuer` nor `jwt_subject`; an `oauth_scope_placement` other than param, claim or both; key material that does not match `jwt_algorithm` or is not a usable key; `oauth_client_secret` without `oauth_client_id`; an unknown `oauth_endpoint_auth_style`; the `signed_jwt` lifetime and skew rules. `jwt_token_lifetime` is the assertion's lifetime; providers commonly refuse more than a few minutes. An unknown grant is refused as `unknown oauth_grant "..." (want authorization_code, client_credentials or jwt_bearer)`. @@ -4397,7 +4397,7 @@ A Google service account key file is a first-class credential on every HTTP-base `google_service_account_secret` names a stored secret (#2051) holding the file instead, so the private key never enters a tool call or the connection's config. It is read at every token exchange through `secretstore.Store.ConnectionValue`, which requires the connection in the secret's `allow_connections` and refuses a secret with `allow_personas` (the cached token serves every persona on the connection); the kinds bind it per connection with `upstreamauth.BindKeySecrets` (`apigateway.Toolkit.SetSecrets`, `graphql.Toolkit.SetConnectionSecrets`, both attached by `apigwwiring.Secrets`). The token endpoint defaults to `https://oauth2.googleapis.com/token`. Setting both keys is refused. -The connection test (`POST /api/v1/admin/connection-instances/{kind}/{name}/test`) prefixes its detail with `upstreamauth.GrantDetail`: the identity the token was issued for and the scopes the token response reports (or those requested). Google's token refusals carry a hint after the description (`invalid_scope`: check `oauth_scope`; `invalid_grant` signature, not found or disabled: create a new key or check the account; `invalid_grant` short-lived: clock skew), and on an `api` connection a `401`/`403` Google API error body sets the call's `hint` (`SERVICE_DISABLED`: enable the API in the account's project; `PERMISSION_DENIED` or "cannot get profile": grant the account's email inside the product). `steerToExport` now clears only a cut body's hint when api_export is absent, so a decode note or this hint on a whole body survives. The portal editor offers "Google service account" in the auth mode select (`GoogleServiceAccountFields`): Key (Key file upload or Stored secret), Account/Project/Key ID read off the file, Scopes as a list of full URLs, and Delegated user (`jwt_subject`). +The connection test (`POST /api/v1/admin/connection-instances/{kind}/{name}/test`) prefixes its detail with `upstreamauth.GrantDetail`: the identity the token was issued for and the scopes the token response reports (or those requested). Google's token refusals carry a hint after the description (`invalid_scope`: check `oauth_scope`; `invalid_grant` signature, not found or disabled: create a new key or check the account; `invalid_grant` short-lived: clock skew), and on an `api` connection a `401`/`403` Google API error body sets the call's `hint` by its `status` and `details[].reason`, never a product's message text (`SERVICE_DISABLED`: enable the API in the account's project; `ACCESS_TOKEN_SCOPE_INSUFFICIENT`: add the scope to the connection's `oauth_scope`; any other `401` or `403`: grant the account's email inside the product). `steerToExport` now clears only a cut body's hint when api_export is absent, so a decode note or this hint on a whole body survives. The portal editor offers "Google service account" in the auth mode select (`GoogleServiceAccountFields`): Key (Key file upload or Stored secret), Account/Project/Key ID read off the file, Scopes as a list of full URLs, and Delegated user (`jwt_subject`). The admin editor offers the grant on the api and graphql forms (`OAuthFields` `jwtBearer`), rendering the Signed JWT block in its `jwt_bearer` variant. The OAuth status card, with Connect and Refresh now, renders only for the `authorization_code` grant, since the status route answers 409 for every other grant. diff --git a/docs/memory/overview.md b/docs/memory/overview.md index 082b80cc6..edf6de285 100644 --- a/docs/memory/overview.md +++ b/docs/memory/overview.md @@ -103,7 +103,7 @@ Reading memory back is served by the universal `search` tool, which federates me #### Hybrid ranking -The `semantic` strategy fuses two signals per record: the embedding cosine similarity and a lexical full-text match flag, blended as `0.6 * semantic + 0.4 * lexical`. This mirrors the api-gateway ranking precedent and materially improves recall on identifier-heavy content (entity URNs, column names, error codes) where pure vector search underweights an exact token. The vector arm is backed by an `hnsw` ANN index on `memory_records.embedding`; the lexical arm by a GIN index on `to_tsvector('english', content)`. +The `semantic` strategy fuses two signals per record: the embedding cosine similarity and a lexical full-text match flag, blended as `0.6 * semantic + 0.4 * lexical`. This mirrors the api-gateway ranking precedent and materially improves recall on identifier-heavy content (entity URNs, column names, error codes) where pure vector search underweights an exact token. The vector arm is backed by an `hnsw` ANN index on `memory_records.embedding`; the lexical arm by a GIN index on `to_tsvector('english', fts_name_words(content))`. `fts_name_words` adds the text again with `-`, `_`, `.`, `/` and `:` read as spaces, so an identifier such as `orders-2024` matches "orders 2024" as well as itself (#2230). The same reading applies to the names, titles, paths, file names, tags and categories every other surface's full-text search matches: resources, assets and collections, prompts, knowledge pages, managed scripts and catalog datasets. #### Graceful degradation diff --git a/docs/portal/admin-connections.md b/docs/portal/admin-connections.md index 3298182d9..7e7485eed 100644 --- a/docs/portal/admin-connections.md +++ b/docs/portal/admin-connections.md @@ -128,6 +128,10 @@ auth mode reveals it: default) or `params`, which some identity providers require. Shown for the HTTP-based kinds. - **OIDC prompt** (`oauth_prompt`) — `authorization_code` only. +- **Token lifetime** (`oauth_token_lifetime`) — `api` and `graphql`: how long a + token is reused when the token endpoint's response states no `expires_in`, a + duration (`30m`) or seconds. Empty stores nothing, and the default of 15 + minutes applies; a negative value is refused. - **Signed assertion (RFC 7523)** — `jwt_bearer` only: the Signed JWT block below, with the algorithm defaulting to `RS256`, the audience to the token URL, and **Issuer** and **Subject** both required. The issuer is usually the @@ -140,6 +144,8 @@ stored, and the other grants store none. ![OAuth 2.1 credentials](../images/screenshots/light/admin-admin-connection-oauth-light.webp#only-light)![OAuth 2.1 credentials](../images/screenshots/dark/admin-admin-connection-oauth-dark.webp#only-dark) +![Token lifetime](../images/screenshots/light/admin-admin-connection-token-lifetime-light.webp#only-light)![Token lifetime](../images/screenshots/dark/admin-admin-connection-token-lifetime-dark.webp#only-dark) + An `api` connection authored before these keys unified across the kinds may still carry the earlier `oauth2_*` spelling with the grant encoded in the auth mode (`oauth2_authorization_code`). The platform reads both, preferring the diff --git a/docs/requirements.txt b/docs/requirements.txt index 0cf09e6eb..3ec1fbf3e 100644 --- a/docs/requirements.txt +++ b/docs/requirements.txt @@ -16,9 +16,9 @@ backrefs==7.0 \ --hash=sha256:ca42ce6a49ace3d75684dfa9937f3373902a63284ecb385ce36d15e5dcb41c12 \ --hash=sha256:f2c52955d631b9e1ac4cd56209f0a3a946d592b98e7790e77699339ae01c102a # via mkdocs-material -certifi==2026.6.17 \ - --hash=sha256:024c88eeec92ca068db80f02b8b07c9cef7b9fe261d1d535abfd5abd6f6af432 \ - --hash=sha256:2227dcbaafe0d2f59279d1762ddddc37783ed4354594f194ffc31d20f41fc3db +certifi==2026.10.19 \ + --hash=sha256:035c1e85572b141bf1c3a3eff0e33a8eb74917522f2559075ff8d2fea780a1ae \ + --hash=sha256:42d35340cc5b9ba78e818ddbff0c5b1c2b779de448a340fd1b989f5fa8670e1b # via requests charset-normalizer==3.4.9 \ --hash=sha256:0327fcd59a935777d83410750c50600ee9571af2846f71ce40f25b13da1ef380 \ diff --git a/docs/server/api-gateway.md b/docs/server/api-gateway.md index 6fe85a1db..197f3ca9b 100644 --- a/docs/server/api-gateway.md +++ b/docs/server/api-gateway.md @@ -620,9 +620,10 @@ The save is refused, naming the field and never a value, when the file is not JS | Token `400 invalid_grant` "Invalid JWT Signature" or "account not found" | The key was deleted or disabled, or the account was removed | create a new key, or check the account still exists | | Token `400 invalid_grant` "Token must be a short-lived token" | Clock skew | check the host clock and `jwt_issued_at_skew` | | API `403` with reason `SERVICE_DISABLED` | The API is not enabled in the account's Cloud project | enable it there | -| API `403 PERMISSION_DENIED`, or `401` "cannot get profile" | The token is valid but the account has no access in the product | grant the account's email access inside the product | +| API `403` with reason `ACCESS_TOKEN_SCOPE_INSUFFICIENT` | The token does not carry the scope this API needs | add the scope to the connection's `oauth_scope` | +| Any other API `401` or `403` in Google's error shape (`PERMISSION_DENIED`, `UNAUTHENTICATED`, ...) | The token is valid but the account has no access in the product | grant the account's email access inside the product | -A token refusal raises the connection's alert like any `jwt_bearer` refusal. The two API refusals arrive on a data call rather than at the token endpoint, so they are the call's `hint` on an `api` connection, not an authentication failure. +A token refusal raises the connection's alert like any `jwt_bearer` refusal. The API refusals arrive on a data call rather than at the token endpoint, so they are the call's `hint` on an `api` connection, not an authentication failure. Pick `signed_jwt` when the upstream validates the client-minted JWT itself, and `jwt_bearer` when it exchanges the JWT for a token of its own; [Signed JWT upstreams](signed-jwt-auth.md#signed_jwt-or-the-jwt_bearer-grant) compares the two. diff --git a/docs/server/configuration.md b/docs/server/configuration.md index 3f46c9b14..f1d90385f 100644 --- a/docs/server/configuration.md +++ b/docs/server/configuration.md @@ -430,7 +430,7 @@ database: | Field | Type | Default | Description | |-------|------|---------|-------------| | `dsn` | string | - | PostgreSQL connection string | -| `max_open_conns` | int | `25` | Maximum open database connections | +| `max_open_conns` | int | `25` | Maximum open database connections. At least `4`: one for the session every background lock shares, one for a locked pass, and two for requests; a smaller value is refused at startup | !!! note "What the database unlocks" Setting `dsn` enables audit logging, knowledge capture, session externalization, and OAuth persistence. Without it, these features degrade to in-memory or noop implementations. diff --git a/docs/server/deployment.md b/docs/server/deployment.md index 63b148cf9..c8eddaabb 100644 --- a/docs/server/deployment.md +++ b/docs/server/deployment.md @@ -34,9 +34,10 @@ Create a `docker-compose.yml`: ```yaml services: - # PostgreSQL for metadata storage + # PostgreSQL for metadata storage, with pgvector (the platform's migrations + # create the vector extension) postgres: - image: postgres:16-alpine@sha256:acf5271bce6b4b62e352341e3b175c2b1e9e0b6f6e3f2e7e3b7f8c9d0e1f2a3b + image: pgvector/pgvector:pg16@sha256:7b822b0aac60967beb1ea5e576b8602c94c300a157d187f385ae3e0da199b90a environment: POSTGRES_USER: postgres POSTGRES_PASSWORD: ${POSTGRES_PASSWORD:-postgres} @@ -52,15 +53,15 @@ services: # Keycloak for authentication keycloak: - image: quay.io/keycloak/keycloak:24.0@sha256:b3c4a5d6e7f8a9b0c1d2e3f4a5b6c7d8e9f0a1b2c3d4e5f6a7b8c9d0e1f2a3b4 + image: quay.io/keycloak/keycloak:26.7.5@sha256:37dbaf6f0722c9ec246335f36e1ef8b2e6cb960f7c27e0d8c615121a3d475a85 command: start-dev --import-realm environment: KC_DB: postgres KC_DB_URL: jdbc:postgresql://postgres:5432/keycloak KC_DB_USERNAME: postgres KC_DB_PASSWORD: ${POSTGRES_PASSWORD:-postgres} - KEYCLOAK_ADMIN: admin - KEYCLOAK_ADMIN_PASSWORD: ${KEYCLOAK_ADMIN_PASSWORD:-admin} + KC_BOOTSTRAP_ADMIN_USERNAME: admin + KC_BOOTSTRAP_ADMIN_PASSWORD: ${KEYCLOAK_ADMIN_PASSWORD:-admin} volumes: - ./keycloak-realm.json:/opt/keycloak/data/import/realm.json ports: @@ -71,7 +72,7 @@ services: # Trino for SQL queries trino: - image: trinodb/trino:440@sha256:d4e5f6a7b8c9d0e1f2a3b4c5d6e7f8a9b0c1d2e3f4a5b6c7d8e9f0a1b2c3d4e5 + image: trinodb/trino:453@sha256:86f7a156aeeca1dda3a1ba1e97474913d10c75f8fb7cda0da1d29a52b5731e0c ports: - "8081:8080" volumes: @@ -519,7 +520,10 @@ spec: ``` `/readyz` reports `draining` (503) as soon as SIGTERM arrives, so the load -balancer stops routing to a terminating pod before the drain begins; +balancer stops routing to a terminating pod before the drain begins, and +`database_pool_starved` (503) when the replica's database pool has handed out +no connection for 30 seconds +([Observability](observability.md#authentication-configuration-and-platform-state)); `/healthz` stays 200 for as long as the process is alive. Size the probe and resource figures from the [Tuning and Scaling guide](../reference/tuning-and-scaling.md), which covers diff --git a/docs/server/observability.md b/docs/server/observability.md index 8b5143917..3ac61a317 100644 --- a/docs/server/observability.md +++ b/docs/server/observability.md @@ -700,6 +700,13 @@ would take every replica out of the load balancer at once during an upstream outage, including the routes that do not need that upstream. Alert on `dependency_up == 0` instead. +The one exception is the replica's own database pool. When every connection +of it has been in use, with none handed out within half a second of being +asked, for 30 seconds, `/readyz` answers 503 `database_pool_starved` and logs +the `database pool starved` WARN: the replica cannot serve anything that reads +the database, and another replica may. A database that is down is not this: +its pool opens no connections, and the replica stays ready (#2169). + **Connections and personas.** `mcp_platform_connections{kind, state}` counts the connections of every kind that lists them by the state the last call through each one found it in. Nothing is sent to an upstream to find diff --git a/docs/server/storage.md b/docs/server/storage.md index 52a0b9959..e0fd77538 100644 --- a/docs/server/storage.md +++ b/docs/server/storage.md @@ -19,16 +19,19 @@ A store with no capacity is listed with its usage and growth and no fill date; i ## Growth and when a store fills -One replica at a time samples every store every 15 minutes +One replica samples every store every 15 minutes (`MCP_PLATFORM_STORAGE_HISTORY_INTERVAL` overrides it) and keeps each sample -for the settings' **History days** (default 180). Growth is the least-squares -slope of the samples over the last 7 and the last 30 days, in bytes per day. -The 7-day rate waits for a day of history and the 30-day rate for a week: a -new install's first hours are setup (migrations, the built-in knowledge pages -and their embeddings, the first catalog scan), not growth, and a 30-day rate -over the same few hours as the 7-day one would only repeat it. Until then the -page says **Not enough history** with how much there is and how much the rate -needs. A store holding one size grows at exactly zero. The fill date uses the +for the settings' **History days** (default 180). Every replica runs the loop, +and one that finds a sample taken within the interval takes none of its own, +so each tick has one sample. Growth is the least-squares slope of the samples +over the last 7 and the last 30 days, in bytes per day. A store's first day is +left out of every fit: a new install's first hours are setup (migrations, the +built-in knowledge pages and their embeddings, seeding, the first catalog +scan), not growth. After it, the 7-day rate waits for a day of history and the +30-day rate for two weeks, so the 30-day rate is read from at least twice the +history the 7-day one is and never repeats it. Until then the page says **Not +enough history** with how much there is and how much the rate needs, the first +day included. A store holding one size grows at exactly zero. The fill date uses the slower of the two that are growing, so a burst does not bring the date in, and the page says which it used. A store that is not growing has no fill date. diff --git a/internal/advisorylock/advisorylock.go b/internal/advisorylock/advisorylock.go new file mode 100644 index 000000000..fb281e520 --- /dev/null +++ b/internal/advisorylock/advisorylock.go @@ -0,0 +1,242 @@ +// Package advisorylock holds the platform's PostgreSQL session advisory locks +// on one database session per process (#2169). +// +// An advisory lock belongs to the session that took it, so a lock held for a +// background pass used to pin a pooled connection of its own for the whole +// pass, while the pass drew its queries from the same pool. Each locked loop +// added one more pinned connection. With four loops locked on a pool of five, +// the passes waited on the pool for their own queries, never finished, never +// released their locks, and starved every request on the replica. +// +// PostgreSQL lets one session hold any number of advisory locks, so every +// try-lock the process takes goes onto one Session: however many loops hold +// a lock, together they pin one connection, and the session goes back to the +// pool when the last lock is released. The pass itself runs on the pool, never +// on the lock's session. +// +// A session lock is reentrant in PostgreSQL: the session holding a key is +// granted it again. A Session therefore refuses a key the process already +// holds, so two goroutines of one process exclude each other as two replicas +// do. +package advisorylock + +import ( + "context" + "database/sql" + "database/sql/driver" + "errors" + "fmt" + "log/slog" + "sync" + "time" + + "github.com/txn2/mcp-data-platform/internal/logsan" +) + +// statementTimeout bounds one lock or unlock statement. It runs on a context +// detached from the caller's, since a statement canceled mid-flight can end +// the session and with it every lock the process holds. +const statementTimeout = 5 * time.Second + +// Key is one advisory lock: a single bigint, or the two-integer form, which +// PostgreSQL keeps in a key space of its own. +type Key struct { + id int64 + space int32 + n int32 + pair bool +} + +// ID is the lock keyed by one bigint. +func ID(id int64) Key { return Key{id: id} } + +// Pair is the lock keyed by two integers. +func Pair(space, n int32) Key { return Key{space: space, n: n, pair: true} } + +// statement is the query taking (or, with unlock, releasing) k. +func (k Key) statement(unlock bool) (query string, args []any) { + fn := "pg_try_advisory_lock" + if unlock { + fn = "pg_advisory_unlock" + } + if k.pair { + return "SELECT " + fn + "($1, $2)", []any{k.space, k.n} + } + return "SELECT " + fn + "($1)", []any{k.id} +} + +// Session is the one database session a process takes its advisory locks on. +// It is safe for concurrent use. +type Session struct { + db *sql.DB + + mu sync.Mutex + // conn is the session, nil while no lock is held. + conn *sql.Conn + // gen counts the sessions opened; a release from an earlier one is a lock + // that ended with its session. + gen uint64 + held map[Key]struct{} +} + +// New builds a Session over db. Production code uses For, which keeps one +// per database handle; New is for a test that stands two replicas on one. +func New(db *sql.DB) *Session { + return &Session{db: db, held: map[Key]struct{}{}} +} + +//nolint:gochecknoglobals // one lock session per database handle in the process, which is the point +var sessions sync.Map + +// For is the process's lock session over db. +func For(db *sql.DB) *Session { + if s, ok := sessions.Load(db); ok { + return s.(*Session) //nolint:forcetypeassert,errcheck // only *Session is stored + } + s, _ := sessions.LoadOrStore(db, New(db)) + return s.(*Session) //nolint:forcetypeassert,errcheck // only *Session is stored +} + +// Try takes key on db's lock session; see Session.Try. +func Try(ctx context.Context, db *sql.DB, key Key) (release func(), ok bool, err error) { + return For(db).Try(ctx, key) +} + +// Try takes key without waiting. ok is false when another session holds it +// or this process already does. release gives it back and is safe to call +// more than once; it does nothing when the session ended in the meantime, +// since that released the lock. +func (s *Session) Try(ctx context.Context, key Key) (release func(), ok bool, err error) { + if err := ctx.Err(); err != nil { + return nil, false, err //nolint:wrapcheck // the caller's own context + } + s.mu.Lock() + defer s.mu.Unlock() + if err := s.open(ctx); err != nil { + return nil, false, err + } + if _, mine := s.held[key]; mine { + s.closeIfIdle() + return nil, false, nil + } + got, err := s.take(ctx, key) + if err != nil { + // A statement that failed took nothing. A session whose connection + // broke has lost every lock on it, and is ended so the next take + // opens another rather than failing on it until its holders finish. + if errors.Is(err, driver.ErrBadConn) || errors.Is(err, sql.ErrConnDone) { + s.lose(ctx, "the lock session's connection broke") + } + s.closeIfIdle() + return nil, false, fmt.Errorf("taking the advisory lock: %w", err) + } + if !got { + s.closeIfIdle() + return nil, false, nil + } + s.held[key] = struct{}{} + gen := s.gen + var once sync.Once + return func() { once.Do(func() { s.release(ctx, key, gen) }) }, true, nil +} + +// Held reports whether any session, this process's included, holds key. +func (s *Session) Held(ctx context.Context, key Key) (bool, error) { + s.mu.Lock() + _, mine := s.held[key] + s.mu.Unlock() + if mine { + return true, nil + } + release, ok, err := s.Try(ctx, key) + if err != nil || !ok { + return err == nil, err + } + release() + return false, nil +} + +// open takes the session from the pool when none is open. It is called with +// mu held and gives mu up while it waits on the pool, so a release is never +// held behind a wait for a connection. +func (s *Session) open(ctx context.Context) error { + if s.conn != nil { + return nil + } + s.mu.Unlock() + conn, err := s.db.Conn(ctx) + s.mu.Lock() + if err != nil { + return fmt.Errorf("acquiring the lock session: %w", err) + } + if s.conn != nil { + _ = conn.Close() + return nil + } + s.conn = conn + s.gen++ + return nil +} + +// take runs key's try-lock statement on the session. +func (s *Session) take(ctx context.Context, key Key) (bool, error) { + stmtCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), statementTimeout) + defer cancel() + q, args := key.statement(false) + var got bool + err := s.conn.QueryRowContext(stmtCtx, q, args...).Scan(&got) + return got, err //nolint:wrapcheck // the caller names the statement +} + +// unlock runs key's unlock statement on the session. +func (s *Session) unlock(ctx context.Context, key Key) error { + stmtCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), statementTimeout) + defer cancel() + q, args := key.statement(true) + _, err := s.conn.ExecContext(stmtCtx, q, args...) + return err //nolint:wrapcheck // the caller names the statement +} + +// release gives key back, unless the session it was taken on has ended. +func (s *Session) release(ctx context.Context, key Key, gen uint64) { + s.mu.Lock() + defer s.mu.Unlock() + if gen != s.gen || s.conn == nil { + return + } + if _, mine := s.held[key]; !mine { + return + } + delete(s.held, key) + if err := s.unlock(ctx, key); err != nil { + // The session may still hold the lock; ending it is what releases it. + slog.WarnContext(ctx, "advisory unlock failed; ending the lock session", + "error", logsan.SanitizeForLog(err.Error())) + s.lose(ctx, "an unlock failed") + return + } + s.closeIfIdle() +} + +// closeIfIdle returns the session to the pool when it holds no lock. +func (s *Session) closeIfIdle() { + if len(s.held) == 0 && s.conn != nil { + _ = s.conn.Close() + s.conn = nil + } +} + +// lose ends the session, which releases every lock on it, and drops it from +// the pool. A lock held when it ends is reported, since its holder still +// runs. +func (s *Session) lose(ctx context.Context, why string) { + if n := len(s.held); n > 0 { + slog.WarnContext(ctx, "advisory lock session ended with locks held", "reason", why, "locks", n) + } + if s.conn != nil { + _ = s.conn.Raw(func(any) error { return driver.ErrBadConn }) + _ = s.conn.Close() + s.conn = nil + } + s.held = map[Key]struct{}{} +} diff --git a/internal/advisorylock/advisorylock_realdb_integration_test.go b/internal/advisorylock/advisorylock_realdb_integration_test.go new file mode 100644 index 000000000..517844b33 --- /dev/null +++ b/internal/advisorylock/advisorylock_realdb_integration_test.go @@ -0,0 +1,81 @@ +//go:build integration + +package advisorylock + +import ( + "context" + "database/sql" + "sync" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/txn2/mcp-data-platform/internal/testdb" +) + +// The production keys of the loops that held a lock at once on the replica +// #2169 reported, and the others that take one, more locks than the pool has +// connections. +var loopKeys = []int64{ + 4713210201, // catalog scan + 4713210301, // maps + 4713210402, // storage capacity alerts + 4713210403, // webhook receiver alerts + 4713210401, // capacity listing + 4713210001, // call catalog sweep +} + +// TestRealDB_LockedPassesFinishOnASmallPool is #2169: a pool of five, six +// passes each holding its lock while it runs its statements, and a request on +// the side. With a session per lock, five locks held every connection, the +// sixth waited for one, and no pass could run a statement; on the shared +// session every pass finishes and the request is served while they run. +func TestRealDB_LockedPassesFinishOnASmallPool(t *testing.T) { + db := testdb.New(t) + db.SetMaxOpenConns(5) + s := New(db) + baseline := db.Stats().InUse // what the harness itself holds + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + + var ( + holding sync.WaitGroup + done sync.WaitGroup + ) + allHeld := make(chan struct{}) + holding.Add(len(loopKeys)) + for _, key := range loopKeys { + done.Go(func() { + release, ok, err := s.Try(ctx, ID(key)) + holding.Done() + if !assert.NoError(t, err) || !assert.True(t, ok, "key %d", key) { + return + } + defer release() + <-allHeld + assert.NoError(t, pass(ctx, db), "key %d", key) + }) + } + holding.Wait() + assert.Equal(t, 1, db.Stats().InUse-baseline, "six locks held on one connection") + close(allHeld) + + var answer int + require.NoError(t, db.QueryRowContext(ctx, "SELECT 1").Scan(&answer), "a request while every pass runs") + done.Wait() + require.NoError(t, ctx.Err(), "the passes finished") + assert.Equal(t, 0, db.Stats().InUse-baseline, "the session went back to the pool with the last lock") +} + +// pass runs a few statements one after another, each on a connection from +// the pool. +func pass(ctx context.Context, db *sql.DB) error { + for range 3 { + if _, err := db.ExecContext(ctx, "SELECT pg_sleep(0.05)"); err != nil { + return err + } + } + return nil +} diff --git a/internal/advisorylock/advisorylock_test.go b/internal/advisorylock/advisorylock_test.go new file mode 100644 index 000000000..dc5dc181d --- /dev/null +++ b/internal/advisorylock/advisorylock_test.go @@ -0,0 +1,218 @@ +package advisorylock + +import ( + "context" + "database/sql/driver" + "errors" + "regexp" + "testing" + + "github.com/DATA-DOG/go-sqlmock" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +var ( + lockQ = regexp.QuoteMeta("SELECT pg_try_advisory_lock($1)") + unlockQ = regexp.QuoteMeta("SELECT pg_advisory_unlock($1)") + pairLockQ = regexp.QuoteMeta("SELECT pg_try_advisory_lock($1, $2)") + pairFreeQ = regexp.QuoteMeta("SELECT pg_advisory_unlock($1, $2)") + errDown = errors.New("down") +) + +func newMock(t *testing.T) (*Session, sqlmock.Sqlmock) { + t.Helper() + db, mock, err := sqlmock.New() + require.NoError(t, err) + t.Cleanup(func() { _ = db.Close() }) + return New(db), mock +} + +func granted(got bool) *sqlmock.Rows { return sqlmock.NewRows([]string{"l"}).AddRow(got) } + +// Two locks share the session, which goes back to the pool when the last is +// released, and a release is safe to call twice. +func TestTry_TakesAndReleases(t *testing.T) { + s, mock := newMock(t) + ctx := context.Background() + mock.ExpectQuery(lockQ).WithArgs(int64(1)).WillReturnRows(granted(true)) + mock.ExpectQuery(lockQ).WithArgs(int64(2)).WillReturnRows(granted(true)) + mock.ExpectExec(unlockQ).WithArgs(int64(1)).WillReturnResult(sqlmock.NewResult(0, 0)) + mock.ExpectExec(unlockQ).WithArgs(int64(2)).WillReturnResult(sqlmock.NewResult(0, 0)) + + first, ok, err := s.Try(ctx, ID(1)) + require.NoError(t, err) + require.True(t, ok) + second, ok, err := s.Try(ctx, ID(2)) + require.NoError(t, err) + require.True(t, ok) + assert.Equal(t, 1, s.db.Stats().InUse, "two locks hold one connection") + + first() + first() + assert.Equal(t, 1, s.db.Stats().InUse, "a lock still held keeps the session") + second() + assert.Equal(t, 0, s.db.Stats().InUse, "the session goes back to the pool with its last lock") + require.NoError(t, mock.ExpectationsWereMet()) +} + +// A key this process holds is refused without asking the database, since +// PostgreSQL would grant the holding session the key again. +func TestTry_RefusesAKeyThisProcessHolds(t *testing.T) { + s, mock := newMock(t) + ctx := context.Background() + mock.ExpectQuery(lockQ).WithArgs(int64(7)).WillReturnRows(granted(true)) + release, ok, err := s.Try(ctx, ID(7)) + require.NoError(t, err) + require.True(t, ok) + + _, ok, err = s.Try(ctx, ID(7)) + require.NoError(t, err) + assert.False(t, ok, "a second holder in one process") + + mock.ExpectExec(unlockQ).WithArgs(int64(7)).WillReturnResult(sqlmock.NewResult(0, 0)) + release() + require.NoError(t, mock.ExpectationsWereMet()) +} + +// A key another session holds is refused, and the session is not kept for it. +func TestTry_AnotherSessionHoldsIt(t *testing.T) { + s, mock := newMock(t) + mock.ExpectQuery(lockQ).WillReturnRows(granted(false)) + _, ok, err := s.Try(context.Background(), ID(3)) + require.NoError(t, err) + assert.False(t, ok) + assert.Equal(t, 0, s.db.Stats().InUse) + require.NoError(t, mock.ExpectationsWereMet()) +} + +// The two-integer form takes and releases its own statements. +func TestTry_PairKey(t *testing.T) { + s, mock := newMock(t) + mock.ExpectQuery(pairLockQ).WithArgs(int32(9), int32(2)).WillReturnRows(granted(true)) + mock.ExpectExec(pairFreeQ).WithArgs(int32(9), int32(2)).WillReturnResult(sqlmock.NewResult(0, 0)) + release, ok, err := s.Try(context.Background(), Pair(9, 2)) + require.NoError(t, err) + require.True(t, ok) + release() + require.NoError(t, mock.ExpectationsWereMet()) +} + +// A failed take is an error and holds nothing; a done context asks nothing; +// a pool that cannot open a session says so. +func TestTry_Failures(t *testing.T) { + s, mock := newMock(t) + mock.ExpectQuery(lockQ).WillReturnError(errDown) + _, ok, err := s.Try(context.Background(), ID(1)) + require.ErrorIs(t, err, errDown) + assert.False(t, ok) + assert.Equal(t, 0, s.db.Stats().InUse) + + done, cancel := context.WithCancel(context.Background()) + cancel() + _, _, err = s.Try(done, ID(1)) + require.ErrorIs(t, err, context.Canceled) + require.NoError(t, mock.ExpectationsWereMet()) + + _ = s.db.Close() + _, _, err = s.Try(context.Background(), ID(1)) + require.ErrorContains(t, err, "acquiring the lock session") +} + +// An unlock that fails ends the session, since a session that may still hold +// the lock must not go back to the pool; a lock that ended with it releases +// as a no-op, and the next take opens a new session. +func TestRelease_AFailedUnlockEndsTheSession(t *testing.T) { + s, mock := newMock(t) + ctx := context.Background() + mock.ExpectQuery(lockQ).WithArgs(int64(1)).WillReturnRows(granted(true)) + mock.ExpectQuery(lockQ).WithArgs(int64(2)).WillReturnRows(granted(true)) + mock.ExpectExec(unlockQ).WithArgs(int64(1)).WillReturnError(errDown) + + first, _, err := s.Try(ctx, ID(1)) + require.NoError(t, err) + second, _, err := s.Try(ctx, ID(2)) + require.NoError(t, err) + gen := s.gen + first() + assert.Nil(t, s.conn, "the session ended") + assert.Empty(t, s.held, "every lock on it ended with it") + second() // no statement: its lock ended with the session + require.NoError(t, mock.ExpectationsWereMet()) + assert.Equal(t, gen, s.gen, "nothing reopened the session") +} + +// Held answers from the process for a key it holds, and otherwise asks the +// database by taking the key and giving it straight back. +func TestHeld(t *testing.T) { + s, mock := newMock(t) + ctx := context.Background() + + mock.ExpectQuery(lockQ).WithArgs(int64(5)).WillReturnRows(granted(true)) + mock.ExpectExec(unlockQ).WithArgs(int64(5)).WillReturnResult(sqlmock.NewResult(0, 0)) + held, err := s.Held(ctx, ID(5)) + require.NoError(t, err) + assert.False(t, held, "free") + + mock.ExpectQuery(lockQ).WithArgs(int64(5)).WillReturnRows(granted(false)) + held, err = s.Held(ctx, ID(5)) + require.NoError(t, err) + assert.True(t, held, "another replica holds it") + + mock.ExpectQuery(lockQ).WithArgs(int64(5)).WillReturnRows(granted(true)) + release, _, err := s.Try(ctx, ID(5)) + require.NoError(t, err) + held, err = s.Held(ctx, ID(5)) + require.NoError(t, err) + assert.True(t, held, "this process holds it") + + mock.ExpectExec(unlockQ).WithArgs(int64(5)).WillReturnResult(sqlmock.NewResult(0, 0)) + release() + + mock.ExpectQuery(lockQ).WillReturnError(errDown) + _, err = s.Held(ctx, ID(5)) + require.ErrorIs(t, err, errDown) + require.NoError(t, mock.ExpectationsWereMet()) +} + +// For keeps one session per database handle, and Try takes on it. +func TestFor_OneSessionPerHandle(t *testing.T) { + db, mock, err := sqlmock.New() + require.NoError(t, err) + t.Cleanup(func() { _ = db.Close() }) + other, _, err := sqlmock.New() + require.NoError(t, err) + t.Cleanup(func() { _ = other.Close() }) + + assert.Same(t, For(db), For(db)) + assert.NotSame(t, For(db), For(other)) + + mock.ExpectQuery(lockQ).WithArgs(int64(4)).WillReturnRows(granted(true)) + mock.ExpectExec(unlockQ).WithArgs(int64(4)).WillReturnResult(sqlmock.NewResult(0, 0)) + release, ok, err := Try(context.Background(), db, ID(4)) + require.NoError(t, err) + require.True(t, ok) + _, ok, err = For(db).Try(context.Background(), ID(4)) + require.NoError(t, err) + assert.False(t, ok, "Try and For share the session") + release() + require.NoError(t, mock.ExpectationsWereMet()) +} + +// A session whose connection broke is ended: the locks on it are gone, a +// release of one asks nothing, and the next take opens a new session. +func TestTry_ABrokenSessionIsEnded(t *testing.T) { + s, mock := newMock(t) + ctx := context.Background() + mock.ExpectQuery(lockQ).WithArgs(int64(1)).WillReturnRows(granted(true)) + held, _, err := s.Try(ctx, ID(1)) + require.NoError(t, err) + + mock.ExpectQuery(lockQ).WithArgs(int64(2)).WillReturnError(driver.ErrBadConn) + _, _, err = s.Try(ctx, ID(2)) + require.ErrorIs(t, err, driver.ErrBadConn) + assert.Nil(t, s.conn, "the broken session was ended") + assert.Empty(t, s.held, "its locks went with it") + held() // no statement: the lock ended with the session + require.NoError(t, mock.ExpectationsWereMet()) +} diff --git a/internal/advisorylock/loops_realdb_integration_test.go b/internal/advisorylock/loops_realdb_integration_test.go new file mode 100644 index 000000000..06ce4a2b8 --- /dev/null +++ b/internal/advisorylock/loops_realdb_integration_test.go @@ -0,0 +1,73 @@ +//go:build integration + +package advisorylock_test + +import ( + "context" + "sync" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/txn2/mcp-data-platform/internal/connlimit" + "github.com/txn2/mcp-data-platform/internal/dbobs" + "github.com/txn2/mcp-data-platform/internal/platform/levelalert" + "github.com/txn2/mcp-data-platform/internal/platform/storagecap" + "github.com/txn2/mcp-data-platform/internal/testdb" + "github.com/txn2/mcp-data-platform/internal/webhook/whalert" + "github.com/txn2/mcp-data-platform/pkg/connoauth" +) + +// TestRealDB_TheLoopsShareOneConnection holds, all at once and on a pool of +// the platform's minimum, the locks of the two loops v1.147.0 added (storage +// capacity and webhook alerts, through levelalert.Locked), two connlimit +// slots and a connoauth refresh lock, and serves a request while they are +// held: between them they hold one connection (#2169). +func TestRealDB_TheLoopsShareOneConnection(t *testing.T) { + db := testdb.New(t) + db.SetMaxOpenConns(dbobs.MinOpenConns) + baseline := db.Stats().InUse + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + + release := make(chan struct{}) + var held, done sync.WaitGroup + for _, key := range []int64{storagecap.LockKey, whalert.LockKey} { + held.Add(1) + done.Go(func() { + err := levelalert.Locked(ctx, db, key, func(ctx context.Context) error { + held.Done() + <-release + var n int + return db.QueryRowContext(ctx, "SELECT count(*) FROM alert_conditions").Scan(&n) + }) + assert.NoError(t, err, "key %d", key) + }) + } + limiter := connlimit.New(db, nil) + var slots []*connlimit.Slot + for range 2 { + slot, _, err := limiter.Acquire(ctx, "api", "crm", 2) + require.NoError(t, err) + slots = append(slots, slot) + } + unlock, ok, err := connoauth.NewPostgresLocker(db).TryLock(ctx, connoauth.Key{Kind: connoauth.KindAPI, Name: "crm"}) + require.NoError(t, err) + require.True(t, ok) + held.Wait() + + assert.Equal(t, 1, db.Stats().InUse-baseline, "two loop locks, two slots and a refresh lock on one connection") + var answer int + require.NoError(t, db.QueryRowContext(ctx, "SELECT 1").Scan(&answer), "a request while they are held") + + close(release) + done.Wait() + for _, s := range slots { + s.Release() + } + unlock() + require.NoError(t, ctx.Err()) + assert.Equal(t, 0, db.Stats().InUse-baseline, "the session went back to the pool") +} diff --git a/internal/connlimit/connlimit.go b/internal/connlimit/connlimit.go index d4e481910..2348ab486 100644 --- a/internal/connlimit/connlimit.go +++ b/internal/connlimit/connlimit.go @@ -10,9 +10,9 @@ // export's page walk, a script's platform.call. // // A slot is a PostgreSQL session-level advisory lock, (connection, slot n), -// held on a pooled database connection for the length of the call, so the -// limit holds across replicas: an upstream that takes one request does not -// take three because the deployment runs three replicas. A call over the +// held on the process's lock session (internal/advisorylock) for the length +// of the call, so the limit holds across replicas: an upstream that takes one +// request does not take three because the deployment runs three replicas. A call over the // limit waits for a slot instead of failing, and the wait counts against the // call's own deadline; a call whose deadline passes while it waits fails // naming the limit and how long it waited. A deployment with no database @@ -22,7 +22,6 @@ package connlimit import ( "context" "database/sql" - "database/sql/driver" "errors" "fmt" "hash/fnv" @@ -30,6 +29,7 @@ import ( "sync/atomic" "time" + "github.com/txn2/mcp-data-platform/internal/advisorylock" "github.com/txn2/mcp-data-platform/pkg/observability" ) @@ -39,9 +39,6 @@ import ( const ( firstPoll = 20 * time.Millisecond maxPoll = 250 * time.Millisecond - // releaseTimeout bounds giving a slot back, which runs after the call's - // own context may have ended. - releaseTimeout = 5 * time.Second ) // keySpace is the first half of every slot's advisory-lock key. It is @@ -134,8 +131,7 @@ func (l *Limiter) Acquire(ctx context.Context, kind, connection string, limit in } // acquireShared takes a slot as an advisory lock, polling until one is free. -// A database connection is held only while a slot is: a waiting call holds -// none. +// A waiting call holds no database connection. func (l *Limiter) acquireShared(ctx context.Context, kind, connection string, limit int) (*Slot, error) { space := keySpace(kind, connection) // One request's wait for a slot, bounded by the request's own context: @@ -164,52 +160,26 @@ func pause(ctx context.Context, d time.Duration) error { } // tryShared takes the first free slot, or reports none free with a nil slot. +// Slots are held on the process's lock session, so however many calls are in +// flight they pin one database connection between them (#2169). func (l *Limiter) tryShared(ctx context.Context, space int32, limit int) (*Slot, error) { - conn, err := l.db.Conn(ctx) - if err != nil { - return nil, fmt.Errorf("reserving a database connection for the slot: %w", err) - } for n := int32(0); int(n) < limit; n++ { - var got bool - if err := conn.QueryRowContext(ctx, `SELECT pg_try_advisory_lock($1, $2)`, space, n).Scan(&got); err != nil { - discard(conn) - // A deadline reached during the query is the driver's cancel - // error; the call's own end is what the caller is told. + release, got, err := advisorylock.Try(ctx, l.db, advisorylock.Pair(space, n)) + if err != nil { + // A deadline reached during the take is the call's own end, and + // that is what the caller is told. if ctxErr := ctx.Err(); ctxErr != nil { return nil, ctxErr //nolint:wrapcheck // WaitError names it } return nil, fmt.Errorf("taking a slot: %w", err) } if got { - return &Slot{release: func() { unlockShared(conn, space, n) }}, nil + return &Slot{release: release}, nil } } - _ = conn.Close() return nil, nil //nolint:nilnil // no slot free is not a failure: the caller waits } -// unlockShared gives a slot back and returns its database connection to the -// pool. A connection whose unlock failed may still hold the lock, so it is -// closed rather than pooled: a pooled connection holding a slot would hold it -// until the pool closed it. -func unlockShared(conn *sql.Conn, space, n int32) { - ctx, cancel := context.WithTimeout(context.Background(), releaseTimeout) - defer cancel() - var released bool - if err := conn.QueryRowContext(ctx, `SELECT pg_advisory_unlock($1, $2)`, space, n).Scan(&released); err != nil || !released { - discard(conn) - return - } - _ = conn.Close() -} - -// discard closes conn and drops it from the pool, ending its session and with -// it any lock the session holds. -func discard(conn *sql.Conn) { - _ = conn.Raw(func(any) error { return driver.ErrBadConn }) - _ = conn.Close() -} - // acquireLocal takes a slot from this process's own count of the connection. func (l *Limiter) acquireLocal(ctx context.Context, kind, connection string, limit int) (*Slot, error) { key := fmt.Sprintf("%s\x00%s\x00%d", kind, connection, limit) diff --git a/internal/connlimit/connlimit_realdb_integration_test.go b/internal/connlimit/connlimit_realdb_integration_test.go index ea02a726c..137579510 100644 --- a/internal/connlimit/connlimit_realdb_integration_test.go +++ b/internal/connlimit/connlimit_realdb_integration_test.go @@ -45,8 +45,8 @@ func TestRealDB_TheLimitHoldsAcrossReplicas(t *testing.T) { } // TestRealDB_ASlotIsHeldOnlyWhileItIsHeld: a waiting call holds no database -// connection, a held slot frees on release, and a second slot is a second -// call in flight. +// connection, held slots share one, a held slot frees on release, and a +// second slot is a second call in flight. func TestRealDB_ASlotIsHeldOnlyWhileItIsHeld(t *testing.T) { db := testdb.New(t) a, b := New(db, nil), New(db, nil) @@ -66,8 +66,8 @@ func TestRealDB_ASlotIsHeldOnlyWhileItIsHeld(t *testing.T) { if _, _, err := b.Acquire(short, "api", "billing", 2); !errors.Is(err, context.DeadlineExceeded) { t.Fatalf("a third call took a slot of two: %v", err) } - if open := db.Stats().InUse - baseline; open != 2 { - t.Errorf("%d database connections in use; the two held slots hold one each and the waiter none", open) + if open := db.Stats().InUse - baseline; open != 1 { + t.Errorf("%d database connections in use; the two held slots share the process's lock session (#2169) and the waiter holds none", open) } first.Release() third, _, err := b.Acquire(ctx, "api", "billing", 2) diff --git a/internal/connlimit/shared_test.go b/internal/connlimit/shared_test.go index 931987463..92283f3fa 100644 --- a/internal/connlimit/shared_test.go +++ b/internal/connlimit/shared_test.go @@ -38,7 +38,7 @@ func TestShared_TakesTheFirstFreeSlotAndWaits(t *testing.T) { mock.ExpectQuery(tryLockQ).WithArgs(space, int32(1)).WillReturnRows(lockRow(false)) mock.ExpectQuery(tryLockQ).WithArgs(space, int32(0)).WillReturnRows(lockRow(false)) mock.ExpectQuery(tryLockQ).WithArgs(space, int32(1)).WillReturnRows(lockRow(true)) - mock.ExpectQuery(unlockQ).WithArgs(space, int32(1)).WillReturnRows(lockRow(true)) + mock.ExpectExec(unlockQ).WithArgs(space, int32(1)).WillReturnResult(sqlmock.NewResult(0, 0)) slot, waited, err := l.Acquire(context.Background(), "api", "crm", 2) if err != nil { @@ -83,7 +83,7 @@ func TestShared_DeadlineDuringTheQuery(t *testing.T) { } // A failed lock query fails the call rather than waiting forever, and an -// unlock that fails drops the database connection instead of pooling it. +// unlock that fails gives the slot back all the same. func TestShared_Failures(t *testing.T) { l, mock := sharedLimiter(t) down := errors.New("db down") @@ -96,21 +96,16 @@ func TestShared_Failures(t *testing.T) { t.Error(err) } - for _, unlock := range []func(*sqlmock.ExpectedQuery){ - func(q *sqlmock.ExpectedQuery) { q.WillReturnError(down) }, - func(q *sqlmock.ExpectedQuery) { q.WillReturnRows(lockRow(false)) }, - } { - l, mock := sharedLimiter(t) - mock.ExpectQuery(tryLockQ).WillReturnRows(lockRow(true)) - unlock(mock.ExpectQuery(unlockQ)) - slot, _, err := l.Acquire(context.Background(), "api", "crm", 1) - if err != nil { - t.Fatal(err) - } - slot.Release() - if err := mock.ExpectationsWereMet(); err != nil { - t.Error(err) - } + l, mock = sharedLimiter(t) + mock.ExpectQuery(tryLockQ).WillReturnRows(lockRow(true)) + mock.ExpectExec(unlockQ).WillReturnError(down) + slot, _, err := l.Acquire(context.Background(), "api", "crm", 1) + if err != nil { + t.Fatal(err) + } + slot.Release() + if err := mock.ExpectationsWereMet(); err != nil { + t.Error(err) } // No database connection to reserve: the call fails naming the limit. diff --git a/internal/dbobs/dbobs.go b/internal/dbobs/dbobs.go index f1ac698cd..995aff297 100644 --- a/internal/dbobs/dbobs.go +++ b/internal/dbobs/dbobs.go @@ -138,3 +138,20 @@ func isVectorSearch(query string) bool { } return false } + +// MinOpenConns is the smallest pool the platform runs on (#2169): one +// connection for the session every background lock shares, one for the +// statements of a locked pass, and two for the requests the replica serves +// meanwhile. +const MinOpenConns = 4 + +// CheckPoolSize refuses a database.max_open_conns too small for the platform +// to finish its own background work. Zero takes the default. +func CheckPoolSize(maxOpen int) error { + if maxOpen > 0 && maxOpen < MinOpenConns { + return fmt.Errorf("database.max_open_conns is %d; the platform needs at least %d: "+ + "one for the session its background locks share, one for a locked pass, and two for requests", + maxOpen, MinOpenConns) + } + return nil +} diff --git a/internal/dbobs/dbobs_test.go b/internal/dbobs/dbobs_test.go index 7018c29a9..e6862ad56 100644 --- a/internal/dbobs/dbobs_test.go +++ b/internal/dbobs/dbobs_test.go @@ -211,3 +211,17 @@ func TestOpen(t *testing.T) { require.NoError(t, err) require.NoError(t, db.Close()) } + +// #2169: a pool too small for the platform's own background work is refused +// at startup, naming the minimum; zero takes the default. +func TestCheckPoolSize(t *testing.T) { + for _, n := range []int{0, MinOpenConns, 25} { + if err := CheckPoolSize(n); err != nil { + t.Errorf("max_open_conns %d refused: %v", n, err) + } + } + err := CheckPoolSize(MinOpenConns - 1) + if err == nil || !strings.Contains(err.Error(), "database.max_open_conns is 3; the platform needs at least 4") { + t.Errorf("max_open_conns 3: %v", err) + } +} diff --git a/internal/httpserver/health/health.go b/internal/httpserver/health/health.go index 7f534c33b..fecaf6128 100644 --- a/internal/httpserver/health/health.go +++ b/internal/httpserver/health/health.go @@ -19,6 +19,13 @@ const ( // It is safe for concurrent use. type Checker struct { state atomic.Int32 + pool atomic.Pointer[PoolProbe] +} + +// WatchPool makes a starved database pool take the replica out of service +// while the process is otherwise ready (#2169). +func (c *Checker) WatchPool(p *PoolProbe) { + c.pool.Store(p) } // NewChecker creates a Checker in the Starting state. @@ -67,10 +74,14 @@ func (*Checker) LivenessHandler() http.HandlerFunc { } // ReadinessHandler returns an http.HandlerFunc that responds 200 when ready -// and 503 when starting or draining. +// and 503 when starting, draining, or its database pool is starved. // Use this for K8s readinessProbe (/readyz). func (c *Checker) ReadinessHandler() http.HandlerFunc { - return func(w http.ResponseWriter, _ *http.Request) { + return func(w http.ResponseWriter, r *http.Request) { + if c.IsReady() && c.pool.Load().Starved(r.Context()) { + writeJSON(w, http.StatusServiceUnavailable, healthResponse{Status: statusPoolStarved}) + return + } if c.IsReady() { writeJSON(w, http.StatusOK, healthResponse{Status: c.State()}) return diff --git a/internal/httpserver/health/pool.go b/internal/httpserver/health/pool.go new file mode 100644 index 000000000..eff259ab4 --- /dev/null +++ b/internal/httpserver/health/pool.go @@ -0,0 +1,83 @@ +package health + +import ( + "context" + "database/sql" + "log/slog" + "sync" + "time" +) + +// Pool starvation (#2169). A replica whose database pool hands out no +// connection serves nothing that reads the database, yet the process state +// alone kept /readyz at 200 while one did. A pool every connection of which +// is in use, and stays so, takes the replica out of service. A database that +// is down is not this: its pool opens no connections, and /readyz leaves an +// upstream outage to the dependency probes so it never takes every replica out +// at once. +const ( + // DefaultPoolWait is how long one probe waits for a connection. It is + // under the one second a readiness probe is given by default. + DefaultPoolWait = 500 * time.Millisecond + // DefaultPoolStarvedFor is how long the pool must go without handing one + // out before the replica reports not ready, so a busy pool that frees a + // connection now and then is not taken out of service. + DefaultPoolStarvedFor = 30 * time.Second + statusPoolStarved = "database_pool_starved" +) + +// PoolProbe reports whether a database pool can hand out a connection. +type PoolProbe struct { + db *sql.DB + wait time.Duration + starvedFor time.Duration + now func() time.Time + + mu sync.Mutex + since time.Time // when the pool was first found starved; zero while it is not +} + +// NewPoolProbe probes db, waiting wait for a connection and reporting it +// starved once it has handed none out for starvedFor. +func NewPoolProbe(db *sql.DB, wait, starvedFor time.Duration) *PoolProbe { + return &PoolProbe{db: db, wait: wait, starvedFor: starvedFor, now: time.Now} +} + +// Starved reports whether every connection of the pool has been in use, with +// none handed out on request, for at least starvedFor. +func (p *PoolProbe) Starved(ctx context.Context) bool { + if p == nil || p.db == nil { + return false + } + st := p.db.Stats() + if st.MaxOpenConnections == 0 || st.InUse < st.MaxOpenConnections || p.answers(ctx) { + p.mu.Lock() + p.since = time.Time{} + p.mu.Unlock() + return false + } + now := p.now() + p.mu.Lock() + defer p.mu.Unlock() + if p.since.IsZero() { + p.since = now + } + if now.Sub(p.since) < p.starvedFor { + return false + } + slog.WarnContext(ctx, "database pool starved: every connection in use and none handed out", + "in_use", st.InUse, "max_open_conns", st.MaxOpenConnections, "since", p.since) + return true +} + +// answers reports whether the pool hands out a connection within wait. +func (p *PoolProbe) answers(ctx context.Context) bool { + ctx, cancel := context.WithTimeout(ctx, p.wait) + defer cancel() + conn, err := p.db.Conn(ctx) + if err != nil { + return false + } + _ = conn.Close() + return true +} diff --git a/internal/httpserver/health/pool_test.go b/internal/httpserver/health/pool_test.go new file mode 100644 index 000000000..7c8f3330e --- /dev/null +++ b/internal/httpserver/health/pool_test.go @@ -0,0 +1,120 @@ +package health + +import ( + "context" + "database/sql" + "encoding/json" + "net/http" + "net/http/httptest" + "testing" + "time" + + "github.com/DATA-DOG/go-sqlmock" +) + +// starvedPool is a pool of one whose connection is taken. +func starvedPool(t *testing.T) (*sql.DB, *sql.Conn) { + t.Helper() + db, _, err := sqlmock.New() + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = db.Close() }) + db.SetMaxOpenConns(1) + conn, err := db.Conn(context.Background()) + if err != nil { + t.Fatal(err) + } + return db, conn +} + +func readyz(hc *Checker) (code int, status string) { + rec := httptest.NewRecorder() + hc.ReadinessHandler()(rec, httptest.NewRequestWithContext(context.Background(), http.MethodGet, "/readyz", http.NoBody)) + var body struct{ Status string } + _ = json.Unmarshal(rec.Body.Bytes(), &body) + return rec.Code, body.Status +} + +// #2169: a pool that hands out no connection for longer than starvedFor takes +// a ready replica out of service, and it comes back once a connection frees. +func TestReadiness_AStarvedPoolIsNotReady(t *testing.T) { + db, conn := starvedPool(t) + clock := time.Unix(1_000, 0) + probe := NewPoolProbe(db, 10*time.Millisecond, 30*time.Second) + probe.now = func() time.Time { return clock } + hc := NewChecker() + hc.SetReady() + hc.WatchPool(probe) + + if code, status := readyz(hc); code != http.StatusOK || status != stateNameReady { + t.Fatalf("first starved probe: %d %s; a pool is given starvedFor before it counts", code, status) + } + clock = clock.Add(29 * time.Second) + if code, _ := readyz(hc); code != http.StatusOK { + t.Fatalf("starved for 29s: %d", code) + } + clock = clock.Add(time.Second) + if code, status := readyz(hc); code != http.StatusServiceUnavailable || status != statusPoolStarved { + t.Fatalf("starved for 30s: %d %s", code, status) + } + + _ = conn.Close() + if code, _ := readyz(hc); code != http.StatusOK { + t.Fatalf("a freed connection: %d", code) + } + clock = clock.Add(time.Minute) + if _, err := db.Conn(context.Background()); err != nil { + t.Fatal(err) + } + if code, _ := readyz(hc); code != http.StatusOK { + t.Fatalf("starvation counts again from its own start: %d", code) + } +} + +// A pool with room, an unbounded pool and no pool at all are never starved; +// a starved pool does not make a draining replica report anything else. +func TestPoolProbe_NotStarved(t *testing.T) { + ctx := context.Background() + var nilProbe *PoolProbe + if nilProbe.Starved(ctx) || NewPoolProbe(nil, time.Millisecond, 0).Starved(ctx) { + t.Error("no pool is starved") + } + db, _, err := sqlmock.New() + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = db.Close() }) + if NewPoolProbe(db, time.Millisecond, 0).Starved(ctx) { + t.Error("an unbounded pool is starved") + } + db.SetMaxOpenConns(2) + if NewPoolProbe(db, time.Millisecond, 0).Starved(ctx) { + t.Error("a pool with room is starved") + } + + starved, _ := starvedPool(t) + hc := NewChecker() + hc.SetDraining() + hc.WatchPool(NewPoolProbe(starved, time.Millisecond, 0)) + if code, status := readyz(hc); code != http.StatusServiceUnavailable || status != stateNameDraining { + t.Errorf("draining with a starved pool: %d %s", code, status) + } +} + +// A pool every connection of which is in use, but which hands one out within +// the wait, is busy rather than starved. +func TestPoolProbe_ABusyPoolIsNotStarved(t *testing.T) { + db, conn := starvedPool(t) + probe := NewPoolProbe(db, 2*time.Second, 0) + freed := make(chan struct{}) + go func() { + defer close(freed) + time.Sleep(20 * time.Millisecond) + _ = conn.Close() + }() + if probe.Starved(context.Background()) { + t.Error("a pool that handed out a connection within the wait read as starved") + } + <-freed +} diff --git a/internal/httpserver/server.go b/internal/httpserver/server.go index 37d585435..c6d980dc3 100644 --- a/internal/httpserver/server.go +++ b/internal/httpserver/server.go @@ -171,6 +171,9 @@ func Serve(ctx context.Context, mcpServer *mcp.Server, p *platform.Platform, add mux := http.NewServeMux() hcfg := extractHTTPConfig(p) hc := health.NewChecker() + if p != nil && p.DB() != nil { + hc.WatchPool(health.NewPoolProbe(p.DB(), health.DefaultPoolWait, health.DefaultPoolStarvedFor)) + } if !hcfg.tlsEnabled { slog.WarnContext(ctx, "HTTP transport without TLS; credentials may be transmitted in plaintext") diff --git a/internal/maps/service.go b/internal/maps/service.go index a8863f911..89fbe8ba1 100644 --- a/internal/maps/service.go +++ b/internal/maps/service.go @@ -13,6 +13,7 @@ import ( "sync" "time" + "github.com/txn2/mcp-data-platform/internal/advisorylock" "github.com/txn2/mcp-data-platform/internal/bgloop" "github.com/txn2/mcp-data-platform/internal/logsan" "github.com/txn2/mcp-data-platform/internal/pmtiles" @@ -150,28 +151,14 @@ func (s *Service) poke() { } } -// advisoryLock takes the fetch lock on a session of its own, so the lock -// lives exactly as long as the pass. +// advisoryLock takes the fetch lock on the process's lock session; the pass +// runs on the pool (#2169). func (s *Service) advisoryLock(ctx context.Context) (unlock func(), ok bool, err error) { - conn, err := s.d.DB.Conn(ctx) + unlock, ok, err = advisorylock.Try(ctx, s.d.DB, advisorylock.ID(FetchLockKey)) if err != nil { - return nil, false, fmt.Errorf("maps: opening a lock session: %w", err) - } - var got bool - if err := conn.QueryRowContext(ctx, "SELECT pg_try_advisory_lock($1)", FetchLockKey).Scan(&got); err != nil { - _ = conn.Close() return nil, false, fmt.Errorf("maps: taking the fetch lock: %w", err) } - if !got { - _ = conn.Close() - return nil, false, nil - } - return func() { - unlockCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), 5*time.Second) - defer cancel() - _, _ = conn.ExecContext(unlockCtx, "SELECT pg_advisory_unlock($1)", FetchLockKey) - _ = conn.Close() - }, true, nil + return unlock, ok, nil } // Pass is one iteration of the fetch loop: under the fetch lock, it records diff --git a/internal/opranking/opranking.go b/internal/opranking/opranking.go index ce3168058..0437948be 100644 --- a/internal/opranking/opranking.go +++ b/internal/opranking/opranking.go @@ -76,6 +76,10 @@ const ScoreFloor = 0.45 // dominant while preserving that precision. const SemanticWeight = 0.6 +// unembeddedSemantic is the semantic term of a candidate with no vector: a +// cosine of zero mapped onto [0,1]. +const unembeddedSemantic = 0.5 + // Candidate is one rankable operation as this package sees it: an // opaque id the caller maps back, the text a query's tokens are matched // against, and the operation's embedding when one is indexed. @@ -169,7 +173,13 @@ func scoreOne(c Candidate, lexical bool, queryVec []float32, mode Mode) float64 if mode == ModeSemantic { return 0 } - return (1 - SemanticWeight) * lex + // A candidate not embedded yet is blended at a cosine of zero, the + // value every SQL ranker gives a row the vector arm cannot see + // (#2230): its lexical match then outranks every candidate that + // matches none of the query's tokens, as it does on every other + // surface. Blended at zero semantic, it fell below any near + // neighbor above 0.67. + return SemanticWeight*unembeddedSemantic + (1-SemanticWeight)*lex } // Map the cosine's [-1,1] onto [0,1] so the blend's two terms are on // one scale. diff --git a/internal/opranking/opranking_test.go b/internal/opranking/opranking_test.go index dd31e3a45..26554ad60 100644 --- a/internal/opranking/opranking_test.go +++ b/internal/opranking/opranking_test.go @@ -124,6 +124,24 @@ func TestScoreCreditsLexicalOnAnUnembeddedCandidate(t *testing.T) { } } +// #2230: an operation not embedded yet that contains every token of the +// query outranks a near neighbor that contains none, however near: it is +// blended at a cosine of zero, as every SQL ranker blends a row the vector +// arm cannot see. +func TestScoreAnUnembeddedMatchOutranksANearNeighbor(t *testing.T) { + cands := []Candidate{ + {ID: "neighbor", Fields: []string{"list invoices"}, Vector: []float32{0, 1, 0}}, + {ID: "match", Fields: []string{"list orders 2024"}}, + } + got := Score(cands, "orders 2024", []float32{0, 1, 0}, ModeHybrid) + if got[0].ID != "match" { + t.Fatalf("hybrid put %q first at %v; the unembedded token match (%v) must lead", got[0].ID, got[0].Score, got[1].Score) + } + if want := SemanticWeight*unembeddedSemantic + (1 - SemanticWeight); got[0].Score != want { + t.Errorf("the unembedded match scored %v; want %v, the SQL rankers' score for it", got[0].Score, want) + } +} + func TestScoreSemanticIgnoresAnUnembeddedCandidate(t *testing.T) { cands := []Candidate{{ID: "match", Fields: []string{"list orders"}}} got := Score(cands, "orders", []float32{0, 1, 0}, ModeSemantic) diff --git a/internal/platform/callrecord/outcome_realdb_integration_test.go b/internal/platform/callrecord/outcome_realdb_integration_test.go new file mode 100644 index 000000000..e04c51cf3 --- /dev/null +++ b/internal/platform/callrecord/outcome_realdb_integration_test.go @@ -0,0 +1,139 @@ +//go:build integration + +package callrecord + +import ( + "context" + "testing" + "time" + + sq "github.com/Masterminds/squirrel" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/txn2/mcp-data-platform/internal/testdb" +) + +// outcomeCatalogSize is #2231's catalog: the admin list took 36 seconds to +// filter about 95,000 records by outcome. +const outcomeCatalogSize = 100_000 + +// outcomeBound is how long one filtered page and its count may take. +const outcomeBound = time.Second + +// outcomeAttempts is how many times each is measured; the fastest counts. +const outcomeAttempts = 3 + +// seedOutcomeCatalog writes outcomeCatalogSize records: every seventeenth +// failed, pairs of reads over one target in one session (the newer supersedes +// the older), a memory record citing every seven-hundredth, and assets citing +// others. +func seedOutcomeCatalog(ctx context.Context, t *testing.T, store *PostgresStore) { + t.Helper() + _, err := store.db.ExecContext(ctx, ` + INSERT INTO call_records (event_id, kind, tool_name, connection, statement, targets, + user_id, session_id, success, created_at) + SELECT 'evt-' || g, 'sql', 'trino_query', 'wh', 'SELECT ' || (g / 2), + jsonb_build_array('trino:wh:t' || (g / 2)), 'u' || (g % 7), 's-' || (g / 20), + g % 17 <> 0, now() - make_interval(secs => g) + FROM generate_series(1, $1) AS g`, outcomeCatalogSize) + require.NoError(t, err) + _, err = store.db.ExecContext(ctx, ` + INSERT INTO memory_records (id, content, metadata) + SELECT 'mem-' || g, 'cites a call', jsonb_build_object('sources', jsonb_build_array('mcp:call:evt-' || (g * 700))) + FROM generate_series(1, $1 / 700) AS g`, outcomeCatalogSize) + require.NoError(t, err) + // Assets cite calls four ways: a save that named its call, an export that + // streamed one, a capture that swept a call in without naming it (cited + // set, not satisfied: the rule still decides), and a deleted asset. + _, err = store.db.ExecContext(ctx, ` + INSERT INTO portal_assets (id, owner_id, name, content_type, s3_bucket, s3_key, provenance, deleted_at) + SELECT 'asset-' || g || '-' || k.n, 'u0', 'cites a call', 'text/html', 'b', 'k' || g || k.n, + jsonb_build_object('captures', jsonb_build_array(jsonb_build_object( + 'tool', k.tool, 'explicit', k.explicit, + 'event_ids', jsonb_build_array('evt-' || (g * 900 + k.n)), + 'calls', jsonb_build_array(jsonb_build_object('event_id', 'evt-' || (g * 900 + k.n), 'cited', k.explicit))))), + CASE WHEN k.deleted THEN now() END + FROM generate_series(1, 60) AS g, + (VALUES (1, 'save_asset', true, false), (3, 'trino_export', true, false), + (5, 'save_asset', false, false), (7, 'save_asset', true, true)) AS k(n, tool, explicit, deleted)`) + require.NoError(t, err) + _, err = store.db.ExecContext(ctx, `ANALYZE call_records; ANALYZE memory_records; ANALYZE portal_assets`) + require.NoError(t, err) +} + +// unguardedOutcomes counts the catalog by outcome with the satisfied rule +// asked of every record, the read before #2231, so the guarded read is held to +// deciding every record the same way. +func unguardedOutcomes(ctx context.Context, t *testing.T, store *PostgresStore) map[string]int { + t.Helper() + inner := sq.Select(storedColumns...). + Column(satisfiedByCase("?")+" AS satisfied_by", callReferencePrefix()). + Column(supersededExpr). + Column(reuseCountExpr). + From("call_records r") + query, args, err := psq.Select("o.outcome", "COUNT(*)"). + FromSelect(outcomeOver(inner), "o").GroupBy("o.outcome").ToSql() + require.NoError(t, err) + rows, err := store.db.QueryContext(ctx, query, args...) + require.NoError(t, err) + defer func() { _ = rows.Close() }() + out := map[string]int{} + for rows.Next() { + var outcome string + var n int + require.NoError(t, rows.Scan(&outcome, &n)) + out[outcome] = n + } + require.NoError(t, rows.Err()) + return out +} + +// TestCallCatalogRealDBFiltersEachOutcomeWithinASecond is #2231: on a catalog +// of 100,000 records, a page of each outcome and its count answer within a +// second, for the whole catalog (the admin list) and for one caller's records +// (the portal's), and each outcome counts what the rule asked of every record +// counts. +func TestCallCatalogRealDBFiltersEachOutcomeWithinASecond(t *testing.T) { + ctx := context.Background() + store := NewPostgresStore(testdb.New(t), Config{}) + seedOutcomeCatalog(ctx, t, store) + want := unguardedOutcomes(ctx, t, store) + for _, outcome := range Outcomes { + require.Positive(t, want[outcome], "the seed holds no %s record", outcome) + } + + for _, scope := range []struct{ name, userID string }{{"admin", ""}, {"portal", "u0"}} { + for _, outcome := range Outcomes { + f := Filter{UserID: scope.userID, Outcome: outcome, Limit: 50} + // The best of three: this runs beside the rest of the real-database + // suite, and one sample that lands on another package's burst + // measures that package rather than this query. + var ( + page []Record + count int + took time.Duration + ) + for attempt := range outcomeAttempts { + start := time.Now() + var err error + page, err = store.List(ctx, f) + require.NoError(t, err) + count, err = store.Count(ctx, f) + require.NoError(t, err) + if d := time.Since(start); attempt == 0 || d < took { + took = d + } + } + t.Logf("%s %s: %d records, page and count in %s (best of %d)", scope.name, outcome, count, took, outcomeAttempts) + assert.Less(t, took, outcomeBound, "%s list filtered to %s", scope.name, outcome) + require.NotEmpty(t, page, "%s %s", scope.name, outcome) + for _, rec := range page { + assert.Equal(t, outcome, rec.Outcome) + } + if scope.userID == "" { + assert.Equal(t, want[outcome], count, "%s records counted", outcome) + } + } + } +} diff --git a/internal/platform/callrecord/postgres.go b/internal/platform/callrecord/postgres.go index 731bdf664..08345957b 100644 --- a/internal/platform/callrecord/postgres.go +++ b/internal/platform/callrecord/postgres.go @@ -153,9 +153,41 @@ func namedSourceExpr(eventID string) string { )` } +// citedSet is every event id an artifact names anywhere it could name a +// source: a memory record's sources and an asset capture's event_ids (#2231). +// It is not a second statement of the rule: every record satisfiedByCase finds +// satisfied is in it, so a read that must decide every record (a count) asks +// the rule only of the records in it and reads every other as unsatisfied, and +// a read of the satisfied records narrows to it first. The rule itself still +// decides. Collected once and hashed, it replaces two index probes per record, +// which on a catalog of 100,000 records was three seconds of every count. +// +// Each array is read only where it is one: containment, which the rule uses, +// never matches through a value that is not an array. The prefix is bound +// three times. +const citedSet = `SELECT substr(src.v, length(?) + 1) AS event_id FROM memory_records m + CROSS JOIN LATERAL jsonb_array_elements_text( + CASE WHEN jsonb_typeof(m.metadata->'sources') = 'array' THEN m.metadata->'sources' ELSE '[]'::jsonb END) src(v) + WHERE left(src.v, length(?)) = ? + UNION + SELECT ids.v FROM portal_assets a + CROSS JOIN LATERAL jsonb_array_elements( + CASE WHEN jsonb_typeof(a.provenance->'captures') = 'array' THEN a.provenance->'captures' ELSE '[]'::jsonb END) cap + CROSS JOIN LATERAL jsonb_array_elements_text( + CASE WHEN jsonb_typeof(cap->'event_ids') = 'array' THEN cap->'event_ids' ELSE '[]'::jsonb END) ids(v) + WHERE a.deleted_at IS NULL` + // satisfiedByExpr is the derived read's column: the rule above, named. var satisfiedByExpr = satisfiedByCase("?") + " AS satisfied_by" +// citedRule is the rule asked only of a record the joined citedSet names, and +// NULL for every other. It reads the joined row, which also keeps the planner +// from asking the rule of every record below the join. +var citedRule = "CASE WHEN cited.event_id IS NOT NULL THEN " + satisfiedByCase("?") + " END" + +// citedJoin joins citedSet as cited; the prefix is bound three times. +const citedJoin = "(" + citedSet + ") cited ON cited.event_id = r.event_id" + // supersededExpr answers whether the same session later ran a better version of // this call: same connection, same kind, same targets, and it succeeded. // @@ -169,7 +201,12 @@ var satisfiedByExpr = satisfiedByCase("?") + " AS satisfied_by" // mutation is not a better version of an earlier mutation, even against the // same resource (#1352). Approving a script twice is two decisions, and // inserting a row twice is two rows. -var supersededExpr = fmt.Sprintf(`EXISTS ( +var supersededExpr = supersededPredicate + " AS superseded" + +// supersededPredicate is the rule supersededExpr names. A count states it +// once more as a condition of the records it reads (#2231), where the planner +// can join it by hash rather than probe it per record. +var supersededPredicate = fmt.Sprintf(`EXISTS ( SELECT 1 FROM call_records l WHERE r.session_id <> '' AND l.session_id = r.session_id @@ -182,7 +219,7 @@ var supersededExpr = fmt.Sprintf(`EXISTS ( AND %s AND %s AND (l.created_at, l.event_id) > (r.created_at, r.event_id) -) AS superseded`, readShapedExpr("r"), readShapedExpr("l")) +)`, readShapedExpr("r"), readShapedExpr("l")) // readStatementPattern matches a statement that opens with a reading verb. It is // anchored at the start, past any opening parentheses a wrapped select carries, @@ -228,19 +265,105 @@ var outcomeExpr = fmt.Sprintf(`CASE ELSE '%s' END AS outcome`, OutcomeFailed, OutcomeSatisfied, OutcomeSuperseded, OutcomeRan) +// supersededLater is supersededPredicate for a read that decides every record +// at once (#2231): one pass over the reads that can take part, each group of +// one session, kind, connection and targets in (created_at, event_id) order, +// marking a read with a later successful read in its group. That is the rule's +// own condition, stated as an ordering: event_id is unique, so a call is never +// later than itself, only a read supersedes or is superseded, and a group's +// targets are non-empty and its session named whenever either call's are. +// Asked per record, the rule probed the session's calls once for each of +// 100,000 records; this sorts them once. +// +// The group is newest first, with the frame from its start to the row before, +// so the aggregate grows rather than being recomputed for every row: a session +// that calls one endpoint thousands of times costs a pass, not its square. +// Targets group by their text in byte order. Every writer stores them as an +// array of strings, for which equal text is equal jsonb, and comparing bytes +// is what keeps the sort from decoding each value. The joined row is sup, and +// sup.later is NULL for a call outside every group. +var supersededLater = fmt.Sprintf(`( + SELECT w.id, bool_or(w.success) OVER ( + PARTITION BY w.session_id, w.kind, w.connection, w.targets::text COLLATE "C" + ORDER BY w.created_at DESC, w.event_id DESC + ROWS BETWEEN UNBOUNDED PRECEDING AND 1 PRECEDING) AS later + FROM call_records w + WHERE w.session_id <> '' AND jsonb_array_length(w.targets) > 0 AND %s +) sup ON sup.id = r.id`, readShapedExpr("w")) + +// supersededAtOnce is the rule's answer for the joined record: a read whose +// group holds a later successful read. A record the pass did not see is not a +// read in a session over targets, and is never superseded. +var supersededAtOnce = "COALESCE(sup.later, FALSE)" + +// outcomeCondition is outcomeExpr's branch for one outcome, as a condition on +// a record, for a count (#2231). Stated as the conditions the CASE reaches +// that outcome by, in its order, a count asks the satisfied rule only of a +// record citedSet names and asks the supersession rule only of a record that +// got past the branches before it. The branches are the CASE's, read from the +// same expressions. +// +// Supersession is read from supersededLater, one ordered pass, rather than +// probed per record. Written as the EXISTS in a WHERE clause it was a join the +// planner hashed on the session, kind, connection and targets, and the records +// with no session, or one session's thousands of calls to one endpoint, were +// then a single bucket compared pair by pair: a count of a catalog of 108,000 +// records, 103,000 with no session, ran for minutes. +func outcomeCondition(outcome string) sq.Sqlizer { + prefix := callReferencePrefix() + unsatisfied := sq.Expr("("+citedRule+") IS NULL", prefix) + switch outcome { + case OutcomeFailed: + return sq.Expr("NOT r.success") + case OutcomeSatisfied: + return sq.And{sq.Expr("r.success"), sq.Expr("("+citedRule+") IS NOT NULL", prefix)} + case OutcomeSuperseded: + return sq.And{sq.Expr("r.success"), unsatisfied, sq.Expr(supersededAtOnce)} + default: + return sq.And{sq.Expr("r.success"), unsatisfied, sq.Expr("NOT " + supersededAtOnce)} + } +} + // derived builds the record rows with their derived columns, before the outcome // is named or filtered. The capture reference prefix is bound rather than // spliced: the mcp: reference grammar has one owner (pkg/portal/knowledgepage) // and this reads it rather than restating it in SQL. +// +// A read of the satisfied records joins citedSet and asks the rule of the +// records it names (#2231): satisfied records are few, and found through the +// rule alone they cost a probe of every record in the catalog. Every other +// read asks the rule per record, so a page read newest first stops after a +// page of matches. func derived(f Filter) sq.SelectBuilder { - qb := sq.Select(storedColumns...). - Column(satisfiedByExpr, callReferencePrefix()). - Column(supersededExpr). - Column(reuseCountExpr). - From("call_records r") + prefix := callReferencePrefix() + qb := sq.Select(storedColumns...) + if facetOutcome(f) == OutcomeSatisfied { + qb = qb.Column(citedRule+" AS satisfied_by", prefix). + Column(supersededExpr). + Column(reuseCountExpr). + From("call_records r"). + Join(citedJoin, prefix, prefix, prefix) + } else { + qb = qb.Column(satisfiedByExpr, prefix). + Column(supersededExpr). + Column(reuseCountExpr). + From("call_records r") + } return applyFilters(qb, f) } +// facetOutcome is the outcome a filter narrows to, empty when none: its +// outcome, or satisfied for the review queue. +func facetOutcome(f Filter) string { + if f.PromotableOnly { + return OutcomeSatisfied + } + if ValidOutcome(f.Outcome) { + return f.Outcome + } + return "" +} + // withOutcome names the outcome over the derived rows. func withOutcome(f Filter) sq.SelectBuilder { return outcomeOver(derived(f)) } @@ -285,7 +408,20 @@ func applyFilters(qb sq.SelectBuilder, f Filter) sq.SelectBuilder { if f.PromotableOnly { qb = qb.Where(sq.Eq{"r.promoted_urn": "", "r.rejected_at": nil}) } - return qb + return successFilter(qb, f) +} + +// successFilter states, on the stored column, the part of an outcome facet +// the record's own success decides (#2231): a failed record is exactly one +// that did not succeed, and every other outcome is of one that did. The facet +// on the named outcome still decides; this keeps the derived columns from +// being computed for records it cannot match. +func successFilter(qb sq.SelectBuilder, f Filter) sq.SelectBuilder { + outcome := facetOutcome(f) + if outcome == "" { + return qb + } + return qb.Where(sq.Eq{"r.success": outcome != OutcomeFailed}) } // outcomeFilter applies the derived-outcome facets over the named rows. The @@ -314,6 +450,31 @@ func listQuery(f Filter) sq.SelectBuilder { return qb.OrderBy("o.created_at DESC", "o.event_id DESC") } +// noJIT turns JIT compilation off for a catalog read (#2231). The derived +// read's estimated cost is high enough that the planner compiles it, and the +// compile took a second and a half of every list and count on a catalog of +// 100,000 records, longer than the read. +const noJIT = "SET LOCAL jit = off" + +// withoutJIT runs fn in a read-only transaction under noJIT. +func (s *PostgresStore) withoutJIT(ctx context.Context, fn func(tx *sql.Tx) error) error { + tx, err := s.db.BeginTx(ctx, &sql.TxOptions{ReadOnly: true}) + if err != nil { + return fmt.Errorf("opening the call record read: %w", err) + } + defer func() { _ = tx.Rollback() }() + if _, err := tx.ExecContext(ctx, noJIT); err != nil { + return fmt.Errorf("opening the call record read: %w", err) + } + if err := fn(tx); err != nil { + return err + } + if err := tx.Commit(); err != nil { + return fmt.Errorf("closing the call record read: %w", err) + } + return nil +} + // List returns records matching the filter, newest first. func (s *PostgresStore) List(ctx context.Context, f Filter) ([]Record, error) { qb := listQuery(f) @@ -328,39 +489,62 @@ func (s *PostgresStore) List(ctx context.Context, f Filter) ([]Record, error) { return nil, fmt.Errorf("building call record list query: %w", err) } - rows, err := s.db.QueryContext(ctx, query, args...) - if err != nil { - return nil, fmt.Errorf("querying call records: %w", err) - } - defer func() { _ = rows.Close() }() - records := make([]Record, 0, listCapacity(f.Limit)) - for rows.Next() { - rec, err := scanRecord(rows) + err = s.withoutJIT(ctx, func(tx *sql.Tx) error { + rows, err := tx.QueryContext(ctx, query, args...) if err != nil { - return nil, err + return fmt.Errorf("querying call records: %w", err) } - records = append(records, rec) - } - if err := rows.Err(); err != nil { - return nil, fmt.Errorf("iterating call records: %w", err) + defer func() { _ = rows.Close() }() + for rows.Next() { + rec, err := scanRecord(rows) + if err != nil { + return err + } + records = append(records, rec) + } + if err := rows.Err(); err != nil { + return fmt.Errorf("iterating call records: %w", err) + } + return nil + }) + if err != nil { + return nil, err } return records, nil } +// countQuery counts the records the filter matches. With an outcome facet it +// counts by outcomeCondition, over the records joined to citedSet; without +// one it names no outcome at all. +func countQuery(f Filter) sq.SelectBuilder { + qb := psq.Select("COUNT(*)").From("call_records r") + if outcome := facetOutcome(f); outcome != "" { + prefix := callReferencePrefix() + qb = qb.LeftJoin(citedJoin, prefix, prefix, prefix) + if outcome == OutcomeSuperseded || outcome == OutcomeRan { + qb = qb.LeftJoin(supersededLater) + } + qb = qb.Where(outcomeCondition(outcome)) + } + return applyFilters(qb, f) +} + // Count returns how many records match the filter, ignoring its paging. func (s *PostgresStore) Count(ctx context.Context, f Filter) (int, error) { - f.Limit, f.Offset = 0, 0 - inner := sq.Select("o.outcome").FromSelect(withOutcome(f), "o") - inner = outcomeFilter(inner, f) - - query, args, err := psq.Select("COUNT(*)").FromSelect(inner, "c").ToSql() + query, args, err := countQuery(f).ToSql() if err != nil { return 0, fmt.Errorf("building call record count query: %w", err) } var count int - if err := s.db.QueryRowContext(ctx, query, args...).Scan(&count); err != nil { - return 0, fmt.Errorf("counting call records: %w", err) + err = s.withoutJIT(ctx, func(tx *sql.Tx) error { + if err := tx.QueryRowContext(ctx, query, args...).Scan(&count); err != nil { + return fmt.Errorf("counting call records: %w", err) + } + return nil + }) + if err != nil { + return 0, err } return count, nil } diff --git a/internal/platform/callrecord/postgres_test.go b/internal/platform/callrecord/postgres_test.go index 1462762b7..5f8cdb088 100644 --- a/internal/platform/callrecord/postgres_test.go +++ b/internal/platform/callrecord/postgres_test.go @@ -76,10 +76,19 @@ func nullable(s string) any { return s } +// expectJITOff queues the read-only transaction a list or a count runs in, +// with JIT off (#2231). +func expectJITOff(mock sqlmock.Sqlmock) { + mock.ExpectBegin() + mock.ExpectExec("SET LOCAL jit = off").WillReturnResult(sqlmock.NewResult(0, 0)) +} + func TestListProjectsARecord(t *testing.T) { store, mock := newMock(t) + expectJITOff(mock) mock.ExpectQuery("FROM call_records"). WillReturnRows(recordRow(OutcomeSatisfied, SatisfiedByCapture, 2)) + mock.ExpectCommit() got, err := store.List(context.Background(), Filter{UserID: "u1", Limit: 10}) require.NoError(t, err) @@ -107,7 +116,9 @@ func TestListCarriesADecidedRecordsTimestamps(t *testing.T) { "asset", false, 0, OutcomeSatisfied, ) + expectJITOff(mock) mock.ExpectQuery("FROM call_records").WillReturnRows(row) + mock.ExpectCommit() got, err := store.List(context.Background(), Filter{}) require.NoError(t, err) require.Len(t, got, 1) @@ -123,9 +134,11 @@ func TestListScopesToTheCallerInSQL(t *testing.T) { store, mock := newMock(t) // The caller is a predicate inside the query, not a check after the read: // another caller's record must never be selected in the first place. + expectJITOff(mock) mock.ExpectQuery("FROM call_records"). WithArgs(sqlmock.AnyArg(), "u1"). WillReturnRows(sqlmock.NewRows(recordColumns)) + mock.ExpectCommit() _, err := store.List(context.Background(), Filter{UserID: "u1"}) require.NoError(t, err) @@ -133,8 +146,10 @@ func TestListScopesToTheCallerInSQL(t *testing.T) { func TestListAppliesEveryFacet(t *testing.T) { store, mock := newMock(t) + expectJITOff(mock) mock.ExpectQuery("FROM call_records"). WillReturnRows(sqlmock.NewRows(recordColumns)) + mock.ExpectCommit() _, err := store.List(context.Background(), Filter{ UserID: "u1", Kind: KindAPI, Connection: "acme", Outcome: OutcomeRan, @@ -148,8 +163,10 @@ func TestListReviewQueueOrdersByReuse(t *testing.T) { store, mock := newMock(t) // A query a stranger re-ran is better evidence than one its own author // vouched for, so the queue leads with reuse. + expectJITOff(mock) mock.ExpectQuery("ORDER BY o.reuse_count DESC"). WillReturnRows(recordRow(OutcomeSatisfied, SatisfiedByAsset, 3)) + mock.ExpectCommit() got, err := store.List(context.Background(), Filter{PromotableOnly: true}) require.NoError(t, err) @@ -158,7 +175,9 @@ func TestListReviewQueueOrdersByReuse(t *testing.T) { func TestListReportsAQueryFailure(t *testing.T) { store, mock := newMock(t) + expectJITOff(mock) mock.ExpectQuery("FROM call_records").WillReturnError(errors.New("db down")) + mock.ExpectRollback() _, err := store.List(context.Background(), Filter{}) require.Error(t, err) @@ -166,7 +185,9 @@ func TestListReportsAQueryFailure(t *testing.T) { func TestCountIgnoresPaging(t *testing.T) { store, mock := newMock(t) + expectJITOff(mock) mock.ExpectQuery("COUNT").WillReturnRows(sqlmock.NewRows([]string{"count"}).AddRow(7)) + mock.ExpectCommit() got, err := store.Count(context.Background(), Filter{UserID: "u1", Limit: 5, Offset: 10}) require.NoError(t, err) @@ -400,3 +421,83 @@ func TestNormalizeTargetsSortsAndDeduplicates(t *testing.T) { // Verify the store satisfies the sql.DB-backed contract the wiring passes it as. var _ Store = (*PostgresStore)(nil) + +// TestCountByOutcomeStatesTheCasesBranch: a count filtered by outcome counts +// the records meeting the condition the outcome CASE reaches it by, over the +// cited join, and asks no outcome of an unfiltered count (#2231). +func TestCountByOutcomeStatesTheCasesBranch(t *testing.T) { + cases := map[string][]string{ + OutcomeFailed: {"NOT r.success", "r.success = $"}, + OutcomeSatisfied: {"cited.event_id IS NOT NULL THEN", ") IS NOT NULL"}, + OutcomeSuperseded: {") IS NULL", "COALESCE(sup.later, FALSE)", "PARTITION BY w.session_id"}, + OutcomeRan: {") IS NULL", "NOT COALESCE(sup.later, FALSE)", "PARTITION BY w.session_id"}, + } + for outcome, fragments := range cases { + query, _, err := countQuery(Filter{Outcome: outcome}).ToSql() + require.NoError(t, err) + assert.Contains(t, query, "LEFT JOIN (SELECT substr(src.v", outcome) + for _, fragment := range fragments { + assert.Contains(t, query, fragment, outcome) + } + assert.NotContains(t, query, "AS outcome", "a count names no outcome") + } + plain, _, err := countQuery(Filter{UserID: "u1"}).ToSql() + require.NoError(t, err) + assert.NotContains(t, plain, "cited.event_id", "an unfiltered count joins nothing") + queue, _, err := countQuery(Filter{PromotableOnly: true}).ToSql() + require.NoError(t, err) + assert.Contains(t, queue, ") IS NOT NULL", "the review queue counts satisfied records") +} + +// TestListOfSatisfiedJoinsTheCitedSet: a page of satisfied records reads the +// records an artifact names, and every other page asks the rule per record so +// it can stop at a page of matches (#2231). +func TestListOfSatisfiedJoinsTheCitedSet(t *testing.T) { + satisfied, _, err := listQuery(Filter{Outcome: OutcomeSatisfied}).ToSql() + require.NoError(t, err) + assert.Contains(t, satisfied, "JOIN (SELECT substr(src.v") + assert.Contains(t, satisfied, "r.success = $") + ran, _, err := listQuery(Filter{Outcome: OutcomeRan}).ToSql() + require.NoError(t, err) + assert.NotContains(t, ran, "cited.event_id") +} + +// TestCatalogReadFailures: each step of the read's transaction that fails is +// reported. +func TestCatalogReadFailures(t *testing.T) { + boom := errors.New("db down") + steps := map[string]func(sqlmock.Sqlmock){ + "begin": func(m sqlmock.Sqlmock) { m.ExpectBegin().WillReturnError(boom) }, + "settings": func(m sqlmock.Sqlmock) { + m.ExpectBegin() + m.ExpectExec("SET LOCAL jit").WillReturnError(boom) + m.ExpectRollback() + }, + "count": func(m sqlmock.Sqlmock) { + expectJITOff(m) + m.ExpectQuery("COUNT").WillReturnError(boom) + m.ExpectRollback() + }, + "commit": func(m sqlmock.Sqlmock) { + expectJITOff(m) + m.ExpectQuery("COUNT").WillReturnRows(sqlmock.NewRows([]string{"count"}).AddRow(1)) + m.ExpectCommit().WillReturnError(boom) + }, + } + for name, expect := range steps { + t.Run(name, func(t *testing.T) { + store, mock := newMock(t) + expect(mock) + _, err := store.Count(context.Background(), Filter{UserID: "u1"}) + require.ErrorIs(t, err, boom) + }) + } + t.Run("scan", func(t *testing.T) { + store, mock := newMock(t) + expectJITOff(mock) + mock.ExpectQuery("FROM call_records").WillReturnRows(sqlmock.NewRows([]string{"id"}).AddRow("x")) + mock.ExpectRollback() + _, err := store.List(context.Background(), Filter{}) + require.Error(t, err) + }) +} diff --git a/internal/platform/callrecord/retention.go b/internal/platform/callrecord/retention.go index d37f7240e..1d5180144 100644 --- a/internal/platform/callrecord/retention.go +++ b/internal/platform/callrecord/retention.go @@ -9,7 +9,9 @@ import ( "github.com/lib/pq" + "github.com/txn2/mcp-data-platform/internal/advisorylock" "github.com/txn2/mcp-data-platform/internal/bgloop" + "github.com/txn2/mcp-data-platform/internal/logsan" "github.com/txn2/mcp-data-platform/internal/opsobs" "github.com/txn2/mcp-data-platform/pkg/script" ) @@ -63,10 +65,6 @@ const ( // subsystem's sweep never blocks another's. sweepLockKey = 4713210001 - // unlockTimeout bounds the advisory unlock after a sweep, so a shutdown - // cannot hang on releasing it. - unlockTimeout = 5 * time.Second - // sweepBatchSize bounds how many records one statement of the sweep // removes. It is the same bound the index queue's own retention purges // by: large enough that a steady-state sweep is one statement, small @@ -242,30 +240,16 @@ func (s *PostgresStore) Close() error { // deletes per tick. It returns bgloop.ErrSkipped when another replica held // the lock, and the error of a sweep that failed. func (s *PostgresStore) sweepTick(ctx context.Context) error { - conn, err := s.db.Conn(ctx) + release, acquired, err := advisorylock.Try(ctx, s.db, advisorylock.ID(sweepLockKey)) if err != nil { - slog.WarnContext(ctx, "call catalog: acquire connection for the retention lock", "error", err) - return fmt.Errorf("call catalog: retention lock connection: %w", err) - } - defer func() { _ = conn.Close() }() - - var acquired bool - if err := conn.QueryRowContext(ctx, - "SELECT pg_try_advisory_lock($1)", sweepLockKey).Scan(&acquired); err != nil { - slog.WarnContext(ctx, "call catalog: try retention lock", "error", err) + slog.WarnContext(ctx, "call catalog: try retention lock", "error", logsan.SanitizeForLog(err.Error())) return fmt.Errorf("call catalog: try retention lock: %w", err) } if !acquired { // Another replica is sweeping; this tick has nothing to do. return bgloop.ErrSkipped } - defer func() { - unlockCtx, cancel := context.WithTimeout(context.Background(), unlockTimeout) - defer cancel() - if _, err := conn.ExecContext(unlockCtx, "SELECT pg_advisory_unlock($1)", sweepLockKey); err != nil { - slog.WarnContext(ctx, "call catalog: release retention lock", "error", err) - } - }() + defer release() removed, err := s.Cleanup(ctx) if err != nil { diff --git a/internal/platform/capacity/capacity.go b/internal/platform/capacity/capacity.go index 5830f5f59..87bbf2dbd 100644 --- a/internal/platform/capacity/capacity.go +++ b/internal/platform/capacity/capacity.go @@ -32,6 +32,7 @@ import ( "sync" "time" + "github.com/txn2/mcp-data-platform/internal/advisorylock" "github.com/txn2/mcp-data-platform/internal/bgloop" "github.com/txn2/mcp-data-platform/internal/logsan" "github.com/txn2/mcp-data-platform/internal/objectobs" @@ -49,8 +50,6 @@ const ( // other maintenance lock (retention 4713210101-4713210105, the call // catalog 4713210001, maps 4713210301). scanLockKey int64 = 4713210401 - - unlockTimeout = 5 * time.Second ) // Sources is what the service reads. @@ -355,27 +354,16 @@ var errListedRecently = errors.New("capacity: listed recently") // recent for. const recentFraction = 0.9 -// locked runs fn while holding the listing's advisory lock on a dedicated -// connection, since an advisory lock belongs to the session that took it. +// locked runs fn while holding the listing's advisory lock on the process's +// lock session; fn runs on the pool (#2169). func (s *Service) locked(ctx context.Context, fn func(context.Context) error) (bool, error) { - conn, err := s.src.DB.Conn(ctx) + release, got, err := advisorylock.Try(ctx, s.src.DB, advisorylock.ID(scanLockKey)) if err != nil { - return false, fmt.Errorf("acquiring a connection for the lock: %w", err) - } - defer func() { _ = conn.Close() }() - var got bool - if err := conn.QueryRowContext(ctx, "SELECT pg_try_advisory_lock($1)", scanLockKey).Scan(&got); err != nil { - return false, fmt.Errorf("taking the advisory lock: %w", err) + return false, err //nolint:wrapcheck // the lock session names the statement } if !got { return false, nil } - defer func() { - unlockCtx, cancel := context.WithTimeout(context.Background(), unlockTimeout) - defer cancel() - if _, err := conn.ExecContext(unlockCtx, "SELECT pg_advisory_unlock($1)", scanLockKey); err != nil { - slog.WarnContext(ctx, "capacity: advisory unlock failed", "error", logsan.SanitizeForLog(err.Error())) - } - }() + defer release() return true, fn(ctx) } diff --git a/internal/platform/capacity/capacity_test.go b/internal/platform/capacity/capacity_test.go index 15aa41bd5..c0deec3a0 100644 --- a/internal/platform/capacity/capacity_test.go +++ b/internal/platform/capacity/capacity_test.go @@ -261,7 +261,7 @@ func TestService_ScanFailures(t *testing.T) { require.NoError(t, mock.ExpectationsWereMet()) _ = db.Close() - require.ErrorContains(t, s.scan(context.Background()), "acquiring a connection") + require.ErrorContains(t, s.scan(context.Background()), "acquiring the lock session") } // TestService_AdministratorBudgetsOverrideTheEnvironment covers #2148: a diff --git a/internal/platform/catalogimport/runner.go b/internal/platform/catalogimport/runner.go index c3202c76f..ecd1be4a0 100644 --- a/internal/platform/catalogimport/runner.go +++ b/internal/platform/catalogimport/runner.go @@ -8,6 +8,7 @@ import ( "log/slog" "sync" + "github.com/txn2/mcp-data-platform/internal/advisorylock" "github.com/txn2/mcp-data-platform/internal/logsan" ) @@ -129,22 +130,16 @@ func (rn *Runner) Start(ctx context.Context, url, token, by string) (Run, error) if err != nil { return Run{}, fmt.Errorf("opening DataHub at %s: %w", url, err) } - conn, err := rn.db.Conn(ctx) + release, got, err := advisorylock.Try(ctx, rn.db, advisorylock.ID(LockKey)) if err != nil { - return Run{}, fmt.Errorf("taking a connection for the import's lock: %w", err) - } - var got bool - if err := conn.QueryRowContext(ctx, "SELECT pg_try_advisory_lock($1)", LockKey).Scan(&got); err != nil { - _ = conn.Close() return Run{}, fmt.Errorf("taking the import's lock: %w", err) } if !got { - _ = conn.Close() return Run{}, ErrRunning } run, err := rn.store.begin(ctx, url, by) if err != nil { - release(conn) + release() return Run{}, err } rn.mu.Lock() @@ -153,7 +148,7 @@ func (rn *Runner) Start(ctx context.Context, url, token, by string) (Run, error) rn.mu.Unlock() go func() { defer rn.wg.Done() - defer release(conn) + defer release() rn.execute(runCtx, run.ID, src) }() return run, nil @@ -176,12 +171,6 @@ func (rn *Runner) execute(ctx context.Context, id string, src Source) { } } -// release lets go of the lock and its connection. -func release(conn *sql.Conn) { - _, _ = conn.ExecContext(context.Background(), "SELECT pg_advisory_unlock($1)", LockKey) - _ = conn.Close() -} - // Runs lists the latest runs, newest first, saying which is under way. func (rn *Runner) Runs(ctx context.Context, limit int) ([]Run, error) { running, err := rn.locked(ctx) @@ -193,19 +182,11 @@ func (rn *Runner) Runs(ctx context.Context, limit int) ([]Run, error) { // locked reports whether some replica holds the import's lock. func (rn *Runner) locked(ctx context.Context) (bool, error) { - conn, err := rn.db.Conn(ctx) + held, err := advisorylock.For(rn.db).Held(ctx, advisorylock.ID(LockKey)) if err != nil { - return false, fmt.Errorf("taking a connection to read the import's lock: %w", err) - } - defer func() { _ = conn.Close() }() - var got bool - if err := conn.QueryRowContext(ctx, "SELECT pg_try_advisory_lock($1)", LockKey).Scan(&got); err != nil { return false, fmt.Errorf("reading the import's lock: %w", err) } - if got { - _, _ = conn.ExecContext(ctx, "SELECT pg_advisory_unlock($1)", LockKey) - } - return !got, nil + return held, nil } // Configured is the address of the DataHub the configuration names, or "". diff --git a/internal/platform/catalogscan/loop.go b/internal/platform/catalogscan/loop.go index a3286a978..2bdeed1e2 100644 --- a/internal/platform/catalogscan/loop.go +++ b/internal/platform/catalogscan/loop.go @@ -9,6 +9,7 @@ import ( "sync" "time" + "github.com/txn2/mcp-data-platform/internal/advisorylock" "github.com/txn2/mcp-data-platform/internal/bgloop" "github.com/txn2/mcp-data-platform/internal/logsan" ) @@ -21,9 +22,6 @@ const LockKey int64 = 4713210201 // it a scan starts. const pollInterval = time.Minute -// unlockTimeout bounds the release of the lock after a scan. -const unlockTimeout = 5 * time.Second - // Loop runs a scan whenever the configured interval has passed since the // last one any replica started, and whenever asked to. type Loop struct { @@ -141,8 +139,8 @@ func (l *Loop) runOnce(ctx context.Context) error { // requested; no row means neither has happened. const scanDue = `SELECT started_at, requested_at FROM catalog_scan_state WHERE id` -// lockedScan takes the lock on a dedicated connection, since an advisory lock -// belongs to the session that took it, and scans when due. It returns +// lockedScan takes the lock on the process's lock session and scans when +// due; the scan's own queries run on the pool (#2169). It returns // bgloop.ErrSkipped when another replica holds the lock, and ran is false // when no scan was due. // @@ -150,31 +148,20 @@ const scanDue = `SELECT started_at, requested_at FROM catalog_scan_state WHERE i // replica that asked was refused the lock, and would otherwise wait for its // next poll to look again (#2218). func (l *Loop) lockedScan(ctx context.Context) (ran bool, err error) { - conn, err := l.db.Conn(ctx) + release, got, err := advisorylock.Try(ctx, l.db, advisorylock.ID(LockKey)) if err != nil { - return false, fmt.Errorf("acquiring a connection for the lock: %w", err) - } - defer func() { _ = conn.Close() }() - var got bool - if err := conn.QueryRowContext(ctx, "SELECT pg_try_advisory_lock($1)", LockKey).Scan(&got); err != nil { - return false, fmt.Errorf("taking the advisory lock: %w", err) + return false, err //nolint:wrapcheck // the lock session names the statement } if !got { return false, bgloop.ErrSkipped } - defer func() { - unlockCtx, cancel := context.WithTimeout(context.Background(), unlockTimeout) - defer cancel() - if _, uerr := conn.ExecContext(unlockCtx, "SELECT pg_advisory_unlock($1)", LockKey); uerr != nil { - slog.WarnContext(ctx, "catalog scan: advisory unlock failed", "error", logsan.SanitizeForLog(uerr.Error())) - } - }() + defer release() for { - due, err := l.due(ctx, conn) + due, err := l.due(ctx) if err != nil || !due { return ran, err } - if _, err := conn.ExecContext(ctx, recordStart); err != nil { + if _, err := l.db.ExecContext(ctx, recordStart); err != nil { return ran, fmt.Errorf("recording the scan's start: %w", err) } if _, err := l.scanner.Scan(ctx); err != nil { @@ -189,13 +176,13 @@ func (l *Loop) lockedScan(ctx context.Context) (ran bool, err error) { // started. // The times are read under the lock, so two replicas cannot both find the // same scan due. -func (l *Loop) due(ctx context.Context, conn *sql.Conn) (bool, error) { +func (l *Loop) due(ctx context.Context) (bool, error) { set, err := l.settings.Get(ctx) if err != nil { return false, fmt.Errorf("reading catalog settings: %w", err) } var last, requested sql.NullTime - err = conn.QueryRowContext(ctx, scanDue).Scan(&last, &requested) + err = l.db.QueryRowContext(ctx, scanDue).Scan(&last, &requested) if err != nil && !errors.Is(err, sql.ErrNoRows) { return false, fmt.Errorf("reading the last scan: %w", err) } diff --git a/internal/platform/knowledgebuiltin/pages/asset-references-and-the-refresh-loop.md b/internal/platform/knowledgebuiltin/pages/asset-references-and-the-refresh-loop.md index 68a339878..15e82204f 100644 --- a/internal/platform/knowledgebuiltin/pages/asset-references-and-the-refresh-loop.md +++ b/internal/platform/knowledgebuiltin/pages/asset-references-and-the-refresh-loop.md @@ -48,7 +48,11 @@ list. Two forms go in it, and they are not interchangeable: The list is the whole declaration. Omitting it leaves the asset's references alone, sending a list replaces them, and sending an empty list removes them -all. At most 20 per asset unless the deployment sets its own cap; the +all. A write that changes the content and the list together grants the newly +listed files before the new version exists, so the version's tile and its +first reader see them, and drops the ones no longer listed once the content is +stored; a write whose content cannot be stored leaves the references as they +were. At most 20 per asset unless the deployment sets its own cap; the `references` field's `maxItems` in the save_asset and manage_asset schemas states this deployment's number. A save above it is refused with the number you declared. diff --git a/internal/platform/levelalert/levelalert.go b/internal/platform/levelalert/levelalert.go index 82fc2fb19..a8e3015ad 100644 --- a/internal/platform/levelalert/levelalert.go +++ b/internal/platform/levelalert/levelalert.go @@ -17,13 +17,12 @@ import ( "database/sql" "errors" "fmt" - "log/slog" "strconv" "strings" "time" + "github.com/txn2/mcp-data-platform/internal/advisorylock" "github.com/txn2/mcp-data-platform/internal/bgloop" - "github.com/txn2/mcp-data-platform/internal/logsan" "github.com/txn2/mcp-data-platform/pkg/notification" ) @@ -224,33 +223,19 @@ func (s *PostgresStore) Delete(ctx context.Context, key string) error { return nil } -// unlockTimeout bounds the unlock after a locked pass, which runs on a context -// that may already be done. -const unlockTimeout = 5 * time.Second - -// Locked runs fn while holding the session advisory lock key on a dedicated -// connection, so one replica evaluates a family at a time. It returns -// bgloop.ErrSkipped when another replica holds the lock. +// Locked runs fn while holding the advisory lock key on the process's lock +// session, so one replica evaluates a family at a time. fn runs on the pool, +// never on the lock's session (#2169). It returns bgloop.ErrSkipped when +// another replica holds the lock. func Locked(ctx context.Context, db *sql.DB, key int64, fn func(ctx context.Context) error) error { - conn, err := db.Conn(ctx) + release, got, err := advisorylock.Try(ctx, db, advisorylock.ID(key)) if err != nil { - return fmt.Errorf("acquiring a connection for the lock: %w", err) - } - defer func() { _ = conn.Close() }() - var got bool - if err := conn.QueryRowContext(ctx, "SELECT pg_try_advisory_lock($1)", key).Scan(&got); err != nil { - return fmt.Errorf("taking the advisory lock: %w", err) + return err //nolint:wrapcheck // the lock session names the statement } if !got { return bgloop.ErrSkipped } - defer func() { - unlockCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), unlockTimeout) - defer cancel() - if _, uerr := conn.ExecContext(unlockCtx, "SELECT pg_advisory_unlock($1)", key); uerr != nil { - slog.WarnContext(ctx, "alert evaluation: advisory unlock failed", "error", logsan.SanitizeForLog(uerr.Error())) - } - }() + defer release() return fn(ctx) } diff --git a/internal/platform/resourcereclass/resourcereclass.go b/internal/platform/resourcereclass/resourcereclass.go index ff61c912b..bb4681ab5 100644 --- a/internal/platform/resourcereclass/resourcereclass.go +++ b/internal/platform/resourcereclass/resourcereclass.go @@ -14,6 +14,7 @@ import ( "github.com/lib/pq" + "github.com/txn2/mcp-data-platform/internal/advisorylock" "github.com/txn2/mcp-data-platform/internal/indexpolicy" "github.com/txn2/mcp-data-platform/pkg/indexjobs" "github.com/txn2/mcp-data-platform/pkg/resource" @@ -76,32 +77,25 @@ type reclassRow struct { // whole walk and answers ErrReclassifyRunning when another holds it. func (r *Reclassifier) Run(ctx context.Context) (ReclassifyResult, error) { var out ReclassifyResult - conn, err := r.db.Conn(ctx) + release, got, err := advisorylock.Try(ctx, r.db, advisorylock.ID(reclassifyLock)) if err != nil { - return out, fmt.Errorf("resourceindex: reclassify connection: %w", err) - } - defer conn.Close() //nolint:errcheck // returning the connection to the pool - var got bool - if err := conn.QueryRowContext(ctx, `SELECT pg_try_advisory_lock($1)`, reclassifyLock).Scan(&got); err != nil { return out, fmt.Errorf("resourceindex: reclassify lock: %w", err) } if !got { return out, ErrReclassifyRunning } - defer func() { - _, _ = conn.ExecContext(context.WithoutCancel(ctx), `SELECT pg_advisory_unlock($1)`, reclassifyLock) - }() + defer release() cursor := "" for { - rows, err := r.batch(ctx, conn, cursor) + rows, err := r.batch(ctx, cursor) if err != nil { return out, err } if len(rows) == 0 { return out, nil } - if err := r.apply(ctx, conn, rows, &out); err != nil { + if err := r.apply(ctx, rows, &out); err != nil { return out, err } cursor = rows[len(rows)-1].id @@ -111,8 +105,8 @@ func (r *Reclassifier) Run(ctx context.Context) (ReclassifyResult, error) { } } -func (*Reclassifier) batch(ctx context.Context, conn *sql.Conn, cursor string) ([]reclassRow, error) { - rows, err := conn.QueryContext(ctx, `SELECT id, index_policy, index_policy_explicit, path, mime_type, +func (r *Reclassifier) batch(ctx context.Context, cursor string) ([]reclassRow, error) { + rows, err := r.db.QueryContext(ctx, `SELECT id, index_policy, index_policy_explicit, path, mime_type, size_bytes, scope, COALESCE(scope_id, ''), uploader_sub, content_text <> '' FROM resources WHERE id > $1 ORDER BY id LIMIT 500`, cursor) if err != nil { @@ -169,13 +163,13 @@ func (r *Reclassifier) plan(rows []reclassRow, out *ReclassifyResult) reclassPla // apply rewrites one batch: each changed row under its new policy, and the // kept text of every row whose policy reads none. -func (r *Reclassifier) apply(ctx context.Context, conn *sql.Conn, rows []reclassRow, out *ReclassifyResult) error { +func (r *Reclassifier) apply(ctx context.Context, rows []reclassRow, out *ReclassifyResult) error { p := r.plan(rows, out) - if err := r.move(ctx, conn, p.changed, out); err != nil { + if err := r.move(ctx, p.changed, out); err != nil { return err } if len(p.stale) > 0 { - if _, err := conn.ExecContext(ctx, `UPDATE resources SET content_text = '' WHERE id = ANY($1)`, + if _, err := r.db.ExecContext(ctx, `UPDATE resources SET content_text = '' WHERE id = ANY($1)`, pq.Array(p.stale)); err != nil { return fmt.Errorf("resourceindex: reclassify clear: %w", err) } @@ -189,10 +183,10 @@ func (r *Reclassifier) apply(ctx context.Context, conn *sql.Conn, rows []reclass // move writes each changed row under its new policy, dropping its vector and // kept text, and enqueues the ones still indexed. -func (r *Reclassifier) move(ctx context.Context, conn *sql.Conn, changed map[indexpolicy.Policy][]string, out *ReclassifyResult) error { +func (r *Reclassifier) move(ctx context.Context, changed map[indexpolicy.Policy][]string, out *ReclassifyResult) error { for policy, ids := range changed { // A writer's choice made after the batch was read is left alone. - res, err := conn.ExecContext(ctx, `UPDATE resources SET index_policy = $1, + res, err := r.db.ExecContext(ctx, `UPDATE resources SET index_policy = $1, content_text = '', content_indexed_at = NULL, embedding = NULL, embedding_model = '', embedding_text_hash = NULL WHERE id = ANY($2) AND NOT index_policy_explicit`, string(policy), pq.Array(ids)) diff --git a/internal/platform/retention/retention.go b/internal/platform/retention/retention.go index e3f7eed65..4cc14c83d 100644 --- a/internal/platform/retention/retention.go +++ b/internal/platform/retention/retention.go @@ -22,6 +22,7 @@ import ( "sync" "time" + "github.com/txn2/mcp-data-platform/internal/advisorylock" "github.com/txn2/mcp-data-platform/internal/bgloop" "github.com/txn2/mcp-data-platform/internal/logsan" ) @@ -51,10 +52,6 @@ func Days(configured, def int) int { } } -// unlockTimeout bounds the advisory unlock after a sweep, so a shutdown -// cannot hang on releasing it. -const unlockTimeout = 5 * time.Second - // Sweep is one removal the loop runs. LockKey is its advisory lock, distinct // from every other maintenance lock in the platform. Run reports how many // rows it removed. @@ -152,29 +149,18 @@ func (l *Loop) RunOnce(ctx context.Context) error { return nil } -// runLocked runs one sweep while holding its advisory lock on a dedicated -// connection, since an advisory lock belongs to the session that took it. -// ran is false when another replica holds the lock. +// runLocked runs one sweep while holding its advisory lock on the process's +// lock session; the sweep runs on the pool (#2169). ran is false when another +// replica holds the lock. func (l *Loop) runLocked(ctx context.Context, s Sweep) (removed int64, ran bool, err error) { - conn, err := l.db.Conn(ctx) + release, got, err := advisorylock.Try(ctx, l.db, advisorylock.ID(s.LockKey)) if err != nil { - return 0, false, fmt.Errorf("acquiring a connection for the lock: %w", err) - } - defer func() { _ = conn.Close() }() - var got bool - if err := conn.QueryRowContext(ctx, "SELECT pg_try_advisory_lock($1)", s.LockKey).Scan(&got); err != nil { - return 0, false, fmt.Errorf("taking the advisory lock: %w", err) + return 0, false, err //nolint:wrapcheck // the lock session names the statement } if !got { return 0, false, nil } - defer func() { - unlockCtx, cancel := context.WithTimeout(context.Background(), unlockTimeout) - defer cancel() - if _, uerr := conn.ExecContext(unlockCtx, "SELECT pg_advisory_unlock($1)", s.LockKey); uerr != nil { - slog.WarnContext(ctx, "retention: advisory unlock failed", "sweep", s.Name, "error", logsan.SanitizeForLog(uerr.Error())) - } - }() + defer release() removed, err = s.Run(ctx) return removed, true, err } diff --git a/internal/platform/retention/retention_test.go b/internal/platform/retention/retention_test.go index 18c28feb5..f9dbb0bf2 100644 --- a/internal/platform/retention/retention_test.go +++ b/internal/platform/retention/retention_test.go @@ -50,12 +50,16 @@ func TestRunOnce(t *testing.T) { mock.ExpectQuery(lockQ).WithArgs(int64(1)).WillReturnRows(sqlmock.NewRows([]string{"l"}).AddRow(true)) mock.ExpectExec(unlockQ).WithArgs(int64(1)).WillReturnResult(sqlmock.NewResult(0, 0)) mock.ExpectQuery(lockQ).WithArgs(int64(2)).WillReturnRows(sqlmock.NewRows([]string{"l"}).AddRow(false)) + mock.ExpectQuery(lockQ).WithArgs(int64(4)).WillReturnError(errors.New("down")) mock.ExpectQuery(lockQ).WithArgs(int64(3)).WillReturnRows(sqlmock.NewRows([]string{"l"}).AddRow(true)) + // An unlock that failed ends the lock session, so it comes last: the + // mock's one connection goes with it. mock.ExpectExec(unlockQ).WithArgs(int64(3)).WillReturnError(errors.New("gone")) - mock.ExpectQuery(lockQ).WithArgs(int64(4)).WillReturnError(errors.New("down")) - New(db, 0, sweep("first", 1, nil), sweep("held", 2, nil), sweep("failing", 3, errors.New("boom")), sweep("unlockable", 4, nil)). + err = New(db, 0, sweep("first", 1, nil), sweep("held", 2, nil), sweep("unlockable", 4, nil), sweep("failing", 3, errors.New("boom"))). RunOnce(context.Background()) + require.ErrorContains(t, err, "boom", "a failing sweep is reported") + require.ErrorContains(t, err, "down", "a lock that could not be taken is reported") assert.Equal(t, []string{"first", "failing"}, ran) assert.NoError(t, mock.ExpectationsWereMet()) diff --git a/internal/platform/storagecap/forecast.go b/internal/platform/storagecap/forecast.go index 3baa99d1a..36b0545c3 100644 --- a/internal/platform/storagecap/forecast.go +++ b/internal/platform/storagecap/forecast.go @@ -11,14 +11,17 @@ const ( longWindow = 30 * 24 * time.Hour day = 24 * time.Hour // minSpan7d and minSpan30d are the shortest stretch of history each rate - // is fitted over (#2201). A new install's first hours are setup, not - // growth: migrations, the built-in pages and their embeddings, the first - // catalog scan. An hour of that read as +112 MB/day, four times the whole - // database. A day of history is the least a weekly rate is read from, and - // a week the least a monthly one is, so the 30-day rate never repeats the - // 7-day one over the same few hours. + // is fitted over (#2201): a day for a weekly rate, and two weeks for a + // monthly one, so the 30-day rate is read from at least twice the history + // the 7-day rate is and never repeats it over the same days (#2203). minSpan7d = day - minSpan30d = 7 * day + minSpan30d = 14 * day + // settle is how much of a store's history is left out of every fit from + // its first sample (#2203). A new install's first hours are setup, not + // growth: migrations, the built-in pages and their embeddings, seeding, + // the first catalog scan. Fitted, they read as +87 MB/day for a database + // of 28 MB that had stopped growing. + settle = day // flatRate is the largest slope read as no growth, in bytes per day: a // store holding one size gives a slope of rounding error, never 0. flatRate = 1.0 @@ -62,16 +65,22 @@ type Forecast struct { // Project fits the store's growth over its history and, with its capacity, // when it fills. points are in time order; the last is the current reading. -func Project(points []Point, now time.Time) Forecast { +// born is when the store's history began, or zero when it began before the +// history kept: a store's first day is left out of every fit. +func Project(points []Point, now, born time.Time) Forecast { + from, wait := time.Time{}, time.Duration(0) + if !born.IsZero() { + from, wait = born.Add(settle), settle + } f := Forecast{ - Rate7dMinHistoryHours: minSpan7d.Hours(), - Rate30dMinHistoryHours: minSpan30d.Hours(), + Rate7dMinHistoryHours: (wait + minSpan7d).Hours(), + Rate30dMinHistoryHours: (wait + minSpan30d).Hours(), } if len(points) > 1 { f.HistoryHours = points[len(points)-1].At.Sub(points[0].At).Hours() } - f.Rate7d = rateOver(points, now.Add(-shortWindow), minSpan7d) - f.Rate30d = rateOver(points, now.Add(-longWindow), minSpan30d) + f.Rate7d = rateOver(points, latest(now.Add(-shortWindow), from), minSpan7d) + f.Rate30d = rateOver(points, latest(now.Add(-longWindow), from), minSpan30d) rate, used := slower(f.Rate7d, f.Rate30d) if rate == nil || len(points) == 0 { return f @@ -93,6 +102,14 @@ func Project(points []Point, now time.Time) Forecast { return f } +// latest is the later of two times. +func latest(a, b time.Time) time.Time { + if b.After(a) { + return b + } + return a +} + // slower is the smaller of the growing rates and which it is; nil when // neither grows. func slower(r7, r30 *float64) (rate *float64, name string) { diff --git a/internal/platform/storagecap/forecast_test.go b/internal/platform/storagecap/forecast_test.go index 5fa1c35a1..13f0c671f 100644 --- a/internal/platform/storagecap/forecast_test.go +++ b/internal/platform/storagecap/forecast_test.go @@ -32,7 +32,7 @@ func TestProjectMatchesTheLinearAnswer(t *testing.T) { total = int64(1 << 40) ) points := linear(30, interval, 100<<30, perDay, total) - f := Project(points, now) + f := Project(points, now, time.Time{}) require.NotNil(t, f.ExhaustionAt) last := points[len(points)-1] want := now.Add(time.Duration(float64(total-last.Used) / float64(perDay) * float64(day))) @@ -55,7 +55,7 @@ func TestProjectUsesTheSlowerRate(t *testing.T) { slow[i].Used += int64(float64(9<<30) * (7 - age.Hours()/24)) } } - f := Project(slow, now) + f := Project(slow, now, time.Time{}) require.NotNil(t, f.Rate7d) require.NotNil(t, f.Rate30d) assert.Less(t, *f.Rate30d, *f.Rate7d) @@ -63,20 +63,20 @@ func TestProjectUsesTheSlowerRate(t *testing.T) { // Only a week of history: the 7-day rate is all there is. week := linear(6, time.Hour, 0, 1<<30, 1<<40) - f = Project(week, now) + f = Project(week, now, time.Time{}) assert.Equal(t, "7d", f.RateUsed) } func TestProjectEdges(t *testing.T) { - empty := Forecast{Rate7dMinHistoryHours: 24, Rate30dMinHistoryHours: 168} - assert.Equal(t, empty, Project(nil, now)) + empty := Forecast{Rate7dMinHistoryHours: 24, Rate30dMinHistoryHours: 336} + assert.Equal(t, empty, Project(nil, now, time.Time{})) single := []Point{{At: now, Used: 1, Total: new(int64(10))}} - assert.Equal(t, empty, Project(single, now), "one sample has no rate") + assert.Equal(t, empty, Project(single, now, time.Time{}), "one sample has no rate") short := []Point{{At: now.Add(-time.Minute), Used: 1}, {At: now, Used: 2}} - assert.Nil(t, Project(short, now).Rate7d, "a minute of history is too short to fit") + assert.Nil(t, Project(short, now, time.Time{}).Rate7d, "a minute of history is too short to fit") shrinking := linear(10, time.Hour, 1<<40, -(1 << 30), 1<<41) - f := Project(shrinking, now) + f := Project(shrinking, now, time.Time{}) require.NotNil(t, f.Rate7d) assert.Negative(t, *f.Rate7d) assert.Nil(t, f.ExhaustionAt, "a store that is not growing does not fill") @@ -86,12 +86,12 @@ func TestProjectEdges(t *testing.T) { for i := range noCapacity { noCapacity[i].Total = nil } - f = Project(noCapacity, now) + f = Project(noCapacity, now, time.Time{}) assert.NotEmpty(t, f.RateUsed) assert.Nil(t, f.DaysLeft, "with no capacity there is no date") full := linear(10, time.Hour, 1<<40, 1<<30, 1<<30) - f = Project(full, now) + f = Project(full, now, time.Time{}) require.NotNil(t, f.DaysLeft) assert.Zero(t, *f.DaysLeft, "a store past its capacity has no days left") @@ -103,7 +103,7 @@ func TestProjectEdges(t *testing.T) { // TestProjectWaitsForEnoughHistory is #2201's first finding: a new install's // setup burst over its first hour and a half is no daily rate, and the 30-day -// rate waits for a week so it never repeats the 7-day one. +// rate waits for two weeks so it never repeats the 7-day one (#2203). func TestProjectWaitsForEnoughHistory(t *testing.T) { // The samples #2201 was reported from: 16 MB to 28 MB in 1h42m. burst := make([]Point, 0, 16) @@ -112,28 +112,78 @@ func TestProjectWaitsForEnoughHistory(t *testing.T) { at := from.Add(time.Duration(i) * 102 * time.Minute / 15) burst = append(burst, Point{At: at, Used: 16_075_923 + int64(i)*(28_011_667-16_075_923)/15}) } - f := Project(burst, now) + f := Project(burst, now, time.Time{}) assert.Nil(t, f.Rate7d, "an hour and a half is no weekly rate") assert.Nil(t, f.Rate30d) assert.Empty(t, f.RateUsed) assert.InDelta(t, 1.7, f.HistoryHours, 0.01) assert.Equal(t, 24.0, f.Rate7dMinHistoryHours) - assert.Equal(t, 168.0, f.Rate30dMinHistoryHours) + assert.Equal(t, 336.0, f.Rate30dMinHistoryHours) three := linear(3, time.Hour, 0, 1<<30, 1<<40) - f = Project(three, now) + f = Project(three, now, time.Time{}) require.NotNil(t, f.Rate7d, "three days is enough for the weekly rate") assert.Nil(t, f.Rate30d, "and not for the monthly one") assert.Equal(t, "7d", f.RateUsed) assert.InDelta(t, 72.0, f.HistoryHours, 0.01) + + // Ten days fill the weekly window and are still no monthly rate: the two + // would be fitted over nearly the same days. + ten := linear(10, time.Hour, 0, 1<<30, 1<<40) + f = Project(ten, now, time.Time{}) + require.NotNil(t, f.Rate7d) + assert.Nil(t, f.Rate30d, "ten days is no monthly rate") + f = Project(linear(15, time.Hour, 0, 1<<30, 1<<40), now, time.Time{}) + assert.NotNil(t, f.Rate30d, "fifteen days is") +} + +// firstBoot is #2203's store: 16 MB to 28 MB over its first half hour of +// setup, then flat, sampled every fifteen minutes for span. +func firstBoot(span time.Duration) []Point { + from := now.Add(-span) + var out []Point + for at := from; !at.After(now); at = at.Add(15 * time.Minute) { + used := int64(28_000_000) + if age := at.Sub(from); age < 30*time.Minute { + used = 16_000_000 + int64(12_000_000*age.Minutes()/30) + } + out = append(out, Point{At: at, Used: used, Total: new(int64(1 << 30))}) + } + return out +} + +// TestProjectLeavesTheFirstDayOut is #2203: a new install's setup is no +// growth. Fitted from its first sample, a store that climbed for half an hour +// and then held its size reads as growing; with its first day left out it +// reads as flat, gives no fill date, and says it needs a day more history. +func TestProjectLeavesTheFirstDayOut(t *testing.T) { + young := firstBoot(150 * time.Minute) + born := young[0].At + f := Project(young, now, born) + assert.Nil(t, f.Rate7d, "two and a half hours is no rate") + assert.Nil(t, f.ExhaustionAt) + assert.Equal(t, 48.0, f.Rate7dMinHistoryHours, "the first day and then a day") + assert.Equal(t, 360.0, f.Rate30dMinHistoryHours, "the first day and then two weeks") + + days := firstBoot(3 * day) + fitted := Project(days, now, time.Time{}) + require.NotNil(t, fitted.Rate7d) + assert.Positive(t, *fitted.Rate7d, "fitted from the first sample, setup reads as growth") + require.NotNil(t, fitted.ExhaustionAt) + + f = Project(days, now, days[0].At) + require.NotNil(t, f.Rate7d) + assert.Zero(t, *f.Rate7d, "after the first day the store holds its size") + assert.Nil(t, f.ExhaustionAt, "and has no fill date") + assert.Empty(t, f.RateUsed) } // TestProjectFlatStoreIsZero is #2201's second finding: a store holding one // size at every sample grows at exactly zero, not at the rounding error of // sums near 1e18. func TestProjectFlatStoreIsZero(t *testing.T) { - flat := linear(10, 7*time.Minute, 16_801_792, 0, 1<<40) - f := Project(flat, now) + flat := linear(15, 7*time.Minute, 16_801_792, 0, 1<<40) + f := Project(flat, now, time.Time{}) require.NotNil(t, f.Rate7d) require.NotNil(t, f.Rate30d) assert.Zero(t, *f.Rate7d) @@ -142,7 +192,7 @@ func TestProjectFlatStoreIsZero(t *testing.T) { // A big store gaining a little still reads its growth. creep := linear(10, 7*time.Minute, 1<<40, 4096, 1<<41) - f = Project(creep, now) + f = Project(creep, now, time.Time{}) require.NotNil(t, f.Rate7d) assert.InDelta(t, 4096, *f.Rate7d, 64) } @@ -166,7 +216,7 @@ func TestSlower(t *testing.T) { // its days left and no date, rather than one overflowed into the past. func TestProjectFarOffHasNoDate(t *testing.T) { points := linear(30, time.Hour, 0, 1<<20, 1<<50) - f := Project(points, now) + f := Project(points, now, time.Time{}) require.NotNil(t, f.DaysLeft) assert.Greater(t, *f.DaysLeft, 106752.0) assert.Nil(t, f.ExhaustionAt) diff --git a/internal/platform/storagecap/service.go b/internal/platform/storagecap/service.go index 7a4bb60b3..4ee7ab7fa 100644 --- a/internal/platform/storagecap/service.go +++ b/internal/platform/storagecap/service.go @@ -128,8 +128,16 @@ func (s *Service) Stop() { } // Pass is one iteration: read every store, record a sample of each, drop the -// history past the settings' period, and evaluate the alerts. +// history past the settings' period, and evaluate the alerts. A pass within +// most of the interval of the last sample is skipped: another replica took it. func (s *Service) Pass(ctx context.Context) error { + recent, err := s.sampledRecently(ctx) + if err != nil { + return err + } + if recent { + return bgloop.ErrSkipped + } settings, err := Read(ctx, s.d.Settings) if err != nil { return err @@ -152,6 +160,24 @@ func (s *Service) Pass(ctx context.Context) error { return nil } +// recentFraction is the share of the interval a sample counts as recent for: +// nine tenths, so a timer that fires a little early does not skip its turn. +const recentFraction = 0.9 + +// sampledRecently reports whether a sample was taken within most of the +// interval (#2203). Every replica runs the loop on its own timer, and the lock +// is held only for a pass, so a replica whose turn came just after another's +// pass would otherwise sample the same tick again. +func (s *Service) sampledRecently(ctx context.Context) (bool, error) { + since := s.d.Now().UTC().Add(-time.Duration(float64(s.d.Interval) * recentFraction)) + var recent bool + if err := s.d.DB.QueryRowContext(ctx, + `SELECT EXISTS (SELECT 1 FROM storage_samples WHERE sampled_at > $1)`, since).Scan(&recent); err != nil { + return false, fmt.Errorf(errReadingHistory, err) + } + return recent, nil +} + // spoolGone is how long a replica's spool goes unreported before its volume // is taken to be gone rather than unread: a replica replaced in a rollout // does not keep its alerts open. @@ -366,6 +392,10 @@ func (s *Service) project(ctx context.Context, r *Report) error { if err != nil { return err } + born, err := s.births(ctx, r.GeneratedAt.Add(-time.Duration(r.HistoryDays)*day+day)) + if err != nil { + return err + } for i := range r.Stores { st := &r.Stores[i] if st.TotalBytes != nil { @@ -374,11 +404,38 @@ func (s *Service) project(ctx context.Context, r *Report) error { st.FreeBytes, st.PercentUsed = &free, &pct } points := slices.Concat(history[st.Store], []Point{{At: r.GeneratedAt, Used: st.UsedBytes, Total: st.TotalBytes}}) - st.Forecast = Project(points, r.GeneratedAt) + st.Forecast = Project(points, r.GeneratedAt, born[st.Store]) } return nil } +// births is when each store's history began, for the stores whose first +// sample is after kept: a store whose first sample is older may have had its +// first days pruned, and its earliest sample kept is not its first. +func (s *Service) births(ctx context.Context, kept time.Time) (map[string]time.Time, error) { + rows, err := s.d.DB.QueryContext(ctx, + `SELECT store, min(sampled_at) FROM storage_samples GROUP BY store HAVING min(sampled_at) > $1`, kept) + if err != nil { + return nil, fmt.Errorf(errReadingHistory, err) + } + defer func() { _ = rows.Close() }() + out := map[string]time.Time{} + for rows.Next() { + var ( + store string + first time.Time + ) + if err := rows.Scan(&store, &first); err != nil { + return nil, fmt.Errorf("scanning storage history: %w", err) + } + out[store] = first.UTC() + } + if err := rows.Err(); err != nil { + return nil, fmt.Errorf(errReadingHistory, err) + } + return out, nil +} + // historySince is every store's samples since since, oldest first. func (s *Service) historySince(ctx context.Context, since time.Time) (map[string][]Point, error) { rows, err := s.d.DB.QueryContext(ctx, diff --git a/internal/platform/storagecap/service_realdb_integration_test.go b/internal/platform/storagecap/service_realdb_integration_test.go index 29da54ef0..beb934afa 100644 --- a/internal/platform/storagecap/service_realdb_integration_test.go +++ b/internal/platform/storagecap/service_realdb_integration_test.go @@ -10,6 +10,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/txn2/mcp-data-platform/internal/bgloop" "github.com/txn2/mcp-data-platform/internal/platform/levelalert" "github.com/txn2/mcp-data-platform/internal/testdb" "github.com/txn2/mcp-data-platform/internal/webhook/whstore" @@ -64,3 +65,44 @@ func TestPassRealDB(t *testing.T) { require.NoError(t, err) assert.Equal(t, map[string]int64{"portal": 1000}, budgetsRead) } + +// TestPassOncePerTickRealDB is #2203's fourth item: two replicas whose +// timers fire in the same tick take one sample between them, the second +// finding the first's; a tick an interval later samples again. The store's +// first sample is its birth, and its first day is left out of the fit. +func TestPassOncePerTickRealDB(t *testing.T) { + db := testdb.New(t) + ctx := context.Background() + settings := NewSettingsStore(db) + at := time.Date(2026, 10, 20, 3, 24, 7, 0, time.UTC) + replica := func() *Service { + return New(Deps{ + DB: db, Settings: settings, Engine: levelalert.New(levelalert.NewPostgresStore(db), nil), + Interval: 15 * time.Minute, Now: func() time.Time { return at }, + }) + } + a, b := replica(), replica() + samples := func() int { + var n int + require.NoError(t, db.QueryRowContext(ctx, `SELECT count(*) FROM storage_samples WHERE store = 'database'`).Scan(&n)) + return n + } + + require.NoError(t, a.Pass(ctx)) + at = at.Add(400 * time.Millisecond) + require.ErrorIs(t, b.Pass(ctx), bgloop.ErrSkipped, "the second replica in one tick") + assert.Equal(t, 1, samples()) + + at = at.Add(15 * time.Minute) + require.NoError(t, b.Pass(ctx)) + assert.Equal(t, 2, samples(), "the next tick samples again") + + r, err := b.Report(ctx) + require.NoError(t, err) + for _, s := range r.Stores { + if s.Store == "database" { + assert.Nil(t, s.Forecast.Rate7d) + assert.Equal(t, 48.0, s.Forecast.Rate7dMinHistoryHours, "a store born a quarter hour ago waits out its first day") + } + } +} diff --git a/internal/platform/storagecap/service_test.go b/internal/platform/storagecap/service_test.go index e32cb1fe9..4982a4b4c 100644 --- a/internal/platform/storagecap/service_test.go +++ b/internal/platform/storagecap/service_test.go @@ -12,6 +12,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/txn2/mcp-data-platform/internal/bgloop" "github.com/txn2/mcp-data-platform/internal/platform/levelalert" "github.com/txn2/mcp-data-platform/internal/platform/settingsjson" "github.com/txn2/mcp-data-platform/internal/webhook/whstore" @@ -66,6 +67,18 @@ func expectReadings(mock sqlmock.Sqlmock, dbBytes int64) { WillReturnRows(sqlmock.NewRows([]string{"store", "at", "used", "total"}). AddRow("database", now.Add(-10*day), int64(500), int64(1000)). AddRow("database", now.Add(-5*day), int64(700), nil)) + mock.ExpectQuery(`SELECT store, min\(sampled_at\) FROM storage_samples GROUP BY store`). + WithArgs(now.Add(-DefaultHistoryDays*day + day)). + WillReturnRows(sqlmock.NewRows([]string{"store", "first"}).AddRow("database", now.Add(-10*day))) +} + +// recentQ is the check a pass makes for a sample another replica took. +const recentQ = `SELECT EXISTS \(SELECT 1 FROM storage_samples WHERE sampled_at > \$1\)` + +// expectNotSampled queues the check finding no recent sample. +func expectNotSampled(mock sqlmock.Sqlmock) { + mock.ExpectQuery(recentQ).WithArgs(now.Add(-DefaultInterval * 9 / 10)). + WillReturnRows(sqlmock.NewRows([]string{"recent"}).AddRow(false)) } // svcRig is a Service over sqlmock with the sender and alert store it routes @@ -109,6 +122,7 @@ func TestPassSamplesAndAlerts(t *testing.T) { } rig := newSvc(t, settings, spools) svc, mock, sender, alerts := rig.svc, rig.mock, rig.sender, rig.alerts + expectNotSampled(mock) expectReadings(mock, 950) for range 5 { mock.ExpectExec(`INSERT INTO storage_samples`).WillReturnResult(sqlmock.NewResult(0, 1)) @@ -127,6 +141,7 @@ func TestPassSamplesAndAlerts(t *testing.T) { // The next pass finds the database back under its thresholds: resolved. sender.keys = nil settings.stored.Value.DatabaseLimitBytes = 100000 + expectNotSampled(mock) expectReadings(mock, 950) for range 5 { mock.ExpectExec(`INSERT INTO storage_samples`).WillReturnResult(sqlmock.NewResult(0, 1)) @@ -192,7 +207,7 @@ func TestReportClusterFailureIsShownNotFatal(t *testing.T) { func TestReportFailures(t *testing.T) { ctx := context.Background() boom := errors.New("down") - steps := []string{"size", "tables", "usage", "reconcile", "scan", "spools", "history"} + steps := []string{"size", "tables", "usage", "reconcile", "scan", "spools", "history", "births"} for i, step := range steps { t.Run(step, func(t *testing.T) { spools := func(context.Context) ([]whstore.SpoolState, error) { @@ -205,6 +220,7 @@ func TestReportFailures(t *testing.T) { svc, mock := rig.svc, rig.mock queries := []string{ `pg_database_size`, `FROM pg_class`, `FROM storage_usage`, `FROM storage_reconciles`, `FROM storage_scans`, "", `FROM storage_samples`, + `min\(sampled_at\)`, } for j := range i { if queries[j] == "" { @@ -239,6 +255,8 @@ func expectStep(mock sqlmock.Sqlmock, j int) { mock.ExpectQuery(`FROM storage_reconciles`).WillReturnRows(sqlmock.NewRows([]string{"a", "b", "c", "d"})) case 4: mock.ExpectQuery(`FROM storage_scans`).WillReturnRows(sqlmock.NewRows([]string{"a", "b", "c"})) + case 6: + mock.ExpectQuery(`FROM storage_samples`).WillReturnRows(sqlmock.NewRows([]string{"a", "b", "c", "d"})) } } @@ -246,12 +264,19 @@ func TestPassFailures(t *testing.T) { ctx := context.Background() boom := errors.New("down") t.Run("settings", func(t *testing.T) { - svc := newSvc(t, &fakeSettings{err: boom}, nil).svc - require.Error(t, svc.Pass(ctx)) + rig := newSvc(t, &fakeSettings{err: boom}, nil) + expectNotSampled(rig.mock) + require.Error(t, rig.svc.Pass(ctx)) + }) + t.Run("recent", func(t *testing.T) { + rig := newSvc(t, &fakeSettings{}, nil) + rig.mock.ExpectQuery(recentQ).WillReturnError(boom) + require.ErrorContains(t, rig.svc.Pass(ctx), "reading storage history") }) t.Run("record", func(t *testing.T) { rig := newSvc(t, &fakeSettings{}, nil) svc, mock := rig.svc, rig.mock + expectNotSampled(mock) expectReadings(mock, 1) mock.ExpectExec(`INSERT INTO storage_samples`).WillReturnError(boom) require.ErrorContains(t, svc.Pass(ctx), "recording a storage sample") @@ -259,6 +284,7 @@ func TestPassFailures(t *testing.T) { t.Run("prune", func(t *testing.T) { rig := newSvc(t, &fakeSettings{}, nil) svc, mock := rig.svc, rig.mock + expectNotSampled(mock) expectReadings(mock, 1) for range 3 { mock.ExpectExec(`INSERT INTO storage_samples`).WillReturnResult(sqlmock.NewResult(0, 1)) @@ -269,11 +295,41 @@ func TestPassFailures(t *testing.T) { t.Run("report", func(t *testing.T) { rig := newSvc(t, &fakeSettings{}, nil) svc, mock := rig.svc, rig.mock + expectNotSampled(mock) mock.ExpectQuery(`pg_database_size`).WillReturnError(boom) require.Error(t, svc.Pass(ctx)) }) } +// TestPassSkipsATickAnotherReplicaSampled is #2203: the lock is held only for +// a pass, so a replica whose turn comes just after another's finds that +// replica's sample and takes none of its own, and evaluates no alert. +func TestPassSkipsATickAnotherReplicaSampled(t *testing.T) { + rig := newSvc(t, &fakeSettings{}, nil) + rig.mock.ExpectQuery(recentQ).WithArgs(now.Add(-DefaultInterval * 9 / 10)). + WillReturnRows(sqlmock.NewRows([]string{"recent"}).AddRow(true)) + require.ErrorIs(t, rig.svc.Pass(context.Background()), bgloop.ErrSkipped) + require.NoError(t, rig.mock.ExpectationsWereMet()) + assert.Empty(t, rig.sender.keys) +} + +// TestBirthsFailures: a store's first sample is its birth only +// when nothing before it could have been pruned; a failed read is an error. +func TestBirthsFailures(t *testing.T) { + boom := errors.New("down") + rig := newSvc(t, &fakeSettings{}, nil) + rig.mock.ExpectQuery(`min\(sampled_at\)`).WillReturnError(boom) + _, err := rig.svc.births(context.Background(), now) + require.Error(t, err) + rig.mock.ExpectQuery(`min\(sampled_at\)`).WillReturnRows(sqlmock.NewRows([]string{"store"}).AddRow("database")) + _, err = rig.svc.births(context.Background(), now) + require.Error(t, err) + rig.mock.ExpectQuery(`min\(sampled_at\)`).WillReturnRows( + sqlmock.NewRows([]string{"store", "first"}).AddRow("database", now).RowError(0, boom)) + _, err = rig.svc.births(context.Background(), now) + require.Error(t, err) +} + func TestHistory(t *testing.T) { ctx := context.Background() rig := newSvc(t, &fakeSettings{}, nil) diff --git a/internal/portal/assetrefs/declare.go b/internal/portal/assetrefs/declare.go index 77fed93bc..a99ad934c 100644 --- a/internal/portal/assetrefs/declare.go +++ b/internal/portal/assetrefs/declare.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "log/slog" + "slices" "strings" "github.com/txn2/mcp-data-platform/internal/logsan" @@ -268,6 +269,73 @@ func (d *Declarer) Apply( return refs, nil } +// Widen records every declared reference the asset does not have yet beside +// the ones it has, ahead of a write whose new content uses them (#2234). The +// new version is what makes the asset owed a tile, and the tile worker may draw +// it the moment it exists, so its files must already be granted then; the +// references the write drops are dropped by the Apply after it, since the +// content still current until then uses them. Each added reference gets the +// token Apply keeps. +// +// restore puts the references back as they were, for a write that failed; it +// does nothing when Widen added nothing. +func (d *Declarer) Widen( + ctx context.Context, assetID string, declared []Declared, author string, +) (restore func(context.Context) error, err error) { + if !d.Available() { + return nil, errNoRefStore() + } + existing, err := d.refs.ListByAsset(ctx, assetID) + if err != nil { + return nil, fmt.Errorf("reading existing references: %w", err) + } + added, err := missingRefs(assetID, existing, declared, author) + if err != nil { + return nil, err + } + if len(added) == 0 { + return func(context.Context) error { return nil }, nil + } + if err := d.refs.Replace(ctx, assetID, slices.Concat(existing, added)); err != nil { + return nil, fmt.Errorf("recording references: %w", err) + } + for _, ref := range added { + LogGranted(assetID, ref) + } + return func(ctx context.Context) error { + if err := d.refs.Replace(ctx, assetID, existing); err != nil { + return fmt.Errorf("restoring references: %w", err) + } + return nil + }, nil +} + +// missingRefs is the declared references existing does not hold, each with a +// new token, positioned after existing. +func missingRefs(assetID string, existing []Ref, declared []Declared, author string) ([]Ref, error) { + have := make(map[string]bool, len(existing)) + for _, ref := range existing { + have[refKey(ref.TargetKind, ref.TargetID)] = true + } + var added []Ref + for _, dec := range declared { + key := refKey(dec.Kind, dec.TargetID) + if have[key] { + continue + } + have[key] = true + token, err := tokenFor(nil, key) + if err != nil { + return nil, err + } + added = append(added, Ref{ + AssetID: assetID, TargetKind: dec.Kind, TargetID: dec.TargetID, URI: dec.URI, + RefToken: token, Position: len(existing) + len(added), DeclaredBy: author, + }) + } + return added, nil +} + // DeclaredURIs returns the reference strings an asset declares now, in declared // order: what a write that leaves the references alone is judged against when // it reports the references its content names undeclared (#1875). A declarer diff --git a/internal/portal/assetrefs/declare_test.go b/internal/portal/assetrefs/declare_test.go index 2d77ee390..b6550a511 100644 --- a/internal/portal/assetrefs/declare_test.go +++ b/internal/portal/assetrefs/declare_test.go @@ -816,3 +816,64 @@ func TestConfig(t *testing.T) { require.Error(t, bad.Validate()) assert.Equal(t, assetrefs.DefaultMaxRefs, bad.Resolved()) } + +// TestWidenAddsWithoutDropping is #2234: ahead of a content write, the +// references it declares are recorded beside the ones the asset has, keeping +// their tokens; restore puts the list back; a declaration that adds nothing +// writes nothing; the token Widen minted is the one Apply keeps. +func TestWidenAddsWithoutDropping(t *testing.T) { + store := newFakeRefs() + d := declarer(store) + logo := assetrefs.Declared{Kind: assetrefs.TargetResource, TargetID: "res-logo", URI: logoURI} + other := assetrefs.Declared{Kind: assetrefs.TargetResource, TargetID: "res-other", URI: "mcp://global/brand/other.png"} + first, err := d.Apply(t.Context(), testAssetID, []assetrefs.Declared{logo}, author) + require.NoError(t, err) + + restore, err := d.Widen(t.Context(), testAssetID, []assetrefs.Declared{other, other}, author) + require.NoError(t, err) + require.NotNil(t, restore) + widened := store.byAsset[testAssetID] + require.Len(t, widened, 2, "the kept reference and the added one, once") + assert.Equal(t, first[0].RefToken, widened[0].RefToken) + assert.Equal(t, "res-other", widened[1].TargetID) + assert.Equal(t, 1, widened[1].Position) + + applied, err := d.Apply(t.Context(), testAssetID, []assetrefs.Declared{other}, author) + require.NoError(t, err) + assert.Equal(t, widened[1].RefToken, applied[0].RefToken, "Apply keeps the token Widen minted") + + require.NoError(t, restore(t.Context())) + require.Len(t, store.byAsset[testAssetID], 1) + assert.Equal(t, "res-logo", store.byAsset[testAssetID][0].TargetID) + + none, err := d.Widen(t.Context(), testAssetID, []assetrefs.Declared{logo}, author) + require.NoError(t, err) + assert.Len(t, store.byAsset[testAssetID], 1, "a declaration the asset already holds adds nothing") + require.NoError(t, none(t.Context())) + assert.Len(t, store.byAsset[testAssetID], 1, "and restoring it changes nothing") +} + +// TestWidenReportsStoreFailures: every store failure is reported, restore's +// included, and an unavailable declarer refuses. +func TestWidenReportsStoreFailures(t *testing.T) { + declared := []assetrefs.Declared{{Kind: assetrefs.TargetResource, TargetID: "res-logo", URI: logoURI}} + + _, err := (*assetrefs.Declarer)(nil).Widen(t.Context(), testAssetID, declared, author) + require.Error(t, err) + + listFails := newFakeRefs() + listFails.listErr = errStore + _, err = declarer(listFails).Widen(t.Context(), testAssetID, declared, author) + require.ErrorIs(t, err, errStore) + + writeFails := newFakeRefs() + writeFails.putErr = errStore + _, err = declarer(writeFails).Widen(t.Context(), testAssetID, declared, author) + require.ErrorIs(t, err, errStore) + + restoreFails := newFakeRefs() + restore, err := declarer(restoreFails).Widen(t.Context(), testAssetID, declared, author) + require.NoError(t, err) + restoreFails.putErr = errStore + require.ErrorIs(t, restore(t.Context()), errStore) +} diff --git a/internal/thumbdue/thumbdue.go b/internal/thumbdue/thumbdue.go index 25acedfdf..a25155601 100644 --- a/internal/thumbdue/thumbdue.go +++ b/internal/thumbdue/thumbdue.go @@ -20,10 +20,8 @@ import ( "encoding/hex" "fmt" "io" - "log/slog" - "time" - "github.com/txn2/mcp-data-platform/internal/logsan" + "github.com/txn2/mcp-data-platform/internal/advisorylock" ) // NotifyChannel is the channel the trigger notifies when a row becomes due. @@ -44,9 +42,6 @@ const ( // holds row locks on more than this many rows at a time. const chunk = 2000 -// unlockTimeout bounds the advisory unlock after a sweep. -const unlockTimeout = 5 * time.Second - // The advisory locks the sweeps run under, one per table, so two replicas // starting together sweep a table once. const ( @@ -97,44 +92,33 @@ func Sweep(ctx context.Context, db *sql.DB, table Table, rule string) (int64, er if err != nil { return 0, err } - conn, err := db.Conn(ctx) + release, got, err := advisorylock.Try(ctx, db, advisorylock.ID(key)) if err != nil { - return 0, fmt.Errorf("thumbdue: acquiring a connection for the sweep: %w", err) - } - defer func() { _ = conn.Close() }() - var got bool - if err := conn.QueryRowContext(ctx, "SELECT pg_try_advisory_lock($1)", key).Scan(&got); err != nil { return 0, fmt.Errorf("thumbdue: taking the sweep lock: %w", err) } if !got { return 0, nil } - defer func() { - unlockCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), unlockTimeout) - defer cancel() - if _, uerr := conn.ExecContext(unlockCtx, "SELECT pg_advisory_unlock($1)", key); uerr != nil { - slog.WarnContext(ctx, "thumbnails: sweep unlock failed", "table", string(table), "error", logsan.SanitizeForLog(uerr.Error())) - } - }() - return sweepLocked(ctx, conn, table, rule) + defer release() + return sweepLocked(ctx, db, table, rule) } // sweepLocked is Sweep once its lock is held: mark the table unless it was // swept under rule, then record rule. -func sweepLocked(ctx context.Context, conn *sql.Conn, table Table, rule string) (int64, error) { +func sweepLocked(ctx context.Context, db *sql.DB, table Table, rule string) (int64, error) { var recorded string - err := conn.QueryRowContext(ctx, `SELECT rule FROM thumbnail_sweeps WHERE subject = $1`, string(table)).Scan(&recorded) + err := db.QueryRowContext(ctx, `SELECT rule FROM thumbnail_sweeps WHERE subject = $1`, string(table)).Scan(&recorded) if err != nil && err != sql.ErrNoRows { //nolint:errorlint // database/sql returns ErrNoRows unwrapped return 0, fmt.Errorf("thumbdue: reading the sweep record: %w", err) } if recorded == rule { return 0, nil } - marked, err := markAll(ctx, conn, table) + marked, err := markAll(ctx, db, table) if err != nil { return marked, err } - if _, err := conn.ExecContext(ctx, ` + if _, err := db.ExecContext(ctx, ` INSERT INTO thumbnail_sweeps (subject, rule, swept_at) VALUES ($1, $2, now()) ON CONFLICT (subject) DO UPDATE SET rule = EXCLUDED.rule, swept_at = EXCLUDED.swept_at`, string(table), rule); err != nil { @@ -144,7 +128,7 @@ func sweepLocked(ctx context.Context, conn *sql.Conn, table Table, rule string) } // markAll marks table due chunk rows at a time, walking its primary key. -func markAll(ctx context.Context, conn *sql.Conn, table Table) (int64, error) { +func markAll(ctx context.Context, db *sql.DB, table Table) (int64, error) { query := markQuery(table) var marked int64 after := "" @@ -152,7 +136,7 @@ func markAll(ctx context.Context, conn *sql.Conn, table Table) (int64, error) { var n int64 var last sql.NullString // #nosec G701 -- table is one of the Table constants; every value is bound - if err := conn.QueryRowContext(ctx, query, after, chunk).Scan(&n, &last); err != nil { + if err := db.QueryRowContext(ctx, query, after, chunk).Scan(&n, &last); err != nil { return marked, fmt.Errorf("thumbdue: marking %s due: %w", string(table), err) } marked += n diff --git a/internal/upstreamauth/googlekey/googlekey.go b/internal/upstreamauth/googlekey/googlekey.go index 7a08e8220..a1c321fa0 100644 --- a/internal/upstreamauth/googlekey/googlekey.go +++ b/internal/upstreamauth/googlekey/googlekey.go @@ -153,7 +153,13 @@ type apiError struct { // APIHint is the operator's next step for a Google API refusing a call a // service-account connection made, or "" when the response is not one. A valid // token is not access: the API must be enabled in the account's Cloud project, -// and the account granted inside the product. body is the decoded response. +// the token must carry the API's scope, and the account must be granted inside +// the product. body is the decoded response. +// +// Every Google API refuses in one shape (error.code, error.status, +// error.details[].reason), and the token was issued by Google a moment before, +// so a 401 or 403 in that shape is read by its status and reasons alone, never +// by one product's message text (#2237). func APIHint(status int, body any) string { if status != http.StatusUnauthorized && status != http.StatusForbidden { return "" @@ -163,19 +169,19 @@ func APIHint(status int, body any) string { return "" } var e apiError - if json.Unmarshal(raw, &e) != nil { + if json.Unmarshal(raw, &e) != nil || e.Error.Status == "" { return "" } reasons := make([]string, 0, len(e.Error.Details)) for _, d := range e.Error.Details { reasons = append(reasons, d.Reason) } - msg := strings.ToLower(e.Error.Message) switch { case slices.Contains(reasons, "SERVICE_DISABLED"): return "the API is not enabled in the service account's Google Cloud project; enable it there (the error names the project and the API)" - case e.Error.Status == "PERMISSION_DENIED" || strings.Contains(msg, "cannot get profile"): + case slices.Contains(reasons, "ACCESS_TOKEN_SCOPE_INSUFFICIENT"): + return "the token does not carry the scope this API needs; add it to the connection's oauth_scope" + default: return "the token is valid but the service account has no access in this product; grant the service account's email access inside the product" } - return "" } diff --git a/internal/upstreamauth/googlekey/googlekey_test.go b/internal/upstreamauth/googlekey/googlekey_test.go index 265e6719a..df8e42ff6 100644 --- a/internal/upstreamauth/googlekey/googlekey_test.go +++ b/internal/upstreamauth/googlekey/googlekey_test.go @@ -118,19 +118,23 @@ func TestAPIHint(t *testing.T) { } disabled := decode(`{"error":{"code":403,"message":"Google Sheets API has not been used in project 1234567890 before or it is disabled.","status":"PERMISSION_DENIED","details":[{"@type":"type.googleapis.com/google.rpc.ErrorInfo","reason":"SERVICE_DISABLED"}]}}`) denied := decode(`{"error":{"code":403,"message":"The caller does not have permission","status":"PERMISSION_DENIED"}}`) - profile := decode(`{"error":{"code":401,"message":"Cannot get profile for this user.","status":"UNAUTHENTICATED"}}`) + unauthenticated := decode(`{"error":{"code":401,"message":"Request had invalid authentication credentials.","status":"UNAUTHENTICATED"}}`) + anyMessage := decode(`{"error":{"code":401,"message":"Something only this product says.","status":"UNAUTHENTICATED"}}`) + scope := decode(`{"error":{"code":403,"message":"Request had insufficient authentication scopes.","status":"PERMISSION_DENIED","details":[{"@type":"type.googleapis.com/google.rpc.ErrorInfo","reason":"ACCESS_TOKEN_SCOPE_INSUFFICIENT"}]}}`) for name, tc := range map[string]struct { status int body any want string }{ - "an API not enabled": {http.StatusForbidden, disabled, "not enabled in the service account's Google Cloud project"}, - "no access in product": {http.StatusForbidden, denied, "grant the service account's email access"}, - "no profile in the product": {http.StatusUnauthorized, profile, "grant the service account's email access"}, - "a success": {http.StatusOK, denied, ""}, - "not Google's shape": {http.StatusForbidden, decode(`{"message":"forbidden"}`), ""}, - "a body that is text": {http.StatusForbidden, "forbidden", ""}, - "an unmarshalable body": {http.StatusForbidden, func() {}, ""}, + "an API not enabled": {http.StatusForbidden, disabled, "not enabled in the service account's Google Cloud project"}, + "a scope the token lacks": {http.StatusForbidden, scope, "add it to the connection's oauth_scope"}, + "no access in product": {http.StatusForbidden, denied, "grant the service account's email access"}, + "a 401 in Google's shape": {http.StatusUnauthorized, unauthenticated, "grant the service account's email access"}, + "a 401 with any other message": {http.StatusUnauthorized, anyMessage, "grant the service account's email access"}, + "a success": {http.StatusOK, denied, ""}, + "not Google's shape": {http.StatusForbidden, decode(`{"message":"forbidden"}`), ""}, + "a body that is text": {http.StatusForbidden, "forbidden", ""}, + "an unmarshalable body": {http.StatusForbidden, func() {}, ""}, } { if got := APIHint(tc.status, tc.body); (tc.want == "" && got != "") || !strings.Contains(got, tc.want) { t.Errorf("%s: hint = %q, want %q", name, got, tc.want) diff --git a/pkg/audit/postgres/store.go b/pkg/audit/postgres/store.go index 84d6a87ba..d6d730e79 100644 --- a/pkg/audit/postgres/store.go +++ b/pkg/audit/postgres/store.go @@ -14,7 +14,9 @@ import ( sq "github.com/Masterminds/squirrel" "github.com/lib/pq" + "github.com/txn2/mcp-data-platform/internal/advisorylock" "github.com/txn2/mcp-data-platform/internal/bgloop" + "github.com/txn2/mcp-data-platform/internal/logsan" "github.com/txn2/mcp-data-platform/pkg/audit" ) @@ -554,54 +556,26 @@ func (s *Store) runMaintenanceTick(ctx context.Context) error { }) } -// unlockTimeout caps the time the deferred advisory_unlock call may take. -// The unlock runs on a detached context so it still fires when the parent -// context has been canceled by Close, but it must not block shutdown -// indefinitely if the database itself is unreachable. -const unlockTimeout = 5 * time.Second - // runUnderMaintenanceLock attempts to acquire the audit maintenance advisory // lock and, if successful, invokes fn. If another replica already holds the // lock, fn is not invoked and bgloop.ErrSkipped is returned; when the // database is unreachable, the error. The next tick will retry. // -// The lock is taken on a dedicated connection so the acquire and release -// happen on the same PostgreSQL session (advisory locks are session-scoped). -// fn itself uses s.db, which may draw other connections from the pool; those -// queries are not blocked by the lock. The lock only prevents OTHER replicas -// from running maintenance concurrently. -// -// The unlock uses a detached context with a short timeout so a canceled -// parent context (e.g. Close called during fn) cannot leave the advisory -// lock held on the pooled connection. +// The lock is taken on the process's lock session (internal/advisorylock), +// which every background lock shares, so holding it pins no connection of its +// own (#2169). fn uses s.db; the lock only prevents OTHER replicas from +// running maintenance concurrently. func (s *Store) runUnderMaintenanceLock(ctx context.Context, fn func(context.Context) error) error { - conn, err := s.db.Conn(ctx) + release, got, err := advisorylock.Try(ctx, s.db, advisorylock.ID(auditMaintenanceLockKey)) if err != nil { - slog.WarnContext(ctx, "audit cleanup: acquire connection for lock", slogKeyError, err) - return fmt.Errorf("audit cleanup: acquire connection for lock: %w", err) - } - defer func() { _ = conn.Close() }() - - var got bool - if err := conn.QueryRowContext(ctx, - "SELECT pg_try_advisory_lock($1)", auditMaintenanceLockKey, - ).Scan(&got); err != nil { - slog.WarnContext(ctx, "audit cleanup: try advisory lock", slogKeyError, err) + slog.WarnContext(ctx, "audit cleanup: try advisory lock", slogKeyError, logsan.SanitizeForLog(err.Error())) return fmt.Errorf("audit cleanup: try advisory lock: %w", err) } if !got { // Another replica holds the lock; skip this tick. return bgloop.ErrSkipped } - defer func() { - unlockCtx, cancel := context.WithTimeout(context.Background(), unlockTimeout) - defer cancel() - if _, err := conn.ExecContext(unlockCtx, - "SELECT pg_advisory_unlock($1)", auditMaintenanceLockKey, - ); err != nil { - slog.WarnContext(ctx, "audit cleanup: advisory unlock", slogKeyError, err) - } - }() + defer release() return fn(ctx) } diff --git a/pkg/connoauth/locker_test.go b/pkg/connoauth/locker_test.go new file mode 100644 index 000000000..3628e14a3 --- /dev/null +++ b/pkg/connoauth/locker_test.go @@ -0,0 +1,47 @@ +package connoauth + +import ( + "context" + "errors" + "regexp" + "testing" + + "github.com/DATA-DOG/go-sqlmock" +) + +// The refresher's lock is a try-lock on the process's lock session (#2169): +// taken, it is released by its release; held elsewhere, it is refused; a +// statement that fails is an error naming the lock. +func TestPostgresLocker_TryLock(t *testing.T) { + db, mock, err := sqlmock.New() + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = db.Close() }) + locker := NewPostgresLocker(db) + key := Key{Kind: KindAPI, Name: "crm"} + lockQ := regexp.QuoteMeta("SELECT pg_try_advisory_lock($1)") + unlockQ := regexp.QuoteMeta("SELECT pg_advisory_unlock($1)") + + mock.ExpectQuery(lockQ).WithArgs(advisoryLockKey(key)).WillReturnRows(sqlmock.NewRows([]string{"l"}).AddRow(true)) + mock.ExpectExec(unlockQ).WithArgs(advisoryLockKey(key)).WillReturnResult(sqlmock.NewResult(0, 0)) + release, ok, err := locker.TryLock(context.Background(), key) + if err != nil || !ok { + t.Fatalf("a free lock: ok=%v err=%v", ok, err) + } + release() + + mock.ExpectQuery(lockQ).WillReturnRows(sqlmock.NewRows([]string{"l"}).AddRow(false)) + if _, ok, err := locker.TryLock(context.Background(), key); err != nil || ok { + t.Fatalf("a lock held elsewhere: ok=%v err=%v", ok, err) + } + + down := errors.New("down") + mock.ExpectQuery(lockQ).WillReturnError(down) + if _, _, err := locker.TryLock(context.Background(), key); !errors.Is(err, down) { + t.Fatalf("a failed statement: %v", err) + } + if err := mock.ExpectationsWereMet(); err != nil { + t.Error(err) + } +} diff --git a/pkg/connoauth/refresher.go b/pkg/connoauth/refresher.go index e796416cf..a9433f25a 100644 --- a/pkg/connoauth/refresher.go +++ b/pkg/connoauth/refresher.go @@ -10,6 +10,7 @@ import ( "sync" "time" + "github.com/txn2/mcp-data-platform/internal/advisorylock" "github.com/txn2/mcp-data-platform/internal/bgloop" "github.com/txn2/mcp-data-platform/pkg/authevents" "github.com/txn2/mcp-data-platform/pkg/observability" @@ -95,10 +96,8 @@ func (NoopLocker) TryLock(_ context.Context, _ Key) (release func(), ok bool, er } // PostgresLocker wraps a *sql.DB with pg_try_advisory_lock semantics. -// The lock is held in the session associated with the *sql.Conn the -// locker checks out from the pool; release() closes the connection -// back to the pool which Postgres treats as a session end (and -// auto-releases the lock). +// The lock is held on the process's lock session, which every background +// try-lock shares. type PostgresLocker struct { db *sql.DB } @@ -111,7 +110,9 @@ func NewPostgresLocker(db *sql.DB) *PostgresLocker { } // TryLock acquires a session-scoped advisory lock keyed by -// hashtext('connoauth-refresh:/'). The hash uses FNV +// hashtext('connoauth-refresh:/'), on the process's lock session +// (internal/advisorylock), so a refresh in flight pins no connection of its +// own (#2169). The hash uses FNV // here (Postgres-equivalent stability is not required because every // lock key is held by the same process for at most one tick) and // converts to a bigint for pg_try_advisory_lock. @@ -119,35 +120,11 @@ func (l *PostgresLocker) TryLock(ctx context.Context, k Key) (release func(), ok if !k.IsValid() { return nil, false, errInvalidKey } - conn, err := l.db.Conn(ctx) + release, ok, err = advisorylock.Try(ctx, l.db, advisorylock.ID(advisoryLockKey(k))) if err != nil { - return nil, false, fmt.Errorf("connoauth: locker checkout: %w", err) - } - keyID := advisoryLockKey(k) - var acquired bool - if err := conn.QueryRowContext(ctx, - `SELECT pg_try_advisory_lock($1)`, keyID).Scan(&acquired); err != nil { - _ = conn.Close() return nil, false, fmt.Errorf("connoauth: try lock: %w", err) } - if !acquired { - _ = conn.Close() - return nil, false, nil - } - release = func() { - // Use a short detached context so the unlock doesn't depend - // on the caller's ctx (a refresh that overran its tick - // timeout would otherwise leak the lock for the duration of - // the session-end-on-conn-close). - unlockCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second) - defer cancel() - if _, err := conn.ExecContext(unlockCtx, `SELECT pg_advisory_unlock($1)`, keyID); err != nil { - slog.WarnContext(ctx, "connoauth: advisory unlock failed (lock released on conn close)", - logKeyKind, k.Kind, logKeyName, k.Name, logKeyError, err) - } - _ = conn.Close() - } - return release, true, nil + return release, ok, nil } // advisoryLockKey hashes "connoauth-refresh:/" into the diff --git a/pkg/database/migrate/migrate_unit_test.go b/pkg/database/migrate/migrate_unit_test.go index 0c8832953..e55d8960f 100644 --- a/pkg/database/migrate/migrate_unit_test.go +++ b/pkg/database/migrate/migrate_unit_test.go @@ -15,7 +15,7 @@ import ( ) const ( - migrateTestFileCount = 414 + migrateTestFileCount = 418 migrateTestSuccess = "success" migrateTestFactoryError = "factory error" ) diff --git a/pkg/database/migrate/migrations/000208_call_records_supersession_index.down.sql b/pkg/database/migrate/migrations/000208_call_records_supersession_index.down.sql new file mode 100644 index 000000000..afc589d54 --- /dev/null +++ b/pkg/database/migrate/migrations/000208_call_records_supersession_index.down.sql @@ -0,0 +1 @@ +DROP INDEX IF EXISTS idx_call_records_supersession; diff --git a/pkg/database/migrate/migrations/000208_call_records_supersession_index.up.sql b/pkg/database/migrate/migrations/000208_call_records_supersession_index.up.sql new file mode 100644 index 000000000..c7664b933 --- /dev/null +++ b/pkg/database/migrate/migrations/000208_call_records_supersession_index.up.sql @@ -0,0 +1,7 @@ +-- #2231: the supersession check a call record's outcome is read from looks for +-- a later call in the same session on the same kind and connection. It was +-- answered by intersecting the session index with the (kind, connection) +-- index, a fifth of a millisecond per record, which every list and count +-- filtered by outcome paid once per record in the catalog. +CREATE INDEX IF NOT EXISTS idx_call_records_supersession + ON call_records(session_id, kind, connection, created_at); diff --git a/pkg/database/migrate/migrations/000209_fts_name_words.down.sql b/pkg/database/migrate/migrations/000209_fts_name_words.down.sql new file mode 100644 index 000000000..d46e94e8e --- /dev/null +++ b/pkg/database/migrate/migrations/000209_fts_name_words.down.sql @@ -0,0 +1,82 @@ +CREATE OR REPLACE FUNCTION resource_fts(display_name text, description text, path text, filename text, tags text[], content_text text) + RETURNS tsvector LANGUAGE sql IMMUTABLE PARALLEL SAFE AS $$ + SELECT to_tsvector('english', + coalesce(display_name, '') || ' ' || + coalesce(description, '') || ' ' || + coalesce(path, '') || ' ' || + coalesce(filename, '') || ' ' || + coalesce(array_to_string(tags, ' '), '') || ' ' || + coalesce(content_text, '')); +$$; + +CREATE OR REPLACE FUNCTION portal_asset_fts(name text, description text, tags jsonb) + RETURNS tsvector LANGUAGE sql IMMUTABLE PARALLEL SAFE AS $$ + SELECT to_tsvector('english', + coalesce(name, '') || ' ' || + coalesce(description, '') || ' ' || + coalesce(tags::text, '')); +$$; + +CREATE OR REPLACE FUNCTION portal_collection_fts(name text, description text, sections_text text) + RETURNS tsvector LANGUAGE sql IMMUTABLE PARALLEL SAFE AS $$ + SELECT to_tsvector('english', + coalesce(name, '') || ' ' || + coalesce(description, '') || ' ' || + coalesce(sections_text, '')); +$$; + +CREATE OR REPLACE FUNCTION prompt_fts(display_name text, name text, description text, content text, tags text[]) + RETURNS tsvector LANGUAGE sql IMMUTABLE PARALLEL SAFE AS $$ + SELECT to_tsvector('english', + coalesce(nullif(display_name, ''), name) || ' ' || + coalesce(description, '') || ' ' || + coalesce(content, '') || ' ' || + coalesce(array_to_string(tags, ' '), '')); +$$; + +CREATE OR REPLACE FUNCTION portal_knowledge_page_fts(title text, body text, tags jsonb) + RETURNS tsvector LANGUAGE sql IMMUTABLE PARALLEL SAFE AS $$ + SELECT to_tsvector('english', + coalesce(title, '') || ' ' || + coalesce(body, '') || ' ' || + coalesce(tags::text, '')); +$$; + +CREATE OR REPLACE FUNCTION script_fts(display_name text, name text, description text, category text, tags text[], params jsonb, source text) + RETURNS tsvector LANGUAGE sql IMMUTABLE PARALLEL SAFE AS $$ + SELECT setweight(to_tsvector('english', + coalesce(nullif(display_name, ''), name) || ' ' || + coalesce(description, '') || ' ' || + coalesce(category, '') || ' ' || + coalesce(array_to_string(tags, ' '), '') || ' ' || + CASE WHEN jsonb_typeof(params) = 'array' THEN + coalesce(( + SELECT string_agg( + coalesce(p->>'name', '') || ' ' || coalesce(p->>'description', ''), ' ') + FROM jsonb_array_elements(params) AS p), '') + ELSE '' END), 'A') + || setweight(to_tsvector('english', coalesce(source, '')), 'D'); +$$; + +CREATE OR REPLACE FUNCTION catalog_dataset_fts(name text, description text, tags text[], domain text) + RETURNS tsvector LANGUAGE sql IMMUTABLE PARALLEL SAFE AS $$ + SELECT to_tsvector('english', + coalesce(name, '') || ' ' || + coalesce(description, '') || ' ' || + coalesce(array_to_string(tags, ' '), '') || ' ' || + coalesce(domain, '')); +$$; + +REINDEX INDEX idx_resources_search_fts; +REINDEX INDEX idx_portal_assets_search_fts; +REINDEX INDEX idx_portal_collections_search_fts; +REINDEX INDEX idx_prompts_search_fts; +REINDEX INDEX idx_portal_knowledge_pages_search_fts; +REINDEX INDEX idx_scripts_search_fts; +REINDEX INDEX idx_catalog_datasets_search_fts; + +DROP INDEX IF EXISTS idx_memory_records_content_fts; +CREATE INDEX idx_memory_records_content_fts + ON memory_records USING gin (to_tsvector('english', content)); + +DROP FUNCTION IF EXISTS fts_name_words(text); diff --git a/pkg/database/migrate/migrations/000209_fts_name_words.up.sql b/pkg/database/migrate/migrations/000209_fts_name_words.up.sql new file mode 100644 index 000000000..538bd507a --- /dev/null +++ b/pkg/database/migrate/migrations/000209_fts_name_words.up.sql @@ -0,0 +1,103 @@ +-- #2230: a name is matched by its words. PostgreSQL's parser reads +-- "acceptance-1627-1792487495133911000" as the word "acceptance" and the +-- signed numbers -1627 and -1792487495133911000, and "q3-2024-report.csv" as +-- one file-name token, so a search for "acceptance 1627 1792487495133911000" +-- or "q3 2024 report" matched neither. fts_name_words keeps the text as it is +-- and adds it again with its separators read as spaces, so a name matches by +-- the words it is made of and still by itself. It is applied to the fields +-- that name a thing (names, titles, paths, file names, tags, categories), and +-- to a memory's content, which is short and is where an agent records an +-- identifier. +-- +-- The search functions below spell the expression out rather than call +-- fts_name_words: PostgreSQL 17 runs an index's functions with search_path set +-- to pg_catalog, where an unqualified call to a function of this schema is not +-- found. fts_name_words itself calls only pg_catalog, so the memory index and +-- the folder search call it. +CREATE OR REPLACE FUNCTION fts_name_words(t text) RETURNS text + LANGUAGE sql IMMUTABLE PARALLEL SAFE AS $$ + SELECT coalesce(t, '') || ' ' || regexp_replace(coalesce(t, ''), '[-_./:]+', ' ', 'g'); +$$; + +CREATE OR REPLACE FUNCTION resource_fts(display_name text, description text, path text, filename text, tags text[], content_text text) + RETURNS tsvector LANGUAGE sql IMMUTABLE PARALLEL SAFE AS $$ + SELECT to_tsvector('english', + (coalesce(display_name, '') || ' ' || regexp_replace(coalesce(display_name, ''), '[-_./:]+', ' ', 'g')) || ' ' || + coalesce(description, '') || ' ' || + (coalesce(path, '') || ' ' || regexp_replace(coalesce(path, ''), '[-_./:]+', ' ', 'g')) || ' ' || + (coalesce(filename, '') || ' ' || regexp_replace(coalesce(filename, ''), '[-_./:]+', ' ', 'g')) || ' ' || + (coalesce(array_to_string(tags, ' '), '') || ' ' || regexp_replace(coalesce(array_to_string(tags, ' '), ''), '[-_./:]+', ' ', 'g')) || ' ' || + coalesce(content_text, '')); +$$; + +CREATE OR REPLACE FUNCTION portal_asset_fts(name text, description text, tags jsonb) + RETURNS tsvector LANGUAGE sql IMMUTABLE PARALLEL SAFE AS $$ + SELECT to_tsvector('english', + (coalesce(name, '') || ' ' || regexp_replace(coalesce(name, ''), '[-_./:]+', ' ', 'g')) || ' ' || + coalesce(description, '') || ' ' || + (coalesce(tags::text, '') || ' ' || regexp_replace(coalesce(tags::text, ''), '[-_./:]+', ' ', 'g'))); +$$; + +CREATE OR REPLACE FUNCTION portal_collection_fts(name text, description text, sections_text text) + RETURNS tsvector LANGUAGE sql IMMUTABLE PARALLEL SAFE AS $$ + SELECT to_tsvector('english', + (coalesce(name, '') || ' ' || regexp_replace(coalesce(name, ''), '[-_./:]+', ' ', 'g')) || ' ' || + coalesce(description, '') || ' ' || + coalesce(sections_text, '')); +$$; + +CREATE OR REPLACE FUNCTION prompt_fts(display_name text, name text, description text, content text, tags text[]) + RETURNS tsvector LANGUAGE sql IMMUTABLE PARALLEL SAFE AS $$ + SELECT to_tsvector('english', + (coalesce(coalesce(nullif(display_name, ''), name), '') || ' ' || regexp_replace(coalesce(coalesce(nullif(display_name, ''), name), ''), '[-_./:]+', ' ', 'g')) || ' ' || + coalesce(description, '') || ' ' || + coalesce(content, '') || ' ' || + (coalesce(array_to_string(tags, ' '), '') || ' ' || regexp_replace(coalesce(array_to_string(tags, ' '), ''), '[-_./:]+', ' ', 'g'))); +$$; + +CREATE OR REPLACE FUNCTION portal_knowledge_page_fts(title text, body text, tags jsonb) + RETURNS tsvector LANGUAGE sql IMMUTABLE PARALLEL SAFE AS $$ + SELECT to_tsvector('english', + (coalesce(title, '') || ' ' || regexp_replace(coalesce(title, ''), '[-_./:]+', ' ', 'g')) || ' ' || + coalesce(body, '') || ' ' || + (coalesce(tags::text, '') || ' ' || regexp_replace(coalesce(tags::text, ''), '[-_./:]+', ' ', 'g'))); +$$; + +CREATE OR REPLACE FUNCTION script_fts(display_name text, name text, description text, category text, tags text[], params jsonb, source text) + RETURNS tsvector LANGUAGE sql IMMUTABLE PARALLEL SAFE AS $$ + SELECT setweight(to_tsvector('english', + (coalesce(coalesce(nullif(display_name, ''), name), '') || ' ' || regexp_replace(coalesce(coalesce(nullif(display_name, ''), name), ''), '[-_./:]+', ' ', 'g')) || ' ' || + coalesce(description, '') || ' ' || + (coalesce(category, '') || ' ' || regexp_replace(coalesce(category, ''), '[-_./:]+', ' ', 'g')) || ' ' || + (coalesce(array_to_string(tags, ' '), '') || ' ' || regexp_replace(coalesce(array_to_string(tags, ' '), ''), '[-_./:]+', ' ', 'g')) || ' ' || + CASE WHEN jsonb_typeof(params) = 'array' THEN + coalesce(( + SELECT string_agg( + coalesce(p->>'name', '') || ' ' || coalesce(p->>'description', ''), ' ') + FROM jsonb_array_elements(params) AS p), '') + ELSE '' END), 'A') + || setweight(to_tsvector('english', coalesce(source, '')), 'D'); +$$; + +CREATE OR REPLACE FUNCTION catalog_dataset_fts(name text, description text, tags text[], domain text) + RETURNS tsvector LANGUAGE sql IMMUTABLE PARALLEL SAFE AS $$ + SELECT to_tsvector('english', + (coalesce(name, '') || ' ' || regexp_replace(coalesce(name, ''), '[-_./:]+', ' ', 'g')) || ' ' || + coalesce(description, '') || ' ' || + (coalesce(array_to_string(tags, ' '), '') || ' ' || regexp_replace(coalesce(array_to_string(tags, ' '), ''), '[-_./:]+', ' ', 'g')) || ' ' || + (coalesce(domain, '') || ' ' || regexp_replace(coalesce(domain, ''), '[-_./:]+', ' ', 'g'))); +$$; + +-- An expression index holds what its function returned when each row was +-- written, so each index on a function changed above is built again. +REINDEX INDEX idx_resources_search_fts; +REINDEX INDEX idx_portal_assets_search_fts; +REINDEX INDEX idx_portal_collections_search_fts; +REINDEX INDEX idx_prompts_search_fts; +REINDEX INDEX idx_portal_knowledge_pages_search_fts; +REINDEX INDEX idx_scripts_search_fts; +REINDEX INDEX idx_catalog_datasets_search_fts; + +DROP INDEX IF EXISTS idx_memory_records_content_fts; +CREATE INDEX idx_memory_records_content_fts + ON memory_records USING gin (to_tsvector('english', fts_name_words(content))); diff --git a/pkg/memory/postgres.go b/pkg/memory/postgres.go index 1620ad19f..72be022a7 100644 --- a/pkg/memory/postgres.go +++ b/pkg/memory/postgres.go @@ -388,8 +388,9 @@ const rawRecordCols = "id, created_at, updated_at, created_by, persona, dimensio // ftsExpr is the Postgres full-text expression the lexical arm matches // and ranks against. It MUST be byte-identical to the expression the // idx_memory_records_content_fts GIN index is built on (migration -// 000054) or the planner will not use the index. -const ftsExpr = "to_tsvector('english', content)" +// 000209, which reads a memory's identifiers by their words, #2230) or the +// planner will not use the index. +const ftsExpr = "to_tsvector('english', fts_name_words(content))" // ftsRankNormalization is the ts_rank_cd normalization bitmask for the // LexicalSearch score. Bit 1 divides the rank by 1 + log(document length) so a diff --git a/pkg/platform/config.go b/pkg/platform/config.go index 8d21de2c4..25e3361d7 100644 --- a/pkg/platform/config.go +++ b/pkg/platform/config.go @@ -15,6 +15,7 @@ import ( "gopkg.in/yaml.v3" + "github.com/txn2/mcp-data-platform/internal/dbobs" "github.com/txn2/mcp-data-platform/internal/indexpolicy" "github.com/txn2/mcp-data-platform/internal/platform/configenv" "github.com/txn2/mcp-data-platform/internal/platform/configwarn" @@ -2050,7 +2051,7 @@ func (c *Config) Validate() error { _, runBudget := c.Scripts.Worker.RunMemoryBudget(0) _, thumbs := c.Thumbnails.Tuning() _, thumbSources := c.Thumbnails.SourceLimits() - for _, err := range []error{admission, runBudget, thumbs, thumbSources, c.Scripts.SQL.Validate(), c.Scripts.Draft.Validate(), c.Webhooks.Validate(), c.Resources.Index.Validate(), c.Tools.ResultBudget.Validate(), c.Portal.AssetRefs.Validate()} { + for _, err := range []error{admission, runBudget, thumbs, thumbSources, dbobs.CheckPoolSize(c.Database.MaxOpenConns), c.Scripts.SQL.Validate(), c.Scripts.Draft.Validate(), c.Webhooks.Validate(), c.Resources.Index.Validate(), c.Tools.ResultBudget.Validate(), c.Portal.AssetRefs.Validate()} { if err != nil { errs = append(errs, err.Error()) } diff --git a/pkg/platform/config_test.go b/pkg/platform/config_test.go index 5d0018aaa..24119b9dc 100644 --- a/pkg/platform/config_test.go +++ b/pkg/platform/config_test.go @@ -1253,6 +1253,20 @@ func TestConfigValidate_SessionsDatabaseWithoutDSN(t *testing.T) { } } +// #2169: a pool too small for the platform's background work is refused at +// startup, naming the minimum. +func TestConfigValidate_RefusesATooSmallPool(t *testing.T) { + cfg := &Config{Database: DatabaseConfig{DSN: "postgres://x", MaxOpenConns: 3}} + err := cfg.Validate() + if err == nil || !strings.Contains(err.Error(), "database.max_open_conns is 3; the platform needs at least 4") { + t.Errorf("Validate() with max_open_conns 3: %v", err) + } + cfg.Database.MaxOpenConns = 4 + if err := cfg.Validate(); err != nil { + t.Errorf("Validate() with max_open_conns 4: %v", err) + } +} + func TestConfigValidate_SessionsDatabaseWithDSN(t *testing.T) { cfg := &Config{ Sessions: SessionsConfig{ diff --git a/pkg/resource/foldersearch.go b/pkg/resource/foldersearch.go index d321bb3aa..e1b007eee 100644 --- a/pkg/resource/foldersearch.go +++ b/pkg/resource/foldersearch.go @@ -21,7 +21,7 @@ type ScoredFolder struct { } // folderFTS is the full-text expression a folder entry is matched on. -const folderFTS = "to_tsvector('english', path || ' ' || description || ' ' || summary_text)" +const folderFTS = "to_tsvector('english', fts_name_words(path) || ' ' || description || ' ' || summary_text)" // SearchFolders ranks the folder entries in the caller's visible libraries. // Only a folder that has an entry -- one holding files that are not indexed diff --git a/pkg/resource/search_realdb_integration_test.go b/pkg/resource/search_realdb_integration_test.go index 4743e4dbb..b7b00e46c 100644 --- a/pkg/resource/search_realdb_integration_test.go +++ b/pkg/resource/search_realdb_integration_test.go @@ -283,3 +283,49 @@ func TestResourceIndex_RealDB_UnsettledContentIsAGap(t *testing.T) { assert.Equal(t, []string{"res_owed"}, gaps, "an embedded row whose content pass never settled must still be a gap") } + +// TestResourceSearch_RealDB_AFileIsFoundByTheWordsOfItsName is #2230. Among +// thirty embedded files whose names look like it, one not embedded yet and +// named acceptance-1627-.csv is the first hit for "acceptance 1627 ": +// its name matches by its words, and an unembedded match outranks every file +// that matches none of the query's tokens. PostgreSQL's parser read the name +// as "acceptance" and the signed numbers -1627 and -, so the file matched +// nothing. A name with a dotted extension is found by its words the same way, +// and still by itself. +func TestResourceSearch_RealDB_AFileIsFoundByTheWordsOfItsName(t *testing.T) { + db := testdb.New(t) + store := NewPostgresStore(db) + ctx := context.Background() + insert := func(id, name string) { + require.NoError(t, store.Insert(ctx, Resource{ + ID: id, Scope: ScopeGlobal, Path: "acceptance-1657", Filename: name + ".bin", DisplayName: name, + MIMEType: "application/octet-stream", S3Key: id, URI: "mcp://global/acceptance-1657/" + name + ".bin", UploaderSub: "sub-1", + })) + } + for i := range 30 { + id := "res_alike_" + strings.Repeat("x", i+1) + insert(id, "Acceptance 1657 thing-"+strings.Repeat("a", i+1)) + _, err := db.ExecContext(ctx, `UPDATE resources SET embedding = $2, embedding_model = 'test' WHERE id = $1`, id, vectorLiteral(768)) + require.NoError(t, err) + } + const name = "acceptance-1627-1792487495133911000" + insert("res_target", name) + _, err := db.ExecContext(ctx, `UPDATE resources SET filename = 'q3-2024-report.csv' WHERE id = 'res_target'`) + require.NoError(t, err) + scopes := []ScopeFilter{{Scope: ScopeGlobal}} + + for _, query := range []string{"acceptance 1627 1792487495133911000", name, "q3 2024 report", "q3-2024-report.csv"} { + got, err := store.(Searcher).Search(ctx, SearchQuery{ + Embedding: unitQueryVector(768), //nolint:mnd // the schema's vector dimensionality + QueryText: query, Scopes: scopes, Limit: 25, + }) + require.NoError(t, err) + require.NotEmpty(t, got, query) + assert.Equal(t, "res_target", got[0].Resource.ID, "the first hit for %q", query) + + lexical, err := store.(Searcher).Search(ctx, SearchQuery{QueryText: query, Scopes: scopes}) + require.NoError(t, err) + require.NotEmpty(t, lexical, "the lexical search for %q", query) + assert.Equal(t, "res_target", lexical[0].Resource.ID, "the first lexical hit for %q", query) + } +} diff --git a/pkg/toolkits/portal/assetrefs.go b/pkg/toolkits/portal/assetrefs.go index a83476a76..83646dcb6 100644 --- a/pkg/toolkits/portal/assetrefs.go +++ b/pkg/toolkits/portal/assetrefs.go @@ -155,6 +155,30 @@ func (t *Toolkit) applyRefs( return len(refs), nil } +// widenRefs grants a write's newly declared references before its content +// write creates the version that makes the asset owed a tile (#2234): the tile +// worker may draw the new version the moment it exists, and a file not yet +// granted is drawn as missing and stays so until the next content change. +// undo puts the references back when that content write fails; applyRefs after +// a write that succeeded records exactly the declared set. +func (t *Toolkit) widenRefs( + ctx context.Context, assetID string, declared []assetrefs.Declared, present bool, +) (undo func(), result *mcp.CallToolResult) { + if !present { + return func() {}, nil + } + restore, err := t.contentRefs.Widen(ctx, assetID, declared, resolveOwnerEmail(ctx)) + if err != nil { + return nil, refResult(err) + } + return func() { + if err := restore(context.WithoutCancel(ctx)); err != nil { + slog.WarnContext(ctx, "portal: restoring an asset's references after a failed write", + "asset_id", assetID, "error", logsan.SanitizeForLog(err.Error())) + } + }, nil +} + // addRefFields reports a completed declaration on a map-shaped tool response. // A count below zero means the write said nothing about references, and adds // nothing. diff --git a/pkg/toolkits/portal/assetrefs_test.go b/pkg/toolkits/portal/assetrefs_test.go index d6068cfd1..1bc6ecdcf 100644 --- a/pkg/toolkits/portal/assetrefs_test.go +++ b/pkg/toolkits/portal/assetrefs_test.go @@ -3,6 +3,7 @@ package portal import ( "context" "encoding/json" + "errors" "fmt" "testing" @@ -605,3 +606,179 @@ func TestSaveRecordsReferencesBeforeTheFirstVersion(t *testing.T) { out := decodeSave(t, result) assert.Equal(t, 1, versions.atNew[out.AssetID], "the reference was not recorded when the first version was created") } + +// refsRig is a toolkit whose version store notes how many references an asset +// had when each version was created, with an asset saved naming the logo. +type refsRig struct { + tk *Toolkit + versions *refsAtVersionStore + refs *refStoreStub + s3 *mockS3Client + id string +} + +func versionRefsRig(t *testing.T) refsRig { + t.Helper() + assets := newInMemoryAssetStore() + refs := newRefStoreStub() + versions := &refsAtVersionStore{inMemoryVersionStore: newInMemoryVersionStore(), refs: refs, atNew: map[string]int{}} + s3 := &mockS3Client{getBody: []byte("

Q4

"), getCT: "text/html"} + tk := New(Config{ + Name: "test", AssetStore: assets, VersionStore: versions, + S3Client: s3, S3Bucket: "bucket", BaseURL: "http://example.com", + }) + tk.SetContentRefs(newDeclarer(refs, assets)) + saved := decodeSave(t, mustSave(t, tk, []string{refLogoURI})) + return refsRig{tk: tk, versions: versions, refs: refs, s3: s3, id: saved.AssetID} +} + +// TestUpdateGrantsNewReferencesBeforeTheNewVersion is #2234: the new version +// is what makes the asset owed a tile, so a reference the update declares is +// already recorded when it is created, beside the one the update drops, which +// the content still current until then uses; once the content is written the +// asset names exactly what the update declared, the kept reference with its +// token. +func TestUpdateGrantsNewReferencesBeforeTheNewVersion(t *testing.T) { + rig := versionRefsRig(t) + tk, versions, refs, id := rig.tk, rig.versions, rig.refs, rig.id + logoToken := refs.byAsset[id][0].RefToken + + result, _, err := tk.handleUpdate(refCtx("finance"), manageAssetInput{ + Action: "update", AssetID: id, Content: ``, + References: []string{refChartURI}, + }) + require.NoError(t, err) + require.False(t, result.IsError, errText(t, result)) + assert.Equal(t, 2, versions.atNew[id], "the new reference was not recorded when the version was created") + stored := refs.byAsset[id] + require.Len(t, stored, 1) + assert.Equal(t, "res-chart", stored[0].TargetID) + + result, _, err = tk.handleUpdate(refCtx("finance"), manageAssetInput{ + Action: "update", AssetID: id, Content: ``, + References: []string{refLogoURI, refChartURI}, + }) + require.NoError(t, err) + require.False(t, result.IsError, errText(t, result)) + assert.Equal(t, 2, versions.atNew[id]) + require.Len(t, refs.byAsset[id], 2) + chartToken := refs.byAsset[id][1].RefToken + assert.NotEqual(t, logoToken, refs.byAsset[id][0].RefToken, "a reference dropped and declared again is a new grant") + + // Content that names what the asset already has adds nothing first. + result, _, err = tk.handleUpdate(refCtx("finance"), manageAssetInput{ + Action: "update", AssetID: id, Content: ``, References: []string{refChartURI}, + }) + require.NoError(t, err) + require.False(t, result.IsError, errText(t, result)) + require.Len(t, refs.byAsset[id], 1) + assert.Equal(t, chartToken, refs.byAsset[id][0].RefToken, "a kept reference keeps its token") +} + +// TestPatchGrantsNewReferencesBeforeTheNewVersion is #2234 for a patch. +func TestPatchGrantsNewReferencesBeforeTheNewVersion(t *testing.T) { + rig := versionRefsRig(t) + tk, versions, refs, id := rig.tk, rig.versions, rig.refs, rig.id + result, _, err := tk.handlePatch(refCtx("finance"), manageAssetInput{ + Action: "patch", AssetID: id, + Edits: []textpatch.Edit{{Op: textpatch.OpInsertAfter, Find: "", Text: ``}}, + References: []string{refLogoURI, refChartURI}, + }) + require.NoError(t, err) + require.False(t, result.IsError, errText(t, result)) + assert.Equal(t, 2, versions.atNew[id], "the new reference was not recorded when the version was created") + assert.Len(t, refs.byAsset[id], 2) +} + +// TestAFailedContentWriteRestoresTheReferences: a write whose content could not +// be stored leaves the asset naming what it named before, not the references +// granted ahead of it. +func TestAFailedContentWriteRestoresTheReferences(t *testing.T) { + for _, action := range []string{"update", "patch"} { + t.Run(action, func(t *testing.T) { + rig := versionRefsRig(t) + tk, refs, s3, id := rig.tk, rig.refs, rig.s3, rig.id + s3.putErr = errors.New("storage down") + input := manageAssetInput{Action: action, AssetID: id, References: []string{refChartURI}} + var ( + result *mcp.CallToolResult + err error + ) + if action == "update" { + input.Content = `` + result, _, err = tk.handleUpdate(refCtx("finance"), input) + } else { + input.Edits = []textpatch.Edit{{Op: textpatch.OpInsertAfter, Find: "", Text: ``}} + result, _, err = tk.handlePatch(refCtx("finance"), input) + } + require.NoError(t, err) + require.True(t, result.IsError) + stored := refs.byAsset[id] + require.Len(t, stored, 1) + assert.Equal(t, "res-logo", stored[0].TargetID) + }) + } +} + +// failingRefs is a reference store that fails its reads once told to, and its +// writes after replacesOK more have succeeded (negative: never). +type failingRefs struct { + *refStoreStub + listErr error + replacesOK int +} + +func (f *failingRefs) ListByAsset(ctx context.Context, id string) ([]assetrefs.Ref, error) { + if f.listErr != nil { + return nil, f.listErr + } + return f.refStoreStub.ListByAsset(ctx, id) +} + +func (f *failingRefs) Replace(ctx context.Context, id string, refs []assetrefs.Ref) error { + if f.replacesOK == 0 { + return errors.New("refs down") + } + f.replacesOK-- + return f.refStoreStub.Replace(ctx, id, refs) +} + +// A write whose references cannot be granted ahead of it changes nothing and +// says so; a failed content write whose references cannot be put back is +// still reported as the content write's failure (#2234). +func TestWidenFailures(t *testing.T) { + assets := newInMemoryAssetStore() + refs := &failingRefs{refStoreStub: newRefStoreStub(), replacesOK: -1} + s3 := &mockS3Client{getBody: []byte("

Q4

"), getCT: "text/html"} + tk := New(Config{ + Name: "test", AssetStore: assets, VersionStore: newInMemoryVersionStore(), + S3Client: s3, S3Bucket: "bucket", BaseURL: "http://example.com", + }) + tk.SetContentRefs(newDeclarer(refs, assets)) + id := decodeSave(t, mustSave(t, tk, []string{refLogoURI})).AssetID + update := manageAssetInput{ + Action: "update", AssetID: id, Content: ``, References: []string{refChartURI}, + } + + refs.listErr = errors.New("refs down") + result, _, err := tk.handleUpdate(refCtx("finance"), update) + require.NoError(t, err) + require.True(t, result.IsError, "a grant that could not be recorded refuses the write") + + refs.listErr = nil + s3.putErr = errors.New("storage down") + result, _, err = tk.handleUpdate(refCtx("finance"), update) + require.NoError(t, err) + require.True(t, result.IsError) + assert.Contains(t, errText(t, result), "failed to upload new content") + + // The grant ahead of the write is recorded; putting it back is not. + refs.replacesOK = 1 + result, _, err = tk.handlePatch(refCtx("finance"), manageAssetInput{ + Action: "patch", AssetID: id, References: []string{refChartURI}, + Edits: []textpatch.Edit{{Op: textpatch.OpInsertAfter, Find: "", Text: "

x

"}}, + }) + require.NoError(t, err) + require.True(t, result.IsError) + assert.Contains(t, errText(t, result), "failed to write patched content") +} diff --git a/pkg/toolkits/portal/patch_handlers.go b/pkg/toolkits/portal/patch_handlers.go index 871ecdc60..eb0bb5ba5 100644 --- a/pkg/toolkits/portal/patch_handlers.go +++ b/pkg/toolkits/portal/patch_handlers.go @@ -155,6 +155,10 @@ func (t *Toolkit) handlePatch(ctx context.Context, input manageAssetInput) (*mcp return toolkit.JSONResultTyped(result) } + undo, widenResult := t.widenRefs(ctx, asset.ID, declaredRefs, hasRefs) + if widenResult != nil { + return widenResult, nil, nil + } version, tables, err := t.uploadContentUpdate(ctx, asset, contentEdit{ content: res.Body, declaredType: asset.ContentType, @@ -162,6 +166,7 @@ func (t *Toolkit) handlePatch(ctx context.Context, input manageAssetInput) (*mcp sources: input.Sources, }) if err != nil { + undo() return toolkit.ErrorResult("failed to write patched content: " + err.Error()), nil, nil } refCount, applyResult := t.applyRefs(ctx, asset.ID, declaredRefs, hasRefs) diff --git a/pkg/toolkits/portal/toolkit.go b/pkg/toolkits/portal/toolkit.go index c8981e296..4c70cb284 100644 --- a/pkg/toolkits/portal/toolkit.go +++ b/pkg/toolkits/portal/toolkit.go @@ -1050,6 +1050,10 @@ func (t *Toolkit) handleUpdate(ctx context.Context, input manageAssetInput) (*mc var followed map[string]any if hasContent { + undo, widenResult := t.widenRefs(ctx, asset.ID, declaredRefs, hasRefs) + if widenResult != nil { + return widenResult, nil, nil + } edit := contentEdit{ content: input.Content, declaredType: input.ContentType, @@ -1058,6 +1062,7 @@ func (t *Toolkit) handleUpdate(ctx context.Context, input manageAssetInput) (*mc } version, tables, contentErr := t.uploadContentUpdate(ctx, asset, edit) if contentErr != nil { + undo() return toolkit.ErrorResult("failed to upload new content: " + contentErr.Error()), nil, nil } followed = tableFields(version, tables) diff --git a/scripts/docs-image-digests.sh b/scripts/docs-image-digests.sh new file mode 100755 index 000000000..c3d55790c --- /dev/null +++ b/scripts/docs-image-digests.sh @@ -0,0 +1,37 @@ +#!/bin/sh +# Resolve every image the documentation pins to a digest (#2224). +# +# A digest written in the docs is checked by nothing else: three counting +# patterns (sha256:acf5271b...e1f2a3b) shipped in the deployment guide's +# compose example, and anyone who copied it got "manifest unknown". This asks +# each image's registry for the digest as written and fails on one it does not +# serve. It needs network access and Docker, so it runs in the E2E Nightly, +# not in verify. +# +# Usage: docs-image-digests.sh [dir] (default: docs) +# Prints one line per image that does not resolve and exits 1 when there is one. +set -eu + +dir=${1:-docs} +refs=$(grep -rhoE '[a-z0-9][a-z0-9./_-]*(:[A-Za-z0-9._-]+)?@sha256:[0-9a-zA-Z]+' "$dir" | sort -u) +if [ -z "$refs" ]; then + echo "docs-image-digests: no image pinned to a digest under $dir" + exit 0 +fi + +failed=0 +for ref in $refs; do + digest=${ref##*@sha256:} + if ! printf '%s' "$digest" | grep -qE '^[0-9a-f]{64}$'; then + echo "docs-image-digests: $ref: a digest is 64 lowercase hex characters" + failed=1 + continue + fi + if ! docker buildx imagetools inspect "$ref" >/dev/null 2>&1; then + echo "docs-image-digests: $ref: the registry does not serve this digest" + failed=1 + continue + fi + echo "docs-image-digests: $ref" +done +exit $failed diff --git a/test/acceptance/issue_1627_test.go b/test/acceptance/issue_1627_test.go index 839a281a6..b084305a2 100644 --- a/test/acceptance/issue_1627_test.go +++ b/test/acceptance/issue_1627_test.go @@ -231,11 +231,9 @@ func objects1627(t *testing.T, raw []any) []map[string]any { func hitTable1627(t *testing.T, c *client, reference, name string) map[string]any { t.Helper() intent := strings.ReplaceAll(name, "-", " ") - // A file is ranked by its embedding beside its words, and one not yet - // embedded is outranked by files whose names merely look alike: a stack - // holding hundreds of acceptance files buried it below the limit. What - // this criterion reads is the hit's table, so it searches once the file's - // index job has finished. + // What this criterion reads is the hit's table, which the file's index + // job records, so it searches once that job has finished. The file is + // found by the words of its name (#2230). db := issue1904DB(t) id := strings.TrimPrefix(reference, "mcp:resource:") issue1904Await(t, "the file's index job finished", func() bool { diff --git a/test/acceptance/issue_2169_test.go b/test/acceptance/issue_2169_test.go new file mode 100644 index 000000000..9a3fd5308 --- /dev/null +++ b/test/acceptance/issue_2169_test.go @@ -0,0 +1,171 @@ +//go:build integration + +package acceptance + +import ( + "context" + "database/sql" + "fmt" + "net/http" + "os" + "os/exec" + "path/filepath" + "strings" + "sync" + "testing" + "time" +) + +// Acceptance for #2169: a replica held its background locks on a pooled +// connection each, and once the locks outnumbered the free connections it +// served nothing that read the database while /readyz said 200. Every +// try-lock the platform holds across work now shares one session per process. +// A replica holding more locks than its pool has connections keeps one of +// them for all of the locks and goes on answering search, admin reads and +// /readyz; and a pool smaller than the platform's minimum is refused at +// startup, naming the minimum. +// +// The locks are connlimit slots: they are the locks a caller can hold for as +// long as it likes, a slot per call in flight on a connection with +// max_concurrent_calls. The background loops' locks are the same lock on the +// same session, held for as long as a pass runs, and +// internal/advisorylock's RealDB tests hold the loops' own together. +// +// Wire forms: the connection's max_concurrent_calls is sent to the admin API +// as a JSON number; api_invoke_endpoint's query_params is a typed object and +// timeout_seconds a typed integer; search's intent and purpose are strings. The startup +// refusal is the configuration file's database.max_open_conns, a YAML +// integer. + +const ( + // issue2169Slots is more slots than a dev replica's pool (10) has + // connections, so a session per slot would hold every one of them. + issue2169Slots = 15 + // issue2169Hold is how long each call holds its slot, in milliseconds. + issue2169Hold = 6000 + // issue2169Answer bounds a request the replica serves while the slots are + // held. + issue2169Answer = 3 * time.Second + // issue2169Purpose is the sentence each search states. + issue2169Purpose = "Acceptance #2169: a replica holding many locks keeps serving." +) + +// issue2169SlotHolders counts the granted two-integer advisory locks (the +// form a slot takes) on the dev database and the sessions holding them. +func issue2169SlotHolders(t *testing.T, db *sql.DB) (locks, sessions int) { + t.Helper() + err := db.QueryRowContext(t.Context(), ` + SELECT count(*), count(DISTINCT pid) FROM pg_locks + WHERE locktype = 'advisory' AND objsubid = 2 AND granted + AND database = (SELECT oid FROM pg_database WHERE datname = current_database())`).Scan(&locks, &sessions) + if err != nil { + t.Fatalf("reading the held advisory locks: %v", err) + } + return locks, sessions +} + +// issue2169Timed runs fn and fails when it takes longer than the bound. +func issue2169Timed(t *testing.T, what string, fn func()) { + t.Helper() + start := time.Now() + fn() + if took := time.Since(start); took > issue2169Answer { + t.Errorf("%s took %s while the replica held %d slots; the bound is %s", what, took, issue2169Slots, issue2169Answer) + } +} + +// TestIssue2169_AReplicaHoldingMoreLocksThanConnectionsKeepsServing: fifteen +// calls in flight through one replica on a connection limited to fifteen hold +// fifteen slots on one database session, and the replica answers search, an +// admin read and /readyz within the bound while they do. With a session per +// slot the fifteen needed fifteen of the replica's ten connections, and +// nothing else on it was served until they finished. +func TestIssue2169_AReplicaHoldingMoreLocksThanConnectionsKeepsServing(t *testing.T) { + target := replicas(t)[0] + c := connectAtFor(t, target.base, devAPIKey(), 3*time.Minute) + db := devDB(t) + name := issue2209Connection(t, c, "2169-slots", issue2169Slots) + c.call("search", map[string]any{"intent": "acceptance 2169: a replica holding many locks", "purpose": issue2169Purpose}) + + before, _ := issue2169SlotHolders(t, db) + var wg sync.WaitGroup + failures := make(chan string, issue2169Slots) + for range issue2169Slots { + wg.Go(func() { + if text, ok := issue2209Slow(c, name, issue2169Hold, 60); !ok { + failures <- text + } + }) + } + + deadline := time.Now().Add(issue2169Hold * time.Millisecond / 2) + var locks, sessions int + for time.Now().Before(deadline) { + if locks, sessions = issue2169SlotHolders(t, db); locks-before >= issue2169Slots { + break + } + time.Sleep(100 * time.Millisecond) + } + if locks-before < issue2169Slots { + t.Fatalf("%d slots held after %s; the criterion needs all %d in flight at once", locks-before, issue2169Hold*time.Millisecond/2, issue2169Slots) + } + if sessions > 1 { + t.Errorf("%d slots are held by %d database sessions; a replica holds its locks on one", locks, sessions) + } + + issue2169Timed(t, "search", func() { + c.call("search", map[string]any{"intent": "acceptance 2169: served while the slots are held", "purpose": issue2169Purpose}) + }) + issue2169Timed(t, "an admin read", func() { + if status, out := c.rest(http.MethodGet, "/api/v1/admin/connection-instances", nil); status != http.StatusOK { + t.Errorf("GET /api/v1/admin/connection-instances: HTTP %d: %v", status, out) + } + }) + issue2169Timed(t, "/readyz", func() { + ctx, cancel := context.WithTimeout(t.Context(), issue2169Answer) + defer cancel() + req, err := http.NewRequestWithContext(ctx, http.MethodGet, target.base+"/readyz", http.NoBody) // #nosec G704 -- the platform under test, named by the suite's own environment + if err != nil { + t.Fatal(err) + } + res, err := http.DefaultClient.Do(req) // #nosec G704 -- the platform under test, named by the suite's own environment + if err != nil { + t.Errorf("/readyz: %v", err) + return + } + _ = res.Body.Close() //nolint:errcheck // only the status is read + if res.StatusCode != http.StatusOK { + t.Errorf("/readyz answered %d", res.StatusCode) + } + }) + + wg.Wait() + close(failures) + for f := range failures { + t.Errorf("a call holding a slot failed: %s", f) + } +} + +// TestIssue2169_APoolBelowTheMinimumIsRefusedAtStartup: the platform started +// with database.max_open_conns 3 exits before it opens anything, naming the +// value and the minimum of 4. +func TestIssue2169_APoolBelowTheMinimumIsRefusedAtStartup(t *testing.T) { + bin := platformBinary(t) + cfg := filepath.Join(t.TempDir(), "platform.yaml") + body := fmt.Sprintf("server:\n name: acc-2169\n transport: stdio\ndatabase:\n dsn: %q\n max_open_conns: 3\n", issue1694DevDSN()) + if err := os.WriteFile(cfg, []byte(body), 0o600); err != nil { + t.Fatal(err) + } + ctx, cancel := context.WithTimeout(t.Context(), time.Minute) + defer cancel() + out, err := exec.CommandContext(ctx, bin, "--config", cfg).CombinedOutput() // #nosec G204 -- the binary this tree built, with a config this test wrote + if err == nil { + t.Fatalf("the platform started with max_open_conns 3:\n%s", out) + } + if ctx.Err() != nil { + t.Fatalf("the platform did not exit within a minute of starting with max_open_conns 3:\n%s", out) + } + if want := "database.max_open_conns is 3; the platform needs at least 4"; !strings.Contains(string(out), want) { + t.Errorf("the refusal does not say %q:\n%s", want, out) + } +} diff --git a/test/acceptance/issue_2203_test.go b/test/acceptance/issue_2203_test.go new file mode 100644 index 000000000..f09d25778 --- /dev/null +++ b/test/acceptance/issue_2203_test.go @@ -0,0 +1,218 @@ +//go:build integration + +package acceptance + +import ( + "encoding/json" + "io" + "math" + "net/http" + "strconv" + "strings" + "testing" + "time" +) + +// Acceptance for #2203: a fresh install's setup was fitted as growth and +// shown as a fill date. A store's first day is now left out of every fit, a +// 30-day rate waits for two weeks of history after it, and each tick is +// sampled by one replica: the other finds the sample and skips. +// +// Wire forms: this ticket adds no tool parameter. GET +// /api/v1/admin/storage takes no parameter; GET /api/v1/admin/storage/history +// takes store and days as query strings, the one form a query has. + +// issue2203Interval is the dev stack's storage sampling interval, the +// default. +const issue2203Interval = 15 * time.Minute + +// issue2203Point is one stored sample as the history route answers it. +type issue2203Point struct { + At time.Time `json:"at"` + Used int64 `json:"used_bytes"` +} + +// issue2203Report is the part of the storage report the criteria read. +type issue2203Report struct { + GeneratedAt time.Time `json:"generated_at"` + Stores []struct { + Store string `json:"store"` + Used int64 `json:"used_bytes"` + History float64 `json:"history_hours"` + Rate7d *float64 `json:"rate_7d_bytes_per_day"` + Rate30d *float64 `json:"rate_30d_bytes_per_day"` + Min7d float64 `json:"rate_7d_min_history_hours"` + Min30d float64 `json:"rate_30d_min_history_hours"` + ExhaustedAt *string `json:"exhaustion_at"` + } `json:"stores"` +} + +// issue2203Get reads one admin route as the suite's administrator. +func issue2203Get(t *testing.T, c *client, path string, into any) { + t.Helper() + req, err := http.NewRequestWithContext(t.Context(), http.MethodGet, c.base+path, http.NoBody) // #nosec G704 -- the platform under test, named by the suite's own environment + if err != nil { + t.Fatal(err) + } + req.Header.Set("Authorization", "Bearer "+c.apiKey) + res, err := http.DefaultClient.Do(req) // #nosec G704 -- the platform under test, named by the suite's own environment + if err != nil { + t.Fatalf("GET %s: %v", path, err) + } + defer res.Body.Close() //nolint:errcheck // best-effort close after read + raw, _ := io.ReadAll(res.Body) + if res.StatusCode != http.StatusOK { + t.Fatalf("GET %s: HTTP %d: %s", path, res.StatusCode, raw) + } + if err := json.Unmarshal(raw, into); err != nil { + t.Fatalf("GET %s: %v\n%s", path, err, raw) + } +} + +// issue2203Slope is the least-squares slope of used bytes over the points at +// or after from, in bytes per day; ok is false with fewer than two. +func issue2203Slope(points []issue2203Point, from time.Time) (slope float64, ok bool) { + var in []issue2203Point + for _, p := range points { + if !p.At.Before(from) { + in = append(in, p) + } + } + if len(in) < 2 { + return 0, false + } + x := func(p issue2203Point) float64 { return p.At.Sub(in[0].At).Hours() / 24 } + var mx, my float64 + for _, p := range in { + mx += x(p) + my += float64(p.Used) + } + n := float64(len(in)) + mx, my = mx/n, my/n + var sxx, sxy float64 + for _, p := range in { + sxx += (x(p) - mx) * (x(p) - mx) + sxy += (x(p) - mx) * (float64(p.Used) - my) + } + return sxy / sxx, sxx > 0 +} + +// TestIssue2203_TheFitLeavesTheStoresFirstDayOut: the database's weekly rate +// on the admin storage report is the fit of its samples from the end of its +// first day, not from its first sample; each rate names the history it needs +// counting that first day; and a store with less than that has no rate. +func TestIssue2203_TheFitLeavesTheStoresFirstDayOut(t *testing.T) { + c := connect(t) + var report issue2203Report + issue2203Get(t, c, "/api/v1/admin/storage", &report) + var history struct { + Points []issue2203Point `json:"points"` + } + issue2203Get(t, c, "/api/v1/admin/storage/history?store=database&days=30", &history) + + for _, s := range report.Stores { + if s.Store != "database" { + continue + } + if s.Min7d != 48 || s.Min30d != 360 { + t.Errorf("a store whose whole history is kept needs 48 h for a weekly rate and 360 h for a monthly one; the report says %v and %v", s.Min7d, s.Min30d) + } + if s.History < s.Min7d { + if s.Rate7d != nil || s.ExhaustedAt != nil { + t.Errorf("%.1f h of history, short of %v, and the report gives a rate %v and a fill date %v", s.History, s.Min7d, s.Rate7d, s.ExhaustedAt) + } + return + } + if s.History < s.Min30d && s.Rate30d != nil { + t.Errorf("%.1f h of history, short of %v, and the report gives a monthly rate", s.History, s.Min30d) + } + var points []issue2203Point + for _, p := range history.Points { + if p.At.Before(report.GeneratedAt) { + points = append(points, p) + } + } + points = append(points, issue2203Point{At: report.GeneratedAt, Used: s.Used}) + from := points[0].At.Add(24 * time.Hour) + if week := report.GeneratedAt.Add(-7 * 24 * time.Hour); week.After(from) { + from = week + } + want, ok := issue2203Slope(points, from) + if !ok || s.Rate7d == nil { + t.Fatalf("the report's weekly rate %v, and %d samples after the first day", s.Rate7d, len(points)) + } + withSetup, _ := issue2203Slope(points, points[0].At) + t.Logf("database: weekly rate %.0f B/day reported, %.0f B/day fitted from the end of the first day, %.0f B/day fitted from the first sample", *s.Rate7d, want, withSetup) + if math.Abs(*s.Rate7d-want) > math.Max(math.Abs(want)*0.01, 1) { + t.Errorf("the weekly rate is %.0f B/day; fitted from the end of the store's first day it is %.0f", *s.Rate7d, want) + } + return + } + t.Fatal("the storage report names no database store") +} + +// issue2203Scrape reads one replica's metrics and returns the value of the +// unlabeled series name, and of name with the given label text. +func issue2203Scrape(t *testing.T, url, line string) float64 { + t.Helper() + req, err := http.NewRequestWithContext(t.Context(), http.MethodGet, url, http.NoBody) // #nosec G704 -- the platform under test, named by the suite's own environment + if err != nil { + t.Fatal(err) + } + res, err := http.DefaultClient.Do(req) // #nosec G704 -- the platform under test, named by the suite's own environment + if err != nil { + t.Fatalf("GET %s: %v", url, err) + } + defer res.Body.Close() //nolint:errcheck // best-effort close after read + raw, _ := io.ReadAll(res.Body) + for l := range strings.SplitSeq(string(raw), "\n") { + if strings.HasPrefix(l, line+" ") { + v, err := strconv.ParseFloat(strings.TrimSpace(strings.TrimPrefix(l, line)), 64) + if err == nil { + return v + } + } + } + return 0 +} + +// TestIssue2203_EachTickIsSampledByOneReplica: since the replicas started, +// no two database samples lie within one sampling tick of each other, and a +// replica that found the other's sample counted its pass as skipped. Each +// replica runs a pass as it starts, so two replicas started together sampled +// the same tick twice before. +func TestIssue2203_EachTickIsSampledByOneReplica(t *testing.T) { + c := connect(t) + urls := metricsURLs() + if len(urls) < 2 { + t.Fatal("this criterion reads both replicas' metrics: run `make dev` with two replicas, or set ACCEPTANCE_METRICS_URL to both") + } + var started time.Time + var skipped float64 + for _, u := range urls { + start := time.Unix(int64(issue2203Scrape(t, u, "process_start_time_seconds")), 0) + if started.IsZero() || start.Before(started) { + started = start + } + skipped += issue2203Scrape(t, u, `background_loop_iterations_total{loop="storage_history",result="skipped"}`) + } + var history struct { + Points []issue2203Point `json:"points"` + } + issue2203Get(t, c, "/api/v1/admin/storage/history?store=database&days=1", &history) + var since []issue2203Point + for _, p := range history.Points { + if !p.At.Before(started) { + since = append(since, p) + } + } + for i := 1; i < len(since); i++ { + if gap := since[i].At.Sub(since[i-1].At); gap < issue2203Interval*9/10 { + t.Errorf("samples at %s and %s, %s apart: one tick sampled twice", since[i-1].At, since[i].At, gap) + } + } + if skipped < 1 { + t.Errorf("no replica skipped a pass since %s; with two replicas each starting with a pass, one finds the other's sample", started) + } + t.Logf("%d database samples since the replicas started at %s; %v passes skipped", len(since), started, skipped) +} diff --git a/test/acceptance/issue_2230_test.go b/test/acceptance/issue_2230_test.go new file mode 100644 index 000000000..947f2e2c5 --- /dev/null +++ b/test/acceptance/issue_2230_test.go @@ -0,0 +1,120 @@ +//go:build integration + +package acceptance + +import ( + "encoding/base64" + "fmt" + "net/http" + "strings" + "testing" + "time" + + "github.com/lib/pq" +) + +// Acceptance for #2230: a file searched for by the words of its own name was +// missing from the answer, behind files whose names only looked alike. +// PostgreSQL's parser read "acceptance-1627-1792487495133911000" as the word +// "acceptance" and the signed numbers -1627 and -1792487495133911000, so a +// search for "acceptance 1627 1792487495133911000" never matched it, and a +// file not yet embedded had nothing else to be found by. A name is now +// matched by its words on every full-text surface, and an operation not yet +// embedded is blended as the SQL rankers blend a row the vector arm cannot +// see. +// +// Wire forms: search's intent is a string and sources a list of strings; +// manage_resource's content_base64, filename, display_name and path are +// strings; save_asset's name, content and content_type are strings. Each is +// sent as literal params. + +const ( + issue2230Alike = 30 + issue2230Purpose = "Acceptance #2230: a file is found by the words of its name." +) + +// issue2230Upload uploads a small file under acceptance-2230 named name and +// returns its reference. +func issue2230Upload(t *testing.T, c *client, name string) string { + t.Helper() + out := c.call("manage_resource", map[string]any{ + "action": "create", "filename": name + ".bin", "display_name": name, "path": "acceptance-2230", + "description": "Acceptance #2230.", + "content_base64": base64.StdEncoding.EncodeToString([]byte("acceptance 2230 " + name)), + "content_type": "application/octet-stream", + }) + id, _ := out["resource_id"].(string) + if id == "" { + t.Fatalf("manage_resource create returned no resource_id: %v", out) + } + t.Cleanup(func() { _, _ = c.rest(http.MethodDelete, "/api/v1/resources/"+id, http.NoBody) }) + return "mcp:resource:" + id +} + +// issue2230First returns the reference of the first hit search answers for +// intent in one source. +func issue2230First(t *testing.T, c *client, intent, source string) string { + t.Helper() + out := c.call("search", map[string]any{ + "intent": intent, "sources": []any{source}, "limit": 25, "purpose": issue2230Purpose, + }) + groups, _ := out["groups"].([]any) + for _, group := range groups { + g, _ := group.(map[string]any) + hits, _ := g["hits"].([]any) + if len(hits) > 0 { + first, _ := hits[0].(map[string]any) + ref, _ := first["reference"].(string) + return ref + } + } + return "" +} + +// TestIssue2230_AFileIsFoundByTheWordsOfItsName: among thirty embedded files +// named alike, a file uploaded a moment ago and named +// acceptance-2230- is the first resource search answers for +// "acceptance 2230 ", before its own embedding lands. +func TestIssue2230_AFileIsFoundByTheWordsOfItsName(t *testing.T) { + c := connectFor(t, 5*time.Minute) + db := issue1904DB(t) + stamp := time.Now().UnixNano() + var alike []string + for i := range issue2230Alike { + ref := issue2230Upload(t, c, fmt.Sprintf("acceptance-2230-thing-%d-%d", stamp, i)) + alike = append(alike, strings.TrimPrefix(ref, "mcp:resource:")) + } + issue1904Await(t, "the alike files are embedded", func() bool { + return issue1904Count(t, db, `SELECT COUNT(*) FROM resources WHERE id = ANY($1) AND embedding IS NOT NULL`, pq.Array(alike)) == issue2230Alike + }) + + name := fmt.Sprintf("acceptance-2230-%d", stamp+1) + target := issue2230Upload(t, c, name) + if got := issue2230First(t, c, strings.ReplaceAll(name, "-", " "), "resources"); got != target { + t.Errorf("the first resource for %q is %s; want the file named so, %s", strings.ReplaceAll(name, "-", " "), got, target) + } + if got := issue2230First(t, c, name, "resources"); got != target { + t.Errorf("the first resource for its own name %q is %s; want %s", name, got, target) + } +} + +// TestIssue2230_AnAssetIsFoundByTheWordsOfItsName: a saved asset named +// q3-2024--report is the first asset search answers for +// "q3 2024 report". +func TestIssue2230_AnAssetIsFoundByTheWordsOfItsName(t *testing.T) { + c := connect(t) + name := fmt.Sprintf("q3-2024-%d-report", time.Now().UnixNano()) + saved := c.call("save_asset", map[string]any{ + "name": name, "content": "

Q3

", "content_type": "text/html", + "description": "Acceptance #2230: an asset found by the words of its name.", + }) + id, _ := saved["asset_id"].(string) + if id == "" { + t.Fatalf("save_asset returned no asset_id: %v", saved) + } + t.Cleanup(func() { _, _, _ = c.callRaw("manage_asset", map[string]any{"action": "delete", "asset_id": id}) }) + intent := strings.ReplaceAll(name, "-", " ") + if got := issue2230First(t, c, intent, "assets"); got != "mcp:asset:"+id { + t.Errorf("the first asset for %q is %s; want mcp:asset:%s", intent, got, id) + } +} diff --git a/test/acceptance/issue_2231_test.go b/test/acceptance/issue_2231_test.go new file mode 100644 index 000000000..22dde8d5f --- /dev/null +++ b/test/acceptance/issue_2231_test.go @@ -0,0 +1,79 @@ +//go:build integration + +package acceptance + +import ( + "encoding/json" + "io" + "net/http" + "testing" + "time" +) + +// Acceptance for #2231: filtering the call list by outcome took 36 seconds on +// a catalog of about 95,000 records, because the outcome was derived per +// record. Each outcome now answers within a second, on the admin list and on +// the portal's own list. +// +// Wire forms: this ticket adds no tool parameter. GET /api/v1/admin/calls and +// GET /api/v1/portal/calls take outcome and per_page as query strings, the one +// form a query takes. + +// issue2231Bound is the time one filtered page and its total may take. +const issue2231Bound = time.Second + +// issue2231Page reads one page of a call list and returns its total and how +// long the request took. +func issue2231Page(t *testing.T, c *client, path string) (total int, took time.Duration) { + t.Helper() + req, err := http.NewRequestWithContext(t.Context(), http.MethodGet, c.base+path, http.NoBody) // #nosec G704 -- the platform under test, named by the suite's own environment + if err != nil { + t.Fatal(err) + } + req.Header.Set("Authorization", "Bearer "+c.apiKey) + start := time.Now() + res, err := http.DefaultClient.Do(req) // #nosec G704 -- the platform under test, named by the suite's own environment + if err != nil { + t.Fatalf("GET %s: %v", path, err) + } + defer res.Body.Close() //nolint:errcheck // best-effort close after read + raw, _ := io.ReadAll(res.Body) + took = time.Since(start) + if res.StatusCode != http.StatusOK { + t.Fatalf("GET %s: HTTP %d: %s", path, res.StatusCode, raw) + } + var page struct { + Total int `json:"total"` + } + if err := json.Unmarshal(raw, &page); err != nil { + t.Fatalf("GET %s: %v", path, err) + } + return page.Total, took +} + +// TestIssue2231_EachOutcomeAnswersWithinASecond: on the dev catalog, each +// outcome filter on the admin call list and on the portal's own answers within +// a second, and the outcomes' totals add up to the unfiltered total, read +// before and after them. +func TestIssue2231_EachOutcomeAnswersWithinASecond(t *testing.T) { + c := connect(t) + for _, list := range []string{"/api/v1/admin/calls", "/api/v1/portal/calls"} { + // The dev stack records calls while this runs, so the outcomes are + // held between the totals read before and after them. + before, _ := issue2231Page(t, c, list+"?per_page=50") + sum := 0 + for _, outcome := range []string{"failed", "satisfied", "superseded", "ran"} { + total, took := issue2231Page(t, c, list+"?per_page=50&outcome="+outcome) + t.Logf("%s outcome=%s: %d records in %s", list, outcome, total, took) + if took > issue2231Bound { + t.Errorf("%s filtered to %s took %s; the bound is %s", list, outcome, took, issue2231Bound) + } + sum += total + } + after, _ := issue2231Page(t, c, list+"?per_page=50") + t.Logf("%s: %d records before the outcomes were read, %d after", list, before, after) + if sum < before || sum > after { + t.Errorf("%s: the outcomes total %d, outside the list's %d to %d", list, sum, before, after) + } + } +} diff --git a/test/acceptance/issue_2234_test.go b/test/acceptance/issue_2234_test.go new file mode 100644 index 000000000..15dbccd5e --- /dev/null +++ b/test/acceptance/issue_2234_test.go @@ -0,0 +1,100 @@ +//go:build integration + +package acceptance + +import ( + "encoding/base64" + "net/http" + "os" + "testing" + "time" +) + +// #2234: manage_asset update and patch created the new version, which makes +// the asset owed a tile, before they recorded the references the write +// declared, so the tile worker could draw the new version while a newly +// declared file was still refused, and keep that tile until the next content +// change. The references a write declares are now granted before its version +// exists. +// +// Each criterion starts from an asset drawn without the font, and writes the +// #2062 page that declares it: the tile of the new version is the page's +// font-loaded color. +// +// Wire forms: manage_asset update's content is a string and references a list +// of strings; manage_asset patch's edits is a list of objects (op, find, +// replace as strings) and references a list of strings. Each is sent once, as +// literal params. + +// plainPage2234 is the page an asset starts as: one color, no reference. +const plainPage2234 = `export default function App() { + return
; +} +` + +// fontURI2234 uploads the #2062 font as a managed resource and saves an +// asset as the plain page, drawn; it returns the asset's id and the font's URI. +func fontURI2234(t *testing.T, c *client) (assetID, uri string) { + t.Helper() + font, err := os.ReadFile("testdata/issue_2062_font.woff2") + if err != nil { + t.Fatalf("reading the font fixture: %v", err) + } + name := "acceptance-2234-" + unique1794() + out := c.call("manage_resource", map[string]any{ + "action": "create", "filename": name + ".woff2", "display_name": name, "path": "acceptance-2234", + "description": "Acceptance #2234: a brand font an update declares.", + "content_base64": base64.StdEncoding.EncodeToString(font), + "content_type": "font/woff2", + }) + resourceID, _ := out["resource_id"].(string) + if resourceID == "" { + t.Fatalf("manage_resource create returned no resource_id: %v", out) + } + t.Cleanup(func() { _, _ = c.rest(http.MethodDelete, "/api/v1/resources/"+resourceID, http.NoBody) }) + status, row := c.rest(http.MethodGet, "/api/v1/resources/"+resourceID, http.NoBody) + uri, _ = row["uri"].(string) + if status != http.StatusOK || uri == "" { + t.Fatalf("the font resource carries no uri: HTTP %d %v", status, row) + } + saved := c.call("save_asset", map[string]any{ + "name": name, "content": plainPage2234, "content_type": "text/jsx", + "description": "Acceptance #2234: a page that gains a referenced font.", + }) + assetID, _ = saved["asset_id"].(string) + if assetID == "" { + t.Fatalf("save_asset returned no asset_id: %v", saved) + } + t.Cleanup(func() { _, _, _ = c.callRaw("manage_asset", map[string]any{"action": "delete", "asset_id": assetID}) }) + awaitAssetTile1794(t, c, assetID) + return assetID, uri +} + +// TestIssue2234_AnUpdateThatDeclaresAFontIsDrawnWithIt: manage_asset update +// writing the page that declares the font, with the reference, yields a tile +// of the new version drawn in the font. +func TestIssue2234_AnUpdateThatDeclaresAFontIsDrawnWithIt(t *testing.T) { + c := connectFor(t, 3*tileWait1794+time.Minute) + assetID, uri := fontURI2234(t, c) + c.call("manage_asset", map[string]any{ + "action": "update", "asset_id": assetID, "content": fontPage2062(uri), + "references": []any{uri}, "change_summary": "Draw the page in the brand font.", + }) + awaitAssetTile1794(t, c, assetID) + assertDrawnInTheFont2062(t, c, assetID) +} + +// TestIssue2234_APatchThatDeclaresAFontIsDrawnWithIt: manage_asset patch +// replacing the page with the one that declares the font, with the +// reference, yields a tile of the new version drawn in the font. +func TestIssue2234_APatchThatDeclaresAFontIsDrawnWithIt(t *testing.T) { + c := connectFor(t, 3*tileWait1794+time.Minute) + assetID, uri := fontURI2234(t, c) + c.call("manage_asset", map[string]any{ + "action": "patch", "asset_id": assetID, + "edits": []any{map[string]any{"op": "replace", "find": plainPage2234, "replace": fontPage2062(uri)}}, + "references": []any{uri}, "change_summary": "Draw the page in the brand font.", + }) + awaitAssetTile1794(t, c, assetID) + assertDrawnInTheFont2062(t, c, assetID) +} diff --git a/test/acceptance/issue_2236_test.go b/test/acceptance/issue_2236_test.go new file mode 100644 index 000000000..8679b63ee --- /dev/null +++ b/test/acceptance/issue_2236_test.go @@ -0,0 +1,90 @@ +//go:build integration + +package acceptance + +import ( + "bytes" + "encoding/json" + "fmt" + "net/http" + "strings" + "testing" + "time" +) + +// Acceptance for #2236: the api and graphql connection editors set +// oauth_token_lifetime as "Token lifetime". The editor saves through the +// admin connection route, so these criteria save through it as the editor +// does: a value saved is the value read back, an empty field stores nothing, +// and a negative value is refused with the message the editor shows. +// +// Wire forms: oauth_token_lifetime is sent as a duration string ("45m") and +// as a JSON number of seconds (900), the two forms the key admits, and left +// out for an empty field; the rest of the config is strings. + +// issue2236Config is an oauth client_credentials connection of kind to the +// dev stack's Keycloak with the lifetime given; nil leaves the key out. +func issue2236Config(kind, name string, lifetime any) map[string]any { + upstream := map[string]string{"api": "base_url", "graphql": "endpoint_url"}[kind] + cfg := map[string]any{ + upstream: issue1759KeycloakBase(), "connection_name": name, + "auth_mode": "oauth", "oauth_grant": "client_credentials", + "oauth_token_url": issue1759KeycloakBase() + "/realms/mcp-platform/protocol/openid-connect/token", + "oauth_client_id": "acc-2236", "oauth_client_secret": "acc-2236-secret", + } + if lifetime != nil { + cfg["oauth_token_lifetime"] = lifetime + } + return cfg +} + +// issue2236Save saves the connection as the editor does and returns the +// status and the decoded answer. +func issue2236Save(t *testing.T, c *client, kind, name string, cfg map[string]any) (int, map[string]any) { + t.Helper() + raw, err := json.Marshal(map[string]any{"config": cfg, "description": "Acceptance #2236."}) + if err != nil { + t.Fatal(err) + } + return c.rest(http.MethodPut, "/api/v1/admin/connection-instances/"+kind+"/"+name, bytes.NewReader(raw)) +} + +// TestIssue2236_TheTokenLifetimeSavesAndReadsBack: on an api and a graphql +// connection, a lifetime saved as a duration and as seconds reads back as +// saved, an empty field stores nothing, and a negative value is refused +// naming the key. +func TestIssue2236_TheTokenLifetimeSavesAndReadsBack(t *testing.T) { + c := connect(t) + for _, kind := range []string{"api", "graphql"} { + t.Run(kind, func(t *testing.T) { + name := fmt.Sprintf("acc-2236-%s-%d", kind, time.Now().UnixNano()) + t.Cleanup(func() { c.rest(http.MethodDelete, "/api/v1/admin/connection-instances/"+kind+"/"+name, http.NoBody) }) + for _, value := range []any{"45m", float64(900)} { + if status, out := issue2236Save(t, c, kind, name, issue2236Config(kind, name, value)); status != http.StatusOK && status != http.StatusCreated { + t.Fatalf("saving a lifetime of %v answered %d: %v", value, status, out) + } + _, read := c.rest(http.MethodGet, "/api/v1/admin/connection-instances/"+kind+"/"+name, http.NoBody) + cfg, _ := read["config"].(map[string]any) + if fmt.Sprint(cfg["oauth_token_lifetime"]) != fmt.Sprint(value) { + t.Errorf("a lifetime saved as %v (%T) reads back as %v", value, value, cfg["oauth_token_lifetime"]) + } + } + + if status, out := issue2236Save(t, c, kind, name, issue2236Config(kind, name, nil)); status != http.StatusOK && status != http.StatusCreated { + t.Fatalf("saving with the field empty answered %d: %v", status, out) + } + _, read := c.rest(http.MethodGet, "/api/v1/admin/connection-instances/"+kind+"/"+name, http.NoBody) + if cfg, _ := read["config"].(map[string]any); cfg["oauth_token_lifetime"] != nil { + t.Errorf("an empty field stored a lifetime: %v", cfg["oauth_token_lifetime"]) + } + + status, out := issue2236Save(t, c, kind, name, issue2236Config(kind, name, "-5m")) + if status != http.StatusBadRequest { + t.Fatalf("a negative lifetime answered %d: %v", status, out) + } + if detail, _ := out["detail"].(string); !strings.Contains(detail, "oauth_token_lifetime must not be negative") { + t.Errorf("the refusal says %q; want it to name the key", detail) + } + }) + } +} diff --git a/test/acceptance/issue_2237_test.go b/test/acceptance/issue_2237_test.go new file mode 100644 index 000000000..491b6ff2b --- /dev/null +++ b/test/acceptance/issue_2237_test.go @@ -0,0 +1,56 @@ +//go:build integration + +package acceptance + +import ( + "net/http" + "strings" + "testing" +) + +// Acceptance for #2237: a Google API refusing a call a service-account +// connection made carries the operator's next step, chosen by the refusal's +// status and reasons rather than by one product's message text. Google answers +// every API in one error shape, so a 401 UNAUTHENTICATED with any message in +// that shape is read as an account without access in the product. +// +// The connection is #2061's: a Google key file whose grant Keycloak answers, +// with a Google API as its upstream. Google refuses the Keycloak-issued token +// with a 401 UNAUTHENTICATED in its own shape, which carried no hint before +// this change: it is neither PERMISSION_DENIED nor the one product's text the +// hint used to match. +// +// Wire forms: google_service_account_json is sent as a string of JSON, +// oauth_scope as a space-delimited string and base_url as a string; +// api_invoke_endpoint takes method and path strings and no body. + +const googleAPI2237 = "https://sheets.googleapis.com" + +// TestIssue2237_AGoogle401CarriesTheAccessStep: a call through a Google +// service-account connection that Google refuses with 401 UNAUTHENTICATED +// answers with the hint to grant the account access inside the product. +func TestIssue2237_AGoogle401CarriesTheAccessStep(t *testing.T) { + admin := connect(t) + name := "acc-2237-" + unique1579() + if status := connection2061(t, admin, name, grant2061(map[string]any{ + "google_service_account_json": keyFile2061(t), + "base_url": googleAPI2237, + })); status != http.StatusOK && status != http.StatusCreated { + t.Fatalf("saving the connection answered %d", status) + } + out := admin.call("api_invoke_endpoint", map[string]any{ + "connection": name, "method": http.MethodGet, "path": "/v4/spreadsheets/acceptance-2237", + "purpose": "Acceptance #2237: what a Google refusal tells the operator.", + }) + if status, _ := out["status"].(float64); status != http.StatusUnauthorized { + t.Fatalf("Google answered %v, not 401: %v", out["status"], out) + } + body, _ := out["body"].(map[string]any) + refusal, _ := body["error"].(map[string]any) + if refusal["status"] != "UNAUTHENTICATED" { + t.Fatalf("the refusal is not in Google's shape: %v", body) + } + if hint, _ := out["hint"].(string); !strings.Contains(hint, "grant the service account's email access inside the product") { + t.Errorf("the hint is %q; want the access step", hint) + } +} diff --git a/test/structure/testdata/allowed_internal_imports.txt b/test/structure/testdata/allowed_internal_imports.txt index ec4042647..95d042693 100644 --- a/test/structure/testdata/allowed_internal_imports.txt +++ b/test/structure/testdata/allowed_internal_imports.txt @@ -95,6 +95,7 @@ internal/admin/webhookapi -> internal/webhook/whadmin internal/admin/webhookapi -> internal/webhook/whsource internal/admin/webhookapi -> internal/webhook/whstore internal/admin/webhookapi -> internal/webhook/whtable +internal/advisorylock -> internal/logsan internal/agentinstructions -> pkg/configstore internal/apigwmetrics -> pkg/mcpcontext internal/apigwmetrics -> pkg/observability @@ -106,6 +107,7 @@ internal/botcheck -> internal/platform/settingsjson internal/botcheck -> pkg/portal/shareguest internal/catalogfake -> pkg/catalog internal/conncatchup -> internal/logsan +internal/connlimit -> internal/advisorylock internal/connlimit -> pkg/observability internal/connsecretref -> internal/secretref internal/connsecretref -> internal/secretstore @@ -554,6 +556,7 @@ internal/indexpolicy -> pkg/contenttype internal/inlinefit -> pkg/toolkit internal/inproc -> internal/portal/viewerlimit internal/listcut -> internal/pagenext +internal/maps -> internal/advisorylock internal/maps -> internal/bgloop internal/maps -> internal/logsan internal/maps -> internal/pmtiles @@ -627,6 +630,7 @@ internal/platform/callcatchup -> pkg/registry internal/platform/callcatchup -> pkg/toolkit internal/platform/callindex -> internal/platform/callrecord internal/platform/callindex -> pkg/indexjobs +internal/platform/callrecord -> internal/advisorylock internal/platform/callrecord -> internal/bgloop internal/platform/callrecord -> internal/gqlschema internal/platform/callrecord -> internal/httpjson @@ -644,6 +648,7 @@ internal/platform/calltask -> internal/logsan internal/platform/calltask -> internal/mcptask internal/platform/calltask -> pkg/mcpcontext internal/platform/calltask -> pkg/middleware +internal/platform/capacity -> internal/advisorylock internal/platform/capacity -> internal/bgloop internal/platform/capacity -> internal/logsan internal/platform/capacity -> internal/objectobs @@ -651,9 +656,11 @@ internal/platform/capacity -> internal/outbound internal/platform/capacity -> internal/platform/toolkitcfg internal/platform/capacity -> pkg/observability internal/platform/capacity -> pkg/portal/s3adapter +internal/platform/catalogimport -> internal/advisorylock internal/platform/catalogimport -> internal/logsan internal/platform/catalogimport -> internal/outbound internal/platform/catalogimport -> pkg/catalog +internal/platform/catalogscan -> internal/advisorylock internal/platform/catalogscan -> internal/bgloop internal/platform/catalogscan -> internal/logsan internal/platform/catalogscan -> pkg/catalog @@ -772,8 +779,8 @@ internal/platform/knowledgelayer -> pkg/portal/knowledgepage internal/platform/knowledgelayer -> pkg/toolkits/knowledge internal/platform/knowledgepageindex -> pkg/indexjobs internal/platform/knowledgepageindex -> pkg/portal/knowledgepage +internal/platform/levelalert -> internal/advisorylock internal/platform/levelalert -> internal/bgloop -internal/platform/levelalert -> internal/logsan internal/platform/levelalert -> pkg/notification internal/platform/listchanged -> pkg/session internal/platform/listenbridge -> internal/logsan @@ -935,6 +942,7 @@ internal/platform/resourcelayer -> pkg/indexjobs internal/platform/resourcelayer -> pkg/observability internal/platform/resourcelayer -> pkg/portal/s3adapter internal/platform/resourcelayer -> pkg/resource +internal/platform/resourcereclass -> internal/advisorylock internal/platform/resourcereclass -> internal/indexpolicy internal/platform/resourcereclass -> pkg/indexjobs internal/platform/resourcereclass -> pkg/resource @@ -954,6 +962,7 @@ internal/platform/resultbudget -> pkg/mcpcontext internal/platform/resultbudget -> pkg/middleware internal/platform/resultbudget -> pkg/registry internal/platform/resultbudget -> pkg/toolkit +internal/platform/retention -> internal/advisorylock internal/platform/retention -> internal/bgloop internal/platform/retention -> internal/botcheck internal/platform/retention -> internal/endpoint/epbinding @@ -1438,7 +1447,7 @@ internal/shutdown -> internal/inprocess internal/soap -> internal/xmltree internal/tableavail -> pkg/query internal/tableparquet -> internal/tabletype -internal/thumbdue -> internal/logsan +internal/thumbdue -> internal/advisorylock internal/thumbtypes -> pkg/contenttype internal/toolanswer -> pkg/registry internal/toolanswer -> pkg/toolkit @@ -1580,7 +1589,9 @@ pkg/admin -> pkg/toolkits/gateway/enrichment pkg/admin -> pkg/toolkits/knowledge pkg/admin -> pkg/user pkg/audit -> pkg/observability +pkg/audit/postgres -> internal/advisorylock pkg/audit/postgres -> internal/bgloop +pkg/audit/postgres -> internal/logsan pkg/audit/postgres -> pkg/audit pkg/audit/postgres -> pkg/prompt pkg/audit/postgres -> pkg/resource @@ -1602,6 +1613,7 @@ pkg/configstore/postgres -> internal/opsobs pkg/configstore/postgres -> pkg/configstore pkg/connid -> pkg/registry pkg/connid -> pkg/toolkit +pkg/connoauth -> internal/advisorylock pkg/connoauth -> internal/bgloop pkg/connoauth -> internal/logsan pkg/connoauth -> internal/outbound diff --git a/ui/e2e/interactive/connections-oauth.spec.ts b/ui/e2e/interactive/connections-oauth.spec.ts index b8dbc25e3..bb7959262 100644 --- a/ui/e2e/interactive/connections-oauth.spec.ts +++ b/ui/e2e/interactive/connections-oauth.spec.ts @@ -40,6 +40,11 @@ test.describe("Connection editor — a canonical OAuth connection", () => { await expect( page.getByRole("button", { name: "Connect", exact: true }), ).toBeVisible(); + // #2236: how long a token is reused when its endpoint states no expiry. + await expect(page.getByLabel("Token lifetime")).toHaveValue("30m"); + await expect( + page.getByText("Used when the token endpoint states no expiry"), + ).toBeVisible(); }); }); diff --git a/ui/e2e/screenshots/route-connections.ts b/ui/e2e/screenshots/route-connections.ts index bbb35560b..ed0bb19cd 100644 --- a/ui/e2e/screenshots/route-connections.ts +++ b/ui/e2e/screenshots/route-connections.ts @@ -53,6 +53,25 @@ export const adminConnectionRoutes: ScreenshotRoute[] = [ } }, }, + { + // The token lifetime field (#2236) under the OAuth block of an api + // connection whose token endpoint states no expiry. + slug: "admin-connection-token-lifetime", + path: "/portal/admin/connections?kind=api&name=acme-billing-api", + category: "admin", + beforeCapture: async (page) => { + const edit = page.locator("button:has-text('Edit')").first(); + if (await edit.isVisible()) { + await edit.click(); + await page.waitForTimeout(600); + } + const lifetime = page.locator("text=Token lifetime").first(); + if (await lifetime.isVisible()) { + await lifetime.scrollIntoViewIfNeeded(); + await page.waitForTimeout(300); + } + }, + }, { // A connection carrying both OAuth config vocabularies (#1682). The values // nothing reads are struck through and badged rather than listed beside the diff --git a/ui/src/mocks/data/connections.ts b/ui/src/mocks/data/connections.ts index 9295bf415..748dddc03 100644 --- a/ui/src/mocks/data/connections.ts +++ b/ui/src/mocks/data/connections.ts @@ -126,6 +126,8 @@ export const mockConnectionInstances: ConnectionInstance[] = [ oauth_client_secret: "[REDACTED]", oauth_scope: "billing.read", oauth_endpoint_auth_style: "params", + // The billing token endpoint states no expires_in (#2236). + oauth_token_lifetime: "30m", }, description: "HTTP API gateway over the internal billing service. Exposes invoice and subscription lookups as a single `api_invoke` tool with discovery.", diff --git a/ui/src/mocks/handlers.ts b/ui/src/mocks/handlers.ts index 5c9ecf456..358a19f87 100644 --- a/ui/src/mocks/handlers.ts +++ b/ui/src/mocks/handlers.ts @@ -1797,6 +1797,8 @@ export const handlers = [ oauth_client_secret: "[REDACTED]", oauth_scope: "billing.read", oauth_endpoint_auth_style: "params", + // The billing token endpoint states no expires_in (#2236). + oauth_token_lifetime: "30m", }, created_by: "admin@example.com", updated_at: new Date(Date.now() - 4 * 86400000).toISOString(), diff --git a/ui/src/pages/settings/connections/ApiGatewayAuthFields.test.tsx b/ui/src/pages/settings/connections/ApiGatewayAuthFields.test.tsx index fcace7d59..c6fa4b6db 100644 --- a/ui/src/pages/settings/connections/ApiGatewayAuthFields.test.tsx +++ b/ui/src/pages/settings/connections/ApiGatewayAuthFields.test.tsx @@ -209,3 +209,39 @@ describe("ApiGatewayAuthFields — the jwt_bearer grant", () => { ); }); }); + +// #2236: oauth_token_lifetime, how long a token whose endpoint states no +// expiry is reused, is set from the form on api and graphql connections. +describe("ApiGatewayAuthFields — token lifetime", () => { + it("shows the field with its help when the mode is oauth, holding the stored value", () => { + renderFields({ ...canonical, oauth_token_lifetime: "45m" }); + + const field = screen.getByLabelText(/^Token lifetime$/i); + expect(field).toHaveValue("45m"); + expect(screen.getByText("Used when the token endpoint states no expiry")).toBeInTheDocument(); + }); + + it("shows a value stored as seconds", () => { + renderFields({ ...canonical, oauth_token_lifetime: 900 }, "graphql"); + + expect(screen.getByLabelText(/^Token lifetime$/i)).toHaveValue("900"); + }); + + it("writes oauth_token_lifetime", () => { + const onChange = renderFields(canonical); + fireEvent.change(screen.getByLabelText(/^Token lifetime$/i), { target: { value: "30m" } }); + expect(onChange).toHaveBeenLastCalledWith(expect.objectContaining({ oauth_token_lifetime: "30m" })); + }); + + it("stores nothing when emptied, so the server's default applies", () => { + const onChange = renderFields({ ...canonical, oauth_token_lifetime: "30m" }); + fireEvent.change(screen.getByLabelText(/^Token lifetime$/i), { target: { value: "" } }); + expect(onChange.mock.lastCall?.[0]).not.toHaveProperty("oauth_token_lifetime"); + }); + + it("is not shown for a mode that fetches no token", () => { + renderFields({ auth_mode: "bearer", credential: "[REDACTED]" }); + + expect(screen.queryByLabelText(/^Token lifetime$/i)).not.toBeInTheDocument(); + }); +}); diff --git a/ui/src/pages/settings/connections/ApiGatewayAuthFields.tsx b/ui/src/pages/settings/connections/ApiGatewayAuthFields.tsx index bd5c3eede..5ade484ef 100644 --- a/ui/src/pages/settings/connections/ApiGatewayAuthFields.tsx +++ b/ui/src/pages/settings/connections/ApiGatewayAuthFields.tsx @@ -193,6 +193,21 @@ function ConnectPanel({ ); } +// TokenLifetimeField sets oauth_token_lifetime (#2236): how long a token is +// reused when its endpoint's response states no expires_in. Empty stores +// nothing, and the server's 15m default applies. +function TokenLifetimeField({ config, onChange }: ConfigFormProps) { + return ( + onChange(update(config, "oauth_token_lifetime", v))} + placeholder="15m" + /> + ); +} + // ApiGatewayAuthFields renders the mode picker plus whichever credential // block the selected mode needs. The mode set matches what the apigateway // toolkit accepts (pkg/toolkits/apigateway/config.go); mtls carries no fields @@ -259,19 +274,22 @@ export function ApiGatewayAuthFields({ )} {mode === AUTH_MODE_OAUTH && ( - - } - /> + <> + + } + /> + + )}