Skip to content

review: whole-repo round — relaxed snapshot touch, pidfd wait, dead Scan path, cloud-image sniff, FindVM reuse, comment budget - #208

Merged
CMGS merged 7 commits into
masterfrom
review/night-2026-09-03
Sep 2, 2026
Merged

review: whole-repo round — relaxed snapshot touch, pidfd wait, dead Scan path, cloud-image sniff, FindVM reuse, comment budget#208
CMGS merged 7 commits into
masterfrom
review/night-2026-09-03

Conversation

@CMGS

@CMGS CMGS commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Whole-repo quality round (2026-09-03): every production file read against the /code Style Self-Check (8 readers) and the judgment lenses (engine, storage, host); five commits, net −197 lines, comment lines +22/−205.

  1. perfDataDir bumped LastAccessedAt on every clone-from-snapshot through a durable transaction (an fsync on sqlite, serialized on the single durable writer); the value only steers GC's LRU, so the touch commits relaxed. terminateWithPidfd held a pidfd but waited for exit by polling kill(pid, 0) every millisecond; it polls the fd, so the kernel wakes it on exit (the VM stop path in every vm rm/release).
  2. cutLog.Scan and metalog.Recorder.Scan had no caller anywhere in the workspace (the metering log was write-only); the OCI sizer's stat fallback served index entries older than v0.1.0; the Firecracker clone renamed cow.raw onto itself (runDir and COWRawPath derive from the same id and config); runningVMClientWithRecord's record was discarded at all three call sites.
  3. fix — the cloud-image Range probe fetched one byte, below the four-byte sniff floor, so a URL serving an HTML error page was downloaded in full (up to 20 GiB) before nonImageSignatures rejected it (perf(cloudimg): sniff before full download and reuse the non-Range probe response #137's Range half); a snapshot delete resumed from a tombstone emitted its storage-stop metering with an empty hypervisor; Prepare and the Add rollback bypassed three of the seven CNI test seams and the helper left statNetnsFn real, so Remove's TAP deletion had no coverage on any platform; gcTick ignores a nil GC hook; the bridge GC skips its LinkList when it has no prefixes to match.
  4. docs — docs/gc.md described a lock-all-modules cycle the code never had and a nics=2/2 log field it never emits; docs/vm.md named a REMOVED event verb (the code emits DELETED) and the json engine's watch mechanism; docs/images.md and the flag help scoped --force to URL pulls (OCI tags are re-resolved every pull); docs/cli.md's tree shows the --from-dir positional.
  5. review — eight verbs resolved a ref through FindHypervisor and immediately Inspected the same ref (the resolver's probe is that Inspect); FindVM returns the record. LogPath opened two views where ResolveAndLoad does one. vm list/vm status --watch reconciled every VM's state twice per render and the watch path never assigned it, so THROTTLED could contradict STATE. CNI interface names come from one helper and ifIndex rejects trailing garbage. Named scan-callback types replace three spelled-out func types. Comments shrink to the one-line budget; test files carry no comments (284 lines); six godocs on interface-implementing methods go. core.ImageJSONNamespace is exported for cocoon-macos, which lost BaseConfig.JSONNamespace in cut: network Inspect/List and batch Delete, Reserver hard-require; images stop importing meta/json #207.

Held with measurement or reason: the CNI Add's four networks.json parses per run/clone (measured 0.1–1.0 % of a 230 ms copy-mode clone; both durable commits are required); deleteOneLocked re-walking /proc per stopped VM (reusing the pre-stop scan contradicts the re-observe-under-lock rule at supervisor.go:83); the oci/cloudimg GC method pairs (a shared base costs more lines than it saves); unexporting ten package-private hypervisor helpers (surface only; the layout churn is not worth it); the vmlock 2 ms retry (the same cadence as gofrs/flock, no cancellable primitive).

Gates: GOWORK=off make lint 0 issues on linux and darwin; make fmt-check clean; asl ./... clean on both GOOS; GOWORK=off go test -race -count=1 ./... green (the Linux-gated CNI/bridge/pidfd paths run in CI). Round diff: 106 files, +269/−466.

CMGS added 7 commits September 3, 2026 02:13
…es the kill(0) poll

DataDir bumps LastAccessedAt on every clone-from-snapshot through a
durable transaction (an fsync on sqlite, serialized on its single
durable writer). The value only steers GC's LRU, so a lost touch costs
one retention round; the touch commits relaxed now.

terminateWithPidfd held a pidfd yet waited for exit by polling
kill(pid, 0) every millisecond; it polls the fd itself, so the kernel
wakes it on exit.
…lient record return

Log.Scan's only production caller was metalog.Recorder.Scan, which no
caller and no interface reaches. The OCI sizer's stat fallback served
index entries older than v0.1.0. The Firecracker clone renamed cow.raw
onto itself: runDir and COWRawPath derive from the same id and config.
runningVMClientWithRecord's record was discarded at all three call sites.
…ad keeps the hypervisor; CNI seams; GC guards

The Range probe fetched one byte, below the four-byte sniff floor, so a
URL serving an HTML error page or a compressed payload was downloaded in
full (up to 20 GiB) before nonImageSignatures rejected it. The probe now
carries the first eight bytes and sniffs before downloadRangesParallel.
A snapshot delete resumed from a tombstone emitted its storage-stop
metering with an empty hypervisor; the payload records it. Prepare and
the Add rollback bypassed three of the seven CNI test seams, and the test
helper left statNetnsFn real, so Remove's TAP deletion had no coverage.
gcTick ignores a nil GC hook; the bridge GC skips its link dump when it
has nothing to match.
…aming helper, comment budget

Eight verbs resolved a ref through FindHypervisor and immediately
Inspected the same ref again; FindVM already returns the record. LogPath
opened two views where ResolveAndLoad does one. vm list and vm status
--watch reconciled each VM's state twice per render and the watch path
never assigned it, so THROTTLED could contradict STATE. CNI interface
names come from one helper and ifIndex rejects trailing garbage. Named
scan-callback types replace three spelled-out func types. Comments
shrink to the one-line budget; test files carry none; six godocs on
interface-implementing methods go. ImageJSONNamespace is exported for
cocoon-macos.
…vfio exports

Generic tombstone recovery (gcRecover, DataDir) rolled an interrupted delete
forward without closing the snapshot's metering interval; the recovery path now
emits the stop with the hypervisor carried in the tombstone payload.
ErrAlreadyRunning and NormalizePath stay exported for embedding callers.
@CMGS
CMGS merged commit 51ff88b into master Sep 2, 2026
4 checks passed
@CMGS
CMGS deleted the review/night-2026-09-03 branch September 2, 2026 19:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant