Conversation
There was a problem hiding this comment.
Pull request overview
Implements a deferred-apply mechanism for infra.database trusted_sources rules of type: app so first-deploy ordering (DB created before app) no longer fails; instead, firewall rules are finalized after all plan actions complete.
Changes:
- Add
ErrAppNotFoundsentinel and queue/flush logic inDatabaseDriverto defer unresolvable app-name trusted sources. - Add a second-pass in
DOProvider.Applyto callFlushDeferredUpdateson drivers with pending deferred updates. - Add TDD-style regression tests covering deferral behavior and provider-level flushing, plus a changelog entry.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/drivers/database.go | Adds ErrAppNotFound, defers app-name firewall rules, and implements deferred flush queueing/processing. |
| internal/provider.go | Adds deferred-updater interface and a second-pass flush in Apply. |
| internal/drivers/database_deferred_test.go | New tests covering create/update deferral and flush behavior (success + error cases). |
| internal/drivers/database_test.go | Updates existing test to reflect new deferral behavior instead of failing on missing app. |
| internal/provider_deferred_test.go | New end-to-end test ensuring Apply performs the deferred flush pass. |
| CHANGELOG.md | Documents the new deferred trusted_sources behavior and its failure semantics. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| if flushErr := du.FlushDeferredUpdates(ctx); flushErr != nil { | ||
| result.Errors = append(result.Errors, interfaces.ActionError{ | ||
| Resource: action.Resource.Name, |
Comment on lines
+598
to
+602
| ruleType := strFromConfig(m, "type", "") | ||
| ruleValue := strFromConfig(m, "value", "") | ||
| if ruleType == "" || ruleValue == "" || ruleType == "app" { | ||
| continue // skip app-type entries; they will be applied in the deferred pass | ||
| } |
Comment on lines
+630
to
+634
| ruleType := strFromConfig(m, "type", "") | ||
| ruleValue := strFromConfig(m, "value", "") | ||
| if ruleType == "" || ruleValue == "" || ruleType == "app" { | ||
| continue // skip app-type entries; they will be applied in the deferred pass | ||
| } |
Comment on lines
+678
to
+680
| d.pendingFirewallUpdates = nil // clear queue after flush | ||
| if len(errs) > 0 { | ||
| return fmt.Errorf("%s", strings.Join(errs, "; ")) |
Comment on lines
+320
to
+337
| // resources exist. Each driver type is flushed at most once. | ||
| seen := make(map[string]struct{}, len(plan.Actions)) | ||
| for _, action := range plan.Actions { | ||
| if _, done := seen[action.Resource.Type]; done { | ||
| continue | ||
| } | ||
| seen[action.Resource.Type] = struct{}{} | ||
| d, dErr := p.ResourceDriver(action.Resource.Type) | ||
| if dErr != nil { | ||
| continue | ||
| } | ||
| du, ok := d.(deferredUpdater) | ||
| if !ok || !du.HasDeferredUpdates() { | ||
| continue | ||
| } | ||
| if flushErr := du.FlushDeferredUpdates(ctx); flushErr != nil { | ||
| result.Errors = append(result.Errors, interfaces.ActionError{ | ||
| Resource: action.Resource.Name, |
…irst deploy When a trusted_sources entry of type=app references an app that does not yet exist (first-deploy ordering), DatabaseDriver.Create/Update now defers the full firewall update instead of failing. The resolvable rule subset is applied immediately; after all plan creates complete, DOProvider.Apply calls FlushDeferredUpdates so the full rule set (including app UUIDs) is applied in a second pass. API-level failures (rate-limit, transient, auth) are never silently deferred — only ErrAppNotFound (app absent from Apps.List) triggers the deferred path. Flush failures are fatal and surface in ApplyResult.Errors. Fixes GoCodeAlone/core-dump#154 (R4 first-deploy ordering finding). See docs/plans/2026-05-02-staging-deploy-blockers-design.md (Blocker 2). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
FlushDeferredUpdates previously cleared the entire pending queue after flush regardless of success. Combined with Diff not comparing trusted_sources, this left any failed entry permanently unretriable — retry Apply would produce no update action and the second-pass flush would never fire. Now only successfully-flushed entries are removed. Failed entries remain in pendingFirewallUpdates so a subsequent Apply automatically re-attempts them. Includes updated test (renamed _APIError_RetainsQueue) and CHANGELOG note. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
intel352
force-pushed
the
feat/db-trusted-sources-deferred-update
branch
from
e602031 to
706138a
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+341
to
+367
| // Second pass: flush deferred updates accumulated by drivers during the main | ||
| // action loop. These arise when a resource's config (e.g. DB trusted_sources | ||
| // with type=app) references another resource provisioned later in the same | ||
| // plan. By this point all plan creates have completed and the referenced | ||
| // resources exist. Each driver type is flushed at most once. | ||
| seen := make(map[string]struct{}, len(plan.Actions)) | ||
| for _, action := range plan.Actions { | ||
| if _, done := seen[action.Resource.Type]; done { | ||
| continue | ||
| } | ||
| seen[action.Resource.Type] = struct{}{} | ||
| d, dErr := p.ResourceDriver(action.Resource.Type) | ||
| if dErr != nil { | ||
| continue | ||
| } | ||
| du, ok := d.(deferredUpdater) | ||
| if !ok || !du.HasDeferredUpdates() { | ||
| continue | ||
| } | ||
| if flushErr := du.FlushDeferredUpdates(ctx); flushErr != nil { | ||
| result.Errors = append(result.Errors, interfaces.ActionError{ | ||
| Resource: action.Resource.Name, | ||
| Action: "deferred_update", | ||
| Error: flushErr.Error(), | ||
| }) | ||
| } | ||
| } |
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
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.
Summary
DatabaseDriver.Createand.Updatenow defer firewall updates when atrusted_sourcesentry oftype: appreferences an app that does not yet exist (first-deploy ordering), instead of failing with a resource-not-found error.DOProvider.Applyruns a second pass after the main action loop, callingFlushDeferredUpdateson any driver with pending updates — the full rule set (including resolved app UUIDs) is applied once apps are provisioned.ErrAppNotFoundsentinel distinguishes "app not yet created" (deferral-eligible) from API-level failures (rate-limit, transient, auth) which are never silently deferred. Flush failures are fatal and surface inApplyResult.Errors.Fixes GoCodeAlone/core-dump#154 (R4 first-deploy ordering finding).
See
docs/plans/2026-05-02-staging-deploy-blockers-design.md(Blocker 2).TDD Regression Invariant Proof
With fix reverted (Create propagates error on ErrAppNotFound without deferring):
With fix restored:
Test plan
TestDatabaseDriver_Create_AppNotYetExisting_DefersAndSucceeds— app absent → Create succeeds, no app rules, deferred queuedTestDatabaseDriver_Create_AppNotYetExisting_MixedRules— ip_addr+app, app absent → ip_addr rule in create request, app deferredTestDatabaseDriver_Create_AppAPIFailure_NotDeferred— API error → Create fails, NOT deferred, error is not ErrAppNotFoundTestDatabaseDriver_Update_AppNotYetExisting_DefersAndAppliesPartialRules— Update with unresolvable app → partial rules applied, deferred queuedTestDatabaseDriver_FlushDeferredUpdates_FullRulesApplied— after deferred create, flush calls UpdateFirewallRules with app UUIDTestDatabaseDriver_FlushDeferredUpdates_APIError_ReturnsError— flush propagates UpdateFirewallRules errorTestDatabaseDriver_HasDeferredUpdates_FalseWhenNone— fresh driver returns falseTestDatabaseDriver_FlushDeferredUpdates_NoopWhenEmpty— flush on empty driver returns nilTestDOProvider_Apply_FlushesDeferred_AfterAllCreates— end-to-end: provider Apply calls flush after creates, UpdateFirewallRules receives app UUIDGOWORK=off go test -count=1 ./internal/...)🤖 Generated with Claude Code