fix(shadows): ack map-entry removals as null in reported and desired#131
Merged
Conversation
Applying a map delta with Patch::Unset removed the entry locally but the
acknowledgment could not express the removal: ReportedLinearMap /
ReportedHashMap held plain K -> R entries, so into_partial_reported
silently skipped unset keys (ack sent {}), and desired_cleanup ignored
them. The cloud document kept the stale entry in reported and the
"unset" tombstone in desired.
Consequences fixed:
- Re-adding an entry identical to the stale reported value produced NO
delta (desired == reported field-for-field), so the device never saw
the re-add. For wifi known_networks: remove network, re-add the same
network -> device never reconnects.
- The desired tombstone kept a permanent pending delta, reprocessed as
a no-op on every boot/GET.
Fix: reported map entries are now Patch<R>. Patch::Set serializes
transparently (present entries keep the exact same wire format) and
Patch::Unset serializes as null, which AWS treats as key deletion. The
unset arm of into_partial_reported emits Patch::Unset, and
desired_cleanup mirrors the removal so both sections are nulled in the
single apply_delta_and_ack update. ReportedDiff always retains Unset
entries (a removal must reach the cloud regardless of prior report).
Both heapless::LinearMap and std HashMap impls updated; regression
tests assert the serialized ack is {"k":null} on both sides.
KennethKnudsen97
approved these changes
Jun 11, 2026
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
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
Applying a map delta with
Patch::Unsetremoved the entry locally but the acknowledgment could not express the removal:ReportedLinearMap/ReportedHashMapheld plainK -> Rentries, sointo_partial_reportedsilently skipped unset keys (the ack sent{}), anddesired_cleanupignored them. After the ack, the cloud document kept the stale entry inreportedand the"unset"tombstone indesired.Empirically (replicating
apply_delta_and_ack's two outputs before this change):Consequences fixed
wifi.known_networks: remove a network, later re-add the same network - the device never reconnects, with no error anywhere."unset"tombstone in desired keeps the delta document non-empty forever; the device reprocesses it as a no-op on every boot/GET.Fix
Reported map entries are now
Patch<R>:Patch::Setserializes transparently, so present entries keep the exact same wire format - no change for existing consumers' shadow documents.Patch::Unsetserializes asnull, which AWS treats as key deletion. The unset arm ofinto_partial_reportednow emits it, anddesired_cleanupmirrors the removal, so the singleapply_delta_and_ackupdate nulls the key in both sections:ReportedDiffalways retainsUnsetentries (a removal must reach the cloud regardless of what was previously reported).Both the heapless
LinearMapand stdHashMapimplementations are updated symmetrically.Compatibility
Set).SCHEMA_HASH/ KV persistence: untouched - no flash migration for KV consumers.ReportedLinearMap.0/ReportedHashMap.0now holdPatch<R>values; theFrom<Map<K, R>>impls wrap entries inPatch::Set, so construction sites keep compiling.Tests
test_linear_map_unset_acks_null_in_reported_and_desired/test_hash_map_unset_acks_null_in_reported_and_desired- serialized ack payloads are{"k":null}on both sides.test_reported_linear_map_diff_retains_unset- diff keeps removals, still prunes unchangedSetentries.test_map_unset_ack_nulls_reported_and_desired(shadow level) - fullparse_delta->apply_delta-> ack pipeline on ashadow_rootfixture.Changelog
Fixed
Patch::Unset) are now acknowledged asnullin bothreportedanddesired, so AWS deletes the key instead of keeping a stale entry + tombstone (which silently swallowed any later re-add of an identical entry)