docs: partial recovery — add guide for host failure recovery and orphaned instance cleanup #261
+516
−2
Conversation
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
…aned instance cleanup
📝 WalkthroughWalkthroughAdds a comprehensive partial-recovery guide, a CleanupInstance activity for removing orphaned DB instances during host-removal Delete flows, docker-level node-unavailability detection ( Changes
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@docs/disaster-recovery/partial-recovery.md`:
- Around line 179-183: Replace the placeholder comments with concrete psql
commands that show how to insert test data on the primary and verify it on the
replica; for example, add a psql INSERT on n1 targeting the example database and
test_table (e.g., INSERT INTO test_table (id, value) VALUES (...)) and a psql
SELECT on n2 to confirm the replicated row (e.g., SELECT * FROM test_table WHERE
value = 'recovery_test'); ensure you include host/port/user flags (-h, -p, -U)
and use the same database name (example) and table name (test_table) as shown in
the suggested improvement so readers can run the steps end-to-end.
- Around line 413-416: Replace the placeholder comments "# Insert on n1" and "#
Verify on n3" with concrete psql commands: on the primary node run a psql
command to INSERT a test row into the example database (e.g., use psql -h host-1
-p 5432 -U admin -d example -c "INSERT INTO test_table (id, value) VALUES
(...);"), and on the restored node run a psql SELECT to verify the row exists
(e.g., psql -h host-3 -p 5432 -U admin -d example -c "SELECT * FROM test_table
WHERE value = '...';"); ensure the SQL matches the test_table schema and uses a
unique value (e.g., 'full_recovery_test') so the verification step is
unambiguous.
In `@server/internal/orchestrator/swarm/postgres_service.go`:
- Around line 171-185: The timeout branch currently falls through and lets
ServiceRemove proceed even though scale-down may not have completed; update the
isTimeoutError(err) case in the same function (the scale-down/ServiceRemove
sequence) to return a descriptive error instead of continuing, e.g., return an
error indicating the scale-down timed out (wrap the original err), so callers
must handle the timeout (until the TODO RemovingHosts/context distinction is
implemented); reference isTimeoutError, the scale-down logic, and the subsequent
ServiceRemove call when making the change.
🧹 Nitpick comments (1)
server/internal/workflows/activities/cleanup_instance.go (1)
27-31:MaxAttempts: 1means no retries on transient failures.With
MaxAttempts: 1, any transient failure (network blip, etcd leader election) will fail the cleanup permanently. Consider allowing a small number of retries with backoff for resilience.Optional: Allow limited retries
options := workflow.ActivityOptions{ Queue: utils.HostQueue(a.Config.HostID), RetryOptions: workflow.RetryOptions{ - MaxAttempts: 1, + MaxAttempts: 3, }, }
Comment on lines
171
to
185
| switch { | ||
| case errors.Is(err, docker.ErrNotFound): | ||
| // Service is already deleted. | ||
| return nil | ||
| } else if err != nil { | ||
| case isTimeoutError(err): | ||
| // NOTE: This assumes timeout always indicates an unreachable host. | ||
| // This is currently intended for remove-host --force operations. | ||
| // If timeout occurs during normal operations (e.g., temporary network | ||
| // issues), this could incorrectly proceed with removal. | ||
| // | ||
| // TODO: Add RemovingHosts to resource.Context to safely distinguish | ||
| // between force-removal scenarios and transient network issues. | ||
| case err != nil: | ||
| return fmt.Errorf("failed to scale down postgres service before removal: %w", err) | ||
| } |
There was a problem hiding this comment.
Timeout case falls through to ServiceRemove without guaranteed scale-down completion.
When a timeout occurs, the code proceeds to ServiceRemove without confirmation that containers have stopped. The comment on lines 161-164 explicitly states the scale-down is performed "to guarantee that containers have stopped before this function returns." The timeout case bypasses this guarantee.
While the TODO acknowledges this is intended for --force operations, without the proposed RemovingHosts context field, there's no way to distinguish force-removal from transient network issues. This could lead to:
- Attempting
ServiceRemovewhile containers are still running - Resource cleanup errors downstream
Consider returning an error for timeout cases until the RemovingHosts context is implemented, or explicitly document that callers must handle potential race conditions.
Alternative: Return error until context distinction is available
case isTimeoutError(err):
- // NOTE: This assumes timeout always indicates an unreachable host.
- // This is currently intended for remove-host --force operations.
- // If timeout occurs during normal operations (e.g., temporary network
- // issues), this could incorrectly proceed with removal.
- //
- // TODO: Add RemovingHosts to resource.Context to safely distinguish
- // between force-removal scenarios and transient network issues.
+ // TODO: Once RemovingHosts is added to resource.Context, allow
+ // proceeding for force-removal scenarios only.
+ return fmt.Errorf("timeout scaling down postgres service; host may be unreachable: %w", err)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| switch { | |
| case errors.Is(err, docker.ErrNotFound): | |
| // Service is already deleted. | |
| return nil | |
| } else if err != nil { | |
| case isTimeoutError(err): | |
| // NOTE: This assumes timeout always indicates an unreachable host. | |
| // This is currently intended for remove-host --force operations. | |
| // If timeout occurs during normal operations (e.g., temporary network | |
| // issues), this could incorrectly proceed with removal. | |
| // | |
| // TODO: Add RemovingHosts to resource.Context to safely distinguish | |
| // between force-removal scenarios and transient network issues. | |
| case err != nil: | |
| return fmt.Errorf("failed to scale down postgres service before removal: %w", err) | |
| } | |
| switch { | |
| case errors.Is(err, docker.ErrNotFound): | |
| // Service is already deleted. | |
| return nil | |
| case isTimeoutError(err): | |
| // TODO: Once RemovingHosts is added to resource.Context, allow | |
| // proceeding for force-removal scenarios only. | |
| return fmt.Errorf("timeout scaling down postgres service; host may be unreachable: %w", err) | |
| case err != nil: | |
| return fmt.Errorf("failed to scale down postgres service before removal: %w", err) | |
| } |
🤖 Prompt for AI Agents
In `@server/internal/orchestrator/swarm/postgres_service.go` around lines 171 -
185, The timeout branch currently falls through and lets ServiceRemove proceed
even though scale-down may not have completed; update the isTimeoutError(err)
case in the same function (the scale-down/ServiceRemove sequence) to return a
descriptive error instead of continuing, e.g., return an error indicating the
scale-down timed out (wrap the original err), so callers must handle the timeout
(until the TODO RemovingHosts/context distinction is implemented); reference
isTimeoutError, the scale-down logic, and the subsequent ServiceRemove call when
making the change.
jason-lynch
reviewed
Feb 3, 2026
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@docs/disaster-recovery/partial-recovery.md`:
- Around line 271-273: Fix the typo and endpoint mismatches: remove the extra
slash in the curl invocation used to set JOIN_TOKEN so it calls
"/v1/cluster/join-token" (the line containing JOIN_TOKEN="$(curl
http://host-1:3000//v1/cluster/join-token)") and update the summary table
entries so they exactly match the routes used earlier—use
"/v1/cluster/join-token" for fetching the join token and "/v1/cluster/join" for
joining the cluster (also correct the same occurrences noted around lines
388-390).
In `@server/internal/docker/docker.go`:
- Around line 424-449: The checkNodesUnavailable function currently treats only
NodeStateDown and NodeStateUnknown as unavailable; update the availability check
in Docker.checkNodesUnavailable to also treat swarm.NodeStateDisconnected as
unavailable (i.e., include node.Status.State == swarm.NodeStateDisconnected in
the conditional alongside NodeStateDown and NodeStateUnknown) and update the
function comment to reflect "(status 'down', 'unknown' or 'disconnected')";
ensure you reference the NodeList call and the loop over nodes in
checkNodesUnavailable when making the change.
🧹 Nitpick comments (1)
docs/disaster-recovery/partial-recovery.md (1)
131-136: Add language identifiers to output blocks (markdownlint MD040).The example output fences are missing a language. Adding
text(orconsole) will satisfy MD040.Example tweak
-``` +```text ID HOSTNAME STATUS AVAILABILITY MANAGER STATUS 4aoqjp3q8jcny4kec5nadcn6x * lima-host-1 Ready Active Leader 959g9937i62judknmr40kcw9r lima-host-2 Ready Active Reachable l0l51d890edg3f0ccd0xppw06 lima-host-3 Down Active Unreachable-
+text
docker swarm join --token SWMTKN-1-xxx...xxx 10.0.0.1:2377Also applies to: 222-224
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@server/internal/docker/docker.go`:
- Around line 424-432: The current loop swallows errors from
checkNodesUnavailable by continuing, which hides Docker API failures; change the
behavior in the block that calls d.checkNodesUnavailable(ctx, stuckNodeIDs) to
return the error (or wrap it with context) instead of continue so upstream sees
the real failure; update the call site that currently checks unavailable, err :=
d.checkNodesUnavailable(...) to handle err by returning fmt.Errorf("checking
node availability: %w", err) (or similar) and keep the existing
ErrNodeUnavailable return path when unavailable is true.
🧹 Nitpick comments (1)
docs/disaster-recovery/partial-recovery.md (1)
131-136: Add language identifiers to fenced output blocks (MD040).
Usetext(or similar) to satisfy linting.📝 Example fix
-``` +```text ID HOSTNAME STATUS AVAILABILITY MANAGER STATUS 4aoqjp3q8jcny4kec5nadcn6x * lima-host-1 Ready Active Leader 959g9937i62judknmr40kcw9r lima-host-2 Ready Active Reachable l0l51d890edg3f0ccd0xppw06 lima-host-3 Down Active UnreachableAlso applies to: 222-224
Comment on lines
+424
to
+432
| // If we have tasks that should be stopping, check if their nodes are unavailable | ||
| if len(stuckNodeIDs) > 0 { | ||
| unavailable, err := d.checkNodesUnavailable(ctx, stuckNodeIDs) | ||
| if err != nil { | ||
| continue | ||
| } | ||
| if unavailable { | ||
| return fmt.Errorf("%w: tasks cannot stop because their nodes are down", ErrNodeUnavailable) | ||
| } |
There was a problem hiding this comment.
Don’t swallow node-list errors.
Continuing on checkNodesUnavailable errors hides persistent Docker API failures and turns them into a timeout instead of a root-cause error.
🔧 Suggested fix
- if err != nil {
- continue
- }
+ if err != nil {
+ return fmt.Errorf("failed to check node availability: %w", err)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // If we have tasks that should be stopping, check if their nodes are unavailable | |
| if len(stuckNodeIDs) > 0 { | |
| unavailable, err := d.checkNodesUnavailable(ctx, stuckNodeIDs) | |
| if err != nil { | |
| continue | |
| } | |
| if unavailable { | |
| return fmt.Errorf("%w: tasks cannot stop because their nodes are down", ErrNodeUnavailable) | |
| } | |
| // If we have tasks that should be stopping, check if their nodes are unavailable | |
| if len(stuckNodeIDs) > 0 { | |
| unavailable, err := d.checkNodesUnavailable(ctx, stuckNodeIDs) | |
| if err != nil { | |
| return fmt.Errorf("failed to check node availability: %w", err) | |
| } | |
| if unavailable { | |
| return fmt.Errorf("%w: tasks cannot stop because their nodes are down", ErrNodeUnavailable) | |
| } |
🤖 Prompt for AI Agents
In `@server/internal/docker/docker.go` around lines 424 - 432, The current loop
swallows errors from checkNodesUnavailable by continuing, which hides Docker API
failures; change the behavior in the block that calls
d.checkNodesUnavailable(ctx, stuckNodeIDs) to return the error (or wrap it with
context) instead of continue so upstream sees the real failure; update the call
site that currently checks unavailable, err := d.checkNodesUnavailable(...) to
handle err by returning fmt.Errorf("checking node availability: %w", err) (or
similar) and keep the existing ErrNodeUnavailable return path when unavailable
is true.
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
This PR adds documentation for partial failure recovery (quorum intact) and improves node unavailability detection during force host removal operations.
Changes
disaster recovery guide for partial failure scenarios(docs/disaster-recovery/partial-recovery.md)ErrNodeUnavailablesentinel error for precise node unavailability detectionWaitForServiceto detect stuck tasks due to unavailable nodes instead of relying on generic timeoutspostgres_service.goto use the new sentinel error during service deletionCleanupInstanceactivity to clean up orphaned instance records when force-removing hostsTesting
host-3and Quorum state Intact (2 healthy hosts remained)Phase 1: Failed Host Removal (Verified)
✅ Step 1.1: Force Remove Host from Control Plane
Executed:
curl -X DELETE http://192.168.105.3:3000/v1/hosts/host-3?force=trueObserved:
Verified via:
Matches documented behavior: forced host removal + automatic database cleanup
✅ Step 1.3: Docker Swarm Cleanup
On a healthy manager (host-1):
Observed:
Confirms documented Swarm cleanup procedure
Phase 2: Reduced-Capacity Operation (Verified)
✅ Step 2.1: Host Status Verification
curl http://192.168.105.3:3000/v1/hostsObserved:
✅ Step 2.2: Database Health Verification
curl http://192.168.105.3:3000/v1/databases/storefrontObserved:
Database state:
availableInstances:
n1 on host-1
n2 on host-2
No references to host-3
✅ Step 2.3: Data Replication Verification
Executed:
Observed:
Confirms cluster remained fully operational with reduced capacity
Phase 3: Restored Host Preparation (Verified)
✅ Step 3.1: Clean Old Control Plane Data
On restored host-3:
Observed:
✅ Step 3.2: Rejoin Docker Swarm (Manager)
From host-1:
docker swarm join-token managerOn host-3:
docker swarm join --token <TOKEN> 192.168.104.1:2377Observed:
✅ Step 3.4: Redeploy Control Plane Stack
docker stack deploy -c /tmp/stack.yaml control-planeObserved:
Verified via:
Phase 4: Control Plane Cluster Rejoin (Verified)
✅ Step 4.1: Generate Join Token
curl http://192.168.105.3:3000/v1/cluster/join-tokenResponse included:
tokenserver_url✅ Step 4.2: Join Cluster from Restored Host
Observed:
✅ Step 4.3: Host Verification
curl http://192.168.105.3:3000/v1/hostsObserved:
host-3presenthealthyPhase 5: Database Capacity Restoration (Verified)
✅ Step 5.1: Add Database Node Back
Observed:
✅ Step 5.3: Final Replication Verification
Executed:
n3n2Observed:
Verification Summary:
Checklist
PLAT-313