snapshot: make the save name preflight read the index the insert enforces - #177
Conversation
…rces `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.
3fe0015 to
986c531
Compare
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.
asl methodpartition: the new exported method landed below nine unexported ones.
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.
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.
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.
|
Rebased onto master (post-#176 squash), then extended per plan; five commits on top of the preflight fix: f5d0b9b — lease-proof early collection (the follow-up the PR body scoped out). Every save already holds its snapshot's exclusive build lease from placeholder to finalize (beginBuild, since #117 — no released version without it), and GC already try-acquires that lease before every collect. So a pending record whose lease GC can acquire is provably ownerless, and the 24h pendingGCGrace age gate was adding nothing but a day of delay: candidate selection and revalidation now key on Pending alone. A live save is never collectable — its held lease fails the try-acquire (pinned by new tests both ways). docs/gc.md updated. 5cfac37 — the same cut for VM creating records. The batch gc sweep kept a 24h gate the daemon's collectOwnerless already dropped in #173; create/clone lock before the placeholder write and hold through finalize (verified at cmd/vm/run.go prereserveVM), so gcCollect's try-lock is the whole proof. CreatingStateGCGrace stays for capture/staging dirs and clone locks where no lock proves death. 9201ba0 — perf: entryGuard peeks in a read transaction, escalating to a write only for a leased-tombstone rollback; under the sqlite default the old shape put every start/snapshot/restore/attach/detach/netresize on the single BEGIN IMMEDIATE writer connection for a read-only check. ErrTombstoned lost its last producer and is gone. e3b74e9 + 547b8df — whole-repo /code + /simplify round (12 partition auditors over all 341 Go files + 4 repo-wide lenses, findings verified before applying): NameOwner moved into the exported set (our methodpartition gate caught it), oversized signatures named into carriers (CloneSpec/restoreRun/cloneLaunch/makeBodyFn/preCheckFn/findIDFn/TableFunc), OCI/CloudImg now embed images.Ops instead of forwarding Inspect/List/Delete, EnsureBaseDirs→EnsureDirs promotion, meta/json vocab-type placement, five WithFunc drift fixes, eight 4-line comments compressed. ~55 two-to-three-line wrapped comments were flagged and deliberately left — single-thought wraps are the established house style. Gates: go test ./... green, golangci-lint 0 issues and asl clean on both GOOS. Branch net vs master: prod +206/−238 (net −32), test +101/−14. |
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.
|
One more commit: the last partition auditor reported late — prepareClone's 7-tuple named into cloneSetup, a ContainsFunc modernization, one import-group fix (its fourth report, NetworkSeam constructor placement, was a false positive per the documented NewFoo-after-type pattern). Full audit coverage now stands at 341/341 files. Gates re-run green: go test ./..., golangci-lint and asl clean on both GOOS. |
2 participants
snapshot save --name Xchecks the name twice through two lookups that disagreeabout pending records:
r.Pendingand reports not-found;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 realincident 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.
Surfaced by fault-injection against vk-cocoon (cocoonstack/vk-cocoon#58); pairs with cocoonstack/vk-cocoon#60.