Sign up
View stargazersSign up

Retry the leaving Server's observe so its warm Moves are released#2019

merged9
main
release-lanes/2-removal-observe
opened28d agobyNNick
6 files changed
View on GitHubGitHub import enabledPR last imported 20d ago

Why

ployz server rm reads the leaving Server once, then hands each warm lane bound for it to a release. That read swallowed its failures.

  • A Cluster frame that timed out made the Server's name null.
  • A copy read that failed became "writes no Volumes".

Inngest memoizes a step's result, so release-volume-lanes then saw no name and no writes. It left a warm Move bound for the gone Server active, with its busy sign, until that Move's own run failed.

What changed

  • observe-leaving-server fails, so Inngest retries it, in three cases. The Cluster frame times out. Cloud holds no pairing for the Cluster. The leaving Server reads up but its copies do not answer.
  • On the step's last try, attempt 5 of retries: 5, an up Server whose copies still do not answer is taken as writing nothing, and the worker logs a warning naming it. A Server that reads suspect, down or unknown counts as gone at once.
  • The step reads one frame itself, with RUNTIME_FRAME_TIMEOUT_MS, the wait the removal step already uses.
  • A lane bound to the leaving Server by id is released even when the Server has no name.
  • observeOrganization fails with ClusterNotPaired. observeOrganizationCopies still returns [] there, so its callers see no change.
  • createProcessMachineRemove takes the house EffectRunner and the copy observer as defaulted parameters, the way server-upgrade.inngest.ts does, so a test can run the real step at a chosen attempt.

Scope

Item 3 of the release-lane follow-ups. Deploy and restore lanes are left as they are. A Deploy never waits on a Move. A restore is past its point of no return, and server rm's Data Loss confirmation gates it.

Tradeoffs

The step fails open on its last try. Picture a Server that stays up while its pool errors on every read, such as a destroyed or foreign pool. Failing closed would block its removal until the user stopped it, and a stopped Server cannot be reset. Such a pool holds no writer. Core's confirmed Data Loss decides what happens to the data. writes only decides whether a warm lane ends now or when its own run fails.

The last try counts the step's attempts. If attempts 0 to 4 failed on frame timeouts, one unanswered read on attempt 5 is accepted. Main accepted every unanswered read.

Blast Radius

ployz server rm waits through Inngest's retries while the Cluster cannot be read, where before it went ahead on a stale read. Nothing else that reads copies changes.

Verification

Proven on Boat with two Machines, with a local script over core/scripts/test-verify-volume-move.py. A verify fault delays SetVolumeCopy on machine-2, which holds a warm Move of data onto it. With the Inngest worker stopped, the script requests ployz server rm machine-2 and stops ployz on both Machines. Then it starts the worker, so observe-leaving-server runs while the Cluster cannot answer. After about 60 seconds it starts ployz again and reads the rows.

Main at 0b6616e0b. observe-leaving-server completed on attempt 0 while the Cluster was down. The removal attempt succeeded with others_remain. No release row was written. The warm Move stayed running after the removal ended, and failed about three minutes later with set_volume_copy on machine-2: not_found: No Server named "3e6dd818…".

This branch. observe-leaving-server failed on attempt 0 while the Cluster was down and completed on attempt 1 once it was back. The removal attempt succeeded with others_remain. The warm Move ended cancelled with "volume release took this run over" at the second the removal finished. A warm release row with no user was written then and was done 7 seconds later.

On three Machines, through the whole CLI. A later Boat run on the stack's top before its rebase onto main held a warm Move of data onto machine-3 and ran ployz server rm machine-3. As the removal started, ployz was stopped on machine-1, the only other Server, for about 70 seconds. The CLI printed Removed Server machine-3. once machine-1 was back. The warm Move ended cancelled with "volume release took this run over" as the removal finished, and db kept running on machine-1 with its three rows. In that run the retry Inngest showed was the remove step's own: with machine-1 down, Cloud reached machine-3 as its entry and refused to remove it through itself. So it confirms the end state, and the two-Machine run above is the evidence for the observe retry.

Every run ended with verify-cluster down exit 0.

Checks

11 successful · 7 skipped
All checks passed
1d5f6b3
Cchatgpt-codex-connectorBot28d ago
Synced from GitHub

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

ReviewStatusCommitReview trigger
📝 Code Review✅ Completed 2026-10-14T20:48:06.908564Z1d5f6b3New commits
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

CcoderabbitaiBot28d ago
Synced from GitHub

Review in Change Stack →

Important

Review skipped

Review was skipped as selected files did not have any reviewable changes.

⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: e48d0fde-c374-4a42-a38e-bdb96a31df1e
📥 Commits

Reviewing files that changed from the base of the PR and between c5beb85000e2d54d150e2224fa73f35f3cb210d8 and 1d5f6b3e42faa2961fad383c27c9ad2b5336fd28.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: f82b82f9-ab1d-45f4-accf-8d5e929d51b3
📥 Commits

Reviewing files that changed from the base of the PR and between 35cc2481e392086447471e6c962c05f447df35f0 and c5beb85000e2d54d150e2224fa73f35f3cb210d8.

📒 Files selected for processing: 6
  • dashboard/src/modules/machines/machine-removal.inngest.test.ts
  • dashboard/src/modules/machines/machine-removal.inngest.ts
  • dashboard/src/modules/machines/machine-removal.server.test.ts
  • dashboard/src/modules/machines/machine-removal.server.ts
  • dashboard/src/modules/volume-run/volume-run.postgres.test.ts
  • dashboard/src/modules/volume-run/volume-run.server.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 2 remain after this review.

📝 Summary

Summary by CodeRabbit

  • Bug Fixes
    • Machine removal now retries when a departing server appears online but does not respond. After the final attempt, removal proceeds with a warning.
    • Warm volume moves targeting a departing server are now released even when its name is unavailable.

Machine removal now observes the leaving Server’s runtime frame and volume copies. If the Server is up but unanswered, the workflow retries before its final attempt and proceeds on that final attempt. Volume-run observation and destination matching also handle unpaired organizations and unnamed machines.

Changes: Machine removal observation and retries

Layer / File(s)Summary
Organization observation and destination matching
dashboard/src/modules/volume-run/volume-run.server.ts, dashboard/src/modules/volume-run/volume-run.postgres.test.ts		Organization observation fails with ClusterNotPaired when connections are not ready. Copies-only observation returns an empty list for that error. Destination matching checks the machine ID when its name is unavailable.
Leaving-Server observation
dashboard/src/modules/machines/machine-removal.server.ts, dashboard/src/modules/machines/machine-removal.server.test.ts		The activity reads the runtime frame and observes organization copies. It returns LeavingServerUnanswered for an up Server with unanswered copies before the final try; on the final try it logs a warning and proceeds.
Retry workflow and injected dependencies
dashboard/src/modules/machines/machine-removal.inngest.ts, dashboard/src/modules/machines/machine-removal.inngest.test.ts		The workflow uses MACHINE_REMOVE_RETRIES for its retry limit and passes the attempt to leaving-Server observation. Effect execution and observation functions are injectable. Tests cover attempts before and at the retry limit.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix

Sequence Diagram(s)

mermaid
1sequenceDiagram
2 participant InngestWorkflow
3 participant executeProcessMachineRemove
4 participant observeLeavingServerActivity
5 participant OrganizationRuntime
6 participant observeOrganization
7 InngestWorkflow->>executeProcessMachineRemove: Pass current attempt
8 executeProcessMachineRemove->>observeLeavingServerActivity: Pass machine and lastTry
9 observeLeavingServerActivity->>OrganizationRuntime: Open runtime and read first frame
10 OrganizationRuntime-->>observeLeavingServerActivity: Return session and frame
11 observeLeavingServerActivity->>observeOrganization: Observe organization copies
12 observeOrganization-->>observeLeavingServerActivity: Return copy observations
13 observeLeavingServerActivity-->>executeProcessMachineRemove: Fail before final try or return observation
Merge Risk: ⚪ Minimal · up to c5beb

No actionable merge-blocking risk was identified; the change is ready to merge after normal checks.

Security architecture risk: 🔵 Low · up to c5beb

Retries and ID-based matching improve cleanup without widening organization access. The final retry still proceeds when copy reads remain unavailable, so recovery guarantees in that case are not fully established.

Retained concerns No architecture-level concerns identified.

Security review details

Security Blast Radius

  • observed — Lane selection spans active warm lanes in the attempt's organization, but takeover requires a matching departing destination or a reported writer volume. Recorded machine IDs take precedence over names, and a missing name does not become a wildcard. The new behavior reaches previously skipped ID-bound lanes without removing the organization filter.

Trust Boundaries and Controls

  • observed — The inspected dashboard initiation path requires operate access to the organization and validates the removal plan. The worker event supplies an attempt ID; organization, machine, and confirmation data come from the durable attempt rather than new caller-controlled observer or runner parameters.

Resilience and Maintainability Implications

  • inferred — If final observation reports no writers, a warm lane associated only with a departing source can remain active until its own workflow ends. Finished-run and stale-run cleanup can then close it and request release, but the stale sweep leaves workflows reported running alone. This residual recovery dependency predates the PR's retry improvement and is not established as a new security finding.
  • observed — The SDK boundary documents unanswered Servers as unknown rather than empty. Its inspected wrapper delegates observation without establishing that an up, unanswered Server cannot still be a writer. The removal activity's stronger no-writer assumption therefore remains unverified.

Hardening Proposals

  • proposed — Establish an explicit contract for final-attempt unanswered observation. If absence of a writer cannot be guaranteed, preserve that uncertainty and provide bounded reconciliation for omitted source lanes rather than relying solely on their workflows eventually ending.
Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check nameStatusExplanationResolution
Docstring CoverageWarningDocstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 6 files.Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check nameStatusExplanation
Linked Issues checkPassedCheck skipped because no linked issues were found for this pull request.
Out of Scope Changes checkPassedCheck skipped because no linked issues were found for this pull request.
Title checkPassedThe title clearly summarizes the primary change: retrying observation of the leaving Server so warm Moves can be released.
Description checkPassedThe description directly explains the failure mode, retry behavior, last-attempt handling, ID-based lane release, scope, and verification results.
✨ Finishing Touches: 💡 2
📝 Generate docstrings: 💡
  • Commit to this branch
  • Create a new PR
🛠️ Fix failing CI checks: 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

Added 9 commits

  • Test that observing a leaving Server fails when the Cluster or its copies cannot be read0dc88b1
  • Retry observing a leaving Server until the Cluster and its copies answer117595b
  • Test that observing a leaving Server retries while it reads up but unanswered, and that a lane bound by id is released169bed7
  • Route process-machine-remove's failure handler through its runner, and keep the last-try check local1d5f6b3
  • Say the unanswered fallback runs on the step's last try, not after every retry5021152
  • Retry observing a leaving Server that reads up but does not answer, and release a lane bound to it by id83a84af
  • Test that observing a leaving Server accepts it unanswered on the step's last trya3fad73
  • Test that process-machine-remove takes an unanswered leaving Server as writing nothing only on its last trya7028e8
  • Take a leaving Server that reads up but never answers as writing nothing on the step's last trye83876e