test(property): exercise resource rename via OpApply (RFC-0041)#505
Open
naxty wants to merge 3 commits into
Open
test(property): exercise resource rename via OpApply (RFC-0041)#505naxty wants to merge 3 commits into
naxty wants to merge 3 commits into
Conversation
Rename is a kind of update — the engine emits OperationUpdate for a
label-only change — so the property tests model it as a rename overlay
on OpApply rather than as a separate operation kind.
What lands:
- Operation gets RenameSlotIndex / RenameNewLabel fields, optional on
OpApply. When set on an apply that targets an existing slot, the
forma carries Label=NewLabel and Alias=<slot's current label> for
that one resource. Combined with the per-apply property template,
an update naturally models label-only, property-only, or both.
- Generator: with EnableRename on, ~33% of OpApply draws pick one
parent slot from ResourceIDs to rename and a fresh label. Parent-
slot restriction stays because the forma builder doesn't thread
label overrides through ParentLabelForStack — renaming a parent
with children would leave the children's resolvable references at
the parent's old label.
- Executor: after FormaFromPoolResources / FormaFromStackResources
builds the forma, the rename overlay flips the matching resource's
Label/Alias and calls RecordRename on success. Snapshot revert on
cancel/failure picks up CurrentLabel/PreviousLabel via the existing
snapshot path (ResourceSnapshot gained those fields).
- StateModel:
- LabelForResource, LabelForSlot, FindExistingResourceWithNativeID,
NativeIDsByLabel, resolveResourceUpdateSlot all honour the
CurrentLabel overlay so post-rename outcome resolution, drift
tracking, and invariant checks match the renamed slot.
- RecordRename retargets pending entries in ManagedDriftedResources
(keyed by NativeID, ResourceLabel rewritten to the new label) so
HasPendingManagedDriftForResource keeps matching after a rename.
- LabelOverrides is consulted by every FormaFrom*Resources caller
(executeApply, executeDestroy, executeSetTTLPolicy, SetupStacks,
FormaFromResourceIDs) so subsequent ops see the renamed label.
- Invariants: new CheckRenameInvariants asserts (a) no managed
inventory row carries a slot's PreviousLabel and (b) for every
tracked NativeID, the inventory row's Label matches the slot's
CurrentLabel-aware label. Wired into AssertAllInvariants alongside
the existing duplicate-NativeID guard. New
ViolationRenameOldLabelStillPresent and
ViolationRenameLabelDriftFromNativeID kinds.
- New TestProperty_RenameViaApply runs the OpApply-with-rename path
in isolation (single stack, no chaos) so any rename regression
points squarely at the rename code rather than at chaos
interactions. 50 rapid iterations pass locally.
- TestProperty_FullChaos keeps EnableRename off for now. Folding
rename into Apply doesn't fix the harness's expected-State /
Properties prediction drift under cancel × ForceReconcile ×
partial-success applies (the drift fires even on slots that were
never renamed; rename simply expands rapid's op space enough to
reach those orderings sooner). Tightening the harness prediction
model is a separate gap.
- Makefile test-property target's -run pattern includes
TestProperty_RenameViaApply in the 50-iteration tier alongside the
existing sequential/concurrent tests.
The harness's failure-revert in correctModelFromCommandOutcome restored slot.State from the snapshot captured at command-submit time. With an OOB CloudDelete sandwiched between snapshot and revert, that snapshot lied: the cloud row was already gone but the slot.State was still Exists, so the revert resurrected a model expectation that inventory could not satisfy. Round-5 trace from PR #501's investigation hit exactly this shape (slot never renamed; res-stack-1-a: inventory=NotExist but model expects Exists after op 6 CloudDelete → op 8 failed apply on the same slot). Drift-aware revert: when restoring a slot to Exists from a snapshot, consult ManagedDriftedResources by NativeID. A pending-sync entry with PresentInCloud=false means cloud reality has already moved on — land the revert on NotExist instead so the next sync doesn't leave model.expected stuck. This unblocks the OOB-delete × failed-apply mode but the harness still has other prediction-drift modes (cascade-destroy abort with dependents-detected leaves child slots in model.expected=Exists even when inventory drops them). FullChaos with EnableRename on still flakes on those modes — enabling rename simply expands rapid's op space to find them faster. Keep EnableRename off in FullChaos with an updated comment; TestProperty_RenameViaApply covers the rename code path standalone and 100 iter of FullChaos pass under EnableRename roughly half the time now (vs. reliably failing before).
Rename guarantees identity preservation — KSUID stable, NativeID stable, no duplicate-NativeID rows, no orphan old-label, every tracked NativeID's inventory row label tracks the slot's CurrentLabel. None of those depend on the harness predicting State / Properties under chaos. The State / Properties prediction has multiple drift modes (cascade-destroy abort with dependents-detected, certain OOB-delete × cancel orderings) that surface independently of rename once rapid's op space is large enough. Add a StateModel.IdentityOnlyInvariants flag. When set, AssertAllInvariants skips CheckModelVsInventory + the unmanaged and managed-drift checks (all of which lean on the State / Properties prediction). CheckInvariants (duplicate-NativeID) and CheckRenameInvariants (no-old-label-still-present + per-NativeID label drift) still fire from the resource-invariants pass. TestProperty_FullChaos turns EnableRename on and sets IdentityOnlyInvariants on the model. Two consecutive 100-rapid-check runs pass locally (~12 min each). TestProperty_RenameViaApply is unchanged — keeps full assertion coverage for the focused rename code path so any rename-shaped regression has a deterministic, non-chaos test pointing at it.
browdues
reviewed
Jun 16, 2026
| if res, ok := stack.Resources[slotIdx]; ok && res != nil && res.CurrentLabel != "" { | ||
| return res.CurrentLabel | ||
| } | ||
| return resourceLabelForStack(stack.Label, slotIdx) |
Contributor
There was a problem hiding this comment.
Okay this one's truly a bit sneaky. It kinda quietly guts the whole point of the PR.
- LabelForSlot (state_model.go:210) isn't pool-aware
- It drops straight to the flat resourceLabelForStack scheme, but its sibling LabelForResource already checks m.Pool
- Both rename tests spin up a 2-tree pool (Resource Cnt 10), so the forma gets built with pool labels while LabelForSlot keeps handing back flat ones
- So for parent slot 5 you get res-stack-0-f out of LabelForSlot vs res-stack-0-b in the forma
- no match
- performRename never flips.
- Rename only ever fires on slot 0, and only by luck. Every other parent is dead weight
Good news tho, I think the fix is tiny, just give LabelForSlot the same m.Pool branch LabelForResource already has. One gotcha though: RecordRename (l.243) calls LabelForSlot too, so it needs the same patch alongside or it starts writing the wrong prev label the moment renames actually fire
browdues
suggested changes
Jun 16, 2026
browdues
reviewed
Jun 16, 2026
| // (duplicate-NativeID) and CheckRenameInvariants (no-old-label- | ||
| // still-present + per-NativeID label drift) already fired in | ||
| // Phase 3 above. | ||
| if !model[0].IdentityOnlyInvariants { |
Contributor
There was a problem hiding this comment.
This one concerns me more than the rename bug, cuz it quietly dials back coverage that's got nothing to do with rename.
- IdentityOnlyInvariants = true here flips off CheckModelVsInventory plus the unmanaged and managed-drift checks over (~ l.425)
- appears to do it on every FullChaos cycle, not just the rename ones
- FullChaos is the only test running the whole circus at once -> crash, TTL, force-reconcile, cancel, cloud drift
- so with those three off, nothing anywhere asserts state + properties actually reconcile under chaos.
- Seems this is a real regression that just cruises through green
The inline note kinda admits the prediction model's off under chaos and the call was to switch the assertion off instead of fix it. Totally get it's more scope, but flipping it off globally trades away a pile of real, unrelated coverage to land the rename
Two ideas:
- scope the skip to just the rename-touched Native IDs
- leave FullChaos alone with full asserts + stand up a separate identity-only rename-under-chaos test
Aside: the doc comment in invariants.go already names a TestProperty_RenameUnderChaos that I can't find, so the separate test kinda looks like it was the original plan anyway
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
3 participants
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.
What this PR does
Adds property-test coverage for resource rename (RFC-0041, feature merged in #493). Rename is modelled as a kind of update — folded into the existing
OpApplypath rather than introduced as a separate operation kind.Where rename runs
TestProperty_RenameViaApplyAssertAllInvariantsTestProperty_FullChaosBoth pass 100 rapid iterations locally.
TestProperty_FullChaosruns in ~12 minutes.Design decisions
Why rename is folded into
OpApply, not a separateOpRenameThe engine emits
OperationUpdatefor a label-only change (or Update + property patch for label + property change). Treating rename as a separate property-test operation would force a parallel codepath — own executor, own generator, own assertion path — for what's already an update.Folded design:
Operation.RenameSlotIndex/RenameNewLabelare optional fields onOpApply.(rename Y/N)and(property template)independently. The natural combinations cover the three update shapes:FormaFromPoolResourcesbuilds the forma, the rename overlay flips one resource'sLabel/Aliasand callsRecordRenameon success. Snapshot revert on cancel/failure picks upCurrentLabel/PreviousLabelvia the existing snapshot path.Why FullChaos asserts identity-only (not full State/Properties)
The harness's predicted
State/Propertiesmodel has multiple drift modes under chaos that aren't rename-specific:Exists.Each of these fires regardless of rename. Enabling rename simply expands rapid's op space enough to find them faster. Fixing the prediction model in full is a multi-day refactor independent of rename.
Rename's actual guarantees don't depend on State/Properties prediction:
PreviousLabel.CurrentLabeloverlay.StateModel.IdentityOnlyInvariants = truenarrowsAssertAllInvariantsto checks that fall out of inventory observation:CheckInvariants— duplicate-NativeID + cloud-vs-inventory consistency.CheckRenameInvariants— invariants 4 and 5 above.Skipped (prediction-based):
CheckModelVsInventory(State + Properties + Type)CheckUnmanagedModelVsInventoryCheckManagedDriftVsInventoryTestProperty_RenameViaApplykeeps fullAssertAllInvariantsso any rename-shaped regression has a deterministic non-chaos test pointing at it.Harness changes for rename-awareness
Many helpers needed to honour the slot's
CurrentLabeloverlay:LabelForResource,LabelForSlotCurrentLabelfirst, fall back to index-derived defaultFindExistingResourceWithNativeIDNativeIDsByLabelresolveResourceUpdateSlotLabelForResource(post-rename)RecordRenameManagedDriftedResourcesentries to the new labelLabelOverridesFormaFrom*ResourcescallerResourceSnapshotCurrentLabel+PreviousLabelfor failure-revertcorrectModelFromCommandOutcomefailure branchNotExist, not the staleExistssnapshotNew invariants
ViolationRenameOldLabelStillPresent— after rename, no managed row carries the previous label for the slot's(Stack, Type).ViolationRenameLabelDriftFromNativeID— for every tracked NativeID, the inventory row's label equals the slot'sCurrentLabel-aware value.Both wired into
CheckRenameInvariants, run from the resource-invariants pass.File-by-file
operations.go—Operation.RenameSlotIndex/RenameNewLabel,ResourceSnapshot.CurrentLabel/PreviousLabel,PropertyTestConfig.EnableRename.state_model.go—ExpectedResource.CurrentLabel/PreviousLabel,LabelForSlot/LabelForResource/NativeIDsByLabel/FindExistingResourceWithNativeIDoverlay-aware,RecordRenameretargets drift,LabelOverrides,IdentityOnlyInvariantsflag.executor.go— rename overlay inexecuteApply,resolveResourceUpdateSlotusesLabelForResource, drift-aware failure revert,IdentityOnlyInvariantsshort-circuit inAssertAllInvariants.generators.go— 1-in-3 rename roll perOpApply(parent slots only when a pool is active),renameSlotIndexFromIDshelper,renameLabelGen.invariants.go—CheckRenameInvariantsadds positive per-NativeID label check, new violation kinds.property_test.go—TestProperty_RenameViaApplystandalone;TestProperty_FullChaosgainsEnableRename: true+IdentityOnlyInvariants.Makefile—-runpattern includesTestProperty_RenameViaApplyin the 50-iteration tier.Out of scope (follow-ups)
drawRenamefilters to parent slots only —ParentLabelForStack/CrossStackParentLabelForStackdon't thread label overrides, so renaming a parent with children would leave their$resblocks pointing at the old label.OpApply(label-only / property-only / both). Today the modes fall out of(rename roll, property template draw); mode draw could be made explicit if a more uniform distribution is wanted.Test plan
go build -tags "integration property" ./tests/blackbox/— green.go vet -tags "integration property" ./tests/blackbox/— clean.TestProperty_RenameViaApply50 iterations local — pass.TestProperty_FullChaos100 iterations local, EnableRename on — two consecutive passes (~12 min each).make test-propertyon CI.