Repository navigation
fix(macapp): make the launch container sweep safe and complete (#284) - #374
Merged
Merged
Conversation
…nknown, 3 at most (#284) The launch sweep removed every labelled container older than its grace that config did not name, with `docker rm --force --volumes` and no prompt. A config that was lost, restored or half-written, or a descriptor that failed lenient decoding, would destroy live remote workrooms and any unpushed work on them. Now a sweep writes down the containers it finds unknown (hosts/unknown-containers-<runtime>-<context>.json) and removes only those the previous sweep, a launch earlier, found unknown too, and at most 3 of them; the rest go on later launches, 3 at a time, and the held-back count is logged. Images are unchanged: one a container still runs from is never removed. Both Docker and Apple's runtime. Still open from #284: no sweep runs while config records nothing. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… records nothing (#284) `RemoteHosts.adopt` swept only runtimes and contexts that config recorded a host on, so a crash during a project's first base create, before the base was recorded, left its container running for good. A container driver now writes `hosts/made-containers-<runtime>[@<context>]` before running a container, and `adopt` also sweeps every runtime and context so marked. A sweep that finds nothing of this build's left removes the marker once it is older than the grace, so a create under way keeps it. A Mac that never made a container workroom has no marker and is still never asked about Docker. Test processes (hosted unit runs and UI-test launches) read no markers, so they never sweep the developer's own runtime. The unknown-containers file now names no context as `<runtime>` and a context as `<runtime>@<context>`, so no context is no longer confused with one named `default`. Still open: the sweep is claimed before it runs, so a runtime that is down at launch is swept only on the next launch. Refs #284 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The launch's one sweep was claimed before it ran, so a runtime that could not list its containers at launch (Docker not started yet, say) went unswept for the whole launch. The sweep is now tracked per runtime and context. `claimSweep` hands out only keys whose sweep neither ran this launch nor is running, and `finishSweep` marks a key done only when its sweep listed its containers. A failed listing leaves the key owed, so the next reload sweeps it. A key is never swept twice in one launch, since the two-launch rule would count the second sweep as the next launch's. A sweep reports a failed listing with `ContainerHostDriver.listingFailed`. Held sweeps (#296) behave as before. Fixes #284 The boxd machine sweep moves to #373: comments that pointed at #284 for it now point there. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…left (#284) Pre-landing review fixes. The made-containers marker let a launch whose config records nothing sweep that runtime with every container unknown. A lost config does that, and so does a Dev launch with a throwaway HOME: the CLI's config follows HOME, but the app's Application Support (and so hosts/) does not. From the second such launch, up to 3 live workroom containers would go per launch. The marker is replaced by a pending-create journal, hosts/pending-containers-<runtime>[@<context>].json. A driver writes each container's name before running it, and the name comes off when config records the host (RemoteHosts.adopt), or when a rollback, a destroy or a sweep removes it. A launch sweeps every runtime and context with a name pending. With no container host recorded (a boxd host says nothing of the containers), it removes only pending names, still under the two-launch rule and the cap, and no images. A name is not dropped for missing from a listing: a key with no context lists whichever daemon is current, and the leftover may be on another. A sweep that cannot write its unknown list now deletes the old one and removes nothing. Before, the list left behind was an older sweep's, and could approve removing a container that was known in between. Tests: the journal around a failed create, a pending-only sweep, adopt with nothing recorded, a recorded host or only a boxd host, a failed write, and a failed listing keeping the last unknown list. The down-at-launch retry test now waits on the running-sweep count rather than a fixed sleep. Refs #284 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2df5664f69
ℹ️ 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".
There was a problem hiding this comment.
🟡 Changes recommended
Failed journal invalidation can permit unsafe container deletion, and the Docker integration test expects an incorrect result.
2 open findings
What changed in this PR
Makes launch-time container cleanup safer and recovers leftovers from interrupted creation, addressing #284 while leaving boxd cleanup to #373.
Changes:
- Adds two-sweep confirmation, removal caps, and pending-container journals.
- Retries unavailable runtimes on later reloads without repeating successful sweeps.
- Updates regression tests and design documentation.
| File | Description |
|---|---|
| macapp/WorkroomAppTests/RemoteHostsTests.swift | Tests sweep safety, pending journals, and retries. |
| macapp/WorkroomAppTests/ContainerProvisioningIntegrationTests.swift | Adds first-sweep retention checks. |
| macapp/WorkroomApp/Core/RemoteWorkrooms.swift | Coordinates per-runtime/context sweeps and pending recovery. |
| macapp/WorkroomApp/Core/ContainerHostDriver.swift | Implements journals, confirmation, and removal caps. |
| docs/designs/remote-workrooms.md | Documents cleanup behavior and deferred boxd work. |
🧠 Review effort: Balanced
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
|
|
||
| // The first sweep to find the orphans unknown only writes them down (#284). | ||
| let firstSweep = await driver.sweep(keeping: [keptID, instanceID], grace: 0) | ||
| XCTAssertEqual(firstSweep, []) |
Owner
Author
There was a problem hiding this comment.
🤖 Left by Claude Code.
Fixed in 2d79531. Confirmed against the code: sweep runs rmi without --force on an old, unrecorded image. On the first sweep the orphaned instance's container survives, so its image is still in use and Docker refuses it. The test now expects only image … failures from that sweep, and keeps the check that all four containers survive. It compiles. It was not run here, because it needs a live Docker fixture.
Copilot: the live-Docker sweep test's first sweep keeps the orphaned instance's container, so its image is still in use and Docker refuses the `rmi`. Expect only image failures there, instead of none. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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.
Fixes #284. The launch sweep of leftover containers (Docker and Apple's runtime) is now safe for live workrooms, and it no longer misses leftovers.
Behaviour changes
hosts/unknown-containers-<runtime>[@<context>].json. A config that was lost, restored or half-written therefore costs nothing on the launch that reads it.hosts/pending-containers-<runtime>[@<context>].json.HOME, cannot make a live container look disposable. (The CLI's config followsHOME; the app's Application Support doesn't.)The boxd case (no sweep for boxd machines) is split out to #373.
Validation
make app-testonRemoteHostsTests,ContainerProvisioningIntegrationTestsandAppleContainerIntegrationTests: 72 passed, 0 failed, 12 skipped.make app-lintis clean.onlyPendingdecision, including the boxd case/review:Not checked
ContainerProvisioningIntegrationTestswas updated for the extra first sweep.Known and left as is
🤖 Generated with Claude Code