Repository navigation
fix(durable): add write fencing and reject conflicting steps - #229
Conversation
d09df8d to
456039d
Compare
ACR round 1 (one reviewer per dimension)Seven reviewers read this PR, then read each other's findings to settle conflicts before any code changed.
Blocker, folded in: the README and docs site said an owner that lost an instance "can't keep writing". Both now state when the fence applies and the one-open-store limit. Non-blockers, folded in:
Reviewers disagreed, and the maintainer decided:
Dropped: comparing digests on keyed appends, which would make an honest retry conflict. Deferred: hashing outside the Known and left open: a keyed |
3177871 to
a2ade84
Compare
ACR rounds 2 to 4: human-readability
Round 3 hit the ACR limit of three rounds. The maintainer chose to keep going, so round 4 ran as a normal round. Round 4's non-blockers are small godoc additions, and they land with the next rebase of this stack. All seven dimensions are now confirmed. |
08e3dcb to
3950c55
Compare
1d93173 to
6e33531
Compare
6e33531 to
b207bf1
Compare
52f82b1 to
af3597e
Compare
Store writes can carry a fence token, and a write below the highest token seen returns ErrFenced and writes nothing. Append now rejects a different record at a recorded step with ErrStepConflict instead of acknowledging a step the journal doesn't hold. Relates to #214. The failed-Append fix and the Handle lock are not included. Signed-off-by: Ray Karnes <ray.karnes@stablekernel.com>
…ions Signed-off-by: Ray Karnes <ray.karnes@stablekernel.com>
Folds in the review of the fencing change. The docs no longer overclaim what a fence guarantees. The Store contract now says the token must be as durable as the Records and the check atomic with the write, and states how explicit keys behave. MemStore treats an instance with a refused first write as never written, matching FileStore, and a directory with no fence keeps its old meaning. Adds a golden digest test and a racing-token test. Relates to #214. Signed-off-by: Ray Karnes <ray.karnes@stablekernel.com>
af3597e to
a92f50a
Compare
A stacked PR re-ran the full suite on every push, though only the PR based on main can merge. Each PR runs CI once the stack retargets it to main, which saves GitHub Actions minutes. Signed-off-by: Ray Karnes <ray.karnes@stablekernel.com>
joshua-temple
left a comment
There was a problem hiding this comment.
Approved pending relayed GB message
ErrInstanceNotFound = errors.New("crucible/durable: instance not found")
For all of the sentinel errors in this durable package, lets swap the value contents to private constants and drop the crucible prefix
Consider this a 'while you're in there' patch - it can be a part of this PR
eg
const (
errorPrefix = "durable: "
errorNotFound = "not found"
errorInvalidStatus = "invalid status"
errorUnauthorized = "unauthorized"
var (
ErrNotFound = errors.New(errorPrefix + errorNotFound
ErrInvalidStatus = errors.New(errorPrefix + errorInvalidStatus)
ErrUnauthorized = errors.New(errorPrefix + errorUnauthorized)
)…e prefix Sentinel error text now lives in private constants built on errorPrefix. Every error in the package now starts with "durable: " rather than "crucible/durable: ". Exported names are unchanged, and errors.Is matching is unaffected. Signed-off-by: Ray Karnes <ray.karnes@stablekernel.com>
The advisory, published after this branch last passed, covers a panic on two-block frames and fails govulncheck on every PR into main. Four modules reach the vulnerable code; all 19 that carry the dependency move together. Signed-off-by: Ray Karnes <ray.karnes@stablekernel.com>
What this change does
First of the #214 fixes, at the bottom of stack #239, on
main. It also limitsci.ymlto PRs intomain, so a stacked PR runs CI only once it reaches the bottom of the stack; that's a separate commit, to save GitHub Actions minutes. Each PR in this chain carries the tests for what it fixes, so it passes on its own.ErrFencedand writes nothing. AddsWithFence,WithCheckpointFence,AppendFence,CheckpointFenceand the runner'sWithOwnerFence.FileStorekeeps the token in afencefile.AppendreturnsErrStepConflictfor a different record at a recorded step, instead of acknowledging a step the journal doesn't hold. AddsAppendIdempotencyKey, so an out-of-treeStorecan read the key.durable/README.mdand the docs site now cover fencing and thefencefile.After
Tick's append succeeds and its checkpoint fails,nextStepdoesn't advance, so the next write on that Handle conflicts until it's recovered.RunServiceandDeliverToActordo the same. #233, further up this stack, fixes that.Not included: event data is in #230, the failed-
Appendfix and theHandlelock are in #233, and the #228FileStorecrash bugs get a separate PR.Please check these:
WithCheckpointFencehas its own name, becauseAppendandCheckpointtake different option types.Recoverwrites no fence token, so a stale write landing before the new owner's first write is accepted, and the owner then getsErrStepConflict.WithIdempotencyKeyskips the record comparison.FileStorefencing only holds within one open store;MarkDispatchedisn't fenced.Why
Without a fence, a stale owner can keep writing to a session it has lost, and
Appendcan acknowledge a step the journal doesn't hold. #214 has the defects.Relates to: #214
Evidence
Run locally on Go 1.27.2, on
durableandstateonly:go test ./...indurableTestRecover_TwoHandlesOnOneInstance_JournalMatchesAnAcknowledgedHistorygo test -race -skip TestRunner_FireDuringTimerLoop_LosesNoAcknowledgedSteps ./...indurableHandlelock defect, and it races on the base toogo test ./...instateGOWORK=off go build ./...indurablego run golang.org/x/vuln/cmd/govulncheck@v1.3.0 ./...indurable,stateOn CI's Go (
GOTOOLCHAIN=go1.27.3), the pinnedgolangci-lintv2.15.0 reports 0 issues indurableandstate.#231 has merged, which fixed the vulnerability findings.
Checklist
git commit -s) per the DCOtype: subject)mage checkpasses locally (lint + race tests + govulncheck): not run as a whole, and it can't pass while the tests above faildurable/fence_test.go