queries to find a dependency of an intent

[insipx]

one commit adds a query for TopicCursor
another commit adds a query for finding a dependency based off a stored group intent

[insipx]

This PR is part of a stack of 11 bookmarks:

1. trunk()
2. insipx/dependency-resolution
3. insipx/causal-sort
4. insipx/resolution
5. insipx/network-backoff
6. insipx/ordered-combinator
7. insipx/test-resolution
8. insipx/test-ordering
9. insipx/add-published-in-epoch
10. insipx/sql-queries ← this PR
11. insipx/sync-conn
12. insipx/set-dependencies

---

Created with jj-stack

[claude]

Claude finished @insipx's task —— View job

---

Code Review

Review Date: November 21, 2025, 00:18 UTC

This PR adds SQL query functionality to find dependencies of intents, which is part of a larger stack implementing dependency resolution. Overall, the implementation is well-structured with good test coverage, but there are a few concerns to address.

---

Critical Issues

1. Complex SQL Join Logic May Have Edge Cases (xmtp_db/src/encrypted_store/group_intent.rs:515-585)

The find_dependant_commits function uses a complex inner join with epoch-based logic:

.inner_join(
    group_messages::table.on(group_messages::group_id
        .eq(dsl::group_id)
        .and(group_messages::kind.eq(GroupMessageKind::MembershipChange))
        .and(group_messages::published_in_epoch.eq(dsl::published_in_epoch - 1))),
)

Issues:

• The join condition published_in_epoch.eq(dsl::published_in_epoch - 1) assumes epochs increment by exactly 1, which may not be guaranteed
• If an intent is published in epoch 0, the query looks for messages in epoch -1, which could cause unexpected behavior
• No database-level constraint prevents intents from being published without a corresponding message in the prior epoch

Recommendation: Add validation to ensure published_in_epoch > 0 before querying, and consider handling the epoch 0 case explicitly.

---

2. Error Handling Logic Allows Silent Success (xmtp_db/src/encrypted_store/group_intent.rs:388-401)

In set_group_intent_published:

if rows_changed == 0 {
    let already_published = self.raw_query_read(|conn| {
        dsl::group_intents
            .filter(dsl::id.eq(intent_id))
            .first::<StoredGroupIntent>(conn)
    });

    if already_published.is_ok() {
        return Ok(());
    } else {
        return Err(NotFound::IntentForToPublish(intent_id).into());
    }
}

Issue: This treats any existing intent as "already published" even if it's in the wrong state (e.g., Error or Committed). This could mask bugs where the caller tries to publish an intent that should not be republished.

Recommendation: Check the actual state of the intent and only return Ok(()) if it's already in the Published state.

---

Performance Concerns

3. Potential N+1 Query Pattern (xmtp_db/src/encrypted_store/refresh_state.rs:377-418)

get_last_cursor_for_originators fetches states from the database, then iterates over missing originators and inserts them individually:

for missing_state in &missing_states {
    missing_state.store_or_ignore(self)?;
}

Issue: This could result in multiple individual INSERT statements if many originators are missing.

Recommendation: Use a batch insert with diesel::insert_into(...).values(&missing_states) to reduce database round trips.

---

4. Inefficient Chunking Strategy (xmtp_db/src/encrypted_store/refresh_state.rs:436-464)

The get_last_cursor_for_ids function uses a chunk size of 900 to stay under SQLite's bind parameter limit. However, the implementation loads all chunks sequentially and then flattens them:

.map(|partial_maps| {
    partial_maps
        .into_iter()
        .flat_map(|partial_map| partial_map.into_iter())
        .collect()
})

Issue: This allocates intermediate Vecs for each chunk, which could be memory-intensive for large datasets.

Recommendation: Consider using an iterator chain without intermediate collections, or use a more efficient merge strategy.

---

Code Quality Issues

5. Unclear Error Message (xmtp_db/src/encrypted_store/group_intent/error.rs:9-13)

#[error(
    "intent {} for group {group_id} has invalid dependencies={}. one message cannot have more than 1 dependency in same epoch",
    hex::encode(payload_hash),
    cursors
)]
MoreThanTwoDependencies {

Issue: The error name is MoreThanTwoDependencies, but the message says "more than 1 dependency." This is confusing.

Recommendation: Rename to MoreThanOneDependency for consistency with the error message.

---

6. Inconsistent NULL Handling (xmtp_db/src/encrypted_store/group_intent.rs:527)

.filter(dsl::published_in_epoch.is_not_null())

The query filters for non-NULL published_in_epoch, but later code assumes it's always present:

dsl::published_in_epoch.assume_not_null(),

Issue: While the assumption is valid given the filter, it's fragile. If the filter logic changes, this could panic.

Recommendation: Add a comment explaining why assume_not_null() is safe here, or use a more explicit NULL check.

---

Minor Issues

7. Proptest Regression Files (xmtp_db/proptest-regressions/encrypted_store/group_intent.txt)

The proptest regression file shows several failure cases have been encountered, including cases with no dependencies and cases with multiple dependencies in the same epoch.

Recommendation: Ensure these regression cases are well-understood and that the fixes are correctly implemented. Consider adding explicit test cases for these scenarios in addition to the proptest coverage.

---

8. Magic Number: CHUNK Size (xmtp_db/src/encrypted_store/refresh_state.rs:435)

const CHUNK: usize = 900;

Issue: The comment says "comfortably under SQLite's default 999-bind limit," but 900 is quite close to 999. If the query structure changes (e.g., adding more bind parameters per item), this could break.

Recommendation: Either document why 900 is safe (e.g., "each item uses exactly 1 bind parameter"), or use a more conservative value like 500.

---

Positive Observations

• Excellent test coverage with property-based testing using proptest
• Good error handling with custom error types
• Type safety with newtype wrappers like PayloadHash and IntentDependency
• Database transaction safety with proper use of Diesel's query builder
• Comprehensive edge case testing including empty inputs, chunking boundaries, and concurrent originators

---

Summary

The PR implements complex dependency resolution logic with good test coverage. The main concerns are around edge cases in the SQL join logic (epoch 0, missing dependencies) and some performance optimizations for batch operations. The error handling could be more precise in a few places.

Recommended actions:

1. Add validation for published_in_epoch > 0 in find_dependant_commits
2. Fix the "already published" check to verify the actual intent state
3. Optimize batch inserts in get_last_cursor_for_originators
4. Rename MoreThanTwoDependencies to MoreThanOneDependency

---

[codecov]

Codecov Report

❌ Patch coverage is 84.25721% with 71 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (insipx/add-published-in-epoch@cfd66ae). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
xmtp_db/src/encrypted_store/refresh_state.rs	85.10%	21 Missing ⚠️
xmtp_proto/src/types/cursor_list.rs	35.29%	11 Missing ⚠️
xmtp_proto/src/types/topic_cursor.rs	38.88%	11 Missing ⚠️
xmtp_db/src/encrypted_store/group_intent.rs	96.24%	8 Missing ⚠️
xmtp_db/src/encrypted_store/group_intent/types.rs	71.42%	6 Missing ⚠️
xmtp_db/src/encrypted_store/group_intent/error.rs	0.00%	5 Missing ⚠️
xmtp_proto/src/types/topic.rs	69.23%	4 Missing ⚠️
xmtp_api_d14n/src/protocol/extractors/data.rs	0.00%	3 Missing ⚠️
xmtp_db/src/encrypted_store/group_message.rs	0.00%	1 Missing ⚠️
xmtp_db/src/errors.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@                       Coverage Diff                        @@
##             insipx/add-published-in-epoch    #2819   +/-   ##
================================================================
  Coverage                                 ?   74.65%           
================================================================
  Files                                    ?      389           
  Lines                                    ?    50309           
  Branches                                 ?        0           
================================================================
  Hits                                     ?    37560           
  Misses                                   ?    12749           
  Partials                                 ?        0           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[macroscopeapp]

store(conn).unwrap() will panic on any failure. Consider returning a Result and using ? so errors propagate (and are logged/reported) instead of crashing.

🚀 Reply to ask Macroscope to explain or update this suggestion.

👍 Helpful? React to give us feedback.