Synced from GitHub
🚅 Environment ployz-pr-2018 in Ployz Dashboard has no services deployed.
Name only a copy that holds the data when a removed Server ends a Move#2018
Why
When the Server a Move committed to is removed, its run fails and the message names a copy to restore from. Two places wrote that message, and each picked the copy its own way.
finishReleasenamed the first member holding any snapshot. That could be the writer, which is not a copy to restore from.endLanesTonamed a copy from aheldmap that machine-removal built from the copy list. The copy list carries no snapshots, so the map could name a slot that held none.
A restore names its source as the only copy to keep, so the wrong name can send the user to throw away the newest data.
What changed
restoreSource(members)inplan.tsnames the stopped copy with the newest snapshot. It names none while a member has not answered, because that Server may hold newer data. It names none when two different snapshots tie on the newest second, because core orders snapshots by txg and Cloud sees only seconds.writerRemovedMessagegivesfinishReleaseandendLanesToone wording.endLanesToobserves each lane's Volume itself. When that observe fails, the lane still ends and names no copy. The message only advises, and a lane left active would block the restore it names.heldOffand theheldmap leave machine-removal.
Scope
Items 1 and 2 of the release-lane follow-ups from the Volume busy stack. The behaviour of a removal, a release and a restore is unchanged. Only the failure message and the copy it names change.
Tradeoffs
A Cluster with one unanswered Server now gets "No other server's copy is known to hold its newest data." even when an answered copy exists. Naming that copy could send the user to restore older data and remove a newer one on the Server that did not answer.
Blast Radius
The failure message of a committed run whose new writer's Server was removed, reached from ployz volume release and from ployz server rm. No row, lane or Machine state changes.
Verification
Proven on Boat with three Machines running the stack's top before its rebase onto main, which includes this branch. A Postgres service db used Volume data with one row in a table. db ran on machine-2, machine-1 held nothing, and machine-3 was empty.
Removing a silent new Server mid-finish. A verify fault held a Move of data onto machine-3 at its promotion. Once it committed, machine-3 was powered off. The Move failed and its release waited for machine-3. ployz server rm machine-3 refused, since a silent Server can't be reset, so machine-3 was forgotten with ployz server forget machine-3 --confirm dead and a Cloud approval. Fourteen seconds later the release failed with:
| 1 | data's writer was on machine-3 (removed). Restore from machine-2's copy: ployz volume restore data --from machine-2 |
machine-2 is the right copy. It is the old Server, its db container was stopped, and it held the data. The message names neither the removed Server nor machine-1, which held no copy.
Nothing stays owed to it. A release then ended done without waiting. A Move onto machine-1 started from machine-2's stopped copy and ended done, and db read its row there. A deploy after it ran db on machine-1 and didn't try machine-3.
The printed restore didn't run. The Move to machine-1 came first, and once machine-1 was the writer the restore was refused because another copy existed. That refusal is #2012, which main has.
Removing an answering new Server mid-finish. A separate run had db on machine-1 with three rows. A verify fault held a Move of data onto machine-2 at its promotion. Once it committed, ployz server rm machine-2 removed machine-2 with a Cloud approval, and the Move failed with:
| 1 | data's writer was on machine-2 (removed). Restore from machine-1's copy: ployz volume restore data --from machine-1 |
machine-1 is the right copy. It was the old writer and held all three rows. A Move onto machine-3 was accepted at once and ended done with all three rows, and a deploy then ran db on machine-3. The printed restore was refused after that Move for the same #2012 reason.
Every run ended with verify-cluster down exit 0.
Checks
1 pending · 10 successful · 7 skipped
In progress1
- In progress
Synced from GitHub
Codex Review Summary
This comment shows the latest Codex review activity on this pull request.
| Review | Status | Commit | Review trigger |
|---|---|---|---|
| 📝 Code Review | ✅ Completed 2026-10-14T20:37:21.229904Z | 35cc248 | New 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.
Synced from GitHub
📝 Summary
Summary by CodeRabbit
- Bug Fixes
- When a volume’s writer server is removed, the recovery message now identifies a restore source only when the remaining copies provide a clear, reliable choice.
- If no safe restore source can be determined—including when server information is unavailable—the message no longer names a potentially incorrect copy.
- Restore guidance now reflects whether another server is known to hold the newest data.
The removal flow no longer passes held-copy maps. When a committed Move target is removed, the volume-run path observes remaining members and uses snapshot data to identify and report a restore source.
Changes: Committed Move target removal
Layer / File(s) Summary Restore-source selection and message construction
dashboard/src/modules/volume-run/plan.ts,dashboard/src/modules/volume-run/plan.test.tsrestoreSourceselects a holder only when all members answered and the newest eligible snapshots agree.writerRemovedMessageformats the removed-writer message with or without a restore command. Tests cover selection, missing observations, and ties.Remove held-copy handoff
dashboard/src/modules/machines/machine-removal.inngest.ts,dashboard/src/modules/machines/machine-removal.server.ts,dashboard/src/modules/machines/machine-removal.server.test.ts,dashboard/src/modules/volume-run/volume-run.server.tsMachine removal and releaseLanesOffno longer pass held-copy maps. The removedheldOfftests are no longer present.Observe remaining copies and report a source
dashboard/src/modules/volume-run/volume-run.server.ts,dashboard/src/modules/volume-run/volume-run.inngest.ts,dashboard/src/modules/volume-run/volume-run.postgres.test.tsCommitted-lane removal observes remaining Volume members and uses the planning helpers to build the failure message. If observation fails or no source can be selected, the message does not name a restore source. Tests cover the remaining-copy cases. Estimated code review effort: 3 (Moderate) | ~20 minutes
Change: Bug fix
Sequence Diagram(s)
mermaid1 sequenceDiagram 2 participant releaseLanesOff 3 participant endLanesTo 4 participant VolumeMembers 5 participant restoreSource 6 participant writerRemovedMessage 7 releaseLanesOff->>endLanesTo: Process lanes targeting the removed machine 8 endLanesTo->>VolumeMembers: Observe members excluding the removed machine 9 VolumeMembers-->>endLanesTo: Return member observations 10 endLanesTo->>restoreSource: Select a restore source 11 restoreSource-->>endLanesTo: Return holder or undefined 12 endLanesTo->>writerRemovedMessage: Build the writer-removal message - Bug Fixes
Pre-merge checks: Passed 4 · Failed 1
Failed Failed checks: 1 warning
Check name Status Explanation Resolution Docstring Coverage Warning Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 7 files. Write docstrings for the functions missing them to satisfy the coverage threshold. Passed Passed checks: 4 passed
Check name Status Explanation Title check Passed The title clearly summarizes the primary change: naming only a copy known to hold the data after a removed Server ends a Move. Description check Passed The description directly explains the problem, implementation, scope, tradeoffs, and verification for the changeset. Linked Issues check Passed Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
- Fix all pre-merge checks with AI
✨ Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
- 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 5 commits
- Test that a removed Server's committed lane names only a stopped copy with a snapshotda89d0f
- Name no restore source while a Server is unanswered or two newest snapshots tie07a3086
- Narrow a restore source's members to answered ones instead of rechecking each35cc248
- Test that a restore source names no copy while a Server is unanswered or two newest snapshots tie72e4545
- Name only a stopped copy with a snapshot when a removed Server ends a committed laned171524
Synced from GitHub
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0ee86d5ad9
ℹ️ 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".
| … | … | @@ -590,11 +590,13 @@ const endLanesTo = Effect.fn("VolumeRun.endLanesTo")(function* ( | |
| 597 | - | : `${lost}. Restore from ${holder}'s copy: ployz volume restore ${lane.volumeName} --from ${holder}`; | |
| 593 | + | // The message only advises; a lane left active would block the restore it names. | |
| 594 | + | const holder = yield* observeVolumeMembers(organizationId, lane.dockerVolume).pipe( | |
| 595 | + | Effect.map((members) => restoreSource(members.filter((member) => member.machine.id !== machine.id))), | |
P2 Badge Bound the per-volume observations during removal
When the removed Server is the committed destination for N volumes and any remaining managed Server is unreachable, this observation runs serially inside the lane loop, while each call can wait up to OBSERVE_TIMEOUT (10 seconds). The release-volume-lanes step therefore takes roughly N×10 seconds and can time out or exhaust retries for a large deployment after the Server has already been removed; observe the affected volumes concurrently with a bounded concurrency or apply one overall deadline.
Useful? React with 👍 / 👎.
| … | … | @@ -234,6 +234,23 @@ function planRestore(name: string, { from, machine }: VolumeRunArgs["restore"], | |
| 236 | 236 | ||
| 237 | + | export function restoreSource(members: readonly Member[]): MachineName | undefined { | |
| 238 | + | const answered = members.filter((member): member is AnsweredMember => member.answered); | |
| 239 | + | if (answered.length < members.length) return undefined; | |
P1 Badge Document when no restore source is named
When another Server is unanswered or two different snapshots tie for the newest second, this new branch intentionally names no source even though copies may remain, but docs/user/services/move-a-volume.md:192-196 still promises that the error names the copy left and shows only the restore-command outcome. Update the shipped-behavior documentation to explain the new uncertainty message and what users should do in these cases.
AGENTS.md reference: AGENTS.md:L6-L6
Useful? React with 👍 / 👎.
Synced from GitHub
🔇 Additional comments: 7
dashboard/src/modules/volume-run/plan.test.ts: 1
256-284: LGTM!dashboard/src/modules/machines/machine-removal.server.ts: 1
3-3: LGTM!Also applies to: 165-170
dashboard/src/modules/machines/machine-removal.server.test.ts: 1
2-7: LGTM!dashboard/src/modules/volume-run/volume-run.server.ts: 1
33-35: LGTM!Also applies to: 533-534, 570-570, 592-598
dashboard/src/modules/volume-run/volume-run.inngest.ts: 1
15-15: LGTM!Also applies to: 426-426
dashboard/src/modules/volume-run/volume-run.postgres.test.ts: 1
951-960: LGTM!Also applies to: 1031-1031, 1041-1062, 1065-1065, 1076-1076, 1086-1092, 1109-1116, 1145-1164, 1170-1170
dashboard/src/modules/volume-run/plan.ts: 1
237-247: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value⚠️ Unverified finding Verification ran but could not confirm this finding. It is shown for review, not as a verified issue.
Treat the unanswered check and the empty-stopped case explicitly.
restoreSourcehandles an emptystoppedlist only by accident.Math.max()of an empty array returns-Infinity.topis then empty, andtop[0]?.namereturnsundefined. The result is correct, but the code does not say so.Add an explicit early return so the intent is clear and does not depend on
-Infinitysemantics.Proposed change
diff
1 + if (stopped.length === 0) return undefined; 2 const second = Math.max(...stopped.map(({ newest }) => newest.created_unix_seconds));
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
d123cac9-77c1-4c16-80e3-0053a0bce09f
📥 Commits
Reviewing files that changed from the base of the PR and between 5cbdb1d9731b1a8eecd43e36919f63f9afe1c1d1 and 35cc2481e392086447471e6c962c05f447df35f0.
📒 Files selected for processing: 8
dashboard/src/modules/machines/machine-removal.inngest.tsdashboard/src/modules/machines/machine-removal.server.test.tsdashboard/src/modules/machines/machine-removal.server.tsdashboard/src/modules/volume-run/plan.test.tsdashboard/src/modules/volume-run/plan.tsdashboard/src/modules/volume-run/volume-run.inngest.tsdashboard/src/modules/volume-run/volume-run.postgres.test.tsdashboard/src/modules/volume-run/volume-run.server.ts
💤 Files with no reviewable changes: 1
- dashboard/src/modules/machines/machine-removal.inngest.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 0 remain after this review.