Repository navigation
Conversation
Retain current, acknowledged and attempted checkpoint references until claim writes settle. Queue cleanup after partial failures and keep expiry progressing without an archive registry.
Use the existing store reference set and pending checkpoints instead of rescanning live claims. Guard stale hibernate and archive writers before they can overwrite a released or replaced claim row.
|
Following up on #379: the implementation for all four parts is complete. This PR is the first piece, split out to keep review focused. I'll submit the remaining parts in dependency order as the earlier PRs land, and handle rebasing and any follow-up changes from review. |
|
Here are the complete-series performance results, including the later volume layers. This is a cumulative source-head comparison, not the isolated effect of #380. Main is
For the checkpoint-pin row, #380 alone measured 392.532 µs. The complete series measured 390.724 µs, so the later layers retain that improvement. The full predicate loop is still 22.8% slower than main. The production replica scan now takes one detaching-owner snapshot instead of repeating that query for every claim; its observed range overlaps main. Commit, checkpoint-positive admission and JSON ranges also overlap, so I am not claiming a speedup for those rows. Admission without checkpoints has a 1.8% higher median and adds 16 bytes per operation; its observed ranges do not overlap. The fake-engine provision/release median is 1.9% higher, with six more allocations. I used the delta/refcount approach you suggested: a This does not make every operation O(delta). The existing full-file Method: Linux amd64, Xeon Gold 6548Y+, Go 1.27.2, GOMAXPROCS=4, four pinned physical cores, five serial interleaved runs per side, 500 ms per case. Profiles ran separately. Pool fixtures have 5,000 background claims. Checkpoint-positive commit/admission cases give each claim a checkpoint. The one-volume scan keeps 5,000 IDs by leaving its volume owner unpinned and giving another owner a second reference; no capture is due. The scan checks all 5,000 resulting states and a reset probe, and the no-op control fails. Files use tmpfs with The native checkpoint benchmarks, volume benchmarks, replica scan benchmark and JSON benchmark are committed. Measurement logs and audit artifacts stay outside the repository. |
CMGS
left a comment
CMGS
left a comment
There was a problem hiding this comment.
thanks for the split, and for the bench files. the delta + refcount design is what i hoped for. i read every prod change and found no correctness bug. the claim-path checks that #379 added are gone.
what i ran on 6638904 (mac arm64, go 1.27):
make go-lint: 10 of 100 issues.(5 modules x 2 GOOS).asl ./...on both GOOS: only the same 5forwarderadvisories as main. nothing new.- added comment lines in the diff: 0. use-modern-go: nothing applies.
go test -race ./pool/: one flaky test, see inline onreplica.go.
my bench, interleaved main/pr/pr/main/main/pr, 2 runs each, 5000 claims, every claim holds a ReplicaCk:
| path | main | this pr |
|---|---|---|
| claims.json commit, no ck | 0.41 ms, 19 allocs | 0.41 ms, 19 allocs |
| claims.json commit, with ck | 0.49 ms, 19 allocs | 0.49 ms, 19 allocs |
| volume claim admission, with ck | 3.6 µs, 10 allocs | 3.6 µs, 10 allocs |
hasVolumeHolds over all claims |
43 µs | 43 µs |
pinnedArchiveCks, no ck |
36 µs | 0.04 µs |
pinnedArchiveCks, with ck |
260 µs, 48 allocs | 112 µs, 18 allocs |
| purge of one archived claim (with the caller's marker) | 4.9 ms | 8.9 ms |
the last row is a mac number, so the fsync is slow. the point is that it has 2 dir fsyncs now, not 1. see inline on claim.go.
size: prod is +244/-73, tests are +950. a few cuts are inline. the rest of the tests read fine to me.
CMGS
left a comment
CMGS
left a comment
There was a problem hiding this comment.
follow-up after a fuller pass. first a correction: in my last review i wrote "the rest of the tests read fine to me". i had not read checkpoint_cleanup_test.go and stale_claim_test.go in full then. i have now, and the notes are inline.
more evidence on 6638904:
- linux arm64 container, go 1.27.1,
go test -race ./pool/:- as uid 1000: pass. the setpriv path in
withoutDACOverridealso works as root. - as root:
TestRestartThatCannotArmTheEgressDoorFailsAndSpendsTheBudgetandTestRenewRollsBackOnPersistFailurefail. main fails the same two as root, so they are not from this pr.
- as uid 1000: pass. the setpriv path in
- the
dropReplicaflake also shows on linux: 2 of 300 runs fail, 0 of 300 on main. - use-modern-go
liston all 19 touched files: same 54 guidelines, none applies to the diff. - size: prod +244/-73 (net +171, +168 of it in
claims.go), tests +950/-16. prod comments inpoolstay at 389.
new inline notes are on the claims.go bookkeeping and the tests. none of them is a correctness bug.
|
Following up specifically on the archived-claim purge regression, the review's 4.9 ms → 8.9 ms result was for caller marker + purge on macOS. I have now measured that path directly on Linux/ext4, separately from full The extra marker-directory fsync is gone. Current latency is back within main's observed range on this workload.
The benchmark times the caller's Method: Xeon Gold 6548Y+, Linux amd64, ext4, Go 1.27.1, GOMAXPROCS=4, four pinned physical cores. Five serial interleaved 500 ms runs per revision. The table excludes the separate The three revisions used the same instrument and checked completed work. All 15 timing runs passed. Raw traces confirm the 1/2/1 successful fsync counts. Main and current timing ranges overlap. These Linux values establish the local result; they do not replace or reproduce the absolute macOS timings from the review. The fix is in 02b96ab: already-marked release/reap calls |
A [4]string on every claimRow made each row 64 bytes larger, although most claims hold no checkpoint. The store now keeps current references in its own map, holding only claims that have one, so rows stay at main's size. checkpointRefs replaces newClaimRow and is the one place that lists the four checkpoint fields; load and the test manager's consistency check use it too.
set, del and reset each moved the reference counts and then replaced or dropped the row, and the two steps had to stay in that order. put does both, with a nil sandbox meaning delete, so a caller cannot get the order wrong or update one without the other. load now reads references in the same loop that fills VolumesWant, and records and marks only the claims that have some. A claim without references had nothing to retain, so marking it changed was a no-op. commit captures each changed claim's references as a [4]string and compares them with ==, so the copy back into fixed slots goes. appendRefs builds both the attempted and the written retained lists.
replicate joined the clear error into the error it returned. When the claim had gone, the clear returned ErrUnknownSandbox, and benignSweepErr then treated the joined error as benign, so the sweep hid the persist or registry failure. replicate returns main's error again and ignores the clear result; settleReplicaIntent still returns it.
The capture, failed-write and post-rename tests ran one sequence: write
under a fault, change the claim, check what stays pinned, commit, check
again. They are now one table of {acked, next or startup rewrite,
fault, mutation, on disk, pinned after the fault, pinned after the
commit}. The on-disk column shows that every checkpoint claims.json
names stays pinned.
TestClaimReferencesSurviveRenameFailure goes: it reached the same
failure branch as failClaimWrites. So does the assertion that mutated
the checkpointReferences result to prove it is a copy. The shared
reference test keeps its two checks that can fail; the others ran while
the second owner still held the checkpoint.
TestRecommitDrainsAChangeItWasTooBusyToSee is back to main's body; the
table covers the snapshot and reference checks it had gained.
The replica retirement, unpublished intent and settled intent tests ran
one sequence: break claim writes, act, check the checkpoint and its
delete marker survive a retry, restore, restart or commit, check the
cleanup finished. They are now one table of {setup, act}, each run live
and across a restart; the settled intent gains the restart case.
The consumed-row purge keeps only its released-claim case, as a table
row; its claimed case repeated the retirement checks. The table's tail
holds the one unrelated-write check and runs the orphan sweep to show
no tombstone outlives the landed write.
failClaimWrites now parks the background retrier itself, so callers no
longer set recommitting by hand. The no-registry fixtures move into
their one test, the checks that could not fire go, and the restored
claim is compared through checkpointRefs.
The stale hibernate test and the intent rollback test both claim,
checkpoint, make the claim stale, released or replaced, and then check
claims.json and the checkpoint cleanup. They are now one table of
{newManager, prepare, run}, each run with and without a replacement,
and the one-caller assertion helper is inlined.
The stale hibernate now drops the claim the same way the rollback cases
do instead of going through Release; either way the sandbox is no
longer the claimed one, which is what hibernateLocked checks. The
writeMu cleanup guard goes: no cleanup takes writeMu, so a failure
there only leaks the blocked goroutine.
The sandbox was claimed one line earlier, so setIfLive always reports it live there. Were it not, the uncommitted intent would fail the adoption checks that follow anyway.
The orphan sweep listed the four checkpoint fields by hand, a second copy that would drift if a field were added.
|
Thanks for the careful review and the concrete sparse-map patch. The four new threads are addressed in I also checked the follow-ups for the same patterns and removed their old reference wrappers, duplicate possible-disk histories and redundant SDK copies. The complete series is at The updated PR head passed its full race suite as UID 1000, build/vet, formatting and Linux/Darwin lint. ASL matches main's five existing advisories. The complete-series head also passed fresh bounded/source mutation checks and the real Linux VM lifecycle run. Tools and evidence remain outside the repository. No follow-up PRs were opened. |
|
Fresh complete-series measurements after the simplification. Main is Linux amd64, Go 1.27.1,
The archived-release comparison is 1.146 → 1.862 → 1.154 ms for main / previous / updated. Updated is 38.0% faster than the previous complete series, and its range overlaps main. A separate syscall trace confirms two marker-directory fsyncs before and one after. Existing reference changes improve 13.0% over the previous series. New-claim latency ranges overlap, so I am not claiming an improvement there; allocations drop from 2 to 1. Existing reference changes drop from 3 to 1 allocations. Both updated mutation cases use 640 B/op, matching main, and remain slower than main in time because they maintain reference accounting.
Commit, pin and downstream ranges overlap the previous series. These runs do not establish a change on those paths. All 15 smoke, 75 primary timing and 48 downstream runs passed work/error checks. Commit writes and reloads the real 5,000-row file, and pin collection checks every expected ID. Archived release uses a fake engine and ext4, includes the durable marker and deletion work, and checks the resulting claim/checkpoint/marker state. CPU/commit fixtures use tmpfs; |
* pool: check one archive checkpoint pin through the reference counts archiveCkPinned copied the whole pin set under m.mu to test one id. The claim store already counts references, so the check is a map lookup: 5000 checkpoint claims, mac m2 max, interleaved, 162 us / 218 KB / 18 allocs -> 25 ns / 0 allocs. DeleteCheckpoint, retryArchiveDelete (once per marker) and dropStaleRow call it. * pool: inline the single-caller deleteCkLocked and drop a dead channel receive * docs: state checkpoint retention after failed journal writes and with failover off Also fix the replica release order (tombstone before replica) and the on_expire value (destroy, not release).
This is the first PR in the four-part stacked series split out after the #379 discussion. It contains the independently reviewable expiry, replica and checkpoint fixes.
Why
A failed claim write can leave a checkpoint referenced on disk after memory has moved on. Expiry can also stall after archive failover is disabled. These node-wide fixes are separate from volume registration.
What changed
refs map[string][4]string, leaving ordinaryclaimRowvalues at main's size.checkpointRefsextracts the four fields directly for load, updates, the invariant test and orphan cleanup.Runtime catalog registration, live volumes with SDK support, and the controller example remain separate follow-ups. Their updated branches are on the fork; this PR keeps its original scope.
Verification
This PR's updated head is
7cb73793d97d7ff95c3a789171b4d799343cac08.ebb8409also passed its full race suites and 358 Python tests on both Python 3.9 and 3.14, plus CI-pinned Ruff and mypy. A refreshed depth-7 model passed five bounded scopes; 12 compiled source mutations failed their intended assertions. These are bounded checks, not a refinement or power-loss proof.Complete-series performance, refreshed this round
Main is
29790b9, the previous complete series isdbeb210, and the updated complete series isebb8409. This table measures the complete series, including the follow-ups, rather than attributing their results to this first PR alone. Earlier measurements in the discussion remain bound to their original commits.Linux amd64, Go 1.27.1,
GOMAXPROCS=1, one pinned CPU. Five serial interleaved runs per case and revision, 500 ms per run. Every background claim in the 5,000-claim fixtures holds a distinct checkpoint. Values are median [min, max]. Profiles and syscall traces ran separately.Existing reference changes improve 13.0% over the previous complete series. New-claim allocation drops from two to one, but its latency ranges overlap, so I am not claiming a latency improvement there. Both mutation cases remain slower than main, which lacks the reference accounting.
Archived release improves 38.0% over the previous complete series and overlaps main. Separate traces confirm two marker-directory fsyncs before, one after. Commit and pin ranges overlap the previous series. Serial file writes/copying limit commit; map traversal/insertion and GC remain visible in pin collection.
The downstream ranges overlap; these runs do not establish a regression. Admission still resolves/stats one backing. Replica scans check all 5,000 states and the expected volume skip.
All 15 smoke runs, 75 primary timing runs and 48 downstream runs completed with nonzero work and no errors or skips. Commit writes and reloads the real approximately 1.189 MB claims file; pin collection checks every expected ID. Archived release uses a fake engine and ext4, with setup outside the timer and
sync_claims=falseon every arm; it checks the claim, checkpoint, pending state and marker are gone. CPU/commit cases use tmpfs. These are local CPU/filesystem measurements, not production VM or macOS latency.The existing full-file claim replacement remains; this does not make every operation O(delta). Benchmark runners, formal models, audit records, raw logs and retained VM evidence stay outside the repository.