[WIP] ncmec: store checkpoint occasionally when start, end diff is one second #1731

prenner · 2025-01-09T16:07:14Z

Summary

sometimes ncmec fails to make progress after hitting a second w/ a large number of results: #1679. when that happens (diff of end and start is a second and we have lots of data), store checkpoints occasionally via a next pointer

Test Plan

confirm that next pointer is used when fetching hashes locally

prenner · 2025-01-09T16:12:26Z

python-threatexchange/threatexchange/exchanges/impl/ncmec_api.py

@@ -39,14 +39,16 @@ class NCMECCheckpoint(

    # The biggest value of "to", and the next "from"
    get_entries_max_ts: int
+    next_fetch: str
+    last_fetch_time: int


this is unsued currently but I know we talked about invalidating checkpoints older than 24hr.. happy to add that! just wanted to get confirmation on the checkpointing piece that this is on the right track

Dcallies

Overall looking good, thanks for making this change, and I think it will help a lot!

I am slightly suspicious that the paging URLs can go sour (e.g. I have noticed that NCMEC API tends to throw exceptions near the very end of the paging list that make me think that they are invaliding), so I think adding the time-based invalidation logic is a requirement.

As part of your test plan, can you also attempt fetching past an extremely dense time segment in the NCMEC API and confirm the behavior works as expected?

Dcallies · 2025-01-13T13:34:01Z

python-threatexchange/threatexchange/exchanges/tests/test_state_compatibility.py

+    class NCMECCheckpointWithoutNext(FetchCheckpointBase):
+        """
+        0.99.x => 1.0.0
+
+        get_entries_max_ts: int =>
+            get_entries_max_ts: int
+            next_fetch: str
+            last_fetch_time: int
+        """
+
+        get_entries_max_ts: int


Thanks for adding this test case!

Dcallies · 2025-01-13T13:34:42Z

python-threatexchange/threatexchange/exchanges/impl/ncmec_api.py

@@ -39,14 +39,16 @@ class NCMECCheckpoint(

    # The biggest value of "to", and the next "from"
    get_entries_max_ts: int
+    next_fetch: t.Optional[str] = ""


blocking: Hmm, not sold on this name. NCMEC calls these "paging urls" so maybe paging_url?
blocking; Add a comment explaining what this variable represents.

nit: Since the default value is empty string, suggest making this non-Optional and just using that.

Dcallies · 2025-01-13T13:36:50Z

python-threatexchange/threatexchange/exchanges/impl/ncmec_api.py

+
+                updates.extend(entry.updates)
+
+                if i % 100 == 0:


blocking: by change this from elif to if, I think it will now print the large update warning every update, which is incorrect, no?

Dcallies · 2025-01-13T13:40:09Z

python-threatexchange/threatexchange/exchanges/impl/ncmec_api.py


    def get_progress_timestamp(self) -> t.Optional[int]:
        return self.get_entries_max_ts

    @classmethod
    def from_ncmec_fetch(cls, response: api.GetEntriesResponse) -> "NCMECCheckpoint":
        """Synthesizes a checkpoint from the API response"""
-        return cls(response.max_timestamp)
+        return cls(response.max_timestamp, response.next, int(time.time()))


blocking: NCMEC's documentation asks us not to store these checkpoints long term, so let's respect that in this implementation.

One way we could handle this is not to directly access this variable, but wrap it with a helper function that will return empty string if it's too old.

def get_resume_url_if_recent(self): if time.time() - self.last_fetch_time < SOME_CONSTANT_MAYBE_12_HOURS: return self.next_fetch return ""

Dcallies · 2025-01-13T13:47:24Z

python-threatexchange/threatexchange/exchanges/impl/ncmec_api.py

@@ -39,14 +39,16 @@ class NCMECCheckpoint(

    # The biggest value of "to", and the next "from"
    get_entries_max_ts: int
+    next_fetch: t.Optional[str] = ""
+    last_fetch_time: t.Optional[int] = 0


blocking: Add a comment on the relationship between the paging URL for why we save this.

ignorable: We could also make this variable explicitly mention the paging_url and not set it if we are not paging

Dcallies · 2025-01-13T13:53:42Z

python-threatexchange/threatexchange/exchanges/tests/test_state_compatibility.py

+    @dataclass
+    class NCMECCheckpointWithoutNext(FetchCheckpointBase):
+        """
+        0.99.x => 1.0.0


blocking: This version isn't quite correct, the current version is 1.2.3

Dcallies · 2025-01-13T13:56:25Z

python-threatexchange/threatexchange/exchanges/clients/ncmec/hash_api.py

@@ -565,7 +565,7 @@ def get_entries(
            )

    def get_entries_iter(
-        self, *, start_timestamp: int = 0, end_timestamp: int = 0
+        self, *, start_timestamp: int = 0, end_timestamp: int = 0, next_: str = ""


blocking: next_ is a bit cryptic, perhaps we can do checkpointed_paging_url and then do

next_ = checkpointed_paging_url

Dcallies · 2025-01-14T00:41:36Z

python-threatexchange/threatexchange/exchanges/impl/ncmec_api.py

-                    log(f"large fetch ({i}), up to {len(updates)}")
-                updates.extend(entry.updates)
+                    # so store the checkpoint occasionally
+                    log(f"large fetch ({i}), up to {len(updates)}. storing checkpoint")


nit: You don't actually store the checkpoint by yielding, technically the caller can decide whether to keep calling or store.

Dcallies · 2025-01-14T00:44:58Z

python-threatexchange/threatexchange/exchanges/impl/ncmec_api.py

-                    start_timestamp=current_start, end_timestamp=current_end
+                    start_timestamp=current_start,
+                    end_timestamp=current_end,
+                    next_=current_next_fetch,


blocking: Danger! It's actually very easy to mess up this argument and accidentally trigger and endless loop. It may be that you have done so in the current code, but it's hard to tell.

The only time current_next_fetch should be populated is when you are resuming from checkpoint, and you need to explicitly disable the overfetch check (L290) then.

There might be a refactoring of this code that makes this easier, or now that we are switching over to the next pointer version we can get rid of the probing behavior, which simplifies the implementation quite a bit.

Dcallies · 2025-01-14T00:47:13Z

python-threatexchange/threatexchange/exchanges/impl/ncmec_api.py

-                    start_timestamp=current_start, end_timestamp=current_end
+                    start_timestamp=current_start,
+                    end_timestamp=current_end,
+                    next_=current_next_fetch,
                )
            ):
                if i == 0:  # First batch, check for overfetch


As a comment, it turns out my implementation for estimation of the entries in range was completely off, and so this is basically always overly cautious. Not sure what to do about it, since the alternatives that I can think of are complicated.

Dcallies · 2025-01-14T00:50:10Z

python-threatexchange/threatexchange/exchanges/impl/ncmec_api.py

@@ -240,15 +242,18 @@ def fetch_iter(
               the cursor
        """
        start_time = 0
+        next_fetch = ""


nit: just call this variable current_next_fetch - it's unconditionally written at 256.

Dcallies · 2025-01-14T00:53:07Z

python-threatexchange/threatexchange/exchanges/impl/ncmec_api.py

+                            last_fetch_time=int(time.time()),
+                        ),
+                    )
+                    current_next_fetch = entry.next


nit: This value is never read, as it always unconditionally replaced on L359

store checkpoint occasionally when start, end diff is one second

fed5dec

prenner requested a review from Dcallies as a code owner January 9, 2025 16:07

facebook-github-bot added the CLA Signed label Jan 9, 2025

prenner commented Jan 9, 2025

View reviewed changes

lint

4f12e50

prenner force-pushed the prenner/checkpoint-ncmec branch from 928ddce to 4f12e50 Compare January 9, 2025 16:14

prenner changed the title ~~ncmec: store checkpoint occasionally when start, end diff is one second~~ [WIP] ncmec: store checkpoint occasionally when start, end diff is one second Jan 9, 2025

prenner force-pushed the prenner/checkpoint-ncmec branch 4 times, most recently from 2965a46 to 5270515 Compare January 9, 2025 17:12

make new fields optional

d7f207e

prenner force-pushed the prenner/checkpoint-ncmec branch from 5270515 to d7f207e Compare January 9, 2025 17:54

ensure last updates are saved

c9376ad

Dcallies requested changes Jan 13, 2025

View reviewed changes

Dcallies reviewed Jan 14, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] ncmec: store checkpoint occasionally when start, end diff is one second #1731

[WIP] ncmec: store checkpoint occasionally when start, end diff is one second #1731

prenner commented Jan 9, 2025

prenner Jan 9, 2025

Dcallies left a comment

Dcallies Jan 13, 2025

Dcallies Jan 13, 2025

Dcallies Jan 13, 2025

Dcallies Jan 13, 2025

Dcallies Jan 13, 2025

Dcallies Jan 13, 2025

Dcallies Jan 13, 2025

Dcallies Jan 14, 2025

Dcallies Jan 14, 2025

Dcallies Jan 14, 2025

Dcallies Jan 14, 2025

Dcallies Jan 14, 2025

[WIP] ncmec: store checkpoint occasionally when start, end diff is one second #1731

Are you sure you want to change the base?

[WIP] ncmec: store checkpoint occasionally when start, end diff is one second #1731

Conversation

prenner commented Jan 9, 2025

Summary

Test Plan

Choose a reason for hiding this comment

Dcallies left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment