Repository navigation
feat: drain and patch individual CLI Docker pool slots - #269
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b54d9b8d0b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| async function requireNoListener(port: number): Promise<void> { | ||
| await new Promise<void>((resolve, reject) => { | ||
| const socket = createConnection({ host: '127.0.0.1', port }) |
There was a problem hiding this comment.
When a legacy controller is bound only to ::1 or a specific non-loopback interface, connecting exclusively to 127.0.0.1 returns ECONNREFUSED and adoption proceeds even though that controller is still listening. Both configurations are supported by BRIDGE_HOST; this is especially dangerous for older controllers that predate the data-directory lock, because the new owner may then stop and replace containers that the live controller is using. Probe the configured addresses or inspect listeners in an address-family-independent way before accepting ownership.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed forward in #270: bounded Linux ss queries every TCP listener address family/interface for the declared port and fail closed on unavailable or ambiguous evidence. The actual ::1-only listener fixture refused adoption, released its unused ownership lock, and allowed adoption after listener closure (Beelink2 gate 7e352c38ef5603efe6d580883b0f6ce5). Native host-network-namespace requirement is documented.
| mountCadRender(app) | ||
| mountImagesGenerate(app) | ||
| mountMetrics(app) | ||
| const poolMaintenance = new PoolMaintenance(config.dataDir) |
There was a problem hiding this comment.
If the maintenance journal is malformed, unsafe, or references a legacy directory whose ownership lock cannot be acquired, this constructor throws after buildApp has already provisioned all configured Docker pools. Because buildApp never returns, startServer cannot access or run extras.shutdownHooks; it only releases the instance locks, leaving the newly created, restart-enabled containers running while startup fails. Initialize maintenance before provisioning pools or unwind the accumulated shutdown hooks on this failure path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed forward in #270: initialize and validate the maintenance journal before any configured backend provisions Docker containers. The real startup fixture supplied a malformed journal and valid Docker backend configuration, observed the intended journal refusal, and verified zero containers were created (same focused Beelink2 gate; typecheck passed).
Existing historical CLI Docker pools can retain a vulnerable runtime while their original bridge process is gone. Add bearer-protected adoption and individual slot drain/immutable-image roll to the existing bridge owner. Admission stops for that slot while its active lease finishes; other slots remain available. Adoption binds explicit container IDs and the original data-directory ownership lock, refusing listeners, execs or unexpected processes.
Replacement preserves existing credential volumes, binds, operator environment and resource settings, verifies the maintained Node pin and Docker preflight, and retains the stopped old container. Durable identity receipts allow interrupted transitions and owner restarts to reconcile safely. Extract provisioning from ContainerPool so the logic remains below1,000lines. No dependency or lockfile change.
Independent source review resolved per-start setup ordering, restart reconciliation and refused-adoption lock lifetime. Exact b54d9b8 passed the real Beelink2 Docker/authenticated HTTP journey and typecheck (gate beelink2-e0f9488daa3427f4d486b5316406cc62,189s): active exec survives drain; another slot serves; unexpected work blocks adoption without retaining an unused lock; volumes/environment/resources survive real replacements; the actual HTTP owner restarts and safely re-admits completed replacements. Fixture containers and volumes were removed through their test owner; no model calls. Earlier fail-closed Docker ps-format failures were retained and fixed by including Docker's required PID field and validating the parent/child process tree.
This source qualification does not claim the eight live pools have already changed image. Their normal source-only deploy and per-slot API rollout provide that production proof. Optional preflight caching is excluded from this proven head.
Run ID: run_4b1d54cfdf2c05412b39044bf98511e1