Skip to content

[backport] Guard against disengaged optional in InTrackedMutationRecords (#1120) - #1388

Open
JasonLi314 wants to merge 4 commits into
valkey-io:1.1from
JasonLi314:backport-1120-to-1.1
Open

JasonLi314 wants to merge 4 commits into
valkey-io:1.1from
JasonLi314:backport-1120-to-1.1

Conversation

@JasonLi314

@JasonLi314 JasonLi314 commented Sep 14, 2026

Copy link
Copy Markdown
Collaborator

cherry picking commit 5d95e4c7 to branch 1.1 that added reader guard to optional pending index updates.

…-io#1120)

ConsumeTrackedMutatedAttribute may leave a tracked_mutated_records_
entry in place with attributes = std::nullopt (the "else" branch at
index_schema.cc:1997-2008 moves the attribute map out and disengages the
optional but does not erase the entry). A subsequent call to
InTrackedMutationRecords on the same key would then dereference the
empty optional via `attributes->find(...)`, which is UB. Under ASAN this
surfaced as a SIGSEGV from absl's SwissTable iterator-generation guard
during the backfill scan path, observed in CI on a fulltext test
(test_proximity_predicate).

The fix is a one-line has_value() check; the semantically correct return
is false (no pending mutation for this key/identifier).

Adds InTrackedMutationRecordsAfterConsumeNoCrash to document and lock in
the contract. The test exercises the exact code path that crashed in CI
but does not, by itself, reliably trigger the crash on the unfixed code:
after a single track/consume cycle the moved-from flat_hash_map inside
the optional has its generation_ptr_ reset to absl's static
EmptyGeneration() sentinel, so the dereference happens to read valid
memory. The production crash needed a more complex sequence (multi-
mutation cycles plus concurrent backfill). The test still asserts the
correct behavior and catches future regressions in the has_value() check
or the consume/disengage sequence.

Signed-off-by: Allen Samuels <allenss@amazon.com>
Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
(cherry picked from commit 5d95e4c)
Signed-off-by: Jason Li <47095666+JasonLi314@users.noreply.github.com>
Signed-off-by: Jason Li <47095666+JasonLi314@users.noreply.github.com>
@JasonLi314
JasonLi314 marked this pull request as ready for review September 14, 2026 04:01
@github-actions

Copy link
Copy Markdown

Reviewers for this PR

  • First Pass Reviewer: @VoletiRam — Please do your best to do a detailed review on the PR and get a response on your feedback. Once the first pass is done, notify the maintainer assigned to this PR to follow up on the final review and getting the PR merged. You can reach out to the people owning the relevant code paths for more help on the review.
  • Maintainer Reviewer: @yairgott — Once the first review is done, please follow up with a final review and help to merge the change in.

Assigned automatically to the least-assigned members of the reviewer pools in .github/reviewer-pools.json. Use /reviewer or /remove-reviewer to adjust.

@greptile-apps

greptile-apps Bot commented Sep 14, 2026

Copy link
Copy Markdown

Greptile Summary

This change prevents reads of tracked mutation records from dereferencing a consumed attribute map and adds regression coverage for the consume-then-read sequence. The test documentation now uses a stable function reference rather than stale source-line numbers.

Confidence Score: 5/5

Safe to merge.

No outstanding findings remain. The previous documentation finding was resolved without an explanatory reply; JasonLi314 manually resolved it without explanation. The current test comment no longer cites the stale source-line range.

Reviews (3): Last reviewed commit: "clang format" | Re-trigger Greptile

Comment thread testing/index_schema_test.cc Outdated
JasonLi314 and others added 2 commits September 14, 2026 14:35
…h after cherry pick

Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Signed-off-by: JasonLi314 <47095666+JasonLi314@users.noreply.github.com>
Signed-off-by: Jason Li <47095666+JasonLi314@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants