Skip to content

fix: separate shared-pool auth from fixed rollout readiness - #1206

Merged
benben merged 1 commit into
mainfrom
ben/fix-pool-readiness-bootstrap
Sep 20, 2026
Merged

benben merged 1 commit into
mainfrom
ben/fix-pool-readiness-bootstrap

Conversation

@benben

@benben benben commented Sep 20, 2026

Copy link
Copy Markdown
Member

Summary

Shared-pool Gateway authentication reuses DUCKGRES_TRINO_ROLLOUT_TOKEN_FILE. The fixed-slot readiness factory incorrectly treated that token as opting into static canary validation, preventing pool-only configurations from starting.

  • Select fixed-cell readiness by the actual registry mode.
  • Leave readiness disabled for pool-only registries without explicit canary configuration, including deployments retaining the legacy single coordinator.
  • Keep incomplete fixed/mixed configuration and explicit invalid canaries fail-closed. Pool Gateway token validation remains unchanged.
  • Document the configuration boundary and real-cluster acceptance limitation.

Validation

  • Red regression reproduced the startup failure with a real shared-pool mode fixture; all 15 configuration cases now pass.
  • Existing readiness, managed Gateway, and pool wiring tests pass through just test-trino.
  • just lint passes with an isolated cache; git diff --check passes.
  • Independent adversarial review completed.
  • Real-cluster pool acceptance still requires deployment of the resulting image. The existing active-pool harness covers that path; this PR does not claim it has passed.

@benben
benben requested a review from a team September 20, 2026 19:27
@github-actions

Copy link
Copy Markdown

Test Impact Plan

Deterministic summary of how this PR changes tests, CI runners, and coverage-risk signals.

Summary

Area Added Changed Deleted
Test files 0 2 0
E2E/journey files 0 0 0
Workflow files 0 0 0

Signals

  • Test cases: +1 / -0
  • Assertions: +5 / -0
  • Skips or known failures added: 0
  • Workflow continue-on-error added: 0
  • Workflow path filters added: 0
  • Test commands removed from justfile: 0
  • E2E/journey retry lines added: 0

Coverage risk: neutral or increased

No coverage-reduction warnings detected.

@bill-ph bill-ph left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed 86e0118. No actionable findings or P0 blocker identified.

The readiness factory now excludes shared-pool cells from fixed-slot validation while preserving failure for incomplete fixed/mixed configurations and explicit pool-only canary configuration. Legacy unregistered wiring does not accidentally count as a fixed cell. I cross-checked the separate pool Gateway construction/token-reading path; this change does not remove that path. The table-driven tests cover the important selection boundaries and verify that mixed-fleet handlers contain only fixed slots.

Overengineering/scope-creep check: the small mode-based filter addresses the shared-token coupling without adding a new flag or credential. The test and documentation additions are proportional and in scope.

Validation: static review of the full diff and surrounding readiness and pool Gateway wiring. I did not run local tests or real-cluster acceptance; the documented deployment/active-pool acceptance check remains necessary.

— Robo Bill

@benben
benben merged commit 39c2363 into main Sep 20, 2026
27 checks passed
@benben
benben deleted the ben/fix-pool-readiness-bootstrap branch September 20, 2026 19:38
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