#18 follow-ups: sync guard, live pending count, one status vocabulary, wire pin, tested feature configs - #61
Conversation
run_incremental_sync loads a connection's SyncState once, mutates it in memory for the whole run and saves at the end; the Slack search backfill does the same over the same (slack, connection_id) record. Nothing serialised those runs, so the periodic loop, the sync RPC and a trigger could sync one connection at once and whichever saved last won — losing either the dedup set (re-fetch, re-spend) or the daily budget count (overspend past the cap). One async guard per (toolkit, connection_id), taken in the host runners that every Composio run funnels through. The key normalises the toolkit exactly as the pipeline gate does, so a padded or mixed-case toolkit names one connection rather than two, and the Slack backfill contends with the Slack sync it shares state with. The guard is non-blocking: a second run returns SYNC_ALREADY_RUNNING rather than queueing. Queueing would stall the periodic loop's whole tick behind a long manual sync and then run a redundant sync of a connection just synced, which is the spend the guard exists to avoid.
The engine-free tree reconnect claimed to mirror the engine adapter's
and did not. It wrote source_id `composio:{toolkit}:{connection}:{doc}`
and passed no path_scope, where the adapter writes
`{toolkit}:{connection}:{doc}` scoped `{toolkit}:{connection}`.
A chunk seals under its path_scope, falling back to its source_id, and
retrieval selects source trees by that scope and classifies them by
platform prefix — gmail is email, slack is chat. Without a path_scope
every synced item became its own single-item tree named
`composio:gmail:conn:msg-7`, which matches no platform. Items were
stored and then unreachable: the #5473 defect the reconnect exists to
fix, reintroduced when the pipelines moved off the engine. The same
scheme keys the `LIKE '{toolkit}:%'` prefix the memory-source status
and diff snapshots query by, so both read zero for Composio sources.
Restores the adapter's scheme, owner and provider, and its skip for an
item with no toolkit or connection to scope by. The adapter has a test
asserting this addressing; the host that replaced it on the live path
had none, and only counted rows — so the drift passed. It has one now.
Per-source status counted pending as `embedding IS NULL` over mem_tree_chunks. That column is not in the table's schema — an idempotent migration adds it and nothing writes it — so every chunk read as pending and every healthy source reported chunks_pending equal to chunks_synced, showing eternal work in flight in the memory-sources UI. Embeddings live in the mem_tree_chunk_embeddings sidecar. A chunk without one is not necessarily pending either: the lifecycle may have dropped it, or it may be recorded in mem_tree_chunk_reembed_skipped. Both are terminal. This is the engine's own predicate from list_sync_statuses, kept identical so the per-source view and the per-provider one cannot disagree about the same chunk. The test asserts the legacy column is still NULL for all four fixtures before asserting the count, so it fails if the predicate ever silently collapses back onto the dead column.
Freshness was declared twice in this crate — same variants, same
snake_case wire strings, same thresholds — with nothing checking the
copies agreed. sources::status now re-exports sync::sync_status's, which
keeps its own path working for callers that name it. The engine holds a
third copy; that one is vendored and stays where it is.
The chunk source-id prefix was likewise defined twice, in sources::status
and in diff::source, the second commented as mirroring the first. Both
matched a Composio source on its toolkit alone, so two connections of one
toolkit each counted the other's chunks as their own. One definition now,
narrowed to {toolkit}:{connection_id}, with the toolkit-only form left as
the degradation for a row that somehow has no connection id.
The diff adapter's three prefix tests moved to the definition; what is
left there asserts the adapter resolves through it, which is the property
that keeps a snapshot and a status agreeing.
sources/src/types.rs and the engine's memory/sources/types.rs are two copies of one contract, joined by a live wire: core's engine seam converts between them with serde_json::to_value/from_value for the tree-coupled source kinds, in both directions. Nothing but the serialised shape holds that seam together — the copies are distinct Rust types in distinct crates and neither compiles against the other. So a renamed field or a new SourceKind variant on either side is not a compile error. It surfaces at runtime on the first external-source sync after the engine pin moves, at the point of conversion, far from the edit that caused it. Every other deliberate twin in this arc has a pin test; this one had none. Pins MemorySourceEntry populated and empty (the second catches a skip_serializing_if dropped from one copy), plus SourceItem and SourceContent, which cross the same seam on the list_items and read_item directions.
Issue tinyhumansai#18 §E2 asks for build **and test** of nine configurations. CI ran cargo test for --all-features and the default set, plus a feature powerset pass that is a check. A check answers whether a combination compiles, which is a different question from whether it behaves: a regression that only shows at runtime under --features mem0 alone merged green. Adds a matrix that tests each engine configuration of the facade on its own, and fail-fast is off because knowing three configurations broke is worth more than stopping at the first. Two of §E2's nine name features the facade does not have. contacts belongs to tinymemory-core and is checked alongside the powerset — its only behaviour is a macOS reader compiled out everywhere else, so testing it on ubuntu would exercise the empty stub, and a macos runner is deliberately not spent. sync-composio names a feature that exists nowhere in the workspace: the Composio sync is unconditional in tinymemory-core, so there is nothing to select. Both are written down in the workflow, because a missing row in a matrix reads as covered.
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
How this change flows4 changed behaviours across 19 relationships. 6 surrounding behaviours are shown (60 graph nodes walked). 42 further behaviours left out to keep the diagram readable. flowchart LR
n0["source_id_prefix<br/>changed"]:::changed
n1["source_status<br/>changed"]:::changed
n2["status_list<br/>changed"]:::changed
n3["run_composio_connection_with_caps<br/>changed"]:::changed
n4["a_held_connection_short_circuits_the_run"]:::impacted
n5["new"]:::impacted
n6["join"]:::impacted
n7["run_pipeline"]:::impacted
n8["...lved_chunks_not_the_dead_embedding_column"]:::impacted
n9["...document_is_keyed_by_its_connection_scope"]:::impacted
n1 -->|calls| n0
n2 -->|calls| n1
n3 -->|calls| n5
n3 -->|calls| n7
n4 -->|calls| n5
n4 -->|tests| n5
n4 -->|calls| n6
n4 -->|tests| n6
n4 -->|calls| n7
n4 -->|tests| n7
n7 -->|calls| n5
n8 -->|calls| n1
n8 -->|tests| n1
n8 -->|calls| n6
n8 -->|tests| n6
n9 -->|calls| n5
n9 -->|tests| n5
n9 -->|calls| n6
n9 -->|tests| n6
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
Green: changed behaviour. Grey: surrounding behaviour. Arrows name the call, use, implementation, or test relationship. Orange: has findings. Red: has a finding that blocks the merge. |
The Docs job builds rustdoc with -D warnings, and diff::source is a public module, so its module documentation linking to source_id_prefix — which is pub(crate) — is a private_intra_doc_links error rather than a warning. Names it in a code span instead, and says why, so the next person does not reintroduce the link. The other two links added on this branch point at public modules and are fine. Caught by CI: the local run that would have caught it, cargo doc --no-deps --all-features, was the one check skipped for disk space. Verified locally after the fix.
The promise made on tinyhumansai#61's two tinysweeper threads, kept after that PR merged so its new matrix jobs are covered too: a tag or branch is mutable, and whoever owns the action's repo can repoint it and run new code with this workflow's secrets. All 21 `uses:` refs now name a full commit SHA with the tag kept as a trailing comment for readability, and Dependabot gains the `github-actions` ecosystem so the pins move by reviewable PR rather than by rot.
Summary
Every remaining finding from #18's end-to-end review, in one branch: a per-connection guard so two Composio syncs cannot clobber each other's cursor and budget, per-source status read from the tables that actually hold embeddings, one
FreshnessLabeland one source-id prefix instead of two of each, a serde pin on thetinymemory-sourcestwin, and CI that runs each engine configuration rather than only compiling it.Verifying item 2 turned up a sixth defect the issue does not know about, and fixing it was a precondition for item 2 meaning anything — see below.
Related issue
Closes #59.
API or behavior changes
Behaviour,
tinymemory-core:note: Some(SYNC_ALREADY_RUNNING)andrecords_ingested: 0, instead of running concurrently. This applies to the Gmail and Slack backfills too — they key theirSyncStateon the same(toolkit, connection_id)as the periodic sync. An operator-initiated backfill fired during a periodic sync is now skipped rather than corrupting the shared state; the note says so, and a retry after the run succeeds.{toolkit}:{connection_id}:{document_id}with tree scope{toolkit}:{connection_id}, restoring the engine adapter's scheme (see the regression note). Items ingested under Sync moves onto the memory API, with its acceptance test (#18 §B1/§B2/§B5/§E4) #48'scomposio:*scheme are orphaned by this and re-ingest once under the correct id. Sync moves onto the memory API, with its acceptance test (#18 §B1/§B2/§B5/§E4) #48 merged the same morning, so the exposure is under a day of syncs; no migration ships here.chunks_pendingin the memory-sources status now reports real in-flight work rather than always equallingchunks_synced. Expect the numbers in that UI to move.Public API: additive —
sync::pipelines::host::SYNC_ALREADY_RUNNING.sources::status::FreshnessLabelis now a re-export ofsync::sync_status::FreshnessLabel; the path still resolves and the serde shape is unchanged. Nothing downstream names it today.The regression this uncovered
The issue states "None is a regression introduced by #18." One is —
29f7c30, inside #48.host.rs's tree reconnect claims in its own comment to mirror the engine adapter's. It does not:engine/sync.rs)host.rsbefore this PRsource_id{toolkit}:{conn}:{doc}composio:{toolkit}:{conn}:{doc}path_scopeSome("{toolkit}:{conn}")owner{toolkit}-sync:{conn}""providercomposio:{toolkit}{toolkit}Verified in the engine:
retrieval/cover.rsderives a chunk's tree scope aspath_scope.unwrap_or(source_id), andretrieval/source.rs'sPLATFORM_KINDSclassifies a scope by its platform prefix —gmail:is email,slack:is chat. With nopath_scope, every synced item became its own single-item tree undercomposio:gmail:conn:msg-7, a prefix matching no platform. Items were stored and then unreachable: precisely the #5473 defect the reconnect exists to fix.It also broke both
source_id LIKE '{toolkit}:%'prefixes, so Composiochunks_syncedread zero as well — fixing item 2's pending count alone would have left the UI at 0/0.#48's own acceptance test asserts only that rows exist, not their addressing, which is why the drift passed. The engine adapter has a test pinning this scheme; the host that replaced it on the live path had none. It has one now.
Validation
Commands actually run, with their outcome:
cargo fmt --all -- --check— passcargo clippy --all-targets --all-features -- -D warnings— passcargo build --all-targets --all-features— passcargo test --all-features— pass, 23 suites, 0 failuresEvery row of the new
feature-configsmatrix was also run locally and passes:--no-default-features,tinycortex,tinycortex,memory-git,mem0,supermemory,cognee,--all-features.Tests
path_scopeandownerasserted rather than a row count, so this drift cannot recur silently. An item with a blank toolkit or connection skips the tree and still lands in the skill store.4 synced / 1 pending. The test first asserts the legacyembeddingcolumn is still NULL for all four, so it fails rather than passes vacuously if the predicate ever collapses back onto the dead column. A source with no chunks reports zeroes rather than tripping onSUMover no rows.MemorySourceEntrypopulated and empty (the second catches askip_serializing_ifdropped from one copy), plusSourceItemandSourceContent, which cross the same seam.Deliberately untested: the
contactsfeature is checked, not executed. Its only behaviour is a macOS CNContactStore reader whose dependencies sit behind acfg(target_os = "macos")table, so on ubuntu there is nothing to run but the empty stub; executing it needs amacos-latestrunner, deliberately not spent. The powerset job already enumerates the feature as a subset of size one.Documentation
Module and item docs updated in the same commits as the behaviour:
sync/pipelines/host.rs— why the addressing scheme is a contract rather than an implementation detail, and what passing nopath_scopeactually does.sources/status.rs— a "Where pending lives" section:mem_tree_chunks.embeddingis a migration artefact nothing writes, and resolution has three terminal forms.diff/source.rs— the prefix map now resolves through the one shared definition..github/workflows/ci.yml— what the matrix adds over the powerset pass (cargo checknever links and never runs a binary), and that §E2'ssync-composionames a feature existing nowhere in the workspace.Notes on the issue's items
core::sync::sync_status" is done forFreshnessLabel, which is what item 4 names.SourceStatusstays where it is: it is per-source, whileMemorySyncStatusis per-provider and carriesbatch_total/batch_processed. Merging them would mean inventing a shape neither caller wants. The engine's thirdFreshnessLabelis vendored and stays.contactsbelongs totinymemory-core;sync-composioexists nowhere in the workspace, because the Composio sync is unconditional there. Both are written into the workflow rather than quietly dropped, since a missing row in a matrix reads as covered.Worth stating plainly about the matrix: the facade's 54 tests are the same in every configuration, because
DriverRegistryadmission is a static policy table rather than a function of which adapters were compiled in. These rows earn their minutes by linking and running — whichcargo checknever does, and where feature-unification bites, as the rootCargo.toml's ownlinks = "git2"warning records.Checklist
#[allow(...)],#[ignore], or relaxed lints.envcontents in the diff or the description🤖 Generated with Claude Code