Repository navigation
INTR - 643 - Sidecar capacity and restoration - #227
Merged
Merged
Conversation
FarinM
changed the title
FarinM
force-pushed
the
intr-643-sidecar-capacity-and-restoration
branch
from
October 13, 2026 15:50
ad966ce to
c5159ee
Compare
alexanderguy
requested changes
Oct 14, 2026
alexanderguy
left a comment
alexanderguy
left a comment
Member
There was a problem hiding this comment.
This review comes from an agent that runs the pull-request-review skill. Separate adversarial agents tried to disprove each finding, and I removed the findings that did not hold.
The reconciler, the Hub handler and the sidecar durability work are careful, and the race tests are thorough. make all passes at c5159eea.
Three items need changes before merge:
- The
destroy()contract changes without anapiVersionchange. - Unconfirmed
destroy_failedrows hold an active slot, and nothing frees them. - Migration 0107 now has the same number as a migration on
main.
The other inline comments are smaller.
One commit-history item: d9348376 adds provider_hold_released, but no code uses that column until 07a4c334. Could the column move to that commit? Could the d9348376 message also say that destroy() must now return cleanup?
A stop or removal acknowledgement must survive a worker restart. Persist stopped and teardown state, sync removed directories, and keep unfinished cleanup represented until its durable removal succeeds. Normalize empty errors so failed deployments remain valid in durable records and stop reports.
Releasing a provider hold alone does not prove its deployment copy is gone. Persist removal proof separately so capacity can settle while provider cleanup remains unresolved. Once no copy remains, leave pending cancellation and dispatch settlement to lifecycle after accepted-history recovery. Provisioner destroy() results must now report whether removal is confirmed or still requires sidecar cleanup, so the provisioner API moves to version 2 and older provisioners are refused. Keep removal owned by the reconciler across reconnects, including replacement cleanup, and distinguish failures before sending from actual cleanup attempts. Wait for unavailable provisioners without counting an attempt. Advance the generation when binding replacement capacity so stale cleanup authority cannot displace it. Share an in-flight undeploy across reconnects for the same deployment generation.
Automatic cleanup must not retry forever or spend its attempt budget merely waiting for a sidecar. Keep provider-release proof across retries so an outage cannot consume the budget after its hold is gone. Bound failed attempts and preparation stalls while retaining unconfirmed capacity. Only disconnect timeouts resume automatically; exhausted attempts or an expired five-minute preparation grace require operator recovery.
Count durably kept copies separately from active deployments. Request retention in the background and release copies refused by the kept pool, preserving capacity until the worker confirms its decision. Keep initialized terminal copies, including cancelled and failed runs, on reconnect. Stop them and request retention while policy permits it. Reject startup when stored inventory exceeds either capacity limit instead of omitting copies from hello. Require current inventory for shared placement and probe adoption. Park inventory waits outside transactions and reconciliation slots. Confirm terminal controls only once the Hub's accepted history is final.
Let operators size active and retained deployment pools independently. Use each sidecar's advertised capacity for shared placement. Allow up to 2,048 records across the two pools.
Carry an explicit capacity refusal through the sidecar protocol and deployment services so it cannot become an uncertain initialization. Record the refused attempt under its durable marker even when the reconciliation lease is lost, without treating other failures as proof that nothing started.
Use the release retry budget and disconnect deadline for replacement cleanup, failing the live workflow when either bound is reached. Keep the existing disconnect deadline when release interrupts replacement, and let reconnect resume timed-out cleanup without reviving the failed workflow.
Count provisioning attempts when binding capacity and base backoff on that count, independently of cleanup attempts. Keep the count through interrupted ensures, Hub restarts, and workers that never initialize. Reset the count and backoff only when replacing an initialized worker. Stop a sequence after ten unsuccessful retryable attempts, recording its last failure before replacement cleanup. Continue cleanup under the same generation, proofs, attempt count, and disconnect deadline.
Give missing or mismatched provisioners five minutes to return before recording failure. Keep the grace period in memory so restarting the Hub starts a new window. Check provider availability only where capacity operations need it, so existing healthy deployments keep recovering and running without an installed provisioner.
FarinM
force-pushed
the
intr-643-sidecar-capacity-and-restoration
branch
from
October 14, 2026 13:41
c5159ee to
1e646f6
Compare
alexanderguy
requested changes
Oct 15, 2026
alexanderguy
left a comment
alexanderguy
left a comment
Member
There was a problem hiding this comment.
This is a follow-up review from an agent that runs the pull-request-review skill. A separate adversarial agent tried to disprove the new finding and reproduced it instead.
All seven earlier comments and the two commit-history items are resolved, so I resolved those threads. make all passes at 1e646f6d. The new recover route locks the row and checks status and generation again. It also retires the in-memory binding, and only owners hold recover by default.
One item needs a change before merge: two recover requests that arrive at the same time can give the second caller a 409, although the release happened. The inline comment has the details.
A destroy_failed allocation keeps its sidecar slot because the Hub cannot tell whether its copy is gone, and nothing could clear it. Once removal is confirmed outside the Hub, a principal holding the run's recover action can release it. The Hub takes that claim on the caller's word rather than verifying it.
FarinM
force-pushed
the
intr-643-sidecar-capacity-and-restoration
branch
from
October 15, 2026 21:05
1e646f6 to
e4eb8c8
Compare
alexanderguy
approved these changes
Oct 15, 2026
alexanderguy
left a comment
alexanderguy
left a comment
Member
There was a problem hiding this comment.
This is a follow-up review from an agent that runs the pull-request-review skill.
LGTM. The fix resolves the duplicate-recover race. The store now makes the decision while it holds the lock, so two recover requests at the same time both get 204. The new test fails against the old code and passes against the new code. make all passes at e4eb8c87.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Shared sidecars could keep old deployments after their provider hold was released, while copies kept for inspection consumed active capacity. Cleanup and replacement also need bounded failure paths without treating an unconfirmed copy as removed.
What this adds
Separate active and retained capacity, with provider-hold release recorded independently from confirmed deployment removal. Unconfirmed copies keep their reservation and a cleanup-only binding. Replacement gets a new generation so a delayed cleanup hello cannot displace the new deployment.
This silently changes the provisioner plugin API without bumping
apiVersion: 1. Successfuldestroy()results must now includecleanup: "confirmed"orcleanup: "required".confirmedguarantees the worker and its restorable deployment state are gone and cannot return;requiredleaves removal to the sidecar and must keep that cleanup path available. Existing version-1 plugins still pass registration, but returning only{ kind: "destroyed" }now fails validation and can exhaust cleanup retries. Provisioners need updating alongside the Hub.The sidecar durably records stop, retention and teardown decisions before acknowledging them. Interrupted removal stays represented across restart, and failed cleanup keeps its capacity reserved until removal or retention is confirmed. Once no deployment remains, pending cancellation and dispatch settlement stay with lifecycle after accepted-history recovery.
SIDECAR_MAX_ACTIVE_DEPLOYMENTSandSIDECAR_MAX_RETAINED_DEPLOYMENTSconfigure the two pools, defaulting to 128 and 256, with a combined ceiling of 2,048. Placement uses the sidecar's advertised limits and current inventory. Retention refusal starts release cleanup.Release and replacement cleanup stop after ten failed attempts by default, or after the cleanup disconnect deadline. Disconnected waits do not spend attempts. Provisioning also has a ten-attempt budget, including parked replacements that never initialize. Missing provisioners and cleanup preparation stalls have separate five-minute grace periods per Hub process. A cleanup disconnect timeout can resume on reconnect without reviving the workflow; other terminal cleanup failures require operator recovery.
Migration
0107adds the two cleanup proof columns. The Hub/sidecar protocol also adds retention controls and structured deployment errors.Validated with focused sidecar, reconnect, lifecycle and allocation tests, including real database tests, plus scoped type, lint and API-doc checks. The full build/test pipeline was not rerun for this revision.