Skip to content

Complete multi-database installation with short-lived metadata fences - #401

Open
Gokhan Gulbiz (gokhangulbiz) wants to merge 8 commits into
mainfrom
gokhangulbiz-complete-durable-multi-db-pr
Open

Gokhan Gulbiz (gokhangulbiz) wants to merge 8 commits into
mainfrom
gokhangulbiz-complete-durable-multi-db-pr

Conversation

@gokhangulbiz

Copy link
Copy Markdown

Summary

Continuation of Pino de Candia's multi-database implementation in #386, integrated with current main. Pino's original contribution is acknowledged in the commit attribution. This draft does not alter or close #386 and does not claim to solve all lifecycle problems. Maintainer review and release conclusions remain outstanding.

  • Preserve the shared-control/local-satellite model: one runtime/provider in the configured control database, with origin-local df APIs, metadata, grants, RLS and credentials in satellites. Explicit SQL targets retain name-based semantics and do not require pg_durable.
  • Keep the shipped 0.2.7 → 0.2.8 migration unchanged; put installation identity/validation DDL in the unreleased 0.2.8 → 0.2.9 upgrade. Preserve legacy control IDs, recorded payloads and old-control-schema compatibility.
  • Restore SQL autocommit for control, default satellite, explicit same-origin and remote execution, including VACUUM and CREATE INDEX CONCURRENTLY. Exact execution-connection OID/UUID preflights end before business SQL and explicitly roll back validation failures.
  • Remove activity-long metadata guards and their client-side DDL lock cycles. Graph/status/retention/reconciliation operations validate identity and access metadata in short transactions on the same connection, outside engine calls and arbitrary activity waits.
  • Refresh trusted source identity after resource waits and before dispatch. Keep endpoint/secret lookup origin-local, preserve credential snapshot consistency, and refresh HTTP execution permission after catalog waits before sending. Preserve current-main HTTP policy, client reuse and secret handling.
  • Update deterministic regressions and documentation for forced/same-OID replacement, metadata-session loss and colliding replacement IDs, both DDL orders, HTTP authorization/credential waits, and upgrade compatibility. Former negative autocommit/lock-cycle diagnostics are now passing normal regressions.

Removal and admission contract

Normal DROP adds no activity-long drain. Ordinary PostgreSQL locks held by caller transactions or user statements still apply and may delay DDL. Already-admitted remote SQL may finish after source removal; committed SQL and accepted HTTP cannot be retracted. Old metadata operations remain fenced from replacement installations.

Fresh source validation rejects the tested stale queued/pre-dispatch cases, but validation is not atomic with a remote send. A source can disappear after the last successful check and before dispatch. No DDL interception hook or administrative quiesce API is introduced.

Deferred items (4 and 5)

4. Preexisting Duroxide task ownership and shutdown bug

Shared control amplifies the impact of an existing runtime lifetime issue. A deterministic live-Tokio reproduction against pinned Duroxide 0.1.30 holds an activity handler, awaits shutdown(Some(0)) or shutdown(Some(100)), and only then releases the handler. In both cases the handler emits an in-process notification after shutdown returns. This is a runtime task-ownership reproduction, not a SQL/HTTP effects reproduction.

The upstream fix needs ownership of the complete descendant task tree, cancellation and abort-plus-join on every shutdown path, including activity managers, handlers and acknowledgment work. A regression must assert no post-stop handler effect, surviving descendant or late acknowledgment after shutdown returns. Duroxide is neither fixed nor bumped here.

5. Preexisting end-to-end SQL cancellation and quiescence gap

Dropping a Rust task, future, socket or execution permit is not confirmation that the PostgreSQL backend has stopped and drained. This requires pg_durable-owned cleanup and exact-backend cancellation/termination with confirmed drain, plus a persistent admission/quiesce policy coordinated with upstream task ownership. It is not solely a Duroxide fix: joining Rust descendants alone does not prove SQL stopped, while local SQL cleanup alone does not reap orphaned runtime dispatch/ack tasks.

Already-committed SQL and remotely accepted HTTP cannot be undone. The final-check-to-send interval and already-admitted-effects overlap of issue 3 remain explicitly unclaimed and deferred. Full persistent administrative quiesce/resume, restart and queued-work semantics are outside this draft's implemented scope.

Validation

Executed in a disposable PostgreSQL 17.7 / Rust 1.93 / pgrx 0.16.1 environment, with recorded source/build/install provenance retained locally:

  • 425 unit tests passed; 16 ignored. Includes exact-connection OID/UUID validation, error rollback, lock release, preserved timeout/isolation settings and autocommit after preflight.
  • 149 upgrade checks passed: fresh/upgraded schema equivalence, supported control schemas 0.2.2–0.2.8 alongside current satellites, and retained/in-flight work with ABI/OID/grant preservation.
  • 16 focused E2E files passed: 11 multi-database cases 75–85, 14_database, and HTTP feature/policy cases 47, 48, 69 and 70. These include all ten autocommit route/statement cases, both DDL orders, queued A→A/A→B replacement, same-OID UUID replacement with a surviving connection, no writes into replacement metadata, and zero disallowed HTTP sends after catalog waits.
  • cargo build --features pg17,http-allow-test-domains
  • cargo fmt -p pg_durable -- --check
  • cargo clippy --features pg17,http-allow-test-domains -- -D warnings
  • Publication checks confirmed the working tree matches the validated handoff, apart from LF-only normalization of new test files; Python syntax and staged diff checks passed. The documented post-test clarification only explains that native PostgreSQL locks can still delay DROP.

Not claimed/run for this final phase: full repository E2E suite, PG18, production/release-package certification, shipped-package upgrade validation, representative many-origin load, repeated-epoch/task/connection-budget certification, or end-to-end live control-replacement SQL/HTTP quiescence. An earlier test-feature package build is not certification of this phase's production release.

Keep this PR in draft pending maintainer review of the admission/removal contract and disposition of the deferred lifecycle items.

Gokhan Gulbiz and others added 2 commits September 22, 2026 20:01
Continue Pino de Candia's implementation from PR #386 on current main. Preserve SQL autocommit, fence origin-local metadata in short transactions, and refresh source identity and HTTP authorization after resource waits. Include migrations, deterministic regressions and documented deferred runtime/SQL quiescence limitations.

Co-authored-by: Pino de Candia <pinod@microsoft.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Keep restart-sensitive and TLS-mock cases in the primary CI runner, preserve SQL failure output under errexit, and fail explicitly on startup or copy errors. Add isolated mock runner regressions and dedicated image/container overrides.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

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.

Copilot review overview

🟡 Changes recommended

Seven unresolved findings cover runner pass detection, monitoring overhead, regression coverage, and persistent E2E cleanup.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 Medium severity

Open (1)
What changed in this PR

This PR completes shared-control multi-database installations with origin-local metadata fencing, autocommit execution, HTTP authorization refreshes, migration support, and focused regression coverage.

Changes:

  • Adds origin routing, installation identity, metadata validation, and namespaced engine IDs.
  • Updates SQL/HTTP execution, monitoring, reconciliation, and upgrade handling.
  • Adds E2E, upgrade, Docker-runner, CI, and documentation coverage.
File Reviewed change / status
tests/​test_docker_runner.py Docker runner unit coverage.
tests/​e2e/​sql/​85_multi_database_http_admission.sql HTTP admission and revalidation coverage.
tests/​e2e/​sql/​84_multi_database_metadata_fence.sql Metadata-session replacement coverage.
tests/​e2e/​sql/​83_multi_database_same_oid.sql Same-OID replacement coverage.
tests/​e2e/​sql/​82_multi_database_ddl_cycle.sql DDL and activity ordering coverage.
tests/​e2e/​sql/​81_multi_database_autocommit.sql Autocommit route coverage; moderate cleanup finding remains.
tests/​e2e/​sql/​80_multi_database_http_origin.sql Origin-local HTTP coverage; moderate authorization cleanup finding remains.
tests/​e2e/​sql/​79_multi_database_remote_origin.sql Remote stale-origin SQL coverage.
tests/​e2e/​sql/​78_multi_database_force_drop.sql Forced-drop fencing coverage.
tests/​e2e/​sql/​77_multi_database_guards.sql Metadata fence and DDL coverage.
tests/​e2e/​sql/​76_multi_database_reconcile.sql Origin reconciliation coverage.
tests/​e2e/​sql/​14_database.sql Control/satellite lifecycle coverage; moderate overload cleanup finding remains.
tests/​e2e/​http_origin_server.py TLS credential-verification test server.
src/​types.rs Dynamic provider-schema and control-state resolution; moderate regression-test finding remains.
src/​registry.rs Routes activities by origin.
src/​origin.rs Origin identity, routing, validation, and connection budgeting.
src/​monitoring.rs Namespaced monitoring lookups; moderate repeated-query performance finding remains.
src/​lib.rs Installation setup, provider schema, and configuration GUCs.
src/​explain.rs Satellite engine-ID resolution.
src/​endpoints.rs Origin-aware endpoint catalog validation.
src/​client.rs Shared control-state discovery and namespaced client operations.
src/​activities/​update_node_status.rs Fenced node status updates.
src/​activities/​update_instance_status.rs Fenced instance status updates.
src/​activities/​load_function_graph.rs Origin-local metadata graph loading.
src/​activities/​http_response.rs Origin-aware response sinks.
src/​activities/​execute_sql.rs Origin validation and autocommit preflight.
src/​activities/​execute_multipart.rs Origin-aware multipart execution.
src/​activities/​execute_http.rs Origin-aware HTTP authorization.
sql/​pg_durable--0.2.8--0.2.9.sql Installation identity migration DDL.
scripts/​test-upgrade.sh Mixed-version upgrade validation.
scripts/​test-e2e-local.sh Multi-database and HTTP test orchestration.
scripts/​test-e2e-docker.sh Docker test handling; moderate explicit-pass-marker finding remains.
docs/​upgrade-testing.md Upgrade guidance; nit regarding stale OnceLock behavior remains.
docs/​spec-security-model.md Origin-local HTTP security model.
docs/​multi-database.md Multi-database architecture and lifecycle scope.
docs/​http-security.md HTTP admission and sink guarantees.
docs/​api-reference.md Multi-database APIs, IDs, metrics, and configuration.
CHANGELOG.md Feature and limitation notes.
.github/​workflows/​docker.yml Docker runner CI coverage.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/types.rs Outdated

Copilot AI left a comment

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.

Copilot review overview

🔵 Needs a closer look

Fix the Docker runner’s incomplete-test false pass and avoid repeated catalog queries during satellite monitoring listings.

Review effort: Lite
Findings: None

Resolved since last review (1)
Previously missed (2)

In code that hasn't changed since last review

Medium severity Incomplete SQL tests can incorrectly pass

scripts/​test-e2e-docker.sh:262

A zero exit status with no TEST FAILED marker is now counted as a pass, so a truncated or otherwise incomplete SQL test can silently succeed. The runner previously required TEST PASSED; keep the failure-marker precedence but retain a separate positive-marker branch and fail when neither marker is present.

Medium severity Per-row identity lookups regress satellite listing performance

src/​monitoring.rs:161

For satellite listings this now calls backend_engine_id once per returned row, and that helper performs SPI lookups for the current database and installation identity on each call. A page of the allowed 1,000 instances therefore adds up to 2,000 backend catalog queries before the single batched engine query, which is a substantial regression for a read API. Resolve the origin identity once per listing (or batch it) and derive all engine IDs from that value.

Gokhan Gulbiz and others added 5 commits September 25, 2026 10:45
Integrate HTTP body policies and benchmarks from main. Preserve origin-local credentials and final authorization checks, default response sinks to the workflow origin, and fence storage admission without changing explicit remote targets. Extend origin HTTP tests to cover default, same-origin and remote sinks.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Rename the pure schema-name validator test to match its assertions and remove the repeated legacy-schema assertion. Do not imply it exercises installation replacement or schema re-resolution.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Give each route/statement case its own table while retaining parallel execution, terminal-state checks and valid-index assertions. Verified 20 PG17 runs covering 200 successful cases.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Require an explicit success marker and resolve caller origin once per monitoring batch without introducing a lifetime cache.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Retain origin-aware response-sink guidance when integrating main's HTTP security GUC and verify legacy/satellite ID derivation across a full listing-sized batch.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

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.

Copilot review overview

🔵 Needs a closer look

Four moderate findings remain regarding validator privileges, connection budgeting, and satellite schema resolution.

Review effort: Lite
Findings: None

Copilot AI left a comment

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.

Copilot review overview

🔵 Needs a closer look

The broad cross-database lifecycle changes require maintainer review, and tolerant monitoring APIs currently regress during satellite control outages.

Review effort: Balanced
Findings: 1 Medium severity

Open (1)

Comment thread src/types.rs
Use fallible schema resolution for both listing overloads, instance info and metrics. Warn and return empty rows on lookup failure while keeping execution history and workflow mutations strict. Cover bounded outage fallback and same-session recovery in the multi-database regression.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

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