fix: correctness bugs across engine, generator, CRDT, and conditions#45
Merged
Conversation
The reflection apply path for OpCopy resolved the source value and called Set with the live reflect.Value, which for reference types (slices, maps, pointers) made the destination share the source's backing storage. Subsequent mutations to either side leaked into the other. OpMove already handled this correctly by allocating a fresh value before Set; OpCopy now does the same via icore.DeepCopyValue so the destination owns its storage. Add TestOpCopyDeepCopies covering []int and map fields.
MapKey formatted keys with fmt.Sprintf("%s/%v", ...) directly into the path
string, so a key containing '/' (e.g. "a/b") produced "/M/a/b" — the path
navigator then treated it as two segments and failed with "map key a not
found". '~' suffered the same RFC 6901 layering issue silently.
Apply icore.EscapeKey to the key before joining so '/' becomes "~1" and
'~' becomes "~0", matching ParsePath's unescaping. End-to-end test asserts
both the produced path string and a successful Set through a key with '/'.
Patch.Reverse switched on op.Kind but had no case for OpLog, so a reversed OpLog became a zero-valued Operation. Since OpAdd is the zero value of OpKind, the result was an OpAdd at the original log path with nil New — re-applying the reversed patch would attempt a bogus add. OpLog has no state effect, so the correct inverse is to drop it. Update TestPatchReverseExhaustive to expect 5 reversed ops (OpLog skipped) and add TestPatchReverseOpLogOnly covering the all-log case.
Operation.Old was overloaded: prior value for OpRemove/OpReplace, but source path string for OpMove/OpCopy. The dual use blocked Reverse from restoring the displaced value when an OpCopy/OpMove had overwritten an existing destination — Old was already spoken for as the source path. Add Operation.From (json "f", omitempty) carrying the source path for OpMove/OpCopy and reserve Old to always mean "prior value". Move and Copy constructors, JSON round-trip, Walk-to-Operation flatten, String(), and the reflection apply path all switch to From. Reverse now produces OpReplace for OpCopy with a non-nil Old (restoring the displaced value) and a symmetric OpMove (Path<->From) for OpMove. Wire-format break: patches serialised with the previous "from" field encoding via the Old field will not round-trip. Generated *_deep.go files are unaffected (they never read Operation.From/Old for Move/Copy — those ops fall through to the reflection path). Tests cover OpCopy reverse with a captured prior value and the symmetric OpMove reverse.
TestEngineFailures previously called deep.Apply for both cases without checking the returned error, so a regression that silently no-op'd these ops would have gone undetected. Tighten the assertions and add coverage for the empty-From early-reject path added in the From/Old split.
The generated applyOperation's root-level strict check used an
unchecked op.Old.({{.TypeName}}) assertion. A strict OpReplace at "/"
whose Old carried any other concrete type would panic rather than
report a strict-check failure.
Switch the assertion to comma-ok and report the strict-check error on
either a type mismatch or a value mismatch. Regenerate all in-tree
examples and add TestStrictRootMismatchedOldType to lock in
non-panicking behavior.
The pointer-variant ApplyOpReflection has always replaced a nil logger with slog.Default. The value-variant ApplyOpReflectionValue did not, so any caller invoking it directly with a nil logger and an OpLog op would NPE on logger.Info. Apply the same nil-check at the top of the value variant. Add TestApplyOpReflectionValueNilLogger covering the OpLog+nil-logger path.
Clock.Latest is exported for serialisation, but direct field writes bypass the clock's internal mutex and race with concurrent Now/Update. The CRDT package's UnmarshalJSON assigned Latest directly, leaving the race detector to fail any test that decoded a CRDT while another goroutine was editing it. Add Clock.SetLatest, which takes the mutex before assigning, and route CRDT.UnmarshalJSON through it. Document Latest as exposed only for serialisation. Add a race-targeted test that exercises SetLatest alongside concurrent Now calls.
Clock.Reserve took n as int and assigned int32(n) into the Logical counter, so a reservation that would push Logical past math.MaxInt32 silently wrapped around. The wrap is invisible at the call site but breaks HLC monotonicity and therefore causal ordering. Pre-check n against the remaining int32 headroom and panic if Reserve would overflow; also reject negative n. Both states are programmer errors and far less harmful as a panic than as a silent wraparound. Add tests covering both panics.
Set.Len delegated to len(s.Items()), which materialised the full deduplicated slice of live elements just to take its length. Inline the dedup against a single map and return its size instead — same result, no slice allocation. Test asserts Len agrees with len(Items) across a churn pattern that includes duplicates and tombstones.
Any {"op":"test","path":"/","if":...} entry in the JSON Patch document
was consumed as the global Guard, so a user-authored second such entry
silently overwrote whatever Guard the leading entry already produced.
Restrict the heuristic to the document's first entry, matching what
ToJSONPatch emits. Document the wire convention on ParseJSONPatch.
Test asserts a trailing same-shape entry leaves Guard untouched.
A Not condition with no Sub fell through the early-return branch and continued into the path-based comparison logic, which would silently mis-resolve the empty Path or produce a confusing error from a downstream operator. Convert the malformed-input case into an explicit error at the top of the Not branch so callers see a clear diagnostic.
Investigating the review note about Set.Add advancing the HLC twice (once for the tag, once inside Edit for the Delta timestamp) showed neither order produces a single-tick path: Edit always ticks for any non-empty patch, and Add always produces a non-empty patch. The duplicate tick is harmless under the HLC mutex. Drop the open question by documenting the design on Add so future readers don't re-spend the analysis budget.
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
1 participant
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.
Summary
Twelve correctness/robustness fixes from the recent code review, plus one doc-only follow-up. Each commit is self-contained and pairs the change with a test.
Bug fixes
OpCopyon reference types (slices, maps, pointers) now deep-copies the source so the destination owns its storage. Previously the two fields aliased after Apply.MapKeyRFC 6901-escapes the key (/→~1,~→~0) so keys containing pointer specials navigate correctly.ReversedropsOpLogoperations (they have no state effect; the old code emitted a zero-value Operation that defaulted toOpAdd).ParseJSONPatchonly lifts the leading{op:test, path:/, if:...}into Guard; a later same-shape entry no longer silently clobbers it.op.Old.(T)assertion so a strict OpReplace with a wrong-typed Old returns an error instead of panicking. All in-tree examples regenerated.ApplyOpReflectionValuenow nil-checks the logger, matching the pointer variant.Clock.SetLatestrehydrates the clock under the mutex;CRDT.UnmarshalJSONroutes through it. DocumentedLatestas serialisation-only.Clock.Reservepanics on negativenorint32overflow instead of silently wrappingLogicaland breaking causal ordering.Notwith an emptySubreturns an explicit error instead of falling through to path resolution.Set.Lenno longer allocates the fullItems()slice just to take its length.TestEngineFailuresnow asserts thatOpMove/OpCopyfrom a non-existent source return an error.Refactor / wire-format break⚠️
operation: split
Operation.From(source path for OpMove/OpCopy, json\"f\") fromOperation.Old(prior value across all kinds). Old was overloaded for move/copy as the source path, which prevented Reverse from restoring the displaced value when a copy/move had overwritten an existing destination.This is a JSON wire-format change. Patches serialised by v5.2.0 and earlier with the previous encoding (source path under
\"o\") will not round-trip cleanly. Generated*_deep.gofiles are unaffected — they never read those fields for Move/Copy ops, which fall through to the reflection path. Landing under v5 as this was completely broken.Docs
Set.Addadvances the HLC twice. Investigation showed neither order produces a single-tick path becauseCRDT.Editticks unconditionally for non-empty patches.Test plan
go build ./...,go vet ./...,staticcheck ./...go test ./...go test -race ./...(covers the newhlc.Clock.SetLatestconcurrency test)