Skip to content

fix(workspace): back off a snapshot query that fails on a working connection - #1022

Open
psteinroe wants to merge 4 commits into
perf/drop-unused-connectionsfrom
fix/snapshot-query-backoff
Open

psteinroe wants to merge 4 commits into
perf/drop-unused-connectionsfrom
fix/snapshot-query-backoff

Conversation

@psteinroe

@psteinroe psteinroe commented Oct 26, 2026 •

Copy link
Copy Markdown
Collaborator

Stacked on #945.

Since #903, only connection failures put a database into the reconnect backoff, so that a statement that fails on a working connection doesn't take the database offline. The flip side, noted in #903's risks: when the schema snapshot query itself fails on a working connection, for example because the role may not read part of the catalog, nothing throttles it. Every request that needs the schema (diagnostics, hover, completions, inlay hints, semantic tokens) ran the snapshot queries again, only for them to fail the same way.

What changed

  • A small Backoff type (server/backoff.rs) holds the schedule that ConnectionManager already used: 5 seconds after the first failure, doubling with every failure in a row, up to a minute. ConnectionManager uses it instead of its own attempts/next_retry_at fields; its behaviour doesn't change.
  • The snapshot slot of a connection (perf(workspace): don't let one database block the others #921) now also remembers the last failure to load the snapshot when it wasn't a connection failure. During its backoff, SnapshotManager::load returns that error right away instead of querying again; after it, the next request retries, and a success clears it. Connection failures are still left to ConnectionManager, as before, so they aren't backed off twice.
  • The backoff goes away with the slot: pgls.invalidateSchemaCache (which clears all snapshots) or connecting as another role or to another database retries right away, so a user who fixed the permission doesn't have to wait.
  • The first failure is logged as a warning with the delay, like a connection failure.

Requests during the backoff still get the error, not an empty result, so hover keeps saying why the schema is missing.

Tests

  • snapshot_manager::tests::failing_snapshot_queries_are_backed_off: a connection as a role that may not read pg_catalog.pg_proc loads the snapshot and gets permission denied. The test then grants the permission; the next load still returns the error without querying, and after the backoff expires the load succeeds and clears the failure. Without the backoff, the second load succeeds and the test fails.
  • backoff::tests::doubles_up_to_a_minute covers the schedule. The existing connection_manager tests pass unchanged apart from building a Backoff.

Not covered

  • The other database queries (type check, splinter, plpgsql_check) aren't backed off when they fail on a working connection. They run per statement or per file, and their errors are reported as diagnostics rather than repeated silently.

Update: The test no longer creates a login role, which outlived the test's database because roles belong to the cluster and showed up in other tests' role completions. Its connections run as test_login from the test migrations instead (-c role=test_login as a startup option); the revoke only affects the test's own database.

Update: the remembered failure belongs to the connection options it happened with. The connection manager creates a new pool when settings that ConnectionKey doesn't cover change (password, connection string parameters such as a startup role), and a load with that pool retries right away instead of returning the old error until the backoff ends. Test: failing_snapshot_queries_are_retried_with_other_options (a pool with role=test_login fails, a pool for the same host, user and database without it loads right away; fails before).

// Callers for the same connection wait for one load instead of starting their own.
let mut snapshot = slot.snapshot.lock().unwrap();
if let Some(cache) = snapshot.as_ref() {
let mut state = slot.state.lock().unwrap();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cached failure survives connection-option changes that preserve host/port/username/database. I reproduced this by loading under a restricted startup role, then replacing the pool with working options: the replacement pool can read pg_proc, but load still returns the previous permission error. Explicit cache invalidation immediately restores loading.

ConnectionManager recreates pools when settings_fingerprint changes, whereas this failure remains attached only to ConnectionKey, potentially delaying recovery by up to 60 seconds.

Maybe we could invalidate the cached failure when the effective connection options change, and add a regression test for this case.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants