vm: add reconcile-stale-create verb over a shared ownerless-create reclaim - #173
Merged
Conversation
…claim An embedder clearing creating skeletons at startup has no safe verb: rm --force takes the ops lock blocking, so against an in-flight clone it queues up and then deletes the freshly created VM, and raw state == creating cannot tell a skeleton from a live clone window. The free ops lock is the only sound predicate, and only the daemon could use it. Backend.ReconcileStaleCreate exposes that predicate as a four-outcome primitive (collected/busy/not-creating/not-found); the daemon's collectOwnerless and the new vm reconcile-stale-create verb share it so the two paths cannot diverge. Closes the cocoon side of #172 (PR-0 for vk-cocoon#54 Fix A).
…dalone helpers Whole-repo declaration-layout pass against the cocoon standard: within each file a type's exported methods precede all unexported ones, standalone functions sit below method sets, vocabulary types cluster ahead of the type they serve, compile-time interface checks stand above the implementing type, and test helpers follow every test func. Pure declaration moves, no behavior change. Fixed sites: hypervisor clone/restore/state/stop/supervisor/teardown (sandwiched unexported Backend methods), cloudhypervisor extend assertions merged above the type, oci layerEntry, meta json/sqlite store helper clusters, contracttest fixtures, localfile gc module, gc Register, storebench benchConfig, fat12 CreateFAT12, progress NewTracker, and three test files' helpers.
Simplify round on the PR: StaleCreateOutcome constants adopt the types.VMState trailing-comment convention, and fakeSupervisor's scripted busy/lockErr check is shared between TryLockVMOps and ReconcileStaleCreate instead of copy-pasted.
tonicmuroq
reviewed
Jul 28, 2026
tonicmuroq
left a comment
tonicmuroq
left a comment
Contributor
There was a problem hiding this comment.
Reviewed at 4d111122, all three commits, tests re-run locally (./hypervisor/... ./daemon/... ./cmd/... — green). LGTM with three non-blocking notes. This is the right PR-0: vk's Fix A now has a safe primitive, and the PR body's rm --force hazard analysis (blocking LockVMOps → queues behind a live clone → deletes the freshly created VM) is correct and worse than what issue #54's review assumed — good catch making this a prerequisite rather than reusing Runtime.Remove.
Verified
- Protocol is sound:
TryLockVMOps(non-blocking) →PeekRecordre-validation under the lock → state gate →collectStaleCreate(orphan-VMM check fails closed, then the tombstoned resumable delete). This is exactly the ownerless proof fromdaemon/reconcile.go's original comment, now reusable. No age heuristics, no force fallback — as designed. - Single shared body, three callers: daemon wrapper delegates outcome handling; GC keeps its own (already-held) lock + age gate and correctly calls the unexported
collectStaleCreate— calling the exported verb there would self-busyon its own lock. The "cannot diverge" claim holds. - Outcome tests are real:
busyactually holds the lock;collectedasserts both record-gone and name-freed;not-creating/not-foundcovered; daemon fake mirrors the semantics (incl. lockErr and busy paths after the 4d11112 dedup). - CLI/embedder contract: all four outcomes exit 0 with
{"id","outcome"}; resolve-timeErrNotFoundis mapped to thenot-foundoutcome instead of an error, which is what a startup-reconcile embedder wants.FindVM/FindHypervisorrefactor is behavior-preserving. docs/cli.md table matches the implementation.
Non-blocking notes
1. The layout commit's evidence claim is overstated — fix the claim, not the commit.
"Verified move-only by sorted added/removed line-set comparison" cannot have passed as stated: raw line-sets differ. Residuals I found (all structurally forced by the moves, all behavior-free): the interface-check block moved extend.go → cloudhypervisor.go, which adds 4 extend/* imports in the destination and removes 1 in the source (disk/fs/vfio stay — still used by method code), dissolves one var ( … ) wrapper, gofmt re-aligns the merged block's = columns, plus one added blank line. I verified there is nothing else hiding in the 501/501. Suggest the PR body say "move-only modulo forced import/var-block/gofmt adjustments" or describe the normalization actually used.
2. Spell out the embedder pattern for busy in docs/cli.md.
The table says "not worth retrying blindly"; the consumer guidance is worth one more sentence: at startup reconcile, busy means an external create/clone legitimately owns the record — leave it un-indexed and revisit on the next pass; it will either become a live VM or become collectable. That is the exact contract vk's Fix A needs, and writing it here keeps the two PRs honest.
3. Non-Supervisable backends hard-error.
backend %s cannot reconcile stale creates exits non-zero, which makes an embedder iterating mixed-backend records treat "unsupported" as "real failure". Fine for today (CH is the only supervisable backend in this deployment), but worth either a doc note or a fifth outcome (unsupported) if firecracker records can ever reach the verb.
Sequencing note for the vk side: with this merged, vk PR-1's Fix A should shell out to vm reconcile-stale-create -o json (or the Backend API if embedded) and branch on outcome, dropping the state==creating filter + Remove --force plan entirely — busy → skip indexing, collected/not-found → proceed to rebuild, not-creating → index as live.
Contributor
Author
|
Addressed the three notes:
|
CMGS
added a commit
that referenced
this pull request
Jul 29, 2026
The batch gc sweep kept a 24h age gate on creating records that the daemon's collectOwnerless already dropped in #173: create and clone acquire the VM ops flock before the placeholder write and hold it through finalize, the flock dies with its holder, and gcCollect already try-locks before collecting — so a free lock is the whole proof, same as the snapshot build lease one commit earlier. A crashed create is now reclaimed by the next gc pass instead of a day later on daemon-less hosts. CreatingStateGCGrace stays for capture/staging dirs and clone locks, where no held lock proves the owner died.
CMGS
added a commit
that referenced
this pull request
Jul 29, 2026
…rces (#177) * snapshot: make the save name preflight read the index the insert enforces `snapshot save --name X` checks the name twice through two lookups that disagree about pending records: - EnsureSnapshotNameFree goes through Inspect, whose lookupRecord rejects `r.Pending` and reports not-found; - insertRecord goes through the raw name index, which a pending record holds. A save killed after beginCreate but before finalizeCreate leaves exactly that pending record: rollbackCreate is cancel-proof but not SIGKILL-proof. Its name then looks free to the preflight and taken to the insert, so the save runs the whole capture -- a multi-GB memory dump plus disk copy -- and only then fails with `already in use by <ID>`. Retries repeat the capture every time; a real incident left 14 GiB of abandoned capture dirs behind. The preflight now consults the name index directly through a new optional NameHolder interface (same shape as Direct/DirectCreator), so a taken name is rejected in under a second, and the message names the holder so a caller can remove it by ID -- name resolution is not guaranteed to reach a pending record. Backends that do not implement it keep the Inspect path. This does not make the pending record self-collecting: GC still owns that, with pendingGCGrace at 24h. It only moves the rejection before the expensive work and makes it actionable. The vk-cocoon side removes the holder and retries. * snapshot: collect a dead save's pending record on lease proof, not age Every save holds its snapshot's exclusive build lease from placeholder to finalize and the flock dies with the holder, so a pending record whose lease GC can acquire is provably ownerless — the same free-lock proof vm stale-create collection uses. GC already takes the exclusive lease before every collect; dropping the 24h age gate from candidate selection and revalidation makes a killed save's name and capture dir reclaimable on the next pass instead of a day later. A live save is never collectable: its held lease fails the try-acquire. * review: move NameOwner into LocalFile's exported method set asl methodpartition: the new exported method landed below nine unexported ones. * gc: reclaim stale creates on lock proof, not age The batch gc sweep kept a 24h age gate on creating records that the daemon's collectOwnerless already dropped in #173: create and clone acquire the VM ops flock before the placeholder write and hold it through finalize, the flock dies with its holder, and gcCollect already try-locks before collecting — so a free lock is the whole proof, same as the snapshot build lease one commit earlier. A crashed create is now reclaimed by the next gc pass instead of a day later on daemon-less hosts. CreatingStateGCGrace stays for capture/staging dirs and clone locks, where no held lock proves the owner died. * perf: entry guard peeks in a read transaction entryGuard ran its tombstone check plus record load inside b.update even though the no-tombstone outcome writes nothing. Under the sqlite default every Update is BEGIN IMMEDIATE on the single writer connection, so each start/snapshot/restore/attach/detach/netresize serialized against the daemon's own writes for a read-only check. The guard now peeks tombstone and record under b.view — the caller's ops lock freezes both — and escalates to a write only to roll a leased tombstone back. ErrTombstoned lost its last producer and is removed. * review: name oversized signatures, embed the images forwarders Whole-repo /code + /simplify round: - Signatures past the 3-line budget get named carriers: CloneSpec (mirrors CreateSpec) through DirectCloneBase/CloneFromStream/ cloneBase, restoreRun for restoreCore, cloneLaunch + launchCloneFn for the FC clone chain, makeBodyFn/preCheckFn/findIDFn for the CH device ops, cliutil.TableFunc; cloudimg's three wrapped signatures reflow to match their single-line siblings. - OCI/CloudImg embed images.Ops so Inspect/List/Delete promote instead of forwarding; EnsureBaseDirs renamed EnsureDirs so promotion also replaces cloudimg's wrapper. - meta/json vocabulary types (table, genericFile) move above the types they serve; WithFunc strings drift-fixed (cloneAfterExtractParsed, cmd/snapshot handlers); eight 4-line comments compressed to the one-line budget. * review: name prepareClone's result, ContainsFunc sweep, import regroup Late p6 partition findings: prepareClone's seven-element tuple (two positional bare func()s) becomes cloneSetup; LegacyJSONPresent's existence loop becomes slices.ContainsFunc; reseed_test's cocoon-agent import rejoins the internal group per sibling files. NetworkSeam's constructor-between-type-and-methods report was a false positive — the NewFoo-after-type placement is the documented house pattern. --------- Co-authored-by: CMGS <ilskdw@gmail.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
2 participants
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.
Closes the cocoon side of #172 (PR-0 for cocoonstack/vk-cocoon#54 Fix A).
Problem
An embedder that must clear
creatingskeletons synchronously (vk's startup reconcile) has no safe verb today:vm rm --forcetakes the ops lock blocking (DeleteAll→LockVMOps), so against a record whose clone is genuinely in flight it queues behind the live clone and then deletes the freshly created VM.state == creatingcannot tell a skeleton from a live clone window; the free ops lock is the only sound predicate (create and clone hold it from prereserve through the final record commit), and until now only the daemon'scollectOwnerlessused it.Change
Backend.ReconcileStaleCreate(ctx, id)— the shared primitive:TryLockVMOps(non-blocking) →PeekRecordre-validation under the lock →collectStaleCreate(orphan-VMM check + tombstoned, resumable delete protocol). Four outcomes:collected/busy/not-creating/not-found. No age heuristics, no force fallback.collectOwnerlessis now a thin wrapper over the same primitive (SupervisableswapsCollectStaleCreateforReconcileStaleCreate), so the daemon and the verb cannot diverge. GC's sweep keeps its own lock+age-gated wrapper (it already holds the ops lock when it reaches the reclaim) but shares the samecollectStaleCreatebody.cocoon vm reconcile-stale-create VMwith--output jsonemitting{"id", "outcome"}; all four outcomes exit 0 so embedders branch on the JSON, errors stay non-zero. Documented in docs/cli.md.cmdcore.FindVMresolves ref → (owner hypervisor, VM) in one pass;FindHypervisornow delegates to it.A half-finished collect is resumable: the delete protocol tombstones first, so a crashed collect is picked up by the next verb invocation, the daemon's resumeDelete, or GC.
Also in this PR
A whole-repo declaration-layout pass (separate
review:commit, +501/−501 pure moves): exported methods before unexported per type per file, standalone helpers below method sets, vocabulary types ahead of the types they serve, compile-time interface checks above the implementing type, test helpers after all test funcs. 17 files fixed; the sweep covered all 340 Go files.Evidence
go test ./...green (new coverage: four outcome tests in hypervisor, daemon fake reworked to the new interface).make lint0 issues on GOOS=linux and GOOS=darwin;asl ./...clean on both.var ( ... )wrapper, gofmt re-alignment of the merged block).