Skip to content

fix(remote): skip records a hosted import finds already held - #184

Merged
YellowSnnowmann merged 1 commit into
tinyhumansai:mainfrom
YellowSnnowmann:fix/hosted-import-skips-unchanged
Oct 1, 2026
Merged

YellowSnnowmann merged 1 commit into
tinyhumansai:mainfrom
YellowSnnowmann:fix/hosted-import-skips-unchanged

Conversation

@YellowSnnowmann

Copy link
Copy Markdown
Contributor

Summary

A hosted import appended every record, and on this wire an append is a new version even when the record is already there. A copy run again therefore wrote every record a second time.

On staging, re-running Copy & switch:

  • took about nine minutes;
  • billed every write again;
  • left 600 events behind 300 Gmail keys, and doubled every other namespace too.

The migrate driver already promises the opposite: "rerunning is safe because targets skip records they recognise".

Now, before a batch first writes to a namespace, it reads what the namespace holds once, using the same fold export uses.

  • A record held with the same content, category, session and taint is counted in ImportOutcome::skipped and not written.
  • Any difference is written as before.
  • The batch counts its own writes as held, so a record repeated in one batch is written once.
  • The read rides out a busy backend with the same pauses as a write (import_patience).

Related issue

Part of tinyhumansai/openhuman#6718 (hosted CortexDB as a memory engine), found in end-to-end testing on staging.

API or behavior changes

No public API change.

Behavior, hosted wire only:

  • import_records now fills skipped for records the target already holds unchanged, instead of writing them again. A re-run costs a few listings instead of one write per record.
  • A first copy pays one extra listing per namespace in each batch. On staging, a namespace never written lists as an empty 200.
  • A listing failure fails the batch, as a failed visibility wait already did.

Existing duplicates are not removed by this change; it only stops new ones.

Validation

Commands actually run, with their outcome:

  • cargo fmt --all -- --check: clean
  • cargo clippy --all-targets --all-features -- -D warnings: clean
  • cargo build --all-targets --all-features: ok
  • cargo test --all-features: 2394 passed, 0 failed

Tests

New tests:

  • a_hosted_import_run_again_writes_only_what_changed:
    • The same five records imported again: 0 written, 5 skipped.
    • Then one changed in each field (content, custom category, session, taint) plus one new: 5 written, 1 skipped.
    • An unchanged custom category is skipped, so categories round-trip.
  • a_record_repeated_in_one_batch_is_written_once.
  • a_hosted_import_rides_out_a_rate_limited_read: one pause fails the batch before any write; two pauses ride out seven 429s.

Changed: a_hosted_import_waits_once_per_scope_and_never_probes_recall now expects two listings, one read and one visibility wait. It still expects no recall.

Mutation-checked. Each of these fails at least one test:

  • never skipping;
  • forgetting the batch's own writes;
  • ignoring taint, category or session in the comparison;
  • an impatient read.

Documentation

The module docs and hosted_import_records docs describe the skip and its cost.

Checklist

  • The change is focused on one logical change
  • No new #[allow(...)], #[ignore], or relaxed lints
  • No secrets, tokens, or .env contents in the diff or the description

A hosted import appended every record, and an append is a new version even
when the record is already there. A copy run again therefore wrote every
record twice: on staging a re-run of Copy & switch took about nine minutes
and left 600 events behind 300 Gmail keys. The migrate driver documents the
opposite ("rerunning is safe because targets skip records they recognise").

Before a batch first writes to a namespace it now reads what the namespace
holds, once, with the fold export reads. A record held with the same
content, category, session and taint is counted as skipped and not written;
any difference is written as before. The batch counts what it wrote as held,
so a record repeated in one batch is written once. The read rides out a
busy backend with the same pauses as a write. A first copy pays one listing
per namespace in each batch, which answers empty.
@tinysweeper

tinysweeper Bot commented Oct 1, 2026 •

Copy link
Copy Markdown

Tiny Sweeper review

Tiny Sweeper reviewed this change across 6 lane(s) and found 1 active actionable finding(s). Detailed lane evidence and any incomplete work are listed below.

State: Incomplete
Priority: medium
Reviewed head: 76613d6b7d9b
Updated: 1790861209 (Unix time)

Review snapshot

Change surface Files Review signal Count
Production 1 Active findings 1
Tests 1 Noted findings 0
Documentation 0 Resolved findings 0
Configuration 0 Pending checks/questions 6

Completeness: Incomplete
Test assessment: No supported feature-to-test mapping was available; this does not mean tests are absent or passed.

What changed

The review could not produce a supported behavioral summary; inspect the cited changed surface and lane details below.

Features

None identified with supported citations.

Tests

No supported feature-to-test mapping was produced. Test execution is not inferred.

Findings

  • medium · e2e · Add an end-to-end test for the skip-on-unchanged import behaviour — The change introduces a new behaviour: `import_records` reads what the namespace holds before writing and skips records whose content, category, session and taint are unchanged. Th (crates/tinymemory\-remote/src/cortex\_provider/portability\.rs:137)

Pending checks: AgentMemory E2E, CortexDB full simulation

Could not review: crates/tinymemory-remote/src/cortex_provider/portability.rs, crates/tinymemory-remote/src/hosted_test.rs

Before merge

  • Complete the critique review for crates/tinymemory-remote/src/cortex_provider/portability.rs, crates/tinymemory-remote/src/hosted_test.rs.
  • Complete the security review for crates/tinymemory-remote/src/cortex_provider/portability.rs, crates/tinymemory-remote/src/hosted_test.rs.
  • Wait for AgentMemory E2E, CortexDB full simulation.

How this fits together

flowchart LR
  n0["...ts_once_per_scope_and_never_probes_recall<br/>changed"]:::changed
  n1["...waits_out_a_slow_listing_and_a_rate_limit<br/>changed"]:::changed
  n2["provider"]:::impacted
  n3["lock"]:::impacted
  n4["iter"]:::impacted
  n5["assert"]:::impacted
  n6["..._maps_to_a_memory_path_and_never_a_v1_one"]:::impacted
  n7["...tch_falls_back_to_ordered_per_item_writes"]:::impacted
  n0 -->|calls| n2
  n0 -->|tests| n2
  n1 -->|calls| n2
  n1 -->|tests| n2
  n6 -->|calls| n2
  n6 -->|tests| n2
  n6 -->|calls| n3
  n6 -->|tests| n3
  n6 -->|calls| n4
  n6 -->|tests| n4
  n6 -->|calls| n5
  n7 -->|calls| n2
  n7 -->|tests| n2
  n7 -->|calls| n3
  n7 -->|tests| n3
  n7 -->|calls| n4
  n7 -->|tests| n4
  n7 -->|calls| n5
  classDef changed fill:#0d4429,stroke:#238636,color:#e6edf3
  classDef impacted fill:#161b22,stroke:#6e7681,color:#c9d1d9
  classDef flagged fill:#5a1e02,stroke:#d93f0b,color:#ffffff
  classDef blocking fill:#67060c,stroke:#f85149,color:#ffffff
Loading
Agent review details

critique

  • Conclusion: Neutral
  • Scope reviewed: incomplete; unanswered: crates/tinymemory-remote/src/cortex_provider/portability.rs, crates/tinymemory-remote/src/hosted_test.rs
  • Lane summary: Reviewed 0 files; 0 findings. 2 files could not be reviewed: crates/tinymemory-remote/src/cortex_provider/portability.rs, crates/tinymemory-remote/src/hosted_test.rs.

security

  • Conclusion: Neutral
  • Scope reviewed: incomplete; unanswered: crates/tinymemory-remote/src/cortex_provider/portability.rs, crates/tinymemory-remote/src/hosted_test.rs
  • Lane summary: Reviewed 0 files; 0 findings. 2 files could not be reviewed: crates/tinymemory-remote/src/cortex_provider/portability.rs, crates/tinymemory-remote/src/hosted_test.rs.

tests

  • Conclusion: Success
  • Scope reviewed: all assigned evidence
  • Lane summary: Adds deduplication to the hosted `import_records`, skipping records whose content, category, session, and taint match what the namespace already holds. The implementation is sound, error handling follows existing patterns, and the new tests cover idempotent re-import, partial changes, same-batch duplication, and rate-limited reads. No findings. _The code index is behind this pull request (indexed at `1c638576608f`), so retrieved context may be out of date._ _Memory was unavailable (model: cortex: v1/recall: timed out after 10s), so this review ran without it._

commits

  • Conclusion: Neutral
  • Scope reviewed: all assigned evidence
  • Lane summary: Nothing sensitive found in what this pull request commits.

description

  • Conclusion: Success
  • Scope reviewed: all assigned evidence
  • Lane summary: This change implements read-before-write skip logic for hosted imports, preventing duplicate writes by comparing content, category, session, and taint. The code is idiomatic, well-documented, and fully tested. All rules are observed; no defects are introduced. _The code index is behind this pull request (indexed at `1c638576608f`), so retrieved context may be out of date._ _Memory was unavailable (model: cortex: v1/recall: timed out after 10s), so this review ran without it._

e2e

  • Conclusion: Neutral
  • Scope reviewed: all assigned evidence
  • Lane summary: The import_records change adds idempotent-skip logic and a per-namespace listing before writes. This is covered by unit tests but not by any end-to-end test that drives the memory service as a client would, making the behavioural change e2e-uncovered. Waiting on end-to-end jobs: `AgentMemory E2E`, `CortexDB full simulation`.
  • Unresolved questions/checks: AgentMemory E2E, CortexDB full simulation
  • Evidence: crates/tinymemory\-remote/src/cortex\_provider/portability\.rs — Add an end-to-end test for the skip-on-unchanged import behaviour
Evidence and run details
  • Models: ladder/vectors, deepseek/deepseek-v4-flash
  • Spend: $0.003147
  • Tokens: 48698 input · 7820 output · 2560 cached · 537 embedding
Head State Pass summary
76613d6b7d9b incomplete 1 active finding(s), 0 resolved finding(s) (at 1790861209)

tinysweeper 0.1.0

@coderabbitai

coderabbitai Bot commented Oct 1, 2026

Copy link
Copy Markdown

Important

  • 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 7bd25718-d12b-4d84-910f-f161403f028e

  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.


Comment @coderabbitai help to get the list of available commands.

@tinysweeper tinysweeper Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tinysweeper found nothing blocking, but could not review everything, so this is not an approval: crates/tinymemory-remote/src/cortex_provider/portability.rs, crates/tinymemory-remote/src/hosted_test.rs.

             $0.0031 · 48,698 in / 7,820 out · 2,560 cached (5%)  · ladder/vectors, deepseek/deepseek-v4-flash · 537 embedded
tests:       $0.0008 · 15,829 in / 2,033 out · 0 cached (0%)      · deepseek/deepseek-v4-flash
description: $0.0004 · 7,634 in  / 1,340 out · 2,048 cached (27%) · deepseek/deepseek-v4-flash
e2e:         $0.0009 · 19,583 in / 1,517 out · 0 cached (0%)      · deepseek/deepseek-v4-flash

/// A record the backend refuses (a 400-class answer, or a namespace that
/// cannot be a hosted scope) is counted in [`ImportOutcome::failed`] with a
/// reason that names the record and the backend's code, never its content.
/// A record the namespace already holds with the same content, category,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

priority medium e2e confident

Add an end-to-end test for the skip-on-unchanged import behaviour

The change introduces a new behaviour: import_records reads what the namespace holds before writing and skips records whose content, category, session and taint are unchanged. This is a meaningful behavioural change with external surface (the public import_records API). It is covered only by unit tests in hosted_test.rs. No end-to-end test in the repository (module_e2e.rs, server_e2e.rs, integration scripts) exercises this skip path. The conformance suite's assert_export_import_round_trip does not test re-importing identical records. Add an end-to-end test that drives the memory provider as a client and verifies that re-importing the same records produces skipped counts and that changes to any of the four fields force a write.

[RULE] e2e-uncovered ·

@tinysweeper tinysweeper Bot added the priority: p2 Soon. Real but survivable — a rough edge, a gap, a thing that will bite later. label Oct 1, 2026
@YellowSnnowmann
YellowSnnowmann merged commit 0ccb96d into tinyhumansai:main Oct 1, 2026
30 checks passed
YellowSnnowmann added a commit to YellowSnnowmann/openhuman that referenced this pull request Oct 1, 2026
v1.22.2 carries two hosted fixes found testing on staging:
tinyhumansai/tinymemory#183 recalls each kind of synced source on its own,
so a search across synced content follows the question, and
tinyhumansai/tinymemory#184 skips records a hosted import finds already
held, so Copy & switch run again writes only what changed.

Registry record (version, release URL, the 11 digests from the release's
checksum.toml), ARTIFACT_CAPABILITIES_PIN and its re-read note, the 6 CI
pins, the vendor gitlink on the tag, and both lockfiles. The capability
files are unchanged since v1.21.1, so the advertised families stay the
same.
YellowSnnowmann added a commit to tinyhumansai/openhuman that referenced this pull request Oct 1, 2026
v1.22.2 carries two hosted fixes found testing on staging:
tinyhumansai/tinymemory#183 recalls each kind of synced source on its own,
so a search across synced content follows the question, and
tinyhumansai/tinymemory#184 skips records a hosted import finds already
held, so Copy & switch run again writes only what changed.

Registry record (version, release URL, the 11 digests from the release's
checksum.toml), ARTIFACT_CAPABILITIES_PIN and its re-read note, the 6 CI
pins, the vendor gitlink on the tag, and both lockfiles. The capability
files are unchanged since v1.21.1, so the advertised families stay the
same.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

priority: p2 Soon. Real but survivable — a rough edge, a gap, a thing that will bite later.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant