feat(#591): post-remap cache invalidation#621
Merged
Conversation
After #589/#590's apply pass rewrites compound handles in the columns of side-relations, any per-relation cache whose hash keys include those handle column values is stale. #591 adds a dedicated post-pass that the rotation helper (#550 Option C) calls once after a successful remap completes: wl_handle_remap_invalidate_side_relation_caches(sess, &out_rels); Walks sess->rels[], filters by the __compound_ name prefix (#580 convention reused from #590), and for each match: 1. col_session_invalidate_arrangements(&sess->base, rel->name) -- the existing helper (arrangement.c) already covers the full, filtered, and differential arrangement caches whose hash keys are row-value derived. No new cache surface introduced. 2. free(rel->dedup_slots); set NULL/0/0. IMPORTANT: dedup_slots is heap-allocated (eval.c lazy-builds via calloc); the issue body says "set dedup_slots = NULL" but doing only that leaks dedup_cap * 8 bytes per remapped relation per rotation. The pre-implementation critic flagged this exactly; we free first. Out of scope (documented in the header for #598 / follow-up): - mat_cache: content-keyed; stale results survive but miss on re-hash. Whether to clear is left to the rotation helper. - sarr_entries: side-relations are not expected to carry sorted arrangements today, so the gap in col_session_invalidate_arrangements is harmless until a follow-up extends the helper itself. Multiplicity (Z-set): timestamps[] is untouched. The pass writes only per-cache fields and dedup_slots/cap/count. Side-by-side with #590's apply pass (which also leaves timestamps untouched) this preserves the per-row multiplicity invariant by construction. No new tests in this commit; the post-remap acceptance harness lands in the next commit. Existing 169-test suite stays green.
Three cases pin the contract that
wl_handle_remap_invalidate_side_relation_caches honours:
1. test_invalidate_arrangement_and_dedup -- live session, build a
side-relation via wl_compound_side_ensure, append rows,
populate the column-0 arrangement so arr_entries[i].arr.
indexed_rows == nrows, plant a fake dedup_slots/cap/count, run
the invalidation pass, assert (a) indexed_rows reset to 0,
(b) dedup_slots == NULL && dedup_cap == 0 && dedup_count == 0,
(c) out_rels_invalidated == 1. Calls the helper a second
time to assert idempotency: free(NULL) is legal, the
arrangement helper is no-op safe.
2. test_invalidate_skips_non_side_relations -- the make_session
fixture registers EDB rel "a" and IDB rel "r" (no
__compound_ prefix). Plant a dedup_slots on EDB "a", run
the helper, verify (a) out_rels_invalidated == 0, (b) "a"'s
dedup_slots / cap / count are byte-identical to the plant.
Tear-down frees the plant before session destroy so the
session doesn't double-free a now-disowned pointer.
3. test_invalidate_einval -- NULL session returns EINVAL with
out_rels_invalidated zeroed.
ASAN+UBSAN clean on build-san; TSan clean on build-tsan. The
dedup_slots-leak failure mode the pre-review critic flagged
(plain "dedup_slots = NULL" without free) would surface as a
LeakSanitizer error in case 1; this commit prevents the
regression.
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
2 atomic commits closing #591 (invalidate side-relation caches after compound-handle remap).
`feat(Rebuild side-relations with new handles #591):` `wl_handle_remap_invalidate_side_relation_caches(sess, *out_rels)` extends `handle_remap_apply_side.{c,h}`. Walks `sess->rels[]`, filters `_compound` prefix, and per match calls `col_session_invalidate_arrangements` (existing helper at `arrangement.c:403` covers full/filtered/diff arr caches) plus `free(rel->dedup_slots); rel->dedup_slots = NULL; rel->dedup_cap = 0; rel->dedup_count = 0;`. The free-before-NULL closes the leak the pre-implementation critic flagged — issue body's literal "dedup_slots = NULL" would have leaked `dedup_cap * 8` bytes per remapped relation per rotation.
Out of scope (header-doc'd for Verify no regressions in existing tests #598 follow-up): `mat_cache` (content-keyed; rotation helper decides), `sarr_entries` (no side-relation usage today; extend the existing helper later).
Multiplicity (Z-set) preserved by construction: `timestamps[]` is never read or written.
`test(Rebuild side-relations with new handles #591):` Three cases — populated arrangement + planted dedup roundtrip with idempotency check, non-side-relation skip (planted dedup on EDB "a" survives), NULL session → EINVAL.
Multi-agent flow
Test plan
Closes #591.