fix(sdk): merge re-fetched nodes into the client store instead of overwriting

Author: ogenstad

changelog/+store-merge-behaviour.changed.md

@@ -0,0 +1 @@
+The client store merge fix comes with four observable behaviour changes. First, the object returned by `get`, `filters` or `all` is a per-query snapshot and is no longer the same Python object as the store entry: `client.get(id) is client.store.get(id)` was previously true and now is false, although the two still compare equal (`==`). Second, the store hands out living objects: `store.get()` returns the same object across calls, and a later query that re-fetches the node updates that object in place. Third, the store is now timestamp-coherent per branch: the first population stamps the branch cache as live or as one `at` point in time, queries at the same timestamp use the store normally (a fully historical script gets complete store functionality), and a query at a mismatching timestamp skips the store with a warning instead of silently blending or overwriting data from a different point in time (which is what pre-1.23.0 versions did). Fourth, a successful `save()`, `create()` or `update()` now resets the node's in-memory mutation tracking: the persisted values count as server state, so later fetches of the same node refresh those fields in the store instead of treating the long-saved edit as a pending local change forever.

changelog/413.fixed.md

@@ -0,0 +1 @@
+Fixed the client store silently losing data when the same node was fetched more than once. Previously the latest query fully replaced the stored node, so a shallow re-fetch (for example a node returned as a related node of another query) dropped attributes and relationships that an earlier, deeper query had loaded, and left duplicate entries in the store. The store now keeps one object per node UUID and merges each fetch into it field by field: fields carried by the new fetch overwrite the stored ones (even to empty or `None`), fields the fetch did not request keep their stored value, and local unsaved edits always win over a re-fetch. The previous replace behaviour remains available per query and per `store.set()` call with `merge=False`, or globally with the new `store_merge` configuration option (`INFRAHUB_STORE_MERGE`).

dev/specs/ihs-138-store-merge/decisions.md

@@ -43,7 +43,10 @@ Add a `save()` test: `save()` calls `store.set(self)`, so `self` merges into the
 canonical copy while the caller keeps `self` - confirm no field is revived that
 the save did not touch.
 
-**Decision:** ____________________
+**Decision: ACCEPTED (a).** Query methods return the per-query object; the store
+holds the merged canonical copy. Guiding rule agreed with the reporter: *"queries
+hand you what you asked for; the store remembers the union of everything it has
+seen."*
 
 ---
 
@@ -70,7 +73,8 @@ Is that the intended model?
 **Cost.** New, documented behaviour. Covered by the section 6 "behaviour change
 must be clear" requirement.
 
-**Decision:** ____________________
+**Decision: ACCEPTED.** The store keeps one always-current, merged object per node;
+`store.get()` references reflect later fetches. Unsaved local edits are protected.
 
 ---
 
@@ -100,7 +104,14 @@ public `store.set(node=...)` default to?
 `Config.store_merge` (default merge); `set()` defaults to replace regardless. That
 is intentional and should be stated in the `set()` docstring.
 
-**Decision:** ____________________
+**Decision: OVERRIDE -> (b) merge by default, uniformly.** The recommendation above
+was not taken. Agreed model: *anything that lands in the store gets merged*, with no
+special-casing by entry point - `store.set()` merges just like the query path. This
+is the simpler, single-rule mental model. Consequence: **replace is opt-in only** -
+the sole way to store a node verbatim / drop previously cached data is
+`store.set(node, merge=False)` or the `store_merge=False` config opt-out. The public
+`store.set()` docstring must state that it merges by default and how to force
+replace.
 
 ---
 
@@ -126,7 +137,8 @@ is intentional and should be stated in the `set()` docstring.
   consistency, and we are willing to accept the enum/bool split (or make the
   per-call arg accept the enum too).
 
-**Decision:** ____________________
+**Decision: ACCEPTED (a).** Bool `store_merge` with the full description. Revisit only
+if the description proves insufficient in review.
 
 ---
 
@@ -150,7 +162,8 @@ types.**
 - Internal only (not public API), so low stakes - but the uniform accessor removes
   per-type special-casing in the merge.
 
-**Decision:** ____________________
+**Decision: ACCEPTED.** `is_fetched` on `Attribute` and `RelatedNode`; uniform
+`is_fetched` accessor on `RelationshipManager` aliasing its existing `initialized`.
 
 ---
 
@@ -168,20 +181,131 @@ node's relationship peers from the store, or only replace the node's own entry?
 - Peers fetched in the same query go through their own `set()` calls and follow the
   same `merge` flag independently. No special cascade needed.
 
-**Decision:** ____________________
+**Decision: ACCEPTED.** `merge=False` replaces the node's own entry only; peers are
+handled independently.
+
+---
+
+## D7 - Mutation-flag lifecycle (added during implementation, 2026-07-04)
+
+**Question.** D2 protects "unsaved local edits" via the mutation flags
+(`value_has_been_mutated`, `_peer_has_been_mutated`, `_has_update`). The code review
+showed those flags were sticky - never reset after a successful save - so a saved
+edit would block merge refreshes of that field forever, and the store would serve
+the stale saved value even after the server changed. What is the flag lifecycle?
+
+**Decision: reset on save, propagate on merge.**
+
+- A successful `create()`/`update()`/`save()` resets all three flag types
+  (`_reset_mutation_tracking()` at the end of `_process_mutation_result`): the
+  persisted values count as server state from then on.
+- The merge propagates the markers from an unsaved incoming copy instead of
+  clearing them, so an unsaved edit merged into the store (manual `store.set`)
+  keeps its will-be-saved status and is still sent by the store copy's next save.
+
+**Why.** The two halves depend on each other: propagation is only safe because
+saving resets the flags (a freshly saved copy no longer reads as "pending"), and
+resetting is what makes "local edits win" mean *unsaved* edits, which is what D2
+intended. Consequence, listed in the changelog: mutation tracking now resets after
+a successful mutation, so a second `update()` no longer re-sends relationship edits
+that were already persisted.
+
+---
+
+## D8 - Same-peer identity fields gate on presence (refines the merge rule)
+
+**Question.** The plan said cardinality-one peer identity "always refreshes" when
+the relationship was fetched. A payload carrying only `node { id }` for the *same*
+peer would then null previously fetched `hfid`/`display_label`/`typename`/`kind`
+and drop the cached `_peer` - the silent-loss class this feature exists to prevent
+(reachable via `from_graphql` on custom payloads + `store.set`, not via
+SDK-generated queries).
+
+**Decision: split by peer change.** A changed peer (different id, including cleared
+to none) takes the full incoming identity, so moves and move-to-root behave as
+specified. The same peer only refreshes the descriptive identity fields the
+incoming payload actually carried - consistent with how attribute properties and
+edge properties already merge.
+
+---
+
+## D9 - Timestamp-coherent store (supersedes grill item 3's mechanism, 2026-07-04)
+
+**Question.** Grill item 3 decided historical (`at`) reads must not blend into the
+live cache. The first implementation made `at` queries skip the store by default,
+with an explicit `populate_store=True` opt-in that forced replace. That required
+widening `populate_store` from `bool = True` to `bool | None = None` (to detect an
+explicit `True`), left `at` + `prefetch_relationships` without `.peer` resolution,
+and - because the opt-in forced replace - quietly revived the IHS-138 bug for
+fully-historical scripts. Was the signature change actually needed?
+
+**Decision: no - make the store timestamp-coherent instead.** The store holds one
+timestamp context per branch: live data, or one `at` instant, stamped by the first
+population. Same-context queries use the store normally, so a script running all
+its queries at one `at` gets full store functionality (merge, `.peer`, hfid
+lookups). A mismatching population (live vs historical, or two different instants)
+emits a warning and skips the store for that query; the query itself still returns
+its results.
+
+**Why.**
+
+- The incoherence grill item 3 feared comes only from *mixing* timestamps; a
+  consistent-`at` session is exactly as coherent as a live one and deserves the
+  same cache behaviour, including the merge semantics this whole feature adds.
+- `populate_store: bool = True` is restored - no public signature change, no `None`
+  sentinel in a boolean parameter.
+- Strictly safer than both alternatives considered: pre-1.23 silently overwrote
+  live entries with historical data; skip-by-default silently degraded historical
+  scripts. Warn-and-skip makes the mismatch visible without breaking mixed scripts
+  (which are safer than they were on 1.22, not worse).
+- Mismatch policy is warn + skip rather than raise: raising would break scripts
+  that mix `at` and live queries, which worked (subtly wrong) before, and this
+  ships as a semver-minor bug fix.
+- Documented footgun: recompute a relative timestamp per call and every query
+  after the first trips the warning - compute `at` once, or use one
+  `client.clone()` per timestamp (the context is per client store, per branch).
 
 ---
 
 ## Summary table
 
-| ID | Decision | Recommendation |
-|----|----------|----------------|
-| D1 | Returned vs stored object | (a) return per-query object; store is canonical |
-| D2 | In-place mutation model | Accept, documented, protect local edits |
-| D3 | `store.set()` default | (a) replace by default; queries merge by default |
-| D4 | Config option name/type | (a) bool `store_merge` + full description |
-| D5 | Presence-flag name | `is_fetched`, uniform accessor on all three types |
+| ID | Decision | Outcome |
+|----|----------|---------|
+| D1 | Returned vs stored object | Return per-query object; store holds the merged canonical copy |
+| D2 | In-place mutation model | Accept; one always-current merged object per node; protect local edits |
+| D3 | `store.set()` default | **Merge by default, uniformly** (override); replace is opt-in via `merge=False` |
+| D4 | Config option name/type | Bool `store_merge` + full description |
+| D5 | Presence-flag name | `is_fetched`, uniform accessor on all three field types |
 | D6 | `merge=False` scope | Node entry only; peers handled independently |
-
-Once D1-D3 are settled, Stage 2 onward is unblocked. D4-D6 can be confirmed during
-implementation without reblocking.
+| D7 | Mutation-flag lifecycle | Reset on successful save; merge propagates pending markers (2026-07-04) |
+| D8 | Same-peer identity fields | Presence-gated for the same peer; full refresh on peer change (2026-07-04) |
+| D9 | `at` and the store | Timestamp-coherent per branch; mismatching populations warn + skip (2026-07-04) |
+
+**Guiding rule:** queries hand you what you asked for; the store remembers the union
+of everything it has seen. Replace happens only when explicitly requested
+(`merge=False` / `store_merge=False`).
+
+All decisions are settled and implemented (D1-D6 as planned; D7/D8/D9 added during
+the 2026-07-04 code-review hardening - see plan section 13 for the full list of
+implementation outcomes, including the performance constraints on `store.set()`).
+The remaining external gate is the 1.23.0 pre-release run of the Ansible collection
+and `infrahubctl` integration suites.
+
+## Grill refinements (2026-07-02)
+
+Stress-testing the plan (see plan section 12) added these, all accepted:
+
+- **Merge scope** also covers node-level scalars (`display_label`, `typename`) and
+  merges attributes/properties field-by-field (not object-swap).
+- **Kind change is a documented exception to D2:** if `typename` differs for the same
+  uuid (a `ConvertObjectType` migration), the store entry is *replaced* wholesale, not
+  mutated in place - merging across schemas is incoherent.
+- **`at` (time-travel) queries skip store population by default** (behaviour change
+  from today; must be in the migration note). Explicit `populate_store=True` + `at`
+  replaces rather than blends. *Superseded by D9:* the shipped mechanism is a
+  timestamp-coherent store (one `at` context per branch, warn + skip on mismatch),
+  which keeps the same goal - never blend timestamps - without the `populate_store`
+  signature change.
+- **Release gate:** Ansible collection + `infrahubctl` integration suites run against
+  the 1.23.0 pre-release; migration note enumerates the D1 identity change and the
+  `at` change with before/after.